On Fri, Apr 14, 2023 at 03:44:16PM +0200, Michal Privoznik wrote:
From: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
virsh command domxml-to-native failed with below error but start
command succeed for same domain xml.
"internal error: invalid PCI passthrough type 'default'"
If a <hostdev> PCI backend is not set in the XML, the supported
one is then chosen in qemuHostdevPreparePCIDevicesCheckSupport().
But this function is not called anywhere from
qemuConnectDomainXMLToNative(). But qemuDomainPrepareHostdev()
is. And it is also called from domain startup/hotplug code.
Therefore, move the backend setting to the common path.
And since qemuDomainPrepareHostdev() is called transitively from
our qemuxml2argvtest, we can just drop the code in
testCompareXMLToArgvCreateArgs() that preselects the backend (if
a mock for qemuHostdevHostSupportsPassthroughVFIO() is provided.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/qemu/qemu_domain.c | 12 ++++++++++++
src/qemu/qemu_hostdev.c | 16 +++++++---------
src/qemu/qemu_hotplug.c | 2 +-
tests/qemuxml2argvmock.c | 7 +++++++
tests/qemuxml2argvtest.c | 6 ------
5 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 63b13b6875..6de846f158 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11275,6 +11275,18 @@ qemuDomainPrepareHostdev(virDomainHostdevDef *hostdev,
}
}
+ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+ bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO();
+ virDomainHostdevSubsysPCIBackendType *backend = &hostdev->source.subsys.u.pci.backend;
+
+ if (*backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT &&
+ supportsPassthroughVFIO &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
+ *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
+ }
+ }
+
return 0;
}
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 45cd1066f0..8408928c00 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -156,7 +156,7 @@ qemuHostdevHostSupportsPassthroughVFIO(void)
static bool
qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDef **hostdevs,
I have a nagging feeling this terrible function needs its name changed, but that's mostly unrelated to this patch.
size_t nhostdevs,
- virQEMUCaps *qemuCaps)
+ virQEMUCaps *qemuCaps G_GNUC_UNUSED)
This is no unused in couple of callers above and could be removed.
{
bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO();
size_t i;
@@ -173,17 +173,15 @@ qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDef **hostdevs,
switch (*backend) {
case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
- if (supportsPassthroughVFIO &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
- *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
Since this is now not setting any defaults the comment above (not in context here, but it says "assign defaults ...") should be removed as well.
+ /* qemuDomainPrepareHostdev() should have set different value */
+ if (supportsPassthroughVFIO) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("QEMU doesn't support VFIO PCI passthrough"));
This error is also thrown in three more places (which itself is mind-blowing), but with a different wording. For the sake of translations they could be unified so that there are not two different strings for the same error. If we want this function to be the know-it-all of all related checks then it should maybe replace the other places that check (almost) the same things. Look for "VFIO PCI device assignment" in the codebase to find the other error messages I'm mentioning here.
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("host doesn't support passthrough of "
- "host PCI devices"));
- return false;
+ _("host doesn't support passthrough of host PCI devices"));
What's really weird here is that the function just uses a host capability to decide which error message to use. But the logic is not visible from here because it depends on another function's behaviour, qemuDomainPrepareHostdev in this case. The comment kind of papers over it. Bear with me for a bit of analysis. qemuHostdevPreparePCIDevicesCheckSupport is only called from qemuHostdevPreparePCIDevices which is called from: - qemuDomainAttachHostPCIDevice (during hotplug) - qemuHostdevPrepareDomainDevices (during startup) Well, during hotplug there is a call to qemuDomainPrepareHostdev just before going down the rabbit hole into ...CheckSupport. And during startup/migration the other function is called from qemuProcessPrepareHost which is (and must) be always called before qemuProcessPrepareDomain which calls qemuDomainPrepareHostdev as well. My question is why we then do not error out right in qemuDomainPrepareHostdev which has basically the same condition and it is easier to see the logic behind the error message?
}
-
- break;
+ return false;
case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
if (!supportsPassthroughVFIO) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 8902b40815..95ac0fd754 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1478,7 +1478,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver,
&hostdev, 1, priv->qemuCaps, flags) < 0)
return -1;
- /* this could have been changed by qemuHostdevPreparePCIDevices */
+ /* this could have been changed by qemuDomainPrepareHostdev */
backend = hostdev->source.subsys.u.pci.backend;
I think this comment was here just to explain why is this variable not initialized in its declaration. With the above change it can be set there and these two lines removed instead.
Attachment:
signature.asc
Description: PGP signature
