Re: [PATCH 09/12] PCI: Add a PCI_ID_F_VFIO_DRIVER_OVERRIDE flag to struct pci_device_id

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Aug 17, 2021 at 04:01:49PM +0300, Max Gurtovoy wrote:
> On 8/16/2021 8:21 PM, Bjorn Helgaas wrote:
> > On Sun, Aug 15, 2021 at 02:27:13AM +0300, Max Gurtovoy wrote:
> > > On 8/13/2021 8:44 PM, Bjorn Helgaas wrote:
> > > > On Fri, Aug 13, 2021 at 02:21:41AM +0300, Max Gurtovoy wrote:
> > > > > On 8/12/2021 11:26 PM, Bjorn Helgaas wrote:
> > > > > > On Thu, Aug 12, 2021 at 04:51:26PM -0300, Jason Gunthorpe wrote:
> > > > > > > On Thu, Aug 12, 2021 at 10:57:07AM -0500, Bjorn Helgaas wrote:
> > > > > > > > On Thu, Aug 12, 2021 at 10:27:28AM -0300, Jason Gunthorpe wrote:
> > > > > > > > > On Wed, Aug 11, 2021 at 02:07:37PM -0500, Bjorn Helgaas wrote:
> > > > > > > > > > On Thu, Aug 05, 2021 at 09:23:57PM -0300, Jason Gunthorpe wrote:
> > > > > > > > > > Do the other bus types have a flag analogous to
> > > > > > > > > > PCI_ID_F_VFIO_DRIVER_OVERRIDE?  If we're doing something similar to
> > > > > > > > > > other bus types, it'd be nice if the approach were similar.
> > > > > > > > > They could, this series doesn't attempt it. I expect the approach to
> > > > > > > > > be similar as driver_override was copied from PCI to other
> > > > > > > > > busses. When this is completed I hope to take a look at it.
> > > > > > > > I think this would make more sense as two patches:
> > > > > > > > 
> > > > > > > >      - Add a "PCI_ID_DRIVER_OVERRIDE" flag.  This is not VFIO-specific,
> > > > > > > >        since nothing in PCI depends on the VFIO-ness of drivers that use
> > > > > > > >        the flag.  The only point here is that driver id_table entries
> > > > > > > >        with this flag only match when driver_override matches the driver.
> > > > > > > This would require using two flags, one to indicate the above to the
> > > > > > > PCI code and another to indicate the vfio_pci string to
> > > > > > > file2alias. This doesn't seem justified at this point, IMHO.
> > > > > > I don't think it requires two flags.  do_pci_entry() has:
> > > > > > 
> > > > > >      if (flags & PCI_ID_F_VFIO_DRIVER_OVERRIDE)
> > > > > >        strcpy(alias, "vfio_pci:");
> > > > > > 
> > > > > > I'm just proposing a rename:
> > > > > > 
> > > > > > s/PCI_ID_F_VFIO_DRIVER_OVERRIDE/PCI_ID_DRIVER_OVERRIDE/
> > > > > > 
> > > > > > > >      - Update file2alias.c to export the flags and the "vfio_pci:" alias.
> > > > > > > >        This seems to be the only place where VFIO comes into play, and
> > > > > > > >        putting it in a separate patch will make it much smaller and it
> > > > > > > >        will be clear how it could be extended for other buses.
> > > > > > > Well, I don't want to see a flag called PCI_ID_DRIVER_OVERRIDE mapped
> > > > > > > to the string "vfio_pci", that is just really confusing.
> > > > > > Hahaha, I see, that's fair :)  It confused me for a long time why you
> > > > > > wanted "VFIO" in the flag name because from the kernel's point of
> > > > > > view, the flag is not related to any VFIO-ness.  It's only related to
> > > > > > a special variety of driver_override, and VFIO happens to be one user
> > > > > > of it.
> > > > > In my original patch I used
> > > > > 
> > > > > #define PCI_ID_DRIVER_OVERRIDE PCI_ID_F_VFIO_DRIVER_OVERRIDE
> > > > > 
> > > > > and in the pci core code I used PCI_ID_DRIVER_OVERRIDE in the "if" clause.
> > > > > 
> > > > > So we can maybe do that and leave the option to future update of the define
> > > > > without changing the core code.
> > > > > 
> > > > > In the future we can have something like:
> > > > > 
> > > > > #define PCI_ID_DRIVER_OVERRIDE (PCI_ID_F_VFIO_DRIVER_OVERRIDE |
> > > > > PCI_ID_F_MY_BUS_DRIVER_OVERRIDE)
> > > > > 
> > > > > The file2alias.c still have to use the exact PCI_ID_F_VFIO_DRIVER_OVERRIDE
> > > > > flag to add "vfio_" prefix.
> > > > > 
> > > > > Is that better ?
> > > > I don't think it's worth having two separate #defines.  If we need
> > > > more in the future, we can add them when we need them.
> > > I meant 1 #define and 1 enum:
> > > 
> > > enum {
> > >      PCI_ID_F_VFIO_DRIVER_OVERRIDE    = 1 << 0,
> > > };
> > > 
> > > #define PCI_ID_DRIVER_OVERRIDE PCI_ID_F_VFIO_DRIVER_OVERRIDE
> > Basically the same thing.  Doesn't seem worthwhile to me to have both.
> > When reading the code, it's not at all obvious why you would define a
> > new name for PCI_ID_F_VFIO_DRIVER_OVERRIDE.
> 
> because we need the "vfio_" prefix in the alias.
> 
> And the match can use PCI_ID_DRIVER_OVERRIDE that in the future cab be
> (#define PCI_ID_DRIVER_OVERRIDE (PCI_ID_F_VFIO_DRIVER_OVERRIDE |
> PCI_ID_F_SOME_OTHER_ALIAS_DRIVER_OVERRIDE)

Read this again:
https://lore.kernel.org/r/20210813174459.GA2594783@bjorn-Precision-5520

That gives you a "vfio_" prefix without the unnecessary VFIO
connection in pci_match_device.