[RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

The crypto devices that we can use with the vfio_ap module are sitting
on the "ap" bus, not on the "vfio_ap" bus that the module defines
itself. With this change, the vfio_ap module now gets automatically
loaded if a supported crypto adapter is available in the host.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Note: Marked as "RFC" since I'm not 100% sure about it ...
       please review carefully!

 drivers/s390/crypto/vfio_ap_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 4d2556bc7fe5..5580e40608a4 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
 	{ /* end of sibling */ },
 };
 
-MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
+MODULE_DEVICE_TABLE(ap, ap_queue_ids);
 
 /**
  * vfio_ap_queue_dev_probe:
-- 
2.27.0

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Harald Freudenberger]

On 01.12.21 15:11, Thomas Huth wrote:
> The crypto devices that we can use with the vfio_ap module are sitting
> on the "ap" bus, not on the "vfio_ap" bus that the module defines
> itself. With this change, the vfio_ap module now gets automatically
> loaded if a supported crypto adapter is available in the host.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>        please review carefully!
>
>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 4d2556bc7fe5..5580e40608a4 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>  	{ /* end of sibling */ },
>  };
>  
> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>  
>  /**
>   * vfio_ap_queue_dev_probe:
Hello Thomas, interesting.
Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
two types of ap devices attached to the ap bus: ap cards and ap queues. This is a fair assumption as the ap bus
is the 'owner' of what's attached to the bus it maintains. But with this change and no further arrangements
in the ap bus code my feeling is ...
However, Tony is the owner of the vfio ap stuff, so maybe he has another opinion.
Harald Freudenberger

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

On 01/12/2021 18.10, Harald Freudenberger wrote:
> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>         please review carefully!
>>
>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>   	{ /* end of sibling */ },
>>   };
>>   
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>   
>>   /**
>>    * vfio_ap_queue_dev_probe:
> Hello Thomas, interesting.
> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?

Yes, I've tested it. Without the patch, the vfio_ap module does not get 
loaded automatically if a crypto card is available. With the patch applied, 
the vfio_ap module correctly gets loaded automatically on my system (similar 
to the vfio_ccw module).

> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
> two types of ap devices attached to the ap bus: ap cards and ap queues.

This is only about getting the module loaded automatically once such a 
device is available ... AFAIK it does not grab any of the devices 
automatically, so there shouldn't be any problems?

  Thomas

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Harald Freudenberger]

On 02.12.21 08:13, Thomas Huth wrote:
> On 01/12/2021 18.10, Harald Freudenberger wrote:
>> On 01.12.21 15:11, Thomas Huth wrote:
>>> The crypto devices that we can use with the vfio_ap module are sitting
>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>> itself. With this change, the vfio_ap module now gets automatically
>>> loaded if a supported crypto adapter is available in the host.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>         please review carefully!
>>>
>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 4d2556bc7fe5..5580e40608a4 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>       { /* end of sibling */ },
>>>   };
>>>   -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>     /**
>>>    * vfio_ap_queue_dev_probe:
>> Hello Thomas, interesting.
>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>
> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>
>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>
> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>
>  Thomas
>
Yes, of course for the automatic module load works this way. But you understand that now
the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
dangling on the ap bus. So you should test what happens when there are real vfio ap devices
in use together with 'regular' ap card and queue devices.

However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
of customers will never use vfio ap devices - this is specific to kvm hosts only. I think this has to be
decided by Tony and maybe some kvm architect. If there is an agreement, I will try to rework the
ap code to be able to deal with foreign devices attached to the ap bus.

So thanks for your investigations. Let's wait for Tony and see how we proceed.

Harald
Harald

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/2/21 03:33, Harald Freudenberger wrote:
> On 02.12.21 08:13, Thomas Huth wrote:
>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>> itself. With this change, the vfio_ap module now gets automatically
>>>> loaded if a supported crypto adapter is available in the host.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>          please review carefully!
>>>>
>>>>    drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>        { /* end of sibling */ },
>>>>    };
>>>>    -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>      /**
>>>>     * vfio_ap_queue_dev_probe:
>>> Hello Thomas, interesting.
>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>
>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>
>>   Thomas
>>
> Yes, of course for the automatic module load works this way. But you understand that now
> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
> in use together with 'regular' ap card and queue devices.
>
> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
> of customers will never use vfio ap devices - this is specific to kvm hosts only. I think this has to be
> decided by Tony and maybe some kvm architect. If there is an agreement, I will try to rework the
> ap code to be able to deal with foreign devices attached to the ap bus.
>
> So thanks for your investigations. Let's wait for Tony and see how we proceed.

I'll have a look at this, but I won't be able to get to it until Monday 
of next week.

>
> Harald
> Harald

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

On 02/12/2021 09.33, Harald Freudenberger wrote:
> On 02.12.21 08:13, Thomas Huth wrote:
>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>> itself. With this change, the vfio_ap module now gets automatically
>>>> loaded if a supported crypto adapter is available in the host.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>          please review carefully!
>>>>
>>>>    drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>        { /* end of sibling */ },
>>>>    };
>>>>    -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>      /**
>>>>     * vfio_ap_queue_dev_probe:
>>> Hello Thomas, interesting.
>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>>
>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>
>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>>
>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>
>>   Thomas
>>
> Yes, of course for the automatic module load works this way. But you understand that now
> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
> in use together with 'regular' ap card and queue devices.

I pondered about this for a while, but I still do not quite understand. The 
MODULE_DEVICE_TABLE macro only adds a __mod_something_device_table symbol to 
the module, it does not change the hierarchy of the vfio devices ... so this 
is really only about loading the module automatically. Or do you say that 
there is already a problem if a user loads the module manually and thus it 
should not get loaded automatically?

> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
> of customers will never use vfio ap devices - this is specific to kvm hosts only.
vfio-ccw also gets loaded automatically via MODULE_DEVICE_TABLE, so I think 
vfio-ap should be handled the same way.
(Or should we maybe rather remove the MODULE_DEVICE_TABLE line from both 
modules instead?)

  Thomas

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Cornelia Huck]

On Wed, Dec 08 2021, Thomas Huth <thuth@redhat.com> wrote:

> On 02/12/2021 09.33, Harald Freudenberger wrote:
>> On 02.12.21 08:13, Thomas Huth wrote:
>>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>>> itself. With this change, the vfio_ap module now gets automatically
>>>>> loaded if a supported crypto adapter is available in the host.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>>          please review carefully!
>>>>>
>>>>>    drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>>        { /* end of sibling */ },
>>>>>    };
>>>>>    -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>>      /**
>>>>>     * vfio_ap_queue_dev_probe:
>>>> Hello Thomas, interesting.
>>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>>>
>>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>>
>>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>>>
>>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>>
>>>   Thomas
>>>
>> Yes, of course for the automatic module load works this way. But you understand that now
>> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
>> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
>> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
>> in use together with 'regular' ap card and queue devices.

Um, the queues/cards are devices on the bus, and just can have
different drivers bound to them, right? The only device that the vfio-ap
driver creates is the matrix device (which does not live on the ap bus),
and this patch doesn't change that. It only correctly creates a table
for a driver that already matched on the ap bus.

>
> I pondered about this for a while, but I still do not quite understand. The 
> MODULE_DEVICE_TABLE macro only adds a __mod_something_device_table symbol to 
> the module, it does not change the hierarchy of the vfio devices ... so this 
> is really only about loading the module automatically. Or do you say that 
> there is already a problem if a user loads the module manually and thus it 
> should not get loaded automatically?

Correct me if I'm wrong, but don't the devices on the ap bus need to be
actually configured before they can attach to a non-default
(i.e. vfio-ap) driver? IOW, it's not a simple bind operation, but extra
configuration is required, so a loaded vfio-ap module should not affect
any devices not configured to actually use it at all.

>
>> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
>> of customers will never use vfio ap devices - this is specific to kvm hosts only.
> vfio-ccw also gets loaded automatically via MODULE_DEVICE_TABLE, so I think 
> vfio-ap should be handled the same way.
> (Or should we maybe rather remove the MODULE_DEVICE_TABLE line from both 
> modules instead?)

MODULE_DEVICE_TABLE declares "I can drive these devices", so it doesn't
feel correct to remove them. If the modules should not be autoloaded,
the system must be configured to not autoload them.

Besides, is loading an extra module really causing that much harm? Does
vfio-ap drag in too much other stuff?

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Harald Freudenberger]

On 01.12.21 15:11, Thomas Huth wrote:
> The crypto devices that we can use with the vfio_ap module are sitting
> on the "ap" bus, not on the "vfio_ap" bus that the module defines
> itself. With this change, the vfio_ap module now gets automatically
> loaded if a supported crypto adapter is available in the host.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>        please review carefully!
>
>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 4d2556bc7fe5..5580e40608a4 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>  	{ /* end of sibling */ },
>  };
>  
> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>  
>  /**
>   * vfio_ap_queue_dev_probe:
I had a chance to check this now.
First I have to apologize about the dispute with vfio devices appearing on the ap bus.
That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
belongs to the ap bus anyway.
So what's left is that with this change the vfio_ap kernel module is automatically loaded
when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
is fulfilled.
Yet another kernel module which may occupy memory but will never get used by most customers.
This may not be a problem but I had a glance at the list of kernel modules loaded on my
LPAR with and without the patch and the difference is:
...
kvm                   512000  1 vfio_ap
vfio_ap                28672  0
...
So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
Do I need to say something more?

If this dependency is removed then I would not hesitate to accept this patch. However
this is up to Tony as he is the maintainer of the vfio ap device driver.

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Cornelia Huck]

On Mon, Dec 13 2021, Harald Freudenberger <freude@linux.ibm.com> wrote:

> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>>        please review carefully!
>>
>>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>  	{ /* end of sibling */ },
>>  };
>>  
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>  
>>  /**
>>   * vfio_ap_queue_dev_probe:
> I had a chance to check this now.
> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
> belongs to the ap bus anyway.
> So what's left is that with this change the vfio_ap kernel module is automatically loaded
> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
> is fulfilled.
> Yet another kernel module which may occupy memory but will never get used by most customers.
> This may not be a problem but I had a glance at the list of kernel modules loaded on my
> LPAR with and without the patch and the difference is:
> ...
> kvm                   512000  1 vfio_ap
> vfio_ap                28672  0
> ...
> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
> Do I need to say something more?
>
> If this dependency is removed then I would not hesitate to accept this patch. However
> this is up to Tony as he is the maintainer of the vfio ap device driver.

I don't think you can drop the kvm reference, as the code in vfio-ap
obviously depends on it...

One possibility is simply blocking autoload of the module in userspace by
default, and only allow it to be loaded automatically when e.g. qemu-kvm
is installed on the system. This is obviously something that needs to be
decided by the distros.

(kvm might actually be autoloaded already, so autoloading vfio-ap would
not really make it worse.)

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/13/21 10:44, Harald Freudenberger wrote:
> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>         please review carefully!
>>
>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>   	{ /* end of sibling */ },
>>   };
>>   
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>   
>>   /**
>>    * vfio_ap_queue_dev_probe:
> I had a chance to check this now.
> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
> belongs to the ap bus anyway.
> So what's left is that with this change the vfio_ap kernel module is automatically loaded
> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
> is fulfilled.
> Yet another kernel module which may occupy memory but will never get used by most customers.
> This may not be a problem but I had a glance at the list of kernel modules loaded on my
> LPAR with and without the patch and the difference is:
> ...
> kvm                   512000  1 vfio_ap
> vfio_ap                28672  0
> ...
> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
> Do I need to say something more?
>
> If this dependency is removed then I would not hesitate to accept this patch. However
> this is up to Tony as he is the maintainer of the vfio ap device driver.

The vfio_ap device driver has a dependency on kvm, it can not be removed.
If the user base for vfio_ap is minimal, then I see no reason why the 
vfio_ap
module should be automatically loaded when an AP device type 10-13 is
recognized by the AP bus. The module is needed only to pass through AP
queue devices to a KVM guest.

>
>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/13/21 11:11, Cornelia Huck wrote:
> On Mon, Dec 13 2021, Harald Freudenberger <freude@linux.ibm.com> wrote:
>
>> On 01.12.21 15:11, Thomas Huth wrote:
>>> The crypto devices that we can use with the vfio_ap module are sitting
>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>> itself. With this change, the vfio_ap module now gets automatically
>>> loaded if a supported crypto adapter is available in the host.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>         please review carefully!
>>>
>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 4d2556bc7fe5..5580e40608a4 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>   	{ /* end of sibling */ },
>>>   };
>>>   
>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>   
>>>   /**
>>>    * vfio_ap_queue_dev_probe:
>> I had a chance to check this now.
>> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
>> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
>> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
>> belongs to the ap bus anyway.
>> So what's left is that with this change the vfio_ap kernel module is automatically loaded
>> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
>> is fulfilled.
>> Yet another kernel module which may occupy memory but will never get used by most customers.
>> This may not be a problem but I had a glance at the list of kernel modules loaded on my
>> LPAR with and without the patch and the difference is:
>> ...
>> kvm                   512000  1 vfio_ap
>> vfio_ap                28672  0
>> ...
>> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
>> Do I need to say something more?
>>
>> If this dependency is removed then I would not hesitate to accept this patch. However
>> this is up to Tony as he is the maintainer of the vfio ap device driver.
> I don't think you can drop the kvm reference, as the code in vfio-ap
> obviously depends on it...
>
> One possibility is simply blocking autoload of the module in userspace by
> default, and only allow it to be loaded automatically when e.g. qemu-kvm
> is installed on the system. This is obviously something that needs to be
> decided by the distros.
>
> (kvm might actually be autoloaded already, so autoloading vfio-ap would
> not really make it worse.)

Of the vfio_ccw module is automatically loaded, then the kvm
module will also get loaded. I startup up a RHEL8.3 system and
sure enough, the vfio_ccw module is loaded along with the
kvm, vfio and mdev modules. If this is true for all distros, then
it wouldn't make much difference if the vfio_ap module is
autoloaded too.

>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

On 14/12/2021 22.55, Tony Krowiak wrote:
> 
> 
> On 12/13/21 11:11, Cornelia Huck wrote:
>> On Mon, Dec 13 2021, Harald Freudenberger <freude@linux.ibm.com> wrote:
>>
>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>> itself. With this change, the vfio_ap module now gets automatically
>>>> loaded if a supported crypto adapter is available in the host.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>         please review carefully!
>>>>
>>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>>>> b/drivers/s390/crypto/vfio_ap_drv.c
>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>       { /* end of sibling */ },
>>>>   };
>>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>   /**
>>>>    * vfio_ap_queue_dev_probe:
>>> I had a chance to check this now.
>>> First I have to apologize about the dispute with vfio devices appearing 
>>> on the ap bus.
>>> That's not the case with this patch. As Connie states the 
>>> MODULE_DEVICE_TABLE() does not
>>> change the parent of a device and vfio_ap_drv is a driver for ap devices 
>>> and thus
>>> belongs to the ap bus anyway.
>>> So what's left is that with this change the vfio_ap kernel module is 
>>> automatically loaded
>>> when an ap device type 10-13 is recognized by the ap bus. So the 
>>> intention of the patch
>>> is fulfilled.
>>> Yet another kernel module which may occupy memory but will never get used 
>>> by most customers.
>>> This may not be a problem but I had a glance at the list of kernel 
>>> modules loaded on my
>>> LPAR with and without the patch and the difference is:
>>> ...
>>> kvm                   512000  1 vfio_ap
>>> vfio_ap                28672  0
>>> ...
>>> So the vfio_ap module has a dependency to the biggest kernel module ever 
>>> - kvm.
>>> Do I need to say something more?
>>>
>>> If this dependency is removed then I would not hesitate to accept this 
>>> patch. However
>>> this is up to Tony as he is the maintainer of the vfio ap device driver.
>> I don't think you can drop the kvm reference, as the code in vfio-ap
>> obviously depends on it...
>>
>> One possibility is simply blocking autoload of the module in userspace by
>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>> is installed on the system. This is obviously something that needs to be
>> decided by the distros.
>>
>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>> not really make it worse.)
> 
> Of the vfio_ccw module is automatically loaded, then the kvm
> module will also get loaded. I startup up a RHEL8.3 system and
> sure enough, the vfio_ccw module is loaded along with the
> kvm, vfio and mdev modules. If this is true for all distros, then
> it wouldn't make much difference if the vfio_ap module is
> autoloaded too.

I think I don't mind too much if we auto-load vfio-ap or not - but I think 
we should make it consistent with vfio-ccw. So either auto-load both modules 
(if the corresponding devices are available), or remove the 
MODULE_DEVICE_TABLE() entries from both modules?

  Thomas

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Cornelia Huck]

On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:

> On 14/12/2021 22.55, Tony Krowiak wrote:
>> 
>> 
>> On 12/13/21 11:11, Cornelia Huck wrote:
>>> One possibility is simply blocking autoload of the module in userspace by
>>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>>> is installed on the system. This is obviously something that needs to be
>>> decided by the distros.
>>>
>>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>>> not really make it worse.)
>> 
>> Of the vfio_ccw module is automatically loaded, then the kvm
>> module will also get loaded. I startup up a RHEL8.3 system and
>> sure enough, the vfio_ccw module is loaded along with the
>> kvm, vfio and mdev modules. If this is true for all distros, then
>> it wouldn't make much difference if the vfio_ap module is
>> autoloaded too.
>
> I think I don't mind too much if we auto-load vfio-ap or not - but I think 
> we should make it consistent with vfio-ccw. So either auto-load both modules 
> (if the corresponding devices are available), or remove the 
> MODULE_DEVICE_TABLE() entries from both modules?

I think we really need to take a step back and think about the purpose
of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
of devices on a certain bus a driver supports, in a way that can be
consumed by userspace (after file2alias.c worked on it).

Userspace typically uses this to match devices it is notified about to
drivers that could possibly drive those devices. In general, the
assumption is that you will want to have the drivers for your devices
loaded. In some cases (drivers only used in special cases, like here),
it might be a better idea to autoload the drivers only under certain
circumstances (e.g. if you know you're going to run KVM guests).

My main point, however, is that we're talking about policy here: whether
a potentially useful driver should be loaded or not is a decision that
should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
not look like the right solution, as it deprives userspace of the
information to autoload the driver, if it actually wants to do so.

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

On 15/12/2021 13.51, Cornelia Huck wrote:
> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 14/12/2021 22.55, Tony Krowiak wrote:
>>>
>>>
>>> On 12/13/21 11:11, Cornelia Huck wrote:
>>>> One possibility is simply blocking autoload of the module in userspace by
>>>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>>>> is installed on the system. This is obviously something that needs to be
>>>> decided by the distros.
>>>>
>>>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>>>> not really make it worse.)
>>>
>>> Of the vfio_ccw module is automatically loaded, then the kvm
>>> module will also get loaded. I startup up a RHEL8.3 system and
>>> sure enough, the vfio_ccw module is loaded along with the
>>> kvm, vfio and mdev modules. If this is true for all distros, then
>>> it wouldn't make much difference if the vfio_ap module is
>>> autoloaded too.
>>
>> I think I don't mind too much if we auto-load vfio-ap or not - but I think
>> we should make it consistent with vfio-ccw. So either auto-load both modules
>> (if the corresponding devices are available), or remove the
>> MODULE_DEVICE_TABLE() entries from both modules?
> 
> I think we really need to take a step back and think about the purpose
> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
> of devices on a certain bus a driver supports, in a way that can be
> consumed by userspace (after file2alias.c worked on it).
> 
> Userspace typically uses this to match devices it is notified about to
> drivers that could possibly drive those devices. In general, the
> assumption is that you will want to have the drivers for your devices
> loaded. In some cases (drivers only used in special cases, like here),
> it might be a better idea to autoload the drivers only under certain
> circumstances (e.g. if you know you're going to run KVM guests).
> 
> My main point, however, is that we're talking about policy here: whether
> a potentially useful driver should be loaded or not is a decision that
> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
> not look like the right solution, as it deprives userspace of the
> information to autoload the driver, if it actually wants to do so.

Thanks, Cornelia, that's a very valid point, I didn't see it from this 
perspective yet, but it sounds like the right way to handle this. So I think 
my patch should be applied, and if we then agree that the module should not 
be auto-loaded by default, it should be handled with the appropriate changes 
in userspace instead.

  Thomas

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Halil Pasic]

On Wed, 15 Dec 2021 13:51:02 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 14/12/2021 22.55, Tony Krowiak wrote:  
> >> 
> >> 
> >> On 12/13/21 11:11, Cornelia Huck wrote:  
> >>> One possibility is simply blocking autoload of the module in userspace by
> >>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
> >>> is installed on the system. This is obviously something that needs to be
> >>> decided by the distros.
> >>>
> >>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
> >>> not really make it worse.)  
> >> 
> >> Of the vfio_ccw module is automatically loaded, then the kvm
> >> module will also get loaded. I startup up a RHEL8.3 system and
> >> sure enough, the vfio_ccw module is loaded along with the
> >> kvm, vfio and mdev modules. If this is true for all distros, then
> >> it wouldn't make much difference if the vfio_ap module is
> >> autoloaded too.  
> >
> > I think I don't mind too much if we auto-load vfio-ap or not - but I think 
> > we should make it consistent with vfio-ccw. So either auto-load both modules 
> > (if the corresponding devices are available), or remove the 
> > MODULE_DEVICE_TABLE() entries from both modules?  
> 
> I think we really need to take a step back and think about the purpose
> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
> of devices on a certain bus a driver supports, in a way that can be
> consumed by userspace (after file2alias.c worked on it).

I did a quick search to locate where this semantic was codified. But
I didn't find the place neither Documentation/ nor in the header where
MODULE_DEVICE_TABLE is defined.

> 
> Userspace typically uses this to match devices it is notified about to
> drivers that could possibly drive those devices. In general, the
> assumption is that you will want to have the drivers for your devices
> loaded. In some cases (drivers only used in special cases, like here),
> it might be a better idea to autoload the drivers only under certain
> circumstances (e.g. if you know you're going to run KVM guests).

Does RHEL do this, or would RHEL do this out of the box? I.e.
would we end up preserving old behavior when this fix hits the distro,
or would the end user end up with kvm and vfio_ap loaded (out of the
box)?

What would be the mechanism of choice to implement if kvm loaded and
APs present/hotplugged load vfio_ap, otherwise don't in the userspace?

Sorry I'm not very familiar with this whole modules auto-loading
business, so I may be asking obvious questions. But a quick google
search did not help me.

> 
> My main point, however, is that we're talking about policy here: whether
> a potentially useful driver should be loaded or not is a decision that
> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
> not look like the right solution, as it deprives userspace of the
> information to autoload the driver, if it actually wants to do so.
> 

I'm sympathetic to this reading of the situation, but I'm not sure
it is as black and white as stated.

I think the current state of affairs in the vfio_ap module is clearly a
bug.

One can argue that not auto-loading vfio_ap and kvm per default out of
the box is not a bug. I mean the tooling (chzdev) seems to do fine
without this and just assuming that both kvm and vfio_ap will be needed
just because we have APs seems wrong.

One of the more important guiding principles of Linux kernel development
is no userspace regressions. And I think suddenly getting vfio_ap and kvm
loaded just because we have AP devices can be thought of as a regression.

So I'm sympathetic to Harald's view as well.

Of course there is the solution that the distros should really make sure
the old behavior is preserved, or some smart behavior is introduced. But
regarding smart, I believe "if you have devices that are configured for
vfio_ap pass-through (with chzdev), then the vfio_ap module should get
loaded" is pretty much as smart as it gets. So blacklisting the module
by default in the distros looks like a viable option. If that is what
we want, we should probably ask the distros, because I don't think
it is just obviously their job to figure out that they have to do so.

Disclaimer: I might be wrong about the current behavior, I didn't verify
my claims

Also what does vfio-pci do? As far as I can tell vfio-pci does not
participate in module auto loading just because there are pci devices.
The have some smart override I don't quite understand:
https://patchwork.ozlabs.org/project/linux-pci/patch/20210826103912.128972-11-yishaih@nvidia.com/
Before, I don't think they had a MODULE_DEVICE_TABLE:
https://elixir.bootlin.com/linux/v5.8.18/source/drivers/vfio/pci/vfio_pci.c

Regards,
Halil 

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Harald Freudenberger]

On 13.12.21 16:44, Harald Freudenberger wrote:
> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>>        please review carefully!
>>
>>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>  	{ /* end of sibling */ },
>>  };
>>  
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>  
>>  /**
>>   * vfio_ap_queue_dev_probe:
> I had a chance to check this now.
> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
> belongs to the ap bus anyway.
> So what's left is that with this change the vfio_ap kernel module is automatically loaded
> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
> is fulfilled.
> Yet another kernel module which may occupy memory but will never get used by most customers.
> This may not be a problem but I had a glance at the list of kernel modules loaded on my
> LPAR with and without the patch and the difference is:
> ...
> kvm                   512000  1 vfio_ap
> vfio_ap                28672  0
> ...
> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
> Do I need to say something more?
>
> If this dependency is removed then I would not hesitate to accept this patch. However
> this is up to Tony as he is the maintainer of the vfio ap device driver.
>
>
I need to throw in another point: with building initrd with dracut the kernel module
dependencies are evaluated. As of now this means that the zcrypt device driver
zoo is required in case you need to have crypto support for an encrypted root or
data disk at boot time. With vfio ap driver dependency enforced as requirement
from the AP bus there also comes the dependency to the kvm kernel module.
So the kvm kernel module needs to be part of the initrd now. I am not sure if this
is desired.

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Cornelia Huck]

On Thu, Dec 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 15 Dec 2021 13:51:02 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
>> 
>> > On 14/12/2021 22.55, Tony Krowiak wrote:  
>> >> 
>> >> 
>> >> On 12/13/21 11:11, Cornelia Huck wrote:  
>> >>> One possibility is simply blocking autoload of the module in userspace by
>> >>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>> >>> is installed on the system. This is obviously something that needs to be
>> >>> decided by the distros.
>> >>>
>> >>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>> >>> not really make it worse.)  
>> >> 
>> >> Of the vfio_ccw module is automatically loaded, then the kvm
>> >> module will also get loaded. I startup up a RHEL8.3 system and
>> >> sure enough, the vfio_ccw module is loaded along with the
>> >> kvm, vfio and mdev modules. If this is true for all distros, then
>> >> it wouldn't make much difference if the vfio_ap module is
>> >> autoloaded too.  
>> >
>> > I think I don't mind too much if we auto-load vfio-ap or not - but I think 
>> > we should make it consistent with vfio-ccw. So either auto-load both modules 
>> > (if the corresponding devices are available), or remove the 
>> > MODULE_DEVICE_TABLE() entries from both modules?  
>> 
>> I think we really need to take a step back and think about the purpose
>> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
>> of devices on a certain bus a driver supports, in a way that can be
>> consumed by userspace (after file2alias.c worked on it).
>
> I did a quick search to locate where this semantic was codified. But
> I didn't find the place neither Documentation/ nor in the header where
> MODULE_DEVICE_TABLE is defined.

This is rather ancient code pre-dating git; the mechanism as it is now
came in during the 2.5 device model redesign and introduction of udev,
IIRC.

>
>> 
>> Userspace typically uses this to match devices it is notified about to
>> drivers that could possibly drive those devices. In general, the
>> assumption is that you will want to have the drivers for your devices
>> loaded. In some cases (drivers only used in special cases, like here),
>> it might be a better idea to autoload the drivers only under certain
>> circumstances (e.g. if you know you're going to run KVM guests).
>
> Does RHEL do this, or would RHEL do this out of the box? I.e.
> would we end up preserving old behavior when this fix hits the distro,
> or would the end user end up with kvm and vfio_ap loaded (out of the
> box)?
>
> What would be the mechanism of choice to implement if kvm loaded and
> APs present/hotplugged load vfio_ap, otherwise don't in the userspace?
>
> Sorry I'm not very familiar with this whole modules auto-loading
> business, so I may be asking obvious questions. But a quick google
> search did not help me.

See /usr/lib/udev/rules.d/80-drivers.rules -- modules are automatically
loaded based on the alias when a device is added. Autoload can be also
be prevented; see the 'blacklist' directive in man 5 modprobe.d, which
will basically prevent autoloading based on the module-based aliases
(you will have to load the modules directly instead.) This is likely to
be the case on other distros as well.

An option would be to e.g. add a 'blacklist' statement for vfio-ap, and
have the qemu-kvm package install a udev rule that loads vfio-ap if any
supported ap devices are added.

>
>> 
>> My main point, however, is that we're talking about policy here: whether
>> a potentially useful driver should be loaded or not is a decision that
>> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
>> not look like the right solution, as it deprives userspace of the
>> information to autoload the driver, if it actually wants to do so.
>> 
>
> I'm sympathetic to this reading of the situation, but I'm not sure
> it is as black and white as stated.
>
> I think the current state of affairs in the vfio_ap module is clearly a
> bug.

Yes, the current code is simply wrong.

>
> One can argue that not auto-loading vfio_ap and kvm per default out of
> the box is not a bug. I mean the tooling (chzdev) seems to do fine
> without this and just assuming that both kvm and vfio_ap will be needed
> just because we have APs seems wrong.
>
> One of the more important guiding principles of Linux kernel development
> is no userspace regressions. And I think suddenly getting vfio_ap and kvm
> loaded just because we have AP devices can be thought of as a regression.

I disagree, that does not have anything to do with regressions.

>
> So I'm sympathetic to Harald's view as well.
>
> Of course there is the solution that the distros should really make sure
> the old behavior is preserved, or some smart behavior is introduced. But
> regarding smart, I believe "if you have devices that are configured for
> vfio_ap pass-through (with chzdev), then the vfio_ap module should get
> loaded" is pretty much as smart as it gets. So blacklisting the module
> by default in the distros looks like a viable option. If that is what
> we want, we should probably ask the distros, because I don't think
> it is just obviously their job to figure out that they have to do so.
>

I don't think the *zdev tools are the right place to control this in
general, I'd prefer to do it with generic tools (see above). But the
most important part is to configure this in userspace (and just fix the
kernel). Nothing prevents anyone from doing their own thing :)

> Disclaimer: I might be wrong about the current behavior, I didn't verify
> my claims
>
> Also what does vfio-pci do? As far as I can tell vfio-pci does not
> participate in module auto loading just because there are pci devices.
> The have some smart override I don't quite understand:
> https://patchwork.ozlabs.org/project/linux-pci/patch/20210826103912.128972-11-yishaih@nvidia.com/
> Before, I don't think they had a MODULE_DEVICE_TABLE:
> https://elixir.bootlin.com/linux/v5.8.18/source/drivers/vfio/pci/vfio_pci.c

I don't think it makes sense to look at pci for inspiration here; they
have a myriad of different device types, while ap only has a few (and
probably not that many different ones on a given system), and css only
has one that really matters.

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Cornelia Huck]

On Thu, Dec 16 2021, Harald Freudenberger <freude@linux.ibm.com> wrote:

> On 13.12.21 16:44, Harald Freudenberger wrote:
>> On 01.12.21 15:11, Thomas Huth wrote:
>>> The crypto devices that we can use with the vfio_ap module are sitting
>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>> itself. With this change, the vfio_ap module now gets automatically
>>> loaded if a supported crypto adapter is available in the host.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>        please review carefully!
>>>
>>>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 4d2556bc7fe5..5580e40608a4 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>  	{ /* end of sibling */ },
>>>  };
>>>  
>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>  
>>>  /**
>>>   * vfio_ap_queue_dev_probe:
>> I had a chance to check this now.
>> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
>> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
>> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
>> belongs to the ap bus anyway.
>> So what's left is that with this change the vfio_ap kernel module is automatically loaded
>> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
>> is fulfilled.
>> Yet another kernel module which may occupy memory but will never get used by most customers.
>> This may not be a problem but I had a glance at the list of kernel modules loaded on my
>> LPAR with and without the patch and the difference is:
>> ...
>> kvm                   512000  1 vfio_ap
>> vfio_ap                28672  0
>> ...
>> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
>> Do I need to say something more?
>>
>> If this dependency is removed then I would not hesitate to accept this patch. However
>> this is up to Tony as he is the maintainer of the vfio ap device driver.
>>
>>
> I need to throw in another point: with building initrd with dracut the kernel module
> dependencies are evaluated. As of now this means that the zcrypt device driver
> zoo is required in case you need to have crypto support for an encrypted root or
> data disk at boot time. With vfio ap driver dependency enforced as requirement
> from the AP bus there also comes the dependency to the kvm kernel module.
> So the kvm kernel module needs to be part of the initrd now. I am not sure if this
> is desired.

Again, this simply means that the vfio-ap device needs to be
blacklisted. Not sure if building the initrd is also honouring the stuff
mentioned in man 5 modprobe.d, I'm not an expert there. But that is
nothing that the kernel should decide.

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Halil Pasic]

On Thu, 16 Dec 2021 11:39:13 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> > Also what does vfio-pci do? As far as I can tell vfio-pci does not
> > participate in module auto loading just because there are pci devices.
> > The have some smart override I don't quite understand:
> > https://patchwork.ozlabs.org/project/linux-pci/patch/20210826103912.128972-11-yishaih@nvidia.com/
> > Before, I don't think they had a MODULE_DEVICE_TABLE:
> > https://elixir.bootlin.com/linux/v5.8.18/source/drivers/vfio/pci/vfio_pci.c  
> 
> I don't think it makes sense to look at pci for inspiration here; they
> have a myriad of different device types, while ap only has a few (and
> probably not that many different ones on a given system), and css only
> has one that really matters.

I really don't understand how they having a myraid of different device
types is relevant here. Are they not taking care of that by using
PCI_ANY_ID anyway? Can you please explain?

Please have another look at the commit message of 
cc6711b0bf36 ("PCI / VFIO: Add 'override_only' support for VFIO PCI sub system").
They generate alias(es) that catch every PCI device (via PCI_ANY_ID), but
in a way that precludes auto-loading and enables a generic 'override'
algorithm.

Regards,
Halil

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Thomas Huth]

On 14/12/2021 22.28, Tony Krowiak wrote:
> 
> 
> On 12/13/21 10:44, Harald Freudenberger wrote:
>> On 01.12.21 15:11, Thomas Huth wrote:
>>> The crypto devices that we can use with the vfio_ap module are sitting
>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>> itself. With this change, the vfio_ap module now gets automatically
>>> loaded if a supported crypto adapter is available in the host.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>         please review carefully!
>>>
>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>>> b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 4d2556bc7fe5..5580e40608a4 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>       { /* end of sibling */ },
>>>   };
>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>   /**
>>>    * vfio_ap_queue_dev_probe:
>> I had a chance to check this now.
>> First I have to apologize about the dispute with vfio devices appearing on 
>> the ap bus.
>> That's not the case with this patch. As Connie states the 
>> MODULE_DEVICE_TABLE() does not
>> change the parent of a device and vfio_ap_drv is a driver for ap devices 
>> and thus
>> belongs to the ap bus anyway.
>> So what's left is that with this change the vfio_ap kernel module is 
>> automatically loaded
>> when an ap device type 10-13 is recognized by the ap bus. So the intention 
>> of the patch
>> is fulfilled.
>> Yet another kernel module which may occupy memory but will never get used 
>> by most customers.
>> This may not be a problem but I had a glance at the list of kernel modules 
>> loaded on my
>> LPAR with and without the patch and the difference is:
>> ...
>> kvm                   512000  1 vfio_ap
>> vfio_ap                28672  0
>> ...
>> So the vfio_ap module has a dependency to the biggest kernel module ever - 
>> kvm.
>> Do I need to say something more?
>>
>> If this dependency is removed then I would not hesitate to accept this 
>> patch. However
>> this is up to Tony as he is the maintainer of the vfio ap device driver.
> 
> The vfio_ap device driver has a dependency on kvm, it can not be removed.
> If the user base for vfio_ap is minimal, then I see no reason why the vfio_ap
> module should be automatically loaded when an AP device type 10-13 is
> recognized by the AP bus. The module is needed only to pass through AP
> queue devices to a KVM guest.

To continue the discussion here - it seems like my patch here won't be 
accepted? Shall I send another one instead to remove the bad 
MODLE_DEVICE_TABLE from the vfio_ap_drv.c file?

  Thomas

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/2/21 03:33, Harald Freudenberger wrote:
> On 02.12.21 08:13, Thomas Huth wrote:
>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>> itself. With this change, the vfio_ap module now gets automatically
>>>> loaded if a supported crypto adapter is available in the host.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>          please review carefully!
>>>>
>>>>    drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>        { /* end of sibling */ },
>>>>    };
>>>>    -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>      /**
>>>>     * vfio_ap_queue_dev_probe:
>>> Hello Thomas, interesting.
>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>
>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>
>>   Thomas
>>
> Yes, of course for the automatic module load works this way. But you understand that now
> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
> in use together with 'regular' ap card and queue devices.
>
> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
> of customers will never use vfio ap devices - this is specific to kvm hosts only. I think this has to be
> decided by Tony and maybe some kvm architect. If there is an agreement, I will try to rework the
> ap code to be able to deal with foreign devices attached to the ap bus.

Can you please explain how foreign devices can get attached to the bus 
if the vfio_ap device
driver is automatically loaded because the MODULE_DEVICE_TABLE 
specification is changed?

Regards,
Tony Krowiak

>
> So thanks for your investigations. Let's wait for Tony and see how we proceed.
>
> Harald
> Harald

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/8/21 09:25, Cornelia Huck wrote:
> On Wed, Dec 08 2021, Thomas Huth <thuth@redhat.com> wrote:
>
>> On 02/12/2021 09.33, Harald Freudenberger wrote:
>>> On 02.12.21 08:13, Thomas Huth wrote:
>>>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>>>> itself. With this change, the vfio_ap module now gets automatically
>>>>>> loaded if a supported crypto adapter is available in the host.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>     Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>>>           please review carefully!
>>>>>>
>>>>>>     drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>>>         { /* end of sibling */ },
>>>>>>     };
>>>>>>     -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>>>       /**
>>>>>>      * vfio_ap_queue_dev_probe:
>>>>> Hello Thomas, interesting.
>>>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>>>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>>>
>>>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>>>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>>>
>>>>    Thomas
>>>>
>>> Yes, of course for the automatic module load works this way. But you understand that now
>>> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
>>> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
>>> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
>>> in use together with 'regular' ap card and queue devices.
> Um, the queues/cards are devices on the bus, and just can have
> different drivers bound to them, right? The only device that the vfio-ap
> driver creates is the matrix device (which does not live on the ap bus),
> and this patch doesn't change that. It only correctly creates a table
> for a driver that already matched on the ap bus.
>
>> I pondered about this for a while, but I still do not quite understand. The
>> MODULE_DEVICE_TABLE macro only adds a __mod_something_device_table symbol to
>> the module, it does not change the hierarchy of the vfio devices ... so this
>> is really only about loading the module automatically. Or do you say that
>> there is already a problem if a user loads the module manually and thus it
>> should not get loaded automatically?
> Correct me if I'm wrong, but don't the devices on the ap bus need to be
> actually configured before they can attach to a non-default
> (i.e. vfio-ap) driver? IOW, it's not a simple bind operation, but extra
> configuration is required, so a loaded vfio-ap module should not affect
> any devices not configured to actually use it at all.

There are two bitmasks - apmask/aqmask - that identify the APQNs of the
queues to be bound to the default drivers. All others are bound to
the non-default driver (vfio_ap). So, you are correct, loading the vfio_ap
module will not affect any queue devices not configured to use it.

>
>>> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
>>> of customers will never use vfio ap devices - this is specific to kvm hosts only.
>> vfio-ccw also gets loaded automatically via MODULE_DEVICE_TABLE, so I think
>> vfio-ap should be handled the same way.
>> (Or should we maybe rather remove the MODULE_DEVICE_TABLE line from both
>> modules instead?)
> MODULE_DEVICE_TABLE declares "I can drive these devices", so it doesn't
> feel correct to remove them. If the modules should not be autoloaded,
> the system must be configured to not autoload them.
>
> Besides, is loading an extra module really causing that much harm? Does
> vfio-ap drag in too much other stuff?
>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/13/21 10:44, Harald Freudenberger wrote:
> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>         please review carefully!
>>
>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>   	{ /* end of sibling */ },
>>   };
>>   
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>   
>>   /**
>>    * vfio_ap_queue_dev_probe:
> I had a chance to check this now.
> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
> belongs to the ap bus anyway.
> So what's left is that with this change the vfio_ap kernel module is automatically loaded
> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
> is fulfilled.
> Yet another kernel module which may occupy memory but will never get used by most customers.
> This may not be a problem but I had a glance at the list of kernel modules loaded on my
> LPAR with and without the patch and the difference is:
> ...
> kvm                   512000  1 vfio_ap
> vfio_ap                28672  0
> ...
> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
> Do I need to say something more?
>
> If this dependency is removed then I would not hesitate to accept this patch. However
> this is up to Tony as he is the maintainer of the vfio ap device driver.

Since the vfio_ccw module also needs the kvm module and is automatically 
loaded,
I see no problem with automatically loading the vfio_ap module. I tested 
this patch by
running all of my regression tests successfully.

>
>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/13/21 11:11, Cornelia Huck wrote:
> On Mon, Dec 13 2021, Harald Freudenberger <freude@linux.ibm.com> wrote:
>
>> On 01.12.21 15:11, Thomas Huth wrote:
>>> The crypto devices that we can use with the vfio_ap module are sitting
>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>> itself. With this change, the vfio_ap module now gets automatically
>>> loaded if a supported crypto adapter is available in the host.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>         please review carefully!
>>>
>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 4d2556bc7fe5..5580e40608a4 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>   	{ /* end of sibling */ },
>>>   };
>>>   
>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>   
>>>   /**
>>>    * vfio_ap_queue_dev_probe:
>> I had a chance to check this now.
>> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
>> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
>> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
>> belongs to the ap bus anyway.
>> So what's left is that with this change the vfio_ap kernel module is automatically loaded
>> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
>> is fulfilled.
>> Yet another kernel module which may occupy memory but will never get used by most customers.
>> This may not be a problem but I had a glance at the list of kernel modules loaded on my
>> LPAR with and without the patch and the difference is:
>> ...
>> kvm                   512000  1 vfio_ap
>> vfio_ap                28672  0
>> ...
>> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
>> Do I need to say something more?
>>
>> If this dependency is removed then I would not hesitate to accept this patch. However
>> this is up to Tony as he is the maintainer of the vfio ap device driver.
> I don't think you can drop the kvm reference, as the code in vfio-ap
> obviously depends on it...
>
> One possibility is simply blocking autoload of the module in userspace by
> default, and only allow it to be loaded automatically when e.g. qemu-kvm
> is installed on the system. This is obviously something that needs to be
> decided by the distros.
>
> (kvm might actually be autoloaded already, so autoloading vfio-ap would
> not really make it worse.)

The vfio_ccw module, which is automatically loaded, also requires the 
kvm module,
so autoloading vfio_ap will not make much difference.

>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/15/21 07:51, Cornelia Huck wrote:
> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
>
>> On 14/12/2021 22.55, Tony Krowiak wrote:
>>>
>>> On 12/13/21 11:11, Cornelia Huck wrote:
>>>> One possibility is simply blocking autoload of the module in userspace by
>>>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>>>> is installed on the system. This is obviously something that needs to be
>>>> decided by the distros.
>>>>
>>>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>>>> not really make it worse.)
>>> Of the vfio_ccw module is automatically loaded, then the kvm
>>> module will also get loaded. I startup up a RHEL8.3 system and
>>> sure enough, the vfio_ccw module is loaded along with the
>>> kvm, vfio and mdev modules. If this is true for all distros, then
>>> it wouldn't make much difference if the vfio_ap module is
>>> autoloaded too.
>> I think I don't mind too much if we auto-load vfio-ap or not - but I think
>> we should make it consistent with vfio-ccw. So either auto-load both modules
>> (if the corresponding devices are available), or remove the
>> MODULE_DEVICE_TABLE() entries from both modules?
> I think we really need to take a step back and think about the purpose
> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
> of devices on a certain bus a driver supports, in a way that can be
> consumed by userspace (after file2alias.c worked on it).
>
> Userspace typically uses this to match devices it is notified about to
> drivers that could possibly drive those devices. In general, the
> assumption is that you will want to have the drivers for your devices
> loaded. In some cases (drivers only used in special cases, like here),
> it might be a better idea to autoload the drivers only under certain
> circumstances (e.g. if you know you're going to run KVM guests).
>
> My main point, however, is that we're talking about policy here: whether
> a potentially useful driver should be loaded or not is a decision that
> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
> not look like the right solution, as it deprives userspace of the
> information to autoload the driver, if it actually wants to do so.

I agree.

>

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 12/15/21 18:02, Halil Pasic wrote:
> On Wed, 15 Dec 2021 13:51:02 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 14/12/2021 22.55, Tony Krowiak wrote:
>>>>
>>>> On 12/13/21 11:11, Cornelia Huck wrote:
>>>>> One possibility is simply blocking autoload of the module in userspace by
>>>>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
>>>>> is installed on the system. This is obviously something that needs to be
>>>>> decided by the distros.
>>>>>
>>>>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
>>>>> not really make it worse.)
>>>> Of the vfio_ccw module is automatically loaded, then the kvm
>>>> module will also get loaded. I startup up a RHEL8.3 system and
>>>> sure enough, the vfio_ccw module is loaded along with the
>>>> kvm, vfio and mdev modules. If this is true for all distros, then
>>>> it wouldn't make much difference if the vfio_ap module is
>>>> autoloaded too.
>>> I think I don't mind too much if we auto-load vfio-ap or not - but I think
>>> we should make it consistent with vfio-ccw. So either auto-load both modules
>>> (if the corresponding devices are available), or remove the
>>> MODULE_DEVICE_TABLE() entries from both modules?
>> I think we really need to take a step back and think about the purpose
>> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
>> of devices on a certain bus a driver supports, in a way that can be
>> consumed by userspace (after file2alias.c worked on it).
> I did a quick search to locate where this semantic was codified. But
> I didn't find the place neither Documentation/ nor in the header where
> MODULE_DEVICE_TABLE is defined.
>
>> Userspace typically uses this to match devices it is notified about to
>> drivers that could possibly drive those devices. In general, the
>> assumption is that you will want to have the drivers for your devices
>> loaded. In some cases (drivers only used in special cases, like here),
>> it might be a better idea to autoload the drivers only under certain
>> circumstances (e.g. if you know you're going to run KVM guests).
> Does RHEL do this, or would RHEL do this out of the box? I.e.
> would we end up preserving old behavior when this fix hits the distro,
> or would the end user end up with kvm and vfio_ap loaded (out of the
> box)?
>
> What would be the mechanism of choice to implement if kvm loaded and
> APs present/hotplugged load vfio_ap, otherwise don't in the userspace?
>
> Sorry I'm not very familiar with this whole modules auto-loading
> business, so I may be asking obvious questions. But a quick google
> search did not help me.
>
>> My main point, however, is that we're talking about policy here: whether
>> a potentially useful driver should be loaded or not is a decision that
>> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
>> not look like the right solution, as it deprives userspace of the
>> information to autoload the driver, if it actually wants to do so.
>>
> I'm sympathetic to this reading of the situation, but I'm not sure
> it is as black and white as stated.
>
> I think the current state of affairs in the vfio_ap module is clearly a
> bug.
>
> One can argue that not auto-loading vfio_ap and kvm per default out of
> the box is not a bug. I mean the tooling (chzdev) seems to do fine
> without this and just assuming that both kvm and vfio_ap will be needed
> just because we have APs seems wrong.
>
> One of the more important guiding principles of Linux kernel development
> is no userspace regressions. And I think suddenly getting vfio_ap and kvm
> loaded just because we have AP devices can be thought of as a regression.
>
> So I'm sympathetic to Harald's view as well.
>
> Of course there is the solution that the distros should really make sure
> the old behavior is preserved, or some smart behavior is introduced. But
> regarding smart, I believe "if you have devices that are configured for
> vfio_ap pass-through (with chzdev), then the vfio_ap module should get
> loaded" is pretty much as smart as it gets. So blacklisting the module
> by default in the distros looks like a viable option. If that is what
> we want, we should probably ask the distros, because I don't think
> it is just obviously their job to figure out that they have to do so.
>
> Disclaimer: I might be wrong about the current behavior, I didn't verify
> my claims
>
> Also what does vfio-pci do?

 From vfio_pci.c:

static const struct pci_device_id vfio_pci_table[] = {
     { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_ANY_ID, PCI_ANY_ID) }, /* 
match all by default */
     {}
};

MODULE_DEVICE_TABLE(pci, vfio_pci_table);

> As far as I can tell vfio-pci does not
> participate in module auto loading just because there are pci devices.
> The have some smart override I don't quite understand:
> https://patchwork.ozlabs.org/project/linux-pci/patch/20210826103912.128972-11-yishaih@nvidia.com/
> Before, I don't think they had a MODULE_DEVICE_TABLE:
> https://elixir.bootlin.com/linux/v5.8.18/source/drivers/vfio/pci/vfio_pci.c
>
> Regards,
> Halil

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Tony Krowiak]

On 1/27/22 05:33, Thomas Huth wrote:
> On 14/12/2021 22.28, Tony Krowiak wrote:
>>
>>
>> On 12/13/21 10:44, Harald Freudenberger wrote:
>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>> itself. With this change, the vfio_ap module now gets automatically
>>>> loaded if a supported crypto adapter is available in the host.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>         please review carefully!
>>>>
>>>>   drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>>>> b/drivers/s390/crypto/vfio_ap_drv.c
>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>       { /* end of sibling */ },
>>>>   };
>>>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>   /**
>>>>    * vfio_ap_queue_dev_probe:
>>> I had a chance to check this now.
>>> First I have to apologize about the dispute with vfio devices 
>>> appearing on the ap bus.
>>> That's not the case with this patch. As Connie states the 
>>> MODULE_DEVICE_TABLE() does not
>>> change the parent of a device and vfio_ap_drv is a driver for ap 
>>> devices and thus
>>> belongs to the ap bus anyway.
>>> So what's left is that with this change the vfio_ap kernel module is 
>>> automatically loaded
>>> when an ap device type 10-13 is recognized by the ap bus. So the 
>>> intention of the patch
>>> is fulfilled.
>>> Yet another kernel module which may occupy memory but will never get 
>>> used by most customers.
>>> This may not be a problem but I had a glance at the list of kernel 
>>> modules loaded on my
>>> LPAR with and without the patch and the difference is:
>>> ...
>>> kvm                   512000  1 vfio_ap
>>> vfio_ap                28672  0
>>> ...
>>> So the vfio_ap module has a dependency to the biggest kernel module 
>>> ever - kvm.
>>> Do I need to say something more?
>>>
>>> If this dependency is removed then I would not hesitate to accept 
>>> this patch. However
>>> this is up to Tony as he is the maintainer of the vfio ap device 
>>> driver.
>>
>> The vfio_ap device driver has a dependency on kvm, it can not be 
>> removed.
>> If the user base for vfio_ap is minimal, then I see no reason why the 
>> vfio_ap
>> module should be automatically loaded when an AP device type 10-13 is
>> recognized by the AP bus. The module is needed only to pass through AP
>> queue devices to a KVM guest.
>
> To continue the discussion here - it seems like my patch here won't be 
> accepted? Shall I send another one instead to remove the bad 
> MODLE_DEVICE_TABLE from the vfio_ap_drv.c file?
>
>  Thomas

After re-reviewing all of the comments, I am okay with this patch:

Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>

If there are any other objections, speak now or forever hold your peace:)

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Halil Pasic]

On Thu, 27 Jan 2022 10:04:46 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 12/15/21 18:02, Halil Pasic wrote:
> > On Wed, 15 Dec 2021 13:51:02 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Wed, Dec 15 2021, Thomas Huth <thuth@redhat.com> wrote:
> >>  
> >>> On 14/12/2021 22.55, Tony Krowiak wrote:  
> >>>>
> >>>> On 12/13/21 11:11, Cornelia Huck wrote:  
> >>>>> One possibility is simply blocking autoload of the module in userspace by
> >>>>> default, and only allow it to be loaded automatically when e.g. qemu-kvm
> >>>>> is installed on the system. This is obviously something that needs to be
> >>>>> decided by the distros.
> >>>>>
> >>>>> (kvm might actually be autoloaded already, so autoloading vfio-ap would
> >>>>> not really make it worse.)  
> >>>> Of the vfio_ccw module is automatically loaded, then the kvm
> >>>> module will also get loaded. I startup up a RHEL8.3 system and
> >>>> sure enough, the vfio_ccw module is loaded along with the
> >>>> kvm, vfio and mdev modules. If this is true for all distros, then
> >>>> it wouldn't make much difference if the vfio_ap module is
> >>>> autoloaded too.  
> >>> I think I don't mind too much if we auto-load vfio-ap or not - but I think
> >>> we should make it consistent with vfio-ccw. So either auto-load both modules
> >>> (if the corresponding devices are available), or remove the
> >>> MODULE_DEVICE_TABLE() entries from both modules?  
> >> I think we really need to take a step back and think about the purpose
> >> of adding a MODULE_DEVICE_TABLE()... basically, it declares which types
> >> of devices on a certain bus a driver supports, in a way that can be
> >> consumed by userspace (after file2alias.c worked on it).  
> > I did a quick search to locate where this semantic was codified. But
> > I didn't find the place neither Documentation/ nor in the header where
> > MODULE_DEVICE_TABLE is defined.
> >  
> >> Userspace typically uses this to match devices it is notified about to
> >> drivers that could possibly drive those devices. In general, the
> >> assumption is that you will want to have the drivers for your devices
> >> loaded. In some cases (drivers only used in special cases, like here),
> >> it might be a better idea to autoload the drivers only under certain
> >> circumstances (e.g. if you know you're going to run KVM guests).  
> > Does RHEL do this, or would RHEL do this out of the box? I.e.
> > would we end up preserving old behavior when this fix hits the distro,
> > or would the end user end up with kvm and vfio_ap loaded (out of the
> > box)?
> >
> > What would be the mechanism of choice to implement if kvm loaded and
> > APs present/hotplugged load vfio_ap, otherwise don't in the userspace?
> >
> > Sorry I'm not very familiar with this whole modules auto-loading
> > business, so I may be asking obvious questions. But a quick google
> > search did not help me.
> >  
> >> My main point, however, is that we're talking about policy here: whether
> >> a potentially useful driver should be loaded or not is a decision that
> >> should be made by userspace. Not providing a MODULE_DEVICE_TABLE does
> >> not look like the right solution, as it deprives userspace of the
> >> information to autoload the driver, if it actually wants to do so.
> >>  
> > I'm sympathetic to this reading of the situation, but I'm not sure
> > it is as black and white as stated.
> >
> > I think the current state of affairs in the vfio_ap module is clearly a
> > bug.
> >
> > One can argue that not auto-loading vfio_ap and kvm per default out of
> > the box is not a bug. I mean the tooling (chzdev) seems to do fine
> > without this and just assuming that both kvm and vfio_ap will be needed
> > just because we have APs seems wrong.
> >
> > One of the more important guiding principles of Linux kernel development
> > is no userspace regressions. And I think suddenly getting vfio_ap and kvm
> > loaded just because we have AP devices can be thought of as a regression.
> >
> > So I'm sympathetic to Harald's view as well.
> >
> > Of course there is the solution that the distros should really make sure
> > the old behavior is preserved, or some smart behavior is introduced. But
> > regarding smart, I believe "if you have devices that are configured for
> > vfio_ap pass-through (with chzdev), then the vfio_ap module should get
> > loaded" is pretty much as smart as it gets. So blacklisting the module
> > by default in the distros looks like a viable option. If that is what
> > we want, we should probably ask the distros, because I don't think
> > it is just obviously their job to figure out that they have to do so.
> >
> > Disclaimer: I might be wrong about the current behavior, I didn't verify
> > my claims
> >
> > Also what does vfio-pci do?  
> 
>  From vfio_pci.c:
> 
> static const struct pci_device_id vfio_pci_table[] = {
>      { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_ANY_ID, PCI_ANY_ID) }, /* 
> match all by default */
>      {}
> };
> 
> MODULE_DEVICE_TABLE(pci, vfio_pci_table);

What are you trying to tell me with this? Did you read the paragraph
below? From that paragraph it should be obvious that I was aware of the
fact that vfio-pci does have MODULE_DEVICE_TABLE, but because of the
"override" stuff the vfio-pci module *won't* get auto-loaded (unlike
what is proposed here for the vfio-ap module).
 
> 
> > As far as I can tell vfio-pci does not
> > participate in module auto loading just because there are pci devices.
> > The have some smart override I don't quite understand:
> > https://patchwork.ozlabs.org/project/linux-pci/patch/20210826103912.128972-11-yishaih@nvidia.com/
> > Before, I don't think they had a MODULE_DEVICE_TABLE:
> > https://elixir.bootlin.com/linux/v5.8.18/source/drivers/vfio/pci/vfio_pci.c
> >
> > Regards,
> > Halil  
> 

Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus

[Harald Freudenberger]

On 27.01.22 15:23, Tony Krowiak wrote:
>
>
> On 12/2/21 03:33, Harald Freudenberger wrote:
>> On 02.12.21 08:13, Thomas Huth wrote:
>>> On 01/12/2021 18.10, Harald Freudenberger wrote:
>>>> On 01.12.21 15:11, Thomas Huth wrote:
>>>>> The crypto devices that we can use with the vfio_ap module are sitting
>>>>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>>>>> itself. With this change, the vfio_ap module now gets automatically
>>>>> loaded if a supported crypto adapter is available in the host.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    Note: Marked as "RFC" since I'm not 100% sure about it ...
>>>>>          please review carefully!
>>>>>
>>>>>    drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>>>> index 4d2556bc7fe5..5580e40608a4 100644
>>>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>>>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>>>>        { /* end of sibling */ },
>>>>>    };
>>>>>    -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>>>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>>>>      /**
>>>>>     * vfio_ap_queue_dev_probe:
>>>> Hello Thomas, interesting.
>>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>>
>>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>>
>>>   Thomas
>>>
>> Yes, of course for the automatic module load works this way. But you understand that now
>> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
>> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
>> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
>> in use together with 'regular' ap card and queue devices.
>>
>> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
>> of customers will never use vfio ap devices - this is specific to kvm hosts only. I think this has to be
>> decided by Tony and maybe some kvm architect. If there is an agreement, I will try to rework the
>> ap code to be able to deal with foreign devices attached to the ap bus.
>
> Can you please explain how foreign devices can get attached to the bus if the vfio_ap device
> driver is automatically loaded because the MODULE_DEVICE_TABLE specification is changed?
Hello Tony,
I think this is not a question to me ? Of course you are right that when the vfio_ap is
automatically loaded there is no change for other 'alternative' device drivers. However, as of now
there exist only two parties here - the 'default' device driver(s) and the vfio_ap dd and as we do
not officially support out-of-tree builds there is nothing to see in the kernel about a third party dd.
>
> Regards,
> Tony Krowiak
>
>>
>> So thanks for your investigations. Let's wait for Tony and see how we proceed.
>>
>> Harald
>> Harald
>