I know you're already working on a v2 of this, just a couple of quick remarks below. On Fri, 2015-10-30 at 05:01 +0530, Shivaprasad G Bhat wrote:
Author: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
The host will crash if a device is bound to host driver when the device belonging to same iommu group is in use by any of the guests. So, do the host driver probe only after all the devices in the iommu group have unbound from the vfio.
The patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=1272300
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 6c24a81..425c1a7 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1128,6 +1128,23 @@ static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char return result; }
+static int virPCIDeviceBoundToVFIODriver(virPCIDeviceAddressPtr devAddr, void *opaque ATTRIBUTE_UNUSED)
Return type on a separate line, please.
+{ + int ret = 0; + virPCIDevicePtr pci = NULL; + + if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function))) + goto cleanup; + + if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci")) + ret = -1;
As mentioned in the comment for Patch 1, I think this is fairly obscure: if I had not read throught the whole series, I'd assume this checks whether the device is configured to use vfio-pci as stub driver, not whether it is currently bound to it, and it would not be immediately clear to me how this check fits in a function called virPCIDeviceBoundToVFIODriver(). I think it would be much cleaner to query the driver explicitly using virPCIDeviceGetDriverPathAndName() and remove that call from virPCIDeviceNew().
+ + cleanup: + virPCIDeviceFree(pci); + return ret; +} + static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED, @@ -1177,11 +1194,34 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, dev->remove_slot = false;
reprobe: - if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) - goto cleanup; - /* Steal the dev from list inactiveDevs */ - if (inactiveDevs) - virPCIDeviceListDel(inactiveDevs, dev); + if (STREQ_NULLABLE(dev->stubDriver, "vfio-pci")) { + size_t i = 0; + virPCIDeviceAddress devAddr = { dev->domain, dev->bus, + dev->slot, dev->function }; + if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, virPCIDeviceBoundToVFIODriver, NULL) < 0) { + result = 0; + goto cleanup; + } + + while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) { + virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i); + if (dev->iommuGroup == pcidev->iommuGroup) { + if (virPCIDeviceReprobeHostDriver(pcidev, driver, drvdir) < 0) + goto cleanup; + /* Steal the dev from list inactiveDevs */ + virPCIDeviceListDel(inactiveDevs, pcidev); + continue; + } + i++; + } + } else { + if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) + goto cleanup; + /* Steal the dev from list inactiveDevs */ + + if (inactiveDevs) + virPCIDeviceListDel(inactiveDevs, dev);
Either put the empty line before the comment or just get rid of it.
+ }
result = 0;
Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team