[libvirt] [PATCH v3 1/5] virpcimock.c: mock /dev/vfio

[Michal Privoznik]

On 8/29/19 9:18 PM, Daniel Henrique Barboza wrote:
> This patch adds mock of the /dev/vfio path, needed for proper
> implementation of the support for multifunction/multiple devices
> per iommu groups.
> 
> To do that, the existing bind and unbind operations were adapted
> to operate with the mocked filesystem as well.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>   tests/virpcimock.c | 159 ++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 143 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index a5045ed97c..e9440e7910 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -47,6 +47,7 @@ char *fakerootdir;
>   
>   # define SYSFS_PCI_PREFIX "/sys/bus/pci/"
>   
> +
>   # define STDERR(...) \
>       fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__); \
>       fprintf(stderr, __VA_ARGS__); \
> @@ -105,6 +106,11 @@ struct pciDriver {
>       size_t len;            /* @len is used for both @vendor and @device */
>   };
>   
> +struct pciIommuGroup {
> +    int iommu;
> +    size_t nDevicesBoundToVFIO; /* Indicates the devices in the group */
> +};
> +
>   struct pciDeviceAddress {
>       unsigned int domain;
>       unsigned int bus;
> @@ -133,6 +139,9 @@ size_t nPCIDevices = 0;
>   struct pciDriver **pciDrivers = NULL;
>   size_t nPCIDrivers = 0;
>   
> +struct pciIommuGroup **pciIommuGroups = NULL;
> +size_t npciIommuGroups = 0;
> +
>   struct fdCallback *callbacks = NULL;
>   size_t nCallbacks = 0;
>   
> @@ -254,6 +263,15 @@ getrealpath(char **newpath,
>               errno = ENOMEM;
>               return -1;
>           }
> +    } else if (STRPREFIX(path, "/sys/kernel/") ||
> +               STRPREFIX(path, "/dev/vfio/")) {
> +
> +        if (virAsprintfQuiet(newpath, "%s/%s",
> +                             fakerootdir,
> +                             path) < 0) {
> +            errno = ENOMEM;
> +            return -1;
> +        }
>       } else {
>           if (VIR_STRDUP_QUIET(*newpath, path) < 0)
>               return -1;
> @@ -389,8 +407,10 @@ static void
>   pci_device_create_iommu(const struct pciDevice *dev,
>                           const char *devid)
>   {
> +    struct pciIommuGroup *iommuGroup;
>       VIR_AUTOFREE(char *) iommuPath = NULL;
>       char tmp[256];
> +    size_t i;
>   
>       if (virAsprintfQuiet(&iommuPath, "%s/sys/kernel/iommu_groups/%d/devices/",
>                            fakerootdir, dev->iommuGroup) < 0)
> @@ -406,6 +426,23 @@ pci_device_create_iommu(const struct pciDevice *dev,
>       }
>   
>       make_symlink(iommuPath, devid, tmp);
> +
> +    /* pci_device_create_iommu can be called more than one for the
> +     * same iommuGroup. Bail out here if the iommuGroup was already
> +     * created beforehand. */
> +    for (i = 0; i < npciIommuGroups; i++)
> +        if (pciIommuGroups[i]->iommu == dev->iommuGroup)
> +            return;
> +
> +    if (VIR_ALLOC_QUIET(iommuGroup) < 0)
> +        ABORT_OOM();
> +
> +    iommuGroup->iommu = dev->iommuGroup;
> +    iommuGroup->nDevicesBoundToVFIO = 0; /* No device bound to VFIO by default */
> +
> +    if (VIR_APPEND_ELEMENT_QUIET(pciIommuGroups, npciIommuGroups,
> +                                 iommuGroup) < 0)
> +        ABORT_OOM();
>   }
>   
>   
> @@ -558,6 +595,77 @@ pci_device_autobind(struct pciDevice *dev)
>       return pci_driver_bind(driver, dev);
>   }
>   
> +static int
> +pci_vfio_release_iommu(struct pciDevice *device)
> +{
> +    VIR_AUTOFREE(char *) vfiopath = NULL;
> +    int ret = -1;
> +    size_t i = 0;
> +
> +    for (i = 0; i < npciIommuGroups; i++) {
> +        if (pciIommuGroups[i]->iommu == device->iommuGroup) {

I'd use "if (!cond) continue" so that you can save one level of indend.

> +
> +            if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0) {
> +                ret = 0;
> +                goto cleanup;
> +            }
> +
> +            pciIommuGroups[i]->nDevicesBoundToVFIO--;
> +
> +            if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> +
> +                if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> +                                     fakerootdir,
> +                                     device->iommuGroup) < 0) {
> +                    errno = ENOMEM;
> +                    goto cleanup;
> +                }
> +
> +                if (unlink(vfiopath) < 0)
> +                    goto cleanup;
> +            }
> +            break;
> +        }
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    return ret;

This cleanup label looks needless to me.

> +}
> +
> +static int
> +pci_vfio_lock_iommu(struct pciDevice *device)
> +{
> +    VIR_AUTOFREE(char *) vfiopath = NULL;
> +    int ret = -1;
> +    size_t i = 0;
> +    int fd = -1;
> +
> +    for (i = 0; i < npciIommuGroups; i++) {
> +        if (pciIommuGroups[i]->iommu == device->iommuGroup) {
> +            if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> +                if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> +                                     fakerootdir,
> +                                     device->iommuGroup) < 0) {
> +                    errno = ENOMEM;
> +                    goto cleanup;
> +                }
> +                if ((fd = real_open(vfiopath, O_CREAT)) < 0)
> +                    goto cleanup;
> +
> +                pciIommuGroups[i]->nDevicesBoundToVFIO++;

No, This needs to go outside of this if() as it needs to be incremented 
every time @device falls into iommu group we're processing here.

> +            }
> +            break;
> +        }
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    real_close(fd);

close() won't work with fd == -1. It will overwrite any errno we set 
earlier.

> +    return ret;
> +}
>   
>   /*
>    * PCI Driver functions
> @@ -719,6 +827,10 @@ pci_driver_bind(struct pciDriver *driver,
>       if (symlink(devpath, driverpath) < 0)
>           return -1;
>   
> +    if (STREQ(driver->name, "vfio-pci"))
> +        if (pci_vfio_lock_iommu(dev) < 0)
> +            return -1;
> +
>       dev->driver = driver;
>       return 0;
>   }
> @@ -749,6 +861,10 @@ pci_driver_unbind(struct pciDriver *driver,
>           unlink(driverpath) < 0)
>           return -1;
>   
> +    if (STREQ(driver->name, "vfio-pci"))
> +        if (pci_vfio_release_iommu(dev) < 0)
> +            return -1;

These can be joined in one condition.

> +
>       dev->driver = NULL;
>       return 0;
>   }
> @@ -865,6 +981,15 @@ init_env(void)
>       make_dir(tmp, "drivers");
>       make_file(tmp, "drivers_probe", NULL, -1);
>   
> +    /* Create /dev/vfio/ dir and /dev/vfio/vfio file */
> +    if (virAsprintfQuiet(&tmp, "%s/dev/vfio", fakerootdir) < 0)
> +        ABORT_OOM();

At this point, before executing virAsprintf() the @tmp variable is 
allocated, so it must be freed to avoid leaking it.

Michal