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(a)redhat.com&gt; --- 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

On Thu, Apr 27, 2017 at 08:14:17PM +0200, Andrea Bolognani wrote: That would change the output of this code, this patch preserver the ouput, just changes the logic to be easier to follow. 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

This is a USB3 controller and it's a better choice than piix3-uhci. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 3 +++ .../qemuxml2argv-aarch64-usb-controller-nec-xhci.args | 19 +++++++++++++++++++ .../qemuxml2argv-aarch64-usb-controller-nec-xhci.xml | 16 ++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 4 files changed, 42 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5cdb99cb4b..6fc8c29f3e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3225,6 +3225,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, /* Explicitly fallback to legacy USB controller for PPC64. */ cont->model = -1; } + } else if (def->os.arch == VIR_ARCH_AARCH64) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; } } /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.args new file mode 100644 index 0000000000..e97431f8be --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.args @@ -0,0 +1,19 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name QEMUGuest1 \ +-S \ +-M virt \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device nec-usb-xhci,id=usb,bus=pcie.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.xml b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.xml new file mode 100644 index 0000000000..1b7320f433 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-nec-xhci.xml @@ -0,0 +1,16 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c1b014b7df..ad05122436 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2469,6 +2469,10 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST("aarch64-usb-controller-nec-xhci", + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_NEC_USB_XHCI); + DO_TEST_PARSE_FLAGS_ERROR("missing-machine", VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, NONE); -- 2.12.2

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1438682 Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- New in v2: - documentation fixed - <currentMemory> removed from text XMLs - reduced one file by using link docs/formatdomain.html.in | 4 ++-- docs/schemas/domaincommon.rng | 1 + src/conf/domain_addr.c | 1 + src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 8 ++++++-- src/qemu/qemu_domain.c | 12 +++++++----- src/qemu/qemu_domain_address.c | 1 + .../qemuxml2argv-usb-controller-qemu-xhci-limit.xml | 14 ++++++++++++++ ...uxml2argv-usb-controller-qemu-xhci-unavailable.xml | 1 + .../qemuxml2argv-usb-controller-qemu-xhci.args | 19 +++++++++++++++++++ .../qemuxml2argv-usb-controller-qemu-xhci.xml | 14 ++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 13 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e31a271a58..e458d2f447 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3463,8 +3463,8 @@ <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci", "nec-xhci", "qusb1" (xen pvusb - with qemu backend, version 1.1) or "qusb2" (xen pvusb with qemu - backend, version 2.0). Additionally, + with qemu backend, version 1.1), "qusb2" (xen pvusb with qemu + backend, version 2.0) or "qemu-xhci". Additionally, <span class="since">since 0.10.0</span>, if the USB bus needs to be explicitly disabled for the guest, <code>model='none'</code> may be used. <span class="since">Since 1.0.5</span>, no default diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eb4b0f7437..86309b2c3a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1877,6 +1877,7 @@ <value>none</value> <value>qusb1</value> <value>qusb2</value> + <value>qemu-xhci</value> </choice> </attribute> </optional> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8b61091996..639168effa 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1673,6 +1673,7 @@ virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont) return 3; case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: if (cont->opts.usbopts.ports != -1) return cont->opts.usbopts.ports; return 4; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61006dea7d..3ba570e0a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -366,6 +366,7 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, "nec-xhci", "qusb1", "qusb2", + "qemu-xhci", "none") VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3b6b174516..31c7a924d7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -735,6 +735,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI, VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1, VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2, + VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI, VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1116d2cd5b..df03fa275d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(qemuControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, "nec-usb-xhci", "qusb1", "qusb2", + "qemu-xhci", "none"); VIR_ENUM_DECL(qemuDomainFSDriver) @@ -2558,6 +2559,8 @@ qemuControllerModelUSBToCaps(int model) return QEMU_CAPS_PCI_OHCI; case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: return QEMU_CAPS_NEC_USB_XHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: + return QEMU_CAPS_DEVICE_QEMU_XHCI; default: return -1; } @@ -2592,8 +2595,9 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, virBufferAsprintf(buf, "%s", smodel); if (def->opts.usbopts.ports != -1) { - if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI || - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI_PORTS)) { + if ((model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI_PORTS)) && + model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("usb controller type %s doesn't support 'ports' " "with this QEMU binary"), smodel); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6fc8c29f3e..2d14f47f28 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3171,7 +3171,7 @@ qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm) } -#define QEMU_USB_NEC_XHCI_MAXPORTS 15 +#define QEMU_USB_XHCI_MAXPORTS 15 static int @@ -3239,11 +3239,13 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, virDomainVirtTypeToString(def->virtType)); return -1; } - if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI && - cont->opts.usbopts.ports > QEMU_USB_NEC_XHCI_MAXPORTS) { + if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) && + cont->opts.usbopts.ports > QEMU_USB_XHCI_MAXPORTS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("nec-xhci controller only supports up to %u ports"), - QEMU_USB_NEC_XHCI_MAXPORTS); + _("'%s' controller only supports up to '%u' ports"), + virDomainControllerModelUSBTypeToString(cont->model), + QEMU_USB_XHCI_MAXPORTS); return -1; } break; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 064d05079c..3da6b7369d 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -502,6 +502,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_CONTROLLER_TYPE_USB: switch ((virDomainControllerModelUSB) cont->model) { case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: return pcieFlags; case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml new file mode 100644 index 0000000000..27cc99127f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml @@ -0,0 +1,14 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='qemu-xhci' ports='16'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml new file mode 120000 index 0000000000..989306fa7f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml @@ -0,0 +1 @@ +qemuxml2argv-usb-controller-qemu-xhci.xml \ No newline at end of file diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.args new file mode 100644 index 0000000000..4f967b7b22 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.args @@ -0,0 +1,19 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device qemu-xhci,p2=8,p3=8,id=usb,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.xml new file mode 100644 index 0000000000..b63f9e1c40 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.xml @@ -0,0 +1,14 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='qemu-xhci' ports='8'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ad05122436..8ad3838059 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1416,6 +1416,10 @@ mymain(void) DO_TEST_PARSE_ERROR("usb-controller-xhci-limit", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_NEC_USB_XHCI_PORTS); + DO_TEST("usb-controller-qemu-xhci", QEMU_CAPS_DEVICE_QEMU_XHCI); + DO_TEST_FAILURE("usb-controller-qemu-xhci-unavailable", NONE); + DO_TEST_PARSE_ERROR("usb-controller-qemu-xhci-limit", + QEMU_CAPS_DEVICE_QEMU_XHCI); DO_TEST("smbios", QEMU_CAPS_SMBIOS_TYPE); DO_TEST_PARSE_ERROR("smbios-date", QEMU_CAPS_SMBIOS_TYPE); -- 2.12.2

On Thu, Apr 27, 2017 at 08:40:22PM +0200, Andrea Bolognani wrote: No, this is not just what file should be used, it's also a name of the test case and it should be unique. It was clear the first time, but it was not a good suggestion so I've used link instead. Pavel

On Fri, Apr 28, 2017 at 10:07:43AM +0200, Andrea Bolognani wrote: I didn't know that there is already a case where we use the name twice so I didn't thought about that at all, otherwise I would let you know, I just thought that it was a honest mistake, not an intention :). Thanks Pavel

On Thu, 2017-04-27 at 18:09 +0200, Pavel Hrdina wrote: Please add QEMU_CAPS_NEC_USB_XHCI to both tests in order to show that qemu-xhci is being preferred to nec-xhci even though both are available. --  Andrea Bolognani / Red Hat / Virtualization