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

Commit Message

Pierre Morel Feb. 14, 2019, 1:51 p.m. UTC

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.

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 <[e-mail]>
---
 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(-)

Comments

Cornelia Huck Feb. 14, 2019, 2:54 p.m. UTC | #1

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <[e-mail]> wrote:

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

> 
> 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 <[e-mail]>
> ---
>  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...

Christian Borntraeger Feb. 14, 2019, 3:01 p.m. UTC | #2

On 14.02.2019 14:51, Pierre Morel wrote:
> 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.
> 
> 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 <[e-mail]>

Didnt you want to a add a reported-by?
> ---


>  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;
>  };
>  
>  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;
>  
>  	ret = device_register(&matrix_dev->device);
>  	if (ret)
>  		goto matrix_reg_err;
>  
> +	ret = driver_register(&matrix_driver.drv);
> +	if (ret)
> +		goto matrix_drv_err;
> +
>  	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;
>

Christian Borntraeger Feb. 14, 2019, 3:05 p.m. UTC | #3

On 14.02.2019 15:54, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:01 +0100
> Pierre Morel <[e-mail]> 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 <[e-mail]>
>> ---
>>  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 Feb. 14, 2019, 3:09 p.m. UTC | #4

On 14/02/2019 16:01, Christian Borntraeger wrote:
> 
> 
> On 14.02.2019 14:51, Pierre Morel wrote:
>> 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.
>>
>> 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 <[e-mail]>
> 
> Didnt you want to a add a reported-by?

Yes, forgot it.

Reported-by: Marc Hartmayer <[e-mail]>
Reported-by: Christian Borntraeger <[e-mail]>



>> ---
> 
> 
>>   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;
>>   };
>>   
>>   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;
>>   
>>   	ret = device_register(&matrix_dev->device);
>>   	if (ret)
>>   		goto matrix_reg_err;
>>   
>> +	ret = driver_register(&matrix_driver.drv);
>> +	if (ret)
>> +		goto matrix_drv_err;
>> +
>>   	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;
>>

Cornelia Huck Feb. 14, 2019, 3:40 p.m. UTC | #5

On Thu, 14 Feb 2019 16:05:35 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 14.02.2019 15:54, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:01 +0100
> > Pierre Morel <[e-mail]> 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.

Ugh, that feels really broken. Adding a bus is probably the easiest way
to get this working, though :/

Pierre Morel Feb. 14, 2019, 3:47 p.m. UTC | #6

On 14/02/2019 15:54, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:01 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> 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...

Christian answered this in another thread, so I don't answer.

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

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

yes, this is what is wanted: this driver can only support one device.
May be another matrix driver can support one or more other devices.

I should update comment message my be.

> 
>>   };
>>   
>>   struct ap_matrix_dev *matrix_dev;

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

Not anymore.
We have two different drivers and devices
matrix_drv <-> matrix_dev
and
vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv
what is needed here is vfio_ap_drv.

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

hum, no I don't think so, no device can register before this module is 
loaded, but what does prevent a device to register later from another 
module?

Cornelia Huck Feb. 14, 2019, 4:57 p.m. UTC | #7

On Thu, 14 Feb 2019 16:47:30 +0100
Pierre Morel <[e-mail]> wrote:

> On 14/02/2019 15:54, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:01 +0100
> > Pierre Morel <[e-mail]> wrote:

> >> +++ 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.  
> 
> yes, this is what is wanted: this driver can only support one device.
> May be another matrix driver can support one or more other devices.
> 
> I should update comment message my be.
> 
> >   
> >>   };
> >>   
> >>   struct ap_matrix_dev *matrix_dev;  
> 
> >>   
> >> -	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?  
> 
> Not anymore.
> We have two different drivers and devices
> matrix_drv <-> matrix_dev
> and
> vfio_ap_drv <-> ap_devices
> 
> The driver behind the matrix_dev->dev->driver is matrix_drv
> what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which
is not on the ap bus? Maybe that's what trips libudev?

(And reading further in the current code, it seems we clear that
structure _after_ the matrix device had been setup, so how can that
even work? Where am I confused?)

> 
> >   
> >>   
> >>   	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?  
> 
> hum, no I don't think so, no device can register before this module is 
> loaded, but what does prevent a device to register later from another 
> module?

Not unless you export the interface, I guess.

Anthony Krowiak Feb. 14, 2019, 5:12 p.m. UTC | #8

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 <[e-mail]> 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 <[e-mail]>
>>> ---
>>>   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 Feb. 14, 2019, 5:35 p.m. UTC | #9

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 <[e-mail]> 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 <[e-mail]>
>>> ---
>>>   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 Feb. 14, 2019, 5:36 p.m. UTC | #10

On 14/02/2019 17:57, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
> <[e-mail]> wrote:
> 
>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>> <[e-mail]> wrote:
> 
>>>> +++ 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.
>> 
>> yes, this is what is wanted: this driver can only support one
>> device. May be another matrix driver can support one or more other
>> devices.
>> 
>> I should update comment message my be.
>> 
>>> 
>>>> };
>>>> 
>>>> struct ap_matrix_dev *matrix_dev;
>> 
>>>> 
>>>> -	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?
>> 
>> Not anymore. We have two different drivers and devices matrix_drv
>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>> 
>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>> needed here is vfio_ap_drv.
> 
> Wait, we had tacked a driver for ap devices unto a matrix device,
> which is not on the ap bus?

...yes -(

> Maybe that's what trips libudev? >
> (And reading further in the current code, it seems we clear that 
> structure _after_ the matrix device had been setup, so how can that 
> even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a 
driver and this field was nor tested nor overwritten.

> 
>> 
>>> 
>>>> 
>>>> 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?
>> 
>> hum, no I don't think so, no device can register before this module
>> is loaded, but what does prevent a device to register later from
>> another module?
> 
> Not unless you export the interface, I guess.
> 

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as 
Christian requested.

Regards,
Pierre

Anthony Krowiak Feb. 14, 2019, 6:30 p.m. UTC | #11

On 2/14/19 12:36 PM, Pierre Morel wrote:
> On 14/02/2019 17:57, Cornelia Huck wrote:
>> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
>> <[e-mail]> wrote:
>>
>>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>>> <[e-mail]> wrote:
>>
>>>>> +++ 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.
>>>
>>> yes, this is what is wanted: this driver can only support one
>>> device. May be another matrix driver can support one or more other
>>> devices.
>>>
>>> I should update comment message my be.
>>>
>>>>
>>>>> };
>>>>>
>>>>> struct ap_matrix_dev *matrix_dev;
>>>
>>>>>
>>>>> -    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?
>>>
>>> Not anymore. We have two different drivers and devices matrix_drv
>>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>>>
>>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>>> needed here is vfio_ap_drv.
>>
>> Wait, we had tacked a driver for ap devices unto a matrix device,
>> which is not on the ap bus?

It's really a bit more complicated than that. Without going into a
lengthy description of the history of AP passthrough support, suffice it
to say that we needed a device to serve as the parent of each mediated
device used to configure a matrix of AP adapter IDs and domain indexes
identifying the devices to which a guest would be granted access. The
AP devices themselves are attached to the AP bus, but the matrix device
is an artificial (virtual?) device whose sole purpose in life is to
serve as an anchor for the mediated devices whose sysfs interfaces are
created and managed by the vfio_ap device driver. The matrix device
itself is created by the vfio_ap device driver - when it is initialized 
- for that purpose. In hindsight, maybe there was a better way to
implement this, but neither this patch nor this discussion belongs in
this series. It distracts from discussion of interrupt support which is
the sole purpose of the patch series.

> 
> ...yes -(
> 
>> Maybe that's what trips libudev? >
>> (And reading further in the current code, it seems we clear that 
>> structure _after_ the matrix device had been setup, so how can that 
>> even work? Where am I confused?)
> 
> On device_register there were no bus, so the core just do not look for a 
> driver and this field was nor tested nor overwritten.
> 
>>
>>>
>>>>
>>>>>
>>>>> 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?
>>>
>>> hum, no I don't think so, no device can register before this module
>>> is loaded, but what does prevent a device to register later from
>>> another module?
>>
>> Not unless you export the interface, I guess.
>>
> 
> :) definitively right
> thanks, this will simplify the code in the next version.
> I will take the patch away from this series to get the way to stable as 
> Christian requested.
> 
> Regards,
> Pierre
>

Cornelia Huck Feb. 15, 2019, 9:11 a.m. UTC | #12

On Thu, 14 Feb 2019 13:30:59 -0500
Tony Krowiak <[e-mail]> wrote:

> On 2/14/19 12:36 PM, Pierre Morel wrote:
> > On 14/02/2019 17:57, Cornelia Huck wrote:  
> >> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
> >> <[e-mail]> wrote:
> >>  
> >>> On 14/02/2019 15:54, Cornelia Huck wrote:  
> >>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
> >>>> <[e-mail]> wrote:  

> >>>>> -    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?  
> >>>
> >>> Not anymore. We have two different drivers and devices matrix_drv
> >>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
> >>>
> >>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
> >>> needed here is vfio_ap_drv.  
> >>
> >> Wait, we had tacked a driver for ap devices unto a matrix device,
> >> which is not on the ap bus?  
> 
> It's really a bit more complicated than that. Without going into a
> lengthy description of the history of AP passthrough support, suffice it
> to say that we needed a device to serve as the parent of each mediated
> device used to configure a matrix of AP adapter IDs and domain indexes
> identifying the devices to which a guest would be granted access. The
> AP devices themselves are attached to the AP bus, but the matrix device
> is an artificial (virtual?) device whose sole purpose in life is to
> serve as an anchor for the mediated devices whose sysfs interfaces are
> created and managed by the vfio_ap device driver. The matrix device
> itself is created by the vfio_ap device driver - when it is initialized 
> - for that purpose. In hindsight, maybe there was a better way to
> implement this, but neither this patch nor this discussion belongs in
> this series. It distracts from discussion of interrupt support which is
> the sole purpose of the patch series.

The we-need-a-parent part is fine; but whatever we're doing with that
driver just looks wrong, so that even the new bus that basically does
nothing looks better...

> 
> > 
> > ...yes -(
> >   
> >> Maybe that's what trips libudev? >
> >> (And reading further in the current code, it seems we clear that 
> >> structure _after_ the matrix device had been setup, so how can that 
> >> even work? Where am I confused?)  
> > 
> > On device_register there were no bus, so the core just do not look for a 
> > driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in
vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
patch fixes more than just libudev issues...

> >   
> >>  
> >>>  
> >>>>  
> >>>>>
> >>>>> 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?  
> >>>
> >>> hum, no I don't think so, no device can register before this module
> >>> is loaded, but what does prevent a device to register later from
> >>> another module?  
> >>
> >> Not unless you export the interface, I guess.
> >>  
> > 
> > :) definitively right
> > thanks, this will simplify the code in the next version.
> > I will take the patch away from this series to get the way to stable as 
> > Christian requested.

Yeah, makes sense.

Anthony Krowiak Feb. 15, 2019, 9:59 p.m. UTC | #13

On 2/15/19 4:11 AM, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 13:30:59 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>> On 14/02/2019 17:57, Cornelia Huck wrote:
>>>> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
>>>> <[e-mail]> wrote:
>>>>   
>>>>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>>>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>>>>> <[e-mail]> wrote:
> 
>>>>>>> -    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?
>>>>>
>>>>> Not anymore. We have two different drivers and devices matrix_drv
>>>>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>>>>>
>>>>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>>>>> needed here is vfio_ap_drv.
>>>>
>>>> Wait, we had tacked a driver for ap devices unto a matrix device,
>>>> which is not on the ap bus?
>>
>> It's really a bit more complicated than that. Without going into a
>> lengthy description of the history of AP passthrough support, suffice it
>> to say that we needed a device to serve as the parent of each mediated
>> device used to configure a matrix of AP adapter IDs and domain indexes
>> identifying the devices to which a guest would be granted access. The
>> AP devices themselves are attached to the AP bus, but the matrix device
>> is an artificial (virtual?) device whose sole purpose in life is to
>> serve as an anchor for the mediated devices whose sysfs interfaces are
>> created and managed by the vfio_ap device driver. The matrix device
>> itself is created by the vfio_ap device driver - when it is initialized
>> - for that purpose. In hindsight, maybe there was a better way to
>> implement this, but neither this patch nor this discussion belongs in
>> this series. It distracts from discussion of interrupt support which is
>> the sole purpose of the patch series.
> 
> The we-need-a-parent part is fine; but whatever we're doing with that
> driver just looks wrong, so that even the new bus that basically does
> nothing looks better...

I believe there might be a much better way to handle this which is why
I objected to this patch being delivered with this series, which
provides AP interrupt support. Quite simply, this patch has no
relationship to interrupt support and should be considered as an item
unto itself. To conduct a review within the context of interrupt
support distracts focus from the pertinent subject matter.

> 
>>
>>>
>>> ...yes -(
>>>    
>>>> Maybe that's what trips libudev? >
>>>> (And reading further in the current code, it seems we clear that
>>>> structure _after_ the matrix device had been setup, so how can that
>>>> even work? Where am I confused?)
>>>
>>> On device_register there were no bus, so the core just do not look for a
>>> driver and this field was nor tested nor overwritten.
> 
> Hm... so has the callback in driver_for_each_device() in
> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> patch fixes more than just libudev issues...

It is this patch that rendered the driver_for_each_device() in
vfio_ap_verify_queue_reserved() erroneous. That function gets called
every time an adapter or domain is assigned to the mdev. This patch
introduced the problem with driver_for_each_device().

> 
>>>    
>>>>   
>>>>>   
>>>>>>   
>>>>>>>
>>>>>>> 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?
>>>>>
>>>>> hum, no I don't think so, no device can register before this module
>>>>> is loaded, but what does prevent a device to register later from
>>>>> another module?
>>>>
>>>> Not unless you export the interface, I guess.
>>>>   
>>>
>>> :) definitively right
>>> thanks, this will simplify the code in the next version.
>>> I will take the patch away from this series to get the way to stable as
>>> Christian requested.
> 
> Yeah, makes sense.
>

Cornelia Huck Feb. 18, 2019, 12:01 p.m. UTC | #14

On Fri, 15 Feb 2019 16:59:33 -0500
Tony Krowiak <[e-mail]> wrote:

> On 2/15/19 4:11 AM, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 13:30:59 -0500
> > Tony Krowiak <[e-mail]> wrote:
> >   
> >> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>> On 14/02/2019 17:57, Cornelia Huck wrote:  

> >>>> (And reading further in the current code, it seems we clear that
> >>>> structure _after_ the matrix device had been setup, so how can that
> >>>> even work? Where am I confused?)  
> >>>
> >>> On device_register there were no bus, so the core just do not look for a
> >>> driver and this field was nor tested nor overwritten.  
> > 
> > Hm... so has the callback in driver_for_each_device() in
> > vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> > patch fixes more than just libudev issues...  
> 
> It is this patch that rendered the driver_for_each_device() in
> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> every time an adapter or domain is assigned to the mdev. This patch
> introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place,
then? (It looks like it was dead code before.)

Anthony Krowiak Feb. 18, 2019, 4:35 p.m. UTC | #15

On 2/18/19 7:01 AM, Cornelia Huck wrote:
> On Fri, 15 Feb 2019 16:59:33 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 2/15/19 4:11 AM, Cornelia Huck wrote:
>>> On Thu, 14 Feb 2019 13:30:59 -0500
>>> Tony Krowiak <[e-mail]> wrote:
>>>    
>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:
> 
>>>>>> (And reading further in the current code, it seems we clear that
>>>>>> structure _after_ the matrix device had been setup, so how can that
>>>>>> even work? Where am I confused?)
>>>>>
>>>>> On device_register there were no bus, so the core just do not look for a
>>>>> driver and this field was nor tested nor overwritten.
>>>
>>> Hm... so has the callback in driver_for_each_device() in
>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
>>> patch fixes more than just libudev issues...
>>
>> It is this patch that rendered the driver_for_each_device() in
>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
>> every time an adapter or domain is assigned to the mdev. This patch
>> introduced the problem with driver_for_each_device().
> 
> So, does this function need to be removed or called from another place,
> then? (It looks like it was dead code before.)

I don't see why you think it's dead code:

assign_adapter_store
==> vfio_ap_mdev_verify_queues_reserved_for_apid
     ==> vfio_ap_verify_queue_reserved
         ==> driver_for_each_device

The only way that the vfio_ap_verify_queue_reserved - the function that
calls driver_for_each_device - does not get called is if no bits have
yet been set in matrix_mdev->matrix.aqm.

>

Cornelia Huck Feb. 18, 2019, 4:57 p.m. UTC | #16

On Mon, 18 Feb 2019 11:35:45 -0500
Tony Krowiak <[e-mail]> wrote:

> On 2/18/19 7:01 AM, Cornelia Huck wrote:
> > On Fri, 15 Feb 2019 16:59:33 -0500
> > Tony Krowiak <[e-mail]> wrote:
> >   
> >> On 2/15/19 4:11 AM, Cornelia Huck wrote:  
> >>> On Thu, 14 Feb 2019 13:30:59 -0500
> >>> Tony Krowiak <[e-mail]> wrote:
> >>>      
> >>>> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>>>> On 14/02/2019 17:57, Cornelia Huck wrote:  
> >   
> >>>>>> (And reading further in the current code, it seems we clear that
> >>>>>> structure _after_ the matrix device had been setup, so how can that
> >>>>>> even work? Where am I confused?)  
> >>>>>
> >>>>> On device_register there were no bus, so the core just do not look for a
> >>>>> driver and this field was nor tested nor overwritten.  
> >>>
> >>> Hm... so has the callback in driver_for_each_device() in
> >>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> >>> patch fixes more than just libudev issues...  
> >>
> >> It is this patch that rendered the driver_for_each_device() in
> >> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> >> every time an adapter or domain is assigned to the mdev. This patch
> >> introduced the problem with driver_for_each_device().  
> > 
> > So, does this function need to be removed or called from another place,
> > then? (It looks like it was dead code before.)  
> 
> I don't see why you think it's dead code:
> 
> assign_adapter_store
> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
>      ==> vfio_ap_verify_queue_reserved
>          ==> driver_for_each_device  
> 
> The only way that the vfio_ap_verify_queue_reserved - the function that
> calls driver_for_each_device - does not get called is if no bits have
> yet been set in matrix_mdev->matrix.aqm.

What I don't see is how this can be called if no device has been, in
fact, bound to the driver in the driver core...

Anthony Krowiak Feb. 19, 2019, 10:27 p.m. UTC | #17

On 2/18/19 11:57 AM, Cornelia Huck wrote:
> On Mon, 18 Feb 2019 11:35:45 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 2/18/19 7:01 AM, Cornelia Huck wrote:
>>> On Fri, 15 Feb 2019 16:59:33 -0500
>>> Tony Krowiak <[e-mail]> wrote:
>>>    
>>>> On 2/15/19 4:11 AM, Cornelia Huck wrote:
>>>>> On Thu, 14 Feb 2019 13:30:59 -0500
>>>>> Tony Krowiak <[e-mail]> wrote:
>>>>>       
>>>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:
>>>    
>>>>>>>> (And reading further in the current code, it seems we clear that
>>>>>>>> structure _after_ the matrix device had been setup, so how can that
>>>>>>>> even work? Where am I confused?)
>>>>>>>
>>>>>>> On device_register there were no bus, so the core just do not look for a
>>>>>>> driver and this field was nor tested nor overwritten.
>>>>>
>>>>> Hm... so has the callback in driver_for_each_device() in
>>>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
>>>>> patch fixes more than just libudev issues...
>>>>
>>>> It is this patch that rendered the driver_for_each_device() in
>>>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
>>>> every time an adapter or domain is assigned to the mdev. This patch
>>>> introduced the problem with driver_for_each_device().
>>>
>>> So, does this function need to be removed or called from another place,
>>> then? (It looks like it was dead code before.)
>>
>> I don't see why you think it's dead code:
>>
>> assign_adapter_store
>> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
>>       ==> vfio_ap_verify_queue_reserved
>>           ==> driver_for_each_device
>>
>> The only way that the vfio_ap_verify_queue_reserved - the function that
>> calls driver_for_each_device - does not get called is if no bits have
>> yet been set in matrix_mdev->matrix.aqm.
> 
> What I don't see is how this can be called if no device has been, in
> fact, bound to the driver in the driver core...

Let's start with the fact that one can create an mdev device regardless
of whether a queue has been bound to the vfio_ap driver. Once an mdev
device is created, one can start assigning adapters, domains and control
domains to it. Let's say the admin now attempts to assign an adapter, in
which case the assign_adapter_store() function is invoked. After
verifying that the APID passed in is a valid adapter number, the
vfio_ap_mdev_verify_queues_reserved_for_apid() function is called.
This function first checks if any domains have been assigned and if not,
calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function
that the driver_for_each_device() function is called. Since there are
no devices bound to the vfio_ap device driver, the callback passed in to
the driver_for_each_device() function will never get called, so the
vfio_ap_mdev_verify_queues_reserved_for_apid() function will return
-EADDRNOTAVAIL. A similar flow will occur if the first assignment is for
a domain. The bottom line is, the driver_for_each_device() function is
called every time an adapter or domain is assigned.

>

Cornelia Huck Feb. 20, 2019, 9:05 a.m. UTC | #18

On Tue, 19 Feb 2019 17:27:05 -0500
Tony Krowiak <[e-mail]> wrote:

> On 2/18/19 11:57 AM, Cornelia Huck wrote:
> > On Mon, 18 Feb 2019 11:35:45 -0500
> > Tony Krowiak <[e-mail]> wrote:
> >   
> >> On 2/18/19 7:01 AM, Cornelia Huck wrote:  
> >>> On Fri, 15 Feb 2019 16:59:33 -0500
> >>> Tony Krowiak <[e-mail]> wrote:
> >>>      
> >>>> On 2/15/19 4:11 AM, Cornelia Huck wrote:  
> >>>>> On Thu, 14 Feb 2019 13:30:59 -0500
> >>>>> Tony Krowiak <[e-mail]> wrote:
> >>>>>         
> >>>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:  
> >>>      
> >>>>>>>> (And reading further in the current code, it seems we clear that
> >>>>>>>> structure _after_ the matrix device had been setup, so how can that
> >>>>>>>> even work? Where am I confused?)  
> >>>>>>>
> >>>>>>> On device_register there were no bus, so the core just do not look for a
> >>>>>>> driver and this field was nor tested nor overwritten.  
> >>>>>
> >>>>> Hm... so has the callback in driver_for_each_device() in
> >>>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> >>>>> patch fixes more than just libudev issues...  
> >>>>
> >>>> It is this patch that rendered the driver_for_each_device() in
> >>>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> >>>> every time an adapter or domain is assigned to the mdev. This patch
> >>>> introduced the problem with driver_for_each_device().  
> >>>
> >>> So, does this function need to be removed or called from another place,
> >>> then? (It looks like it was dead code before.)  
> >>
> >> I don't see why you think it's dead code:
> >>
> >> assign_adapter_store  
> >> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
> >>       ==> vfio_ap_verify_queue_reserved
> >>           ==> driver_for_each_device  
> >>
> >> The only way that the vfio_ap_verify_queue_reserved - the function that
> >> calls driver_for_each_device - does not get called is if no bits have
> >> yet been set in matrix_mdev->matrix.aqm.  
> > 
> > What I don't see is how this can be called if no device has been, in
> > fact, bound to the driver in the driver core...  
> 
> Let's start with the fact that one can create an mdev device regardless
> of whether a queue has been bound to the vfio_ap driver. Once an mdev
> device is created, one can start assigning adapters, domains and control
> domains to it. Let's say the admin now attempts to assign an adapter, in
> which case the assign_adapter_store() function is invoked. After
> verifying that the APID passed in is a valid adapter number, the
> vfio_ap_mdev_verify_queues_reserved_for_apid() function is called.
> This function first checks if any domains have been assigned and if not,
> calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function
> that the driver_for_each_device() function is called. Since there are
> no devices bound to the vfio_ap device driver, the callback passed in to
> the driver_for_each_device() function will never get called, so the
> vfio_ap_mdev_verify_queues_reserved_for_apid() function will return
> -EADDRNOTAVAIL. A similar flow will occur if the first assignment is for
> a domain. The bottom line is, the driver_for_each_device() function is
> called every time an adapter or domain is assigned.

Indeed. I just got lost with the various drivers and devices in play
here :(

Patch

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;
 };
 
 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;
 
 	ret = device_register(&matrix_dev->device);
 	if (ret)
 		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+
 	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;