[PATCH 21/33] qemu: Clean up qemuDomainDefaultUSBControllerModel()

Andrea Bolognani posted 33 patches 8 months, 2 weeks ago
There is a newer version of this series
[PATCH 21/33] qemu: Clean up qemuDomainDefaultUSBControllerModel()
Posted by Andrea Bolognani 8 months, 2 weeks ago
Mostly reduce the number of 'else if' and improve comments.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d992b51877..dcf73c0e08 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4183,11 +4183,9 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
                                     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
+    bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
+
+    /* 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.
@@ -4200,50 +4198,62 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
              * 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)) {
+    }
+
+    if (ARCH_IS_PPC64(def->os.arch)) {
+        /* In order not to break migration compatibility, we need to
+         * set default USB controller to pci-ohci (USB2) for existing
+         * guests. New guests can use USB3 instead */
+        if (abiUpdate && 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)) {
+        if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
             return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
-        } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
+        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) {
+
+        /* For ppc64 specifically, returning -1 here will result in
+         * an attempt to use the legacy USB controller rather than an
+         * outright failure */
+        return -1;
+    }
+
+    if (def->os.arch == VIR_ARCH_AARCH64) {
+        /* Prefer USB3 */
         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))
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
             return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+
+        /* We should probably return -1 here and avoid the
+         * possibility of falling back to piix3-uhci (USB1), but
+         * historically we haven't done that and starting now might
+         * cause backwards compatibility issues */
     }
 
     if (ARCH_IS_X86(def->os.arch)) {
         if (qemuDomainIsQ35(def) && autoAdded) {
-            /* Prefer adding a USB3 controller if supported, fall back
-             * to USB2 if there is no USB3 available, and if that's
-             * unavailable don't add anything.
-             */
+            /* Prefer USB3 */
             if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
                 return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
             if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
                 return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+
+            /* Fall back to USB2 */
             if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
                 return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
+
+            /* If no suitable device is available, simply avoid
+             * adding the controller */
             return -2;
         }
     }
 
-    /* Default USB controller is piix3-uhci if available. */
+    /* piix3-uhci (USB1) is the last ditch effort before giving up */
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
         return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
 
+    /* No suitable model could be found. This will return in either
+     * an error or the use of the legacy USB controller */
     return -1;
 }
 
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 21/33] qemu: Clean up qemuDomainDefaultUSBControllerModel()
Posted by Peter Krempa 8 months, 2 weeks ago
On Wed, Jan 24, 2024 at 20:37:41 +0100, Andrea Bolognani wrote:
> Mostly reduce the number of 'else if' and improve comments.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 62 ++++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d992b51877..dcf73c0e08 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c

[...]

> @@ -4200,50 +4198,62 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def,

[...]

> -    } else if (def->os.arch == VIR_ARCH_AARCH64) {
> +
> +        /* For ppc64 specifically, returning -1 here will result in

VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT

> +         * an attempt to use the legacy USB controller rather than an
> +         * outright failure */
> +        return -1;

VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT

> +    }
> +
> +    if (def->os.arch == VIR_ARCH_AARCH64) {
> +        /* Prefer USB3 */
>          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))
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
>              return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +
> +        /* We should probably return -1 here and avoid the

VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT/VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE

> +         * possibility of falling back to piix3-uhci (USB1), but
> +         * historically we haven't done that and starting now might
> +         * cause backwards compatibility issues */
>      }
>  
>      if (ARCH_IS_X86(def->os.arch)) {
>          if (qemuDomainIsQ35(def) && autoAdded) {
> -            /* Prefer adding a USB3 controller if supported, fall back
> -             * to USB2 if there is no USB3 available, and if that's
> -             * unavailable don't add anything.
> -             */
> +            /* Prefer USB3 */
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
>                  return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
>                  return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +
> +            /* Fall back to USB2 */
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
>                  return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
> +
> +            /* If no suitable device is available, simply avoid
> +             * adding the controller */
>              return -2;
>          }
>      }
>  
> -    /* Default USB controller is piix3-uhci if available. */
> +    /* piix3-uhci (USB1) is the last ditch effort before giving up */
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
>          return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
>  
> +    /* No suitable model could be found. This will return in either
> +     * an error or the use of the legacy USB controller */

The caller will report this as an error or use the legacy USB
controller.

>      return -1;
>  }

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