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
