Re: [PATCH v9 09/22] s390: vfio-ap: register matrix device with VFIO mdev framework

From: Halil Pasic
Date: Mon Sep 10 2018 - 17:59:56 EST



On 09/10/2018 03:38 PM, Tony Krowiak wrote:
On 09/06/2018 04:49 AM, Pierre Morel wrote:
On 13/08/2018 23:48, Tony Krowiak wrote:
From: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>

Registers the matrix device created by the VFIO AP device
driver with the VFIO mediated device framework.
Registering the matrix device will create the sysfs
structures needed to create mediated matrix devices
each of which will be used to configure the AP matrix
for a guest and connect it to the VFIO AP device driver.

Registering the matrix device with the VFIO mediated device
framework will create the following sysfs structures:

/sys/devices/vfio_ap/matrix/
...... [mdev_supported_types]
......... [vfio_ap-passthrough]
............ create

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/vfio_ap/matrix/
...... [mdev_supported_types]
......... [vfio_ap-passthrough]
............ [devices]
............... [$uuid]

A symbolic link to the mediated device will also be created
in the vfio_ap matrix's directory:

/sys/devices/vfio_ap/matrix/[$uuid]

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
Tested-by: Michael Mueller <mimu@xxxxxxxxxxxxx>
Tested-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
---
 MAINTAINERS | 1 +
 drivers/s390/crypto/Makefile | 2 +-
 drivers/s390/crypto/vfio_ap_drv.c | 23 ++++++
 drivers/s390/crypto/vfio_ap_ops.c | 124 +++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h | 45 ++++++++++++
 include/uapi/linux/vfio.h | 1 +
 6 files changed, 195 insertions(+), 1 deletions(-)
 create mode 100644 drivers/s390/crypto/vfio_ap_ops.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e84c559..f60dd56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12427,6 +12427,7 @@ W: http://www.ibm.com/developerworks/linux/linux390/
 S: Supported
 F: drivers/s390/crypto/vfio_ap_drv.c
 F: drivers/s390/crypto/vfio_ap_private.h
+F:ÂÂÂ drivers/s390/crypto/vfio_ap_ops.c

 S390 ZFCP DRIVER
 M: Steffen Maier <maier@xxxxxxxxxxxxx>
diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile
index 48e466e..8d36b05 100644
--- a/drivers/s390/crypto/Makefile
+++ b/drivers/s390/crypto/Makefile
@@ -17,5 +17,5 @@ pkey-objs := pkey_api.o
 obj-$(CONFIG_PKEY) += pkey.o

 # adjunct processor matrix
-vfio_ap-objs := vfio_ap_drv.o
+vfio_ap-objs := vfio_ap_drv.o vfio_ap_ops.o
 obj-$(CONFIG_VFIO_AP) += vfio_ap.o
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 5069580..fa04c5a 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -11,6 +11,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <asm/zcrypt.h>
 #include "vfio_ap_private.h"

 #define VFIO_AP_ROOT_NAME "vfio_ap"
@@ -65,6 +66,19 @@ static int vfio_ap_matrix_dev_init(void)
ÂÂÂÂÂÂÂÂÂ return ret;
ÂÂÂÂÂ }

+ÂÂÂ mutex_init(&matrix_dev.lock);
+ÂÂÂ INIT_LIST_HEAD(&matrix_dev.mdev_list);
+
+ÂÂÂ /* Test if PQAP(QCI) instruction is available */
+ÂÂÂ if (test_facility(12)) {
+ÂÂÂÂÂÂÂ ret = ap_qci(&matrix_dev.info);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ root_device_unregister(root_device);
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ }
+
+ÂÂÂ atomic_set(&matrix_dev.available_instances, MAX_ZDEV_ENTRIES_EXT);
ÂÂÂÂÂ matrix_dev.device.type = &vfio_ap_dev_type;
ÂÂÂÂÂ dev_set_name(&matrix_dev.device, "%s", VFIO_AP_DEV_NAME);
ÂÂÂÂÂ matrix_dev.device.type = &vfio_ap_dev_type;
@@ -105,11 +119,20 @@ int __init vfio_ap_init(void)
ÂÂÂÂÂÂÂÂÂ return ret;
ÂÂÂÂÂ }

+ÂÂÂ ret = vfio_ap_mdev_register();
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ ap_driver_unregister(&vfio_ap_drv);
+ÂÂÂÂÂÂÂ vfio_ap_matrix_dev_destroy();
+
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
ÂÂÂÂÂ return 0;
 }

 void __exit vfio_ap_exit(void)
 {
+ÂÂÂ vfio_ap_mdev_unregister();
ÂÂÂÂÂ ap_driver_unregister(&vfio_ap_drv);
ÂÂÂÂÂ vfio_ap_matrix_dev_destroy();
 }
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
new file mode 100644
index 0000000..8018c2d
--- /dev/null
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Adjunct processor matrix VFIO device driver callbacks.
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
+ *ÂÂÂÂÂÂÂÂÂ Halil Pasic <pasic@xxxxxxxxxxxxx>
+ *ÂÂÂÂÂÂÂÂÂ Pierre Morel <pmorel@xxxxxxxxxxxxx>
+ */
+#include <linux/string.h>
+#include <linux/vfio.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/ctype.h>
+
+#include "vfio_ap_private.h"
+
+#define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
+#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
+
+static void vfio_ap_matrix_init(struct ap_config_info *info,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ap_matrix *matrix)
+{
+ÂÂÂ matrix->apm_max = info->apxa ? info->Na : 63;
+ÂÂÂ matrix->aqm_max = info->apxa ? info->Nd : 15;
+ÂÂÂ matrix->adm_max = info->apxa ? info->Nd : 15;
+}
+
+static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+ÂÂÂ struct ap_matrix_mdev *matrix_mdev;
+
+ÂÂÂ matrix_mdev = kzalloc(sizeof(*matrix_mdev), GFP_KERNEL);
+ÂÂÂ if (!matrix_mdev)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ matrix_mdev->name = dev_name(mdev_dev(mdev));
+ÂÂÂ vfio_ap_matrix_init(&matrix_dev.info, &matrix_mdev->matrix);
+ÂÂÂ mdev_set_drvdata(mdev, matrix_mdev);
+
+ÂÂÂ if (atomic_dec_if_positive(&matrix_dev.available_instances) < 0) {
+ÂÂÂÂÂÂÂ kfree(matrix_mdev);
+ÂÂÂÂÂÂÂ return -EPERM;
+ÂÂÂ }
+
+ÂÂÂ mutex_lock(&matrix_dev.lock);
+ÂÂÂ list_add(&matrix_mdev->list, &matrix_dev.mdev_list);
+ÂÂÂ mutex_unlock(&matrix_dev.lock);
+
Hi Tony,

You need to initialize the matrix_mdev->list before
using it.

Why? Initialization only sets its prev and next pointers to point to itself. The
list_add immediately sets those pointers to insert it into the matrix_dev.mdev_list,
so what would be the purose?


I think the source of the confusion is that matrix_mdev->list a new element to be
added to the matrix_dev.mdev_list and not a sentinel node of some list (which in
a sense stands for the list as a whole).

Regards,
Halil