The stubDriver name can be used to make useful decisions if readily available. Set it if bound to a valid one during initialisation. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virpci.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 35b1459..5acf486 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1080,6 +1080,22 @@ static const char *virPCIKnownStubs[] = { NULL }; +static bool virPCIIsAKnownStub(char *driver) +{ + const char **stubTest; + bool ret = false; + + for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) { + if (STREQ_NULLABLE(driver, *stubTest)) { + ret = true; + VIR_DEBUG("Found stub driver %s", *stubTest); + break; + } + } + + return ret; +} + static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { @@ -1087,8 +1103,6 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) char *drvdir = NULL; char *path = NULL; char *driver = NULL; - const char **stubTest; - bool isStub = false; /* If the device is currently bound to one of the "well known" * stub drivers, then unbind it, otherwise ignore it. @@ -1105,14 +1119,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) goto remove_slot; /* If the device isn't bound to a known stub, skip the unbind. */ - for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) { - if (STREQ(driver, *stubTest)) { - isStub = true; - VIR_DEBUG("Found stub driver %s", *stubTest); - break; - } - } - if (!isStub) + if (!virPCIIsAKnownStub(driver)) goto remove_slot; if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) @@ -1556,6 +1563,8 @@ virPCIDeviceNew(unsigned int domain, virPCIDevicePtr dev; char *vendor = NULL; char *product = NULL; + char *drvpath = NULL; + char *driver = NULL; if (VIR_ALLOC(dev) < 0) return NULL; @@ -1603,9 +1612,16 @@ virPCIDeviceNew(unsigned int domain, goto error; } + if (virPCIDeviceGetDriverPathAndName(dev, &drvpath, &driver) < 0) + goto cleanup; + + if (virPCIIsAKnownStub(driver)) + dev->stubDriver = driver; + VIR_DEBUG("%s %s: initialized", dev->id, dev->name); cleanup: + VIR_FREE(drvpath); VIR_FREE(product); VIR_FREE(vendor); return dev;

Thanks for the comments Andrea. On Fri, Oct 30, 2015 at 8:27 PM, Andrea Bolognani <abologna(a)redhat.com&gt; wrote: Sure.. Will do that. Its used in P3 as well in virHostdevPCINodeDeviceReAttach(). I want to keep that function as simple as it is now. And as you pointed out, i am using it in P7 too.Hope its okay now.

The iommu group number need not be fetched from the sysfs everytime as it remains constant. Fetch it once during allocation Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virpci.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 5acf486..eba285a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -75,6 +75,7 @@ struct _virPCIDevice { bool has_pm_reset; bool managed; char *stubDriver; + unsigned int iommuGroup; /* used by reattach function */ bool unbind_from_stub; @@ -1565,6 +1566,8 @@ virPCIDeviceNew(unsigned int domain, char *product = NULL; char *drvpath = NULL; char *driver = NULL; + virPCIDeviceAddress devAddr = { domain, bus, + slot, function }; if (VIR_ALLOC(dev) < 0) return NULL; @@ -1618,6 +1621,8 @@ virPCIDeviceNew(unsigned int domain, if (virPCIIsAKnownStub(driver)) dev->stubDriver = driver; + dev->iommuGroup = virPCIDeviceAddressGetIOMMUGroupNum(&devAddr); + VIR_DEBUG("%s %s: initialized", dev->id, dev->name); cleanup:

It is incorrect to attempt the device reattach of a function, when some other domain is using some functions belonging to the same iommu group. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virhostdev.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index de029a0..91f28e9 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1590,6 +1590,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { virPCIDeviceAddressPtr devAddr = NULL; + bool usesVfio = STREQ_NULLABLE(virPCIDeviceGetStubDriver(pci), "vfio-pci"); struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1600,8 +1601,16 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, if (!(devAddr = virPCIDeviceGetAddress(pci))) goto out; - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) + if (usesVfio) { + /* Doesn't matter which function. If any domain is actively using the + iommu group, refuse to reattach */ + if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, + virHostdevIsPCINodeDeviceUsed, + &data) < 0) + goto out; + } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) { goto out; + } virPCIDeviceReattachInit(pci);

Before unbind from stub driver libvirt should be sure the guest is not using the device anymore. The libvirt today waits for pci-stub driver alone in /proc/iommu. The same wait is needed for vfio devices too. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virhostdev.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 91f28e9..6247477 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -756,6 +756,13 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) usleep(100*1000); retries--; } + } else if (STREQ(virPCIDeviceGetStubDriver(dev), "vfio-pci")) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(dev, "vfio-pci") + && retries) { + usleep(100*1000); + retries--; + } } if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs,

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(a)linux.vnet.ibm.com&gt; --- src/util/virpci.c | 54 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index eba285a..2709ddd 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1097,6 +1097,37 @@ static bool virPCIIsAKnownStub(char *driver) return ret; } +static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char *drvdir) +{ + char *path = NULL; + int result = -1; + + if (!dev->reprobe) + return 0; + + /* 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. + */ + 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; + } + } + result = 0; + cleanup: + VIR_FREE(path); + dev->reprobe = false; + return result; +} + static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { @@ -1144,28 +1175,8 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) dev->remove_slot = false; reprobe: - if (!dev->reprobe) { - result = 0; + if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) 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"))) - 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; @@ -1173,7 +1184,6 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) /* do not do it again */ dev->unbind_from_stub = false; dev->remove_slot = false; - dev->reprobe = false; VIR_FREE(drvdir); VIR_FREE(path);

The inactiveDevs need to be selectively altered for more than one device in case of vfio devices. So, pass the whole list. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virpci.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 2709ddd..6c24a81 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1129,7 +1129,9 @@ static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char } static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, + virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED, + virPCIDeviceListPtr inactiveDevs) { int result = -1; char *drvdir = NULL; @@ -1177,6 +1179,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) reprobe: if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) goto cleanup; + /* Steal the dev from list inactiveDevs */ + if (inactiveDevs) + virPCIDeviceListDel(inactiveDevs, dev); result = 0; @@ -1195,7 +1200,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) static int virPCIDeviceBindToStub(virPCIDevicePtr dev, - const char *stubDriverName) + const char *stubDriverName, + virPCIDeviceListPtr activeDevs, + virPCIDeviceListPtr inactiveDevs) { int result = -1; bool reprobe = false; @@ -1317,6 +1324,14 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, goto cleanup; } + /* Add *a copy of* the dev into list inactiveDevs, if + * it's not already there. + */ + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) { + result = -1; + } + cleanup: VIR_FREE(stubDriverPath); VIR_FREE(driverLink); @@ -1324,7 +1339,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, if (result < 0) { VIR_FREE(newDriverName); - virPCIDeviceUnbindFromStub(dev); + virPCIDeviceUnbindFromStub(dev, activeDevs, NULL); } else { VIR_FREE(dev->stubDriver); dev->stubDriver = newDriverName; @@ -1371,16 +1386,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev, return -1; } - if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0) - return -1; - - /* Add *a copy of* the dev into list inactiveDevs, if - * it's not already there. - */ - if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && - virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) { + if (virPCIDeviceBindToStub(dev, dev->stubDriver, + activeDevs, inactiveDevs) < 0) return -1; - } return 0; } @@ -1396,13 +1404,9 @@ virPCIDeviceReattach(virPCIDevicePtr dev, return -1; } - if (virPCIDeviceUnbindFromStub(dev) < 0) + if (virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs) < 0) return -1; - /* Steal the dev from list inactiveDevs */ - if (inactiveDevs) - virPCIDeviceListDel(inactiveDevs, dev); - return 0; }

On Fri, Oct 30, 2015 at 9:13 PM, Andrea Bolognani <abologna(a)redhat.com&gt; wrote: Correct. Will Pass the inactiveDevs. The devices are removed from inactiveDevs before coming here. We need to restore if going on error path.

I know you're already working on a v2 of this, just a couple of quick remarks below. On Fri, 2015-10-30 at 05:01 +0530, Shivaprasad G Bhat wrote: Return type on a separate line, please. As mentioned in the comment for Patch 1, I think this is fairly obscure: if I had not read throught the whole series, I'd assume this checks whether the device is configured to use vfio-pci as stub driver, not whether it is currently bound to it, and it would not be immediately clear to me how this check fits in a function called virPCIDeviceBoundToVFIODriver(). I think it would be much cleaner to query the driver explicitly using virPCIDeviceGetDriverPathAndName() and remove that call from virPCIDeviceNew(). Either put the empty line before the comment or just get rid of it. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 05:04 +0530, Shivaprasad G Bhat wrote: This line is redundant since the information is already stored in git, plus you have the Signed-off-by below. This applies to Patch 7 as well. There's one trailing space here, and git complains about it when applying the patch. syntax-check complains too. Return type on a separate line, please. VFIO is an acronym, like PCI, and should always be all-caps. Do we want this to be in seconds, or would a higher granularity be better? This line is very long, please split the error message into two parts. I'd say "The" and "even" can be dropped. You should use dev->name instead of formatting the name yourself. Please remove the space between the label and the semicolon, and add one in front of the label. I'd also add an empty line before the label. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team