On 07/11/2016 02:23 PM, Jim Fehlig wrote: Looking at the original code, the condition (whether the device is bound to the stub driver) is checked after writing the PCI ID of the device to the stub driver's "new_id" node. If you look here: https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-pci It says that when you write a PCI ID to a driver's "new_id", if there are any devices with the given PCI ID currently not bound to any driver, they will be immediately bound to the given driver. So I don't think the check is unnecessary - if the device wasn't bound to any driver to begin with (e.g. if the host driver for the device was blacklisted), it will be bound to the stub driver *immediately* after writing the PCI ID to new_id (ie before getting to the 2nd check). So the condition isn't unnecessarily checked. It really can happen that we weren't bound to the stub in the first case, but were by the time we get to the 2nd. On the other hand, this points out how utterly atrocious the new_id interface is - when I tested this by blacklisting the igbvf driver (so all the VFs of my 82576 card would have no driver bound to them), then started a domain that used a single VF, *all* of the VFs were suddenly bound to vfio-pci !!! I also made a bunch of comments below before I noticed this part that I've put at the top, but in the end I think that especially since this whole bind/unbind/new_id interface is a dying thing, it would be better to leave it untouched (to avoid unexpected regressions) unless it's going to make a significant difference in how the driver_override stuff is added in. I understand where you got these two lines from (they were part of the 2nd "if (virFileLinkPointsTo(driverLink, stubDriverPath))" clause that you removed), but I don't think they should be put here - if we start out with the device already bound to the stub driver, then when we're finished we *won't* want to unbind_from_stub *or* remove_slot. To test this I commented them out and did several tests with three different builds: A) a build without this patch B) a build with this patch as-is C) a build with this patch, but with the two lines above commented out. With each libvirt build, I started then destroyed a domain with a <hostdev managed='yes'>, looking at the driver link in the devices sysfs directory before, during, and after the test. I did this with the following starting states: 1) device is bound to host driver before we start 2) device is bound to stub driver before we start 3) device isn't bound to *any* driver before we start In all those (9) cases the device ends up bound to the same driver at the end as it was bound to at the beginning. (I also tried starting the domain, then restarting libvirtd and then destroying the domain - both the old and new code have the same bug: even if the device was bound to the stub driver in the beginning, it ends up being bound to the host driver (or nothing, if the host driver has been blacklisted in modprobe.d) at the end.) Result: I still believe those two lines above shouldn't be added. Maybe instead of just "later" you could say "later when we unbind from the stub". As mentioned above, this really *could* happen -writing to new_id could immediately bind the device to the stub driver. And if it wasn't immediately bound to the stub, that means it was already bound to something else, so we need to write the device's PCI address to the original driver's "unbind" (and *that* will cause it to be I think the above conditional also needs to stay - apparently it's possible that unbinding from the host driver still wouldn't be enough to get the device bound to the stuf, in which case you'd need to write the device's PCI address to the stub driver's "new_slot" in order to make it bind. But you don't want to do that unconditionally - I'd wager that it's more expenside to do the remove_slot later, and that's why the code is trying to avoid it. In the end, I think this function should remain as it is.

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; } -- 2.1.4

On 07/29/2016 11:43 AM, Laine Stump wrote: Yeah, I was trying to shoehorn this in the existing code, which we have found to be unwise. I was certain it was needed in my last round of testing, but while working on V2 I've found it is unnecessary. Good suggestion. I've incorporated it into V2 https://www.redhat.com/archives/libvir-list/2016-August/msg00087.html Regards, Jim