Re: [libvirt] [PATCH 8/8] Maintain a list of active PCI hostdevs and use it in pciResetDevice()

As we start/shutdown guests, or hotplug/hot-unplug devices, we can add or delete devices as appropriate from a list of active devices. Then, in pciReset(), we can use this to determine whether its safe to reset a device as a side effect of resetting another device. * src/qemu_conf.h: add activePciHostdevs to qemud_driver * src/qemu_driver.c: maintain the activePciHostdevs list, and pass it to pciResetDevice() * src/pci.[ch]: pass the activeDevs list to pciResetDevice() and use it to determine whether a Secondary Bus Reset is safe --- src/pci.c | 102 ++++++++++++++++++++++++++------------------ src/pci.h | 3 +- src/qemu_conf.h | 3 + src/qemu_driver.c | 123 ++++++++++++++++++++++++++++++++++++++++++---------- src/xen_unified.c | 2 +- 5 files changed, 165 insertions(+), 68 deletions(-) diff --git a/src/pci.c b/src/pci.c index a37eaf7..96e5d6d 100644 --- a/src/pci.c +++ b/src/pci.c @@ -226,7 +226,7 @@ pciWrite32(pciDevice *dev, unsigned pos, uint32_t val) pciWrite(dev, pos, &buf[0], sizeof(buf)); } -typedef int (*pciIterPredicate)(pciDevice *, pciDevice *); +typedef int (*pciIterPredicate)(pciDevice *, pciDevice *, void *); /* Iterate over available PCI devices calling @predicate * to compare each one to @dev. @@ -237,7 +237,8 @@ static int pciIterDevices(virConnectPtr conn, pciIterPredicate predicate, pciDevice *dev, - pciDevice **matched) + pciDevice **matched, + void *data) { DIR *dir; struct dirent *entry; @@ -255,7 +256,7 @@ pciIterDevices(virConnectPtr conn, while ((entry = readdir(dir))) { unsigned domain, bus, slot, function; - pciDevice *try; + pciDevice *check; /* Ignore '.' and '..' */ if (entry->d_name[0] == '.') @@ -267,18 +268,18 @@ pciIterDevices(virConnectPtr conn, continue; } - try = pciGetDevice(conn, domain, bus, slot, function); - if (!try) { + check = pciGetDevice(conn, domain, bus, slot, function); + if (!check) { ret = -1; break; } - if (predicate(try, dev)) { - VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, try->name); - *matched = try; + if (predicate(dev, check, data)) { + VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name); + *matched = check; break; } - pciFreeDevice(conn, try); + pciFreeDevice(conn, check); } closedir(dir); return ret; @@ -380,63 +381,70 @@ pciDetectPowerManagementReset(pciDevice *dev) return 0; } -/* Any devices other than the one supplied on the same domain/bus ? */ +/* Any active devices other than the one supplied on the same domain/bus ? */ static int -pciSharesBus(pciDevice *a, pciDevice *b) +pciSharesBusWithActive(pciDevice *dev, pciDevice *check, void *data) { - return - a->domain == b->domain && - a->bus == b->bus && - (a->slot != b->slot || - a->function != b->function); -} + pciDeviceList *activeDevs = data; -static int -pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev) -{ - pciDevice *matched = NULL; - if (pciIterDevices(conn, pciSharesBus, dev, &matched) < 0) - return 1; - if (!matched) + if (dev->domain != check->domain || + dev->bus != check->bus || + (check->slot == check->slot && + check->function == check->function)) + return 0; + + if (activeDevs && !pciDeviceListFind(activeDevs, check)) return 0; - pciFreeDevice(conn, matched); + return 1; } -/* Is @a the parent of @b ? */ +static pciDevice * +pciBusContainsActiveDevices(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) +{ + pciDevice *active = NULL; + if (pciIterDevices(conn, pciSharesBusWithActive, + dev, &active, activeDevs) < 0) + return NULL; + return active; +} + +/* Is @check the parent of @dev ? */ static int -pciIsParent(pciDevice *a, pciDevice *b) +pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) { uint16_t device_class; uint8_t header_type, secondary, subordinate; - if (a->domain != b->domain) + if (dev->domain != check->domain) return 0; /* Is it a bridge? */ - device_class = pciRead16(a, PCI_CLASS_DEVICE); + device_class = pciRead16(check, PCI_CLASS_DEVICE); if (device_class != PCI_CLASS_BRIDGE_PCI) return 0; /* Is it a plane? */ - header_type = pciRead8(a, PCI_HEADER_TYPE); + header_type = pciRead8(check, PCI_HEADER_TYPE); if ((header_type & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_BRIDGE) return 0; - secondary = pciRead8(a, PCI_SECONDARY_BUS); - subordinate = pciRead8(a, PCI_SUBORDINATE_BUS); + secondary = pciRead8(check, PCI_SECONDARY_BUS); + subordinate = pciRead8(check, PCI_SUBORDINATE_BUS); - VIR_DEBUG("%s %s: found parent device %s\n", b->id, b->name, a->name); + VIR_DEBUG("%s %s: found parent device %s\n", dev->id, dev->name, check->name); /* No, it's superman! */ - return (b->bus >= secondary && b->bus <= subordinate); + return (dev->bus >= secondary && dev->bus <= subordinate); } static pciDevice * pciGetParentDevice(virConnectPtr conn, pciDevice *dev) { pciDevice *parent = NULL; - pciIterDevices(conn, pciIsParent, dev, &parent); + pciIterDevices(conn, pciIsParent, dev, &parent, NULL); return parent; } @@ -444,9 +452,11 @@ pciGetParentDevice(virConnectPtr conn, pciDevice *dev) * devices behind a bus. */ static int -pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) +pciTrySecondaryBusReset(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) { - pciDevice *parent; + pciDevice *parent, *conflict; uint8_t config_space[PCI_CONF_LEN]; uint16_t ctl; int ret = -1; @@ -456,10 +466,10 @@ pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) * In future, we could allow it so long as those devices * are not in use by the host or other guests. */ - if (pciBusContainsOtherDevices(conn, dev)) { + if ((conflict = pciBusContainsActiveDevices(conn, dev, activeDevs))) { pciReportError(conn, VIR_ERR_NO_SUPPORT, - _("Other devices on bus with %s, not doing bus reset"), - dev->name); + _("Active %s devices on bus with %s, not doing bus reset"), + conflict->name, dev->name); return -1; } @@ -573,10 +583,18 @@ pciInitDevice(virConnectPtr conn, pciDevice *dev) } int -pciResetDevice(virConnectPtr conn, pciDevice *dev) +pciResetDevice(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) { int ret = -1; + if (activeDevs && pciDeviceListFind(activeDevs, dev)) { + pciReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Not resetting active device %s"), dev->name); + return -1; + } + if (!dev->initted && pciInitDevice(conn, dev) < 0) return -1; @@ -595,7 +613,7 @@ pciResetDevice(virConnectPtr conn, pciDevice *dev) /* Bus reset is not an option with the root bus */ if (ret < 0 && dev->bus != 0) - ret = pciTrySecondaryBusReset(conn, dev); + ret = pciTrySecondaryBusReset(conn, dev, activeDevs); if (ret < 0) { virErrorPtr err = virGetLastError(); diff --git a/src/pci.h b/src/pci.h index 75fbd51..685b0af 100644 --- a/src/pci.h +++ b/src/pci.h @@ -44,7 +44,8 @@ int pciDettachDevice (virConnectPtr conn, int pciReAttachDevice (virConnectPtr conn, pciDevice *dev); int pciResetDevice (virConnectPtr conn, - pciDevice *dev); + pciDevice *dev, + pciDeviceList *activeDevs); void pciDeviceSetManaged(pciDevice *dev, unsigned managed); unsigned pciDeviceGetManaged(pciDevice *dev); diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 8f9e1c1..a126dac 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -35,6 +35,7 @@ #include "threads.h" #include "security.h" #include "cgroup.h" +#include "pci.h" #define qemudDebug(fmt, ...) do {} while(0) @@ -111,6 +112,8 @@ struct qemud_driver { virSecurityDriverPtr securityDriver; char *saveImageFormat; + + pciDeviceList *activePciHostdevs; }; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f2b807b..70d3d55 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -128,6 +128,9 @@ static int qemudDomainSetMemoryBalloon(virConnectPtr conn, static int qemudDetectVcpuPIDs(virConnectPtr conn, virDomainObjPtr vm); +static int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, + virDomainDefPtr def); + static struct qemud_driver *qemu_driver = NULL; static int qemuCgroupControllerActive(struct qemud_driver *driver, @@ -320,6 +323,10 @@ qemuReconnectDomain(struct qemud_driver *driver, goto error; } + if (qemuUpdateActivePciHostdevs(driver, obj->def) < 0) { + goto error; + } + if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && driver->securityDriver && driver->securityDriver->domainReserveSecurityLabel && @@ -524,6 +531,9 @@ qemudStartup(int privileged) { if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory; + if ((qemu_driver->activePciHostdevs = pciDeviceListNew(NULL)) == NULL) + goto error; + if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { goto error; } @@ -648,6 +658,7 @@ qemudShutdown(void) { return -1; qemuDriverLock(qemu_driver); + pciDeviceListFree(NULL, qemu_driver->activePciHostdevs); virCapabilitiesFree(qemu_driver->caps); virDomainObjListFree(&qemu_driver->domains); @@ -1371,7 +1382,38 @@ qemuGetPciHostDeviceList(virConnectPtr conn, } static int -qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) +qemuUpdateActivePciHostdevs(struct qemud_driver *driver, + virDomainDefPtr def) +{ + pciDeviceList *pcidevs; + int i, ret; + + if (!def->nhostdevs) + return 0; + + if (!(pcidevs = qemuGetPciHostDeviceList(NULL, def))) + return -1; + + ret = 0; + + for (i = 0; i < pcidevs->count; i++) { + if (pciDeviceListAdd(NULL, + driver->activePciHostdevs, + pcidevs->devs[i]) < 0) { + ret = -1; + break; + } + pcidevs->devs[i] = NULL; + } + + pciDeviceListFree(NULL, pcidevs); + return ret; +} + +static int +qemuPrepareHostDevices(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def) { pciDeviceList *pcidevs; int i; @@ -1382,10 +1424,11 @@ qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) return -1; - /* We have to use 2 loops here. *All* devices must + /* We have to use 3 loops here. *All* devices must * be detached before we reset any of them, because * in some cases you have to reset the whole PCI, - * which impacts all devices on it + * which impacts all devices on it. Also, all devices + * must be reset before being marked as active. */ /* XXX validate that non-managed device isn't in use, eg @@ -1401,9 +1444,19 @@ qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) /* Now that all the PCI hostdevs have be dettached, we can safely * reset them */ for (i = 0; i < pcidevs->count; i++) - if (pciResetDevice(conn, pcidevs->devs[i]) < 0) + if (pciResetDevice(conn, pcidevs->devs[i], + driver->activePciHostdevs) < 0) goto error; + /* Now mark all the devices as active */ + for (i = 0; i < pcidevs->count; i++) { + if (pciDeviceListAdd(conn, + driver->activePciHostdevs, + pcidevs->devs[i]) < 0) + goto error; + pcidevs->devs[i] = NULL; + } + pciDeviceListFree(conn, pcidevs); return 0; @@ -1413,7 +1466,9 @@ error: } static void -qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) +qemuDomainReAttachHostDevices(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def) { pciDeviceList *pcidevs; int i; @@ -1429,10 +1484,15 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) return; } - /* Again 2 loops; reset all the devices before re-attach */ + /* Again 3 loops; mark all devices as inactive before reset + * them and reset all the devices before re-attach */ for (i = 0; i < pcidevs->count; i++) - if (pciResetDevice(conn, pcidevs->devs[i]) < 0) { + pciDeviceListDel(conn, driver->activePciHostdevs, pcidevs->devs[i]); + + for (i = 0; i < pcidevs->count; i++) + if (pciResetDevice(conn, pcidevs->devs[i], + driver->activePciHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s\n"), err ? err->message : ""); @@ -1976,7 +2036,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (qemuSetupCgroup(conn, driver, vm) < 0) goto cleanup; - if (qemuPrepareHostDevices(conn, vm->def) < 0) + if (qemuPrepareHostDevices(conn, driver, vm->def) < 0) goto cleanup; if (VIR_ALLOC(vm->monitor_chr) < 0) { @@ -2158,7 +2218,7 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, VIR_WARN("Failed to restore all device ownership for %s", vm->def->name); - qemuDomainReAttachHostDevices(conn, vm->def); + qemuDomainReAttachHostDevices(conn, driver, vm->def); retry: if ((ret = qemuRemoveCgroup(conn, driver, vm)) < 0) { @@ -5279,6 +5339,7 @@ cleanup: } static int qemudDomainAttachHostPciDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -5301,42 +5362,42 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1; if ((hostdev->managed && pciDettachDevice(conn, pci) < 0) || - pciResetDevice(conn, pci) < 0) { + pciResetDevice(conn, pci, driver->activePciHostdevs) < 0) { + pciFreeDevice(conn, pci); + return -1; + } + + if (pciDeviceListAdd(conn, driver->activePciHostdevs, pci) < 0) { pciFreeDevice(conn, pci); return -1; } - pciFreeDevice(conn, pci); + cmd = reply = NULL; if (virAsprintf(&cmd, "pci_add pci_addr=auto host host=%.2x:%.2x.%.1x", hostdev->source.subsys.u.pci.bus, hostdev->source.subsys.u.pci.slot, hostdev->source.subsys.u.pci.function) < 0) { virReportOOMError(conn); - return -1; + goto error; } if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("cannot attach host pci device")); - VIR_FREE(cmd); - return -1; + goto error; } if (strstr(reply, "invalid type: host")) { qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("PCI device assignment is not supported by this version of qemu")); - VIR_FREE(cmd); - VIR_FREE(reply); - return -1; + goto error; } if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("parsing pci_add reply failed: %s"), reply); - VIR_FREE(cmd); - VIR_FREE(reply); - return -1; + goto error; } hostdev->source.subsys.u.pci.guest_addr.domain = domain; @@ -5349,6 +5410,14 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, VIR_FREE(cmd); return 0; + +error: + pciDeviceListDel(conn, driver->activePciHostdevs, pci); + + VIR_FREE(reply); + VIR_FREE(cmd); + + return -1; } static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, @@ -5422,7 +5491,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - return qemudDomainAttachHostPciDevice(conn, vm, dev); + return qemudDomainAttachHostPciDevice(conn, driver, vm, dev); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: return qemudDomainAttachHostUsbDevice(conn, vm, dev); default: @@ -5749,6 +5818,7 @@ cleanup: } static int qemudDomainDetachHostPciDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -5832,7 +5902,8 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn, if (!pci) ret = -1; else { - if (pciResetDevice(conn, pci) < 0) + pciDeviceListDel(conn, driver->activePciHostdevs, pci); + if (pciResetDevice(conn, pci, driver->activePciHostdevs) < 0) ret = -1; if (detach->managed && pciReAttachDevice(conn, pci) < 0) ret = -1; @@ -5868,7 +5939,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn, switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - ret = qemudDomainDetachHostPciDevice(conn, vm, dev); + ret = qemudDomainDetachHostPciDevice(conn, driver, vm, dev); break; default: qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, @@ -7092,6 +7163,7 @@ out: static int qemudNodeDeviceReset (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -7103,11 +7175,14 @@ qemudNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1; - if (pciResetDevice(dev->conn, pci) < 0) + qemuDriverLock(driver); + + if (pciResetDevice(dev->conn, pci, driver->activePciHostdevs) < 0) goto out; ret = 0; out: + qemuDriverUnlock(driver); pciFreeDevice(dev->conn, pci); return ret; } diff --git a/src/xen_unified.c b/src/xen_unified.c index f2ffc25..dfa9ca5 100644 --- a/src/xen_unified.c +++ b/src/xen_unified.c @@ -1641,7 +1641,7 @@ xenUnifiedNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1; - if (pciResetDevice(dev->conn, pci) < 0) + if (pciResetDevice(dev->conn, pci, NULL) < 0) goto out; ret = 0;