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 <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>