On Fri, 2015-11-20 at 11:33 -0500, Laine Stump wrote:
On 11/14/2015 03:36 AM, Shivaprasad G Bhat wrote:
The reprobe needs to be called multiple times for vfio devices for each device in the iommu group in future patch. So split the reprobe into a new function. No functional change.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 89e69e2..febf100 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1099,6 +1099,36 @@ virPCIIsKnownStub(char *driver) }
static int +virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, + const char *driver, + const char *drvdir) +{ + char *path = NULL; + int ret = -1; + + /* Trigger a re-probe of the device is not in the stub's dynamic
As long as you're moving the code, s/of/if/
+ * ID table. If the stub is available, but 'remove_id' isn't + * available, then re-probing would just cause the device to be + * re-bound to the stub. + */ + if (driver && !(path = virPCIDriverFile(driver, "remove_id"))) + goto cleanup; + + if (!driver || !virFileExists(drvdir) || virFileExists(path)) { + 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; + } + } + ret = 0; + cleanup: + VIR_FREE(path); + return ret; +} + +static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { int result = -1; @@ -1150,24 +1180,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) 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 - * re-bound to the stub. - */ - VIR_FREE(path); - if (driver && !(path = virPCIDriverFile(driver, "remove_id"))) + if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) goto cleanup;
- if (!driver || !virFileExists(drvdir) || virFileExists(path)) { - 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; - } - } - result = 0;
cleanup:
Seems safe, but is this really what we want to do? I haven't read/understood the remaining patches yet, but this makes it sound like what is going to happen is that all of the devices will be unbound from vfio-pci immediately, so they are "in limbo", and will then be reprobed once all devices are unused (and therefore unbound from vfio-pci).
I think that may be a bit dangerous. Instead, we should leave the devices bound to vfio-pci until all of them are unused, and at that time, we should unbind them all from vfio-pci, then reprobe them all. (again, I may have misunderstood the direction, if so ignore this).
If I am misunderstanding, and unbinding from vfio-pci will also be delayed until all devices are unused, then ACK.
Why don't we start making use of the driver_override feature that we've had in the kernel instead of continuing to hack on the racy add_id/remove_id stuff? We've already solved the problem in the kernel. Want to bind a device to vfio-pci: echo vfio-pci > /sys/bus/pci/devices/<dev>/driver_override if [ -e /sys/bus/pci/devices/<dev>/driver ]; then echo <dev> > /sys/bus/pci/devices/<dev>/driver/unbind fi echo <dev> > /sys/bus/pci/drivers_probe To rebind, replace vfio-pci in the first echo with null and repeat the rest. The affects are limited to a single device, we're not going to have surprise unbound devices show up bound to the driver, and we don't race anyone manipulating other devices. Thanks, Alex