[libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

[Daniel Henrique Barboza]

Michal, I believe the problem you're trying to fix in this patch
is somehow the same I'm trying to fix in this patch here:


https://www.redhat.com/archives/libvir-list/2019-June/msg01270.html

Do you mind taking a look? If that's the case, I can drop that patch
from the series that implements Multifunction PCI hotplug.


Thanks,

DHB


On 8/14/19 8:57 AM, Michal Privoznik wrote:
> The virHostdevPreparePCIDevices() function works in several
> steps. In the very first one, it checks if devices we want to
> detach from the host are not taken already by some other domain.
> However, this piece of code returns different results depending
> on the stub driver used (which is not wrong per se, but keep on
> reading). If the stub driver is KVM then
> virHostdevIsPCINodeDeviceUsed() is called which basically checks
> if a PCI device from the detach list is not used by any domain
> (including the one we are preparing the device for). If that is
> the case, an error is reported ("device in use") and -1 is
> returned.
>
> However, that is not what happens if the stub driver is VFIO. If
> the stub driver is VFIO, then we iterate over all PCI devices
> from the same IOMMU group and check if they are taken by some
> other domain (because a PCI device, well IOMMU group, can't be
> shared between two or more qemu processes). But we fail to check,
> if the device we are trying to detach from the host is not
> already taken by a domain. That is, calling
> virHostdevPreparePCIDevices() over a hostdev device twice
> succeeds the first time and fails too late in the second run
> (fortunately, virHostdevResetAllPCIDevices() will throw an error,
> but this is already too late because the PCI device in question
> was moved to the list of inactive PCI devices and now it appears
> in both lists).
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>   src/util/virhostdev.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index cb69582c21..6861b8a84e 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
>       virHostdevManagerPtr mgr;
>       const char *driverName;
>       const char *domainName;
> -    const bool usesVFIO;
> +    bool usesVFIO;
>   };
>   
>   /* This module makes heavy use of bookkeeping lists contained inside a
> @@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>           virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>           bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
>           bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == VIR_PCI_STUB_DRIVER_VFIO);
> -        struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, dom_name, usesVFIO};
> +        struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, dom_name, false};
>           int hdrType = -1;
>   
>           if (virPCIGetHeaderType(pci, &hdrType) < 0)
> @@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>           }
>   
>           /* The device is in use by other active domain if
> -         * the dev is in list activePCIHostdevs. VFIO devices
> -         * belonging to same iommu group can't be shared
> -         * across guests.
> -         */
> +         * the dev is in list activePCIHostdevs. */
>           devAddr = virPCIDeviceGetAddress(pci);
> +        if (virHostdevIsPCINodeDeviceUsed(devAddr, &data))
> +            goto cleanup;
> +
> +        /* VFIO devices belonging to same IOMMU group can't be
> +         * shared across guests. Check if that's the case. */
>           if (usesVFIO) {
> +            data.usesVFIO = true;
>               if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
>                                                        virHostdevIsPCINodeDeviceUsed,
>                                                        &data) < 0)
>                   goto cleanup;
> -        } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) {
> -            goto cleanup;
>           }
>       }
>   

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190814/62b6c068/attachment-0001.htm>