Re: [libvirt] [PATCH 2/2] virpci: support driver_override sysfs interface

Currently, 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 changes the stub binding/unbinding code to use the driver_override interface if present. If not present, the new_id interface will be used to provide compatibility with older kernels lacking driver_override. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/util/virpci.c | 199 +++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 152 insertions(+), 47 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 127b3b6..3c2fc9f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1158,6 +1158,19 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) VIR_DEBUG("Reprobing for PCI device %s", dev->name); + /* Remove driver_override if it exists */ + VIR_FREE(path); + if (!(path = virPCIFile(dev->name, "driver_override"))) + goto cleanup; + + if (virFileExists(path) && virFileWriteStr(path, "\n", 0) < 0) { + virReportSystemError(errno, + _("Failed to remove stub driver from " + "driver_override interface of PCI device '%s'"), + dev->name); + goto cleanup; + } +

/* Trigger a re-probe of the device is not in the stub's dynamic * ID table. If the stub is available, but 'remove_id' isn't * available, then re-probing would just cause the device to be @@ -1193,49 +1206,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) static int -virPCIDeviceBindToStub(virPCIDevicePtr dev) +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev, + const char *stubDriverName) { - int result = -1; - char *stubDriverPath = NULL; - char *driverLink = NULL; - char *path = NULL; /* reused for different purposes */ - const char *stubDriverName = NULL; + int ret = -1; + char *path = NULL; virErrorPtr err = 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); - dev->unbind_from_stub = true; - dev->remove_slot = true; - result = 0; - goto cleanup; - } - /* - * If the device is bound to a driver that is not the stub, we'll - * need to reprobe later - */ - dev->reprobe = true; - } - /* Add the PCI device ID to the stub's dynamic ID table; * this is needed to allow us to bind the device to the stub. * Note: if the device is not currently bound to any driver, @@ -1283,7 +1260,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) } dev->unbind_from_stub = true; - result = 0; + ret = 0; remove_id: err = virSaveLastError(); @@ -1299,7 +1276,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) "cannot be probed again.", dev->id, stubDriverName); } dev->reprobe = false; - result = -1; + ret = -1; goto cleanup; } @@ -1314,11 +1291,143 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) "cannot be probed again.", dev->id, stubDriverName); } dev->reprobe = false; - result = -1; + ret = -1; goto cleanup; } cleanup: + VIR_FREE(path); + + if (err) + virSetError(err); + virFreeError(err); + + return ret; +} + + +static int +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev, + const char *stubDriverName) +{ + int ret = -1; + char *path = NULL; + + /* + * Add stub to the device's driver_override, falling back to + * adding the device ID to the stub's dynamic ID table. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + return -1; + + if (virFileWriteStr(path, stubDriverName, 0) < 0) { + virReportSystemError(errno, + _("Failed to add stub driver '%s' to " + "driver_override interface of PCI device '%s'"), + stubDriverName, dev->name); + goto cleanup; + } + + if (virPCIDeviceUnbind(dev) < 0) + goto cleanup; + + /* Xen's pciback.ko wants you to use new_slot first */ + VIR_FREE(path); + if (!(path = virPCIDriverFile(stubDriverName, "new_slot"))) + goto cleanup;

+ + if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to add slot for " + "PCI device '%s' to %s"), + dev->name, stubDriverName); + goto cleanup; + } + dev->remove_slot = true; + + if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to trigger a re-probe for PCI device '%s'"), + dev->name); + goto cleanup; + } + + /* + * Device is now bound to the stub. Set reprobe so it will be re-bound + * when unbinding from the stub. + */ + dev->reprobe = true; + dev->unbind_from_stub = true; + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + + +static int +virPCIDeviceBindToStub(virPCIDevicePtr dev) +{ + int result = -1; + char *stubDriverPath = NULL; + char *driverLink = NULL; + char *path = NULL; /* reused for different purposes */ + const char *stubDriverName = 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); + dev->unbind_from_stub = true; + dev->remove_slot = true;

+ result = 0; + goto cleanup; + } + /* + * If the device is bound to a driver that is not the stub, we'll + * need to reprobe later + */ + dev->reprobe = true; + } + + /* + * Add stub to the device's driver_override, falling back to + * adding the device ID to the stub's dynamic ID table. + */ + if (!(path = virPCIFile(dev->name, "driver_override"))) + goto cleanup; + + if (virFileExists(path)) { + if (virPCIDeviceBindToStubWithOverride(dev, stubDriverName) < 0) + goto cleanup; + } else { + if (virPCIDeviceBindToStubWithNewid(dev, stubDriverName) < 0) + goto cleanup; + }

+ + result = 0; + + cleanup: VIR_FREE(stubDriverPath); VIR_FREE(driverLink); VIR_FREE(path); @@ -1326,10 +1435,6 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) if (result < 0) virPCIDeviceUnbindFromStub(dev); - if (err) - virSetError(err); - virFreeError(err); - return result; }