[libvirt] [PATCH 8/8] Wait for iommmu device to go away before reprobing the host driver

[Andrea Bolognani]

On Fri, 2015-10-30 at 05:04 +0530, Shivaprasad G Bhat wrote:
> Author: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>

This line is redundant since the information is
already stored in git, plus you have the Signed-off-by
below. This applies to Patch 7 as well.

> 
> There could be a delay of 1 or 2 seconds before the vfio-pci driver
> is unbound and the device file /dev/vfio/<iommu> is actually
> removed. If the file exists, the host driver probing the device
> can lead to crash. So, wait and avoid the crash. Setting the
> timeout to 15 seconds for now.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
>  src/util/virpci.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 425c1a7..6bf640d 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1097,6 +1097,42 @@ static bool virPCIIsAKnownStub(char *driver)
>      return ret;
>  }
>  
> +#define VFIO_UNBIND_TIMEOUT 15

There's one trailing space here, and git complains about
it when applying the patch. syntax-check complains too.

> +
> +/* It is not safe to initiate host driver probe if the vfio driver has not
> + * completely unbound the device.
> + * So, return if the unbind didn't complete in 15 seconds.
> + */
> +static int virPCIWaitForVfioUnbindCompletion(virPCIDevicePtr dev)

Return type on a separate line, please. VFIO is an
acronym, like PCI, and should always be all-caps.

> +{
> +    int retry = 0;
> +    int ret = -1;
> +    char *path = NULL;
> +
> +    if (!(path = virPCIDeviceGetIOMMUGroupDev(dev)))
> +        goto cleanup;
> +
> +    while (retry++ < VFIO_UNBIND_TIMEOUT) {
> +        if (!virFileExists(path))
> +            break;
> +         sleep(1);

Do we want this to be in seconds, or would a higher
granularity be better?

> +    }
> +
> +    if (virFileExists(path)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("The VFIO unbind not completed even after %d seconds for device %.4x:%.2x:%.2x.%.1x"),
> +                       retry, dev->domain, dev->bus, dev->slot, dev->function);

This line is very long, please split the error message into
two parts. I'd say "The" and "even" can be dropped. You should
use dev->name instead of formatting the name yourself.

> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup :

Please remove the space between the label and the semicolon,
and add one in front of the label. I'd also add an empty
line before the label.

> +    VIR_FREE(path);
> +    return ret;
> +
> +}
> +
> +
>  static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char *drvdir)
>  {
>      char *path = NULL;
> @@ -1203,6 +1239,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
>              goto cleanup;
>          }
>  
> +        if (virPCIWaitForVfioUnbindCompletion(dev) < 0)
> +            goto cleanup;
> +
>          while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) {
>              virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i);
>              if (dev->iommuGroup == pcidev->iommuGroup) {

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team