Allow pciResetDevice() to reset multiple devices

When using a Secondary Bus Reset, all devices on the bus are reset.

Extend the pciResetDevice() API so that a 'check' callback can be
supplied which will verify that it is safe to reset the other devices
on the bus.

The virDomainObjPtr parameter is needed so that when the check function
iterates over the domain list, it can avoid double locking.

* src/pci.[ch]: add a 'check' callback to pciResetDevice(), re-work
  pciIterDevices() to pass the check function to the iter functions,
  use the check function in the bus iterator, return the first unsafe
  device from pciBusCheckOtherDevices() and include its details in
  the bus reset error message.

* src/qemu_driver.c, src/xen_uninified.c: just pass NULL as the
  check function for now

diff --git a/src/pci.c b/src/pci.c
index 74f7ef071fe9fee1c7bd8c52859b52e16024f6e4..6a2e860007e46e8891046f173ebfb34aac4b23eb 100644 (file)
--- a/src/pci.c
+++ b/src/pci.c
@@ -225,7 +225,11 @@ pciWrite32(pciDevice *dev, unsigned pos, uint32_t val)
     pciWrite(dev, pos, &buf[0], sizeof(buf));
 }
 
-typedef int (*pciIterPredicate)(pciDevice *, pciDevice *);
+typedef int (*pciIterPredicate)(virConnectPtr,
+                                virDomainObjPtr,
+                                pciDevice *,
+                                pciResetCheckFunc,
+                                pciDevice *);
 
 /* Iterate over available PCI devices calling @predicate
  * to compare each one to @dev.
@@ -234,8 +238,10 @@ typedef int (*pciIterPredicate)(pciDevice *, pciDevice *);
  */
 static int
 pciIterDevices(virConnectPtr conn,
+               virDomainObjPtr vm,
                pciIterPredicate predicate,
                pciDevice *dev,
+               pciResetCheckFunc check,
                pciDevice **matched)
 {
     DIR *dir;
@@ -254,7 +260,7 @@ pciIterDevices(virConnectPtr conn,
 
     while ((entry = readdir(dir))) {
         unsigned domain, bus, slot, function;
-        pciDevice *try;
+        pciDevice *check_dev;
 
         /* Ignore '.' and '..' */
         if (entry->d_name[0] == '.')
@@ -266,18 +272,18 @@ pciIterDevices(virConnectPtr conn,
             continue;
         }
 
-        try = pciGetDevice(conn, domain, bus, slot, function);
-        if (!try) {
+        check_dev = pciGetDevice(conn, domain, bus, slot, function);
+        if (!check_dev) {
             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(conn, vm, dev, check, check_dev)) {
+            VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check_dev->name);
+            *matched = check_dev;
             break;
         }
-        pciFreeDevice(conn, try);
+        pciFreeDevice(conn, check_dev);
     }
     closedir(dir);
     return ret;
@@ -379,63 +385,73 @@ pciDetectPowerManagementReset(pciDevice *dev)
     return 0;
 }
 
-/* Any devices other than the one supplied on the same domain/bus ? */
+/* Check any devices other than the one supplied on the same domain/bus */
 static int
-pciSharesBus(pciDevice *a, pciDevice *b)
+pciCheckSharesBus(virConnectPtr conn,
+                  virDomainObjPtr vm,
+                  pciDevice *dev,
+                  pciResetCheckFunc check,
+                  pciDevice *check_dev)
 {
-    return
-        a->domain == b->domain &&
-        a->bus == b->bus &&
-        (a->slot != b->slot ||
-         a->function != b->function);
+    if (check_dev->domain != dev->domain || check_dev->bus != dev->bus)
+        return 0;
+    if (check_dev->slot == dev->slot && check_dev->function == dev->function)
+        return 0;
+    if (check(conn, vm, check_dev))
+        return 0;
+    return 1;
 }
 
-static int
-pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev)
+static pciDevice *
+pciBusCheckOtherDevices(virConnectPtr conn,
+                        virDomainObjPtr vm,
+                        pciDevice *dev,
+                        pciResetCheckFunc check)
 {
-    pciDevice *matched = NULL;
-    if (pciIterDevices(conn, pciSharesBus, dev, &matched) < 0)
-        return 1;
-    if (!matched)
-        return 0;
-    pciFreeDevice(conn, matched);
-    return 1;
+    pciDevice *conflict = NULL;
+    pciIterDevices(conn, vm, pciCheckSharesBus, dev, check, &conflict);
+    return conflict;
 }
 
-/* Is @a the parent of @b ? */
+/* Is @check_dev the parent of @dev ? */
 static int
-pciIsParent(pciDevice *a, pciDevice *b)
+pciIsParent(virConnectPtr conn ATTRIBUTE_UNUSED,
+            virDomainObjPtr vm ATTRIBUTE_UNUSED,
+            pciDevice *dev,
+            pciResetCheckFunc check ATTRIBUTE_UNUSED,
+            pciDevice *check_dev)
 {
     uint16_t device_class;
     uint8_t header_type, secondary, subordinate;
 
-    if (a->domain != b->domain)
+    if (check_dev->domain != dev->domain)
         return 0;
 
     /* Is it a bridge? */
-    device_class = pciRead16(a, PCI_CLASS_DEVICE);
+    device_class = pciRead16(check_dev, 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_dev, 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_dev, PCI_SECONDARY_BUS);
+    subordinate = pciRead8(check_dev, 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_dev->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, NULL, pciIsParent, dev, NULL, &parent);
     return parent;
 }
 
@@ -443,9 +459,12 @@ pciGetParentDevice(virConnectPtr conn, pciDevice *dev)
  * devices behind a bus.
  */
 static int
-pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev)
+pciTrySecondaryBusReset(virConnectPtr conn,
+                        virDomainObjPtr vm,
+                        pciDevice *dev,
+                        pciResetCheckFunc check)
 {
-    pciDevice *parent;
+    pciDevice *parent, *conflict;
     uint8_t config_space[PCI_CONF_LEN];
     uint16_t ctl;
     int ret = -1;
@@ -455,10 +474,11 @@ 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 = pciBusCheckOtherDevices(conn, vm, dev, check))) {
         pciReportError(conn, VIR_ERR_NO_SUPPORT,
-                       _("Other devices on bus with %s, not doing bus reset"),
-                       dev->name);
+                       _("Unable to reset %s using bus reset as this would cause %s to be reset"),
+                       dev->name, conflict->name);
+        pciFreeDevice(conn, conflict);
         return -1;
     }
 
@@ -572,13 +592,24 @@ pciInitDevice(virConnectPtr conn, pciDevice *dev)
 }
 
 int
-pciResetDevice(virConnectPtr conn, pciDevice *dev)
+pciResetDevice(virConnectPtr conn,
+               virDomainObjPtr vm,
+               pciDevice *dev,
+               pciResetCheckFunc check)
 {
     int ret = -1;
 
     if (!dev->initted && pciInitDevice(conn, dev) < 0)
         return -1;
 
+    /* Check that the device isn't owned by a running VM */
+    if (!check(conn, vm, dev)) {
+        pciReportError(conn, VIR_ERR_NO_SUPPORT,
+                       _("Unable to reset PCI device %s: device is in use"),
+                       dev->name);
+        return -1;
+    }
+
     /* KVM will perform FLR when starting and stopping
      * a guest, so there is no need for us to do it here.
      */
@@ -594,7 +625,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, vm, dev, check);
 
     if (ret < 0) {
         virErrorPtr err = virGetLastError();

diff --git a/src/pci.h b/src/pci.h
index 47882efb4d21f8d48a2e160ef69fbd2867229a5f..15da057f167471a7659fb4ce9d48a4fe59516452 100644 (file)
--- a/src/pci.h
+++ b/src/pci.h
@@ -24,6 +24,7 @@
 
 #include <config.h>
 #include "internal.h"
+#include "domain_conf.h"
 
 typedef struct _pciDevice pciDevice;
 
@@ -38,7 +39,17 @@ int        pciDettachDevice  (virConnectPtr  conn,
                               pciDevice     *dev);
 int        pciReAttachDevice (virConnectPtr  conn,
                               pciDevice     *dev);
-int        pciResetDevice    (virConnectPtr  conn,
-                              pciDevice     *dev);
+
+/* pciResetDevice() may cause other devices to be reset;
+ * The check function is called for each other device to
+ * be reset and by returning zero may cause the reset to
+ * fail if it is not safe to reset the supplied device.
+ */
+typedef int (*pciResetCheckFunc)(virConnectPtr, virDomainObjPtr, pciDevice *);
+
+int pciResetDevice(virConnectPtr      conn,
+                   virDomainObjPtr    vm,
+                   pciDevice         *dev,
+                   pciResetCheckFunc  check);
 
 #endif /* __VIR_PCI_H__ */

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 6d1ec0663065071f98993268dd0d4c2f51f6914f..bfa06a55d0408717433ecb94e7f3716a17918306 100644 (file)
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1329,8 +1329,10 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
     return -1;
 }
 
-static int qemuPrepareHostDevices(virConnectPtr conn,
-                                  virDomainDefPtr def) {
+static int
+qemuPrepareHostDevices(virConnectPtr conn, virDomainObjPtr vm)
+{
+    virDomainDefPtr def = vm->def;
     int i;
 
     /* We have to use 2 loops here. *All* devices must
@@ -1388,7 +1390,7 @@ static int qemuPrepareHostDevices(virConnectPtr conn,
         if (!dev)
             goto error;
 
-        if (pciResetDevice(conn, dev) < 0) {
+        if (pciResetDevice(conn, vm, dev, NULL) < 0) {
             pciFreeDevice(conn, dev);
             goto error;
         }
@@ -1403,8 +1405,9 @@ error:
 }
 
 static void
-qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def)
+qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainObjPtr vm)
 {
+    virDomainDefPtr def = vm->def;
     int i;
 
     /* Again 2 loops; reset all the devices before re-attach */
@@ -1431,7 +1434,7 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def)
             continue;
         }
 
-        if (pciResetDevice(conn, dev) < 0) {
+        if (pciResetDevice(conn, vm, dev, NULL) < 0) {
             virErrorPtr err = virGetLastError();
             VIR_ERROR(_("Failed to reset PCI device: %s\n"),
                       err ? err->message : "");
@@ -2001,7 +2004,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
     if (qemuSetupCgroup(conn, driver, vm) < 0)
         goto cleanup;
 
-    if (qemuPrepareHostDevices(conn, vm->def) < 0)
+    if (qemuPrepareHostDevices(conn, vm) < 0)
         goto cleanup;
 
     if (VIR_ALLOC(vm->monitor_chr) < 0) {
@@ -2183,7 +2186,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, vm);
 
 retry:
     if ((ret = qemuRemoveCgroup(conn, driver, vm)) < 0) {
@@ -5247,7 +5250,7 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn,
             return -1;
 
         if (pciDettachDevice(conn, pci) < 0 ||
-            pciResetDevice(conn, pci) < 0) {
+            pciResetDevice(conn, vm, pci, NULL) < 0) {
             pciFreeDevice(conn, pci);
             return -1;
         }
@@ -7049,7 +7052,7 @@ qemudNodeDeviceReset (virNodeDevicePtr dev)
     if (!pci)
         return -1;
 
-    if (pciResetDevice(dev->conn, pci) < 0)
+    if (pciResetDevice(dev->conn, NULL, pci, NULL) < 0)
         goto out;
 
     ret = 0;

diff --git a/src/xen_unified.c b/src/xen_unified.c
index f2ffc2512d80eaf9828c273605cf33cf42eb06ba..dd8f10b161cf808d15b20f7c5dcef6294c3e2e58 100644 (file)
--- 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, NULL, pci, NULL) < 0)
         goto out;
 
     ret = 0;