2:23 a.m.
On Thu, Feb 15, 2024 at 08:46:17 -0800, Andrea Bolognani wrote:
On Thu, Feb 15, 2024 at 05:01:44PM +0100, Peter Krempa wrote:
> On Wed, Feb 14, 2024 at 18:11:18 +0100, Andrea Bolognani wrote:
> > Right now we call qemuValidateDomainDeviceDefControllerUSB()
> > quite late, just as we're generating the QEMU command line.
> >
> > The intention here is to prevent configurations from being
> > rejected, even though a default USB controller model could not
> > be found, because using -usb could work as a last resort.
> >
> > As it turns out, this premise is flawed, as can easily be
> > demonstrated by using a build of QEMU which has the default
> > USB controller compiled out:
> >
> > $ qemu-system-x86_64 -M pc -device piix3-usb-uhci
> > 'piix3-usb-uhci' is not a valid device model name
> >
> > $ qemu-system-x86_64 -M pc -usb
> > missing object type 'piix3-usb-uhci'
>
> Could you please elaborate how you've got this? I had to disable the
> CONFIG_I440FX=n option to achieve that (without messing with the machine
> dependencies in the first place) which also means that that the 'pc'
> machine type will not work:
>
> 'piix3-usb-uhci' is provided by 'hw/usb/hcd-uhci.c', which is
> conditionally compiled based on the follwing rule:
>
> system_ss.add(when: 'CONFIG_USB_UHCI', if_true:
files('hcd-uhci.c'))
>
> USB_UHCI is selected(required) by 'config PIIX' (hw/isa/Kconfig).
>
> PIIX is selected by 'config I440FX'.
>
> Thus it is impossible to build qemu with a 'pc' machine type but
> missing 'piix3-usb-uhci.
>
> By breaking the 'config PIIX' option you can achieve that.
>
> Thus by definition all 'pc' machine-type based devices must have the
> 'piix3-usb-uhci' type compiled in.
>
> And which thus implies that '-usb' will never be used in such case.
The exact diff is at the bottom of the message, but yeah, I've had to
hack things up quite a bit just to convince QEMU's build system to
produce a binary that contains the pc machine type but no
piix3-usb-uhci device.
To be honest I don't understand QEMU's build system very well, so
it's possible that the only reason why I had to go to such lengths is
lack of knowledge. The experience you describe above seems to suggest
otherwise though :)
TBH, it wasn't exactly straightforward at first. Until I've understood
the relationship between "config USB_UHCI" and CONFIG_USB_UHCI, as it
was un-greppable before. After that it is relatively staightforward as
the hierarchy of declarations can be explored easily.
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'
I'll look a bit more deeply about other machines arches we care about to
see if we can justify this nicely.
I very much love to see that the '-usb' stuff can be deleted.
> > In other words, if the device that needs to be built into
QEMU
> > in order for -usb to work was available, we would have detected
> > it and used it via -device instead; if we didn't, using -usb
> > won't change anything.
>
> So if the above is the case, I'd rephrase this paragraph to say that
> qemu can't be built without the explicitly detectable controller.
>
> Now the question is only whether that applies for qemu-4.2 and other
> machines. as well. But if yes, all the -usb thing can be removed.
Note that we're discussing two slightly different claims:
* that -usb cannot work when the default USB controller has been
compiled out;
* 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.
Anyway, we can just ask an actual QEMU developer to confirm.
> > +++ b/src/qemu/qemu_validate.c
> > @@ -3523,6 +3523,69 @@ qemuValidateDomainDeviceDefControllerSCSI(const
virDomainControllerDef *controll
> > }
> >
> >
> > +static int
> > +qemuControllerModelUSBToCaps(int model)
>
> 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.
> The summary and first paragraph of the commit message makes it
seem that
> this is just moving when the validation happens but the commit is in
> fact severly reworking the semantics of the validation too. Please
> clarify that in the commit message; especially the summary.
Okay, I'll try to come up with something that does a better job at
explaining the changes.