On 7/19/19 8:52 AM, Michal Privoznik wrote:
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 <danielhb413@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?
Yep, I'll probably merge this up with patch 3 in a re-spin.
+ +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