On Mon, 1 Aug 2022 16:02:05 +0200 Erik Skultety <eskultet@xxxxxxxxxx> wrote: > Putting Alex on CC since I don't see him there: > +alex.williamson@xxxxxxxxxx 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