[PATCH 19/33] qemu: Add qemuDomainDefaultUSBControllerModel()

Andrea Bolognani posted 33 patches 1 year, 1 month ago
There is a newer version of this series
[PATCH 19/33] qemu: Add qemuDomainDefaultUSBControllerModel()
Posted by Andrea Bolognani 1 year, 1 month ago
Extract the logic from qemuDomainControllerDefPostParse().

The code is mostly unchanged, but there's a subtle difference:
the piix3-uhci has been moved from the top of the chunk to the
bottom. This is because the original code set cont->model
directly, which made it okay to start with a suboptimal default
and subsequently overwrite it with a better one; now that we
return the selected value instead, we need to make sure that
we only ever consider piix3-uhci if everything else has failed.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7475fb4f39..09f572b0b5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4151,6 +4151,61 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
 }
 
 
+static int
+qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
+                                    const virDomainControllerDef *cont,
+                                    virQEMUCaps *qemuCaps,
+                                    unsigned int parseFlags)
+{
+    /* Pick a suitable default model for the USB controller if none
+     * has been selected by the user and we have the qemuCaps for
+     * figuring out which controllers are supported.
+     *
+     * We rely on device availability instead of setting the model
+     * unconditionally because, for some machine types, there's a
+     * chance we will get away with using the legacy USB controller
+     * when the relevant device is not available.
+     *
+     * See qemuBuildControllersCommandLine() */
+
+    if (ARCH_IS_S390(def->os.arch)) {
+        if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+            /* set the default USB model to none for s390 unless an
+             * address is found */
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
+        }
+    } else if (ARCH_IS_PPC64(def->os.arch)) {
+        /* To not break migration we need to set default USB controller
+         * for ppc64 to pci-ohci if we cannot change ABI of the VM.
+         * The nec-usb-xhci or qemu-xhci controller is used as default
+         * only for newly defined domains or devices. */
+        if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+            virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+        } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+            virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+        } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
+        } else {
+            /* Explicitly fallback to legacy USB controller for PPC64. */
+            return -1;
+        }
+    } else if (def->os.arch == VIR_ARCH_AARCH64) {
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+        else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+    }
+
+    /* Default USB controller is piix3-uhci if available. */
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
+        return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+
+    return -1;
+}
+
+
 static int
 qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
                                virDomainDef *def,
@@ -5640,50 +5695,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
 
     case VIR_DOMAIN_CONTROLLER_TYPE_USB:
         if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) {
-            /* Pick a suitable default model for the USB controller if none
-             * has been selected by the user and we have the qemuCaps for
-             * figuring out which controllers are supported.
-             *
-             * We rely on device availability instead of setting the model
-             * unconditionally because, for some machine types, there's a
-             * chance we will get away with using the legacy USB controller
-             * when the relevant device is not available.
-             *
-             * See qemuBuildControllersCommandLine() */
-
-            /* Default USB controller is piix3-uhci if available. */
-            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
-                cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
-
-            if (ARCH_IS_S390(def->os.arch)) {
-                if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
-                    /* set the default USB model to none for s390 unless an
-                     * address is found */
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
-                }
-            } else if (ARCH_IS_PPC64(def->os.arch)) {
-                /* To not break migration we need to set default USB controller
-                 * for ppc64 to pci-ohci if we cannot change ABI of the VM.
-                 * The nec-usb-xhci or qemu-xhci controller is used as default
-                 * only for newly defined domains or devices. */
-                if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
-                    virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
-                } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
-                    virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
-                } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
-                } else {
-                    /* Explicitly fallback to legacy USB controller for PPC64. */
-                    cont->model = -1;
-                }
-            } else if (def->os.arch == VIR_ARCH_AARCH64) {
-                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
-                else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
-                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
-            }
+            cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags);
         }
         /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */
         if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 ||
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 19/33] qemu: Add qemuDomainDefaultUSBControllerModel()
Posted by Peter Krempa 1 year, 1 month ago
On Wed, Jan 24, 2024 at 20:37:39 +0100, Andrea Bolognani wrote:
> Extract the logic from qemuDomainControllerDefPostParse().
> 
> The code is mostly unchanged, but there's a subtle difference:
> the piix3-uhci has been moved from the top of the chunk to the
> bottom. This is because the original code set cont->model
> directly, which made it okay to start with a suboptimal default
> and subsequently overwrite it with a better one; now that we
> return the selected value instead, we need to make sure that
> we only ever consider piix3-uhci if everything else has failed.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 100 +++++++++++++++++++++++------------------
>  1 file changed, 56 insertions(+), 44 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7475fb4f39..09f572b0b5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4151,6 +4151,61 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
>  }
>  
>  
> +static int
> +qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
> +                                    const virDomainControllerDef *cont,
> +                                    virQEMUCaps *qemuCaps,
> +                                    unsigned int parseFlags)
> +{
> +    /* Pick a suitable default model for the USB controller if none
> +     * has been selected by the user and we have the qemuCaps for
> +     * figuring out which controllers are supported.
> +     *
> +     * We rely on device availability instead of setting the model
> +     * unconditionally because, for some machine types, there's a
> +     * chance we will get away with using the legacy USB controller
> +     * when the relevant device is not available.
> +     *
> +     * See qemuBuildControllersCommandLine() */
> +
> +    if (ARCH_IS_S390(def->os.arch)) {
> +        if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> +            /* set the default USB model to none for s390 unless an
> +             * address is found */
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> +        }
> +    } else if (ARCH_IS_PPC64(def->os.arch)) {
> +        /* To not break migration we need to set default USB controller
> +         * for ppc64 to pci-ohci if we cannot change ABI of the VM.
> +         * The nec-usb-xhci or qemu-xhci controller is used as default
> +         * only for newly defined domains or devices. */
> +        if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> +        } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +        } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> +        } else {
> +            /* Explicitly fallback to legacy USB controller for PPC64. */
> +            return -1;

So this is extracting the logic of passing
VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT as model, which is -1, thus at
this point faithful representation what the code did. The very next
commit though declares -1 to be an error in the function comment which
is not true.

Extract it here as the proper enum value.

> +        }
> +    } else if (def->os.arch == VIR_ARCH_AARCH64) {
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> +        else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> +            return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +    }
> +
> +    /* Default USB controller is piix3-uhci if available. */
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> +        return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> +
> +    return -1;

This is later on documented via a comment but also should use the proper
enum value.

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