Re: [PATCH v3 13/13] qemu: automatically bind to a vfio variant driver, if available

On 1/5/24 8:46 AM, Peter Krempa wrote: > On Fri, Jan 05, 2024 at 03:20:16 -0500, Laine Stump wrote: > > Rather than always binding to the vfio-pci driver, use the new > > function virPCIDeviceFindBestVFIOVariant() to see if the running > > kernel has a VFIO variant driver available that is a better match for > > the device, and if one is found, use that instead. > > > > virPCIDeviceFindBestVFIOVariant() function reads the modalias file for > > the given device from sysfs, then looks through > > /lib/modules/${kernel_release}/modules.alias for the vfio_pci alias > > that matches with the least number of wildcard ('*') fields. > > > > The appropriate "VFIO variant" driver for a device will be the PCI > > driver implemented by the discovered module - these drivers are > > compatible with (and provide the entire API of) the standard vfio-pci > > driver, but have additional device-specific APIs that can be useful > > for, e.g., saving/restoring state for migration. > > > > If a specific driver is named (using <driver model='blah'/> in the > > device XML), that will still be used rather than searching > > modules.alias; this makes it possible to force binding of vfio-pci if > > there is an issue with the auto-selected variant driver. > > > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > > --- > > > > Changes from V2: > > > > * fail if device modalias file isn't found. > > This [1] ... > > > * use unsigned int instead of int for wildcardCt > > * increase file memory buffer from 4MB to 8MB > > * other minor nits pointed out by Peter > > [...] > > > +/* virPCIDeviceFindBestVFIOVariant: > > + * > > + * Find the "best" match of all vfio_pci aliases for @dev in the host > > + * modules.alias file. This uses the algorithm of finding every > > + * modules.alias line that begins with "vfio_pci:", then picking the > > + * one that matches the device's own modalias value (from the file of > > + * that name in the device's sysfs directory) with the fewest > > + * "wildcards" (* character, meaning "match any value for this > > + * attribute"). > > + */ > > +int > > +virPCIDeviceFindBestVFIOVariant(virPCIDevice *dev, > > + char **moduleName) > > +{ > > + g_autofree char *devModAliasPath = NULL; > > + g_autofree char *devModAliasContent = NULL; > > + const char *devModAlias; > > + g_autoptr(virPCIDeviceAliasInfo) devModAliasInfo = NULL; > > + struct utsname unameInfo; > > + g_autofree char *modulesAliasPath = NULL; > > + g_autofree char *modulesAliasContent = NULL; > > + const char *line; > > + unsigned int currentBestWildcardCt = INT_MAX; > > UINT_MAX > > > + > > + *moduleName = NULL; > > + > > + /* get the modalias values for the device from sysfs */ > > + devModAliasPath = virPCIFile(dev->name, "modalias"); > > + if (virFileReadAll(devModAliasPath, 100, &devModAliasContent) < 0) > > ... [1] isn't true. How so? If the file can't be opened, then virFileReadAll logs an error and returns -1, and changed changed this code from ignoring that to also returning -1, which will cause the caller to fail. What am I missing?

> > + return -1; > > + > > + VIR_DEBUG("modalias path: '%s' contents: '%s'", > > + devModAliasPath, devModAliasContent); > > + > > + /* "pci:vNNNNNNNNdNNNNNNNNsvNNNNNNNNsdNNNNNNNNbcNNscNNiNN\n" */ > > + if ((devModAlias = STRSKIP(devModAliasContent, "pci:")) == NULL || > > + !(devModAliasInfo = virPCIDeviceAliasInfoNew(devModAlias))) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("device modalias file %1$s content has improper format"), > > + devModAliasPath); > > + return -1; > > + } > > + > > + uname(&unameInfo); > > + modulesAliasPath = g_strdup_printf("/lib/modules/%s/modules.alias", unameInfo.release); > > + if (virFileReadAll(modulesAliasPath, 8 * 1024 * 1024, &modulesAliasContent) < 0) > > + return -1; > > IIUC this is picking a driver which is 'better' than the default > 'vfio_pci', but the device itself would still work if the default were > used, right? Correct. > > In such case do we really want to treat any of the errors above as > failures? I was on the fence about this, which was why earlier versions ignored the error. But your earlier suggestion to check for file existence before trying to read the file (thus eliminating any sort of message at all, which I think would be wrong) pushed me in the direction of logging the error and failing (since it would be *really* broken for the modalias file to be missing). I would also be fine with logging the error when virFileReadAll() fails (ie *not* checking if the file exists so we could silently avoid the error log), but then clearing the and returning success. Does that work for you?

> I presueme a workaround for if any of the above breaks is to > use an explicitly specified driver, right? Correct. If a driver is explicitly given, then we don't attempt to find a better match. So in the case that a device really had no modalias file but was otherwise working, the error could be worked around by adding <driver model='vfio-pci'/> to the XML (assuming that whatever is creating the XML is able to do that).