On Fri, 2015-10-30 at 05:04 +0530, Shivaprasad G Bhat wrote:
Author: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
This line is redundant since the information is already stored in git, plus you have the Signed-off-by below. This applies to Patch 7 as well.
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. Setting the timeout to 15 seconds for now.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 425c1a7..6bf640d 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1097,6 +1097,42 @@ static bool virPCIIsAKnownStub(char *driver) return ret; }
+#define VFIO_UNBIND_TIMEOUT 15
There's one trailing space here, and git complains about it when applying the patch. syntax-check complains too.
+ +/* It is not safe to initiate host driver probe if the vfio driver has not + * completely unbound the device. + * So, return if the unbind didn't complete in 15 seconds. + */ +static int virPCIWaitForVfioUnbindCompletion(virPCIDevicePtr dev)
Return type on a separate line, please. VFIO is an acronym, like PCI, and should always be all-caps.
+{ + int retry = 0; + int ret = -1; + char *path = NULL; + + if (!(path = virPCIDeviceGetIOMMUGroupDev(dev))) + goto cleanup; + + while (retry++ < VFIO_UNBIND_TIMEOUT) { + if (!virFileExists(path)) + break; + sleep(1);
Do we want this to be in seconds, or would a higher granularity be better?
+ } + + if (virFileExists(path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("The VFIO unbind not completed even after %d seconds for device %.4x:%.2x:%.2x.%.1x"), + retry, dev->domain, dev->bus, dev->slot, dev->function);
This line is very long, please split the error message into two parts. I'd say "The" and "even" can be dropped. You should use dev->name instead of formatting the name yourself.
+ goto cleanup; + } + + ret = 0; +cleanup :
Please remove the space between the label and the semicolon, and add one in front of the label. I'd also add an empty line before the label.
+ VIR_FREE(path); + return ret; + +} + + static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char *drvdir) { char *path = NULL; @@ -1203,6 +1239,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, goto cleanup; }
+ if (virPCIWaitForVfioUnbindCompletion(dev) < 0) + goto cleanup; + while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) { virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i); if (dev->iommuGroup == pcidev->iommuGroup) {
Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team