Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 28 Jan 2022 02:35:53 +0100
From:   Halil Pasic <pasic@linux.ibm.com>
To:     Tony Krowiak <akrowiak@linux.ibm.com>
Cc:     Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>,
        Harald Freudenberger <freude@linux.ibm.com>,
        linux-s390@vger.kernel.org, Jason Herne <jjherne@linux.ibm.com>,
        linux-kernel@vger.kernel.org, Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the
 "ap" parent bus
Message-ID: <20220128023553.56a8b135.pasic@linux.ibm.com>
In-Reply-To: <75637be1-7459-1cdc-3b4d-f8f3225f7b8b@linux.ibm.com>
References: <20211201141110.94636-1-thuth@redhat.com>
        <8512bb0a-a34a-09b0-65f3-781f3d092364@linux.ibm.com>
        <87k0g8scx1.fsf@redhat.com>
        <1eb9ca5c-b1bb-b768-64ee-e4a1b31bb171@linux.ibm.com>
        <6aaf6c60-a258-29e3-fcec-82c77d3945a4@redhat.com>
        <87tufaqbex.fsf@redhat.com>
        <20211216000229.7a284661.pasic@linux.ibm.com>
        <75637be1-7459-1cdc-3b4d-f8f3225f7b8b@linux.ibm.com>
Organization: IBM
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk

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  
>