[libvirt] [PATCH 7/8] Postpone reprobing till all the devices in iommu group are unbound from vfio

[Andrea Bolognani]

I know you're already working on a v2 of this, just a
couple of quick remarks below.

On Fri, 2015-10-30 at 05:01 +0530, Shivaprasad G Bhat wrote:
> Author: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> 
> The host will crash if a device is bound to host driver when the device
> belonging to same iommu group is in use by any of the guests. So,
> do the host driver probe only after all the devices in the iommu group
> have unbound from the vfio.
> 
> The patch fixes
> https://bugzilla.redhat.com/show_bug.cgi?id=1272300
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
>  src/util/virpci.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 6c24a81..425c1a7 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1128,6 +1128,23 @@ static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char
>      return result;
>  }
>  
> +static int virPCIDeviceBoundToVFIODriver(virPCIDeviceAddressPtr devAddr, void *opaque ATTRIBUTE_UNUSED)

Return type on a separate line, please.

> +{
> +    int ret = 0;
> +    virPCIDevicePtr pci = NULL;
> +
> +    if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus,
> +                                devAddr->slot, devAddr->function)))
> +        goto cleanup;
> +
> +    if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci"))
> +        ret = -1;

As mentioned in the comment for Patch 1, I think this is
fairly obscure: if I had not read throught the whole
series, I'd assume this checks whether the device is
configured to use vfio-pci as stub driver, not whether
it is currently bound to it, and it would not be
immediately clear to me how this check fits in a function
called virPCIDeviceBoundToVFIODriver().

I think it would be much cleaner to query the driver
explicitly using virPCIDeviceGetDriverPathAndName() and
remove that call from virPCIDeviceNew().

> +
> + cleanup:
> +    virPCIDeviceFree(pci);
> +    return ret;
> +}
> +
>  static int
>  virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
>                             virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED,
> @@ -1177,11 +1194,34 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
>      dev->remove_slot = false;
>  
>   reprobe:
> -    if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> -        goto cleanup;
> -    /* Steal the dev from list inactiveDevs */
> -    if (inactiveDevs)
> -        virPCIDeviceListDel(inactiveDevs, dev);
> +    if (STREQ_NULLABLE(dev->stubDriver, "vfio-pci")) {
> +        size_t i = 0;
> +        virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
> +                                        dev->slot, dev->function };
> +        if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, virPCIDeviceBoundToVFIODriver, NULL) < 0) {
> +            result = 0;
> +            goto cleanup;
> +        }
> +
> +        while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) {
> +            virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i);
> +            if (dev->iommuGroup == pcidev->iommuGroup) {
> +                if (virPCIDeviceReprobeHostDriver(pcidev, driver, drvdir) < 0)
> +                   goto cleanup;
> +                /* Steal the dev from list inactiveDevs */
> +                virPCIDeviceListDel(inactiveDevs, pcidev);
> +                continue;
> +            }
> +            i++;
> +        }
> +    } else {
> +        if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> +            goto cleanup;
> +        /* Steal the dev from list inactiveDevs */
> +
> +        if (inactiveDevs)
> +            virPCIDeviceListDel(inactiveDevs, dev);

Either put the empty line before the comment or just
get rid of it.

> +    }
>  
>      result = 0;

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team