Re: [PATCH v2 2/7] tests: pci: Mock the iommu groups and vfio

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 7/25/19 8:09 PM, Daniel Henrique Barboza wrote:
> From: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
>
> The iommu group, /dev/vfio/<iommuGroupNumber> behaviors
> of the host are mocked. This patch implements support for
> multifunction/multiple devices per iommu groups and emulates
> the /dev/vfio/<iommuGroupNumber> file correctly.
>
> This code helps adding necessary testcases for pci-hotplug
> code.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
> ---
>   tests/virpcimock.c | 177 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 162 insertions(+), 15 deletions(-)

Looks good. We will have some merge conflicts because we fix the same issue.

> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index 5ec9abe05f..1b44ed0a44 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -44,6 +44,8 @@ char *fakerootdir;
>   char *fakesysfspcidir;
>   
>   # define SYSFS_PCI_PREFIX "/sys/bus/pci/"
> +# define SYSFS_KERNEL_PREFIX "/sys/kernel/"
> +
>   
>   # define STDERR(...) \
>       fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__); \
> @@ -123,6 +125,11 @@ struct pciDevice {
>       struct pciDriver *driver;   /* Driver attached. NULL if attached to no driver */
>   };
>   
> +struct pciIommuGroup {
> +    int iommu;
> +    size_t nDevicesBoundToVFIO;        /* Indicates the devices in the group */
> +};
> +
>   struct fdCallback {
>       int fd;
>       char *path;
> @@ -134,6 +141,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;
>   
> @@ -247,6 +257,13 @@ getrealpath(char **newpath,
>               errno = ENOMEM;
>               return -1;
>           }
> +    } else if (STRPREFIX(path, SYSFS_KERNEL_PREFIX)) {
> +        if (virAsprintfQuiet(newpath, "%s/%s",
> +                             fakerootdir,
> +                             path) < 0) {
> +            errno = ENOMEM;
> +            return -1;
> +        }
>       } else {
>           if (VIR_STRDUP_QUIET(*newpath, path) < 0)
>               return -1;
> @@ -460,6 +477,101 @@ pci_device_autobind(struct pciDevice *dev)
>       return pci_driver_bind(driver, dev);
>   }
>   
> +static void
> +pci_iommu_new(int num)
> +{
> +    char *iommupath, *kerneldir;
> +    struct pciIommuGroup *iommuGroup;
> +
> +    if (VIR_ALLOC_QUIET(iommuGroup) < 0)
> +        ABORT_OOM();
> +
> +    iommuGroup->iommu = num;
> +    iommuGroup->nDevicesBoundToVFIO = 0; /* No device bound to vfio by default */
> +
> +    if (virAsprintfQuiet(&kerneldir, "%s%s",
> +                         fakerootdir, SYSFS_KERNEL_PREFIX) < 0)
> +        ABORT_OOM();
> +
> +    if (virAsprintfQuiet(&iommupath, "%s/iommu_groups/%d/devices", kerneldir, num) < 0)
> +        ABORT_OOM();
> +    VIR_FREE(kerneldir);
> +
> +    if (virFileMakePath(iommupath) < 0)
> +        ABORT("Unable to create: %s", iommupath);
> +    VIR_FREE(iommupath);
> +
> +    if (VIR_APPEND_ELEMENT_QUIET(pciIommuGroups, npciIommuGroups, iommuGroup) < 0)
> +        ABORT_OOM();
> +}
> +
> +static int
> +pci_vfio_release_iommu(struct pciDevice *device)
> +{
> +    char *vfiopath = NULL;
> +    int ret = -1;
> +    size_t i = 0;
> +
> +    for (i = 0; i < npciIommuGroups; i++) {
> +        if (pciIommuGroups[i]->iommu == device->iommuGroup)
> +            break;
> +    }
> +
> +    if (i != npciIommuGroups) {
> +        if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0) {
> +            ret = 0;
> +            goto cleanup;
> +        }
> +        pciIommuGroups[i]->nDevicesBoundToVFIO--;
> +        if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> +            if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> +                         fakesysfspcidir, device->iommuGroup) < 0) {
> +                errno = ENOMEM;
> +                goto cleanup;
> +            }
> +            if (unlink(vfiopath) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(vfiopath);
> +    return ret;
> +}
> +
> +static int
> +pci_vfio_lock_iommu(struct pciDevice *device)
> +{
> +    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)
> +            break;
> +    }
> +
> +    if (i != npciIommuGroups) {
> +        if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> +            if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> +                         fakesysfspcidir, device->iommuGroup) < 0) {
> +                errno = ENOMEM;
> +                goto cleanup;
> +            }
> +            if ((fd = real_open(vfiopath, O_CREAT)) < 0)
> +                goto cleanup;
> +        }
> +        pciIommuGroups[i]->nDevicesBoundToVFIO++;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    real_close(fd);
> +    VIR_FREE(vfiopath);
> +    return ret;
> +}
>   
>   /*
>    * PCI Driver functions
> @@ -583,6 +695,10 @@ pci_driver_bind(struct pciDriver *driver,
>       if (symlink(devpath, driverpath) < 0)
>           goto cleanup;
>   
> +    if (STREQ(driver->name, "vfio-pci"))
> +        if (pci_vfio_lock_iommu(dev) < 0)
> +            goto cleanup;
> +
>       dev->driver = driver;
>       ret = 0;
>    cleanup:
> @@ -617,6 +733,10 @@ pci_driver_unbind(struct pciDriver *driver,
>           unlink(driverpath) < 0)
>           goto cleanup;
>   
> +    if (STREQ(driver->name, "vfio-pci"))
> +        if (pci_vfio_release_iommu(dev) < 0)
> +            goto cleanup;
> +
>       dev->driver = NULL;
>       ret = 0;
>    cleanup:
> @@ -813,6 +933,8 @@ init_syms(void)
>   static void
>   init_env(void)
>   {
> +    char *devVFIO;
> +
>       if (fakerootdir && fakesysfspcidir)
>           return;
>   
> @@ -828,6 +950,29 @@ init_env(void)
>   
>       make_file(fakesysfspcidir, "drivers_probe", NULL, -1);
>   
> +    if (virAsprintfQuiet(&devVFIO, "%s/dev/vfio", fakesysfspcidir) < 0)
> +        ABORT_OOM();
> +
> +    if (virFileMakePath(devVFIO) < 0)
> +        ABORT("Unable to create: %s", devVFIO);
> +
> +    /* Create /dev/vfio/vfio file */
> +    make_file(devVFIO, "vfio", NULL, -1);
> +    VIR_FREE(devVFIO);
> +
> +    pci_iommu_new(0);
> +    pci_iommu_new(1);
> +    pci_iommu_new(2);
> +    pci_iommu_new(3);
> +    pci_iommu_new(4);
> +    pci_iommu_new(5);
> +    pci_iommu_new(6);
> +    pci_iommu_new(7);
> +    pci_iommu_new(8);
> +    pci_iommu_new(9);
> +    pci_iommu_new(10);
> +    pci_iommu_new(11);

I wonder if we can make these somehow dyamic. E.g. called from 
pci_device_new_from_stub() and require iommuGroup in MAKE_PCI_DEVICE() 
because every device belongs to an IOMMU group.


Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list