On 07/29/2016 11:43 AM, Laine Stump wrote: > On 07/11/2016 02:23 PM, Jim Fehlig wrote: >> 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@xxxxxxxx> >> --- >> 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; >> + } >> + > > For unbinding the stub, you're just adding in the "clearing" of > driver_override, without eliminating any of the other stuff that's done for > the older deprecated method. If the driver_override file exists, you shouldn't > do *any* of the old operations, just do what is described in the commit log > message of the patch that added driver_override to the kernel: > > https://www.redhat.com/archives/libvir-list/2014-May/msg00670.html > > I'll review the rest of it based on what ends up in the code rather than the > diffs, since the diffs will be messed up when you get rid of patch 1/2... Yeah, I was trying to shoehorn this in the existing code, which we have found to be unwise. > > >> /* 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; > > What is this "new_slot" stuff all about? Are you certain that it's still > needed when using driver_override, or is it just some lore from the old method > that isn't needed anymore? (I'm unable to test this because I don't have a > host with Xen). The only things that should be needed are: I was certain it was needed in my last round of testing, but while working on V2 I've found it is unnecessary. > > > 1) write the stub driver name to driver_override > 2) write the device's PCI address to driver/unbind > 3) write the device's PCI address to /sys/bus/pci/drivers_probe. > > (As a matter of fact, it's exactly the same operation as when binding back to > the host driver, just the string written to driver_override changes, so there > should probably be a common routine that takes the driver name as an argument > - maybe just add driverName to this function and rename it?). Good suggestion. I've incorporated it into V2 https://www.redhat.com/archives/libvir-list/2016-August/msg00087.html Regards, Jim -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list