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