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 <abologna@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
+ * -2 if no suitable model could be found but it's okay to
+ * just skip the controller altogether.
+ */
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);
}
/* 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
To unsubscribe send an email to devel-leave@lists.libvirt.org
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 <abologna@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-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Jan 25, 2024 at 03:00:10PM +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
> > +/**
> > + * 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.
Fair enough.
> I also don't think that this function needs to know
> whether the controller was auto-added, or not
It does though. For q35, we need to handle two cases:
1. a USB controller was present in the input XML, with no model
provided;
2. a USB controller was NOT present in the input XML, but we're
trying to add one by default.
The outcome is different, as can be seen by comparing the output
files for the relevant default-models and minimal tests:
1. the controller will be piix3-uhci;
2. the controller will be qemu-xhci.
To complicate things further, the behavior is also different if the
necessary devices are not available in the QEMU binary:
1. an error will be reported;
2. the controller will simply not be added.
Personally I think that this is borderline madness, but it is the
current behavior and with this series I'm only trying to reorganize
things, not change them, at least until the last few patches.
So, the additional argument is necessary in order to know which one
of the two behaviors is desired/expected by the caller.
Of course we didn't have this problem when we had two distinct chunks
of code dealing with this default, but that came with its own issues
and overall this approach seems preferable to me.
> > + * -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.
Technically, yeah, that would work. But mostly by chance.
Right now the -2 return value is only treated differently in the
context of the auto-added USB controller, which is something that we
don't do for s390x guests. But for those, NONE is a valid return
value that needs to be handled by the other caller of the helper...
Making the two overlap would IMO make the code more fragile against
future changes, however unlikely they might be.
And yes, that's another piece of borderline madness that however we
have to preserve :)
> > @@ -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.
This is exactly the place where we need to make sure that we do *not*
consider NONE an error, because of s390x.
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Jan 25, 2024 at 10:59:29 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 03:00:10PM +0100, Peter Krempa wrote:
> > On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
> > > +/**
> > > + * 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.
>
> Fair enough.
>
> > I also don't think that this function needs to know
> > whether the controller was auto-added, or not
>
> It does though. For q35, we need to handle two cases:
>
> 1. a USB controller was present in the input XML, with no model
> provided;
>
> 2. a USB controller was NOT present in the input XML, but we're
> trying to add one by default.
>
> The outcome is different, as can be seen by comparing the output
> files for the relevant default-models and minimal tests:
>
> 1. the controller will be piix3-uhci;
>
> 2. the controller will be qemu-xhci.
>
> To complicate things further, the behavior is also different if the
> necessary devices are not available in the QEMU binary:
>
> 1. an error will be reported;
>
> 2. the controller will simply not be added.
>
> Personally I think that this is borderline madness, but it is the
> current behavior and with this series I'm only trying to reorganize
> things, not change them, at least until the last few patches.
>
> So, the additional argument is necessary in order to know which one
> of the two behaviors is desired/expected by the caller.
Oh, that's indeed madness. I didn't realize originally that there's a
difference between auto-selecting a model for an existing controller and
auto-selecting a model for an auto-added controller.
This obviously makes sense in the extent of the existing logic.
> Of course we didn't have this problem when we had two distinct chunks
> of code dealing with this default, but that came with its own issues
> and overall this approach seems preferable to me.
>
> > > + * -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.
>
> Technically, yeah, that would work. But mostly by chance.
>
> Right now the -2 return value is only treated differently in the
> context of the auto-added USB controller, which is something that we
> don't do for s390x guests. But for those, NONE is a valid return
> value that needs to be handled by the other caller of the helper...
>
> Making the two overlap would IMO make the code more fragile against
> future changes, however unlikely they might be.
>
> And yes, that's another piece of borderline madness that however we
> have to preserve :)
>
> > > @@ -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.
>
> This is exactly the place where we need to make sure that we do *not*
> consider NONE an error, because of s390x.
As long as you properly document the return values and mention the
actual enum value name in addition to -1:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2024 Red Hat, Inc.