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