Re: [PATCH v2 11/17] qemu: Validate USB controllers earlier
On Fri, Feb 16, 2024 at 09:23:12AM +0100, Peter Krempa wrote:
I'll investigate a bit more in terms of implications to machines.
One
thing I've observed is that e.g. with q35, which doesn't require you
breaking the machine type definitions you can get a qemu without the
UHCI subsystem by setting just:
CONFIG_ISAPC=n
CONFIG_I440FX=n
CONFIG_USB_UHCI=n
Which then fails with similar error as above:
$ ./build/qemu-system-x86_64 -M q35 -usb
qemu-system-x86_64: could not find a module for type 'ich9-usb-uhci1'
Interestingly, when I apply that configuration I get a slighly
different behavior:
$ ./build/qemu-system-x86_64 -M q35 -usb
qemu-system-x86_64: unknown type 'ich9-usb-uhci1'
Aborted (core dumped)
You've made those changes to configs/devices/i386-softmmu/default.mak,
right? I wonder why our builds don't act the same.
> Note that we're discussing two slightly different claims:
>
> 1) that -usb cannot work when the default USB controller has been
> compiled out;
>
> 2) that the default USB controller cannot be compiled out.
>
> Either one being true across architectures and machine types is
> sufficient ground for dropping -usb usage from libvirt. I'm quite
> convinced that the former always holds true, and that's what I was
> referring to in the commit message.
IMO we should only ever consider stuff that is possible to achieve with
qemu. If it's impossible to cleanly compile out the default USB
controller that is the main point our deductions should revolve around
whether if the default USB is always compiled in the 'fallback' means
make any sense, and not the other way around.
Well, just above you have demonstrated that it is indeed possible to
create a QEMU binary that includes a machine type but not the device
used by its default USB controller, exclusively through the standard
configuration mechanisms offered by QEMU's build system. So that
already tells us that claim (2) does not hold.
Which is fine, because at least so far (1) still does and either one
allows us to drop -usb from libvirt.
> > Any reason why 'model' doesn't use the proper
type:
> > virDomainControllerModelUSB and the return value type isn't
virQEMUCapsFlags?
>
> I've just moved the code exacty as is, modulo adding the
>
> if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> return 0;
>
> bit without which it would clearly not work correctly.
>
> I can add a further commit that cleans things up a bit.
In a commit that clearly modifies the logic, reasoning that it's just a
code move is IMO not appropriate. If logic is being modified the code
should be brought up to standards right away, thus squash in that
cleanup.
Sure :)
--
Andrea Bolognani / Red Hat / Virtualization