Historically libvirt hasn't differentiated between the name of a
loadable kernel module, and the name of the device driver that module
implements, but these two names can be (and usually are) at least
subtly different.
For example, the loadable module called "vfio_pci" implements a PCI
driver called "vfio-pci". We have always used the name "vfio-pci" both
to load the module (with modprobe) and to check (in
/sys/bus/pci/drivers) if the driver is available. (This has happened
to work because modprobe "normalizes" all the names it is given by
replacing "-" with "_", so "vfio-pci" works for both loading the
module and checking for the driver.)
When we recently gained the ability to manually specify the driver for
"virsh nodedev-detach", the fragility of this system became apparent -
if a user gave the "driver name" as "vfio_pci", then we would modprobe
the module correctly, 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, we could deal with this by telling
the user "always use the correct name for the driver, don't assume
that it has the same name as the module", 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).
This will only get worse when an upcoming patch starts automatically
determining the driver to use for VFIO-assigned devices - it will look
in the kernel's modules.alias file to find "best" VFIO variant
*module* for a device, and 3 out of 4 current examples of
vfio-pci/variant drivers have a mismatch between module name and
driver name, so the current code would end up properly loading the
module, but then erroneously think that the driver wasn't available.
This patch makes the code more forgiving by
1) checking for both $drivername and underscore($drivername) in
/sys/bus/pci/drivers
2) when we determine a module needs to be loaded, look at the link in
/sys/module/$modulename/driver/pci:$drivername to determine the
name of the driver we need to bind to the device(rather than just
assuming the driver has the same name as the module
Signed-off-by: Laine Stump <laine@redhat.com>
---
Change from V1: I tried to simplify the explanation in the commit log message
src/util/virpci.c | 193 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 163 insertions(+), 30 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index afce7b52b7..f6bdf56057 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -222,6 +222,13 @@ virPCIDriverDir(const char *driver)
}
+static char *
+virPCIModuleDir(const char *module)
+{
+ return g_strdup_printf("/sys/module/%s", module);
+}
+
+
static char *
virPCIFile(const char *device, const char *file)
{
@@ -1154,44 +1161,166 @@ virPCIDeviceReset(virPCIDevice *dev,
}
+/**
+ * virPCINameDashToUnderscore:
+ * @path: the module/driver name or path - will be directly modified
+ *
+ * Replace all occurences of "-" with "_" in the name
+ * part of the path (everything after final "/"
+ *
+ * return true if any change was made, otherwise false.
+ */
static int
-virPCIProbeDriver(const char *driverName)
+virPCINameDashToUnderscore(char *path)
{
- g_autofree char *drvpath = NULL;
+ bool changed = false;
+ char *tmp = strrchr(path, '/');
+
+ if (!tmp)
+ tmp = path;
+
+ while (*tmp) {
+ if (*tmp == '-') {
+ *tmp = '_';
+ changed = true;
+ }
+ tmp++;
+ }
+
+ return changed;
+}
+
+
+static int
+virPCIProbeModule(const char *moduleName)
+{
+ g_autofree char *modulePath = NULL;
g_autofree char *errbuf = NULL;
- drvpath = virPCIDriverDir(driverName);
+ modulePath = virPCIModuleDir(moduleName);
/* driver previously loaded, return */
- if (virFileExists(drvpath))
+ if (virFileExists(modulePath))
return 0;
- if ((errbuf = virKModLoad(driverName))) {
- VIR_WARN("failed to load driver %s: %s", driverName, errbuf);
- goto cleanup;
+ if ((errbuf = virKModLoad(moduleName))) {
+ /* If we know failure was because of admin config, let's report that;
+ * otherwise, report a more generic failure message
+ */
+ if (virKModIsProhibited(moduleName)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to load PCI driver module %1$s: administratively prohibited"),
+ moduleName);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to load PCI driver module %1$s: %2$s"),
+ moduleName, errbuf);
+ }
+ return -1;
}
/* driver loaded after probing */
- if (virFileExists(drvpath))
+ if (virFileExists(modulePath))
return 0;
- cleanup:
- /* If we know failure was because of admin config, let's report that;
- * otherwise, report a more generic failure message
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("modprobe reported success loading module '%1$s', but module is missing from /sys/module"),
+ moduleName);
+ return -1;
+}
+
+/**
+ * virPCIDeviceFindDriver:
+ * @dev: initialized virPCIDevice, including desired stubDriverName
+ *
+ * Checks if there is a driver named @dev->stubDriverName already
+ * loaded. If there is, we're done. If not, look for a driver with
+ * that same name, except with dashes replaced with underscores.
+
+ * If neither of the above is found, then look for/load the module of
+ * the underscored version of the name, and follow the links from
+ * /sys/module/$name/drivers/pci:* to the PCI driver associated with that
+ * module, and update @dev->stubDriverName with that name.
+ *
+ * On a successful return, @dev->stubDriverName will be updated with
+ * the proper name for the driver, and that driver will be loaded.
+ *
+ * returns 0 on success, -1 on failure
+ */
+static int
+virPCIDeviceFindDriver(virPCIDevice *dev)
+{
+ g_autofree char *driverPath = virPCIDriverDir(dev->stubDriverName);
+ g_autofree char *moduleName = NULL;
+ g_autofree char *moduleDriversDir = NULL;
+ g_autoptr(DIR) dir = NULL;
+ struct dirent *ent;
+ int direrr;
+
+ /* unchanged stubDriverName */
+ if (virFileExists(driverPath))
+ return 0;
+
+ /* try replacing "-" with "_" */
+ if (virPCINameDashToUnderscore(driverPath) && virFileExists(driverPath)) {
+
+ /* update original name in dev */
+ virPCINameDashToUnderscore(dev->stubDriverName);
+ return 0;
+ }
+
+ /* look for a module with this name (but always replacing
+ * "-" with "_", since that's what modprobe does)
*/
- if (virKModIsProhibited(driverName)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to load PCI driver module %1$s: administratively prohibited"),
- driverName);
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to load PCI driver module %1$s"),
- driverName);
+
+ moduleName = g_strdup(dev->stubDriverName);
+ virPCINameDashToUnderscore(moduleName);
+
+ if (virPCIProbeModule(moduleName) < 0)
+ return -1;
+
+ /* module was found/loaded. Now find the PCI driver it implements,
+ * linked to by /sys/module/$moduleName/drivers/pci:$driverName
+ */
+
+ moduleDriversDir = g_strdup_printf("/sys/module/%s/drivers", moduleName);
+
+ if (virDirOpen(&dir, moduleDriversDir) < 0)
+ return -1;
+
+ while ((direrr = virDirRead(dir, &ent, moduleDriversDir))) {
+
+ if (STRPREFIX(ent->d_name, "pci:")) {
+ /* this is the link to the driver we want */
+
+ g_autofree char *drvName = NULL;
+ g_autofree char *drvPath = NULL;
+
+ /* extract the driver name from the link name */
+ drvName = g_strdup(ent->d_name + strlen("pci:"));
+
+ /* make sure that driver is actually loaded */
+ drvPath = virPCIDriverDir(drvName);
+ if (!virFileExists(drvPath)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("pci driver '%1$s' supposedly loaded by module '%2$s' not found in sysfs"),
+ drvName, moduleName);
+ return -1;
+ }
+
+ g_free(dev->stubDriverName);
+ dev->stubDriverName = g_steal_pointer(&drvName);
+ return 0;
+ }
}
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("module '%1$s' does not implement any pci driver"),
+ moduleName);
return -1;
}
+
int
virPCIDeviceUnbind(virPCIDevice *dev)
{
@@ -1291,7 +1420,6 @@ virPCIDeviceUnbindFromStub(virPCIDevice *dev)
static int
virPCIDeviceBindToStub(virPCIDevice *dev)
{
- const char *stubDriverName = dev->stubDriverName;
g_autofree char *stubDriverPath = NULL;
g_autofree char *driverLink = NULL;
@@ -1303,30 +1431,35 @@ virPCIDeviceBindToStub(virPCIDevice *dev)
return -1;
}
- if (!stubDriverName
- && !(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown stub driver configured for PCI device %1$s"),
- dev->name);
- return -1;
+ if (!dev->stubDriverName) {
+
+ const char *stubDriverName = NULL;
+
+ if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown stub driver configured for PCI device %1$s"),
+ dev->name);
+ return -1;
+ }
+ dev->stubDriverName = g_strdup(stubDriverName);
}
- if (virPCIProbeDriver(stubDriverName) < 0)
+ if (virPCIDeviceFindDriver(dev) < 0)
return -1;
- stubDriverPath = virPCIDriverDir(stubDriverName);
+ stubDriverPath = virPCIDriverDir(dev->stubDriverName);
driverLink = virPCIFile(dev->name, "driver");
if (virFileExists(driverLink)) {
if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
/* The device is already bound to the correct driver */
VIR_DEBUG("Device %s is already bound to %s",
- dev->name, stubDriverName);
+ dev->name, dev->stubDriverName);
return 0;
}
}
- if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0)
+ if (virPCIDeviceBindWithDriverOverride(dev, dev->stubDriverName) < 0)
return -1;
dev->unbind_from_stub = true;
--
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote:
> Historically libvirt hasn't differentiated between the name of a
> loadable kernel module, and the name of the device driver that module
> implements, but these two names can be (and usually are) at least
> subtly different.
>
> For example, the loadable module called "vfio_pci" implements a PCI
> driver called "vfio-pci". We have always used the name "vfio-pci" both
> to load the module (with modprobe) and to check (in
> /sys/bus/pci/drivers) if the driver is available. (This has happened
> to work because modprobe "normalizes" all the names it is given by
> replacing "-" with "_", so "vfio-pci" works for both loading the
> module and checking for the driver.)
>
> When we recently gained the ability to manually specify the driver for
> "virsh nodedev-detach", the fragility of this system became apparent -
> if a user gave the "driver name" as "vfio_pci", then we would modprobe
> the module correctly, 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, we could deal with this by telling
> the user "always use the correct name for the driver, don't assume
> that it has the same name as the module", 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).
>
> This will only get worse when an upcoming patch starts automatically
> determining the driver to use for VFIO-assigned devices - it will look
> in the kernel's modules.alias file to find "best" VFIO variant
> *module* for a device, and 3 out of 4 current examples of
> vfio-pci/variant drivers have a mismatch between module name and
> driver name, so the current code would end up properly loading the
> module, but then erroneously think that the driver wasn't available.
>
> This patch makes the code more forgiving by
>
> 1) checking for both $drivername and underscore($drivername) in
> /sys/bus/pci/drivers
>
> 2) when we determine a module needs to be loaded, look at the link in
> /sys/module/$modulename/driver/pci:$drivername to determine the
> name of the driver we need to bind to the device(rather than just
> assuming the driver has the same name as the module
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>
> Change from V1: I tried to simplify the explanation in the commit log message
>
> src/util/virpci.c | 193 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 163 insertions(+), 30 deletions(-)
[...]
> @@ -1154,44 +1161,166 @@ virPCIDeviceReset(virPCIDevice *dev,
> }
>
>
> +/**
> + * virPCINameDashToUnderscore:
> + * @path: the module/driver name or path - will be directly modified
> + *
> + * Replace all occurences of "-" with "_" in the name
> + * part of the path (everything after final "/"
> + *
> + * return true if any change was made, otherwise false.
> + */
> static int
'int'
> -virPCIProbeDriver(const char *driverName)
> +virPCINameDashToUnderscore(char *path)
> {
> - g_autofree char *drvpath = NULL;
> + bool changed = false;
'bool'
> + char *tmp = strrchr(path, '/');
> +
> + if (!tmp)
> + tmp = path;
> +
> + while (*tmp) {
> + if (*tmp == '-') {
> + *tmp = '_';
> + changed = true;
> + }
> + tmp++;
> + }
> +
> + return changed;
bool returned as int.
> +}
> +
> +
> +static int
> +virPCIProbeModule(const char *moduleName)
> +{
> + g_autofree char *modulePath = NULL;
> g_autofree char *errbuf = NULL;
>
> - drvpath = virPCIDriverDir(driverName);
> + modulePath = virPCIModuleDir(moduleName);
>
> /* driver previously loaded, return */
> - if (virFileExists(drvpath))
> + if (virFileExists(modulePath))
> return 0;
>
> - if ((errbuf = virKModLoad(driverName))) {
> - VIR_WARN("failed to load driver %s: %s", driverName, errbuf);
> - goto cleanup;
> + if ((errbuf = virKModLoad(moduleName))) {
> + /* If we know failure was because of admin config, let's report that;
> + * otherwise, report a more generic failure message
> + */
> + if (virKModIsProhibited(moduleName)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to load PCI driver module %1$s: administratively prohibited"),
Based on this message it's definitely not an VIR_ERR_INTERNAL_ERROR
> + moduleName);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
and this too is not really internal
> + _("Failed to load PCI driver module %1$s: %2$s"),
> + moduleName, errbuf);
> + }
> + return -1;
> }
>
> /* driver loaded after probing */
> - if (virFileExists(drvpath))
> + if (virFileExists(modulePath))
> return 0;
>
> - cleanup:
> - /* If we know failure was because of admin config, let's report that;
> - * otherwise, report a more generic failure message
> + virReportError(VIR_ERR_INTERNAL_ERROR,
and this too.
> + _("modprobe reported success loading module '%1$s', but module is missing from /sys/module"),
> + moduleName);
> + return -1;
> +}
> +
> +/**
> + * virPCIDeviceFindDriver:
> + * @dev: initialized virPCIDevice, including desired stubDriverName
> + *
> + * Checks if there is a driver named @dev->stubDriverName already
> + * loaded. If there is, we're done. If not, look for a driver with
> + * that same name, except with dashes replaced with underscores.
> +
> + * If neither of the above is found, then look for/load the module of
> + * the underscored version of the name, and follow the links from
> + * /sys/module/$name/drivers/pci:* to the PCI driver associated with that
> + * module, and update @dev->stubDriverName with that name.
> + *
> + * On a successful return, @dev->stubDriverName will be updated with
> + * the proper name for the driver, and that driver will be loaded.
> + *
> + * returns 0 on success, -1 on failure
> + */
> +static int
> +virPCIDeviceFindDriver(virPCIDevice *dev)
> +{
> + g_autofree char *driverPath = virPCIDriverDir(dev->stubDriverName);
> + g_autofree char *moduleName = NULL;
> + g_autofree char *moduleDriversDir = NULL;
> + g_autoptr(DIR) dir = NULL;
> + struct dirent *ent;
> + int direrr;
> +
> + /* unchanged stubDriverName */
> + if (virFileExists(driverPath))
> + return 0;
> +
> + /* try replacing "-" with "_" */
> + if (virPCINameDashToUnderscore(driverPath) && virFileExists(driverPath)) {
> +
> + /* update original name in dev */
> + virPCINameDashToUnderscore(dev->stubDriverName);
> + return 0;
> + }
> +
> + /* look for a module with this name (but always replacing
> + * "-" with "_", since that's what modprobe does)
> */
> - if (virKModIsProhibited(driverName)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to load PCI driver module %1$s: administratively prohibited"),
> - driverName);
> - } else {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to load PCI driver module %1$s"),
> - driverName);
> +
> + moduleName = g_strdup(dev->stubDriverName);
> + virPCINameDashToUnderscore(moduleName);
> +
> + if (virPCIProbeModule(moduleName) < 0)
> + return -1;
> +
> + /* module was found/loaded. Now find the PCI driver it implements,
> + * linked to by /sys/module/$moduleName/drivers/pci:$driverName
> + */
> +
> + moduleDriversDir = g_strdup_printf("/sys/module/%s/drivers", moduleName);
> +
> + if (virDirOpen(&dir, moduleDriversDir) < 0)
> + return -1;
> +
> + while ((direrr = virDirRead(dir, &ent, moduleDriversDir))) {
This is only assigned but not read. Also iteration must not continue if
-1 is returned, which is usually what direrr is used for.
> +
> + if (STRPREFIX(ent->d_name, "pci:")) {
> + /* this is the link to the driver we want */
> +
> + g_autofree char *drvName = NULL;
> + g_autofree char *drvPath = NULL;
> +
> + /* extract the driver name from the link name */
> + drvName = g_strdup(ent->d_name + strlen("pci:"));
> +
> + /* make sure that driver is actually loaded */
> + drvPath = virPCIDriverDir(drvName);
> + if (!virFileExists(drvPath)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
Not an VIR_ERR_INTERNAL_ERROR
> + _("pci driver '%1$s' supposedly loaded by module '%2$s' not found in sysfs"),
> + drvName, moduleName);
> + return -1;
> + }
> +
> + g_free(dev->stubDriverName);
> + dev->stubDriverName = g_steal_pointer(&drvName);
> + return 0;
> + }
> }
>
> + virReportError(VIR_ERR_INTERNAL_ERROR,
Not an VIR_ERR_INTERNAL_ERROR
> + _("module '%1$s' does not implement any pci driver"),
> + moduleName);
> return -1;
> }
>
> +
> int
> virPCIDeviceUnbind(virPCIDevice *dev)
> {
> @@ -1291,7 +1420,6 @@ virPCIDeviceUnbindFromStub(virPCIDevice *dev)
> static int
> virPCIDeviceBindToStub(virPCIDevice *dev)
> {
> - const char *stubDriverName = dev->stubDriverName;
> g_autofree char *stubDriverPath = NULL;
> g_autofree char *driverLink = NULL;
>
> @@ -1303,30 +1431,35 @@ virPCIDeviceBindToStub(virPCIDevice *dev)
> return -1;
> }
>
> - if (!stubDriverName
> - && !(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unknown stub driver configured for PCI device %1$s"),
> - dev->name);
> - return -1;
> + if (!dev->stubDriverName) {
> +
> + const char *stubDriverName = NULL;
> +
> + if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
Not an VIR_ERR_INTERNAL_ERROR
> + _("Unknown stub driver configured for PCI device %1$s"),
> + dev->name);
> + return -1;
> + }
> + dev->stubDriverName = g_strdup(stubDriverName);
> }
>
> - if (virPCIProbeDriver(stubDriverName) < 0)
> + if (virPCIDeviceFindDriver(dev) < 0)
> return -1;
>
> - stubDriverPath = virPCIDriverDir(stubDriverName);
> + stubDriverPath = virPCIDriverDir(dev->stubDriverName);
> driverLink = virPCIFile(dev->name, "driver");
>
> if (virFileExists(driverLink)) {
> if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
> /* The device is already bound to the correct driver */
> VIR_DEBUG("Device %s is already bound to %s",
> - dev->name, stubDriverName);
> + dev->name, dev->stubDriverName);
> return 0;
> }
> }
>
> - if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0)
> + if (virPCIDeviceBindWithDriverOverride(dev, dev->stubDriverName) < 0)
> return -1;
>
> dev->unbind_from_stub = true;
With the stuff above addressed:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 1/5/24 3:03 PM, Peter Krempa wrote:
> On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote:
>> Historically libvirt hasn't differentiated between the name of a
>> loadable kernel module, and the name of the device driver that module
>> implements, but these two names can be (and usually are) at least
>> subtly different.
>>
>> For example, the loadable module called "vfio_pci" implements a PCI
>> driver called "vfio-pci". We have always used the name "vfio-pci" both
>> to load the module (with modprobe) and to check (in
>> /sys/bus/pci/drivers) if the driver is available. (This has happened
>> to work because modprobe "normalizes" all the names it is given by
>> replacing "-" with "_", so "vfio-pci" works for both loading the
>> module and checking for the driver.)
>>
>> When we recently gained the ability to manually specify the driver for
>> "virsh nodedev-detach", the fragility of this system became apparent -
>> if a user gave the "driver name" as "vfio_pci", then we would modprobe
>> the module correctly, 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, we could deal with this by telling
>> the user "always use the correct name for the driver, don't assume
>> that it has the same name as the module", 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).
>>
>> This will only get worse when an upcoming patch starts automatically
>> determining the driver to use for VFIO-assigned devices - it will look
>> in the kernel's modules.alias file to find "best" VFIO variant
>> *module* for a device, and 3 out of 4 current examples of
>> vfio-pci/variant drivers have a mismatch between module name and
>> driver name, so the current code would end up properly loading the
>> module, but then erroneously think that the driver wasn't available.
>>
>> This patch makes the code more forgiving by
>>
>> 1) checking for both $drivername and underscore($drivername) in
>> /sys/bus/pci/drivers
>>
>> 2) when we determine a module needs to be loaded, look at the link in
>> /sys/module/$modulename/driver/pci:$drivername to determine the
>> name of the driver we need to bind to the device(rather than just
>> assuming the driver has the same name as the module
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>>
>> Change from V1: I tried to simplify the explanation in the commit log message
>>
>> src/util/virpci.c | 193 +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 163 insertions(+), 30 deletions(-)
>
> [...]
>
>
>> @@ -1154,44 +1161,166 @@ virPCIDeviceReset(virPCIDevice *dev,
>> }
>>
>>
>> +/**
>> + * virPCINameDashToUnderscore:
>> + * @path: the module/driver name or path - will be directly modified
>> + *
>> + * Replace all occurences of "-" with "_" in the name
>> + * part of the path (everything after final "/"
>> + *
>> + * return true if any change was made, otherwise false.
>> + */
>> static int
>
> 'int'
>
>> -virPCIProbeDriver(const char *driverName)
>> +virPCINameDashToUnderscore(char *path)
>> {
>> - g_autofree char *drvpath = NULL;
>> + bool changed = false;
>
> 'bool'
>
>> + char *tmp = strrchr(path, '/');
>> +
>> + if (!tmp)
>> + tmp = path;
>> +
>> + while (*tmp) {
>> + if (*tmp == '-') {
>> + *tmp = '_';
>> + changed = true;
>> + }
>> + tmp++;
>> + }
>> +
>> + return changed;
>
> bool returned as int.
Sigh. How am I still alive? (well, this is pretty innocuous, but still
incorrect... Good thing I don't work on nuclear plant safety software or
something) :-/
>
>> +}
>> +
>> +
>
>
>
>> +static int
>> +virPCIProbeModule(const char *moduleName)
>> +{
>> + g_autofree char *modulePath = NULL;
>> g_autofree char *errbuf = NULL;
>>
>> - drvpath = virPCIDriverDir(driverName);
>> + modulePath = virPCIModuleDir(moduleName);
>>
>> /* driver previously loaded, return */
>> - if (virFileExists(drvpath))
>> + if (virFileExists(modulePath))
>> return 0;
>>
>> - if ((errbuf = virKModLoad(driverName))) {
>> - VIR_WARN("failed to load driver %s: %s", driverName, errbuf);
>> - goto cleanup;
>> + if ((errbuf = virKModLoad(moduleName))) {
>> + /* If we know failure was because of admin config, let's report that;
>> + * otherwise, report a more generic failure message
>> + */
>> + if (virKModIsProhibited(moduleName)) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to load PCI driver module %1$s: administratively prohibited"),
>
> Based on this message it's definitely not an VIR_ERR_INTERNAL_ERROR
I always hate trying to categorize errors. What's your opinion of the
proper category for these?
>
>> + moduleName);
>> + } else {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> and this too is not really internal
>
>> + _("Failed to load PCI driver module %1$s: %2$s"),
>> + moduleName, errbuf);
>> + }
>> + return -1;
>> }
>>
>> /* driver loaded after probing */
>> - if (virFileExists(drvpath))
>> + if (virFileExists(modulePath))
>> return 0;
>>
>> - cleanup:
>> - /* If we know failure was because of admin config, let's report that;
>> - * otherwise, report a more generic failure message
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> and this too.
>
>> + _("modprobe reported success loading module '%1$s', but module is missing from /sys/module"),
>> + moduleName);
>> + return -1;
>> +}
>> +
>> +/**
>> + * virPCIDeviceFindDriver:
>> + * @dev: initialized virPCIDevice, including desired stubDriverName
>> + *
>> + * Checks if there is a driver named @dev->stubDriverName already
>> + * loaded. If there is, we're done. If not, look for a driver with
>> + * that same name, except with dashes replaced with underscores.
>> +
>> + * If neither of the above is found, then look for/load the module of
>> + * the underscored version of the name, and follow the links from
>> + * /sys/module/$name/drivers/pci:* to the PCI driver associated with that
>> + * module, and update @dev->stubDriverName with that name.
>> + *
>> + * On a successful return, @dev->stubDriverName will be updated with
>> + * the proper name for the driver, and that driver will be loaded.
>> + *
>> + * returns 0 on success, -1 on failure
>> + */
>> +static int
>> +virPCIDeviceFindDriver(virPCIDevice *dev)
>> +{
>> + g_autofree char *driverPath = virPCIDriverDir(dev->stubDriverName);
>> + g_autofree char *moduleName = NULL;
>> + g_autofree char *moduleDriversDir = NULL;
>> + g_autoptr(DIR) dir = NULL;
>> + struct dirent *ent;
>> + int direrr;
>> +
>> + /* unchanged stubDriverName */
>> + if (virFileExists(driverPath))
>> + return 0;
>> +
>> + /* try replacing "-" with "_" */
>> + if (virPCINameDashToUnderscore(driverPath) && virFileExists(driverPath)) {
>> +
>> + /* update original name in dev */
>> + virPCINameDashToUnderscore(dev->stubDriverName);
>> + return 0;
>> + }
>> +
>> + /* look for a module with this name (but always replacing
>> + * "-" with "_", since that's what modprobe does)
>> */
>> - if (virKModIsProhibited(driverName)) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Failed to load PCI driver module %1$s: administratively prohibited"),
>> - driverName);
>> - } else {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Failed to load PCI driver module %1$s"),
>> - driverName);
>> +
>> + moduleName = g_strdup(dev->stubDriverName);
>> + virPCINameDashToUnderscore(moduleName);
>> +
>> + if (virPCIProbeModule(moduleName) < 0)
>> + return -1;
>> +
>> + /* module was found/loaded. Now find the PCI driver it implements,
>> + * linked to by /sys/module/$moduleName/drivers/pci:$driverName
>> + */
>> +
>> + moduleDriversDir = g_strdup_printf("/sys/module/%s/drivers", moduleName);
>> +
>> + if (virDirOpen(&dir, moduleDriversDir) < 0)
>> + return -1;
>> +
>> + while ((direrr = virDirRead(dir, &ent, moduleDriversDir))) {
>
> This is only assigned but not read. Also iteration must not continue if
> -1 is returned, which is usually what direrr is used for.
Again, how am I still alive? It's troublesome that I would write this,
since every single example of virDirRead in the entire tree checks for >
0 return :-/.
>
>> +
>> + if (STRPREFIX(ent->d_name, "pci:")) {
>> + /* this is the link to the driver we want */
>> +
>> + g_autofree char *drvName = NULL;
>> + g_autofree char *drvPath = NULL;
>> +
>> + /* extract the driver name from the link name */
>> + drvName = g_strdup(ent->d_name + strlen("pci:"));
>> +
>> + /* make sure that driver is actually loaded */
>> + drvPath = virPCIDriverDir(drvName);
>> + if (!virFileExists(drvPath)) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> Not an VIR_ERR_INTERNAL_ERROR
>
>> + _("pci driver '%1$s' supposedly loaded by module '%2$s' not found in sysfs"),
>> + drvName, moduleName);
>> + return -1;
>> + }
>> +
>> + g_free(dev->stubDriverName);
>> + dev->stubDriverName = g_steal_pointer(&drvName);
>> + return 0;
>> + }
>> }
>>
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> Not an VIR_ERR_INTERNAL_ERROR
>
>> + _("module '%1$s' does not implement any pci driver"),
>> + moduleName);
>> return -1;
>> }
>>
>> +
>> int
>> virPCIDeviceUnbind(virPCIDevice *dev)
>> {
>> @@ -1291,7 +1420,6 @@ virPCIDeviceUnbindFromStub(virPCIDevice *dev)
>> static int
>> virPCIDeviceBindToStub(virPCIDevice *dev)
>> {
>> - const char *stubDriverName = dev->stubDriverName;
>> g_autofree char *stubDriverPath = NULL;
>> g_autofree char *driverLink = NULL;
>>
>> @@ -1303,30 +1431,35 @@ virPCIDeviceBindToStub(virPCIDevice *dev)
>> return -1;
>> }
>>
>> - if (!stubDriverName
>> - && !(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Unknown stub driver configured for PCI device %1$s"),
>> - dev->name);
>> - return -1;
>> + if (!dev->stubDriverName) {
>> +
>> + const char *stubDriverName = NULL;
>> +
>> + if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> Not an VIR_ERR_INTERNAL_ERROR
>
>> + _("Unknown stub driver configured for PCI device %1$s"),
>> + dev->name);
>> + return -1;
>> + }
>> + dev->stubDriverName = g_strdup(stubDriverName);
>> }
>>
>> - if (virPCIProbeDriver(stubDriverName) < 0)
>> + if (virPCIDeviceFindDriver(dev) < 0)
>> return -1;
>>
>> - stubDriverPath = virPCIDriverDir(stubDriverName);
>> + stubDriverPath = virPCIDriverDir(dev->stubDriverName);
>> driverLink = virPCIFile(dev->name, "driver");
>>
>> if (virFileExists(driverLink)) {
>> if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
>> /* The device is already bound to the correct driver */
>> VIR_DEBUG("Device %s is already bound to %s",
>> - dev->name, stubDriverName);
>> + dev->name, dev->stubDriverName);
>> return 0;
>> }
>> }
>>
>> - if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0)
>> + if (virPCIDeviceBindWithDriverOverride(dev, dev->stubDriverName) < 0)
>> return -1;
>>
>> dev->unbind_from_stub = true;
>
> With the stuff above addressed:
>
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote: > Historically libvirt hasn't differentiated between the name of a > loadable kernel module, and the name of the device driver that module > implements, but these two names can be (and usually are) at least > subtly different. > > For example, the loadable module called "vfio_pci" implements a PCI > driver called "vfio-pci". We have always used the name "vfio-pci" both > to load the module (with modprobe) and to check (in > /sys/bus/pci/drivers) if the driver is available. (This has happened > to work because modprobe "normalizes" all the names it is given by > replacing "-" with "_", so "vfio-pci" works for both loading the > module and checking for the driver.) > > When we recently gained the ability to manually specify the driver for > "virsh nodedev-detach", the fragility of this system became apparent - > if a user gave the "driver name" as "vfio_pci", then we would modprobe > the module correctly, 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, we could deal with this by telling > the user "always use the correct name for the driver, don't assume > that it has the same name as the module", 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). > > This will only get worse when an upcoming patch starts automatically > determining the driver to use for VFIO-assigned devices - it will look > in the kernel's modules.alias file to find "best" VFIO variant > *module* for a device, and 3 out of 4 current examples of > vfio-pci/variant drivers have a mismatch between module name and > driver name, so the current code would end up properly loading the > module, but then erroneously think that the driver wasn't available. > > This patch makes the code more forgiving by > > 1) checking for both $drivername and underscore($drivername) in > /sys/bus/pci/drivers > > 2) when we determine a module needs to be loaded, look at the link in > /sys/module/$modulename/driver/pci:$drivername to determine the > name of the driver we need to bind to the device(rather than just > assuming the driver has the same name as the module > > Signed-off-by: Laine Stump <laine@redhat.com> > --- > > Change from V1: I tried to simplify the explanation in the commit log message I don't think this addresses Jason's comment from V1, stating that we should only deal with driver names and let modprobe find the correct module. Same thing when you are looking for the best *driver* for the device. Yes you find a module name, but you can query the module itself for the driver and just use the driver. As Jason stated, we should not deal with modules at all. Without addressing this I will not give a R-b. _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 1/5/24 9:22 AM, Peter Krempa wrote: > On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote: >> Historically libvirt hasn't differentiated between the name of a >> loadable kernel module, and the name of the device driver that module >> implements, but these two names can be (and usually are) at least >> subtly different. >> >> For example, the loadable module called "vfio_pci" implements a PCI >> driver called "vfio-pci". We have always used the name "vfio-pci" both >> to load the module (with modprobe) and to check (in >> /sys/bus/pci/drivers) if the driver is available. (This has happened >> to work because modprobe "normalizes" all the names it is given by >> replacing "-" with "_", so "vfio-pci" works for both loading the >> module and checking for the driver.) >> >> When we recently gained the ability to manually specify the driver for >> "virsh nodedev-detach", the fragility of this system became apparent - >> if a user gave the "driver name" as "vfio_pci", then we would modprobe >> the module correctly, 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, we could deal with this by telling >> the user "always use the correct name for the driver, don't assume >> that it has the same name as the module", 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). >> >> This will only get worse when an upcoming patch starts automatically >> determining the driver to use for VFIO-assigned devices - it will look >> in the kernel's modules.alias file to find "best" VFIO variant >> *module* for a device, and 3 out of 4 current examples of >> vfio-pci/variant drivers have a mismatch between module name and >> driver name, so the current code would end up properly loading the >> module, but then erroneously think that the driver wasn't available. >> >> This patch makes the code more forgiving by >> >> 1) checking for both $drivername and underscore($drivername) in >> /sys/bus/pci/drivers >> >> 2) when we determine a module needs to be loaded, look at the link in >> /sys/module/$modulename/driver/pci:$drivername to determine the >> name of the driver we need to bind to the device(rather than just >> assuming the driver has the same name as the module >> >> Signed-off-by: Laine Stump <laine@redhat.com> >> --- >> >> Change from V1: I tried to simplify the explanation in the commit log message > > I don't think this addresses Jason's comment from V1, stating that we > should only deal with driver names and let modprobe find the correct > module. modprobe has no way of "finding" the proper module for a given driver - all it does is replace "-" with "_" in the name and hope for the best. The problem is that the way to get to module name from a driver name is to follow the links in the driver's directory in sysfs, but the driver's directory in sysfs doesn't exist until the module has been loaded, and if the module is already loaded then you don't need to know the name of the module any more. > > Same thing when you are looking for the best *driver* for the device. > Yes you find a module name, but you can query the module itself for the > driver and just use the driver. That is what we end up doing - when we find the module name for a variant, we load that module and then follow the link to get to the driver. > > As Jason stated, we should not deal with modules at all. I disagree. If the module for a driver isn't loaded, then (unless the modulename happens to be (underscore($drivername)) there is no way to figure out what module to load. (If the module had already been loaded, then we could follow the links in sysfs from driver to module, but of course if the module is already loaded then we don't need to know the module name!) In the end, though, with the currently existing VFIO/VFIO-variant drivers, the only point of contention is that my current code allows specifying <driver name='vfio_pci'/> in the XML, and you (and Jason) are saying that we shouldn't allow that, but should require <driver name='vfio-pci'/>. Since, at some point *internally* we do need to support starting with "vfio_pci" and end up with "vfio-pci", this all comes down to whether the extra flexibility is at a higher level (as in this patch) or if it's only done at a lower level (i.e. doing it during the virPCIDeviceFindBestVFIOVariant() in patch 13). I'll see if I can make something that does it at that level and resubmit (but will then forward any bug reports about failing with <driver name='vfio_pci'/> to you :-P) > Without addressing this I will not give a R-b. > _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Jan 05, 2024 at 10:33:33 -0500, Laine Stump wrote: > On 1/5/24 9:22 AM, Peter Krempa wrote: > > On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote: [...] > > > Change from V1: I tried to simplify the explanation in the commit log message > > > > I don't think this addresses Jason's comment from V1, stating that we > > should only deal with driver names and let modprobe find the correct > > module. > > modprobe has no way of "finding" the proper module for a given driver - all > it does is replace "-" with "_" in the name and hope for the best. The > problem is that the way to get to module name from a driver name is to > follow the links in the driver's directory in sysfs, but the driver's > directory in sysfs doesn't exist until the module has been loaded, and if > the module is already loaded then you don't need to know the name of the > module any more. > > > > > > Same thing when you are looking for the best *driver* for the device. > > Yes you find a module name, but you can query the module itself for the > > driver and just use the driver. > > That is what we end up doing - when we find the module name for a variant, > we load that module and then follow the link to get to the driver. > > > > > As Jason stated, we should not deal with modules at all. > > I disagree. If the module for a driver isn't loaded, then (unless the > modulename happens to be (underscore($drivername)) there is no way to figure > out what module to load. > > (If the module had already been loaded, then we could follow the links in > sysfs from driver to module, but of course if the module is already loaded > then we don't need to know the module name!) > > In the end, though, with the currently existing VFIO/VFIO-variant drivers, > the only point of contention is that my current code allows specifying > <driver name='vfio_pci'/> in the XML, and you (and Jason) are saying that we > shouldn't allow that, but should require <driver name='vfio-pci'/>. Since, > at some point *internally* we do need to support starting with "vfio_pci" > and end up with "vfio-pci", this all comes down to whether the extra > flexibility is at a higher level (as in this patch) or if it's only done at > a lower level (i.e. doing it during the virPCIDeviceFindBestVFIOVariant() in > patch 13). I'll see if I can make something that does it at that level and > resubmit (but will then forward any bug reports about failing with <driver > name='vfio_pci'/> to you :-P) Fair enough, I think this explanation is sufficient for me. I've done a proper review in another subthread. _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.