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. > >> 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... >
