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