[PATCH] util: basic support for vendor-specific vfio drivers

[Alex Williamson]

On Mon, 1 Aug 2022 16:02:05 +0200
Erik Skultety <eskultet at redhat.com> wrote:

> Putting Alex on CC since I don't see him there:
> +alex.williamson at redhat.com

Hmm, Laine cc'd me on the initial post but it seems it got dropped
somewhere.
 
> On Mon, Aug 01, 2022 at 09:30:38AM -0400, Laine Stump wrote:
> > On 8/1/22 7:58 AM, Erik Skultety wrote:  
> > > On Mon, Aug 01, 2022 at 12:02:22AM -0400, Laine Stump wrote:  
> > > > Before a PCI device can be assigned to a guest with VFIO, that device
> > > > must be bound to the vfio-pci driver rather than to the device's
> > > > normal driver. The vfio-pci driver provides APIs that permit QEMU to
> > > > perform all the necessary operations to make the device accessible to
> > > > the guest.
> > > > 
> > > > There has been kernel work recently to support vendor/device-specific
> > > > VFIO drivers that provide the basic vfio-pci driver functionality
> > > > while adding support for device-specific operations (for example these
> > > > device-specific drivers are planned to support live migration of
> > > > certain devices). All that will be needed to make this functionality
> > > > available will be to bind the new vendor-specific driver to the device
> > > > (rather than the generic vfio-pci driver, which will continue to work
> > > > just without the extra functionality).
> > > > 
> > > > But until now libvirt has required that all PCI devices being assigned
> > > > to a guest with VFIO specifically have the "vfio-pci" driver bound to
> > > > the device. So even if the user manually binds a shiny new
> > > > vendor-specific driver to the device (and puts "managed='no'" in the
> > > > config to prevent libvirt from changing that), libvirt will just fail
> > > > during startup of the guest (or during hotplug) because the driver
> > > > bound to the device isn't named exactly "vfio-pci".
> > > > 
> > > > Fortunately these new vendor/device-specific drivers can be easily
> > > > identified as being "vfio-pci + extra stuff" - all that's needed is to
> > > > look at the output of the "modinfo $driver_name" command to see if
> > > > "vfio_pci" is in the alias list for the driver.
> > > > 
> > > > That's what this patch does. When libvirt checks the driver bound to a
> > > > device (either to decide if it needs to bind to a different driver or
> > > > perform some other operation, or if the current driver is acceptable
> > > > as-is), if the driver isn't specifically "vfio-pci", then it will look
> > > > at the output of modinfo for the driver that *is* bound to the device;
> > > > if modinfo shows vfio_pci as an alias for that device, then we'll
> > > > behave as if the driver was exactly "vfio-pci".  
> > > 
> > > Since you say that the vendor/device-specific drivers does each of such drivers
> > > implement the base vfio-pci functionality or they simply call into the base
> > > driver? The reason why I'm asking is that if each of the vendor-specific
> > > drivers depend on the vfio-pci module to be loaded as well, then reading
> > > /proc/modules should suffice as vfio-pci should be listed right next to the
> > > vendor-specific one. What am I missing?  
> > I don't know the definitive answer to that, as I have no example of a
> > working vendor-specific driver to look at and only know about the kernel
> > work going on second-hand from Alex. It looks like even the vfio_pci driver
> > itself depends on other presumably lower level vfio-* modules (it directly
> > uses vfio_pci_core, which in turn uses vfio and vfio_vifqfd), so possibly
> > these new drivers would be depending on one or more of those lower level
> > modules rather than vfio_pci. Also I would imagine it would be possible for
> > other drivers to also depend on the vfio-pci driver while not themselves
> > being a vfio driver.

A module dependency on vfio-pci (actually vfio-pci-core) is a pretty
loose requirement, *any* symbol dependency generates such a linkage,
without necessarily exposing a vfio-pci uAPI.  The alias support
introduced to the kernel is intended to allow userspace to determine
the most appropriate vfio-pci driver for a device, whether that's
vfio-pci itself or a variant driver that augments device specific
features.  See the upstream commit here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc6711b0bf36de068b10490198d05ac168377989

All we're doing here is extending libvirt to say that if the driver is
vfio-pci or the modalias for the driver is prefixed with vfio-pci, then
the driver exposes a vfio-pci compatible uAPI.  I expect in the future
libvirt, or some other utility, may take on the role as described in
the above commit log to not only detect that a driver supports a
vfio-pci uAPI, but also to identify the most appropriate driver for the
device which exposes a vfio-uAPI.

> > > The 'alias' field is optional so do we have any support guarantees from the
> > > vendors that the it will always be filled in correctly? I mean you surely
> > > handle that case in the code, but once we start supporting this there's no way
> > > back and we already know how painful it can be to convince the vendors to
> > > follow some kind of standard so that we don't need to maintain several code
> > > paths based on a vendor-matrix.  
> > 
> > The aliases are what is used to determine the "best" vfio driver for a
> > particular device, so I don't think it would be possible for a driver to not
> > implement it, and the method I've used here to determine if a driver is a
> > vfio driver was recommended by Alex after a couple of discussions on the
> > subject.
> >   
> > > 
> > > ...
> > >   
> > > > +int
> > > > +virPCIDeviceGetDriverNameAndType(virPCIDevice *dev,
> > > > +                                 char **drvName,
> > > > +                                 virPCIStubDriver *drvType)
> > > > +{
> > > > +    g_autofree char *drvPath = NULL;
> > > > +    g_autoptr(virCommand) cmd = NULL;
> > > > +    g_autofree char *output = NULL;
> > > > +    g_autoptr(GRegex) regex = NULL;
> > > > +    g_autoptr(GError) err = NULL;
> > > > +    g_autoptr(GMatchInfo) info = NULL;
> > > > +    int exit;
> > > > +    int tmpType;
> > > > +
> > > > +    if (virPCIDeviceGetDriverPathAndName(dev, &drvPath, drvName) < 0)
> > > > +        return -1;
> > > > +
> > > > +    if (!*drvName) {
> > > > +        *drvType = VIR_PCI_STUB_DRIVER_NONE;
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    tmpType = virPCIStubDriverTypeFromString(*drvName);
> > > > +
> > > > +    if (tmpType > VIR_PCI_STUB_DRIVER_NONE) {
> > > > +        *drvType = tmpType;
> > > > +        return 0; /* exact match of a known driver name (or no name) */
> > > > +    }
> > > > +
> > > > +    /* Check the output of "modinfo $drvName" to see if it has
> > > > +     * "vfio_pci" as an alias. If it does, then this driver should
> > > > +     * also be considered as a vfio-pci driver, because it implements
> > > > +     * all the functionality of the basic vfio-pci (plus additional
> > > > +     * device-specific stuff).
> > > > +     */  
> > > 
> > > Instead of calling an external program and then grepping its output which
> > > technically could change in the future, wouldn't it be better if we read
> > > /lib/modules/`uname -r`/modules.alias and filtered whatever line had the
> > > vfio-pci' substring and compared the module name with the user-provided device
> > > driver?  
> > 
> > Again, although I was hesistant about calling an external command, and asked
> > if there was something simpler, Alex still suggested modinfo, so I'll let
> > him answer that. Alex?
> > 
> > (Also, although the format of the output of "uname -r" is pretty much
> > written in stone, you're still running an external command :-))

The above commit log actually suggests modules.alias, so if you'd
rather rely on that than modinfo, sure, that's fine.  Thanks,

Alex