[PATCH 29/33] qemu: Move qemuDomainDefault*() functions together

Andrea Bolognani posted 33 patches 1 year ago
There is a newer version of this series
[PATCH 29/33] qemu: Move qemuDomainDefault*() functions together
Posted by Andrea Bolognani 1 year ago
Most of the functions responsible for choosing architecture and
machine specific defaults are already close to one another, with
just a couple of strays. Having everything in one place will
hopefully make it harder to miss updating any of the functions
when new architectures are being introduced.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c | 151 ++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 76 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 22980c25a9..cede7c9eb2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4292,6 +4292,81 @@ qemuDomainForbidLegacyUSBController(const virDomainDef *def)
 }
 
 
+/**
+ * qemuDomainDefaultNetModel:
+ * @def: domain definition
+ * @qemuCaps: qemu capabilities
+ *
+ * Returns the default network model for a given domain. Note that if @qemuCaps
+ * is NULL this function may return NULL if the default model depends on the
+ * capabilities.
+ */
+static int
+qemuDomainDefaultNetModel(const virDomainDef *def,
+                          virQEMUCaps *qemuCaps)
+{
+    /* When there are no backwards compatibility concerns getting in
+     * the way, virtio is a good default */
+    if (ARCH_IS_S390(def->os.arch) ||
+        qemuDomainIsRISCVVirt(def)) {
+        return VIR_DOMAIN_NET_MODEL_VIRTIO;
+    }
+
+    if (ARCH_IS_ARM(def->os.arch)) {
+        if (STREQ(def->os.machine, "versatilepb"))
+            return VIR_DOMAIN_NET_MODEL_SMC91C111;
+
+        if (qemuDomainIsARMVirt(def))
+            return VIR_DOMAIN_NET_MODEL_VIRTIO;
+
+        /* Incomplete. vexpress (and a few others) use this, but not all
+         * arm boards */
+        return VIR_DOMAIN_NET_MODEL_LAN9118;
+    }
+
+    /* In all other cases the model depends on the capabilities. If they were
+     * not provided don't report any default. */
+    if (!qemuCaps)
+        return VIR_DOMAIN_NET_MODEL_UNKNOWN;
+
+    /* Try several network devices in turn; each of these devices is
+     * less likely be supported out-of-the-box by the guest operating
+     * system than the previous one */
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139))
+        return VIR_DOMAIN_NET_MODEL_RTL8139;
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000))
+        return VIR_DOMAIN_NET_MODEL_E1000;
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET))
+        return VIR_DOMAIN_NET_MODEL_VIRTIO;
+
+    /* We've had no luck detecting support for any network device,
+     * but we have to return something: might as well be rtl8139 */
+    return VIR_DOMAIN_NET_MODEL_RTL8139;
+}
+
+
+static int
+qemuDomainDefaultVideoModel(const virDomainDef *def,
+                            virQEMUCaps *qemuCaps)
+{
+    if (ARCH_IS_PPC64(def->os.arch))
+        return VIR_DOMAIN_VIDEO_TYPE_VGA;
+
+    if (qemuDomainIsARMVirt(def) ||
+        qemuDomainIsRISCVVirt(def) ||
+        ARCH_IS_S390(def->os.arch)) {
+        return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
+    }
+
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA))
+        return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
+        return VIR_DOMAIN_VIDEO_TYPE_VGA;
+
+    return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
+}
+
+
 static int
 qemuDomainDefaultSerialType(const virDomainDef *def)
 {
@@ -5633,60 +5708,6 @@ qemuDomainValidateStorageSource(virStorageSource *src,
 }
 
 
-/**
- * qemuDomainDefaultNetModel:
- * @def: domain definition
- * @qemuCaps: qemu capabilities
- *
- * Returns the default network model for a given domain. Note that if @qemuCaps
- * is NULL this function may return NULL if the default model depends on the
- * capabilities.
- */
-static int
-qemuDomainDefaultNetModel(const virDomainDef *def,
-                          virQEMUCaps *qemuCaps)
-{
-    /* When there are no backwards compatibility concerns getting in
-     * the way, virtio is a good default */
-    if (ARCH_IS_S390(def->os.arch) ||
-        qemuDomainIsRISCVVirt(def)) {
-        return VIR_DOMAIN_NET_MODEL_VIRTIO;
-    }
-
-    if (ARCH_IS_ARM(def->os.arch)) {
-        if (STREQ(def->os.machine, "versatilepb"))
-            return VIR_DOMAIN_NET_MODEL_SMC91C111;
-
-        if (qemuDomainIsARMVirt(def))
-            return VIR_DOMAIN_NET_MODEL_VIRTIO;
-
-        /* Incomplete. vexpress (and a few others) use this, but not all
-         * arm boards */
-        return VIR_DOMAIN_NET_MODEL_LAN9118;
-    }
-
-    /* In all other cases the model depends on the capabilities. If they were
-     * not provided don't report any default. */
-    if (!qemuCaps)
-        return VIR_DOMAIN_NET_MODEL_UNKNOWN;
-
-    /* Try several network devices in turn; each of these devices is
-     * less likely be supported out-of-the-box by the guest operating
-     * system than the previous one */
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139))
-        return VIR_DOMAIN_NET_MODEL_RTL8139;
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000))
-        return VIR_DOMAIN_NET_MODEL_E1000;
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET))
-        return VIR_DOMAIN_NET_MODEL_VIRTIO;
-
-    /* We've had no luck detecting support for any network device,
-     * but we have to return something: might as well be rtl8139 */
-    return VIR_DOMAIN_NET_MODEL_RTL8139;
-}
-
-
-
 static bool
 qemuDomainChrMatchDefaultPath(const char *prefix,
                               const char *infix,
@@ -6095,28 +6116,6 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDef *net,
 }
 
 
-static int
-qemuDomainDefaultVideoModel(const virDomainDef *def,
-                            virQEMUCaps *qemuCaps)
-{
-    if (ARCH_IS_PPC64(def->os.arch))
-        return VIR_DOMAIN_VIDEO_TYPE_VGA;
-
-    if (qemuDomainIsARMVirt(def) ||
-        qemuDomainIsRISCVVirt(def) ||
-        ARCH_IS_S390(def->os.arch)) {
-        return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
-    }
-
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA))
-        return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
-        return VIR_DOMAIN_VIDEO_TYPE_VGA;
-
-    return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
-}
-
-
 static int
 qemuDomainDeviceVideoDefPostParse(virDomainVideoDef *video,
                                   const virDomainDef *def,
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 29/33] qemu: Move qemuDomainDefault*() functions together
Posted by Peter Krempa 1 year ago
On Wed, Jan 24, 2024 at 20:37:49 +0100, Andrea Bolognani wrote:
> Most of the functions responsible for choosing architecture and
> machine specific defaults are already close to one another, with
> just a couple of strays. Having everything in one place will
> hopefully make it harder to miss updating any of the functions
> when new architectures are being introduced.

Too bad that the code is using a eclectic selection of ARCH_IS_* macros
together with checkers such as qemuDomainIsRISCVVirt etc, because it's
hard to create a proper fix which would be a properly typed switch
statement, where the compiler would enforce what you want to achieve
here.

Also too bad that the list of arches is *massive* to have it everywhere,
despite the fac that we effectively ignore a half of them.

> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 151 ++++++++++++++++++++---------------------
>  1 file changed, 75 insertions(+), 76 deletions(-)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH 29/33] qemu: Move qemuDomainDefault*() functions together
Posted by Andrea Bolognani 1 year ago
On Thu, Jan 25, 2024 at 03:58:29PM +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:49 +0100, Andrea Bolognani wrote:
> > Most of the functions responsible for choosing architecture and
> > machine specific defaults are already close to one another, with
> > just a couple of strays. Having everything in one place will
> > hopefully make it harder to miss updating any of the functions
> > when new architectures are being introduced.
>
> Too bad that the code is using a eclectic selection of ARCH_IS_* macros
> together with checkers such as qemuDomainIsRISCVVirt etc, because it's
> hard to create a proper fix which would be a properly typed switch
> statement, where the compiler would enforce what you want to achieve
> here.

Unfortunately that's needed because some architectures have wildly
different machine types.

On Arm, for example, the virt machine type is lean, modern and geared
towards PCI/virtio, but you also have machine types implementing very
old embedded boards where all controllers are hard-coded.

The solution would be to restrict support to the small subset of
virt-friendly machine types, one or two per architecture, that we
actually test in any capacity, but since things have been
free-for-all until now there is the expectation that even machine
types that libvirt knows nothing about will keep working the same as
they ever have.

> Also too bad that the list of arches is *massive* to have it everywhere,
> despite the fac that we effectively ignore a half of them.

Yeah, my first instinct was to add switch()es everywhere, but as soon
as I started I realized that unfortunately it just made the code
much, much worse :(

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org