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@xxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxx> > --- > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list