2:35 p.m.
The current virHostdevPreparePCIDevices code fails to detect an unmanaged
VFIO device that is in the activePCIHostdevs, and active in the same
domain name as dom_name, as a device in use. Considering a call to this
function, when activePCIHostdevs has a VFIO deviceA in the list, and
deviceA is active in domain 'dom_name', this is what happens:
- At step 1, the code goes to the 'if (usesVFIO)' block. All devices in
the same IOMMU group of deviceA are used as argument of
virHostdevIsPCINodeDeviceUsed, via the IOMMUGroupIterate function;
- Inside virHostdevIsPCINodeDeviceUsed, an 'usesVFIO' verification is
made, following to an 'iommu_owner' jump that sets ret=0. This will
happen to all devices of the IOMMU group that belongs to the same domain
as dom_name, including deviceA;
- Step 2 starts, thinking that all devices inside hostdevs are available,
which is not the case.
This error was detected when changing tests/virhostdev.c to use
vfio-pci instead of pci-stub (a change that will follow this one).
The side effect observed was a test failure in
testVirHostdevPreparePCIHostdevs_unmanaged, result of the behavior
mentioned above:
Unexpected count of items in mgr->inactivePCIHostdevs: 1, expecting 0
This patch fixes virHostdevIsPCINodeDeviceUsed to consider the case of
a VFIO device that is already active in the domain. First, check if
the device is in the activePCIHostdev list and bail immediately if
true. Otherwise, in case of VFIO, check for IOMMU group ownership
of the domain. This is done by a new callback function for
IOMMUGroupIterate. If the VFIO device isn't in the active list and
its domain has ownership of the IOMMU, then it is unused.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/util/virhostdev.c | 74 +++++++++++++++++++++++++++++++++----------
1 file changed, 57 insertions(+), 17 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a3647a6cf4..35be8fede1 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -55,6 +55,46 @@ struct virHostdevIsPCINodeDeviceUsedData {
const bool usesVFIO;
};
+
+static virPCIDevicePtr
+virHostdevFindActivePCIDevWithAddr(virPCIDeviceAddressPtr devAddr,
+ virHostdevManagerPtr mgr)
+{
+ virPCIDevicePtr ret = NULL;
+
+ ret = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
+ devAddr->domain, devAddr->bus,
+ devAddr->slot, devAddr->function);
+
+ return ret;
+}
+
+/* Callback to be used inside virHostdevIsPCINodeDeviceUsed to check
+ * for IOMMU ownership of a domain given by helperData->domainName. */
+static int
+virHostdevDomainHasIOMMUOwnershipCb(virPCIDeviceAddressPtr devAddr, void *opaque)
+{
+ struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque;
+ virPCIDevicePtr actual;
+ int ret = 0;
+
+ actual = virHostdevFindActivePCIDevWithAddr(devAddr, helperData->mgr);
+ if (!actual)
+ return ret;
+
+ if (helperData->usesVFIO) {
+ const char *actual_drvname = NULL;
+ const char *actual_domname = NULL;
+ virPCIDeviceGetUsedBy(actual, &actual_drvname, &actual_domname);
+
+ if ((actual_domname && helperData->domainName) &&
+ (STRNEQ(actual_domname, helperData->domainName)))
+ ret = -1;
+ }
+
+ return ret;
+}
+
/* This module makes heavy use of bookkeeping lists contained inside a
* virHostdevManager instance to keep track of the devices' status. To make
* it easy to spot potential ownership errors when moving devices from one
@@ -82,19 +122,13 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr
devAddr, void *o
int ret = -1;
struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque;
- actual = virPCIDeviceListFindByIDs(helperData->mgr->activePCIHostdevs,
- devAddr->domain, devAddr->bus,
- devAddr->slot, devAddr->function);
+ actual = virHostdevFindActivePCIDevWithAddr(devAddr, helperData->mgr);
+
if (actual) {
const char *actual_drvname = NULL;
const char *actual_domname = NULL;
virPCIDeviceGetUsedBy(actual, &actual_drvname, &actual_domname);
- if (helperData->usesVFIO &&
- (actual_domname && helperData->domainName) &&
- (STREQ(actual_domname, helperData->domainName)))
- goto iommu_owner;
-
if (actual_drvname && actual_domname)
virReportError(VIR_ERR_OPERATION_INVALID,
_("PCI device %s is in use by "
@@ -105,9 +139,20 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr
devAddr, void *o
virReportError(VIR_ERR_OPERATION_INVALID,
_("PCI device %s is in use"),
virPCIDeviceGetName(actual));
+
goto cleanup;
}
- iommu_owner:
+
+ /* For VFIO devices, the domain helperData->domainName must have ownership
+ * of the IOMMU group that contains devAddr. Otherwise, even if the devAddr
+ * is not in use, another device of that IOMMU group is in use by anotherdomain,
+ * forbidding devAddr to be used in helperData->domainName. */
+ if (helperData->usesVFIO)
+ if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
+ virHostdevDomainHasIOMMUOwnershipCb,
+ helperData) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
return ret;
@@ -673,17 +718,12 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
/* The device is in use by other active domain if
* the dev is in list activePCIHostdevs. VFIO devices
* belonging to same iommu group can't be shared
- * across guests.
+ * across guests. virHostdevIsPCINodeDeviceUsedData handles
+ * both cases.
*/
devAddr = virPCIDeviceGetAddress(pci);
- if (usesVFIO) {
- if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
- virHostdevIsPCINodeDeviceUsed,
- &data) < 0)
- goto cleanup;
- } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) {
+ if (virHostdevIsPCINodeDeviceUsed(devAddr, &data))
goto cleanup;
- }
}
/* Step 1.5: For non-802.11Qbh SRIOV network devices, save the
--
2.20.1