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