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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list