Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 14/02/2019 16:05, Christian Borntraeger wrote:
On 14.02.2019 15:54, Cornelia Huck wrote:
On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

Pierre,
this is independent from this series and should have been sent separately.
In the end (when we have the final solution) this will require cc stable.

Yes, I will wait until tomorrow for more feed back and I will send a separate v2 for this patch.

Regards,
Pierre


Libudev relies on having a subsystem link for non-root devices. To
avoid libudev (and potentially other userspace tools) choking on the
matrix device let us introduce a vfio_ap bus and with that the vfio_ap
bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that
basically does nothing...

I have seen libvirt looping when a matrix device was available before the
libvirt start.
Marc Hartmayer debugged this and circumvented this in libvirt:
https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html

Still libudev expects a subsystem link in the matrix folder when doing the
udev_enumerate_scan_devices call.

Having a bus is one way of adding a subsystem link.



We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to
the vfio_ap driver and we suppress the device_type we do not need
anymore.

Since the associated matrix driver is not the vfio_ap driver any more,
we have to change the search for the devices on the vfio_ap driver in
the function vfio_ap_verify_queue_reserved.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 31c6c84..1fd5fe6 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
static struct ap_driver vfio_ap_drv; -static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;

This counter basically ensures that at most one device may bind with
this driver... you'd still have that device on the bus, though.

  };
struct ap_matrix_dev *matrix_dev;
@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
  	kfree(matrix_dev);
  }
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
  static int vfio_ap_matrix_dev_create(void)
  {
  	int ret;
@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
  	if (IS_ERR(root_device))
  		return PTR_ERR(root_device);
+ ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
  	if (!matrix_dev) {
  		ret = -ENOMEM;
@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
  	mutex_init(&matrix_dev->lock);
  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
- matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead
when you need it in the function below?

ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;
+ ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+

As you already have several structures that can be registered exactly
once (the root device, the bus, the driver, ...), you can already be
sure that there's only one device on the bus, can't you?

  	return 0;
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
  matrix_reg_err:
  	put_device(&matrix_dev->device);
  matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
  	root_device_unregister(root_device);
-
  	return ret;
  }
static void vfio_ap_matrix_dev_destroy(void)
  {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
  	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
  }
static int __init vfio_ap_init(void)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 272ef42..900b9cf 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
  	qres.apqi = apqi;
  	qres.reserved = false;
- ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
  	if (ret)
  		return ret;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5675492..76b7f98 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -40,6 +40,7 @@ struct ap_matrix_dev {
  	struct ap_config_info info;
  	struct list_head mdev_list;
  	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
  };
extern struct ap_matrix_dev *matrix_dev;

This feels like a lot of boilerplate code, just to create a bus that
basically doesn't do anything. I'm surprised that libudev can't deal
with bus-less devices properly...




--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux