[libvirt] [PATCH v2 2/3] virhostdev: remove virHostdevReattachPCIDevice

[Daniel Henrique Barboza]

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 at gmail.com>
---
 src/util/virhostdev.c | 40 ++++++++++------------------------------
 src/util/virpci.c     | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 4dd24a8f65..31d075a11a 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -927,33 +927,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
  */
@@ -1072,12 +1045,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 6dc0a2711c..05232888c3 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1509,6 +1509,10 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
     return 0;
 }
 
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
 int
 virPCIDeviceReattach(virPCIDevicePtr dev,
                      virPCIDeviceListPtr activeDevs,
@@ -1520,6 +1524,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;
 
-- 
2.21.0