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@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