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

[Pierre Morel <pmorel@linux.ibm.com>]

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

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


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