On 08/12/2015 10:52 AM, Ján Tomko wrote:
> If USB addresses have been provided for all USB devices,
> or we are defining a new domain, reserve the addresses.
>
> Check if they fit on the USB controllers the domain has,
> and error out if two devices try to use the same address.
>
> Do not error out on missing hubs.
>
> The input-usbmouse test used port=4 even though the implicit
> USB controller only has two.
> ---
> src/conf/domain_addr.c | 109 ++++++++++++++++
> src/conf/domain_addr.h | 6 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 137 ++++++++++++++++++++-
> src/qemu/qemu_domain.h | 1 +
> .../qemuxml2argv-input-usbmouse-addr.args | 2 +-
> .../qemuxml2argv-input-usbmouse-addr.xml | 2 +-
> .../qemuxml2argv-usb-hub-conflict.xml | 22 ++++
> tests/qemuxml2argvtest.c | 3 +
> 9 files changed, 280 insertions(+), 3 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-conflict.xml
>
Wouldn't qemuDomainObjPrivateFree need to be updated as well to free
'usbaddrs' (similar to pciaddrs, ccwaddrs, vioserialaddrs)?
What about qemuProcessStop where the various addr's have their free
routines called and then the *addrs pointer set to NULL?
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 024d47b..cda6e08 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1313,6 +1313,69 @@ static int virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs,
> }
>
>
> +/* Finds the port specified by the port path in the device info.
> + * The USB bus is expected to exist.
> + * The USB hubs will be auto-created.
> + */
> +static int
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +virDomainUSBAddressFindPort(virDomainUSBAddressHubPtr **port,
> + virDomainUSBAddressSetPtr addrs,
> + virDomainDeviceInfoPtr info)
> +{
> + ssize_t i;
> + virDomainUSBAddressHubPtr *hub = NULL;
> + char *portstr = NULL;
> + int ret = -1;
> +
> + portstr = virDomainUSBAddressGetPortString(info->addr.usb.port);
What happens if portstr == NULL ?
> +
> + if (addrs->nbuses <= info->addr.usb.bus ||
> + !addrs->buses[info->addr.usb.bus]) {
Personally it's easier to think about it as if info->addr.usb.bus >
addrs->nbuses...
This would perhaps be a place where a find a bus by ctlr_idx number
could be useful - considering earlier review comments where I noted we
may not want to create 'cont->idx' buses.
> + virReportError(VIR_ERR_XML_ERROR, _("Missing USB bus %u"),
> + info->addr.usb.bus);
> + goto cleanup;
> + }
> + hub = &(addrs->buses[info->addr.usb.bus]);
I would think !hub would be a problem here since it's the "root/bus
hub"... To start out with at least the root hub better point to
something... Hence my ATTRIBUTE_NONNULL above...
> +
> + for (i = 0; i < VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH; i++) {
> + int portnum = info->addr.usb.port[i];
> + if (!portnum)
> + break;
oh or oy - here's the nested ports (eg the dotted octet's)...
Assuming we have "port='1'", then we fall into the rest of this when it
feels to me that we shouldn't. It would seem we're allocating perhaps an
extra layer that we don't want to. To me it seems the current algorithm
would allocate an AddressHub off the bus/root hub at index[0] with
nports=9.
Assuming a 2 port root usb hub, after return from here we'd end up with:
AddressSet
+---------+ AddressHub
| buses[] |----> +--------+
| nbuses | | ports[]|----> +--------+ AddressHub
+---------+ | nports | |ports[0]|----> +--------+
+--------+ |ports[1]| | ports[]| ...
nports == 2 +--------+ | nports |
+--------+
nports == 9
Then the caller would allocate another AddressHub with nports=0 at the
AddressHub on the right at ports[0]. I guess I would have assumed that
the AddressHub would nports=0 could be allocated at ports[0] off the
root/usb hub.
So the rather than actually plugging something directly into the root
usb hub, the algorithm creates an unnecessary hub, leaving
host -> root hub[0] -> hub[0] -> device
instead of
host -> root hub[0] -> device
I think perhaps what's trying to be accomplished is to count the "depth"
of the portstr, where the depth is 1, 2, 3, or 4. If the depth is 1,
then I think we'd return to the caller which would hopefully allocate an
AddressHub w/ nports=0 and plug the device directly into the root hub.
If the depth was 2, 3, or 4, then we'd have to allocate 'depth' nested
hubs each at the location of the port[i] value and then return to the
caller which would plug our device into that last port.
The following is just hard to read/comprehend mentally...
> +
> + if (!(*hub)) {
> + /* FIXME: Add the hub to the domain definition */
If we do that - does it get removed from the private area?
> + if (VIR_ALLOC(*hub) < 0)
> + goto cleanup;
> + if (VIR_ALLOC_N((*hub)->ports, 8+1) < 0) {
> + VIR_FREE((*hub));
> + goto cleanup;
> + }
> + (*hub)->nports = 8+1;
"8+1" is such an ugly magic number... Again, like my note in patch6 -
good idea to add a note 'why' the "+1" is there...
If there was a 'ports' allocation API this would be opaque... eg.
newhub = virDomainUSBAddressAllocPort(8); /* or some constant */
if (!newhub)
failure
(*hub) = newhub;
newhub = NULL
> + }
> + if ((*hub)->nports < portnum) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("port %u out of range in port path %s"),
> + portnum, portstr);
> + goto cleanup;
> + }
This check seems out of place. I think this answers a question I posed
somewhere along the way whether "port='3.1'" would be legal in a 2 port
root usb and that "port='1.10'" is perhaps not legal either...
I would think this should go right after we get the portnum before the
break; in the above loop/context.
> + hub = &((*hub)->ports[info->addr.usb.port[i]]);
> + if ((*hub) && (*hub)->nports == 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Port %d is already occupied in port path %s"),
> + portnum, portstr);
> + goto cleanup;
> + }
Similarly this could be out of place too - although perhaps it's better
as an else of the (!(*hub))...
> + }
> +
> + ret = 0;
> + *port = hub;
> +
> + cleanup:
> + VIR_FREE(portstr);
> + return ret;
> +}
> +
> +
> int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> virDomainDefPtr def)
> {
> @@ -1327,3 +1390,49 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> }
> return 0;
> }
> +
> +
> +int
> +virDomainUSBAddressReserve(virDomainDefPtr def ATTRIBUTE_UNUSED,
def "unused" but required since using virDomainDeviceInfoIterate...
Perhaps something to note...
> + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> + virDomainDeviceInfoPtr info,
> + void *data)
> +{
> + virDomainUSBAddressSetPtr addrs = data;
> + virDomainUSBAddressHubPtr *port = NULL;
> + char *portstr = NULL;
> + int ret = -1;
> +
> + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + return 0;
> +
> + portstr = virDomainUSBAddressGetPortString(info->addr.usb.port);
What happens if portstr == NULL ?
> + VIR_DEBUG("Reserving USB addr bus=%u port=%s", info->addr.usb.bus, portstr);
> +
> + if (virDomainUSBAddressFindPort(&port, addrs, info) < 0)
> + goto cleanup;
> +
> + if (dev &&
> + dev->type == VIR_DOMAIN_DEVICE_HUB &&
> + dev->data.hub->type == VIR_DOMAIN_HUB_TYPE_USB) {
> + if (!*port) {
> + if (VIR_ALLOC(*port) < 0)
> + goto cleanup;
> +
> + if (VIR_ALLOC_N((*port)->ports, 8 + 1) < 0) {
> + VIR_FREE(*port);
> + goto cleanup;
> + }
> + (*port)->nports = 8 + 1;
Here again a magic 8 + 1... and I'm not sure why this is necessary. It
feels duplicitous with the FindPort API
> + }
> + } else {
> + if (VIR_ALLOC(*port) < 0)
> + goto cleanup;
Ahhh... here's the place where nports == 0 - took me a while to dig into
that... Might be nice to note that this is the "terminus" or final
device of the AddressHub.
FWIW: I'm surprised the bitmap code wasn't used - although that could be
tricky too
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(portstr);
> + return ret;
> +}
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 64d35e9..8f866ae 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -269,4 +269,10 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);
>
> +int
> +virDomainUSBAddressReserve(virDomainDefPtr def,
> + virDomainDeviceDefPtr dev,
> + virDomainDeviceInfoPtr info,
> + void *data)
> + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
> #endif /* __DOMAIN_ADDR_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7db4839..1420b19 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -108,6 +108,7 @@ virDomainPCIAddressSlotInUse;
> virDomainPCIAddressValidate;
> virDomainUSBAddressGetPortBuf;
> virDomainUSBAddressGetPortString;
> +virDomainUSBAddressReserve;
> virDomainUSBAddressSetAddControllers;
> virDomainUSBAddressSetCreate;
> virDomainUSBAddressSetFree;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4b520d7..a6a3438 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1563,6 +1563,137 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
> }
>
>
> +static void
> +qemuDomainCountUSBAddresses(virDomainDefPtr def,
> + size_t *specified,
> + size_t *total)
> +{
> + size_t i;
> + size_t spec = 0, tot = 0;
> +
> + /* usb-hub */
> + for (i = 0; i < def->nhubs; i++) {
> + virDomainHubDefPtr hub = def->hubs[i];
> + if (hub->type == VIR_DOMAIN_HUB_TYPE_USB) {
> + tot++;
> + if (hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* usb-host */
> + for (i = 0; i < def->nhostdevs; i++) {
> + virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> + tot++;
> + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* usb-storage */
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDefPtr disk = def->disks[i];
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> + tot++;
> + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* TODO: usb-net */
> +
> + /* usb-ccid */
> + for (i = 0; i < def->ncontrollers; i++) {
> + virDomainControllerDefPtr cont = def->controllers[i];
> + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_CCID) {
> + tot++;
> + if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* usb-kbd, usb-mouse, usb-tablet */
> + for (i = 0; i < def->ninputs; i++) {
> + virDomainInputDefPtr input = def->inputs[i];
> +
> + if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) {
> + tot++;
> + if (input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* usb-serial */
> + for (i = 0; i < def->nserials; i++) {
> + virDomainChrDefPtr serial = def->serials[i];
> + if (serial->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
> + tot++;
> + if (serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + /* usb-audio model=usb */
> + for (i = 0; i < def->nsounds; i++) {
> + virDomainSoundDefPtr sound = def->sounds[i];
> + if (sound->model == VIR_DOMAIN_SOUND_MODEL_USB) {
> + tot++;
> + if (sound->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + spec++;
> + }
> + }
> +
> + *specified = spec;
> + *total = tot;
> +}
> +
> +static int
> +qemuDomainAssignUSBAddresses(virDomainDefPtr def,
> + virDomainObjPtr obj,
> + bool newDomain)
> +{
> + int ret = -1;
> + size_t total, specified;
> + virDomainUSBAddressSetPtr addrs = NULL;
> + qemuDomainObjPrivatePtr priv = NULL;
> +
> + qemuDomainCountUSBAddresses(def, &specified, &total);
> + VIR_DEBUG("%zu out of %zu USB addresses have been specified", specified, total);
> +
> + /* if ALL USB devices have their addresses assigned
> + * or we're creating a new domain, we can continue allocation,
> + * otherwise QEMU might create an incompatible machine by adding hubs
> + * and own addresses */
> + if (!(newDomain || total == specified))
> + return 0;
Does domain persistence matter here? EG do we want to skip this for
transient domains?
John
> +
> + if (!(addrs = virDomainUSBAddressSetCreate()))
> + goto cleanup;
> +
> + if (virDomainUSBAddressSetAddControllers(addrs, def) < 0)
> + goto cleanup;
> +
> + if (virDomainDeviceInfoIterate(def, virDomainUSBAddressReserve, addrs) < 0)
> + goto cleanup;
> +
> + VIR_DEBUG("Existing USB addresses have been reserved");
> +
> + if (obj && obj->privateData) {
> + priv = obj->privateData;
> + /* if this is the live domain object, we persist the addresses */
> + priv->usbaddrs = addrs;
> + priv->persistentAddrs = 1;
> + addrs = NULL;
> + }
> + ret = 0;
> +
> + cleanup:
> + virDomainUSBAddressSetFree(addrs);
> + return ret;
> +}
> +
> +
> static int
> qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> virDomainDeviceDefPtr device,
> @@ -1774,7 +1905,7 @@ qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus)
> int qemuDomainAssignAddresses(virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps,
> virDomainObjPtr obj,
> - bool newDomain ATTRIBUTE_UNUSED)
> + bool newDomain)
> {
> int rc;
>
> @@ -1796,6 +1927,10 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
> if (rc)
> return rc;
>
> + rc = qemuDomainAssignUSBAddresses(def, obj, newDomain);
> + if (rc)
> + return rc;
> +
> return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
> }
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 2af7c59..454f759 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -167,6 +167,7 @@ struct _qemuDomainObjPrivate {
> virDomainPCIAddressSetPtr pciaddrs;
> virDomainCCWAddressSetPtr ccwaddrs;
> virDomainVirtioSerialAddrSetPtr vioserialaddrs;
> + virDomainUSBAddressSetPtr usbaddrs;
> int persistentAddrs;
>
> virQEMUCapsPtr qemuCaps;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.args
> index 07ea004..2f45b03 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.args
> @@ -2,5 +2,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
> /usr/bin/qemu -S \
> -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
> -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
> --hda /dev/HostVG/QEMUGuest1 -device usb-mouse,id=input0,bus=usb.0,port=4 \
> +-hda /dev/HostVG/QEMUGuest1 -device usb-mouse,id=input0,bus=usb.0,port=2 \
> -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.xml
> index a996838..dde3517 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-input-usbmouse-addr.xml
> @@ -22,7 +22,7 @@
> <controller type='usb' index='0'/>
> <controller type='ide' index='0'/>
> <input type='mouse' bus='usb'>
> - <address type='usb' bus='0' port='4'/>
> + <address type='usb' bus='0' port='2'/>
> </input>
> </devices>
> </domain>
> 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 8815087..feb1d85 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1171,6 +1171,9 @@ mymain(void)
> DO_TEST("usb-hub",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB,
> QEMU_CAPS_NODEFCONFIG);
> + DO_TEST_ERROR("usb-hub-conflict",
> + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB,
> + QEMU_CAPS_NODEFCONFIG);
> DO_TEST("usb-ports",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB,
> QEMU_CAPS_NODEFCONFIG);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list