11:06 a.m.
On Thu, Jul 14, 2016 at 10:59:11AM -0400, John Ferlan wrote:
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.
Will add those in a separate patch.
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?
Leftover from the development process.
> +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?
Or any type other than USB.
At least one of the hubs should be non-NULL, otherwise we would have no
reason to add anything to addrs->buses. But even if it returns -2,
the error below is still true, even though it could be a bit cryptic.
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.
Yes, if virDomainUSBAddressAssign returns 0, then the device should have
a valid address.
On the other hand, the condition:
if (hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
needs to only continue if the port is also valid, otherwise
hubs with just a bus will not get a port assigned (and won't be
considered in the allocation algorithm).
> + 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?
Yes
Jan