Messages in this thread | | | Subject | Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem | From | Tony Krowiak <> | Date | Thu, 14 Feb 2019 12:12:31 -0500 |
| |
On 2/14/19 10:05 AM, Christian Borntraeger wrote: > On 14.02.2019 15:54, Cornelia Huck wrote: >> On Thu, 14 Feb 2019 14:51:01 +0100 >> Pierre Morel <pmorel@linux.ibm.com> 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.
I agree wholeheartedly with this statement. It has nothing to do with interrupt processing.
>> >>> 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@linux.ibm.com> >>> --- >>> 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... >> >
| |