[libvirt] [PATCH 0/6] Small PCI hostdev improvements and fixes
While working on v2 of my series[1] intended to fix https://bugzilla.redhat.com/show_bug.cgi?id=1272300 I'm running into some smaller stuff that isn't necessarily strictly related to the issue at hand. The idea is to have this smaller patches merged before the bigger, more invasive changes that series will bring, and hopefully make it easier to review by laying down the groundwork in advance. The series is organized as follows: 1-2: remove function parameters that are either not inspected by the function or never have an interesting value 3: change a string field that can only assume one of three values to a proper enumeration 4: improve bookkeeping when detaching PCI devices from the host, by making sure they always transition through the 'inactive' state (when they're attached to the stub driver) 5: make sure we never try to reattach an unmanaged device to the host, not even when recovering from a previous error 6: add some useful debug messages I expect patches 1, 2 and 6 to be fairly uncontroversial and easy to review; patch 3 is kinda big, but it's IMHO a nice improvement over the current situation; patches 4 and 5 are small but definitely need a long hard stare from a different pair of eyes to ensure they're not breaking anything. Cheers. [1] https://www.redhat.com/archives/libvir-list/2015-December/msg00070.html Andrea Bolognani (6): pci: Remove redundant parameter from virPCIDeviceBindToStub() pci: Remove 'reprobe' parameter from virPCIDeviceUnbind() pci: Introduce virPCIStubDriver enumeration hostdev: Mark PCI devices as inactive as they're detached hostdev: Only rollback detach of managed devices on error hostdev: Emit debug messages while handling PCI hostdevs src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 +-- src/util/virhostdev.c | 107 ++++++++++++++++++++++++++++++----------------- src/util/virpci.c | 102 ++++++++++++++++++++++---------------------- src/util/virpci.h | 18 +++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++- tests/virpcitest.c | 35 ++++++++++------ 9 files changed, 163 insertions(+), 118 deletions(-) -- 2.5.0
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 6f0cb8c..424983d 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1177,20 +1177,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)) { @@ -1305,13 +1303,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); @@ -1354,7 +1347,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
The value is not inspected inside the function, so it makes more sense for the caller to change the device's setting explicitly. --- src/util/virpci.c | 10 ++++++---- src/util/virpci.h | 2 +- tests/virpcitest.c | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 424983d..21eacf5 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1037,7 +1037,7 @@ virPCIProbeStubDriver(const char *driver) } int -virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe) +virPCIDeviceUnbind(virPCIDevicePtr dev) { char *path = NULL; char *drvpath = NULL; @@ -1063,7 +1063,6 @@ virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe) dev->name, driver); goto cleanup; } - dev->reprobe = reprobe; } ret = 0; @@ -1116,7 +1115,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (!isStub) goto remove_slot; - if (virPCIDeviceUnbind(dev, dev->reprobe) < 0) + if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false; @@ -1230,9 +1229,12 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) goto remove_id; } - if (virPCIDeviceUnbind(dev, reprobe) < 0) + if (virPCIDeviceUnbind(dev) < 0) goto remove_id; + /* If the device was bound to a driver we'll need to reprobe later */ + dev->reprobe = reprobe; + /* If the device isn't already bound to pci-stub, try binding it now. */ if (!virFileLinkPointsTo(driverLink, stubDriverPath)) { diff --git a/src/util/virpci.h b/src/util/virpci.h index f3d5676..e628ab8 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -201,7 +201,7 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, char **pfname, int *vf_index); -int virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe); +int virPCIDeviceUnbind(virPCIDevicePtr dev); int virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index f1c5369..1ade3e5 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -341,7 +341,7 @@ testVirPCIDeviceUnbind(const void *opaque) if (!dev) goto cleanup; - if (virPCIDeviceUnbind(dev, false) < 0) + if (virPCIDeviceUnbind(dev) < 0) goto cleanup; ret = 0; -- 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 | 45 ++++++++++------------------ src/util/virpci.c | 77 +++++++++++++++++++++++++----------------------- src/util/virpci.h | 16 +++++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++-- tests/virpcitest.c | 33 ++++++++++++++------- 9 files changed, 99 insertions(+), 91 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b5ddc1..79074cc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2005,6 +2005,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 4f82c01..9491f0f 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5073,8 +5073,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 783a7cd..26a63f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12882,8 +12882,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", @@ -12891,8 +12890,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 4065535..f9072a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -237,22 +237,13 @@ 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 +565,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}; @@ -745,7 +736,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) { @@ -913,19 +904,15 @@ 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; - - } virPCIDeviceSetUsedBy(dev, drv_name, dom_name); + 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); + /* Setup the original states for the PCI device */ virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub); virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot); diff --git a/src/util/virpci.c b/src/util/virpci.c index 21eacf5..71eee0d 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 { virPCIDeviceAddress address; @@ -71,7 +77,8 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; - char *stubDriver; + + virPCIStubDriver stubDriver; /* used by reattach function */ bool unbind_from_stub; @@ -941,7 +948,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; @@ -992,13 +999,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; @@ -1009,8 +1024,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; } @@ -1022,15 +1037,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; @@ -1073,13 +1088,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) { @@ -1087,8 +1095,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,16 +1111,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)) { - isStub = true; - VIR_DEBUG("Found stub driver %s", *stubTest); - break; - } - } - if (!isStub) + if (virPCIStubDriverTypeFromString(driver) < 0) goto remove_slot; + VIR_DEBUG("Found stub driver %s", driver); + if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false; @@ -1183,9 +1184,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; @@ -1338,8 +1346,6 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { - sa_assert(dev->stubDriver); - if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1; @@ -1622,10 +1628,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 +1650,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); @@ -1683,14 +1687,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 e628ab8..506a68c 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 285a553..8d9ac07 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2532,8 +2532,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 2a74976..1b30681 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 1ade3e5..ebabf7f 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
On 17.12.2015 16:23, Andrea Bolognani wrote:
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.
diff --git a/src/util/virpci.h b/src/util/virpci.h index e628ab8..506a68c 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;
Is there any specific reason to explicitly set _XEN = 0? I mean, by default any new PCI device that we create in memory (virPCIDeviceNew()) will have _XEN as stub driver until set by virPCIDeviceSetStubDriver(). I'm not saying it's wrong, just asking. For instance, if we reserve zero for a special case when stub driver hasn't been set, we can throw an error and detect a bug in our code.
+ +VIR_ENUM_DECL(virPCIStubDriver); + +typedef enum { VIR_PCIE_LINK_SPEED_NA = 0, VIR_PCIE_LINK_SPEED_25, VIR_PCIE_LINK_SPEED_5,
Michal
On Thu, 2015-12-17 at 17:28 +0100, Michal Privoznik wrote:
typedef enum { + VIR_PCI_STUB_DRIVER_XEN = 0, + VIR_PCI_STUB_DRIVER_KVM, + VIR_PCI_STUB_DRIVER_VFIO, + VIR_PCI_STUB_DRIVER_LAST +} virPCIStubDriver; Is there any specific reason to explicitly set _XEN = 0? I mean, by default any new PCI device that we create in memory (virPCIDeviceNew()) will have _XEN as stub driver until set by virPCIDeviceSetStubDriver(). I'm not saying it's wrong, just asking. For instance, if we reserve zero for a special case when stub driver hasn't been set, we can throw an error and detect a bug in our code.
Not really. I've changed it to follow your advice. https://www.redhat.com/archives/libvir-list/2015-December/msg00740.html Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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. --- Changes in v2: * add VIR_PCI_STUB_DRIVER_NONE so we can detect when no driver has been configured for a specific device * simplify code in testVirPCIDeviceDetachFail() by not reading the driver back from the device, since we set it a few lines above testVirPCIDeviceDetachFail src/libvirt_private.syms | 2 ++ src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 ++-- src/util/virhostdev.c | 45 +++++++++---------------- src/util/virpci.c | 86 +++++++++++++++++++++++++++--------------------- src/util/virpci.h | 17 +++++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 +-- tests/virpcitest.c | 23 ++++++------- 9 files changed, 99 insertions(+), 91 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b5ddc1..79074cc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2005,6 +2005,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 4f82c01..9491f0f 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5073,8 +5073,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 783a7cd..26a63f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12882,8 +12882,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", @@ -12891,8 +12890,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 4065535..f9072a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -237,22 +237,13 @@ 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 +565,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}; @@ -745,7 +736,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) { @@ -913,19 +904,15 @@ 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; - - } virPCIDeviceSetUsedBy(dev, drv_name, dom_name); + 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); + /* Setup the original states for the PCI device */ virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub); virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot); diff --git a/src/util/virpci.c b/src/util/virpci.c index 21eacf5..aec7b69 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -55,6 +55,13 @@ 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, + "none", + "pciback", /* XEN */ + "pci-stub", /* KVM */ + "vfio-pci", /* VFIO */ +); + struct _virPCIDevice { virPCIDeviceAddress address; @@ -71,7 +78,8 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; - char *stubDriver; + + virPCIStubDriver stubDriver; /* used by reattach function */ bool unbind_from_stub; @@ -941,7 +949,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; @@ -992,13 +1000,22 @@ 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)) || + driver == VIR_PCI_STUB_DRIVER_NONE) { + 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; @@ -1009,8 +1026,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; } @@ -1022,15 +1039,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; @@ -1073,13 +1090,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) { @@ -1087,8 +1097,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,16 +1113,12 @@ 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 (virPCIStubDriverTypeFromString(driver) < 0 || + virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) goto remove_slot; + VIR_DEBUG("Found stub driver %s", driver); + if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false; @@ -1183,9 +1187,22 @@ 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; + /* Check the device is configured to use one of the known stub drivers */ + if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No stub driver configured for PCI device %s"), + dev->name); + return -1; + } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown stub driver configured for PCI device %s"), + dev->name); + return -1; + } + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || !(driverLink = virPCIFile(dev->name, "driver"))) goto cleanup; @@ -1338,8 +1355,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); @@ -1683,14 +1696,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 e628ab8..1606731 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -43,6 +43,16 @@ struct _virPCIDeviceAddress { }; typedef enum { + VIR_PCI_STUB_DRIVER_NONE = 0, + VIR_PCI_STUB_DRIVER_XEN, + 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 +100,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 285a553..8d9ac07 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2532,8 +2532,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 2a74976..1b30681 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 1ade3e5..fb0c970 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; @@ -269,8 +271,7 @@ testVirPCIDeviceDetachFail(const void *opaque) 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()) @@ -281,7 +282,7 @@ testVirPCIDeviceDetachFail(const void *opaque) virReportError(VIR_ERR_INTERNAL_ERROR, "Attaching device %s to %s should have failed", virPCIDeviceGetName(dev), - virPCIDeviceGetStubDriver(dev)); + virPCIStubDriverTypeToString(VIR_PCI_STUB_DRIVER_VFIO)); } cleanup: -- 2.5.0
On 17.12.2015 18:59, Andrea Bolognani wrote:
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. --- Changes in v2:
* add VIR_PCI_STUB_DRIVER_NONE so we can detect when no driver has been configured for a specific device * simplify code in testVirPCIDeviceDetachFail() by not reading the driver back from the device, since we set it a few lines above
testVirPCIDeviceDetachFail src/libvirt_private.syms | 2 ++ src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 ++-- src/util/virhostdev.c | 45 +++++++++---------------- src/util/virpci.c | 86 +++++++++++++++++++++++++++--------------------- src/util/virpci.h | 17 +++++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 +-- tests/virpcitest.c | 23 ++++++------- 9 files changed, 99 insertions(+), 91 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 4065535..f9072a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -237,22 +237,13 @@ 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 +565,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);
I believe these braces are redundant. But do not hurt either.
struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name, usesVfio};
@@ -745,7 +736,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) { @@ -913,19 +904,15 @@ 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; - - } virPCIDeviceSetUsedBy(dev, drv_name, dom_name);
+ 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); + /* Setup the original states for the PCI device */ virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub); virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot); diff --git a/src/util/virpci.c b/src/util/virpci.c index 21eacf5..aec7b69 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -55,6 +55,13 @@ 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, + "none", + "pciback", /* XEN */ + "pci-stub", /* KVM */ + "vfio-pci", /* VFIO */ +); + struct _virPCIDevice { virPCIDeviceAddress address;
@@ -71,7 +78,8 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; - char *stubDriver; + + virPCIStubDriver stubDriver;
/* used by reattach function */ bool unbind_from_stub; @@ -941,7 +949,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; @@ -992,13 +1000,22 @@ 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)) || + driver == VIR_PCI_STUB_DRIVER_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Attempting to use unknown stub driver")); + return -1; + }
Hm. Interesting, I thought that checking for TypeToString(driver) would be useless, since we are passing an enum into the function. But apparently I was wrong since the following code does not throw any error whatsoever: virPCIProbeStubDriver(20); So I guess it's a good idea to have it there. Also, this brings up an interesting question - should we check for other TypeToString() return values too? e.g. like we do in virCPUDefFormatBuf().
+ recheck: - if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) { + if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) { /* driver already loaded, return */ VIR_FREE(drvpath); return 0;
ACK Michal
On 12/18/2015 07:05 AM, Michal Privoznik wrote:
On 17.12.2015 18:59, Andrea Bolognani wrote:
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. --- Changes in v2:
* add VIR_PCI_STUB_DRIVER_NONE so we can detect when no driver has been configured for a specific device * simplify code in testVirPCIDeviceDetachFail() by not reading the driver back from the device, since we set it a few lines above
testVirPCIDeviceDetachFail src/libvirt_private.syms | 2 ++ src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 ++-- src/util/virhostdev.c | 45 +++++++++---------------- src/util/virpci.c | 86 +++++++++++++++++++++++++++--------------------- src/util/virpci.h | 17 +++++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 +-- tests/virpcitest.c | 23 ++++++------- 9 files changed, 99 insertions(+), 91 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 4065535..f9072a4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -237,22 +237,13 @@ 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 +565,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); I believe these braces are redundant. But do not hurt either.
struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name, usesVfio};
@@ -745,7 +736,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) { @@ -913,19 +904,15 @@ 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; - - } virPCIDeviceSetUsedBy(dev, drv_name, dom_name);
+ 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); + /* Setup the original states for the PCI device */ virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub); virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot); diff --git a/src/util/virpci.c b/src/util/virpci.c index 21eacf5..aec7b69 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -55,6 +55,13 @@ 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, + "none", + "pciback", /* XEN */ + "pci-stub", /* KVM */ + "vfio-pci", /* VFIO */ +); + struct _virPCIDevice { virPCIDeviceAddress address;
@@ -71,7 +78,8 @@ struct _virPCIDevice { bool has_flr; bool has_pm_reset; bool managed; - char *stubDriver; + + virPCIStubDriver stubDriver;
/* used by reattach function */ bool unbind_from_stub; @@ -941,7 +949,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; @@ -992,13 +1000,22 @@ 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)) || + driver == VIR_PCI_STUB_DRIVER_NONE) {
You could save a small bit of time by comparing to VIR_PCI_STUB_DRIVER_NONE first, and then calling ...TypeToString()...
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Attempting to use unknown stub driver")); + return -1; + } Hm. Interesting, I thought that checking for TypeToString(driver) would be useless, since we are passing an enum into the function. But apparently I was wrong since the following code does not throw any error whatsoever:
virPCIProbeStubDriver(20);
You mean no error at compile time I guess (I was a bit confused at first, trying to see how virPCIStubDriverTypeToString could possibly not return NULL for an out of range enum value :-)
So I guess it's a good idea to have it there. Also, this brings up an interesting question - should we check for other TypeToString() return values too? e.g. like we do in virCPUDefFormatBuf().
There is inconsistency in this throughout the code. I think older *Format functions tend to not check the return, as they are assuming that the data being formatted was previously parsed (and the parse would only have put a proper enum in there), but newer code is more likely to be paranoid and check the return value. I admit to having done both, depending on just how paranoid I'm feeling at the time - not checking the return makes the code cleaner and there is a *very* low probability that the data will have come from somewhere other than the output of the parser; on the other hand, this is not always the case, and adding in the checks for NULL assures that we catch a bug in the code sooner rather than later. Definitely if you're calling it with data that didn't come directly out of a parser, then you really really should be checking the return for NULL.
+ recheck: - if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) { + if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) { /* driver already loaded, return */ VIR_FREE(drvpath); return 0;
ACK
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, 2015-12-18 at 11:12 -0500, Laine Stump wrote:
+ if (!(drvname = virPCIStubDriverTypeToString(driver)) || + driver == VIR_PCI_STUB_DRIVER_NONE) { You could save a small bit of time by comparing to VIR_PCI_STUB_DRIVER_NONE first, and then calling ...TypeToString()...
Okay, will squash that in before pushing.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Attempting to use unknown stub driver")); + return -1; + } Hm. Interesting, I thought that checking for TypeToString(driver) would be useless, since we are passing an enum into the function. But apparently I was wrong since the following code does not throw any error whatsoever:
virPCIProbeStubDriver(20); You mean no error at compile time I guess (I was a bit confused at first, trying to see how virPCIStubDriverTypeToString could possibly not return NULL for an out of range enum value :-)
Yes, he meant compile time :)
So I guess it's a good idea to have it there. Also, this brings up an interesting question - should we check for other TypeToString() return values too? e.g. like we do in virCPUDefFormatBuf(). There is inconsistency in this throughout the code. I think older *Format functions tend to not check the return, as they are assuming that the data being formatted was previously parsed (and the parse would only have put a proper enum in there), but newer code is more likely to be paranoid and check the return value. I admit to having done both, depending on just how paranoid I'm feeling at the time - not checking the return makes the code cleaner and there is a *very* low probability that the data will have come from somewhere other than the output of the parser; on the other hand, this is not always the case, and adding in the checks for NULL assures that we catch a bug in the code sooner rather than later. Definitely if you're calling it with data that didn't come directly out of a parser, then you really really should be checking the return for NULL.
Both me and Michal thought the compiler would be smart enough to realize that the code above doesn't make much sense and emit at the very least a warning. Of course it's not going to be able to do anything if the value is read at runtime, but in this case it would be right in the code and still it would compile just fine. Maybe other tools like coverity can detect this kind of mistake and point it out? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
We want to eventually factor out the code dealing with device detaching and reattaching, so that we can share it and make sure it's called eg. when 'virsh nodedev-detach' is used. For that to happen, it's important that the lists of active and inactive PCI devices are updated every time a device changes its state. Instead of passing NULL as the last argument of virPCIDeviceDetach() and virPCIDeviceReattach(), pass the proper list so that it can be updated. --- src/util/virhostdev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f9072a4..afacd4e 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -595,11 +595,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + if (virPCIDeviceGetManaged(dev) && - virPCIDeviceDetach(dev, hostdev_mgr->activePCIHostdevs, NULL) < 0) - goto reattachdevs; + virPCIDeviceDetach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs) < 0) + goto reattachdevs; } + /* At this point, all devices are attached to the stub driver and have + * been marked as inactive */ + /* Loop 3: Now that all the PCI hostdevs have been detached, we * can safely reset them */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -708,8 +714,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* NB: This doesn't actually re-bind to original driver, just * unbinds from the stub driver */ - ignore_value(virPCIDeviceReattach(dev, hostdev_mgr->activePCIHostdevs, - NULL)); + ignore_value(virPCIDeviceReattach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs)); } cleanup: -- 2.5.0
On 12/17/2015 10:23 AM, Andrea Bolognani wrote:
We want to eventually factor out the code dealing with device detaching and reattaching, so that we can share it and make sure it's called eg. when 'virsh nodedev-detach' is used.
For that to happen, it's important that the lists of active and inactive PCI devices are updated every time a device changes its state.
We need to know which devices from an iommu group were unbound from the host driver by us, so that we'll only re-bind those that we had unbound in the first place. So I can see the reasoning for wanting the inactive list to always hold the complete list of devices that libvirt has detached from the host driver, but that aren't at the moment in use by any domain. In this case that you're fixing, though, it's only a temporary inconsistency, since "Loop 6" of that same function will add the detached devices to the inactive list. However, when I trace down into the one use of the inactive list between Loop 2 (where you're adding the devices in this patch) and Loop 6 (where they were previously added), I've found that your patch may actually be fixing a latent bug, but only in the case where someone is using the legacy KVM device assignment - if virPCIDeviceReset() (which returns with no action when vfio-pci is used) is unsuccessful at resetting the device individually, it will try resetting the entire bus the device is on using virPCIDeviceTrySecondaryBusReset(), but it can't do this unless all the other devices on the bus are unused. This is determined by calling virPCIDeviceBusContainsActiveDevices(), which iterates through every PCI device on the host calling virPCIDeviceSharesBusWithActive(). That function will return true if it finds a device on the same bus that *isn't* on the inactive list. So if a device is on a bus with multiple devices, those devices have all been assigned to the guest using legacy KVM assignment, and the normal device reset functions don't work for them, the current code would fail, while yours would succeed. *Highly* unlikely (and we don't really care, since legacy KVM device assignment is, well, legacy; deprecated; soon to go away; "pining for the fjords" as it were :-) (sorry for the digression. I spent so much time investigating that it didn't feel right to just conclude the search by saying "nothing here. Move on!) Anyway I see no problem caused by this patch, so ACK. I guess you also intend to begin storing the inactive device list somewhere so that it can be reread if libvirtd is restarted?
Instead of passing NULL as the last argument of virPCIDeviceDetach() and virPCIDeviceReattach(), pass the proper list so that it can be updated. --- src/util/virhostdev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f9072a4..afacd4e 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -595,11 +595,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + if (virPCIDeviceGetManaged(dev) && - virPCIDeviceDetach(dev, hostdev_mgr->activePCIHostdevs, NULL) < 0) - goto reattachdevs; + virPCIDeviceDetach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs) < 0) + goto reattachdevs; }
+ /* At this point, all devices are attached to the stub driver and have + * been marked as inactive */ + /* Loop 3: Now that all the PCI hostdevs have been detached, we * can safely reset them */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -708,8 +714,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* NB: This doesn't actually re-bind to original driver, just * unbinds from the stub driver */ - ignore_value(virPCIDeviceReattach(dev, hostdev_mgr->activePCIHostdevs, - NULL)); + ignore_value(virPCIDeviceReattach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs)); }
cleanup:
Very quick reply as I have a bus to Italy to catch in a few hours :) On Fri, 2015-12-18 at 10:37 -0500, Laine Stump wrote:
We want to eventually factor out the code dealing with device detaching and reattaching, so that we can share it and make sure it's called eg. when 'virsh nodedev-detach' is used. For that to happen, it's important that the lists of active and inactive PCI devices are updated every time a device changes its state. We need to know which devices from an iommu group were unbound from the host driver by us, so that we'll only re-bind those that we had unbound in the first place. So I can see the reasoning for wanting the inactive
On 12/17/2015 10:23 AM, Andrea Bolognani wrote: list to always hold the complete list of devices that libvirt has detached from the host driver, but that aren't at the moment in use by any domain.
In general, we're doing an okay job at keeping track of active / inactive devices, but the handling is all over the place which makes it kinda difficult to split off chunks of code for reuse.
In this case that you're fixing, though, it's only a temporary inconsistency, since "Loop 6" of that same function will add the detached devices to the inactive list.
I'm aware of that, but once the "detach devices" and "reattach devices" parts have been split off for reuse we can no longer count on it :)
However, when I trace down into the one use of the inactive list between Loop 2 (where you're adding the devices in this patch) and Loop 6 (where they were previously added), I've found that your patch may actually be fixing a latent bug, but only in the case where someone is using the legacy KVM device assignment - if virPCIDeviceReset() (which returns with no action when vfio-pci is used) is unsuccessful at resetting the device individually, it will try resetting the entire bus the device is on using virPCIDeviceTrySecondaryBusReset(), but it can't do this unless all the other devices on the bus are unused. This is determined by calling virPCIDeviceBusContainsActiveDevices(), which iterates through every PCI device on the host calling virPCIDeviceSharesBusWithActive(). That function will return true if it finds a device on the same bus that *isn't* on the inactive list. So if a device is on a bus with multiple devices, those devices have all been assigned to the guest using legacy KVM assignment, and the normal device reset functions don't work for them, the current code would fail, while yours would succeed. *Highly* unlikely (and we don't really care, since legacy KVM device assignment is, well, legacy; deprecated; soon to go away; "pining for the fjords" as it were :-)
I see why that would be a problem. I believe that, in general, the more explicit we are about marking devices as active or inactive, the less likely it will be to end up in such a situation.
(sorry for the digression. I spent so much time investigating that it didn't feel right to just conclude the search by saying "nothing here. Move on!)
Doesn't look like something you need to apologize for :)
Anyway I see no problem caused by this patch, so ACK. I guess you also intend to begin storing the inactive device list somewhere so that it can be reread if libvirtd is restarted?
I haven't really tackled the problem yet, but the more I think about it the more I believe we need to. We could of course figure out at least a good part of the information we need by looking at the system state, but we wouldn't be able to eg. tell whether a device that's bound to vfio-pci was bound to a host driver before being detached. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Since we don't detach unmanaged devices before attaching them to a domain, we shouldn't reattach them to rollback an error either. --- src/util/virhostdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index afacd4e..c8da8e5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -711,12 +711,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); + if (virPCIDeviceGetManaged(dev)) { + /* NB: This doesn't actually re-bind to original driver, just + * unbinds from the stub driver + */ + ignore_value(virPCIDeviceReattach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs)); + } } cleanup: -- 2.5.0
On 17.12.2015 16:23, Andrea Bolognani wrote:
Since we don't detach unmanaged devices before attaching them to a domain, we shouldn't reattach them to rollback an error either. --- src/util/virhostdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index afacd4e..c8da8e5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -711,12 +711,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); + if (virPCIDeviceGetManaged(dev)) { + /* NB: This doesn't actually re-bind to original driver, just + * unbinds from the stub driver + */ + ignore_value(virPCIDeviceReattach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs)); + } }
cleanup:
This is exactly what has came to my mind when reviewing previous patch :-) Michal
On 12/17/2015 10:23 AM, Andrea Bolognani wrote:
Since we don't detach unmanaged devices before attaching them to a domain, we shouldn't reattach them to rollback an error either.
Yes, definitely ACK. This patch makes the error cleanup code exactly mirror the code it is cleaning up from.
--- src/util/virhostdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index afacd4e..c8da8e5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -711,12 +711,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); + if (virPCIDeviceGetManaged(dev)) { + /* NB: This doesn't actually re-bind to original driver, just + * unbinds from the stub driver + */ + ignore_value(virPCIDeviceReattach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs)); + } }
cleanup:
Both detach and reattach are complex operations involving several steps, and it can be useful to be able to follow along by reading the log. --- src/util/virhostdev.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index c8da8e5..f31ad41 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -596,11 +596,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - if (virPCIDeviceGetManaged(dev) && - virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + if (virPCIDeviceGetManaged(dev)) { + VIR_DEBUG("Detaching managed PCI device %s", + virPCIDeviceGetName(dev)); + if (virPCIDeviceDetach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs) < 0) goto reattachdevs; + } else { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + } } /* At this point, all devices are attached to the stub driver and have @@ -611,6 +617,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs) < 0) goto reattachdevs; @@ -632,14 +639,20 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 5: Now mark all the devices as active */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Adding PCI device %s to active list", + virPCIDeviceGetName(dev)); if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) goto inactivedevs; } /* Loop 6: Now remove the devices from inactive list. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Removing PCI device %s from inactive list", + virPCIDeviceGetName(dev)); + virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); } /* Loop 7: Now set the used_by_domain of the device in @@ -651,6 +664,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceListGet(pcidevs, i); activeDev = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev); + VIR_DEBUG("Setting driver and domain information for PCI device %s", + virPCIDeviceGetName(dev)); if (activeDev) virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); } @@ -674,6 +689,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * "reprobe" were already set by pciDettachDevice in * loop 2. */ + VIR_DEBUG("Saving network configuration of PCI device %s", + virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { hostdev->origstates.states.pci.unbind_from_stub = virPCIDeviceGetUnbindFromStub(pcidev); @@ -699,6 +716,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Removing PCI device %s from active list", + virPCIDeviceGetName(dev)); virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, dev); } @@ -715,9 +735,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* NB: This doesn't actually re-bind to original driver, just * unbinds from the stub driver */ + VIR_DEBUG("Reattaching managed PCI device %s", + virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs)); + } else { + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); } } @@ -739,6 +764,8 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) * successfully, it must have been inactive. */ if (!virPCIDeviceGetManaged(dev)) { + VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", + virPCIDeviceGetName(dev)); if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) virPCIDeviceFree(dev); return; @@ -754,6 +781,7 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) } } + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); @@ -821,6 +849,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } + VIR_DEBUG("Removing PCI device %s from active list", + virPCIDeviceGetName(dev)); virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); i++; } @@ -843,6 +873,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, pcisrc->addr.slot, pcisrc->addr.function); if (dev) { if (virPCIDeviceListFind(pcidevs, dev)) { + VIR_DEBUG("Restoring network configuration of PCI device %s", + virPCIDeviceGetName(dev)); virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, oldStateDir); } @@ -855,6 +887,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); -- 2.5.0
On 12/17/2015 10:24 AM, Andrea Bolognani wrote:
Both detach and reattach are complex operations involving several steps, and it can be useful to be able to follow along by reading the log. ---
ACK. Unfortunately it will all just get buried in the inane miles-long logs of converting PCI address strings into integers step by step. (there are some things that really should only be logged in a "super-debug-dev" mode. We don't need to fill up GB and GB (oh, sorry - GiB and GiB :-P) of space all over the world with debug logs that simply verify that our LongToStr function is still working. But that's not your problem...)
src/util/virhostdev.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index c8da8e5..f31ad41 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -596,11 +596,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (virPCIDeviceGetManaged(dev) && - virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + if (virPCIDeviceGetManaged(dev)) { + VIR_DEBUG("Detaching managed PCI device %s", + virPCIDeviceGetName(dev)); + if (virPCIDeviceDetach(dev, + hostdev_mgr->activePCIHostdevs, + hostdev_mgr->inactivePCIHostdevs) < 0) goto reattachdevs; + } else { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); + } }
/* At this point, all devices are attached to the stub driver and have @@ -611,6 +617,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs) < 0) goto reattachdevs; @@ -632,14 +639,20 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 5: Now mark all the devices as active */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Adding PCI device %s to active list", + virPCIDeviceGetName(dev)); if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) goto inactivedevs; }
/* Loop 6: Now remove the devices from inactive list. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Removing PCI device %s from inactive list", + virPCIDeviceGetName(dev)); + virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); }
/* Loop 7: Now set the used_by_domain of the device in @@ -651,6 +664,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceListGet(pcidevs, i); activeDev = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev);
+ VIR_DEBUG("Setting driver and domain information for PCI device %s", + virPCIDeviceGetName(dev)); if (activeDev) virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); } @@ -674,6 +689,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * "reprobe" were already set by pciDettachDevice in * loop 2. */ + VIR_DEBUG("Saving network configuration of PCI device %s", + virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { hostdev->origstates.states.pci.unbind_from_stub = virPCIDeviceGetUnbindFromStub(pcidev); @@ -699,6 +716,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + VIR_DEBUG("Removing PCI device %s from active list", + virPCIDeviceGetName(dev)); virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, dev); }
@@ -715,9 +735,14 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* NB: This doesn't actually re-bind to original driver, just * unbinds from the stub driver */ + VIR_DEBUG("Reattaching managed PCI device %s", + virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs)); + } else { + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(dev)); } }
@@ -739,6 +764,8 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) * successfully, it must have been inactive. */ if (!virPCIDeviceGetManaged(dev)) { + VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", + virPCIDeviceGetName(dev)); if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) virPCIDeviceFree(dev); return; @@ -754,6 +781,7 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) } }
+ VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); @@ -821,6 +849,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } }
+ VIR_DEBUG("Removing PCI device %s from active list", + virPCIDeviceGetName(dev)); virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); i++; } @@ -843,6 +873,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, pcisrc->addr.slot, pcisrc->addr.function); if (dev) { if (virPCIDeviceListFind(pcidevs, dev)) { + VIR_DEBUG("Restoring network configuration of PCI device %s", + virPCIDeviceGetName(dev)); virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, oldStateDir); } @@ -855,6 +887,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, hostdev_mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError();
On 17.12.2015 16:20, Andrea Bolognani wrote:
While working on v2 of my series[1] intended to fix
https://bugzilla.redhat.com/show_bug.cgi?id=1272300
I'm running into some smaller stuff that isn't necessarily strictly related to the issue at hand. The idea is to have this smaller patches merged before the bigger, more invasive changes that series will bring, and hopefully make it easier to review by laying down the groundwork in advance.
The series is organized as follows:
1-2: remove function parameters that are either not inspected by the function or never have an interesting value 3: change a string field that can only assume one of three values to a proper enumeration 4: improve bookkeeping when detaching PCI devices from the host, by making sure they always transition through the 'inactive' state (when they're attached to the stub driver) 5: make sure we never try to reattach an unmanaged device to the host, not even when recovering from a previous error 6: add some useful debug messages
I expect patches 1, 2 and 6 to be fairly uncontroversial and easy to review; patch 3 is kinda big, but it's IMHO a nice improvement over the current situation; patches 4 and 5 are small but definitely need a long hard stare from a different pair of eyes to ensure they're not breaking anything.
Cheers.
[1] https://www.redhat.com/archives/libvir-list/2015-December/msg00070.html
Andrea Bolognani (6): pci: Remove redundant parameter from virPCIDeviceBindToStub() pci: Remove 'reprobe' parameter from virPCIDeviceUnbind() pci: Introduce virPCIStubDriver enumeration hostdev: Mark PCI devices as inactive as they're detached hostdev: Only rollback detach of managed devices on error hostdev: Emit debug messages while handling PCI hostdevs
src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 +-- src/util/virhostdev.c | 107 ++++++++++++++++++++++++++++++----------------- src/util/virpci.c | 102 ++++++++++++++++++++++---------------------- src/util/virpci.h | 18 +++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++- tests/virpcitest.c | 35 ++++++++++------ 9 files changed, 163 insertions(+), 118 deletions(-)
ACK series, although before pushing I expect some answers in 3/6. Michal
On Thu, 2015-12-17 at 17:28 +0100, Michal Privoznik wrote:
Andrea Bolognani (6): pci: Remove redundant parameter from virPCIDeviceBindToStub() pci: Remove 'reprobe' parameter from virPCIDeviceUnbind() pci: Introduce virPCIStubDriver enumeration hostdev: Mark PCI devices as inactive as they're detached hostdev: Only rollback detach of managed devices on error hostdev: Emit debug messages while handling PCI hostdevs src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 +-- src/util/virhostdev.c | 107 ++++++++++++++++++++++++++++++----------------- src/util/virpci.c | 102 ++++++++++++++++++++++---------------------- src/util/virpci.h | 18 +++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++- tests/virpcitest.c | 35 ++++++++++------ 9 files changed, 163 insertions(+), 118 deletions(-) ACK series, although before pushing I expect some answers in 3/6.
I posted a v2 for 3/6. I'm also going to wait until tomorrow before pushing to give other people a chance to take a look at 4/6 and 5/6 and weigh in. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, 2015-12-17 at 17:28 +0100, Michal Privoznik wrote:
Andrea Bolognani (6): pci: Remove redundant parameter from virPCIDeviceBindToStub() pci: Remove 'reprobe' parameter from virPCIDeviceUnbind() pci: Introduce virPCIStubDriver enumeration hostdev: Mark PCI devices as inactive as they're detached hostdev: Only rollback detach of managed devices on error hostdev: Emit debug messages while handling PCI hostdevs src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 3 +- src/qemu/qemu_driver.c | 6 +-- src/util/virhostdev.c | 107 ++++++++++++++++++++++++++++++----------------- src/util/virpci.c | 102 ++++++++++++++++++++++---------------------- src/util/virpci.h | 18 +++++--- src/xen/xen_driver.c | 3 +- tests/virhostdevtest.c | 5 ++- tests/virpcitest.c | 35 ++++++++++------ 9 files changed, 163 insertions(+), 118 deletions(-) ACK series, although before pushing I expect some answers in 3/6.
Pushed after v2 3/6 was ACKed and having squashed in the improvement suggested by Laine. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (3)
-
Andrea Bolognani -
Laine Stump -
Michal Privoznik