[libvirt] [PATCH v4 09/10] Postpone reprobing till all the devices in iommu group are unbound from vfio

The host will crash if a device is bound to host driver when the device belonging to same iommu group is in use by any of the guests. So, do the host driver probe only after all the devices in the iommu group have unbound from the vfio. The patch also adds the test case. The patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=1272300 Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/util/virpci.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++----- tests/virpcitest.c | 115 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 14 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index a8a22d1..5462cd2 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1129,6 +1129,31 @@ virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, } static int +virPCIDeviceBoundToVFIODriver(virPCIDeviceAddressPtr devAddr, void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + virPCIDevicePtr pci = NULL; + char *drvpath = NULL; + char *driver = NULL; + + if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function))) + goto cleanup; + + if (virPCIDeviceGetDriverPathAndName(pci, &drvpath, &driver) < 0) + goto cleanup; + + if (STREQ_NULLABLE(driver, "vfio-pci")) + ret = -1; + + cleanup: + VIR_FREE(drvpath); + VIR_FREE(driver); + virPCIDeviceFree(pci); + return ret; +} + +static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED, virPCIDeviceListPtr inactiveDevs) @@ -1145,21 +1170,91 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, goto cleanup; if (!driver) { - /* The device is not bound to any driver and we are almost done. */ + /* This device was probably unbound before and libvirt restared. + * Add to the inactive list if not already there so that we + * reprobe it if needed. + */ + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) goto reprobe; } - if (!dev->unbind_from_stub) + if (!dev->unbind_from_stub) { + /* Add to the inactive list if not already there so that we + * reprobe it if needed. + */ + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) goto remove_slot; + } /* If the device isn't bound to a known stub, skip the unbind. */ if (!virPCIIsKnownStub(driver)) goto remove_slot; - if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) - goto cleanup; - dev->unbind_from_stub = false; + if (STREQ_NULLABLE(driver, "vfio-pci")) { + size_t i = 0; + bool inInactiveList = false; + virPCIDevicePtr temp; + while (activeDevs && (i < virPCIDeviceListCount(activeDevs))) { + virPCIDevicePtr pcidev = virPCIDeviceListGet(activeDevs, i); + if (dev->iommuGroup == pcidev->iommuGroup) { + /* If Part of activelist, deal with current device later */ + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) + goto cleanup; + result = 0; + goto revisit; + } + i++; + } + /* No guests are using any of the devices in the iommu. + * This is the first device after guest shutdown/unplug + * of all devices. So, rest of the devices are in inactiveDevs. + * OR This is the first device after libvirt start and nothing is in + * inactiveDevs. + * If the device is in inactiveList(User can issue reattach multiple times + * for the same device), mark it as already unbound. + */ + if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) + goto cleanup; + dev->unbind_from_stub = false; + if (inactiveDevs && (temp = virPCIDeviceListFind(inactiveDevs, dev))) { + temp->unbind_from_stub = false; + inInactiveList = true; + } + /* Unbind rest of the devices in the same iommu group. + * After the fresh libvirt start, nothing is detached in the step + * as inactiveDevs is empty.*/ + i = 0; + while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) { + virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i); + if (dev->iommuGroup == pcidev->iommuGroup) { + if (pcidev->unbind_from_stub && + virPCIDeviceUnbind(pcidev, pcidev->reprobe) < 0) { + goto cleanup; + } + pcidev->unbind_from_stub = false; + } + i++; + } + /* Libvirt just restarted and inactive list is empty or yet to get into + * the list. But no devices used actively from same iommu group. + * Basically this could be the first one to unbind from vfio but + * could be the last function to be bound to vfio if this is after + * libvirt restart. + */ + if (!inInactiveList) { + if (inactiveDevs && virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) + goto cleanup; + } + } else { + if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) + goto cleanup; + dev->unbind_from_stub = false; + } + /* The remove_slot means nothing for VFIO. Dont manage it in active/inactiveList */ remove_slot: if (!dev->remove_slot) goto reprobe; @@ -1177,25 +1272,58 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, dev->remove_slot = false; reprobe: - if (!dev->reprobe) { - result = 0; - goto cleanup; - } - if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) - goto cleanup; + /* For VFIO devices if there is a pending reprobe, the new reattach + * request would come with device->stubDriver set to null as the + * device is actually unbound. Dont bind to host driver if the + * iommu group is in use. */ + if (!driver || STREQ_NULLABLE(driver, "vfio-pci")) { + size_t i = 0; + virPCIDeviceAddress devAddr = { dev->domain, dev->bus, + dev->slot, dev->function }; + if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, virPCIDeviceBoundToVFIODriver, NULL) < 0) { + result = 0; + goto cleanup; + } + /* This device is the last to unbind from vfio. As we explicitly + * add a missing device in the list to inactiveList, we will just + * go through the list. */ + while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) { + virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i); + if (dev->iommuGroup == pcidev->iommuGroup) { + if (pcidev->reprobe && + virPCIDeviceReprobeHostDriver(pcidev, driver, drvdir) < 0) + goto cleanup; + /* Steal the dev from list inactiveDevs */ + virPCIDeviceListDel(inactiveDevs, pcidev); + continue; + } + i++; + } + /* If the list was null, we failed to add to the list before. + * Reprobe the current device explicitly. */ + if (!inactiveDevs) { + if (dev->reprobe && + virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) + goto cleanup; + } + } else { + if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) + goto cleanup; + + if (inactiveDevs) + virPCIDeviceListDel(inactiveDevs, dev); + } result = 0; cleanup: - if ((result == 0) && inactiveDevs) - virPCIDeviceListDel(inactiveDevs, dev); - /* do not do it again */ dev->unbind_from_stub = false; dev->remove_slot = false; dev->reprobe = false; + revisit: VIR_FREE(drvdir); VIR_FREE(path); VIR_FREE(driver); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 25591f9..070775c 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -211,6 +211,118 @@ testVirPCIDeviceReattach(const void *opaque ATTRIBUTE_UNUSED) return ret; } +static int +testVirPCIDeviceVFIOReattach(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + virPCIDevicePtr dev[] = {NULL, NULL, NULL, NULL, NULL}; + size_t i, nDev = ARRAY_CARDINALITY(dev); + virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; + int count; + + if (!(activeDevs = virPCIDeviceListNew()) || + !(inactiveDevs = virPCIDeviceListNew())) + goto cleanup; + + /* Mark the device 0001:1:00.0 as active in a guest */ + for (i = 0; i < 2; i++) { + if (!(dev[i] = virPCIDeviceNew(1, 0x01, 0, i))) + goto cleanup; + if (virPCIDeviceSetStubDriver(dev[i], "vfio-pci") < 0) + goto cleanup; + if (virPCIDeviceDetach(dev[i], activeDevs, inactiveDevs) < 0) + goto cleanup; + if (virPCIDeviceListAddCopy(activeDevs, dev[i]) < 0) + goto cleanup; + virPCIDeviceListDel(inactiveDevs, dev[i]); + } + CHECK_LIST_COUNT(activeDevs, 2); + CHECK_LIST_COUNT(inactiveDevs, 0); + + for (i = 0; i < 3; i++) { + if (!(dev[i+2] = virPCIDeviceNew(5, 0x90, 1, i))) + goto cleanup; + /* + if (virPCIDeviceListAdd(inactiveDevs, dev[i]) < 0) { + virPCIDeviceFree(dev[i]); + goto cleanup; + } + */ + if (virPCIDeviceSetStubDriver(dev[i+2], "vfio-pci") < 0) + goto cleanup; + + + if (virPCIDeviceDetach(dev[i+2], activeDevs, inactiveDevs) < 0) + goto cleanup; + } + + CHECK_LIST_COUNT(activeDevs, 2); + CHECK_LIST_COUNT(inactiveDevs, 3); + /* Mark the device 0005:90:01.0 as active in a guest */ + if (virPCIDeviceListAddCopy(activeDevs, dev[2]) < 0) + goto cleanup; + + virPCIDeviceListDel(inactiveDevs, dev[2]); + CHECK_LIST_COUNT(activeDevs, 3); + CHECK_LIST_COUNT(inactiveDevs, 2); + + /* Check that the reattach silently queues when any domain is actively + * using any of the devices in the iommu. */ + + if (virPCIDeviceReattach(dev[3], activeDevs, inactiveDevs) < 0) + goto cleanup; + + if (virPCIDeviceReattach(dev[4], activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 3); + CHECK_LIST_COUNT(inactiveDevs, 2); + + virPCIDeviceListDel(activeDevs, dev[2]); + /* The devices should get queued in inactiveDevs */ + CHECK_LIST_COUNT(activeDevs, 2); + CHECK_LIST_COUNT(inactiveDevs, 2); + + if (virPCIDeviceReattach(dev[2], activeDevs, inactiveDevs) < 0) + goto cleanup; + + /* The queued devices should all be Reattached */ + CHECK_LIST_COUNT(activeDevs, 2); + CHECK_LIST_COUNT(inactiveDevs, 0); + + virPCIDeviceListDel(activeDevs, dev[0]); + virPCIDeviceListDel(activeDevs, dev[1]); + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 0); + /* Already reattached. No need to queue */ + + if (virPCIDeviceReattach(dev[0], activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(inactiveDevs, 1); + + if (virPCIDeviceReattach(dev[1], activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(inactiveDevs, 0); + + for (i = 2; i < nDev; i++) { + if (virPCIDeviceReattach(dev[i], activeDevs, inactiveDevs) < 0) + goto cleanup; + } + + /* Already reattached. No need to queue */ + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 0); + + ret = 0; + cleanup: + virObjectUnref(activeDevs); + virObjectUnref(inactiveDevs); + return ret; +} + struct testPCIDevData { unsigned int domain; unsigned int bus; @@ -444,6 +556,9 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 3, 0); DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL); + /* Reattach an unknown unbound device */ + DO_TEST(testVirPCIDeviceVFIOReattach); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakesysfsdir);