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.
> 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.
I'll get back to that tomorrow.
>
> Unsurprisingly, a number of test cases start failing, or fail
> in a different way, because of this change. That's okay: even
> in the unlikely event that there actually are any existing
> guests with such problematic configurations out there, they
> will not disappear but merely fail to start, and the user will
> have the opportunity to fix them.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 68 +------------------
> src/qemu/qemu_validate.c | 68 ++++++++++++++++++-
> ...ault-unavailable-i440fx.x86_64-latest.args | 33 ---------
> ...fault-unavailable-i440fx.x86_64-latest.err | 1 +
> ...fault-unavailable-i440fx.x86_64-latest.xml | 31 ---------
> ...-default-unavailable-q35.x86_64-latest.xml | 33 ---------
> ...ler-nec-xhci-unavailable.x86_64-latest.xml | 33 ---------
> .../usb-legacy-device.x86_64-latest.args | 33 ---------
> .../usb-legacy-device.x86_64-latest.err | 1 +
> .../usb-legacy-device.x86_64-latest.xml | 30 --------
> .../usb-legacy-multiple.x86_64-latest.err | 2 +-
> .../usb-legacy-multiple.x86_64-latest.xml | 32 ---------
> tests/qemuxmlconftest.c | 12 ++--
> 13 files changed, 79 insertions(+), 298 deletions(-)
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args
> create mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
> create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 73afd094a9..ad1621a120 100644
> --- a/src/qemu/qemu_validate.c
> +++ 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?
> +{
> + switch (model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> + return QEMU_CAPS_PIIX3_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> + return QEMU_CAPS_PIIX4_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> + return QEMU_CAPS_USB_EHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> + return QEMU_CAPS_ICH9_USB_EHCI1;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> + return QEMU_CAPS_VT82C686B_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> + return QEMU_CAPS_PCI_OHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> + return QEMU_CAPS_NEC_USB_XHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
> + return QEMU_CAPS_DEVICE_QEMU_XHCI;
> + default:
> + return -1;
In case you'll need a catch-all use QEMU_CAPS_LAST as "error"
> + }
> +}
> +
> +
> +static int
> +qemuValidateDomainDeviceDefControllerUSB(const virDomainControllerDef *def,
> + virQEMUCaps *qemuCaps)
> +{
> + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> + return 0;
> +
> + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("no model provided for USB controller"));
> + return -1;
> + }
> +
> + if (!virQEMUCapsGet(qemuCaps, qemuControllerModelUSBToCaps(def->model))) {
Return value is used directly as capability flag.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("USB controller model '%1$s' not supported in this QEMU binary"),
> + virDomainControllerModelUSBTypeToString(def->model));
> + return -1;
> + }
> +
> + if (def->opts.usbopts.ports != -1) {
> + if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI &&
> + def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("usb controller type '%1$s' doesn't support 'ports' with this QEMU binary"),
> + virDomainControllerModelUSBTypeToString(def->model));
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
[...]
> --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 \
> --msg timestamp=on
> diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> new file mode 100644
> index 0000000000..7a71aa107d
> --- /dev/null
> +++ b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> @@ -0,0 +1 @@
> +unsupported configuration: no model provided for USB controller
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.
The change itself makes very much sense to me at least for the 'x86'
machines, as the '-usb' reallistically won't ever be used. I'll have a
look at the possible implications for other machines, but as such I like
the direction of this patch, thus once you reword the commit message:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
