This internal function supports, in theory, binding to a different stub driver than the one the PCI device has been configured to use. In practice, it is only ever called like virPCIDeviceBindToStub(dev, dev->stubDriver); which makes its second parameter redundant. Get rid of it, along with the extra string copy required to support it. --- src/util/virpci.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index bececb5..f57070d 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1176,20 +1176,18 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) static int -virPCIDeviceBindToStub(virPCIDevicePtr dev, - const char *stubDriverName) +virPCIDeviceBindToStub(virPCIDevicePtr dev) { int result = -1; bool reprobe = false; char *stubDriverPath = NULL; char *driverLink = NULL; char *path = NULL; /* reused for different purposes */ - char *newDriverName = NULL; + char *stubDriverName = dev->stubDriver; virErrorPtr err = NULL; if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || - !(driverLink = virPCIFile(dev->name, "driver")) || - VIR_STRDUP(newDriverName, stubDriverName) < 0) + !(driverLink = virPCIFile(dev->name, "driver"))) goto cleanup; if (virFileExists(driverLink)) { @@ -1304,13 +1302,8 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, VIR_FREE(driverLink); VIR_FREE(path); - if (result < 0) { - VIR_FREE(newDriverName); + if (result < 0) virPCIDeviceUnbindFromStub(dev); - } else { - VIR_FREE(dev->stubDriver); - dev->stubDriver = newDriverName; - } if (err) virSetError(err); @@ -1353,7 +1346,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev, return -1; } - if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0) + if (virPCIDeviceBindToStub(dev) < 0) return -1; /* Add *a copy of* the dev into list inactiveDevs, if -- 2.5.0

This replaces the virPCIKnownStubs string array that was used internally for stub driver validation. Advantages: * possible values are well-defined * typos in driver names will be detected at compile time * avoids having several copies of the same string around * no error checking required when setting / getting value The names used mirror those in the virDomainHostdevSubsysPCIBackendType enumeration. --- src/libvirt_private.syms | 2 ++ src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 ++-- src/util/virhostdev.c | 43 +++++++++------------------- src/util/virpci.c | 74 ++++++++++++++++++++++++++++-------------------- src/util/virpci.h | 16 ++++++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++-- tests/virpcitest.c | 33 ++++++++++++++------- 9 files changed, 100 insertions(+), 85 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd085c3..c1fd9f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1999,6 +1999,8 @@ virPCIGetVirtualFunctionIndex; virPCIGetVirtualFunctionInfo; virPCIGetVirtualFunctions; virPCIIsVirtualFunction; +virPCIStubDriverTypeFromString; +virPCIStubDriverTypeToString; # util/virpidfile.h diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 35d7fae..c28b884 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4972,8 +4972,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, goto cleanup; if (!driverName || STREQ(driverName, "xen")) { - if (virPCIDeviceSetStubDriver(pci, "pciback") < 0) - goto cleanup; + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); } else { virReportError(VIR_ERR_INVALID_ARG, _("unsupported driver name '%s'"), driverName); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ae1d8e7..91fb1c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12860,8 +12860,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, "supported on this system")); goto cleanup; } - if (virPCIDeviceSetStubDriver(pci, "vfio-pci") < 0) - goto cleanup; + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); } else if (STREQ(driverName, "kvm")) { if (!legacy) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", @@ -12869,8 +12868,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, "supported on this system")); goto cleanup; } - if (virPCIDeviceSetStubDriver(pci, "pci-stub") < 0) - goto cleanup; + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM); } else { virReportError(VIR_ERR_INVALID_ARG, _("unknown driver name '%s'"), driverName); diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index de029a0..33a45cb 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -237,22 +237,12 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) } virPCIDeviceSetManaged(dev, hostdev->managed); - if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0) { - virObjectUnref(list); - return NULL; - } - } else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) { - if (virPCIDeviceSetStubDriver(dev, "pciback") < 0) { - virObjectUnref(list); - return NULL; - } - } else { - if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) { - virObjectUnref(list); - return NULL; - } - } + if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); + else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN); + else + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); } return list; @@ -574,7 +564,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); - bool usesVfio = STREQ(virPCIDeviceGetStubDriver(dev), "vfio-pci"); + bool usesVfio = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name, usesVfio}; @@ -749,7 +739,7 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) } /* Wait for device cleanup if it is qemu/kvm */ - if (STREQ(virPCIDeviceGetStubDriver(dev), "pci-stub")) { + if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { int retries = 100; while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") && retries) { @@ -917,17 +907,12 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, goto cleanup; virPCIDeviceSetManaged(dev, hostdev->managed); - if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0) - goto cleanup; - } else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) { - if (virPCIDeviceSetStubDriver(dev, "pciback") < 0) - goto cleanup; - } else { - if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) - goto cleanup; - - } + if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); + else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN); + else + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); virPCIDeviceSetUsedBy(dev, drv_name, dom_name); /* Setup the original states for the PCI device */ diff --git a/src/util/virpci.c b/src/util/virpci.c index 8094735..e82583a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -55,6 +55,12 @@ VIR_LOG_INIT("util.pci"); VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, "", "2.5", "5", "8") +VIR_ENUM_IMPL(virPCIStubDriver, VIR_PCI_STUB_DRIVER_LAST, + "pciback", /* XEN */ + "pci-stub", /* KVM */ + "vfio-pci", /* VFIO */ +); + struct _virPCIDevice { unsigned int domain; unsigned int bus; @@ -74,7 +80,8 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; - char *stubDriver; + + virPCIStubDriver stubDriver; /* used by reattach function */ bool unbind_from_stub; @@ -940,7 +947,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, if (virPCIDeviceGetDriverPathAndName(dev, &drvPath, &drvName) < 0) goto cleanup; - if (STREQ_NULLABLE(drvName, "vfio-pci")) { + if (virPCIStubDriverTypeFromString(drvName) == VIR_PCI_STUB_DRIVER_VFIO) { VIR_DEBUG("Device %s is bound to vfio-pci - skip reset", dev->name); ret = 0; @@ -991,13 +998,21 @@ virPCIDeviceReset(virPCIDevicePtr dev, static int -virPCIProbeStubDriver(const char *driver) +virPCIProbeStubDriver(virPCIStubDriver driver) { + const char *drvname = NULL; char *drvpath = NULL; bool probed = false; + if (!(drvname = virPCIStubDriverTypeToString(driver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Attempting to use unknown stub driver")); + return -1; + } + recheck: - if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) { + if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) { /* driver already loaded, return */ VIR_FREE(drvpath); return 0; @@ -1008,8 +1023,8 @@ virPCIProbeStubDriver(const char *driver) if (!probed) { char *errbuf = NULL; probed = true; - if ((errbuf = virKModLoad(driver, true))) { - VIR_WARN("failed to load driver %s: %s", driver, errbuf); + if ((errbuf = virKModLoad(drvname, true))) { + VIR_WARN("failed to load driver %s: %s", drvname, errbuf); VIR_FREE(errbuf); goto cleanup; } @@ -1021,15 +1036,15 @@ virPCIProbeStubDriver(const char *driver) /* If we know failure was because of blacklist, let's report that; * otherwise, report a more generic failure message */ - if (virKModIsBlacklisted(driver)) { + if (virKModIsBlacklisted(drvname)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s: " "administratively prohibited"), - driver); + drvname); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s"), - driver); + drvname); } return -1; @@ -1072,13 +1087,6 @@ virPCIDeviceUnbind(virPCIDevicePtr dev) return ret; } -static const char *virPCIKnownStubs[] = { - "pciback", /* used by xen */ - "pci-stub", /* used by kvm legacy passthrough */ - "vfio-pci", /* used by VFIO device assignment */ - NULL -}; - static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { @@ -1086,7 +1094,8 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) char *drvdir = NULL; char *path = NULL; char *driver = NULL; - const char **stubTest; + virPCIStubDriver stubDriver; + const char *stubDriverName; bool isStub = false; /* If the device is currently bound to one of the "well known" @@ -1104,10 +1113,11 @@ 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)) { + for (stubDriver = 0; stubDriver < VIR_PCI_STUB_DRIVER_LAST; stubDriver++) { + stubDriverName = virPCIStubDriverTypeToString(stubDriver); + if (stubDriverName && STREQ(driver, stubDriverName)) { isStub = true; - VIR_DEBUG("Found stub driver %s", *stubTest); + VIR_DEBUG("Found stub driver %s", stubDriverName); break; } } @@ -1182,9 +1192,16 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) char *stubDriverPath = NULL; char *driverLink = NULL; char *path = NULL; /* reused for different purposes */ - char *stubDriverName = dev->stubDriver; + const char *stubDriverName = NULL; virErrorPtr err = NULL; + if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Attempting to use unknown stub driver")); + return -1; + } + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || !(driverLink = virPCIFile(dev->name, "driver"))) goto cleanup; @@ -1337,8 +1354,6 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { - sa_assert(dev->stubDriver); - if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1; @@ -1622,10 +1637,9 @@ virPCIDeviceCopy(virPCIDevicePtr dev) /* shallow copy to take care of most attributes */ *copy = *dev; - copy->path = copy->stubDriver = NULL; + copy->path = NULL; copy->used_by_drvname = copy->used_by_domname = NULL; if (VIR_STRDUP(copy->path, dev->path) < 0 || - VIR_STRDUP(copy->stubDriver, dev->stubDriver) < 0 || VIR_STRDUP(copy->used_by_drvname, dev->used_by_drvname) < 0 || VIR_STRDUP(copy->used_by_domname, dev->used_by_domname) < 0) { goto error; @@ -1645,7 +1659,6 @@ virPCIDeviceFree(virPCIDevicePtr dev) return; VIR_DEBUG("%s %s: freeing", dev->id, dev->name); VIR_FREE(dev->path); - VIR_FREE(dev->stubDriver); VIR_FREE(dev->used_by_drvname); VIR_FREE(dev->used_by_domname); VIR_FREE(dev); @@ -1694,14 +1707,13 @@ virPCIDeviceGetManaged(virPCIDevicePtr dev) return dev->managed; } -int -virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const char *driver) +void +virPCIDeviceSetStubDriver(virPCIDevicePtr dev, virPCIStubDriver driver) { - VIR_FREE(dev->stubDriver); - return VIR_STRDUP(dev->stubDriver, driver); + dev->stubDriver = driver; } -const char * +virPCIStubDriver virPCIDeviceGetStubDriver(virPCIDevicePtr dev) { return dev->stubDriver; diff --git a/src/util/virpci.h b/src/util/virpci.h index d9911d0..c7bdb58 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -43,6 +43,15 @@ struct _virPCIDeviceAddress { }; typedef enum { + VIR_PCI_STUB_DRIVER_XEN = 0, + VIR_PCI_STUB_DRIVER_KVM, + VIR_PCI_STUB_DRIVER_VFIO, + VIR_PCI_STUB_DRIVER_LAST +} virPCIStubDriver; + +VIR_ENUM_DECL(virPCIStubDriver); + +typedef enum { VIR_PCIE_LINK_SPEED_NA = 0, VIR_PCIE_LINK_SPEED_25, VIR_PCIE_LINK_SPEED_5, @@ -90,10 +99,9 @@ int virPCIDeviceReset(virPCIDevicePtr dev, void virPCIDeviceSetManaged(virPCIDevice *dev, bool managed); unsigned int virPCIDeviceGetManaged(virPCIDevice *dev); -int virPCIDeviceSetStubDriver(virPCIDevicePtr dev, - const char *driver) - ATTRIBUTE_NONNULL(2); -const char *virPCIDeviceGetStubDriver(virPCIDevicePtr dev); +void virPCIDeviceSetStubDriver(virPCIDevicePtr dev, + virPCIStubDriver driver); +virPCIStubDriver virPCIDeviceGetStubDriver(virPCIDevicePtr dev); virPCIDeviceAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev); int virPCIDeviceSetUsedBy(virPCIDevice *dev, const char *drv_name, diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index ce31f0f..7b2a3aa 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2531,8 +2531,7 @@ xenUnifiedNodeDeviceDetachFlags(virNodeDevicePtr dev, return -1; if (!driverName) { - if (virPCIDeviceSetStubDriver(pci, "pciback") < 0) - goto out; + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); } else { virReportError(VIR_ERR_INVALID_ARG, _("unknown driver name '%s'"), driverName); diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index faebdd4..f20992b 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -97,9 +97,10 @@ myInit(void) } for (i = 0; i < nhostdevs; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)) || - virPCIDeviceSetStubDriver(dev[i], "pci-stub") < 0) + if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup; + + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); } if (VIR_ALLOC(mgr) < 0) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 35b4781..b5052e5 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -107,10 +107,11 @@ testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0); for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)) || - virPCIDeviceSetStubDriver(dev[i], "pci-stub") < 0) + if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup; + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + if (virPCIDeviceDetach(dev[i], activeDevs, inactiveDevs) < 0) goto cleanup; @@ -147,10 +148,11 @@ testVirPCIDeviceReset(const void *opaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0); for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)) || - virPCIDeviceSetStubDriver(dev[i], "pci-stub") < 0) + if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup; + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + if (virPCIDeviceReset(dev[i], activeDevs, inactiveDevs) < 0) goto cleanup; } @@ -189,8 +191,7 @@ testVirPCIDeviceReattach(const void *opaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, i + 1); - if (virPCIDeviceSetStubDriver(dev[i], "pci-stub") < 0) - goto cleanup; + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); } CHECK_LIST_COUNT(activeDevs, 0); @@ -248,8 +249,9 @@ testVirPCIDeviceDetachSingle(const void *opaque) if (!dev) goto cleanup; - if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0 || - virPCIDeviceDetach(dev, NULL, NULL) < 0) + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); + + if (virPCIDeviceDetach(dev, NULL, NULL) < 0) goto cleanup; ret = 0; @@ -262,15 +264,16 @@ static int testVirPCIDeviceDetachFail(const void *opaque) { const struct testPCIDevData *data = opaque; + const char *stubDriverName = NULL; int ret = -1; virPCIDevicePtr dev; + virPCIStubDriver stubDriver; dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); if (!dev) goto cleanup; - if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0) - goto cleanup; + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); if (virPCIDeviceDetach(dev, NULL, NULL) < 0) { if (virTestGetVerbose() || virTestGetDebug()) @@ -278,10 +281,18 @@ testVirPCIDeviceDetachFail(const void *opaque) virResetLastError(); ret = 0; } else { + stubDriver = virPCIDeviceGetStubDriver(dev); + if (!(stubDriverName = virPCIStubDriverTypeToString(stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + "Attempting to use unknown stub driver"); + goto cleanup; + } + virReportError(VIR_ERR_INTERNAL_ERROR, "Attaching device %s to %s should have failed", virPCIDeviceGetName(dev), - virPCIDeviceGetStubDriver(dev)); + stubDriverName); } cleanup: -- 2.5.0

This function, previously called virHostdevIsPCINodeDeviceUsed(), was written to work both when used on its own and when used with virPCIDeviceAddressIOMMUGroupIterate(); as a consequence, it was slightly more convoluted than it needed to be. Rework it to perform a single task (checking that a PCI device is not used by a domain) and remove the redundant "Node" from its name. Add some documentation as well. The safety checks that are no longer performed by this function will be reintroduced later on in a different form. --- src/util/virhostdev.c | 110 ++++++++++++++++---------------------------------- 1 file changed, 35 insertions(+), 75 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 33a45cb..77757c5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -54,51 +54,43 @@ static virClassPtr virHostdevManagerClass; static void virHostdevManagerDispose(void *obj); static virHostdevManagerPtr virHostdevManagerNew(void); -struct virHostdevIsPCINodeDeviceUsedData { - virHostdevManagerPtr hostdev_mgr; - const char *domainName; - const bool usesVfio; -}; - -static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) +/** + * virHostdevIsPCIDeviceUsed: + * @dev: PCI device + * @hostdev_mgr: hostdev manager + * + * Checks whether @dev is already assigned to a domain, reporting a detailed + * error if that's the case. + * + * Returns: true if @dev is in use by some domain, false otherwise + */ +static bool +virHostdevIsPCIDeviceUsed(virPCIDevicePtr dev, + virHostdevManagerPtr hostdev_mgr) { virPCIDevicePtr other; - int ret = -1; - virPCIDevicePtr pci = NULL; - struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque; + const char *other_drvname = NULL; + const char *other_domname = NULL; + bool ret = false; - if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus, - devAddr->slot, devAddr->function))) - goto cleanup; + if (!(other = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev))) + goto out; - other = virPCIDeviceListFind(helperData->hostdev_mgr->activePCIHostdevs, - pci); - if (other) { - const char *other_drvname = NULL; - const char *other_domname = NULL; - virPCIDeviceGetUsedBy(other, &other_drvname, &other_domname); + virPCIDeviceGetUsedBy(other, &other_drvname, &other_domname); - if (helperData->usesVfio && - (other_domname && helperData->domainName) && - (STREQ(other_domname, helperData->domainName))) - goto iommu_owner; + if (other_drvname && other_domname) + virReportError(VIR_ERR_OPERATION_INVALID, + _("PCI device %s is in use by " + "driver %s, domain %s"), + virPCIDeviceGetName(dev), + other_drvname, other_domname); + else + virReportError(VIR_ERR_OPERATION_INVALID, + _("PCI device %s is in use"), + virPCIDeviceGetName(dev)); + ret = true; - if (other_drvname && other_domname) - virReportError(VIR_ERR_OPERATION_INVALID, - _("PCI device %s is in use by " - "driver %s, domain %s"), - virPCIDeviceGetName(pci), - other_drvname, other_domname); - else - virReportError(VIR_ERR_OPERATION_INVALID, - _("PCI device %s is in use"), - virPCIDeviceGetName(pci)); - goto cleanup; - } - iommu_owner: - ret = 0; - cleanup: - virPCIDeviceFree(pci); + out: return ret; } @@ -538,7 +530,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, int last_processed_hostdev_vf = -1; size_t i; int ret = -1; - virPCIDeviceAddressPtr devAddr = NULL; if (!nhostdevs) return 0; @@ -565,8 +556,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); bool usesVfio = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); - struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name, - usesVfio}; if (!usesVfio && !virPCIDeviceIsAssignable(dev, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -575,23 +564,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - VIR_FREE(devAddr); - if (!(devAddr = virPCIDeviceGetAddress(dev))) - goto cleanup; - - /* The device is in use by other active domain if - * the dev is in list activePCIHostdevs. VFIO devices - * belonging to same iommu group can't be shared - * across guests. - */ - if (usesVfio) { - if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, - virHostdevIsPCINodeDeviceUsed, - &data) < 0) - goto cleanup; - } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) { + /* Host devices can't be shared between domains */ + if (virHostdevIsPCIDeviceUsed(dev, hostdev_mgr)) goto cleanup; - } } /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ @@ -718,7 +693,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnref(pcidevs); - VIR_FREE(devAddr); return ret; } @@ -1543,18 +1517,12 @@ int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { - virPCIDeviceAddressPtr devAddr = NULL; - struct virHostdevIsPCINodeDeviceUsedData data = { hostdev_mgr, NULL, - false }; int ret = -1; virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (!(devAddr = virPCIDeviceGetAddress(pci))) - goto out; - - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) + if (virHostdevIsPCIDeviceUsed(pci, hostdev_mgr)) goto out; if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, @@ -1566,7 +1534,6 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } @@ -1574,18 +1541,12 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { - virPCIDeviceAddressPtr devAddr = NULL; - struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, - false}; int ret = -1; virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (!(devAddr = virPCIDeviceGetAddress(pci))) - goto out; - - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) + if (virHostdevIsPCIDeviceUsed(pci, hostdev_mgr)) goto out; virPCIDeviceReattachInit(pci); @@ -1598,7 +1559,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } -- 2.5.0

When reattaching VFIO devices to the host, we have to be careful not to end up in a situation where ownership of the IOMMU group is shared between the hosts and a domain, as doing so will result in host crashes. If a PCI device is not safe for reattach, as detected by virHostdevIsPCIDeviceSafeForReattach(), we mark as inactive and delay actually reattaching it to the host to a later time when doing so will not result in shared IOMMU group ownership. Note that, with this commit, VFIO devices will no longer be reattached to the host, even when it would be safe to do so. A later commit will restore the missing functionality. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1272300 --- src/util/virhostdev.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 9174525..0f2916b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -155,6 +155,38 @@ virHostdevIsPCIDeviceSafeForDetach(virPCIDeviceAddressPtr addr, return -1; } +/** + * virHostdevIsPCIDeviceSafeForReattach: + * @addr: PCI device address + * @opaque: user data (virHostdevIsPCIDeviceSafeData*) + * + * Checks whether a PCI device can be safely reattached to the host. + * + * It is meant to be called from virPCIDeviceAddressIOMMUGroupIterate() or + * virPCIDeviceIOMMUGroupIterate(), and the checks it performs only make + * sense when using VFIO device assignment. + * + * Returns: 0 if the device can be safely reattached to the host, <0 otherwise + */ +static int +virHostdevIsPCIDeviceSafeForReattach(virPCIDeviceAddressPtr addr, + void *opaque) +{ + virHostdevIsPCIDeviceSafeData *data = opaque; + + /* Active devices are not safe: IOMMU groups can't be shared between + * the host and its domains */ + if (data->activeDevices && + virPCIDeviceListFindByIDs(data->activeDevices, + addr->domain, addr->bus, + addr->slot, addr->function)) + return -1; + + /* All other situations are either safe (pending device, inactive device) + * or can't happen (device in use by the host) */ + return 0; +} + static int virHostdevManagerOnceInit(void) { if (!(virHostdevManagerClass = virClassNew(virClassForObject(), @@ -900,6 +932,41 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } + /* Loop 3: delay handling of devices that are not yet safe for reattach */ + i = 0; + while (i < virPCIDeviceListCount(pcidevs)) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virHostdevIsPCIDeviceSafeData data = { dom_name, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs, + pcidevs }; + bool usesVFIO = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); + + if (usesVFIO && + virPCIDeviceIOMMUGroupIterate(dev, + virHostdevIsPCIDeviceSafeForReattach, + &data) < 0) { + /* Don't process the device further. Later on, when the last + * device in this IOMMU group will be unassigned from its domain, + * we will process all the devices at once, including this one */ + VIR_DEBUG("Delaying reattaching of PCI device %s", + virPCIDeviceGetName(dev)); + dev = virPCIDeviceListSteal(pcidevs, dev); + } else { + /* This device is safe, keep it in the list and skip ahead */ + VIR_DEBUG("PCI device %s can be safely reattached", + virPCIDeviceGetName(dev)); + i++; + } + + /* Mark the device as inactive */ + virPCIDeviceListAddCopy(hostdev_mgr->inactivePCIHostdevs, dev); + } + + /* At this point, all devices we were asked to reattach have been removed + * from activePCIHostdevs and have been added to inactivePCIHostdevs; + * pcidevs contains the devices that we can safely process right now */ + /* Loop 3: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); -- 2.5.0

We mark all VFIO devices as inactive after we've detached them from the domain, so that step doesn't need to be performed afterwards. --- src/util/virhostdev.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1042ab3..b350de0 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -894,12 +894,10 @@ virHostdevExpandPCIDeviceListIOMMUGroups(virPCIDeviceListPtr list, static void virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) { - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ + /* We don't need to do anything for unmanaged devices */ if (!virPCIDeviceGetManaged(dev)) { - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); + VIR_DEBUG("Skipping reattach of unmanaged PCI device %s", + virPCIDeviceGetName(dev)); return; } -- 2.5.0

On 12/02/2015 12:39 PM, Andrea Bolognani wrote: Instead of creating this new function, how about if we change virPCIDevice to contain a virPCIDeviceAddress rather than individual domain, bus, slot, and function?