On 07/01/2016 11:38 AM, Ján Tomko wrote:

We were requiring a USB port path in the schema, but not enforcing it. Omitting the USB port would lead to libvirt formatting it as (null). Such domain cannot be started and will disappear after libvirtd restart (since it cannot parse back the XML).

Only format the port if it has been specified and mark it as optional in the XML schema. --- docs/schemas/domaincommon.rng | 8 +++-- src/conf/domain_conf.c | 5 ++- src/qemu/qemu_command.c | 3 +- .../qemuxml2argv-usb-port-missing.args | 26 ++++++++++++++++ .../qemuxml2argv-usb-port-missing.xml | 25 +++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ .../qemuxml2xmlout-usb-port-missing.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-port-missing.xml

As pointed out to me during the LUKS changes, if your "data" XML is essentially the same as your output XML, you could make a file link from output to data. See the *gic* files for an example. All I did was copy the output to data, removed output, and then recreated output as file link to data. It's not a "requirement" and it doesn't really matter to me, but I think in the effort to reduce the size of things is what started that. Of course, it's hard to know without tripping across it a few times... As long as there's no ABI issues that you can come up with and you fix the .args file, then ACK John

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 563cb3c..0876daa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4055,9 +4055,11 @@ <attribute name="bus"> <ref name="usbAddr"/> </attribute> - <attribute name="port"> - <ref name="usbPort"/> - </attribute> + <optional> + <attribute name="port"> + <ref name="usbPort"/> + </attribute> + </optional> </define> <define name="spaprvioaddress"> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b88afbc..61ad136 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4859,9 +4859,8 @@ virDomainDeviceInfoFormat(virBufferPtr buf, break;

case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: - virBufferAsprintf(buf, " bus='%d' port='%s'", - info->addr.usb.bus, - info->addr.usb.port); + virBufferAsprintf(buf, " bus='%d'", info->addr.usb.bus); + virBufferEscapeString(buf, " port='%s'", info->addr.usb.port); break;

case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3898ed7..90b7684 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -375,7 +375,8 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, VIR_DOMAIN_CONTROLLER_TYPE_USB, info->addr.usb.bus))) goto cleanup; - virBufferAsprintf(buf, ",bus=%s.0,port=%s", contAlias, info->addr.usb.port); + virBufferAsprintf(buf, ",bus=%s.0", contAlias); + virBufferEscapeString(buf, ",port=%s", info->addr.usb.port); } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { if (info->addr.spaprvio.has_reg) virBufferAsprintf(buf, ",reg=0x%llx", info->addr.spaprvio.reg); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.args new file mode 100644 index 0000000..d43c58d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \

This will "fail" now without the ",sockets=1,cores=1,threads=1" due to commit id 'e114b09157'

+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-device usb-hub,id=hub0,bus=usb.0 \ +-device usb-hub,id=hub1,bus=usb.0 \ +-device usb-mouse,id=input0,bus=usb.0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.xml new file mode 100644 index 0000000..593fcd1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-missing.xml @@ -0,0 +1,25 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + <input type='mouse' bus='usb'> + <address type='usb' bus='0'/> + </input> + <hub type='usb'> + <address type='usb' bus='0'/> + </hub> + <hub type='usb'> + <address type='usb' bus='0'/> + </hub> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a73db5e..4389e24 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1159,6 +1159,9 @@ mymain(void) DO_TEST("usb-hub", QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); + DO_TEST("usb-port-missing", + QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, + QEMU_CAPS_NODEFCONFIG); DO_TEST("usb-ports", QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-port-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-port-missing.xml new file mode 100644 index 0000000..2e29cbd --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-port-missing.xml @@ -0,0 +1,36 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='usb'> + <address type='usb' bus='0'/> + </input> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hub type='usb'> + <address type='usb' bus='0'/> + </hub> + <hub type='usb'> + <address type='usb' bus='0'/> + </hub> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7db9cb7..c6ef28c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -535,6 +535,7 @@ mymain(void) DO_TEST("interface-server"); DO_TEST("virtio-lun");

+ DO_TEST("usb-port-missing"); DO_TEST("usb-redir"); DO_TEST("usb-redir-filter"); DO_TEST("usb-redir-filter-version");

On Wed, Jul 13, 2016 at 06:43:51PM -0400, John Ferlan wrote:

On 07/01/2016 11:38 AM, Ján Tomko wrote:

...

Walk through all the usb hubs in the domain definition that have a USB address specified, create the corresponding structures in the virDomainUSBAddressSet and mark the port it occupies as used. --- src/conf/domain_addr.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 82fe295..2c511ba 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1437,6 +1437,109 @@ virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs, }

+static ssize_t +virDomainUSBAddressGetLastIdx(virDomainDeviceInfoPtr info) +{ + ssize_t i; + for (i = VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH - 1; i > 0; i--) { + if (info->addr.usb.port[i] != 0) + break; + } + return i; +}

I would say the one thing that concerns me if some code calls virDomainUSBAddressSetAddHub or virDomainUSBAddressFindPort without first checking virDomainUSBAddressPortIsValid

...

+ + +/* Find the USBAddressHub structure representing the hub/controller + * that corresponds to the bus/port path specified by info. + * Returns the index of the requested port in targetIdx. + */ +static virDomainUSBAddressHubPtr +virDomainUSBAddressFindPort(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info, + int *targetIdx, + const char *portStr) +{ + virDomainUSBAddressHubPtr hub = NULL; + ssize_t i, lastIdx; + + if (info->addr.usb.bus >= addrs->nbuses || + !addrs->buses[info->addr.usb.bus]) { + virReportError(VIR_ERR_XML_ERROR, _("Missing USB bus %u"), + info->addr.usb.bus); + return NULL; + } + hub = addrs->buses[info->addr.usb.bus]; + + lastIdx = virDomainUSBAddressGetLastIdx(info);

Again to the same point if lastIdx == 0, then someone didn't check virDomainUSBAddressPortIsValid before calling... and there's some internal error, but I'm not sure it's worth checking... on the fence for now I guess.

I think checking it would be paranoid.

...

+ + for (i = 0; i < lastIdx; i++) { + /* ports are numbered from 1 */ + int portIdx = info->addr.usb.port[i] - 1; + + if (hub->nports <= portIdx) { + virReportError(VIR_ERR_XML_ERROR, + _("port %u out of range in USB address bus: %u port: %s"), + info->addr.usb.port[i], + info->addr.usb.bus, + portStr); + return NULL; + } + hub = hub->hubs[portIdx]; + } + + *targetIdx = info->addr.usb.port[lastIdx] - 1;

It almost feels like the caller should do this since it's the only one that cares, but that means the caller has not know about the addr.usb.port... It's a coin flip. Since you designed it this way, I'm OK with it as is.

All the callers care.

...

+ return hub; +} + + +#define VIR_DOMAIN_USB_HUB_PORTS 8 + +static int +virDomainUSBAddressSetAddHub(virDomainUSBAddressSetPtr addrs, + virDomainHubDefPtr hub) +{ + virDomainUSBAddressHubPtr targetHub = NULL, newHub = NULL; + int ret = -1; + int targetPort; + char *portStr = NULL; + + if (hub->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {

Oy - hub->type and hub->info.type... Seems there should be some other check somewhere in the code that would ensure that the <address> used for a <hub> was USB <sigh>

Currently, usb is the only possible hub type. Other address types get ignored by the condition below and are overwritten by a freshly assigned USB address later.

...

+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("Wrong address type for USB hub")); + goto cleanup; + } + + if (!(portStr = virDomainUSBAddressPortFormat(hub->info.addr.usb.port))) + goto cleanup; + + VIR_DEBUG("Adding a USB hub with 8 ports on bus=%u port=%s", + hub->info.addr.usb.bus, portStr); + + if (!(newHub = virDomainUSBAddressHubNew(VIR_DOMAIN_USB_HUB_PORTS))) + goto cleanup; + + if (!(targetHub = virDomainUSBAddressFindPort(addrs, &(hub->info), &targetPort, + portStr))) + goto cleanup; + + if (targetHub->hubs[targetPort]) { + virReportError(VIR_ERR_XML_ERROR, + _("Dupicate USB hub on bus %u port %s"), ^^^^^^^^ Duplicate

It could be a USB hub or another USB device, right? IOW should we say that the port is already in use by some other USB device.

At this point, only USB hubs have been added to the structure.

...

+ hub->info.addr.usb.bus, portStr); + goto cleanup; + } + ignore_value(virBitmapSetBit(targetHub->ports, targetPort)); + targetHub->hubs[targetPort] = newHub; + newHub = NULL; + + ret = 0; + cleanup: + virDomainUSBAddressHubFree(newHub); + VIR_FREE(portStr); + return ret; +} + + int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, virDomainDefPtr def) { @@ -1449,5 +1552,17 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, return -1; } } + + for (i = 0; i < def->nhubs; i++) { + virDomainHubDefPtr hub = def->hubs[i]; + if (hub->type == VIR_DOMAIN_HUB_TYPE_USB && + hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && + virDomainUSBAddressPortIsValid(hub->info.addr.usb.port)) { ^^^ This line ensures we have a port value from the XML; however, as pointed out above I'm hoping no other future caller will need to also check this. I'd almost say we should move that check inside the SetAddHub. Your call though as you know where this is headed (I'm thinking are there going to be issues with hotplug and/or migration).

The other usage will be adding a hub we've just created with an address we've generating. Jan

On 07/01/2016 11:38 AM, Ján Tomko wrote:

Check if they fit on the USB controllers the domain has, and error out if two devices try to use the same address. --- src/conf/domain_addr.c | 42 ++++++++++++++++++++++ src/conf/domain_addr.h | 4 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_domain_address.c | 38 +++++++++++++++++++- .../qemuxml2argv-usb-hub-conflict.xml | 22 ++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 7 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.xml

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 2c511ba..bc9dc59 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1566,3 +1566,45 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, } return 0; } + + +int +virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, + void *data) +{ + virDomainUSBAddressSetPtr addrs = data; + virDomainUSBAddressHubPtr targetHub = NULL; + char *portStr = NULL; + int ret = -1; + int targetPort; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) + return 0; + + if (!virDomainUSBAddressPortIsValid(info->addr.usb.port)) + return 0; + + portStr = virDomainUSBAddressPortFormat(info->addr.usb.port); + if (!portStr) + goto cleanup; + VIR_DEBUG("Reserving USB address bus=%u port=%s", info->addr.usb.bus, portStr); + + if (!(targetHub = virDomainUSBAddressFindPort(addrs, info, &targetPort, + portStr))) + goto cleanup; + + if (virBitmapIsBitSet(targetHub->ports, targetPort)) {

FWIW: Earlier comment I made in review of 3/9 about using portmap makes something like this a bit more readable, IMO.

+ virReportError(VIR_ERR_XML_ERROR, + _("Duplicate USB address bus %u port %s"), + info->addr.usb.bus, portStr); + goto cleanup; + } + + ignore_value(virBitmapSetBit(targetHub->ports, targetPort)); + + ret = 0; + + cleanup: + VIR_FREE(portStr); + return ret; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 4cb0212..4230ea2 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -275,4 +275,8 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);

+int +virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, + void *data) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

There's only 2 arguments.

#endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f0fed8e..f66ccf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -110,6 +110,7 @@ virDomainPCIControllerModelToConnectType; virDomainUSBAddressPortFormat; virDomainUSBAddressPortFormatBuf; virDomainUSBAddressPortIsValid; +virDomainUSBAddressReserve; virDomainUSBAddressSetAddControllers; virDomainUSBAddressSetCreate; virDomainUSBAddressSetFree; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c87dcc7..b71a28d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -190,6 +190,7 @@ struct _qemuDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; virDomainCCWAddressSetPtr ccwaddrs; virDomainVirtioSerialAddrSetPtr vioserialaddrs; + virDomainUSBAddressSetPtr usbaddrs;

virQEMUCapsPtr qemuCaps; char *lockState; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index ee44d45..f66b2f0 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1622,11 +1622,44 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, }

+static int +qemuDomainAssignUSBAddresses(virDomainDefPtr def, + virDomainObjPtr obj) +{ + int ret = -1; + virDomainUSBAddressSetPtr addrs = NULL; + qemuDomainObjPrivatePtr priv = NULL; + + if (!(addrs = virDomainUSBAddressSetCreate())) + goto cleanup; + + if (virDomainUSBAddressSetAddControllers(addrs, def) < 0) + goto cleanup; + + if (virDomainUSBDeviceDefForeach(def, virDomainUSBAddressReserve, addrs, + true) < 0) + goto cleanup; + + VIR_DEBUG("Existing USB addresses have been reserved"); + + if (obj && obj->privateData) { + priv = obj->privateData; + priv->usbaddrs = addrs;

Corresponding change in qemuDomainObjPrivateFree needs to be made.

+ addrs = NULL; + } + ret = 0; + + cleanup: + virDomainUSBAddressSetFree(addrs); + return ret; +} + + int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainObjPtr obj, - bool newDomain ATTRIBUTE_UNUSED) + bool newDomain) { if (qemuDomainAssignVirtioSerialAddresses(def, obj) < 0) return -1; @@ -1642,6 +1675,9 @@ qemuDomainAssignAddresses(virDomainDefPtr def, if (qemuDomainAssignPCIAddresses(def, qemuCaps, obj) < 0) return -1;

+ if (newDomain && qemuDomainAssignUSBAddresses(def, obj) < 0) + return -1; + return 0; }

Shouldn't qemuDomainReleaseDeviceAddress handle info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB? At the very least when hotplug comes along and something is unplugged, I'd expect the address being used to be free'd up. Maybe that's later, but since the function views the priv->{ccw|pci|vioserial}addrs, the usbaddrs should also be considered. ACK with the appropriate adjustments (two free calls and adjustment to prototype ATTRIBUTE_NONNULL. John

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.xml new file mode 100644 index 0000000..9a48ba0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.xml @@ -0,0 +1,22 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + <hub type='usb'> + <address type='usb' bus='0' port='1'/> + </hub> + <input type='mouse' bus='usb'> + <address type='usb' bus='0' port='1'/> + </input> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4389e24..9c18989 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1159,6 +1159,9 @@ mymain(void) DO_TEST("usb-hub", QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); + DO_TEST_PARSE_ERROR("usb-hub-conflict", + QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, + QEMU_CAPS_NODEFCONFIG); DO_TEST("usb-port-missing", QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG);

On 07/01/2016 11:38 AM, Ján Tomko wrote:

Automatically assign addresses to USB devices.

Just like reserving, this is only done for newly defined domains.

https://bugzilla.redhat.com/show_bug.cgi?id=1215968 --- src/conf/domain_addr.c | 125 ++++++++++++++++++++- src/conf/domain_addr.h | 14 +++ src/libvirt_private.syms | 3 + src/qemu/qemu_domain_address.c | 64 +++++++++++ .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-bios.args | 2 +- .../qemuxml2argv-controller-order.args | 8 +- .../qemuxml2argv-disk-usb-device-removable.args | 3 +- .../qemuxml2argv-disk-usb-device.args | 2 +- .../qemuxml2argv-graphics-spice-timeout.args | 2 +- .../qemuxml2argv-graphics-spice-usb-redir.args | 2 +- ...muxml2argv-hostdev-usb-address-device-boot.args | 2 +- .../qemuxml2argv-hostdev-usb-address-device.args | 2 +- .../qemuxml2argv-hostdev-usb-address.args | 2 +- .../qemuxml2argv-hugepages-numa.args | 6 +- .../qemuxml2argv-input-usbmouse.args | 2 +- .../qemuxml2argv-input-usbtablet.args | 2 +- .../qemuxml2argv-pseries-usb-kbd.args | 2 +- .../qemuxml2argv-serial-spiceport.args | 2 +- .../qemuxml2argv-smartcard-controller.args | 2 +- .../qemuxml2argv-smartcard-host-certificates.args | 2 +- .../qemuxml2argv-smartcard-host.args | 2 +- ...emuxml2argv-smartcard-passthrough-spicevmc.args | 2 +- .../qemuxml2argv-smartcard-passthrough-tcp.args | 2 +- .../qemuxml2argv-sound-device.args | 2 +- .../qemuxml2argv-usb-hub-conflict.args | 25 +++++ .../qemuxml2argv-usb-port-autoassign.args | 28 +++++ .../qemuxml2argv-usb-port-autoassign.xml | 27 +++++ .../qemuxml2argv-usb-redir-boot.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-usb-redir.args | 2 +- tests/qemuxml2argvtest.c | 3 + 31 files changed, 317 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml

Similar to 1/9, your .args file need ",sockets=1,cores=1,threads=1" due to commit id 'e114b09157'. To a degree I assume the values that were generated for each args file are what qemu "expected" (or perhaps generated before this change)? The qemuxml2argv-usb-port-autoassign - I believe uses a default USB controller - perhaps would be nice to have similar type tests for other specific controller models to show/ensure the port allocation algorithm does what is expected. Furthermore a test similar to qemuxml2argv-pci-bridge-many-disks.xml - that is something that will show progression through the algorithm in order to assign more than a few port values. Why would qemuxml2argv-usb-hub-conflict.args get generated?

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index bc9dc59..e1a8385 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1494,7 +1494,7 @@ virDomainUSBAddressFindPort(virDomainUSBAddressSetPtr addrs,

#define VIR_DOMAIN_USB_HUB_PORTS 8

-static int +int

So the radar goes up - thinking about comments from patch 5....

virDomainUSBAddressSetAddHub(virDomainUSBAddressSetPtr addrs, virDomainHubDefPtr hub) { @@ -1568,6 +1568,111 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, }

In my opinion - this could use some comments/details as to "how" a free port is determined. A future reader may wonder. For some .args file ports used are "1", "1.1", "1.2", and "2" for a usb controller with 2 ports per hub (qemuxml2argv-controller-order). While another has ports "1", "2", and "3" for a usb controller with 6 ports per hub (qemuxml2argv-hugepages-numa) - although I suppose that has more to do with usb-redir than anything. It's just that first glance wonder... I think I'm looking to understand what/how/why a 3 or 4 dot usb port would be generated. It seems as though the algorithm is designed to perhaps have 1 level of depth with a preference to go from "1.2" to "2" rather than "1.1" to "1.1.1". Hmm, aha... Maybe the depth has to do with the hub->ports bitmap being created using nports rather than factoring in the VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH?

+static int +virDomainUSBAddressFindFreePort(virDomainUSBAddressHubPtr hub, + unsigned int *portpath, + unsigned int level) +{ + unsigned int port; + ssize_t portIdx; + size_t i; + + /* Look for free ports on the current hub */ + if ((portIdx = virBitmapNextClearBit(hub->ports, -1)) >= 0) { + port = portIdx + 1; + VIR_DEBUG("Found a free port %u at level %u", port, level); + portpath[level] = port; + return 0; + } + + VIR_DEBUG("No ports found on hub %p, trying the hubs on it", hub); + + if (level >= VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH - 1) + return -1; + + /* Recursively search through the ports that contain another hub */ + for (i = 0; i < hub->nports; i++) { + if (!hub->hubs[i]) + continue; + + port = i + 1; + VIR_DEBUG("Looking at USB hub at level: %u port: %u", level, port); + if (virDomainUSBAddressFindFreePort(hub->hubs[i], portpath, + level + 1) < 0) + continue; + + portpath[level] = port; + return 0; + } + return -1; +} + +

More comments especially w/r/t return values and what they mean... Seems to me -2 means multiple things.

+static int +virDomainUSBAddressAssignFromBus(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info, + size_t bus) +{ + unsigned int portpath[VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH] = { 0 }; + virDomainUSBAddressHubPtr hub = addrs->buses[bus]; + char *portStr = NULL; + int ret = -1; + + if (!hub) + return -2; + + if (virDomainUSBAddressFindFreePort(hub, portpath, 0) < 0) + return -2; + + /* we found a free port */ + if (!(portStr = virDomainUSBAddressPortFormat(portpath))) + goto cleanup; + + info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB; + info->addr.usb.bus = bus; + memcpy(info->addr.usb.port, portpath, sizeof(portpath)); + VIR_DEBUG("Assigning USB addr bus=%u port=%s", + info->addr.usb.bus, portStr); + if (virDomainUSBAddressReserve(info, addrs) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(portStr); + return ret; +} + + +int +virDomainUSBAddressAssign(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + size_t i; + int rc; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + if (!addrs->buses[info->addr.usb.bus]) { + virReportError(VIR_ERR_XML_ERROR, + _("USB bus %u requested but no controller " + "with that index is present"), info->addr.usb.bus); + return -1;

For this case, the subsequent call at least won't get the !hub failure

+ } + rc = virDomainUSBAddressAssignFromBus(addrs, info, info->addr.usb.bus); + if (rc >= -1) + return rc; + } else { + for (i = 0; i < addrs->nbuses; i++) { + rc = virDomainUSBAddressAssignFromBus(addrs, info, i); + if (rc >= -1) + return rc;

A !hub failure from caller falls into the code below... Although I'm not sure it's possible - I keep having to go back to remind myself... A info->type would I assume be NONE at this point, right? John

+ } + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No free USB ports")); + return -1; +} + + int virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, void *data) @@ -1608,3 +1713,21 @@ virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, VIR_FREE(portStr); return ret; } + + +int +virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || + (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && + !virDomainUSBAddressPortIsValid(info->addr.usb.port))) { + if (virDomainUSBAddressAssign(addrs, info) < 0) + return -1; + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + if (virDomainUSBAddressReserve(info, addrs) < 0) + return -1; + } + + return 0; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 4230ea2..4ae860c 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -273,10 +273,24 @@ virDomainUSBAddressSetPtr virDomainUSBAddressSetCreate(void); int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, virDomainDefPtr def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainUSBAddressSetAddHub(virDomainUSBAddressSetPtr addrs, + virDomainHubDefPtr hub) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);

int virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, void *data) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int +virDomainUSBAddressAssign(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f66ccf5..4727d39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -107,11 +107,14 @@ virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; virDomainPCIControllerModelToConnectType; +virDomainUSBAddressAssign; +virDomainUSBAddressEnsure; virDomainUSBAddressPortFormat; virDomainUSBAddressPortFormatBuf; virDomainUSBAddressPortIsValid; virDomainUSBAddressReserve; virDomainUSBAddressSetAddControllers; +virDomainUSBAddressSetAddHub; virDomainUSBAddressSetCreate; virDomainUSBAddressSetFree; virDomainVirtioSerialAddrAssign; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f66b2f0..93c90de 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1622,6 +1622,62 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, }

+struct qemuAssignUSBIteratorInfo { + virDomainUSBAddressSetPtr addrs; + size_t count; +}; + + +static int +qemuDomainAssignUSBPortsIterator(virDomainDeviceInfoPtr info, + void *opaque) +{ + struct qemuAssignUSBIteratorInfo *data = opaque; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + + return virDomainUSBAddressAssign(data->addrs, info); +} + + +static int +qemuDomainAssignUSBHubs(virDomainUSBAddressSetPtr addrs, + virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->nhubs; i++) { + virDomainHubDefPtr hub = def->hubs[i]; + if (hub->type != VIR_DOMAIN_HUB_TYPE_USB) + continue; + + if (hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) + continue; + if (virDomainUSBAddressAssign(addrs, &hub->info) < 0) + return -1; + + if (virDomainUSBAddressSetAddHub(addrs, hub) < 0)

Going back to review comments made in 5/9 regarding this call... Are we sure here that virDomainUSBAddressPortIsValid is true and/or does it matter? I'm not even sure how this should look - mixing in the usb-hub's in causing brain stack overflow. I think you're getting what is expected based on qemuxml2argv-usb-port-autoassign.

+ return -1; + } + + return 0; +} + + +static int +qemuDomainAssignUSBPorts(virDomainUSBAddressSetPtr addrs, + virDomainDefPtr def) +{ + struct qemuAssignUSBIteratorInfo data = { .addrs = addrs }; + + return virDomainUSBDeviceDefForeach(def, + qemuDomainAssignUSBPortsIterator, + &data, + true); +} + + static int qemuDomainAssignUSBAddresses(virDomainDefPtr def, virDomainObjPtr obj) @@ -1642,6 +1698,14 @@ qemuDomainAssignUSBAddresses(virDomainDefPtr def,

VIR_DEBUG("Existing USB addresses have been reserved");

+ if (qemuDomainAssignUSBHubs(addrs, def) < 0) + goto cleanup; + + if (qemuDomainAssignUSBPorts(addrs, def) < 0) + goto cleanup; +

Is this the later that the comment "/* USB hubs that do not yet have an USB address have to be dealt with later */" in patch 5 is referencing?

+ VIR_DEBUG("Finished assigning USB ports"); + if (obj && obj->privateData) { priv = obj->privateData; priv->usbaddrs = addrs; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args index fe4e419..848a029 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args @@ -21,5 +21,5 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -serial pty \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios.args b/tests/qemuxml2argvdata/qemuxml2argv-bios.args index 012af85..604b871 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-bios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios.args @@ -22,5 +22,5 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -serial pty \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args index 70f3fdb..7b98beb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args @@ -19,8 +19,8 @@ nowait \ -boot order=cna,menu=off \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x7 \ --device usb-ccid,id=ccid0 \ --device usb-hub,id=hub0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1.1 \ +-device usb-hub,id=hub0,bus=usb.0,port=1 \ -drive file=/tmp/fdr.img,format=raw,if=none,id=drive-virtio-disk0,cache=none,\ aio=native \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,\ @@ -37,10 +37,10 @@ media=cdrom,id=drive-ide0-1-0,readonly=on \ -chardev spicevmc,id=charchannel0,name=vdagent \ -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ id=channel0,name=com.redhat.spice.0 \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1.2 \ -spice port=5901,tls-port=5902,addr=0.0.0.0,x509-dir=/etc/pki/libvirt-spice \ -vga cirrus \ -device intel-hda,id=sound0,bus=pci.0,addr=0x4 \ -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \ --device usb-host,hostbus=14,hostaddr=6,id=hostdev0 \ +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bus=usb.0,port=2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args index 63e2bb2..7cda592 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args @@ -21,5 +21,6 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/tmp/usbdisk.img,format=raw,if=none,id=drive-usb-disk0 \ --device usb-storage,drive=drive-usb-disk0,id=usb-disk0,removable=on \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk0,id=usb-disk0,\ +removable=on \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args index 5d1ea98..03ef44f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args @@ -21,5 +21,5 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/tmp/usbdisk.img,format=raw,if=none,id=drive-usb-disk0 \ --device usb-storage,drive=drive-usb-disk0,id=usb-disk0 \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk0,id=usb-disk0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args index c0be4ee..cead7d6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args @@ -28,7 +28,7 @@ media=cdrom,id=drive-ide0-1-0,readonly=on \ -device rtl8139,vlan=0,id=net0,mac=52:54:00:71:70:89,bus=pci.0,addr=0x7 \ -net tap,fd=3,vlan=0,name=hostnet0 \ -serial pty \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=5900,addr=127.0.0.1 \ -vga std \ -device AC97,id=sound0,bus=pci.0,addr=0x3 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args index fa248b3..3f00da4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args @@ -30,7 +30,7 @@ zlib-glz-wan-compression=auto,playback-compression=on,streaming-video=filter,\ disable-copy-paste \ -vga cirrus \ -chardev socket,id=charredir0,host=localhost,port=4000 \ --device usb-redir,chardev=charredir0,id=redir0 \ +-device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 \ -chardev spicevmc,id=charredir1,name=usbredir \ -device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args index 8c00055..86f394b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args @@ -19,5 +19,5 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bootindex=1 \ +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bootindex=1,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args index b5e6834..7883c61 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args @@ -20,5 +20,5 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --device usb-host,hostbus=14,hostaddr=6,id=hostdev0 \ +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args index bb5d55a..d1c3e8f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -19,4 +19,4 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --device usb-host,hostbus=14,hostaddr=6,id=hostdev0 +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bus=usb.0,port=1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index c5a9e53..5c356ef 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -46,7 +46,7 @@ id=channel0,name=org.qemu.guest_agent.0 \ -chardev spicevmc,id=charchannel1,name=vdagent \ -device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\ id=channel1,name=com.redhat.spice.0 \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=5901,tls-port=5902,addr=127.0.0.1,x509-dir=/etc/pki/libvirt-spice \ -vga qxl \ -global qxl-vga.ram_size=67108864 \ @@ -54,7 +54,7 @@ id=channel1,name=com.redhat.spice.0 \ -device intel-hda,id=sound0,bus=pci.0,addr=0x4 \ -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \ -chardev spicevmc,id=charredir0,name=usbredir \ --device usb-redir,chardev=charredir0,id=redir0 \ +-device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 \ -chardev spicevmc,id=charredir1,name=usbredir \ --device usb-redir,chardev=charredir1,id=redir1 \ +-device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse.args b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse.args index bd0e5c6..df96e6a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse.args @@ -19,4 +19,4 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --device usb-mouse,id=input0 +-device usb-mouse,id=input0,bus=usb.0,port=1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-input-usbtablet.args b/tests/qemuxml2argvdata/qemuxml2argv-input-usbtablet.args index 294515f..faf21d5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-input-usbtablet.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-input-usbtablet.args @@ -19,4 +19,4 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --device usb-tablet,id=input0 +-device usb-tablet,id=input0,bus=usb.0,port=1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args index 25c16cb..5887616 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args @@ -22,4 +22,4 @@ server,nowait \ -device pci-ohci,id=usb,bus=pci,addr=0x1 \ -chardev pty,id=charserial0 \ -device spapr-vty,chardev=charserial0,reg=0x30000000 \ --device usb-kbd,id=input0 +-device usb-kbd,id=input0,bus=usb.0,port=1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args index 246e854..f05c3f2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args @@ -23,7 +23,7 @@ server,nowait \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev spiceport,id=charserial0,name=org.qemu.console.serial.0 \ -device isa-serial,chardev=charserial0,id=serial0 \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=5903,tls-port=5904,addr=127.0.0.1,x509-dir=/etc/pki/libvirt-spice \ -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,\ addr=0x2 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args index d3135c2..beb2935 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args @@ -19,7 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device usb-ccid,id=ccid0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ -device ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args index 09ef26c..72cf24b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args @@ -19,7 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device usb-ccid,id=ccid0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ -device ccid-card-emulated,backend=certificates,cert1=cert1,cert2=cert2,\ cert3=cert3,db=/etc/pki/nssdb,id=smartcard0,bus=ccid0.0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args index d3135c2..beb2935 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args @@ -19,7 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device usb-ccid,id=ccid0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ -device ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args index b618507..cdca4c4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args @@ -19,7 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device usb-ccid,id=ccid0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ -chardev spicevmc,id=charsmartcard0,name=smartcard \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args index e0fcb49..0c526c8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args @@ -19,7 +19,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -boot c \ --device usb-ccid,id=ccid0 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ -chardev socket,id=charsmartcard0,host=127.0.0.1,port=2001,server,nowait \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args index 8d846a0..b084f4e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args @@ -34,5 +34,5 @@ QEMU_AUDIO_DRV=none \ -device ich9-intel-hda,id=sound7,bus=pci.0,addr=0x8 \ -device hda-micro,id=sound7-codec0,bus=sound7.0,cad=0 \ -device hda-duplex,id=sound7-codec1,bus=sound7.0,cad=1 \ --device usb-audio,id=sound8 \ +-device usb-audio,id=sound8,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x9 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.args new file mode 100644 index 0000000..6ec2b85 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-device usb-hub,id=hub0,bus=usb.0,port=1 \ +-device usb-mouse,id=input0,bus=usb.0,port=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args new file mode 100644 index 0000000..ac5cfdd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-device usb-hub,id=hub0,bus=usb.0,port=1 \ +-device usb-hub,id=hub1,bus=usb.0,port=2 \ +-device usb-mouse,id=input0,bus=usb.0,port=1.1 \ +-device usb-mouse,id=input1,bus=usb.0,port=1.2 \ +-device usb-mouse,id=input2,bus=usb.0,port=1.3 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml new file mode 100644 index 0000000..a2fe34e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml @@ -0,0 +1,27 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + <hub type='usb'> + <address type='usb' bus='0' port='1'/> + </hub> + <input type='mouse' bus='usb'> + </input> + <hub type='usb'> + </hub> + <input type='mouse' bus='usb'> + </input> + <input type='mouse' bus='usb'> + </input> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-boot.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-boot.args index 53b9040..bc47963 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-boot.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-boot.args @@ -24,7 +24,7 @@ addr=0x4 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \ -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 \ -chardev socket,id=charredir0,host=localhost,port=4000 \ --device usb-redir,chardev=charredir0,id=redir0,bootindex=1 \ +-device usb-redir,chardev=charredir0,id=redir0,bootindex=1,bus=usb.0,port=1 \ -chardev spicevmc,id=charredir1,name=usbredir \ -device usb-redir,chardev=charredir1,id=redir1,bootindex=2,bus=usb.0,port=4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir.args index 08e8f3e..0999c97 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir.args @@ -25,7 +25,7 @@ addr=0x4 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \ -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 \ -chardev socket,id=charredir0,host=localhost,port=4000 \ --device usb-redir,chardev=charredir0,id=redir0 \ +-device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 \ -chardev spicevmc,id=charredir1,name=usbredir \ -device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9c18989..a872ba8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1168,6 +1168,9 @@ mymain(void) DO_TEST("usb-ports", QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); + DO_TEST("usb-port-autoassign", + QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB, + QEMU_CAPS_NODEFCONFIG); DO_TEST("usb-redir", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_USB_HUB,

On Thu, Jul 14, 2016 at 01:00:09PM -0400, John Ferlan wrote:

On 07/01/2016 11:38 AM, Ján Tomko wrote:

...

When parsing a command line with USB devices that have no address specified, QEMU automatically adds a USB hub if the device would fill up all the available USB ports.

To help most of the users, add one hub if there are more USB devices than available ports. For wilder configurations, expect the user to provide us with more hubs and/or controllers. --- src/conf/domain_addr.c | 20 +++++++++ src/conf/domain_addr.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 48 ++++++++++++++++++++++ .../qemuxml2argv-usb-hub-autoadd.args | 28 +++++++++++++ .../qemuxml2argv-usb-hub-autoadd.xml | 23 +++++++++++ tests/qemuxml2argvtest.c | 3 ++ 7 files changed, 125 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml

...

+static int +qemuDomainUSBAddressAddHubs(virDomainDefPtr def) +{ + struct qemuAssignUSBIteratorInfo data = { .count = 0 }; + virDomainHubDefPtr hub = NULL; + size_t available_ports; + int ret = -1; + + available_ports = virDomainUSBAddressCountAvailablePorts(def); + ignore_value(virDomainUSBDeviceDefForeach(def, + qemuDomainAssignUSBPortsCounter, + &data, + false)); + VIR_DEBUG("Found %zu USB devices and %zu provided USB ports", + data.count, available_ports); + + /* Add one hub if there are more devices than ports + * otherwise it's up to the user to specify more hubs/controllers */

What if one hub isn't enough? Wouldn't we need to keep adding until we have enough? I'm sure a test could be created for it...

For that many devices, most of the users would probably be better off with more USB controllers than chaining hubs. The aim of this code is to cover most of the cases, not all of them.

...

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml new file mode 100644 index 0000000..43e0f1f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml @@ -0,0 +1,23 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/>

So to try and answer something I was thinking about earlier... If I just change index from 0 to 1, then I get an error:

libvirt: QEMU Driver error : unsupported configuration: Multiple legacy USB controllers are not supported

If I then adjust the qemuxml2argvtest to add QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_PCI_MULTIFUNCTION, then I get the failure:

error : virDomainUSBAddressReserve:1724 : XML error: Duplicate USB address bus 0 port 1

I could not reproduce this error. Maybe I fixed it along the way? Jan

So is it a "valid" test to have one defined for index='1' and allow the code to create index='0'?

John