[libvirt] [PATCH] Fix USB model defaults for ppc64

The condition was checking for UHCI (and OHCI for ppc64) availability so that it can specify the proper device instead of legacy usb. However, for ppc64, we don't need to check both OHCI and UHCI, but only OHCI as that is the legacy default. The condition is so big that it was just a matter of time when someone will make a mistake there, so let's use more lines so that it is visible what the condition checks for. This fixes usage of -device instead of -usb for ppc64 that supports pci-usb-ohci and does not support piix3-usb-uhci. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 34 +++++++++++++++------- .../qemuxml2argv-ppc64-usb-controller-legacy.args | 20 +++++++++++++ .../qemuxml2argv-ppc64-usb-controller-legacy.xml | 1 + .../qemuxml2argv-ppc64-usb-controller.args | 20 +++++++++++++ .../qemuxml2argv-ppc64-usb-controller.xml | 28 ++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 6 files changed, 95 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1f0593526b55..3f464a5fc21d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10049,18 +10049,30 @@ qemuBuildCommandLine(virConnectPtr conn, if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && - !qemuDomainMachineIsQ35(def) && - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI) && - ARCH_IS_PPC64(def->os.arch)))) { - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); - goto error; + !qemuDomainMachineIsQ35(def)) { + bool need_legacy = false; + + /* We're not using legacy usb controller for q35 */ + if (ARCH_IS_PPC64(def->os.arch)) { + /* For ppc64 the legacy was OHCI */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) + need_legacy = true; + } else { + /* For anything else, we used * PIIX3_USB_UHCI */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) + need_legacy = true; + } + + if (need_legacy) { + if (usblegacy) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported")); + goto error; + } + usblegacy = true; + continue; } - usblegacy = true; - continue; } virCommandAddArg(cmd, "-device"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args new file mode 100644 index 000000000000..3dee41e86481 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args @@ -0,0 +1,20 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-system-ppc64 \ +-name QEMUGuest1 \ +-S \ +-M pseries \ +-m 256 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-boot c \ +-usb \ +-net none \ +-serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.xml b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.xml new file mode 120000 index 000000000000..831d9d4f1c1b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.xml @@ -0,0 +1 @@ +qemuxml2argv-ppc64-usb-controller.xml \ No newline at end of file diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args new file mode 100644 index 000000000000..3dee41e86481 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args @@ -0,0 +1,20 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-system-ppc64 \ +-name QEMUGuest1 \ +-S \ +-M pseries \ +-m 256 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-boot c \ +-usb \ +-net none \ +-serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.xml b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.xml new file mode 100644 index 000000000000..ce180ee34989 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 63480ce8af19..1476def2f57d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1764,6 +1764,9 @@ mymain(void) QEMU_CAPS_VIRTIO_MOUSE, QEMU_CAPS_VIRTIO_TABLET); DO_TEST("virtio-input-passthrough", QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_INPUT_HOST); + DO_TEST("ppc64-usb-controller", QEMU_CAPS_PCI_OHCI); + DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.7.0

On Fri, Jan 08, 2016 at 01:53:59PM +0100, Martin Kletzander wrote:
The condition was checking for UHCI (and OHCI for ppc64) availability so that it can specify the proper device instead of legacy usb. However, for ppc64, we don't need to check both OHCI and UHCI, but only OHCI as that is the legacy default. The condition is so big that it was just a matter of time when someone will make a mistake there, so let's use more lines so that it is visible what the condition checks for.
This fixes usage of -device instead of -usb for ppc64 that supports pci-usb-ohci and does not support piix3-usb-uhci.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 34 +++++++++++++++------- .../qemuxml2argv-ppc64-usb-controller-legacy.args | 20 +++++++++++++ .../qemuxml2argv-ppc64-usb-controller-legacy.xml | 1 + .../qemuxml2argv-ppc64-usb-controller.args | 20 +++++++++++++ .../qemuxml2argv-ppc64-usb-controller.xml | 28 ++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 6 files changed, 95 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.xml
This is perfect example of how to do things wrong if you forget something. As you can see, I created the *.args files to be the same and wanted to change the non-legacy one after everything compiles and the test fails. But I forgot about that and after I've done some additional changes to the code, I forgot that I need to tune one of the files and because none of the tests failed. And of course it did not, because I forgot to add the QEMU_CAPS_DEVICE capability. So whenever someone will be reviewing this, the following diff needs to be considered part of the patch. It is already squashed into my local commit. diff --git i/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args w/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args index 3dee41e86481..7d301f32bec7 100644 --- i/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args +++ w/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-legacy.args @@ -12,9 +12,8 @@ QEMU_AUDIO_DRV=none \ -smp 1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ +-nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait \ -boot c \ -usb \ --net none \ --serial none \ --parallel none +-device virtio-balloon-pci,id=balloon0,bus=pci,addr=0x6 diff --git i/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args w/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args index 3dee41e86481..f17a377dfd6f 100644 --- i/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args +++ w/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller.args @@ -12,9 +12,8 @@ QEMU_AUDIO_DRV=none \ -smp 1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ +-nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait \ -boot c \ --usb \ --net none \ --serial none \ --parallel none +-device pci-ohci,id=usb,bus=pci,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci,addr=0x6 diff --git i/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c index 1476def2f57d..e17dcc765870 100644 --- i/tests/qemuxml2argvtest.c +++ w/tests/qemuxml2argvtest.c @@ -1764,8 +1764,10 @@ mymain(void) QEMU_CAPS_VIRTIO_MOUSE, QEMU_CAPS_VIRTIO_TABLET); DO_TEST("virtio-input-passthrough", QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_INPUT_HOST); - DO_TEST("ppc64-usb-controller", QEMU_CAPS_PCI_OHCI); - DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST("ppc64-usb-controller", + QEMU_CAPS_DEVICE, QEMU_CAPS_PCI_OHCI); + DO_TEST("ppc64-usb-controller-legacy", + QEMU_CAPS_DEVICE, QEMU_CAPS_PIIX3_USB_UHCI); qemuTestDriverFree(&driver); -- Martin

On Fri, 2016-01-08 at 19:00 +0100, Martin Kletzander wrote:
So whenever someone will be reviewing this, the following diff needs to be considered part of the patch. It is already squashed into my local commit.
Already caught that, and I'm currently testing a version that includes pretty much the same diff squashed in. I'll let you know once I'm done :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Fri, 2016-01-08 at 13:53 +0100, Martin Kletzander wrote:
The condition was checking for UHCI (and OHCI for ppc64) availability so that it can specify the proper device instead of legacy usb. However, for ppc64, we don't need to check both OHCI and UHCI, but only OHCI as that is the legacy default. The condition is so big that it was just a matter of time when someone will make a mistake there, so let's use more lines so that it is visible what the condition checks for. This fixes usage of -device instead of -usb for ppc64 that supports pci-usb-ohci and does not support piix3-usb-uhci.
I've filed https://bugzilla.redhat.com/show_bug.cgi?id=1297020 to keep track of this, you might want to reference it in your commit message.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1f0593526b55..3f464a5fc21d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10049,18 +10049,30 @@ qemuBuildCommandLine(virConnectPtr conn, if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && - !qemuDomainMachineIsQ35(def) && - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI) && - ARCH_IS_PPC64(def->os.arch)))) { - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); - goto error; + !qemuDomainMachineIsQ35(def)) { + bool need_legacy = false; + + /* We're not using legacy usb controller for q35 */ + if (ARCH_IS_PPC64(def->os.arch)) { + /* For ppc64 the legacy was OHCI */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) + need_legacy = true; + } else { + /* For anything else, we used * PIIX3_USB_UHCI */
-----^ I guess you joined together two lines, please get rid of it. ACK with that fixed and the other diff[1] squashed in. Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-January/msg00248.html -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani
-
Martin Kletzander