[libvirt] [PATCH 0/3] add support for qemu-xhci USB controller
Pavel Hrdina (3): qemu: introduce QEMU_CAPS_DEVICE_QEMU_XHCI qemu: add support for qemu-xhci USB controller qemu: use qemu-xhci USB controller by default for ppc64 and aarch64 docs/formatdomain.html.in | 2 +- 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_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 8 +++++-- src/qemu/qemu_domain.c | 25 ++++++++++++++------ src/qemu/qemu_domain_address.c | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + ...uxml2argv-aarch64-usb-controller-qemu-xhci.args | 20 ++++++++++++++++ ...muxml2argv-aarch64-usb-controller-qemu-xhci.xml | 27 ++++++++++++++++++++++ ...emuxml2argv-ppc64-usb-controller-qemu-xhci.args | 19 +++++++++++++++ ...qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml | 1 + ...qemuxml2argv-usb-controller-qemu-xhci-limit.xml | 15 ++++++++++++ ...l2argv-usb-controller-qemu-xhci-unavailable.xml | 15 ++++++++++++ .../qemuxml2argv-usb-controller-qemu-xhci.args | 19 +++++++++++++++ .../qemuxml2argv-usb-controller-qemu-xhci.xml | 15 ++++++++++++ tests/qemuxml2argvtest.c | 12 ++++++++++ 20 files changed, 177 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.args create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml create mode 100644 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 -- 2.12.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cc3e1f8084..9a97c18d47 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -364,6 +364,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-cpu-definitions", /* 250 */ "block-write-threshold", "query-named-block-nodes", + "qemu-xhci", ); @@ -1609,6 +1610,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vhost-scsi", QEMU_CAPS_DEVICE_VHOST_SCSI }, { "nvdimm", QEMU_CAPS_DEVICE_NVDIMM }, { "pcie-root-port", QEMU_CAPS_DEVICE_PCIE_ROOT_PORT }, + { "qemu-xhci", QEMU_CAPS_DEVICE_QEMU_XHCI }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index a7eec52a94..9c96e22acf 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -401,6 +401,7 @@ typedef enum { QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */ QEMU_CAPS_BLOCK_WRITE_THRESHOLD, /* BLOCK_WRITE_THRESHOLD event */ QEMU_CAPS_QUERY_NAMED_BLOCK_NODES, /* qmp query-named-block-nodes */ + QEMU_CAPS_DEVICE_QEMU_XHCI, /* -device qemu-xhci */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index 6386c4ed0d..f48975e598 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -207,6 +207,7 @@ <flag name='query-cpu-definitions'/> <flag name='block-write-threshold'/> <flag name='query-named-block-nodes'/> + <flag name='qemu-xhci'/> <version>2008090</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0-rc0-142-g940a8ce)</package> -- 2.12.2
On Thu, 2017-04-20 at 15:44 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+)
ACK -- Andrea Bolognani / Red Hat / Virtualization
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1438682 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 2 +- 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 | 15 +++++++++++++++ ...uxml2argv-usb-controller-qemu-xhci-unavailable.xml | 15 +++++++++++++++ .../qemuxml2argv-usb-controller-qemu-xhci.args | 19 +++++++++++++++++++ .../qemuxml2argv-usb-controller-qemu-xhci.xml | 15 +++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 13 files changed, 87 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml create mode 100644 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 b1e38f00e4..4e2fef2c00 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3464,7 +3464,7 @@ "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, + backend, version 2.0), 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 edc225fe50..9d87088a5a 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 705deb39a1..97c48335a9 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 7da554f8ee..d958364072 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -734,6 +734,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 b2e76ca373..248ff4dcf8 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 b3e1573c69..2c17abaf84 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3135,7 +3135,7 @@ qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm) } -#define QEMU_USB_NEC_XHCI_MAXPORTS 15 +#define QEMU_USB_XHCI_MAXPORTS 15 static int @@ -3195,11 +3195,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..d7b6947aad --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-limit.xml @@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <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 100644 index 0000000000..78d13e0c1e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml @@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <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/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..78d13e0c1e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci.xml @@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <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 c1b014b7df..e58a72120e 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, 2017-04-20 at 15:44 +0200, Pavel Hrdina wrote: [...]
@@ -3464,7 +3464,7 @@ "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, + backend, version 2.0), qemu-xhci. Additionally,
Add double quotes around qemu-xhci to conform with the rest of the documentation. Additionally, having "nec-xhci", "qusb1" or "qusb2", "qemu-xhci" looks weird, I think you should change it to "nec-xhci", "qusb1", "qusb2" or "qemu-xhci" I won't get into the whole Oxford comma argument ;) [...]
@@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory>
I like having minimal input files for test cases! You can make them even smaller, without reducing the test surface, by getting rid of the <currentMemory> element... [...]
new file mode 100644 index 0000000000..78d13e0c1e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-controller-qemu-xhci-unavailable.xml
... and you can get rid of this file altogether by using...
@@ -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);
... "usb-controller-qemu-xhci" again for this test case. ACK with these issues fixed. -- Andrea Bolognani / Red Hat / Virtualization
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1438682 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++-- ...uxml2argv-aarch64-usb-controller-qemu-xhci.args | 20 ++++++++++++++++ ...muxml2argv-aarch64-usb-controller-qemu-xhci.xml | 27 ++++++++++++++++++++++ ...emuxml2argv-ppc64-usb-controller-qemu-xhci.args | 19 +++++++++++++++ ...qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml | 1 + tests/qemuxml2argvtest.c | 8 +++++++ 6 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.args create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2c17abaf84..d2e76dbbe4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3172,14 +3172,23 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, } 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 controller is used as default only for - * newly defined domains or devices. */ + * 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 if (def->os.arch == VIR_ARCH_AARCH64 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) { + /* If possible use qemu-xhci as default controller for + * aarch64, it's USB3 controller and we want to avoid using + * nec-usb-xhci. */ + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; } else { /* Default USB controller for anything else is piix3-uhci */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.args new file mode 100644 index 0000000000..ab6284470d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.args @@ -0,0 +1,20 @@ +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 qemu-xhci,id=usb,bus=pcie.0,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pcie.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml new file mode 100644 index 0000000000..ebadce1af6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml @@ -0,0 +1,27 @@ +<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='aarch64' machine='virt'>hvm</type> + </os> + <features> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <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='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.args b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.args new file mode 100644 index 0000000000..0f19eb7e66 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-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-ppc64 \ +-name QEMUGuest1 \ +-S \ +-M pseries \ +-m 256 \ +-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 \ +-boot c \ +-device qemu-xhci,id=usb,bus=pci.0,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml new file mode 120000 index 0000000000..831d9d4f1c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ppc64-usb-controller-qemu-xhci.xml @@ -0,0 +1 @@ +qemuxml2argv-ppc64-usb-controller.xml \ No newline at end of file diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e58a72120e..2f3eec0c22 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2472,6 +2472,14 @@ mymain(void) QEMU_CAPS_PCI_OHCI); DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", NULL, -1, 0, + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, GIC_NONE, + QEMU_CAPS_DEVICE_QEMU_XHCI); + + DO_TEST("aarch64-usb-controller-qemu-xhci", + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_QEMU_XHCI); DO_TEST_PARSE_FLAGS_ERROR("missing-machine", VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, -- 2.12.2
[David and Drew added to CC, feel free to skip to the bottom] On Thu, 2017-04-20 at 15:44 +0200, Pavel Hrdina wrote: [...]
+ } else if (def->os.arch == VIR_ARCH_AARCH64 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) { + /* If possible use qemu-xhci as default controller for + * aarch64, it's USB3 controller and we want to avoid using + * nec-usb-xhci. */ + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
If qemu-xhci is not available but usb-nec-xhci is, it would probably be nicer to pick the latter rather than leaving it unspecified (which will result in an error later on). So I would basically use the same code as ppc64 here, minus the pci-ohci part of course. [...]
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-usb-controller-qemu-xhci.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory>
You don't need <currentMemory>, ... [...]
+ <features> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash>
... you can remove all these...
+ <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='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon>
... and set the model for <memballoon> to 'none'. [...]
@@ -2472,6 +2472,14 @@ mymain(void) QEMU_CAPS_PCI_OHCI); DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", NULL, -1, 0, + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, GIC_NONE, + QEMU_CAPS_DEVICE_QEMU_XHCI); + + DO_TEST("aarch64-usb-controller-qemu-xhci", + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_DEVICE_IOH3420,
You don't need QEMU_CAPS_DEVICE_IOH3420 here. The rest looks good, but I'd like to make sure changing the default is something that's okay with everyone. David, Drew, do you have anything against changing the default USB controller model for ppc64 and aarch64 guests respectively to qemu-xhci? -- Andrea Bolognani / Red Hat / Virtualization
[*actually* added David and Drew to CC smh] On Wed, 2017-04-26 at 18:13 +0200, Andrea Bolognani wrote:
The rest looks good, but I'd like to make sure changing the default is something that's okay with everyone.
David, Drew, do you have anything against changing the default USB controller model for ppc64 and aarch64 guests respectively to qemu-xhci?
-- Andrea Bolognani / Red Hat / Virtualization
On Wed, 26 Apr 2017 18:20:24 +0200 Andrea Bolognani <abologna@redhat.com> wrote:
[*actually* added David and Drew to CC smh]
On Wed, 2017-04-26 at 18:13 +0200, Andrea Bolognani wrote:
The rest looks good, but I'd like to make sure changing the default is something that's okay with everyone.
David, Drew, do you have anything against changing the default USB controller model for ppc64 and aarch64 guests respectively to qemu-xhci?
No objection here. We do want to make the decision one way or another ASAP, so QE know what to test (they've already asked about testing nec-xhci versus qemu-xhci). -- David Gibson <dgibson@redhat.com> Principal Software Engineer, Virtualization, Red Hat
On Thu, Apr 27, 2017 at 11:14:42AM +1000, David Gibson wrote:
On Wed, 26 Apr 2017 18:20:24 +0200 Andrea Bolognani <abologna@redhat.com> wrote:
[*actually* added David and Drew to CC smh]
On Wed, 2017-04-26 at 18:13 +0200, Andrea Bolognani wrote:
The rest looks good, but I'd like to make sure changing the default is something that's okay with everyone.
David, Drew, do you have anything against changing the default USB controller model for ppc64 and aarch64 guests respectively to qemu-xhci?
No objection here. We do want to make the decision one way or another ASAP, so QE know what to test (they've already asked about testing nec-xhci versus qemu-xhci).
No objection, and actually the contrary. I would object to using nec-xhci on mach-virt, when the more general qemu-xhci is available. Thanks, drew
On Wed, Apr 26, 2017 at 06:13:44PM +0200, Andrea Bolognani wrote:
[David and Drew added to CC, feel free to skip to the bottom]
On Thu, 2017-04-20 at 15:44 +0200, Pavel Hrdina wrote: [...]
+ } else if (def->os.arch == VIR_ARCH_AARCH64 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) { + /* If possible use qemu-xhci as default controller for + * aarch64, it's USB3 controller and we want to avoid using + * nec-usb-xhci. */ + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
If qemu-xhci is not available but usb-nec-xhci is, it would probably be nicer to pick the latter rather than leaving it unspecified (which will result in an error later on).
This is not true, if the qemu-xhci is not available libvirt will set piix3-uhci if available as a default for aarch64. Only in case that piix3-uhci is not available it will fail.
So I would basically use the same code as ppc64 here, minus the pci-ohci part of course.
That should be addressed by a separate patch. Pavel
On Wed, 2017-04-26 at 18:59 +0200, Pavel Hrdina wrote:
If qemu-xhci is not available but usb-nec-xhci is, it would probably be nicer to pick the latter rather than leaving it unspecified (which will result in an error later on). This is not true, if the qemu-xhci is not available libvirt will set piix3-uhci if available as a default for aarch64. Only in case that piix3-uhci is not available it will fail.
You're technically correct[1]. However, piix3-uhci is another piece of Intel-derived hardware so in practice qemu-system-aarch64 is very unlikely to have it compiled in and most users will end up getting the error instead.
So I would basically use the same code as ppc64 here, minus the pci-ohci part of course. That should be addressed by a separate patch.
Agreed. I suggest adding a patch at the beginning of the series that makes nec-xhci the default for aarch64 if available as in ppc64, and then make qemu-xhci the new default for both in this patch. Would that work for you? [1] Which is the best kind of correct! ;) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 27, 2017 at 09:42:24AM +0200, Andrea Bolognani wrote:
On Wed, 2017-04-26 at 18:59 +0200, Pavel Hrdina wrote:
If qemu-xhci is not available but usb-nec-xhci is, it would probably be nicer to pick the latter rather than leaving it unspecified (which will result in an error later on). This is not true, if the qemu-xhci is not available libvirt will set piix3-uhci if available as a default for aarch64. Only in case that piix3-uhci is not available it will fail.
You're technically correct[1]. However, piix3-uhci is another piece of Intel-derived hardware so in practice qemu-system-aarch64 is very unlikely to have it compiled in and most users will end up getting the error instead.
Isn't the nec-xhci also Intel hardware, so the same would apply to that controller as well. Moreover, this probably happens only for downstream builds of QEMU (most likely only RHEL/CentOS) as there is no configure option for that. QEMU has some default configs for different architectures but in upstream QEMU the set of UHCI usb controllers is enabled by default for aarch64.
So I would basically use the same code as ppc64 here, minus the pci-ohci part of course. That should be addressed by a separate patch.
Agreed. I suggest adding a patch at the beginning of the series that makes nec-xhci the default for aarch64 if available as in ppc64, and then make qemu-xhci the new default for both in this patch. Would that work for you?
I don't mind if we add the same logic for ppc64 and aarch64, so I'll send a v2 with this change included. Thanks for review. Pavel
[1] Which is the best kind of correct! ;) -- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-04-27 at 10:24 +0200, Pavel Hrdina wrote:
You're technically correct[1]. However, piix3-uhci is another piece of Intel-derived hardware so in practice qemu-system-aarch64 is very unlikely to have it compiled in and most users will end up getting the error instead. Isn't the nec-xhci also Intel hardware, so the same would apply to that controller as well.
Not quite: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] [8086:7020] NEC Corporation uPD720200 USB 3.0 Host Controller [1033:0194] The vendor for piix3-uhci is Intel Corporation, the vendor for nec-xhci is NEC Corporation. I guess both are unlikely to show up in actual aarch64 hardware, but the former is clearly x86-specific while the latter is somewhat more architecture-agnostic.
Moreover, this probably happens only for downstream builds of QEMU (most likely only RHEL/CentOS) as there is no configure option for that. QEMU has some default configs for different architectures but in upstream QEMU the set of UHCI usb controllers is enabled by default for aarch64.
The upstream QEMU configuration takes the kitchen sink approach, eg. qemu-system-ppc64 will include allwinner-ahci and other devices that clearly have no place in a ppc64 guest, so I don't think we should take that as an indication that piix-uhci is something anyone will want to reasonably use on aarch64 :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 27, 2017 at 04:13:43PM +0200, Andrea Bolognani wrote:
On Thu, 2017-04-27 at 10:24 +0200, Pavel Hrdina wrote:
You're technically correct[1]. However, piix3-uhci is another piece of Intel-derived hardware so in practice qemu-system-aarch64 is very unlikely to have it compiled in and most users will end up getting the error instead. Isn't the nec-xhci also Intel hardware, so the same would apply to that controller as well.
Not quite:
Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] [8086:7020] NEC Corporation uPD720200 USB 3.0 Host Controller [1033:0194]
The vendor for piix3-uhci is Intel Corporation, the vendor for nec-xhci is NEC Corporation.
I guess both are unlikely to show up in actual aarch64 hardware, but the former is clearly x86-specific while the latter is somewhat more architecture-agnostic.
Moreover, this probably happens only for downstream builds of QEMU (most likely only RHEL/CentOS) as there is no configure option for that. QEMU has some default configs for different architectures but in upstream QEMU the set of UHCI usb controllers is enabled by default for aarch64.
The upstream QEMU configuration takes the kitchen sink approach, eg. qemu-system-ppc64 will include allwinner-ahci and other devices that clearly have no place in a ppc64 guest, so I don't think we should take that as an indication that piix-uhci is something anyone will want to reasonably use on aarch64 :)
My point was that we should not make any decisions based on downstream configurations :). Like I said, I'll send v2 with nec-xhci enabled for aarch64. Pavel
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Andrea Bolognani -
Andrew Jones -
David Gibson -
Pavel Hrdina