On Fri, 13 Oct 2017 13:38:51 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> 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 <akrowiak@xxxxxxxxxxxxxxxxxx>
> ---
> 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 <akrowiak@xxxxxxxxxxxxxxxxxx>
> + *
> + */
> +#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);
> + }
> +}
