[libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice

Michal Privoznik mprivozn at redhat.com
Fri Jul 19 11:52:51 UTC 2019


On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
> We have 3 pieces of code that do slightly the same thing, but
> it varies depending on where it is called:
> 
> - virPCIDeviceReattach(). This is where the actual re-attach
> work happens;
> 
> - virHostdevReattachPCIDevice(). This is a static function from
> virhostdev.c that calls virPCIDeviceReattach() after waiting
> for device cleanup for the KVM PCI stub driver;
> 
> - virHostdevPCINodeDeviceReAttach(). This function also
> calls virPCIDeviceReattach(), but setting some device properties
> beforehand.
> 
> All these extra operations that are done before virPCIDeviceReattach()
> can be moved inside the function itself,  allowing for the
> same re-attach behavior everywhere.
> 
> This patch consolidates all these pre-conditions inside the
> body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice()
> can be removed and the callers can use virPCIDeviceReattach()
> directly instead.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>   src/util/virhostdev.c | 43 +++++++++----------------------------------
>   src/util/virpci.c     | 29 ++++++++++++++++++++++++++---
>   2 files changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 53aacb59b4..23be037a39 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -613,33 +613,6 @@ 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();
> -    }
> -}
> -
>   static void
>   virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
>                                   virHostdevManagerPtr mgr)
> @@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
>           if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
>               continue;
>   
> -        if (virPCIDeviceGetManaged(actual))
> -            virHostdevReattachPCIDevice(mgr, actual);
> -        else
> +        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));
> +        }
>       }
>   }
>   
> @@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr,
>       if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
>           goto cleanup;
>   
> -    virPCIDeviceSetUnbindFromStub(pci, true);
> -    virPCIDeviceSetRemoveSlot(pci, true);
> -    virPCIDeviceSetReprobe(pci, true);
> -

The real change here, is that these lines ^^

>       if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs,
>                                mgr->inactivePCIHostdevs) < 0)
>           goto cleanup;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 75e8daadd5..4594643d3c 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
>       return 0;
>   }
>   
> +/*
> + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
> + * are locked
> + */
>   int
>   virPCIDeviceReattach(virPCIDevicePtr dev,
>                        virPCIDeviceListPtr activeDevs,
>                        virPCIDeviceListPtr inactiveDevs)
>   {
> +    int ret = -1;
> +
>       if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Not reattaching active device %s"), dev->name);
> -        return -1;
> +        goto exit;
> +    }
> +
> +    virPCIDeviceSetUnbindFromStub(dev, true);
> +    virPCIDeviceSetRemoveSlot(dev, true);
> +    virPCIDeviceSetReprobe(dev, true);

are moved here ^^^

> +
> +    /* 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--;
> +        }
>       }
>   

and the wait is done here. I'm not against it, but maybe you can explain 
more why do you think this change is needed? Did you run into any 
troubles? Also, if we set these three attributes unconditionally, what 
good is there to keep those APIs around? Also, you forgot to update 
testVirPCIDeviceReattachSingle().

>       if (virPCIDeviceUnbindFromStub(dev) < 0)
> -        return -1;
> +        goto exit;
>   
>       /* Steal the dev from list inactiveDevs */
>       if (inactiveDevs) {
> @@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
>           virPCIDeviceListDel(inactiveDevs, dev);
>       }
>   
> -    return 0;
> +    ret = 0;
> +
> + exit:

This is a useless label.

Michal




More information about the libvir-list mailing list