The new logic will set the piix3-uhci if available regardless of
any architecture and it will be updated to better model based on
architecture and device existence.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
src/qemu/qemu_domain.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6824af565b..5cdb99cb4b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3200,11 +3200,17 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
* when the relevant device is not available.
*
* See qemuBuildControllerDevCommandLine() */
- if (ARCH_IS_S390(def->os.arch) &&
- cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
- /* set the default USB model to none for s390 unless an
- * address is found */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
+
+ /* Default USB controller is piix3-uhci if available. */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+
+ if (ARCH_IS_S390(def->os.arch)) {
+ if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+ /* set the default USB model to none for s390 unless an
+ * address is found */
+ 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.
@@ -3215,11 +3221,10 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
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 = -1;
}
- } else {
- /* Default USB controller for anything else is piix3-uhci */
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
}
}
/* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */
--
2.12.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-04-27 at 18:09 +0200, Pavel Hrdina wrote:
> @@ -3200,11 +3200,17 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
> * when the relevant device is not available.
> *
> * See qemuBuildControllerDevCommandLine() */
> - if (ARCH_IS_S390(def->os.arch) &&
> - cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> - /* set the default USB model to none for s390 unless an
> - * address is found */
> - cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> +
> + /* Default USB controller is piix3-uhci if available. */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> +
> + if (ARCH_IS_S390(def->os.arch)) {
> + if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> + /* set the default USB model to none for s390 unless an
> + * address is found */
> + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> + }
You should add an else branch where you unset cont->model
explicitly, like you do for ppc64 below, otherwise s390
guests will start being assigned piix3-uhci controllers in
some situations.
We don't have checks for that in our test suite, and it's
probably not going to be much of an issue in the real world
at least as far as downstream distros are concerned, but it
should be avoided nonetheless.
Overall I'm not convinced this is any more readable than
what we had before or that it makes subsequent changes
any easier, so I'd prefer if you'd just drop this patch.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 27, 2017 at 08:14:17PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-04-27 at 18:09 +0200, Pavel Hrdina wrote:
> > @@ -3200,11 +3200,17 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
> > * when the relevant device is not available.
> > *
> > * See qemuBuildControllerDevCommandLine() */
> > - if (ARCH_IS_S390(def->os.arch) &&
> > - cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > - /* set the default USB model to none for s390 unless an
> > - * address is found */
> > - cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> > +
> > + /* Default USB controller is piix3-uhci if available. */
> > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> > + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> > +
> > + if (ARCH_IS_S390(def->os.arch)) {
> > + if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > + /* set the default USB model to none for s390 unless an
> > + * address is found */
> > + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> > + }
>
> You should add an else branch where you unset cont->model
> explicitly, like you do for ppc64 below, otherwise s390
> guests will start being assigned piix3-uhci controllers in
> some situations.
That would change the output of this code, this patch preserver
the ouput, just changes the logic to be easier to follow.
> We don't have checks for that in our test suite, and it's
> probably not going to be much of an issue in the real world
> at least as far as downstream distros are concerned, but it
> should be avoided nonetheless.
>
>
> Overall I'm not convinced this is any more readable than
> what we had before or that it makes subsequent changes
> any easier, so I'd prefer if you'd just drop this patch.
The issue with original code is that the if else-if else condition
is not consistent.
The first if checks S390 and address type together, however the second
else-if checks only for PPC64, the capability checks are inside that block.
So if arch is S390 but the address type is different from
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE it continues to the last else block and
sets the model to piix3-uhci it it's available.
The second else-if for PPC64 doesn't fallback to piix3-uhci it the
architecture is PPC64 but none of the capabilities from that block
are set.
This patch changes the logic and also makes the if else-if more clearer
so the first check is only for architecture and after that in each block
we test for capabilities.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-04-27 at 20:58 +0200, Pavel Hrdina wrote: > The issue with original code is that the if else-if else condition > is not consistent. > > The first if checks S390 and address type together, however the second > else-if checks only for PPC64, the capability checks are inside that block. > > So if arch is S390 but the address type is different from > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE it continues to the last else block and > sets the model to piix3-uhci it it's available. > > The second else-if for PPC64 doesn't fallback to piix3-uhci it the > architecture is PPC64 but none of the capabilities from that block > are set. > > This patch changes the logic and also makes the if else-if more clearer > so the first check is only for architecture and after that in each block > we test for capabilities. I see now that I was mistaken and your change is correct. Sorry for the noise. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.