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

@@ -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);
+        check = pciGetDevice(conn, domain, bus, slot, function);
-        if (!try) {
+        if (!check) {
             ret = -1;
             break;
         }
 
-        if (predicate(try, dev)) {
+        if (predicate(dev, check, data)) {
-            VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, try->name);
+            VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name);
-            *matched = try;
+            *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
+    pciDeviceList *activeDevs = data;
-        a->domain == b->domain &&
-        a->bus == b->bus &&
-        (a->slot != b->slot ||
-         a->function != b->function);
-}
 
-static int
+    if (dev->domain != check->domain ||
-pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev)
+        dev->bus != check->bus ||
-{
+        (check->slot == check->slot &&
-    pciDevice *matched = NULL;
+         check->function == check->function))
-    if (pciIterDevices(conn, pciSharesBus, dev, &matched) < 0)
-        return 1;
-    if (!matched)
         return 0;
-    pciFreeDevice(conn, matched);
+
+    if (activeDevs && !pciDeviceListFind(activeDevs, check))
+        return 0;
+
     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);
+    secondary   = pciRead8(check, PCI_SECONDARY_BUS);
-    subordinate = pciRead8(a, PCI_SUBORDINATE_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"),
+                       _("Active %s devices on bus with %s, not doing bus reset"),
-                       dev->name);
+                       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();

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);

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;
 };
 
 

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;
     }
 
-    pciFreeDevice(conn, pci);
+    if (pciDeviceListAdd(conn, driver->activePciHostdevs, pci) < 0) {
+        pciFreeDevice(conn, pci);
+        return -1;
+    }
+
+    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);
+        goto error;
-        return -1;
     }
 
     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);
+        goto error;
-        VIR_FREE(reply);
-        return -1;
     }
 
     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);
+        goto error;
-        VIR_FREE(reply);
-        return -1;
     }
 
     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;
 }

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;