[libvirt] [PATCH V2] virpci: support driver_override sysfs interface
libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver to a PCI device. The new_id interface is known to be buggy and racey, hence a more deterministic interface was introduced in the 3.12 kernel: driver_override. For more details see https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html This patch adds support for the driver_override interface by - adding new virPCIDevice{BindTo,UnbindFrom}StubWithOverride functions that use the driver_override interface - renames the existing virPCIDevice{BindTo,UnbindFrom}Stub functions to virPCIDevice{BindTo,UnbindFrom}StubWithNewid to perserve existing behavior on new_id interface - changes virPCIDevice{BindTo,UnbindFrom}Stub function to call one of the above depending on availability of driver_override The patch includes a bit of duplicate code, but allows for easily dropping the new_id code once support for older kernels is no longer desired. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- V1: https://www.redhat.com/archives/libvir-list/2016-July/msg00370.html Changes since V1: - drop patch1 - change patch2 to preserve the existing new_id code and add new functions to implement the driver_override interface src/util/virpci.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 132948d..6c8174a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1089,8 +1089,54 @@ virPCIDeviceUnbind(virPCIDevicePtr dev) return ret; } +/* + * Bind a PCI device to a driver using driver_override sysfs interface. + * E.g. + * + * echo driver-name > /sys/bus/pci/devices/0000:03:00.0/driver_override + * echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind + * echo 0000:03:00.0 > /sys/bus/pci/drivers_probe + * + * An empty driverName will cause the device to be bound to its + * preferred driver. + */ static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, + const char *driverName) +{ + int ret = -1; + char *path; + + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileWriteStr(path, driverName, 0) < 0) { + virReportSystemError(errno, + _("Failed to add driver '%s' to driver_override " + " interface of PCI device '%s'"), + driverName, dev->name); + goto cleanup; + } + + if (virPCIDeviceUnbind(dev) < 0) + goto cleanup; + + if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to trigger a probe for PCI device '%s'"), + dev->name); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + +static int +virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev) { int result = -1; char *drvdir = NULL; @@ -1191,9 +1237,41 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) return result; } +static int +virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) +{ + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); + return 0; + } + + return virPCIDeviceBindWithDriverOverride(dev, "\n"); +} + +static int +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceUnbindFromStubWithOverride(dev); + else + ret = virPCIDeviceUnbindFromStubWithNewid(dev); + + VIR_FREE(path); + return ret; +} static int -virPCIDeviceBindToStub(virPCIDevicePtr dev) +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) { int result = -1; bool reprobe = false; @@ -1345,6 +1423,75 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return result; } +static int +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) +{ + int ret = -1; + const char *stubDriverName; + char *stubDriverPath = NULL; + char *driverLink = NULL; + + /* Check the device is configured to use one of the known stub drivers */ + if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No stub driver configured for PCI device %s"), + dev->name); + return -1; + } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown stub driver configured for PCI device %s"), + dev->name); + return -1; + } + + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || + !(driverLink = virPCIFile(dev->name, "driver"))) + goto cleanup; + + 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); + ret = 0; + goto cleanup; + } + } + + if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0) + goto cleanup; + + dev->unbind_from_stub = true; + ret = 0; + + cleanup: + VIR_FREE(stubDriverPath); + VIR_FREE(driverLink); + return ret; +} + +static int +virPCIDeviceBindToStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceBindToStubWithOverride(dev); + else + ret = virPCIDeviceBindToStubWithNewid(dev); + + VIR_FREE(path); + return ret; +} + /* virPCIDeviceDetach: * * Detach this device from the host driver, attach it to the stub -- 2.1.4
Hi Laine, Did you have a chance to look at V2 of this patch? As discussed in V1, I left the existing code untouched and added new functions for the driver_override interface. Thanks! Regards, Jim Jim Fehlig wrote:
libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver to a PCI device. The new_id interface is known to be buggy and racey, hence a more deterministic interface was introduced in the 3.12 kernel: driver_override. For more details see
https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html
This patch adds support for the driver_override interface by
- adding new virPCIDevice{BindTo,UnbindFrom}StubWithOverride functions that use the driver_override interface - renames the existing virPCIDevice{BindTo,UnbindFrom}Stub functions to virPCIDevice{BindTo,UnbindFrom}StubWithNewid to perserve existing behavior on new_id interface - changes virPCIDevice{BindTo,UnbindFrom}Stub function to call one of the above depending on availability of driver_override
The patch includes a bit of duplicate code, but allows for easily dropping the new_id code once support for older kernels is no longer desired.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
V1:
https://www.redhat.com/archives/libvir-list/2016-July/msg00370.html
Changes since V1: - drop patch1 - change patch2 to preserve the existing new_id code and add new functions to implement the driver_override interface
src/util/virpci.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 132948d..6c8174a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1089,8 +1089,54 @@ virPCIDeviceUnbind(virPCIDevicePtr dev) return ret; }
+/* + * Bind a PCI device to a driver using driver_override sysfs interface. + * E.g. + * + * echo driver-name > /sys/bus/pci/devices/0000:03:00.0/driver_override + * echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind + * echo 0000:03:00.0 > /sys/bus/pci/drivers_probe + * + * An empty driverName will cause the device to be bound to its + * preferred driver. + */ static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, + const char *driverName) +{ + int ret = -1; + char *path; + + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileWriteStr(path, driverName, 0) < 0) { + virReportSystemError(errno, + _("Failed to add driver '%s' to driver_override " + " interface of PCI device '%s'"), + driverName, dev->name); + goto cleanup; + } + + if (virPCIDeviceUnbind(dev) < 0) + goto cleanup; + + if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to trigger a probe for PCI device '%s'"), + dev->name); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + +static int +virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev) { int result = -1; char *drvdir = NULL; @@ -1191,9 +1237,41 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) return result; }
+static int +virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) +{ + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); + return 0; + } + + return virPCIDeviceBindWithDriverOverride(dev, "\n"); +} + +static int +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceUnbindFromStubWithOverride(dev); + else + ret = virPCIDeviceUnbindFromStubWithNewid(dev); + + VIR_FREE(path); + return ret; +}
static int -virPCIDeviceBindToStub(virPCIDevicePtr dev) +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) { int result = -1; bool reprobe = false; @@ -1345,6 +1423,75 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return result; }
+static int +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) +{ + int ret = -1; + const char *stubDriverName; + char *stubDriverPath = NULL; + char *driverLink = NULL; + + /* Check the device is configured to use one of the known stub drivers */ + if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No stub driver configured for PCI device %s"), + dev->name); + return -1; + } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown stub driver configured for PCI device %s"), + dev->name); + return -1; + } + + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || + !(driverLink = virPCIFile(dev->name, "driver"))) + goto cleanup; + + 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); + ret = 0; + goto cleanup; + } + } + + if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0) + goto cleanup; + + dev->unbind_from_stub = true; + ret = 0; + + cleanup: + VIR_FREE(stubDriverPath); + VIR_FREE(driverLink); + return ret; +} + +static int +virPCIDeviceBindToStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceBindToStubWithOverride(dev); + else + ret = virPCIDeviceBindToStubWithNewid(dev); + + VIR_FREE(path); + return ret; +} + /* virPCIDeviceDetach: * * Detach this device from the host driver, attach it to the stub
On 08/01/2016 11:36 PM, Jim Fehlig wrote:
libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver to a PCI device. The new_id interface is known to be buggy and racey, hence a more deterministic interface was introduced in the 3.12 kernel: driver_override. For more details see
https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html
That message details the change to the kernel that caused the regression for Xen, but not the driver_override interface. Maybe you could add a link to the kernel commit that adds driver_override: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci-driver.c?h=v3.12&id=782a985d7af26db39e86070d28f987cad21313c0 Everything else looks good, and passes my tests for vfio device assignment (including when the host driver has been blacklisted). ACK. (Sorry I forgot about this earlier in the month :-/)
This patch adds support for the driver_override interface by
- adding new virPCIDevice{BindTo,UnbindFrom}StubWithOverride functions that use the driver_override interface - renames the existing virPCIDevice{BindTo,UnbindFrom}Stub functions to virPCIDevice{BindTo,UnbindFrom}StubWithNewid to perserve existing behavior on new_id interface - changes virPCIDevice{BindTo,UnbindFrom}Stub function to call one of the above depending on availability of driver_override
The patch includes a bit of duplicate code, but allows for easily dropping the new_id code once support for older kernels is no longer desired.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
V1:
https://www.redhat.com/archives/libvir-list/2016-July/msg00370.html
Changes since V1: - drop patch1 - change patch2 to preserve the existing new_id code and add new functions to implement the driver_override interface
src/util/virpci.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 132948d..6c8174a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1089,8 +1089,54 @@ virPCIDeviceUnbind(virPCIDevicePtr dev) return ret; }
+/* + * Bind a PCI device to a driver using driver_override sysfs interface. + * E.g. + * + * echo driver-name > /sys/bus/pci/devices/0000:03:00.0/driver_override + * echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind + * echo 0000:03:00.0 > /sys/bus/pci/drivers_probe + * + * An empty driverName will cause the device to be bound to its + * preferred driver. + */ static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, + const char *driverName) +{ + int ret = -1; + char *path; + + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileWriteStr(path, driverName, 0) < 0) { + virReportSystemError(errno, + _("Failed to add driver '%s' to driver_override " + " interface of PCI device '%s'"), + driverName, dev->name); + goto cleanup; + } + + if (virPCIDeviceUnbind(dev) < 0) + goto cleanup; + + if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to trigger a probe for PCI device '%s'"), + dev->name); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + +static int +virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev) { int result = -1; char *drvdir = NULL; @@ -1191,9 +1237,41 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) return result; }
+static int +virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) +{ + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); + return 0; + } + + return virPCIDeviceBindWithDriverOverride(dev, "\n"); +} + +static int +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceUnbindFromStubWithOverride(dev); + else + ret = virPCIDeviceUnbindFromStubWithNewid(dev); + + VIR_FREE(path); + return ret; +}
static int -virPCIDeviceBindToStub(virPCIDevicePtr dev) +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) { int result = -1; bool reprobe = false; @@ -1345,6 +1423,75 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return result; }
+static int +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) +{ + int ret = -1; + const char *stubDriverName; + char *stubDriverPath = NULL; + char *driverLink = NULL; + + /* Check the device is configured to use one of the known stub drivers */ + if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No stub driver configured for PCI device %s"), + dev->name); + return -1; + } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown stub driver configured for PCI device %s"), + dev->name); + return -1; + } + + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || + !(driverLink = virPCIFile(dev->name, "driver"))) + goto cleanup; + + 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); + ret = 0; + goto cleanup; + } + } + + if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0) + goto cleanup; + + dev->unbind_from_stub = true; + ret = 0; + + cleanup: + VIR_FREE(stubDriverPath); + VIR_FREE(driverLink); + return ret; +} + +static int +virPCIDeviceBindToStub(virPCIDevicePtr dev) +{ + int ret; + char *path; + + /* + * Prefer using the device's driver_override interface, falling back + * to the unpleasant new_id interface. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileExists(path)) + ret = virPCIDeviceBindToStubWithOverride(dev); + else + ret = virPCIDeviceBindToStubWithNewid(dev); + + VIR_FREE(path); + return ret; +} + /* virPCIDeviceDetach: * * Detach this device from the host driver, attach it to the stub
On 08/30/2016 11:59 PM, Laine Stump wrote:
On 08/01/2016 11:36 PM, Jim Fehlig wrote:
libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver to a PCI device. The new_id interface is known to be buggy and racey, hence a more deterministic interface was introduced in the 3.12 kernel: driver_override. For more details see
https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html
That message details the change to the kernel that caused the regression for Xen, but not the driver_override interface. Maybe you could add a link to the kernel commit that adds driver_override:
Yep, good point. Nice to have a description of the interface and examples of its use. I'll add it to the commit messages.
Everything else looks good, and passes my tests for vfio device assignment (including when the host driver has been blacklisted).
ACK. (Sorry I forgot about this earlier in the month :-/)
Thanks! I'll wait until 2.2.0 is out before pushing this. Regards, Jim
On 08/31/2016 01:42 PM, Jim Fehlig wrote:
On 08/30/2016 11:59 PM, Laine Stump wrote:
On 08/01/2016 11:36 PM, Jim Fehlig wrote:
libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver to a PCI device. The new_id interface is known to be buggy and racey, hence a more deterministic interface was introduced in the 3.12 kernel: driver_override. For more details see
https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html That message details the change to the kernel that caused the regression for Xen, but not the driver_override interface. Maybe you could add a link to the kernel commit that adds driver_override:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci-driver.c?h=v3.12&id=782a985d7af26db39e86070d28f987cad21313c0 Yep, good point. Nice to have a description of the interface and examples of its use. I'll add it to the commit messages.
Everything else looks good, and passes my tests for vfio device assignment (including when the host driver has been blacklisted).
ACK. (Sorry I forgot about this earlier in the month :-/)
Thanks! I'll wait until 2.2.0 is out before pushing this.
Pushed now that the release is out. Regards, Jim
participants (2)
-
Jim Fehlig -
Laine Stump