Re: [libvirt] [PATCH v4 03/10] Refuse to reattach from vfio if the iommu group is in use by any domain

On 11/14/2015 03:36 AM, Shivaprasad G Bhat wrote: > Refuse to reattach from vfio if the iommu group is in use by any domain util: refuse to reattach device if any device in the same group is in use by any domain > It is incorrect to attempt the device reattach of a function, s/function/device/ > when some other domain is using some functions belonging to the same > iommu > group. s/some other domain/any other domain/ s/some functions/any device/ > > Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > --- > src/util/virhostdev.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c > index de029a0..f24ccd8 100644 > --- a/src/util/virhostdev.c > +++ b/src/util/virhostdev.c > @@ -1590,18 +1590,35 @@ > virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, > virPCIDevicePtr pci) > { > virPCIDeviceAddressPtr devAddr = NULL; > + bool usesVfio = false; > + char *drvPath = NULL; > + char *drvName = NULL; > struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, > NULL, > false}; > int ret = -1; > + if (virPCIDeviceGetDriverPathAndName(pci, &drvPath, &drvName) > < 0) > + goto out; > + > + if (STREQ_NULLABLE(drvName, "vfio-pci")) > + usesVfio = true; > + > virObjectLock(hostdev_mgr->activePCIHostdevs); > virObjectLock(hostdev_mgr->inactivePCIHostdevs); > if (!(devAddr = virPCIDeviceGetAddress(pci))) > goto out; > - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) > + if (usesVfio) { > + /* Doesn't matter which device. If any domain is actively using the > + * iommu group, refuse to reattach */ > + if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, > + virHostdevIsPCINodeDeviceUsed, > + &data) < 0) > + goto out; Calling the iterator seems very inefficient to me (which is why I suggested in the original BZ comments that we save the iommuGroup of each device to the activelist): 1) you know the iommu group of the current PCI device (it is in pci->iommuGroup) 2) all that virHostdevIsPCINodeDeviceUsed does is check through activePCIHostdevs for the device, and this iterator is just calling virHostdevIsPCINodeDeviceUsed for each device in the same iommuGroup as the current device. Therefore, all you really need to do is look for any devices in acticePCIHostdevs that have dev->iommuGroup == pci->iommuGroup. Calling the iterator results in a lot of accesses to sysfs, opening and closing files, printfs, etc. Unless that is necessary to catch some extra case (and I don't see that), it's much better to do it the simpler way - more efficient and easier to understand a year from now when we come back to this.

> + } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) { > goto out; > + } > virPCIDeviceReattachInit(pci); > @@ -1614,6 +1631,8 @@ > virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, > virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); > virObjectUnlock(hostdev_mgr->activePCIHostdevs); > VIR_FREE(devAddr); > + VIR_FREE(drvPath); > + VIR_FREE(drvName); > return ret; > } > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >