On Tue, Aug 19, 2025 at 18:22:34 +0200, Andrea Bolognani via Devel wrote:
> At this point the USB controller model selection logic is
> mostly sane, but there are still a few remaining oddities.
>
> First of all, piix3-uhci (USB1) is used in way too many
> contexts, either as default or fallback choice, while it
> really should be relegated to x86 guests only.
>
> Additionally, we're explicitly defaulting to pci-ohci (USB1)
> for a couple of Arm machine types, and limiting the default
> of qemu-xhci (USB3) to aarch64 only instead of using it for
> 32-bit guests as well.
>
> Address all of the aforementioned quirks. The result is a
> reasonably consistent experience across architectures, with
> USB3 generally being used whenever available, Intel-specific
> USB controllers only showing up in x86 guests, and pci-ohci
> acting as the vaguely reasonable falllback across the board.
>
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
> src/qemu/qemu_domain.c | 28 ++++++++-----------
> ...iew-minimal.aarch64-latest.abi-update.args | 2 +-
> ...view-minimal.aarch64-latest.abi-update.xml | 2 +-
> ...rch64-realview-minimal.aarch64-latest.args | 2 +-
> ...arch64-realview-minimal.aarch64-latest.xml | 2 +-
> ...lepb-minimal.armv7l-latest.abi-update.args | 2 +-
> ...ilepb-minimal.armv7l-latest.abi-update.xml | 2 +-
> ...v7l-versatilepb-minimal.armv7l-latest.args | 2 +-
> ...mv7l-versatilepb-minimal.armv7l-latest.xml | 2 +-
> tests/qemuxmlconfdata/ppc-dtb.ppc-latest.args | 2 +-
> tests/qemuxmlconfdata/ppc-dtb.ppc-latest.xml | 2 +-
> ...c-mac99-minimal.ppc-latest.abi-update.args | 2 +-
> ...pc-mac99-minimal.ppc-latest.abi-update.xml | 2 +-
> .../ppc-mac99-minimal.ppc-latest.args | 2 +-
> .../ppc-mac99-minimal.ppc-latest.xml | 2 +-
> .../ppce500-serial.ppc-latest.args | 2 +-
> .../ppce500-serial.ppc-latest.xml | 2 +-
> ...ler-automatic-realview.aarch64-latest.args | 2 +-
> ...ller-automatic-realview.aarch64-latest.xml | 2 +-
> ...r-automatic-versatilepb.armv7l-latest.args | 2 +-
> ...er-automatic-versatilepb.armv7l-latest.xml | 2 +-
> ...ault-fallback-realview.aarch64-latest.args | 2 +-
> ...fault-fallback-realview.aarch64-latest.xml | 2 +-
> ...ontroller-default-mac99ppc.ppc-latest.args | 2 +-
> ...controller-default-mac99ppc.ppc-latest.xml | 2 +-
> ...ler-default-versatilepb.armv7l-latest.args | 2 +-
> ...ller-default-versatilepb.armv7l-latest.xml | 2 +-
> tests/qemuxmlconftest.c | 16 +++++------
> 28 files changed, 46 insertions(+), 50 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index df64ddbec1..2c7d91f597 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4334,7 +4334,7 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef
> *def,
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> }
>
> - if (def->os.arch == VIR_ARCH_AARCH64) {
> + if (ARCH_IS_ARM(def->os.arch)) {
> /* Prefer USB3 */
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> @@ -4366,14 +4366,17 @@ qemuDomainDefaultUSBControllerModel(const
> virDomainDef *def,
> 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))
> + if (ARCH_IS_X86(def->os.arch)) {
> + /* Use piix3-uhci (USB1) for backwards compatibility */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> + }
> +
> + /* Most common architectures and machine types have been already
> + * handled above; for the remaining cases, pci-ohci (USB1) is a
> + * sensible enough fallback */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
>
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
This hunk is partially what I'd want to see in patch 26 (making x86 a
separate case with explicit logic).
> @@ -4425,13 +4428,6 @@ qemuDomainDefaultUSBControllerModelAutoAdded(const
> virDomainDef *def,
> }
> }
>
> - if (ARCH_IS_ARM(def->os.arch)) {
> - if (STREQ(def->os.machine, "versatilepb") ||
> - STRPREFIX(def->os.machine, "realview-eb"))
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
> - return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
> - }
> -
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
> }
[...]
> diff --git
> a/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.args
> b/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.args
> index a05a413290..88fe2b62e8 100644
> --- a/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.args
> +++ b/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.args
> @@ -26,7 +26,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armv7ltest/.config \
> -rtc base=utc \
> -no-shutdown \
> -boot strict=on \
> --device '{"driver":"pci-ohci","id":"usb","bus":"pci","addr":"0x1"}' \
> +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci","addr":"0x1"}' \
This change seems to have happened also in code paths not allowing ABI
update at least according to the filename.
> -audiodev '{"id":"audio1","driver":"none"}' \
> -sandbox
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> -msg timestamp=on
> diff --git
> a/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.xml
> b/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.xml
> index 7b21b24927..a9da2a3b13 100644
> --- a/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.xml
> +++ b/tests/qemuxmlconfdata/armv7l-versatilepb-minimal.armv7l-latest.xml
> @@ -17,7 +17,7 @@
> <on_crash>destroy</on_crash>
> <devices>
> <emulator>/usr/bin/qemu-system-armv7l</emulator>
> - <controller type='usb' index='0' model='pci-ohci'>
> + <controller type='usb' index='0' model='qemu-xhci'>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> function='0x0'/>
> </controller>
> <controller type='pci' index='0' model='pci-root'/>
> diff --git a/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.args
> b/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.args
> index 8032ad7f0e..bdc86620c8 100644
> --- a/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.args
> +++ b/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.args
> @@ -29,7 +29,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
> -initrd /media/ram/ramdisk \
> -append 'root=/dev/ram rw console=ttyS0,115200' \
> -dtb /media/ram/test.dtb \
> --device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1"}' \
> +-device '{"driver":"pci-ohci","id":"usb","bus":"pci.0","addr":"0x1"}' \
> -chardev pty,id=charserial0 \
> -serial chardev:charserial0 \
> -audiodev '{"id":"audio1","driver":"none"}' \
> diff --git a/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.xml
> b/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.xml
> index 400f749eb0..31fcc3d053 100644
> --- a/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.xml
> +++ b/tests/qemuxmlconfdata/ppc-dtb.ppc-latest.xml
> @@ -18,7 +18,7 @@
> <on_crash>destroy</on_crash>
> <devices>
> <emulator>/usr/bin/qemu-system-ppc</emulator>
> - <controller type='usb' index='0' model='piix3-uhci'>
> + <controller type='usb' index='0' model='pci-ohci'>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> function='0x0'/>
> </controller>
> <controller type='pci' index='0' model='pci-root'/>
> diff --git
> a/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.args
> b/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.args
> index 8600eec328..9c7c884c83 100644
> --- a/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.args
> +++ b/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.args
> @@ -25,7 +25,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
> -rtc base=utc \
> -no-shutdown \
> -boot strict=on \
> --device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1"}' \
> +-device '{"driver":"pci-ohci","id":"usb","bus":"pci.0","addr":"0x1"}' \
> -audiodev '{"id":"audio1","driver":"none"}' \
> -device
> '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
> -sandbox
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> diff --git
> a/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.xml
> b/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.xml
> index 215c196fbf..633aa684da 100644
> --- a/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.xml
> +++ b/tests/qemuxmlconfdata/ppc-mac99-minimal.ppc-latest.abi-update.xml
> @@ -14,7 +14,7 @@
> <on_crash>destroy</on_crash>
> <devices>
> <emulator>/usr/bin/qemu-system-ppc</emulator>
> - <controller type='usb' index='0' model='piix3-uhci'>
> + <controller type='usb' index='0' model='pci-ohci'>
I think I need to think about this a bit more. I agree that the
controller we picked didn't make sense for this machine, but in cases
when it did work (e.g. when you run linux with the proper driver) you
get something which doesn't resemble real hardware but likely works
better than 'pci-ohci'.
So I'm not sure about the downgrade in this case, although we're
unlikely to break anything that'd be used widely in this case, breaking
it would go against our philosophy
