As well as qemuDomainDefaultUSBControllerModelAutoAdded().
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.
Signed-off-by: Andrea Bolognani <abologna@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;
- /* 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;
+ }
+
+ 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 */
+ }
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;
+ }
+
+ /* 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;
+ 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;
}
}
@@ -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;
}
--
2.50.1On 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 <abologna@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 <pkrempa@redhat.com>
On Fri, Sep 19, 2025 at 11:19:38AM +0200, Peter Krempa wrote:
> On Tue, Aug 19, 2025 at 18:22:30 +0200, Andrea Bolognani via Devel wrote:
> > 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 ...
>
> > + 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) ...
That was unintentional.
Not that I think it's reasonable for loongarch64 domains to fall back
to pci-ohci, but a change in that direction should happen in its own
commit and be justified appropriately, not as part of what was
supposed to be a pure refactor.
This only serves to highlight how error-prone the current approach
makes things. It's way just too easy to miss stuff.
> > + 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.
>
[...]
>
> 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.
As you noticed, that happens in a later patch.
I figured it would be slightly nicer to do things in multiple
discrete steps, but looking back at the patches it's ultimately one
of those situations where you can't really split things cleanly, so
effectively I've just needlessly divided the big, confusing rewrite
into two big, confusing patches. Not very helpful.
I'll rewrite things according to your suggestions.
--
Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2026 Red Hat, Inc.