[libvirt] [PATCHv4 7/9] Assign addresses to USB devices
Ján Tomko
jtomko at redhat.com
Mon Jul 18 16:06:29 UTC 2016
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
More information about the libvir-list
mailing list