On Tue, Aug 19, 2025 at 18:22:30 +0200, Andrea Bolognani via Devel wrote:
> As well as qemuDomainDefaultUSBControllerModelAutoAdded().
Either split the patch or mention both in the summary.
>
> Switch from the current approach, in which an initial (poor)
> default is picked and then a better one later overwrites it,
> to the more common and easy to reason about pattern where
> the value is returned directly as soon as possible. The
> behavior is unchanged.
The commit message doesn't mention functional changes ...
>
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
> src/qemu/qemu_domain.c | 121 ++++++++++++++++++++---------------------
> 1 file changed, 60 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 639506d22a..3886b59026 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4319,60 +4319,60 @@ qemuDomainDefaultUSBControllerModel(const
> virDomainDef *def,
> virQEMUCaps *qemuCaps,
> unsigned int parseFlags)
> {
> - virDomainControllerModelUSB model =
> VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> + bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
>
> - /* 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_LOONGARCH(def->os.arch)) {
> + /* Use qemu-xhci (USB3) for modern architectures */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
... but the code considered VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI if
available as the default on non-x86 which is not preserved in the
loongarch branch. (by setting it first and the loongarch branch not
doing anything if XHCI is not present) ...
>
> - /* Default USB controller is piix3-uhci if available. Fall back to
> - * 'pci-ohci' otherwise which is the default for non-x86 machines
> - * which honour -usb */
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> - else if (!ARCH_IS_X86(def->os.arch) &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> + /* No fallback if that's not available */
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
... but this skips the fallback.
> + }
> +
> + 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;
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +
> + /* Fall through */
Here with aarch the logic is preserved. Although I think for clarity
it'd be better to simply condense all arch-specific logic here (e.g. by
copying out the default.
> + }
>
> if (ARCH_IS_S390(def->os.arch)) {
> /* No default model on s390x, one has to be provided
> * explicitly by the user */
> - 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)) {
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> - } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> - } else {
> - /* Explicitly fallback to legacy USB controller for PPC64. */
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> - }
> - } else if (def->os.arch == VIR_ARCH_AARCH64) {
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> - } else if (ARCH_IS_LOONGARCH(def->os.arch)) {
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> }
>
> - return model;
> + if (ARCH_IS_PPC64(def->os.arch)) {
> + /* Newly-defined guests should use USB3 if possible */
> + if (abiUpdate && virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_DEVICE_QEMU_XHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> + if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> +
> + /* To preserve backwards compatibility, existing guests need to
> + * use pci-ohci (USB1) instead */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> +
> + /* If neither USB3 nor USB1 can be used, bail */
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
So that we don't have "fall through" and normal blocks.
> + }
> +
> + /* The default USB controller is piix3-uhci (USB1) if available.
> + * This choice is a fairly poor one, rooted primarily in
> + * historical reasons; thankfully, in most cases we will have
> + * picked a much more reasonable value before ever getting here */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
Including having an explicit block for x86(_64) with and leaving the
rest of the logic for non-mentioned arches.
> + else if (!ARCH_IS_X86(def->os.arch) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> +
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> }
>
>
> @@ -4403,22 +4403,21 @@ virDomainControllerModelUSB
> qemuDomainDefaultUSBControllerModelAutoAdded(const virDomainDef *def,
> virQEMUCaps *qemuCaps)
> {
> - virDomainControllerModelUSB model =
> VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> -
> if (ARCH_IS_X86(def->os.arch)) {
> if (qemuDomainIsQ35(def)) {
> - /* 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))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
> - else
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> + 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 neither USB3 nor USB2 are available, do not add
> + * the controller at all */
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> }
This explicit logic is much better.
> }
>
> @@ -4426,10 +4425,10 @@ qemuDomainDefaultUSBControllerModelAutoAdded(const
> virDomainDef *def,
> if (STREQ(def->os.machine, "versatilepb") ||
> STRPREFIX(def->os.machine, "realview-eb"))
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> }
>
> - return model;
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> }
>
With the changes I've suggested:
Reviewed-by: Peter Krempa <pkre...@redhat.com>
