Messages in this thread |  | | Date | Thu, 27 Jan 2022 09:41:49 -0500 | Subject | Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the "ap" parent bus | From | 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? >
|  |