virhostdev: remove virHostdevReattachPCIDevice

virHostdevReattachPCIDevice() is a static that simply does
a wait loop with virPCIDeviceWaitForCleanup() before
calling virPCIDeviceReattach().

This loop traces back to commit d1e5676c0d, aiming to
solve a race condition between Libvirt returning the
device back to the host and QEMU trying to access it in
the meantime, which resulted in QEMU exiting on error
and killing the guest. This happens because device_del
is asynchronous, returning OK even if the guest didn't
release the device. Commit 01abc8a1b8 moved this code
to qemu_hostdev.c, 82e8dd4cf8 added the pci-stub conditional
for the loop, 899b261127 moved the code to virhostdev.c
where it stood until now.

The intent of this wait loop is still valid: device_del
is still not bullet proof into preventing the conditions
that commit d1e5676c0d aimed to fix, especially when considering
all the architectures we must support. However, this loop
is executed only in virHostdevReattachPCIDevice(), leaving
every other virPCIDeviceReattach() call prone to that error.

Let's move the wait loop code to virPCIDeviceReattach(). This
will:

-  make every reattach call safe from this race condition
with the pci-stub;

-  allow for a bit of code cleanup (virHostdevReattachPCIDevice()
can be erased, and virHostdevReAttachPCIDevices() can use
virPCIDeviceReattach() directly);

- make it easier to understand the overall reattach mechanisms in
Libvirt, without the risk of a newcomer wondering why reattach
is done slightly different in some instances.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 7247b73cf2fd544b1e4c70211e5a19755d4bd106..162e1c2ff85cc802e1d1d3c27cbe4efa7dda5724 100644 (file)
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -926,33 +926,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
     return ret;
 }
 
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
-                            virPCIDevicePtr actual)
-{
-    /* Wait for device cleanup if it is qemu/kvm */
-    if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
-        int retries = 100;
-        while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
-               && retries) {
-            usleep(100*1000);
-            retries--;
-        }
-    }
-
-    VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
-    if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
-                             mgr->inactivePCIHostdevs) < 0) {
-        VIR_ERROR(_("Failed to re-attach PCI device: %s"),
-                  virGetLastErrorMessage());
-        virResetLastError();
-    }
-}
-
 /* @oldStateDir:
  * For upgrade purpose: see virHostdevRestoreNetConfig
  */
@@ -1071,12 +1044,19 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
         virPCIDevicePtr actual;
 
         /* We need to look up the actual device because that's what
-         * virHostdevReattachPCIDevice() expects as its argument */
+         * virPCIDeviceReattach() expects as its argument */
         if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
             continue;
 
-        if (virPCIDeviceGetManaged(actual))
-            virHostdevReattachPCIDevice(mgr, actual);
+        if (virPCIDeviceGetManaged(actual)) {
+            if (virPCIDeviceReattach(actual,
+                                     mgr->activePCIHostdevs,
+                                     mgr->inactivePCIHostdevs) < 0) {
+                VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+                          virGetLastErrorMessage());
+                virResetLastError();
+            }
+        }
         else
             VIR_DEBUG("Not reattaching unmanaged PCI device %s",
                       virPCIDeviceGetName(actual));

diff --git a/src/util/virpci.c b/src/util/virpci.c
index bc7ff461947e6f9315b6e674a3527c52d4dbcbd1..8deaeb639b27202f43a6150bf1ea035f332a6cae 100644 (file)
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1508,6 +1508,10 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
     return 0;
 }
 
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
 int
 virPCIDeviceReattach(virPCIDevicePtr dev,
                      virPCIDeviceListPtr activeDevs,
@@ -1519,6 +1523,16 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
         return -1;
     }
 
+    /* Wait for device cleanup if it is qemu/kvm */
+    if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
+        int retries = 100;
+        while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
+               && retries) {
+            usleep(100*1000);
+            retries--;
+        }
+    }
+
     if (virPCIDeviceUnbindFromStub(dev) < 0)
         return -1;