Extract the logic from qemuDomainControllerDefPostParse() to
a dedicated helper. The behavior is unchanged.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/qemu/qemu_domain.c | 82 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 3 ++
src/qemu/qemu_postparse.c | 51 +-----------------------
3 files changed, 86 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 186e36da03..f03624eda8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4294,6 +4294,88 @@ qemuDomainDefaultPanicModel(const virDomainDef *def)
}
+/**
+ * qemuDomainDefaultUSBControllerModel:
+ * @def: domain definition
+ * @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.
+ *
+ * The return value can be a specific controller model, or
+ * VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT; the latter indicates that
+ * no suitable model could be identified. How to behave in that
+ * scenario is entirely up to the caller.
+ *
+ * Returns: the model
+ */
+virDomainControllerModelUSB
+qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags)
+{
+ virDomainControllerModelUSB model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+
+ /* Pick a suitable default model for the USB controller if none
+ * has been selected by the user and we have the qemuCaps for
+ * figuring out which controllers are supported.
+ *
+ * We rely on device availability instead of setting the model
+ * unconditionally because, for some machine types, there's a
+ * chance we will get away with using the legacy USB controller
+ * when the relevant device is not available.
+ *
+ * See qemuBuildControllersCommandLine() */
+
+ /* Default USB controller is piix3-uhci if available. Fall back to
+ * 'pci-ohci' otherwise which is the default for non-x86 machines
+ * which honour -usb */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+ else if (!ARCH_IS_X86(def->os.arch) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
+
+ if (ARCH_IS_S390(def->os.arch)) {
+ /* No default model on s390x, one has to be provided
+ * explicitly by the user */
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
+ } else if (ARCH_IS_PPC64(def->os.arch)) {
+ /* To not break migration we need to set default USB controller
+ * for ppc64 to pci-ohci if we cannot change ABI of the VM.
+ * The nec-usb-xhci or qemu-xhci controller is used as default
+ * only for newly defined domains or devices. */
+ if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
+ } else {
+ /* Explicitly fallback to legacy USB controller for PPC64. */
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ }
+ } else if (def->os.arch == VIR_ARCH_AARCH64) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ } else if (ARCH_IS_LOONGARCH(def->os.arch)) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ }
+
+ return model;
+}
+
+
/**
* qemuDomainDefNumaCPUsRectify:
* @numa: pointer to numa definition
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ffe5bee1bf..bdcfcc6e86 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -842,6 +842,9 @@ bool qemuDomainSupportsPCI(const virDomainDef *def);
bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
virDomainControllerModelSCSI qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
virQEMUCaps *qemuCaps);
+virDomainControllerModelUSB qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags);
int qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
virDomainDef *def);
diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
index cbef101104..46ba12cd4a 100644
--- a/src/qemu/qemu_postparse.c
+++ b/src/qemu/qemu_postparse.c
@@ -359,56 +359,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) {
- /* Pick a suitable default model for the USB controller if none
- * has been selected by the user and we have the qemuCaps for
- * figuring out which controllers are supported.
- *
- * We rely on device availability instead of setting the model
- * unconditionally because, for some machine types, there's a
- * chance we will get away with using the legacy USB controller
- * when the relevant device is not available.
- *
- * See qemuBuildControllersCommandLine() */
-
- /* Default USB controller is piix3-uhci if available. Fall back to
- * 'pci-ohci' otherwise which is the default for non-x86 machines
- * which honour -usb */
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
- else if (!ARCH_IS_X86(def->os.arch) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
-
- if (ARCH_IS_S390(def->os.arch)) {
- /* No default model on s390x, one has to be provided
- * explicitly by the user */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- } else if (ARCH_IS_PPC64(def->os.arch)) {
- /* To not break migration we need to set default USB controller
- * for ppc64 to pci-ohci if we cannot change ABI of the VM.
- * The nec-usb-xhci or qemu-xhci controller is used as default
- * only for newly defined domains or devices. */
- if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
- } else {
- /* Explicitly fallback to legacy USB controller for PPC64. */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
- }
- } else if (def->os.arch == VIR_ARCH_AARCH64) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- } else if (ARCH_IS_LOONGARCH(def->os.arch)) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- }
+ cont->model = qemuDomainDefaultUSBControllerModel(def, qemuCaps, parseFlags);
}
/* Make sure the 'none' USB controller doesn't have an address
--
2.50.1On Tue, Aug 19, 2025 at 18:22:28 +0200, Andrea Bolognani via Devel wrote: > Extract the logic from qemuDomainControllerDefPostParse() to > a dedicated helper. The behavior is unchanged. > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > --- > src/qemu/qemu_domain.c | 82 +++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 3 ++ > src/qemu/qemu_postparse.c | 51 +----------------------- > 3 files changed, 86 insertions(+), 50 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 186e36da03..f03624eda8 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -4294,6 +4294,88 @@ qemuDomainDefaultPanicModel(const virDomainDef *def) > } > > > +/** > + * qemuDomainDefaultUSBControllerModel: > + * @def: domain definition > + * @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. This statement is a bit misleading. In cases where qemuCaps is NULL you must not be doing any decisions that depend on the caps that wouldn't be undone by another run of this function with caps present. In cases where the caps for given qemu are not present and the XML parsed is one of the persistent configs loaded from disk, the postparse code will be re-tried at next startup attempt where it has to be present for the VM to work. If you'd pick an incorrect model based on missing caps that'd be remembered and not fixed on startup, but it would behave differently if the caps are present.
On Thu, Sep 18, 2025 at 03:57:10PM +0200, Peter Krempa wrote: > On Tue, Aug 19, 2025 at 18:22:28 +0200, Andrea Bolognani via Devel wrote: > > Extract the logic from qemuDomainControllerDefPostParse() to > > a dedicated helper. The behavior is unchanged. > > I don't see it changed anywhere else ... is it just to separate stuff? I'm not sure I understand the question. Yes, this commit is intended to merely factor out the existing code into a dedicated helper, without altering the behavior. Then later commits perform the modifications we want. > > +/** > > + * qemuDomainDefaultUSBControllerModel: > > + * @def: domain definition > > + * @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. > > This statement is a bit misleading. In cases where qemuCaps is NULL you > must not be doing any decisions that depend on the caps that wouldn't be > undone by another run of this function with caps present. > > In cases where the caps for given qemu are not present and the XML > parsed is one of the persistent configs loaded from disk, the postparse > code will be re-tried at next startup attempt where it has to be present > for the VM to work. > > If you'd pick an incorrect model based on missing caps that'd be > remembered and not fixed on startup, but it would behave differently if > the caps are present. Got it. In practice things should already work correctly with the current code, and continue to do so even after my patches, since neither this helper nor its AutoAdded variant are ever called when qemuCaps is NULL. Based on this and the fact that you provided a R-b for the patch, it should be enough for me to update the comment so that it's not misleading, right? Basically s/, if provided, might/will/g -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Sep 19, 2025 at 07:26:21 -0700, Andrea Bolognani wrote: > On Thu, Sep 18, 2025 at 03:57:10PM +0200, Peter Krempa wrote: > > On Tue, Aug 19, 2025 at 18:22:28 +0200, Andrea Bolognani via Devel wrote: > > > Extract the logic from qemuDomainControllerDefPostParse() to > > > a dedicated helper. The behavior is unchanged. > > > > I don't see it changed anywhere else ... is it just to separate stuff? > > I'm not sure I understand the question. > > Yes, this commit is intended to merely factor out the existing code > into a dedicated helper, without altering the behavior. Then later > commits perform the modifications we want. I was asking for the reason for exporting it since it's used in single place. Especially since you put it in a different module so it needs to be exported. Extracting is fine, but extracting to a different module is a bit weird if you don't reuse it. > > > +/** > > > + * qemuDomainDefaultUSBControllerModel: > > > + * @def: domain definition > > > + * @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. > > > > This statement is a bit misleading. In cases where qemuCaps is NULL you > > must not be doing any decisions that depend on the caps that wouldn't be > > undone by another run of this function with caps present. > > > > In cases where the caps for given qemu are not present and the XML > > parsed is one of the persistent configs loaded from disk, the postparse > > code will be re-tried at next startup attempt where it has to be present > > for the VM to work. > > > > If you'd pick an incorrect model based on missing caps that'd be > > remembered and not fixed on startup, but it would behave differently if > > the caps are present. > > Got it. In practice things should already work correctly with the > current code, and continue to do so even after my patches, since > neither this helper nor its AutoAdded variant are ever called when > qemuCaps is NULL. > > Based on this and the fact that you provided a R-b for the patch, it > should be enough for me to update the comment so that it's not > misleading, right? Basically s/, if provided, might/will/g Yes, updating the comment is sufficient. It needs to mention that 'qemuCaps' *might* be NULL, in which case it must not try to infer any information that something is *not* supported because it actually may be, thus must pick the default that makes us re-check.
On Fri, Sep 19, 2025 at 04:31:07PM +0200, Peter Krempa wrote: > On Fri, Sep 19, 2025 at 07:26:21 -0700, Andrea Bolognani wrote: > > Yes, this commit is intended to merely factor out the existing code > > into a dedicated helper, without altering the behavior. Then later > > commits perform the modifications we want. > > I was asking for the reason for exporting it since it's used in single > place. Especially since you put it in a different module so it needs to > be exported. > > Extracting is fine, but extracting to a different module is a bit weird > if you don't reuse it. I moved it next to the other functions that pick default models, namely SCSI controllers and panic devices. Admittedly the helper for network devices is in qemu_postparse.c, so we're not entirely consistent. Big surprise :) I can keep the new helpers in the current file if you prefer, but I think in the long run it would be better to have them all in one place, and since the SCSI one is called outside of qemu_postparse.c too it seems that qemu_domain.c is a better location. But we can have that conversation at a later date. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Aug 19, 2025 at 18:22:28 +0200, Andrea Bolognani via Devel wrote: > Extract the logic from qemuDomainControllerDefPostParse() to > a dedicated helper. The behavior is unchanged. I don't see it changed anywhere else ... is it just to separate stuff? > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > --- > src/qemu/qemu_domain.c | 82 +++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 3 ++ > src/qemu/qemu_postparse.c | 51 +----------------------- > 3 files changed, 86 insertions(+), 50 deletions(-) Reviewed-by: Peter Krempa <pkrempa@redhat.com>
© 2016 - 2026 Red Hat, Inc.