On Tue, Aug 17, 2021 at 04:01:49PM +0300, Max Gurtovoy wrote:

On 8/16/2021 8:21 PM, Bjorn Helgaas wrote:

quoted

On Sun, Aug 15, 2021 at 02:27:13AM +0300, Max Gurtovoy wrote:

quoted

On 8/13/2021 8:44 PM, Bjorn Helgaas wrote:

quoted

On Fri, Aug 13, 2021 at 02:21:41AM +0300, Max Gurtovoy wrote:

quoted

On 8/12/2021 11:26 PM, Bjorn Helgaas wrote:

quoted

On Thu, Aug 12, 2021 at 04:51:26PM -0300, Jason Gunthorpe wrote:

quoted

On Thu, Aug 12, 2021 at 10:57:07AM -0500, Bjorn Helgaas wrote:

quoted

On Thu, Aug 12, 2021 at 10:27:28AM -0300, Jason Gunthorpe wrote:

quoted

On Wed, Aug 11, 2021 at 02:07:37PM -0500, Bjorn Helgaas wrote:

quoted

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/

quoted

quoted

     - 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 (local)

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