[PATCH v2 0/1] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[PATCH v2 0/1] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

This is the second version of this patch.
The first version was part of a patch series on VFIO AP IRQ.

The goal of this patch is to standardize the device-driver interface
for the VFIO_AP ap_matrix_device to satisfy user-land tools working on
hot-plug (UDEV/LIBVIRT).

Christian Borntraeger reported 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


Pierre Morel (1):
  s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

 drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 43 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 1/1] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

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.

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.

Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 31c6c84..8e45559 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
 
 /* Only type 10 adapters (CEX4 and later) are supported
@@ -62,6 +58,27 @@ 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)
+{
+	return 0;
+}
+
+static struct device_driver matrix_driver = {
+	.name = "vfio_ap",
+	.bus = &matrix_bus,
+	.probe = matrix_probe,
+};
+
 static int vfio_ap_matrix_dev_create(void)
 {
 	int ret;
@@ -71,6 +88,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 +108,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);
+	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);
 	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;
-- 
2.7.4

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

[Christian Borntraeger]

On 18.02.2019 19:08, 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.
> 
> 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.
> 
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>

Can you also add a "Fixes:" tag

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

Would be good to have someone with device experience reviewing the changed.

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..8e45559 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>  
>  /* Only type 10 adapters (CEX4 and later) are supported
> @@ -62,6 +58,27 @@ 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)
> +{
> +	return 0;
> +}
> +
> +static struct device_driver matrix_driver = {
> +	.name = "vfio_ap",
> +	.bus = &matrix_bus,
> +	.probe = matrix_probe,
> +};
> +
>  static int vfio_ap_matrix_dev_create(void)
>  {
>  	int ret;
> @@ -71,6 +88,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 +108,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);
> +	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);
>  	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;
> 

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

[Cornelia Huck]

On Mon, 18 Feb 2019 19:08:48 +0100
Pierre Morel <pmorel@linux.ibm.com> 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.
> 
> 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

s/suppress/remove/ ?

> 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.
> 
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 43 insertions(+), 10 deletions(-)

(...)

> @@ -62,6 +58,27 @@ 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)
> +{
> +	return 0;
> +}

I don't think you need this (the important function is the match
function of the bus).

> +
> +static struct device_driver matrix_driver = {
> +	.name = "vfio_ap",
> +	.bus = &matrix_bus,
> +	.probe = matrix_probe,
> +};
> +
>  static int vfio_ap_matrix_dev_create(void)
>  {
>  	int ret;

It's a bit annoying that we need to introduce a bus that basically does
nothing, but I think this looks sane.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Christian Borntraeger]

On 19.02.2019 10:22, Cornelia Huck wrote:
> On Mon, 18 Feb 2019 19:08:48 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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.
>>
>> 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
> 
> s/suppress/remove/ ?
> 
>> 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.
>>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>>  3 files changed, 43 insertions(+), 10 deletions(-)
> 
> (...)
> 
>> @@ -62,6 +58,27 @@ 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)
>> +{
>> +	return 0;
>> +}
> 
> I don't think you need this (the important function is the match
> function of the bus).

Yes, with 

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 8e45559795429..8ceec41afe322 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -68,15 +68,9 @@ static struct bus_type matrix_bus = {
        .match = &matrix_bus_match,
 };
 
-static int matrix_probe(struct device *dev)
-{
-       return 0;
-}
-
 static struct device_driver matrix_driver = {
        .name = "vfio_ap",
        .bus = &matrix_bus,
-       .probe = matrix_probe,
 };
 
 static int vfio_ap_matrix_dev_create(void)

on top things still look fine.


> 
>> +
>> +static struct device_driver matrix_driver = {
>> +	.name = "vfio_ap",
>> +	.bus = &matrix_bus,
>> +	.probe = matrix_probe,
>> +};
>> +
>>  static int vfio_ap_matrix_dev_create(void)
>>  {
>>  	int ret;
> 
> It's a bit annoying that we need to introduce a bus that basically does
> nothing, but I think this looks sane.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

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

[Pierre Morel]

On 19/02/2019 10:44, Christian Borntraeger wrote:
> 
> 
> On 19.02.2019 10:22, Cornelia Huck wrote:
>> On Mon, 18 Feb 2019 19:08:48 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>

...snip...

>>> +static struct bus_type matrix_bus = {
>>> +	.name = "vfio_ap",
>>> +	.match = &matrix_bus_match,
>>> +};
>>> +
>>> +static int matrix_probe(struct device *dev)
>>> +{
>>> +	return 0;
>>> +}
>>
>> I don't think you need this (the important function is the match
>> function of the bus).
> 
> Yes, with
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 8e45559795429..8ceec41afe322 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -68,15 +68,9 @@ static struct bus_type matrix_bus = {
>          .match = &matrix_bus_match,
>   };
>   
> -static int matrix_probe(struct device *dev)
> -{
> -       return 0;
> -}
> -
>   static struct device_driver matrix_driver = {
>          .name = "vfio_ap",
>          .bus = &matrix_bus,
> -       .probe = matrix_probe,
>   };
>   
>   static int vfio_ap_matrix_dev_create(void)
> 
> on top things still look fine.

OK, simpler is better, I remove the probe.

Thanks,
Pierre

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

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

[Halil Pasic]

On Mon, 18 Feb 2019 19:08:48 +0100
Pierre Morel <pmorel@linux.ibm.com> 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.
> 
> 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.
> 
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..8e45559 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,10 +24,6 @@ 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,
> -};
> -

Would have been nice had you mentioned this in the commit message --
vfio_ap_dev_type was useless. And you should have removed
the define of VFIO_AP_DEV_TYPE_NAME a couple of lines above this.

>  struct ap_matrix_dev *matrix_dev;
>  
>  /* Only type 10 adapters (CEX4 and later) are supported
> @@ -62,6 +58,27 @@ 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)
> +{
> +	return 0;
> +}
> +
> +static struct device_driver matrix_driver = {
> +	.name = "vfio_ap",
> +	.bus = &matrix_bus,
> +	.probe = matrix_probe,
> +};
> +

With the changes suggested by Christian:

Acked-by: Halil Pasic <pasic@linux.ibm.com>

[..]

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

[Tony Krowiak]

On 2/18/19 1:08 PM, 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.
> 
> 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.
> 
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>   3 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..8e45559 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>   
>   /* Only type 10 adapters (CEX4 and later) are supported
> @@ -62,6 +58,27 @@ 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;

I think we should verify the following:

* dev == matrix_dev->device
* drv == &matrix_driver

The model employed is for the matrix device to be a singleton, so I
think we should verify that the matrix device and driver defined herein
ought to be the only possible choices for a match. Of course, doing so
will require some restructuring of this patch.

> +}
> +
> +static struct bus_type matrix_bus = {
> +	.name = "vfio_ap",
> +	.match = &matrix_bus_match,
> +};
> +
> +static int matrix_probe(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static struct device_driver matrix_driver = {
> +	.name = "vfio_ap",

This is the same name used for the original device driver. I think
this driver ought to have a different name to avoid confusion.
How about vfio_ap_matrix or some other name to differentiate the
two.

> +	.bus = &matrix_bus,
> +	.probe = matrix_probe,

I would add:
	.suppress_bind_attrs = true;

This will remove the sysfs bind/unbind interfaces. Since there is only
one matrix device and it's lifecycle is controlled herein, there is no
sense in allowing a root user to bind/unbind it.

> +};
> +
>   static int vfio_ap_matrix_dev_create(void)
>   {
>   	int ret;
> @@ -71,6 +88,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 +108,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);
> +	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);
>   	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;
> 

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

[Pierre Morel]

On 19/02/2019 19:52, Tony Krowiak wrote:
> On 2/18/19 1:08 PM, 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.
>>
>> 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.
>>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 48 
>> +++++++++++++++++++++++++++++------
>>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>   3 files changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index 31c6c84..8e45559 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>>   /* Only type 10 adapters (CEX4 and later) are supported
>> @@ -62,6 +58,27 @@ 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;
> 
> I think we should verify the following:
> 
> * dev == matrix_dev->device
> * drv == &matrix_driver
> 
> The model employed is for the matrix device to be a singleton, so I
> think we should verify that the matrix device and driver defined herein
> ought to be the only possible choices for a match. Of course, doing so
> will require some restructuring of this patch.

I think Conny already answered this question.

> 
>> +}
>> +
>> +static struct bus_type matrix_bus = {
>> +    .name = "vfio_ap",
>> +    .match = &matrix_bus_match,
>> +};
>> +
>> +static int matrix_probe(struct device *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static struct device_driver matrix_driver = {
>> +    .name = "vfio_ap",
> 
> This is the same name used for the original device driver. I think
> this driver ought to have a different name to avoid confusion.
> How about vfio_ap_matrix or some other name to differentiate the
> two.

I would like too, but changing this will change the path to the mediated 
device supported type.


> 
>> +    .bus = &matrix_bus,
>> +    .probe = matrix_probe,
> 
> I would add:
>      .suppress_bind_attrs = true;
> 
> This will remove the sysfs bind/unbind interfaces. Since there is only
> one matrix device and it's lifecycle is controlled herein, there is no
> sense in allowing a root user to bind/unbind it.
> 

OTOH bind/unbind has no impact.
If no one else ask for this I will not change what has already been 
reviewed by Conny and Christian.

Regards,
Pierre


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

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

[Cornelia Huck]

On Tue, 19 Feb 2019 22:31:17 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 19/02/2019 19:52, Tony Krowiak wrote:
> > On 2/18/19 1:08 PM, 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.
> >>
> >> 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.
> >>
> >> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>   drivers/s390/crypto/vfio_ap_drv.c     | 48 
> >> +++++++++++++++++++++++++++++------
> >>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
> >>   drivers/s390/crypto/vfio_ap_private.h |  1 +
> >>   3 files changed, 43 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
> >> b/drivers/s390/crypto/vfio_ap_drv.c
> >> index 31c6c84..8e45559 100644
> >> --- a/drivers/s390/crypto/vfio_ap_drv.c
> >> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> >> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
> >>   /* Only type 10 adapters (CEX4 and later) are supported
> >> @@ -62,6 +58,27 @@ 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;  
> > 
> > I think we should verify the following:
> > 
> > * dev == matrix_dev->device
> > * drv == &matrix_driver
> > 
> > The model employed is for the matrix device to be a singleton, so I
> > think we should verify that the matrix device and driver defined herein
> > ought to be the only possible choices for a match. Of course, doing so
> > will require some restructuring of this patch.  
> 
> I think Conny already answered this question.

Not quite :), but I don't think we need any magic in there, as there's
only one device and only one driver on that bus. No need to make this
more complicated.

> 
> >   
> >> +}
> >> +
> >> +static struct bus_type matrix_bus = {
> >> +    .name = "vfio_ap",
> >> +    .match = &matrix_bus_match,
> >> +};
> >> +
> >> +static int matrix_probe(struct device *dev)
> >> +{
> >> +    return 0;
> >> +}
> >> +
> >> +static struct device_driver matrix_driver = {
> >> +    .name = "vfio_ap",  
> > 
> > This is the same name used for the original device driver. I think
> > this driver ought to have a different name to avoid confusion.
> > How about vfio_ap_matrix or some other name to differentiate the
> > two.  
> 
> I would like too, but changing this will change the path to the mediated 
> device supported type.

Yes, we don't want to change that.

> 
> 
> >   
> >> +    .bus = &matrix_bus,
> >> +    .probe = matrix_probe,  
> > 
> > I would add:
> >      .suppress_bind_attrs = true;
> > 
> > This will remove the sysfs bind/unbind interfaces. Since there is only
> > one matrix device and it's lifecycle is controlled herein, there is no
> > sense in allowing a root user to bind/unbind it.
> >   
> 
> OTOH bind/unbind has no impact.
> If no one else ask for this I will not change what has already been 
> reviewed by Conny and Christian.

As we only have one driver, it does not really make sense anyway.

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

[Halil Pasic]

On Wed, 20 Feb 2019 10:27:31 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 19 Feb 2019 22:31:17 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > On 19/02/2019 19:52, Tony Krowiak wrote:
> > > On 2/18/19 1:08 PM, 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.
> > >>
> > >> 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.
> > >>
> > >> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> > >> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > >> ---
> > >>   drivers/s390/crypto/vfio_ap_drv.c     | 48 
> > >> +++++++++++++++++++++++++++++------
> > >>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
> > >>   drivers/s390/crypto/vfio_ap_private.h |  1 +
> > >>   3 files changed, 43 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
> > >> b/drivers/s390/crypto/vfio_ap_drv.c
> > >> index 31c6c84..8e45559 100644
> > >> --- a/drivers/s390/crypto/vfio_ap_drv.c
> > >> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> > >> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
> > >>   /* Only type 10 adapters (CEX4 and later) are supported
> > >> @@ -62,6 +58,27 @@ 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;  
> > > 
> > > I think we should verify the following:
> > > 
> > > * dev == matrix_dev->device
> > > * drv == &matrix_driver
> > > 
> > > The model employed is for the matrix device to be a singleton, so I
> > > think we should verify that the matrix device and driver defined herein
> > > ought to be the only possible choices for a match. Of course, doing so
> > > will require some restructuring of this patch.  
> > 
> > I think Conny already answered this question.
> 
> Not quite :), but I don't think we need any magic in there, as there's
> only one device and only one driver on that bus. No need to make this
> more complicated.
> 


I agree, no need to complicate this any further.

> > 
> > >   
> > >> +}
> > >> +
> > >> +static struct bus_type matrix_bus = {
> > >> +    .name = "vfio_ap",
> > >> +    .match = &matrix_bus_match,
> > >> +};
> > >> +
> > >> +static int matrix_probe(struct device *dev)
> > >> +{
> > >> +    return 0;
> > >> +}
> > >> +
> > >> +static struct device_driver matrix_driver = {
> > >> +    .name = "vfio_ap",  
> > > 
> > > This is the same name used for the original device driver. I think
> > > this driver ought to have a different name to avoid confusion.
> > > How about vfio_ap_matrix or some other name to differentiate the
> > > two.  
> > 
> > I would like too, but changing this will change the path to the mediated 
> > device supported type.
> 
> Yes, we don't want to change that.
> 

Nod.

> > 
> > 
> > >   
> > >> +    .bus = &matrix_bus,
> > >> +    .probe = matrix_probe,  
> > > 
> > > I would add:
> > >      .suppress_bind_attrs = true;
> > > 
> > > This will remove the sysfs bind/unbind interfaces. Since there is only
> > > one matrix device and it's lifecycle is controlled herein, there is no
> > > sense in allowing a root user to bind/unbind it.
> > >   
> > 
> > OTOH bind/unbind has no impact.
> > If no one else ask for this I will not change what has already been 
> > reviewed by Conny and Christian.
> 
> As we only have one driver, it does not really make sense anyway.
> 

I see this as a reason to suppress_bind_attrs. It is much easier than to
think about what should happen when one unbinds the matrix device from
the vfio_ap driver on the vfio_ap bus. With the code as is it seems to
just keep working as if nothing happened.
And /sys/devices/vfio_ap/matrix/mdev_supported_types/ referencing the
name of the driver that is already gone sounds a bit weird.

Regards,
Halil

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

[Pierre Morel]

On 20/02/2019 13:51, Halil Pasic wrote:
> On Wed, 20 Feb 2019 10:27:31 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Tue, 19 Feb 2019 22:31:17 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 19/02/2019 19:52, Tony Krowiak wrote:
>>>> On 2/18/19 1:08 PM, Pierre Morel wrote:
>>>>> Libudev relies on having a subsystem link for non-root devices. To

...snip...

>>>>> +
>>>>> +static struct device_driver matrix_driver = {
>>>>> +    .name = "vfio_ap",
>>>>
>>>> This is the same name used for the original device driver. I think
>>>> this driver ought to have a different name to avoid confusion.
>>>> How about vfio_ap_matrix or some other name to differentiate the
>>>> two.
>>>
>>> I would like too, but changing this will change the path to the mediated
>>> device supported type.
>>
>> Yes, we don't want to change that.
>>
> 
> Nod.

However if I cannot change the driver name, I can change the bus name to 
matrix.
At least one less "vfio_ap" name

> 
>>>
>>>
>>>>    
>>>>> +    .bus = &matrix_bus,
>>>>> +    .probe = matrix_probe,
>>>>
>>>> I would add:
>>>>       .suppress_bind_attrs = true;
>>>>
>>>> This will remove the sysfs bind/unbind interfaces. Since there is only
>>>> one matrix device and it's lifecycle is controlled herein, there is no
>>>> sense in allowing a root user to bind/unbind it.
>>>>    
>>>
>>> OTOH bind/unbind has no impact.
>>> If no one else ask for this I will not change what has already been
>>> reviewed by Conny and Christian.
>>
>> As we only have one driver, it does not really make sense anyway.
>>
> 
> I see this as a reason to suppress_bind_attrs. It is much easier than to
> think about what should happen when one unbinds the matrix device from
> the vfio_ap driver on the vfio_ap bus. With the code as is it seems to
> just keep working as if nothing happened.
> And /sys/devices/vfio_ap/matrix/mdev_supported_types/ referencing the
> name of the driver that is already gone sounds a bit weird.
> 
> Regards,
> Halil
> 

If there is no objection I will do this,
It seems more logical to me too.

Regards,
Pierre

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

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

[Christian Borntraeger]

On 21.02.2019 13:10, Pierre Morel wrote:
> On 20/02/2019 13:51, Halil Pasic wrote:
>> On Wed, 20 Feb 2019 10:27:31 +0100
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Tue, 19 Feb 2019 22:31:17 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>
>>>> On 19/02/2019 19:52, Tony Krowiak wrote:
>>>>> On 2/18/19 1:08 PM, Pierre Morel wrote:
>>>>>> Libudev relies on having a subsystem link for non-root devices. To
> 
> ...snip...
> 
>>>>>> +
>>>>>> +static struct device_driver matrix_driver = {
>>>>>> +    .name = "vfio_ap",
>>>>>
>>>>> This is the same name used for the original device driver. I think
>>>>> this driver ought to have a different name to avoid confusion.
>>>>> How about vfio_ap_matrix or some other name to differentiate the
>>>>> two.
>>>>
>>>> I would like too, but changing this will change the path to the mediated
>>>> device supported type.
>>>
>>> Yes, we don't want to change that.
>>>
>>
>> Nod.
> 
> However if I cannot change the driver name, I can change the bus name to matrix.
> At least one less "vfio_ap" name
> 
>>
>>>>
>>>>
>>>>>   
>>>>>> +    .bus = &matrix_bus,
>>>>>> +    .probe = matrix_probe,
>>>>>
>>>>> I would add:
>>>>>       .suppress_bind_attrs = true;
>>>>>
>>>>> This will remove the sysfs bind/unbind interfaces. Since there is only
>>>>> one matrix device and it's lifecycle is controlled herein, there is no
>>>>> sense in allowing a root user to bind/unbind it.
>>>>>    
>>>>
>>>> OTOH bind/unbind has no impact.
>>>> If no one else ask for this I will not change what has already been
>>>> reviewed by Conny and Christian.
>>>
>>> As we only have one driver, it does not really make sense anyway.
>>>
>>
>> I see this as a reason to suppress_bind_attrs. It is much easier than to
>> think about what should happen when one unbinds the matrix device from
>> the vfio_ap driver on the vfio_ap bus. With the code as is it seems to
>> just keep working as if nothing happened.
>> And /sys/devices/vfio_ap/matrix/mdev_supported_types/ referencing the
>> name of the driver that is already gone sounds a bit weird.
>>
>> Regards,
>> Halil
>>
> 
> If there is no objection I will do this,
> It seems more logical to me too.

Go ahead and send this as v3?

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

[Pierre Morel]

On 21/02/2019 13:35, Christian Borntraeger wrote:
> 
> 
> On 21.02.2019 13:10, Pierre Morel wrote:
>> On 20/02/2019 13:51, Halil Pasic wrote:
>>> On Wed, 20 Feb 2019 10:27:31 +0100
>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>
>>>> On Tue, 19 Feb 2019 22:31:17 +0100
>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>
>>>>> On 19/02/2019 19:52, Tony Krowiak wrote:
>>>>>> On 2/18/19 1:08 PM, Pierre Morel wrote:
>>>>>>> Libudev relies on having a subsystem link for non-root devices. To
>>
>> ...snip...
>>
>>>>>>> +
>>>>>>> +static struct device_driver matrix_driver = {
>>>>>>> +    .name = "vfio_ap",
>>>>>>
>>>>>> This is the same name used for the original device driver. I think
>>>>>> this driver ought to have a different name to avoid confusion.
>>>>>> How about vfio_ap_matrix or some other name to differentiate the
>>>>>> two.
>>>>>
>>>>> I would like too, but changing this will change the path to the mediated
>>>>> device supported type.
>>>>
>>>> Yes, we don't want to change that.
>>>>
>>>
>>> Nod.
>>
>> However if I cannot change the driver name, I can change the bus name to matrix.
>> At least one less "vfio_ap" name
>>
>>>
>>>>>
>>>>>
>>>>>>    
>>>>>>> +    .bus = &matrix_bus,
>>>>>>> +    .probe = matrix_probe,
>>>>>>
>>>>>> I would add:
>>>>>>        .suppress_bind_attrs = true;
>>>>>>
>>>>>> This will remove the sysfs bind/unbind interfaces. Since there is only
>>>>>> one matrix device and it's lifecycle is controlled herein, there is no
>>>>>> sense in allowing a root user to bind/unbind it.
>>>>>>     
>>>>>
>>>>> OTOH bind/unbind has no impact.
>>>>> If no one else ask for this I will not change what has already been
>>>>> reviewed by Conny and Christian.
>>>>
>>>> As we only have one driver, it does not really make sense anyway.
>>>>
>>>
>>> I see this as a reason to suppress_bind_attrs. It is much easier than to
>>> think about what should happen when one unbinds the matrix device from
>>> the vfio_ap driver on the vfio_ap bus. With the code as is it seems to
>>> just keep working as if nothing happened.
>>> And /sys/devices/vfio_ap/matrix/mdev_supported_types/ referencing the
>>> name of the driver that is already gone sounds a bit weird.
>>>
>>> Regards,
>>> Halil
>>>
>>
>> If there is no objection I will do this,
>> It seems more logical to me too.
> 
> Go ahead and send this as v3?
> 

OK
CC stable ?


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

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

[Christian Borntraeger]

On 21.02.2019 13:51, Pierre Morel wrote:
> On 21/02/2019 13:35, Christian Borntraeger wrote:
>>
>>
[..]

>> Go ahead and send this as v3?
>>
> 
> OK
> CC stable ?

Yes, also add a "Fixes:" tag.

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

[Pierre Morel]

On 21/02/2019 14:01, Christian Borntraeger wrote:
> On 21.02.2019 13:51, Pierre Morel wrote:
>> On 21/02/2019 13:35, Christian Borntraeger wrote:
>>>
>>>
> [..]
> 
>>> Go ahead and send this as v3?
>>>
>>
>> OK
>> CC stable ?
> 
> Yes, also add a "Fixes:" tag.
> 

Yes, I will not forget this time :)

Thanks
Pierre

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

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

[Harald Freudenberger]

On 18.02.19 19:08, 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.
>
> 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.
>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..8e45559 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>  
>  /* Only type 10 adapters (CEX4 and later) are supported
> @@ -62,6 +58,27 @@ 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)
> +{
> +	return 0;
> +}
> +
> +static struct device_driver matrix_driver = {
> +	.name = "vfio_ap",
> +	.bus = &matrix_bus,
> +	.probe = matrix_probe,
> +};
> +
>  static int vfio_ap_matrix_dev_create(void)
>  {
>  	int ret;
> @@ -71,6 +88,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 +108,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);
> +	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);
>  	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;

You are introducing a new bus just for a user space application which is unable
to deal with a device directly attached to the root of devices ? So you are fixing
kernel code because of disability of userspace code. Hm, you are switching
root cause and effect. However, not my job.

Why do you need this dummy bus ? Did you evaluate using a "class" subsystem
instead ? This is very common and my assumption is that libudev is able to handle
this. I am using a "zcrypt" class for providing additional zcrypt device nodes and
this works fine together with udev. I would avoid the introduction and maintenance
of bus code at any cost.

Btw. having a look onto the naming ... the module is named "vfio_ap", the
driver is named "vfio_ap", the bus is named "vfio_ap", the root bus device is
named "vfio_ap" ... a bunch of vfio_aps naming different things.

regards
Harald Freudenberger

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

[Christian Borntraeger]

On 20.02.2019 14:12, Harald Freudenberger wrote:
> On 18.02.19 19:08, 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.
>>
>> 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.
>>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>>  3 files changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 31c6c84..8e45559 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>>  
>>  /* Only type 10 adapters (CEX4 and later) are supported
>> @@ -62,6 +58,27 @@ 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)
>> +{
>> +	return 0;
>> +}
>> +
>> +static struct device_driver matrix_driver = {
>> +	.name = "vfio_ap",
>> +	.bus = &matrix_bus,
>> +	.probe = matrix_probe,
>> +};
>> +
>>  static int vfio_ap_matrix_dev_create(void)
>>  {
>>  	int ret;
>> @@ -71,6 +88,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 +108,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);
>> +	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);
>>  	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;
> 
> You are introducing a new bus just for a user space application which is unable
> to deal with a device directly attached to the root of devices ? So you are fixing
> kernel code because of disability of userspace code. Hm, you are switching
> root cause and effect. However, not my job.

the kernel rule is pretty simple. If userspace breaks due to a kernel change fix the
kernel.

> 
> Why do you need this dummy bus ? Did you evaluate using a "class" subsystem
> instead ? This is very common and my assumption is that libudev is able to handle
> this. I am using a "zcrypt" class for providing additional zcrypt device nodes and
> this works fine together with udev. I would avoid the introduction and maintenance
> of bus code at any cost.

The class variant sounds promising. Pierre what do you think?
> 
> Btw. having a look onto the naming ... the module is named "vfio_ap", the
> driver is named "vfio_ap", the bus is named "vfio_ap", the root bus device is
> named "vfio_ap" ... a bunch of vfio_aps naming different things.
> 
> regards
> Harald Freudenberger
> 

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

[Harald Freudenberger]

On 21.02.19 08:37, Christian Borntraeger wrote:
>
> On 20.02.2019 14:12, Harald Freudenberger wrote:
>> On 18.02.19 19:08, 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.
>>>
>>> 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.
>>>
>>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>  drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>>>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>  3 files changed, 43 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 31c6c84..8e45559 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>>>  
>>>  /* Only type 10 adapters (CEX4 and later) are supported
>>> @@ -62,6 +58,27 @@ 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)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static struct device_driver matrix_driver = {
>>> +	.name = "vfio_ap",
>>> +	.bus = &matrix_bus,
>>> +	.probe = matrix_probe,
>>> +};
>>> +
>>>  static int vfio_ap_matrix_dev_create(void)
>>>  {
>>>  	int ret;
>>> @@ -71,6 +88,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 +108,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);
>>> +	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);
>>>  	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;
>> You are introducing a new bus just for a user space application which is unable
>> to deal with a device directly attached to the root of devices ? So you are fixing
>> kernel code because of disability of userspace code. Hm, you are switching
>> root cause and effect. However, not my job.
> the kernel rule is pretty simple. If userspace breaks due to a kernel change fix the
> kernel.
>
>> Why do you need this dummy bus ? Did you evaluate using a "class" subsystem
>> instead ? This is very common and my assumption is that libudev is able to handle
>> this. I am using a "zcrypt" class for providing additional zcrypt device nodes and
>> this works fine together with udev. I would avoid the introduction and maintenance
>> of bus code at any cost.
> The class variant sounds promising. Pierre what do you think?
I checked that. My additional zcrypt devices (you can easily create one
with just echo "blubber" >/sys/class/zcrypt/create) and then have a look
to the new device entry in sysfs at /sys/devices/virtual/zcrypt/blubber).
Maybe you need to alloc device numbers for this - I am not sure if it is
sufficient to device_register() without a device number. However, have
a look into zcrpyt_api.c zcdn_init() and zcdn_create().
With a new class, you can also remove the root device needed as an
anchor for the old and for the bus code.
>> Btw. having a look onto the naming ... the module is named "vfio_ap", the
>> driver is named "vfio_ap", the bus is named "vfio_ap", the root bus device is
>> named "vfio_ap" ... a bunch of vfio_aps naming different things.
>>
>> regards
>> Harald Freudenberger
>>

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

[Pierre Morel]

On 21/02/2019 08:37, Christian Borntraeger wrote:
> 
> 
> On 20.02.2019 14:12, Harald Freudenberger wrote:
>> On 18.02.19 19:08, 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.
>>>
>>> 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.
>>>
>>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_drv.c     | 48 +++++++++++++++++++++++++++++------
>>>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>   3 files changed, 43 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 31c6c84..8e45559 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -24,10 +24,6 @@ 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 ap_matrix_dev *matrix_dev;
>>>   
>>>   /* Only type 10 adapters (CEX4 and later) are supported
>>> @@ -62,6 +58,27 @@ 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)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static struct device_driver matrix_driver = {
>>> +	.name = "vfio_ap",
>>> +	.bus = &matrix_bus,
>>> +	.probe = matrix_probe,
>>> +};
>>> +
>>>   static int vfio_ap_matrix_dev_create(void)
>>>   {
>>>   	int ret;
>>> @@ -71,6 +88,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 +108,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);
>>> +	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);
>>>   	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;
>>
>> You are introducing a new bus just for a user space application which is unable
>> to deal with a device directly attached to the root of devices ? So you are fixing
>> kernel code because of disability of userspace code. Hm, you are switching
>> root cause and effect. However, not my job.
> 
> the kernel rule is pretty simple. If userspace breaks due to a kernel change fix the
> kernel.
> 
>>
>> Why do you need this dummy bus ? Did you evaluate using a "class" subsystem
>> instead ? This is very common and my assumption is that libudev is able to handle
>> this. I am using a "zcrypt" class for providing additional zcrypt device nodes and
>> this works fine together with udev. I would avoid the introduction and maintenance
>> of bus code at any cost.
> 
> The class variant sounds promising. Pierre what do you think?



AFAIU the device driver model, the struct class is higher level 
representation of the device used to provide the informations to create 
devices in /dev.
The device and the class are associated with the call to device_create 
which send the event.

However the device passed to device_create must hang on a bus and be 
managed by a driver.

I do not see the interest of having a class if we do not plan to create 
a device in /dev.
We could add one in prevision, but it is not a replacement for the bus.

Regards,
Pierre



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