On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
> In addition to the code in qemuDomainControllerDefPostParse(),
> which we have just factored into its own function, we also have
> some code in qemuDomainDefAddDefaultDevices() that deals with
> choosing the model for a USB controller, specifically for q35
> guests. Integrate it into the newly-created function.
>
> Since we want slightly different behaviors depending on whether
> the USB controller that we're working on is the one that we try
> to automatically add for certain new guests (addDefaultUSB),
> introduce a new parameter to the function and a new possible
> return value.
>
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
> src/qemu/qemu_domain.c | 74 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 56 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 09f572b0b5..d992b51877 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4151,9 +4151,35 @@ qemuDomainDefaultSCSIControllerModel(const
> virDomainDef *def,
> }
>
>
> +/**
> + * qemuDomainDefaultUSBControllerModel:
> + * @def: domain definition
> + * @cont: controller definition, or NULL
> + * @autoAdded: whether this controller is being automatically added
> + * @qemuCaps: QEMU capabilities, or NULL
> + * @parseFlags: parse flags
> + *
> + * Choose a reasonable model to use for a USB controller where a
> + * specific one hasn't been provided by the user.
> + *
> + * The choice is based on a number of factors, including the guest's
> + * architecture and machine type. @qemuCaps, if provided, might be
> + * taken into consideration too.
> + *
> + * @autoAdded should be true is this is a controller that libvirt is
> + * trying to automatically add on domain creation for the user's
> + * convenience: in that case, the function might decide to simply not
> + * add the controller instead of reporting a failure.
> + *
> + * Returns: >=0 (a virDomainControllerModelUSB value) on success,
> + * -1 on error, and
This is NOT an error and is misrepresenting the _DEFAULT case which has
-1, which is also a success case at least in some situations.
> + * -2 if no suitable model could be found but it's okay to
> + * just skip the controller altogether.
IMO this should be VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE and not a new
arbitrary value. I also don't think that this function needs to know
whether the controller was auto-added, or not
> + */
> static int
> qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
> const virDomainControllerDef *cont,
> + bool autoAdded,
> virQEMUCaps *qemuCaps,
> unsigned int parseFlags)
> {
> @@ -4169,7 +4195,7 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef
> *def,
> * See qemuBuildControllersCommandLine() */
>
> if (ARCH_IS_S390(def->os.arch)) {
> - if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> + if (cont && cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> /* set the default USB model to none for s390 unless an
> * address is found */
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> @@ -4198,6 +4224,22 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef
> *def,
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> }
>
> + if (ARCH_IS_X86(def->os.arch)) {
> + if (qemuDomainIsQ35(def) && autoAdded) {
> + /* Prefer adding a USB3 controller if supported, fall back
> + * to USB2 if there is no USB3 available, and if that's
> + * unavailable don't add anything.
> + */
> + 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;
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
> + return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
> + return -2;
> + }
> + }
> +
> /* Default USB controller is piix3-uhci if available. */
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> @@ -4209,7 +4251,8 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef
> *def,
> static int
> qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> virDomainDef *def,
> - virQEMUCaps *qemuCaps)
> + virQEMUCaps *qemuCaps,
> + unsigned int parseFlags)
> {
> bool addDefaultUSB = false;
> int usbModel = -1; /* "default for machinetype" */
> @@ -4243,20 +4286,6 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> addPCIeRoot = true;
> addImplicitSATA = true;
> addITCOWatchdog = true;
> -
> - /* Prefer adding a USB3 controller if supported, fall back
> - * to USB2 if there is no USB3 available, and if that's
> - * unavailable don't add anything.
> - */
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
> - usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
> - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
> - usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
> - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
> - usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
> - else
> - addDefaultUSB = false;
> - break;
> }
> if (qemuDomainIsI440FX(def))
> addPCIRoot = true;
> @@ -4348,6 +4377,15 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> break;
> }
>
> + if (addDefaultUSB) {
> + usbModel = qemuDomainDefaultUSBControllerModel(def, NULL, true,
> qemuCaps, parseFlags);
> + /* A return value of -2 indicates that no reasonable default
> + * could be figured out, and that we should handle that by
> + * not adding the USB controller */
> + if (usbModel == -2)
> + addDefaultUSB = false;
> + }
> +
> if (addDefaultUSB &&
> virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0
> &&
> virDomainDefAddUSBController(def, 0, usbModel) < 0)
> @@ -5091,7 +5129,7 @@ qemuDomainDefPostParse(virDomainDef *def,
> if (qemuDomainDefBootPostParse(def, driver, parseFlags) < 0)
> return -1;
>
> - if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0)
> + if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps, parseFlags) <
> 0)
> return -1;
>
> if (qemuDomainDefSetDefaultCPU(def, driver->hostarch, qemuCaps) < 0)
> @@ -5695,7 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef
> *cont,
>
> case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
> qemuCaps) {
> - cont->model = qemuDomainDefaultUSBControllerModel(def, cont,
> qemuCaps, parseFlags);
> + cont->model = qemuDomainDefaultUSBControllerModel(def, cont,
> false, qemuCaps, parseFlags);
The code can check here explicitly whether _NONE was returned and
report appropriate error.
> }
> /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */
> if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 ||
> --
> 2.43.0
> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
ws> To unsubscribe send an email to devel-le...@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
