Re: [qemu-s390x] [RFC 06/19] s390/zcrypt: register matrix device with VFIO mediated device framework

[Cornelia Huck]

On Fri, 13 Oct 2017 13:38:51 -0400
Tony Krowiak <address@hidden> wrote:

> Registers the matrix device created by the AP matrix bus with
> the VFIO mediated device framework. Registering the matrix
> device will create the sysfs structures needed to create
> mediated matrix devices that can be configured with a matrix
> of adapters, usage domains and control domains for a guest
> machine.
> 
> Registering the matrix device with the VFIO mediated device
> framework will create the following sysfs structures:
> 
> /sys/devices/ap_matrix
> ... [matrix]
> ...... [mdev_supported_types]
> ......... [ap_matrix-passthrough]
> ............ available_instances
> ............ create
> ............ device_api
> ............ [devices]
> ............ name
> 
> To create a mediated device for the AP matrix device, write a UUID
> to the create file:
> 
>     uuidgen > create
> 
> A symbolic link to the mediated device's directory will be created in the
> devices subdirectory named after the generated $uuid:
> 
> /sys/devices/ap_matrix
> ... [matrix]
> ...... [mdev_supported_types]
> ......... [ap_matrix-passthrough]
> ............ [devices]
> ............... [$uuid]
> 
> Signed-off-by: Tony Krowiak <address@hidden>
> ---
>  MAINTAINERS                                  |    1 +
>  drivers/s390/crypto/Makefile                 |    2 +-
>  drivers/s390/crypto/ap_matrix_bus.c          |    1 +
>  drivers/s390/crypto/ap_matrix_bus.h          |    4 +
>  drivers/s390/crypto/vfio_ap_matrix_drv.c     |    5 +
>  drivers/s390/crypto/vfio_ap_matrix_ops.c     |  172 
> ++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_matrix_private.h |    3 +
>  include/uapi/linux/vfio.h                    |    1 +
>  8 files changed, 188 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/s390/crypto/vfio_ap_matrix_ops.c
> 

> diff --git a/drivers/s390/crypto/ap_matrix_bus.c 
> b/drivers/s390/crypto/ap_matrix_bus.c
> index 66bfa54..418c23b 100644
> --- a/drivers/s390/crypto/ap_matrix_bus.c
> +++ b/drivers/s390/crypto/ap_matrix_bus.c
> @@ -61,6 +61,7 @@ static int ap_matrix_dev_create(void)
>       matrix->device.bus = &ap_matrix_bus_type;
>       matrix->device.parent = ap_matrix_root_device;
>       matrix->device.release = ap_matrix_dev_release;
> +     INIT_LIST_HEAD(&matrix->queues);

Don't you also need to init the spin lock?

>  
>       ret = device_register(&matrix->device);
>       if (ret) {
> diff --git a/drivers/s390/crypto/ap_matrix_bus.h 
> b/drivers/s390/crypto/ap_matrix_bus.h
> index c2aff23..3eccc36 100644
> --- a/drivers/s390/crypto/ap_matrix_bus.h
> +++ b/drivers/s390/crypto/ap_matrix_bus.h
> @@ -12,8 +12,12 @@
>  
>  #include <linux/device.h>
>  
> +#include "ap_bus.h"

Why do you need this include now? (IOW, does that need to go into
another patch?)

> +
>  struct ap_matrix {
>       struct device device;
> +     spinlock_t qlock;
> +     struct list_head queues;
>  };
>  
>  struct ap_matrix *ap_matrix_get_device(void);

> diff --git a/drivers/s390/crypto/vfio_ap_matrix_ops.c 
> b/drivers/s390/crypto/vfio_ap_matrix_ops.c
> new file mode 100644
> index 0000000..7d01f18
> --- /dev/null
> +++ b/drivers/s390/crypto/vfio_ap_matrix_ops.c
> @@ -0,0 +1,172 @@
> +/*
> + * Adjunct processor matrix VFIO device driver callbacks.
> + *
> + * Copyright IBM Corp. 2017
> + * Author(s): Tony Krowiak <address@hidden>
> + *
> + */
> +#include <asm/ap-config.h>
> +#include <linux/string.h>
> +#include <linux/vfio.h>
> +#include <linux/mdev.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/ctype.h>
> +
> +#include "vfio_ap_matrix_private.h"
> +
> +#define AP_MATRIX_MDEV_TYPE_HWVIRT "passthrough"
> +#define AP_MATRIX_MDEV_NAME_HWVIRT "AP Matrix Passthrough Device"
> +
> +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES    255
> +
> +struct ap_matrix_mdev {
> +     struct mdev_device *mdev;
> +     struct list_head node;
> +};
> +
> +struct ap_matrix     *matrix;
> +struct mutex         mdev_devices_lock;
> +struct list_head     mdev_devices;
> +int                  available_instances;

Having this as a global variable feels wrong. I see that vfio-ccw keeps
things like this in driver data attached to its parent. Can you do
something like that as well?

> +
> +static struct ap_matrix_mdev *ap_matrix_mdev_find_by_uuid(uuid_le uuid)
> +{
> +     struct ap_matrix_mdev *matrix_mdev;
> +
> +     list_for_each_entry(matrix_mdev, &mdev_devices, node) {
> +             if (uuid_le_cmp(mdev_uuid(matrix_mdev->mdev), uuid) == 0)
> +                     return matrix_mdev;
> +     }
> +
> +     return NULL;
> +}

(...)

> +int ap_matrix_mdev_register(struct ap_matrix *ap_matrix)
> +{
> +     int ret;
> +     struct device *dev = &ap_matrix->device;
> +
> +     ret = mdev_register_device(dev, &vfio_ap_matrix_ops);
> +     if (ret)
> +             return ret;
> +
> +     matrix = ap_matrix;

I'd just grab the (one) matrix device here...

> +     mutex_init(&mdev_devices_lock);
> +     INIT_LIST_HEAD(&mdev_devices);
> +     available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES;
> +
> +     return 0;
> +}
> +
> +void ap_matrix_mdev_unregister(struct ap_matrix *ap_matrix)
> +{
> +     struct device *dev;
> +
> +     if (ap_matrix == matrix) {

...and lose this extra if.

> +             dev = &matrix->device;
> +             available_instances--;
> +             mdev_unregister_device(dev);
> +     }
> +}