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(a)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(a)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