Re: [libvirt] [PATCH v4 10/10] Wait for iommmu device to go away before reprobing the host driver

On 11/14/2015 03:39 AM, Shivaprasad G Bhat wrote: > > 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. If this is true, it is a kernel bug and must be fixed, not glossed over with a clunky timed loop. In a discussion on IRC, Alex has told me that by the time you return from unbinding the last device from vfio-pci (by writing the ASCII representation of the device's PCI address to its driver/unbind in sysfs), it should be safe to reprobe, and the reprobe should be successful. In other words, this wait should be unnecessary. If libvirt added fixups like this for every transient kernel bug, it would end up being a fragile unmaintainable mess understood by nobody. Instead, we should push for the kernel to have its bugs fixed.

> > This cant be tested with the mock as the delay cant be simulated. > The mock changes here are to just let the test cases pass for access() > call to check if the /dev/vfio/<iommu> exists. > > Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > --- > src/util/virpci.c | 41 +++++++++++++++++++++++++++++++++++++++++ > tests/virpcimock.c | 11 ++++++++++- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/src/util/virpci.c b/src/util/virpci.c > index 5462cd2..4765302 100644 > --- a/src/util/virpci.c > +++ b/src/util/virpci.c > @@ -1098,6 +1098,43 @@ virPCIIsKnownStub(char *driver) > return ret; > } > +#define VFIO_UNBIND_TIMEOUT 100 > + > +/* It is not safe to initiate host driver probe if the vfio driver has > not > + * completely unbound the device. Usual wait time is 1 to 2 seconds. > + * So, return if the unbind didn't complete in 10 seconds. > + */ > +static int > +virPCIWaitForVFIOUnbindCompletion(virPCIDevicePtr dev) > +{ > + int retry = 0; > + int ret = -1; > + char *path = NULL; > + > + if (!(path = virPCIDeviceGetIOMMUGroupDev(dev))) > + goto cleanup; > + > + while (retry++ < VFIO_UNBIND_TIMEOUT) { > + if (!virFileExists(path)) > + break; > + usleep(100000); /* Sleep for 1/10th of a second */ > + } > + > + if (virFileExists(path)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("VFIO unbind not completed even after %d > seconds" > + " for device %s"), retry, dev->name); > + goto cleanup; > + } > + > + ret = 0; > + cleanup: > + VIR_FREE(path); > + return ret; > + > +} > + > + > static int > virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, > const char *driver, > @@ -1288,6 +1325,10 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, > /* This device is the last to unbind from vfio. As we explicitly > * add a missing device in the list to inactiveList, we will > just > * go through the list. */ > + > + if (virPCIWaitForVFIOUnbindCompletion(dev) < 0) > + goto cleanup; > + > while (inactiveDevs && (i < > virPCIDeviceListCount(inactiveDevs))) { > virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, > i); > if (dev->iommuGroup == pcidev->iommuGroup) { > diff --git a/tests/virpcimock.c b/tests/virpcimock.c > index 837976a..cf92c58 100644 > --- a/tests/virpcimock.c > +++ b/tests/virpcimock.c > @@ -52,6 +52,7 @@ static DIR * (*realopendir)(const char *name); > char *fakesysfsdir; > # define PCI_SYSFS_PREFIX "/sys/bus/pci/" > +# define ROOT_DEV_PREFIX "/dev/" > # define STDERR(...) > \ > fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__); > \ > @@ -248,6 +249,13 @@ getrealpath(char **newpath, > errno = ENOMEM; > return -1; > } > + } else if (STRPREFIX(path, ROOT_DEV_PREFIX)) { > + if (virAsprintfQuiet(newpath, "%s/%s", > + fakesysfsdir, > + path) < 0) { > + errno = ENOMEM; > + return -1; > + } > } else { > if (VIR_STRDUP_QUIET(*newpath, path) < 0) > return -1; > @@ -1002,7 +1010,8 @@ access(const char *path, int mode) > init_syms(); > - if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { > + if (STRPREFIX(path, PCI_SYSFS_PREFIX) || > + STRPREFIX(path, ROOT_DEV_PREFIX)) { > char *newpath; > if (getrealpath(&newpath, path) < 0) > return -1; > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list