[PATCH 2/4] qemu: Move <hostdev/> PCI backend setting into qemuDomainPrepareHostdev()

[Martin Kletzander]

On Fri, Apr 14, 2023 at 03:44:16PM +0200, Michal Privoznik wrote:
>From: Zhenzhong Duan <zhenzhong.duan at intel.com>
>
>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 at intel.com>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230421/feacb503/attachment.sig>