Re: [libvirt PATCH v2 01/15] util: properly deal with module vs. driver when binding device to driver

Historically libvirt has treated the concept of "loadable kernel module" and "device driver" as being effectively the same (at least in the case of the vfio-pci driver used for VFIO device assignment). The code assumed that a module named "vfio-pci" implemented a driver named "vfio-pci". In reality, the module is named "vfio_pci", and it implements a device driver named "vfio-pci" (note the difference in separator characters),

and our code worked only because the modprobe utility we use to load the module will always "normalize" the module name it's given by replacing all "-" (dash) with "_" (underscore) (this has been verified in the modprobe source, which is in the kmod package - there are 3 separate functions that perform this same operation!).

So even though we asked modprobe to load the "vfio-pci" module, it would actually load the "vfio_pci" module. After loading the module with modprobe, libvirt then looks for the desired *driver* to be present in sysfs, by looking for the directory /sys/bus/pci/drivers/${driverName}, which would succeed because we were still looking for the original "dash version" of the name ("vfio-pci"). When we recently gained the ability to manually specify a driver to bind to with virsh nodedev-detach, the fragility of this system became apparent - if a user gives the driver name as "vfio_pci", then we would modprobe the module, but then erroneously believe it hadn't been loaded because /sys/bus/pci/drivers/vfio_pci didn't exist. For manual specification of the driver name, this could be dealt with by telling the user "always use the correct name for the driver, don't assume that it is the same as the modulename", but it would still end up confusing people, especially since some drivers do use underscore in their name (e.g. the mlx5_vfio_pci driver/module). More more importantly, when we begin looking in the modules.alias file for the "best" VFIO variant driver for a particular device (in an upcoming patch), that lookup will provide us with the name of a *module*, not a driver, and 3 of the 4 examples of vfio-pci/variant drivers I have access to use underscore in the module name and dash in the driver, while the 4th uses underscore in both, so 3 out of 4 fail with the current code. To make the current code more forgiving (and yet more correct!), as well as to make the upcoming variant auto-selection actually useful, this patch follows the following steps: 1) if the requested driver (${driverName}) is present in sysfs drivers list (/sys/bus/pci/drivers/${driverName}) then use ${driverName} - DONE

2) if underscored(${driverName}) (call it ${underName} for short) is in sysfs drivers list then use ${underName} - DONE 3) if ${underName} *isn't* in the sysfs modules list (/sys/module/${underName}) then "modprobe ${underName}" to load it. 4) look for the PCI driver implemented by this module, it will be at /sys/module/${underName}/driver/pci:${newName}. 5) use ${newName} - DONE. A few notes: a) This will *always* replace dash with underscore in the name used to load the module, which seems it could be problematic, but I have verified this is what modprobe itself does, so I don't think there will ever be a module with "-" in its name as modprobe would be unable to load it (the same can't be said for drivers though!) b) The one case where the above steps wouldn't work would be if someone implemented a driver within a module where the driver and module had names that differed other than dash vs. underscore. I haven't seen an example of this, so the convention seems to be that they will "match" (modulo - & _). If this mismatch were to ever occur, though, it could be worked around by simply loading the module out-of-band during the host system startup, and then using the driver's name in the config. c) All of this is conveniently ignoring the possibility of a VFIO variant driver that is statically linked in the kernel. The entire design of variant driver auto-detection is based on doing a lookup in modules.alias, and that only lists *loadable modules* (not drivers), so unless I'm missing something, it would be impossible to auto-detect a VFIO variant driver that was statically linked. This is beyond libvirt's ability to fix; the best that could be done would be to manually specify the driver name in the libvirt config, which I suppose is better than nothing :-) Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virpci.c | 194 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 164 insertions(+), 30 deletions(-)