Re: [libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices
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(a)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