[libvirt] [PATCH v4 03/10] Refuse to reattach from vfio if the iommu group is in use by any domain

[Laine Stump]

On 11/14/2015 03:36 AM, Shivaprasad G Bhat wrote:

 > Refuse to reattach from vfio if the iommu group is in use by any domain

util: refuse to reattach device if any device in the same group is in 
use by any domain

> It is incorrect to attempt the device reattach of a function,

s/function/device/

> when some other domain is using some functions belonging to the same iommu
> group.

s/some other domain/any other domain/
s/some functions/any device/

>
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
>   src/util/virhostdev.c |   21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index de029a0..f24ccd8 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -1590,18 +1590,35 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
>                                   virPCIDevicePtr pci)
>   {
>       virPCIDeviceAddressPtr devAddr = NULL;
> +    bool usesVfio = false;
> +    char *drvPath = NULL;
> +    char *drvName = NULL;
>       struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
>                                                        false};
>       int ret = -1;
>   
> +    if (virPCIDeviceGetDriverPathAndName(pci, &drvPath, &drvName) < 0)
> +        goto out;
> +
> +    if (STREQ_NULLABLE(drvName, "vfio-pci"))
> +        usesVfio = true;
> +
>       virObjectLock(hostdev_mgr->activePCIHostdevs);
>       virObjectLock(hostdev_mgr->inactivePCIHostdevs);
>   
>       if (!(devAddr = virPCIDeviceGetAddress(pci)))
>           goto out;
>   
> -    if (virHostdevIsPCINodeDeviceUsed(devAddr, &data))
> +    if (usesVfio) {
> +    /* Doesn't matter which device. If any domain is actively using the
> +     * iommu group, refuse to reattach */
> +        if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
> +                                                 virHostdevIsPCINodeDeviceUsed,
> +                                                 &data) < 0)
> +            goto out;

Calling the iterator seems very inefficient to me (which is why I 
suggested in the original BZ comments that we save the iommuGroup of 
each device to the activelist):

1) you know the iommu group of the current PCI device (it is in 
pci->iommuGroup)

2) all that virHostdevIsPCINodeDeviceUsed does is check through 
activePCIHostdevs for the device, and this iterator is just calling 
virHostdevIsPCINodeDeviceUsed for each device in the same iommuGroup as 
the current device.

Therefore, all you really need to do is look for any devices in 
acticePCIHostdevs that have dev->iommuGroup == pci->iommuGroup.

Calling the iterator results in a lot of accesses to sysfs, opening and 
closing files, printfs, etc. Unless that is necessary to catch some 
extra case (and I don't see that), it's much better to do it the simpler 
way - more efficient and easier to understand a year from now when we 
come back to this.

> +    } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) {
>           goto out;
> +    }
>   
>       virPCIDeviceReattachInit(pci);
>   
> @@ -1614,6 +1631,8 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
>       virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
>       virObjectUnlock(hostdev_mgr->activePCIHostdevs);
>       VIR_FREE(devAddr);
> +    VIR_FREE(drvPath);
> +    VIR_FREE(drvName);
>       return ret;
>   }
>   
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>