On Fri, 2015-10-30 at 04:56 +0530, Shivaprasad G Bhat wrote:

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@linux.vnet.ibm.com> --- 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)

Return type on a separate line, please.

+{ + 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)

I would split this patch in two: everything above is just moving an existing check into a separate function, without introducing any functional change. The code below, on the other hand, is changing the behavior of virPCIDeviceNew() and as such should go in its own patch. But see below.

@@ -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;

What are you doing this for? AFAICT you're using this so you can, in Patch 7, do pci = virPCIDeviceNew(...); if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci")) ... Is that so, or is there another reason I'm missing? If the former, I'd rather not overload the meaning of dev->stubDriver and call virPCIDeviceGetDriverPathAndName() explicitly in Patch 7, so that the intentions behind the code are abundantly clear. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 20:34 +0530, Shivaprasad bhat wrote:

...

...

@@ -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;

What are you doing this for? AFAICT you're using this so you can, in Patch 7, do

pci = virPCIDeviceNew(...); if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci")) ...

Is that so, or is there another reason I'm missing?

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.

I don't see how it's used in that function, as neither the function itself nor the calls to virHostdevIsPCINodeDeviceUsed() you've added with Patch 3 seem to touch dev->stubDriver... Please walk me through it, I'm probably just missing it because it's Friday :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

n Fri, 2015-10-30 at 04:56 +0530, Shivaprasad G Bhat wrote:

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@linux.vnet.ibm.com> --- 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;

This can't be unsigned int, as virPCIDeviceAddressGetIOMMUGroupNum() will return -2 if there is no IOMMU group for the device.

/* 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:

Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 04:57 +0530, Shivaprasad G Bhat wrote:

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@linux.vnet.ibm.com> --- 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 */

Please indent this comment properly. The second line should start with * as well.

+ if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, + virHostdevIsPCINodeDeviceUsed, + &data) < 0) + goto out; + } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) { goto out; + }

virPCIDeviceReattachInit(pci);

Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 04:58 +0530, Shivaprasad G Bhat wrote:

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@linux.vnet.ibm.com> --- 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,

This looks good. Weak ACK, we need somebody who's more familiar with this area of libvirt to go over the whole series anyway. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 04:59 +0530, 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 | 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; - }

This actually introduces a flow change in the function: before this patch, when dev->reprobe is false we set result = 0 and jumped to cleanup, while now virPCIDeviceReprobeHostDriver() returns 0 and the rest of virPCIDeviceUnbindFromStub() is executed. I would leave the check for the value of dev->reprobe and setting it to false in cleanup out of virPCIDeviceReprobeHostDriver().

- - /* 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);

Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2015-10-30 at 05:00 +0530, Shivaprasad G Bhat wrote:

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@linux.vnet.ibm.com> --- 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. + */

Just close the comment in the previous line.

+ if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {

Really weird alignment here...

+ 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);

Why pass NULL instead of inactiveDevs here?

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

Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team