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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 27 Jul 2021 14:14:58 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, Jul 27, 2021 at 10:34:18AM -0600, Alex Williamson wrote:
> > On Wed, 21 Jul 2021 19:16:06 +0300
> > Yishai Hadas <yishaih@nvidia.com> wrote:
> >   
> > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > 
> > > The new flag field is be used to allow PCI drivers to signal the core code
> > > during driver matching and when generating the modules.alias information.
> > > 
> > > The first use will be to define a VFIO flag that indicates the PCI driver
> > > is a VFIO driver.
> > > 
> > > VFIO drivers have a few special properties compared to normal PCI drivers:
> > >  - They do not automatically bind. VFIO drivers are used to swap out the
> > >    normal driver for a device and convert the PCI device to the VFIO
> > >    subsystem.
> > > 
> > >    The admin must make this choice and following the current uAPI this is
> > >    usually done by using the driver_override sysfs.
> > > 
> > >  - The modules.alias includes the IDs of the VFIO PCI drivers, prefixing
> > >    them with 'vfio_pci:' instead of the normal 'pci:'.
> > > 
> > >    This allows the userspace machinery that switches devices to VFIO to
> > >    know what kernel drivers support what devices and allows it to trigger
> > >    the proper device_override.
> > > 
> > > As existing tools do not recognize the "vfio_pci:" mod-alias prefix this
> > > keeps todays behavior the same. VFIO remains on the side, is never
> > > autoloaded and can only be activated by direct admin action.
> > > 
> > > This patch is the infrastructure to provide the information in the
> > > modules.alias to userspace and enable the only PCI VFIO driver. Later
> > > series introduce additional HW specific VFIO PCI drivers.  
> > 
> > I don't really understand why we're combining the above "special
> > properties" into a single flag.   
> 
> Currently I can't think of any reason to have two flags. We always
> need both behaviors together. It is trivial for someone to change down
> the road, so I prefer to keep the flag bit usage to a minimum.
> 
> > For instance, why wouldn't we create a flag that just indicates a
> > match entry is only for driver override?  
> 
> We still need to signal the generation of vfio_pci: string in the
> modules.alias.
> 
> > Or if we're only using this for full wildcard matches, we could
> > detect that even without a flag.  
> 
> The mlx/hns/etc drivers will not use wildcard matches. This series is
> the prep and the only driver we have right at this point is the
> wildcard vfio_pci generic driver.
> 
> > Then, how does the "vfio_pci:" alias extend to other drivers?    
> 
> After the HW drivers are merged we have a list of things in the
> modules.alias file. Eg we might have something like:
> 
> alias vfio_pci:v000015B3d00001011sv*sd*bc*sc*i* mlx5_vfio_pci
> alias vfio_pci:v0000abc1d0000abcdsv*sd*bc*sc*i* hns_vfio_pci
> alias vfio_pci:v*d*sv*sd*bc*sc*i* vfio_pci
> 
> This flag, and the vfio_pci string, is only for the VFIO subsystem. If
> someday another subsystem wants to use driver_override then it will
> provide its own subsystem name here instead.
> 
> This is solving the problem you had at the start - that userspace must
> be able to self identify the drivers.  Starting with a PCI BDF
> userspace can match the modules.alias for vfio_pci: prefixes and
> determine which string to put into the driver_override sysfs. This is
> instead of having userspace hardwire vfio_pci.
> 
> > Is this expected to be the only driver that would use an alias ever
> > or would other drivers use new bits of the flag?  
> 
> Not sure what you mean by "only driver"? As above every driver
> implementing VFIO on top of PCI will use this flag. If another
> subsystem wants to use driver_override it will define its own flag,
> and it's userspace will look for othersubsytem_pci: tags in
> modules.alias when it wants to change a PCI device over.
> 
> > Seems some documentation is necessary; the comment on
> > PCI_DRIVER_OVERRIDE_DEVICE_VFIO doesn't really help, "This macro is
> > used to create a struct pci_device_id that matches a specific
> > device", then we proceed to use it with PCI_ANY_ID.  
> 
> Fair enough, this is ment in the broader context, the generic vfio_pci
> is just special.
> 
> > vfio-pci has always tried (as much as possible) to be "just another
> > PCI" driver to avoid all the nasty issues that used to exist with
> > legacy KVM device assignment, so I cringe at seeing these vfio specific
> > hooks in PCI-core.  Thanks,  
> 
> It is has always had very special behavior - a PCI driver without a
> match table is is not "just another PCI" driver.
> 
> While this is not entirely elegant, considering where we have ended up
> and the historical ABI that has to be preserved, it is the best idea
> so far anyone has presented.

In general I think my confusion is lack of documentation and examples.
There's good information here and in the cover letter, but reviewing
the patch itself I'm not sure if vfio_pci: is meant to indicate the
vfio_pci driver or the vfio_pci device api or as I've finally decided,
just prepending "vfio_" to the modalias for a device to indicate the
class of stuff, ie. no automatic binding but discoverable by userspace
as a "vfio" driver suitable for this device.

I think we need libvirt folks onboard and maybe a clearer idea what
userspace helpers might be available.  For example would driverctl have
an option to choose a vfio class driver for a device?

I can also imagine that if the flag only covered the
matching/driver_override aspect and pci_device_id further included an
optional modalias prefix, we could do this without littering pci-core
with vfio eccentricities.  I'll be interest to see Bjorn's thoughts on
this.  Thanks,

Alex