On Fri, Nov 20, 2015 at 9:07 PM, Laine Stump <laine@xxxxxxxxx> wrote: > 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. > Hi Laine, I got it confirmed from Alexey(CC'ed) that this delay is actually not needed. So, I feel this patch and Patch 9 which unbinds from vfio in a way to help this wait to take place, both can be dropped. Andrea's approach of unbind can be taken which is rather clean. My patch http://www.redhat.com/archives/libvir-list/2015-November/msg00007.html still fixes a genuine issue which I'll rework after Andrea's series gets merged. Andrea, I think we can use the mock changes and the test case(with some change) from this series for your series. Please let me know if you can pull them along or I'll rework and send them after you series is merged. Thanks and Regards, Shivaprasad >> >> 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@xxxxxxxxxxxxxxxxxx> >> --- >> 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@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list >> > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list