On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
There are two places in virhostdev that executes a re-attach operation
in all pci devices of a virPCIDeviceListPtr array:
virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The
difference is that the code inside virHostdevPreparePCIDevices uses
virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices
virHostdevReattachPCIDevice() is used. Both are called in the
same manner inside a loop.

To make the code easier to follow and to standardize it further,
a new virHostdevReattachAllPCIDevices() helper function is created,
which is now called in both places. virHostdevReattachPCIDevice()
was chosen as re-attach function because it is an improved version
of the raw virPCIDeviceReattach(), including a timer to wait for
device cleanup in case of pci_stub_kvm.

Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
---
  src/util/virhostdev.c | 114 +++++++++++++++++++-----------------------
  1 file changed, 52 insertions(+), 62 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 7cb0beb545..53aacb59b4 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,56 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
      }
  }
+/*
+ * 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();
+    }
+}

What good is there in introducing this new function if you're removing in the very next patch?

+
+static void
+virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
+                                virHostdevManagerPtr mgr)
+{
+    size_t i;
+
+    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+        virPCIDevicePtr actual;
+
+        /* We need to look up the actual device because that's what
+         * virHostdevReattachPCIDevice() expects as its argument */
+        if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
+            continue;
+
+        if (virPCIDeviceGetManaged(actual))
+            virHostdevReattachPCIDevice(mgr, actual);

Just move its internals here. I mean, even from the code you're moving we would be calling virPCIDeviceReattach().

+        else
+            VIR_DEBUG("Not reattaching unmanaged PCI device %s",
+                      virPCIDeviceGetName(actual));
+    }
+}
+

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to