Perhaps needs a bit more meat/description for the commit message... On 08/12/2015 10:52 AM, Ján Tomko wrote:

--- src/conf/domain_addr.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 4 +++ src/libvirt_private.syms | 1 + 3 files changed, 93 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 3962357..024d47b 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1239,3 +1239,91 @@ void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs) VIR_FREE(addrs->buses); VIR_FREE(addrs); } + + +static int virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs, + virDomainControllerDefPtr cont)

static int virDomain*

+{ + virDomainUSBAddressHubPtr hub = NULL; + size_t ports = 0;

Perhaps easier if use 'nports'

+ int ret = -1; + int model = cont->model; + + VIR_DEBUG("addrs=%p controller type=%d model=%d", + addrs, cont->type, cont->model); + + if (VIR_ALLOC(hub) < 0) + goto cleanup;

This should be paired this with the ports allocation and have a separate API to also be used by next patch

+ + if (model == -1) + model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + + switch ((virDomainControllerModelUSB)model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: + ports = 2; + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: + ports = 6; + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: + return 0; + /* FIXME * check the companions? */ + ports = 2; + break;

This needs to be fixed/resolved.

+ + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: + ports = 3; + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + ports = 15; + + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST: + ret = 0; + goto cleanup; + } + + if (cont->idx >= addrs->nbuses) { + if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, cont->idx - addrs->nbuses + 1) < 0) + goto cleanup; + }

So if I read this correctly, someone defining a single controller in their XML with an index of 5 (or 10 or 100 or anything up to 0xfff - which is supposed to be the bus# max) will be creating that many AddressSets. There's no requirement that one numbers their controllers starting at '0' - just that the index is used as the order to be loaded. Perhaps not what is desired. Perhaps need some sort of number of controllers of type 'model' found API rather than a straight 'idx' to 'nbuses' comparison. Once it's figured out which controller index is being used, that could be set in 'addrs->ctlr_idx'. Could also create a companion search API or two that could take the 'ctlr_idx' and "find" the controller knowing the eventual USB device type. Since there's no guarantee of having cont->idx start at 0 and run through some maximum, should this use a VIR_INSERT_ELEMENT type algorithm to keep things ordered? Using ctlr_idx for ordering (if it even matters).

+ + if (VIR_ALLOC_N(hub->ports, ports + 1) < 0) + goto cleanup; + hub->nports = ports + 1; +

Mental note #1 - why are we allocating an extra port? Answered later on - using port#0 is not valid (nor is using 0 in dotted notation - eg 1.0.1 would equate to just 1). Since this isn't obvious - it might be beneficial to add a note as to why we're adding 1. Mental note #2 - this is what I'll refer to later as the root usb hub... The VIR_ALLOC(hub) and this VIR_ALLOC_N should have a separate API to be shared in next patch as well. It can take a parameter of 'nports'. What's not fully clear (and something I note in the next patch) is whether a root hub port can contain a device or if it has to contain another hub. I would think physically the answer would be a device could live on the root hub, but dealing with the physical world is not something I know all that well..

+ VIR_DEBUG("Added %zu ports on hub %p", hub->nports - 1, hub); + + /* FIXME: is there a bus already? */ + addrs->buses[cont->idx] = hub;

Wouldn't this be solved with VIR_INSERT_ELEMENT? If buses[cont->idx] != NULL before this line what does that say - something's wrong with the algorithm. At the very least you'll leak memory and lose at least 1 device and possibly more.

+ hub = NULL; + + ret = 0; + cleanup: + VIR_FREE(hub); + return ret; +} + + +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, + virDomainDefPtr def)

int virDomain* John

+{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { + if (virDomainUSBAddressSetAddController(addrs, cont) < 0) + return -1; + } + } + return 0; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index dcf86d4..64d35e9 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -263,6 +263,10 @@ typedef struct _virDomainUSBAddressSet virDomainUSBAddressSet; typedef virDomainUSBAddressSet *virDomainUSBAddressSetPtr;

virDomainUSBAddressSetPtr virDomainUSBAddressSetCreate(void); + +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, + virDomainDefPtr def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);

#endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a628d00..7db4839 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,6 +108,7 @@ virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; virDomainUSBAddressGetPortBuf; virDomainUSBAddressGetPortString; +virDomainUSBAddressSetAddControllers; virDomainUSBAddressSetCreate; virDomainUSBAddressSetFree; virDomainVirtioSerialAddrAssign;

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);

On 08/12/2015 10:52 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 | 88 +++++++++++++++++++++- src/conf/domain_addr.h | 6 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 87 +++++++++++++++++++++ ...qemuhotplug-console-compat-2+console-virtio.xml | 4 +- .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-bios.args | 2 +- .../qemuxml2argv-console-compat-2.xml | 4 +- .../qemuxml2argv-controller-order.args | 7 +- .../qemuxml2argv-controller-order.xml | 2 + .../qemuxml2argv-disk-usb-device-removable.args | 3 +- .../qemuxml2argv-disk-usb-device.args | 3 +- .../qemuxml2argv-graphics-spice-timeout.args | 2 +- ...muxml2argv-hostdev-usb-address-device-boot.args | 2 +- .../qemuxml2argv-hostdev-usb-address-device.args | 3 +- .../qemuxml2argv-hugepages-numa.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 | 3 +- .../qemuxml2argv-smartcard-passthrough-tcp.args | 2 +- .../qemuxml2argv-sound-device.args | 2 +- .../qemuxml2argv-usb-port-autoassign.args | 15 ++++ .../qemuxml2argv-usb-port-autoassign.xml | 27 +++++++ tests/qemuxml2argvtest.c | 6 +- 27 files changed, 260 insertions(+), 23 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml

NB: Less thorough thinking than last 3...

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index cda6e08..15f4753 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1392,9 +1392,95 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, }

+static int +virDomainUSBAddressFindFreePort(virDomainUSBAddressHubPtr hub, + unsigned int *portpath, + unsigned int idx) +{ + size_t i; + + /* Look for free ports on the current hub */ + for (i = 1; i < hub->nports; i++) { + if (!hub->ports[i]) { + VIR_DEBUG("Found a free port %zu at level %u", i, idx); + portpath[idx] = i; + return 0; + } + }

This seems to indicate something can be plugged directly into the root hub... Definitely would help me for some kind of picture <sigh>.

+ + VIR_DEBUG("No ports found on hub %p, trying the hubs on it", hub); + + if (idx >= VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH - 1) + return -1; + + /* Recursively search through the ports + * that contain another hub */ + for (i = 1; i < hub->nports; i++) { + VIR_DEBUG("level: %u port: %zu, nports: %zu", idx, i, + hub->ports[i]->nports); + + if (hub->ports[i]->nports && + virDomainUSBAddressFindFreePort(hub->ports[i], + portpath, + idx + 1) == 0) { + portpath[idx] = i; + return 0; + } + + } + return -1; +} + + +int +virDomainUSBAddressAssign(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info, + bool isHub) +{ + unsigned int portpath[VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH] = { 0 }; + virDomainDeviceDefPtr dev = NULL; + char *portstr; + size_t i; + int ret = -1; + + if (isHub) { + if (VIR_ALLOC(dev) < 0) + goto cleanup; + dev->type = VIR_DOMAIN_DEVICE_HUB; + if (VIR_ALLOC(dev->data.hub) < 0) + goto cleanup; + dev->data.hub->type = VIR_DOMAIN_HUB_TYPE_USB; + } + + for (i = 0; i < addrs->nbuses; i++) { + virDomainUSBAddressHubPtr hub = addrs->buses[i]; + if (!hub) + continue; + + if (virDomainUSBAddressFindFreePort(hub, portpath, 0) == 0) { + info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB; + info->addr.usb.bus = i; + portstr = virDomainUSBAddressGetPortString(portpath);

What happens if portstr == NULL ?

+ memcpy(info->addr.usb.port, portpath, VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH); + VIR_DEBUG("Assigning USB addr bus=%u port=%s", + info->addr.usb.bus, portstr); + if (virDomainUSBAddressReserve(NULL, dev, info, addrs) == 0) + ret = 0; + VIR_FREE(portstr); + goto cleanup; + } + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No free USB ports")); + cleanup: + virDomainDeviceDefFree(dev); + return ret; +} + + int virDomainUSBAddressReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, virDomainDeviceInfoPtr info, void *data) { diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 8f866ae..8ecf588 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -270,6 +270,12 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs, void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);

int +virDomainUSBAddressAssign(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info, + bool hub) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int virDomainUSBAddressReserve(virDomainDefPtr def, virDomainDeviceDefPtr dev, virDomainDeviceInfoPtr info, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1420b19..048ec68 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -106,6 +106,7 @@ virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainUSBAddressAssign; virDomainUSBAddressGetPortBuf; virDomainUSBAddressGetPortString; virDomainUSBAddressReserve; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a6a3438..9df542e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1648,6 +1648,88 @@ qemuDomainCountUSBAddresses(virDomainDefPtr def, *total = tot; }

+ +static int +qemuDomainAssignUSBPorts(virDomainUSBAddressSetPtr addrs, + virDomainDefPtr def) +{ + size_t i; + + /* usb-hub */ + for (i = 0; i < def->nhubs; i++) { + virDomainHubDefPtr hub = def->hubs[i]; + if (hub->type == VIR_DOMAIN_HUB_TYPE_USB && + hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &hub->info, true) < 0) + return -1; + } + } + + /* 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 && + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, hostdev->info, false) < 0) + return -1; + } + } + + /* usb-storage */ + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB && + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &disk->info, false) < 0) + return -1; + } + } + + /* TODO: usb-net */

?? John

+ + /* usb-ccid */ + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_CCID && + cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &cont->info, false) < 0) + return -1; + } + } + + /* 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 && + input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &input->info, false) < 0) + return -1; + } + } + + /* usb-serial */ + for (i = 0; i < def->nserials; i++) { + virDomainChrDefPtr serial = def->serials[i]; + if (serial->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB && + serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &serial->info, false) < 0) + return -1; + } + } + + /* usb-audio model=usb */ + for (i = 0; i < def->nsounds; i++) { + virDomainSoundDefPtr sound = def->sounds[i]; + if (sound->model == VIR_DOMAIN_SOUND_MODEL_USB && + sound->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainUSBAddressAssign(addrs, &sound->info, false) < 0) + return -1; + } + } + return 0; +} + static int qemuDomainAssignUSBAddresses(virDomainDefPtr def, virDomainObjPtr obj, @@ -1679,6 +1761,11 @@ qemuDomainAssignUSBAddresses(virDomainDefPtr def,

VIR_DEBUG("Existing USB addresses have been reserved");

+ if (qemuDomainAssignUSBPorts(addrs, def) < 0) + goto cleanup; + + VIR_DEBUG("Finished assigning USB ports"); + if (obj && obj->privateData) { priv = obj->privateData; /* if this is the live domain object, we persist the addresses */ diff --git a/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml b/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml index d848677..9bc01e5 100644 --- a/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml +++ b/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml @@ -81,7 +81,9 @@ <target type='virtio' name='org.qemu.guest_agent.0'/> <address type='virtio-serial' controller='0' bus='0' port='1'/> </channel> - <input type='tablet' bus='usb'/> + <input type='tablet' bus='usb'> + <address type='usb' bus='0' port='1'/> + </input> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args index b51e8f3..d71ad5d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args @@ -6,5 +6,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -monitor unix:/tmp/test-monitor,server,nowait -boot c -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,format=raw \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --serial pty -device usb-tablet,id=input0 \ +-serial pty -device usb-tablet,id=input0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios.args b/tests/qemuxml2argvdata/qemuxml2argv-bios.args index e8ef763..ea7d631 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-bios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -m 1024 -smp 1 -nographic -nodefaults -device sga \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -usb -hda /dev/HostVG/QEMUGuest1 -serial pty \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml index a7209b2..c9f943f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml @@ -78,7 +78,9 @@ <target type='virtio' name='org.qemu.guest_agent.0'/> <address type='virtio-serial' controller='0' bus='0' port='1'/> </channel> - <input type='tablet' bus='usb'/> + <input type='tablet' bus='usb'> + <address type='usb' bus='0' port='1'/> + </input> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args index d68011d..d0c10c5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args @@ -5,7 +5,8 @@ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -boot order=cna,menu=off \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device \ virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x7 -device \ -usb-ccid,id=ccid0 -drive \ +usb-ccid,id=ccid0,bus=usb.0,port=1.1 \ +-device usb-hub,id=hub0,bus=usb.0,port=1 -drive \ file=/tmp/fdr.img,if=none,id=drive-virtio-disk0,cache=off,aio=native \ -device \ virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0 \ @@ -22,9 +23,9 @@ isa-serial,chardev=charserial0,id=serial0 -chardev \ spicevmc,id=charchannel0,name=vdagent \ -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ id=channel0,name=com.redhat.spice.0 \ --device usb-tablet,id=input0 -spice \ +-device usb-tablet,id=input0,bus=usb.0,port=1.2 -spice \ port=0,addr=0.0.0.0 -device \ intel-hda,id=sound0,bus=pci.0,addr=0x4 -device \ hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device \ -usb-host,hostbus=14,hostaddr=6,id=hostdev0 -device \ +usb-host,hostbus=14,hostaddr=6,id=hostdev0,bus=usb.0,port=2 -device \ virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.xml b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.xml index 07db77e..10f5558 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.xml @@ -84,6 +84,8 @@ <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </memballoon> + <hub type='usb'> + </hub> </devices> <seclabel type='dynamic' model='selinux' relabel='yes'/> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args index 793c597..53bbdb4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args @@ -4,6 +4,7 @@ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \ file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 -device ide-drive,\ bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive file=/tmp/usbdisk.img,\ -if=none,id=drive-usb-disk0 -device usb-storage,drive=drive-usb-disk0,\ +if=none,id=drive-usb-disk0 \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk0,\ id=usb-disk0,removable=on -device virtio-balloon-pci,id=balloon0,bus=pci.0,\ addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args index d2b80d7..88727fa 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device.args @@ -4,5 +4,6 @@ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \ file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 -device ide-drive,\ bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive file=/tmp/usbdisk.img,\ -if=none,id=drive-usb-disk0 -device usb-storage,drive=drive-usb-disk0,\ +if=none,id=drive-usb-disk0 \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk0,\ id=usb-disk0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args index 8b5d9ee..2e74a24 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args @@ -11,7 +11,7 @@ media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -device rtl8139,vlan=0,id=net0,mac=52:54:00:71:70:89,bus=pci.0,addr=0x7 \ -net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 -serial pty \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=5900 -vga std \ -device AC97,id=sound0,bus=pci.0,addr=0x3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args index f2cc35d..6b89b79 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device-boot.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ unix:/tmp/test-monitor,server,nowait -no-acpi -usb -hda \ /dev/HostVG/QEMUGuest1 \ --device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bootindex=1 \ +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bootindex=1,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args index 4c73a51..78fed2b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args @@ -2,5 +2,6 @@ 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-host,hostbus=14,hostaddr=6,id=hostdev0 \ +/dev/HostVG/QEMUGuest1 \ +-device usb-host,hostbus=14,hostaddr=6,id=hostdev0,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 37511b1..2a15a87 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -34,7 +34,7 @@ chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 \ -chardev spicevmc,id=charchannel1,name=vdagent \ -device virtserialport,bus=virtio-serial0.0,nr=2,\ chardev=charchannel1,id=channel1,name=com.redhat.spice.0 \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=0 \ -vga qxl \ -global qxl-vga.ram_size=67108864 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args index 373c72a..c46eb2d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-usb-kbd.args @@ -6,4 +6,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device pci-ohci,id=usb,bus=pci,addr=0x1 \ -chardev pty,id=charserial0 \ -device spapr-vty,chardev=charserial0,reg=0x30000000 \ --device usb-kbd,id=input0 +-device usb-kbd,id=input0,bus=usb.0,port=1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args index a806d63..e6cff64 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args @@ -6,7 +6,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -hda /dev/HostVG/QEMUGuest1 \ -chardev spiceport,id=charserial0,name=org.qemu.console.serial.0 \ -device isa-serial,chardev=charserial0,id=serial0 \ --device usb-tablet,id=input0 \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ -spice port=5903,tls-port=5904,addr=127.0.0.1,x509-dir=/etc/pki/libvirt-spice \ -device \ qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x2 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args index 5e58867..c1d126a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-controller.args @@ -3,6 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ -usb-ccid,id=ccid0 -usb -device \ +usb-ccid,id=ccid0,bus=usb.0,port=1 -usb -device \ ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args index 33d3b08..bc8b23a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args @@ -3,7 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ -usb-ccid,id=ccid0 -usb -device \ +usb-ccid,id=ccid0,bus=usb.0,port=1 -usb -device \ ccid-card-emulated,backend=certificates,cert1=cert1,cert2=cert2,cert3=cert3\ ,db=/etc/pki/nssdb,id=smartcard0,bus=ccid0.0 -device \ virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args index 5e58867..c1d126a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host.args @@ -3,6 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ -usb-ccid,id=ccid0 -usb -device \ +usb-ccid,id=ccid0,bus=usb.0,port=1 -usb -device \ ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args index eed319c..11998d3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-spicevmc.args @@ -3,6 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ -usb-ccid,id=ccid0 -usb -chardev spicevmc,id=charsmartcard0,name=smartcard \ +usb-ccid,id=ccid0,bus=usb.0,port=1 -usb \ +-chardev spicevmc,id=charsmartcard0,name=smartcard \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args index c350977..284eb58 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smartcard-passthrough-tcp.args @@ -3,7 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ -usb-ccid,id=ccid0 -usb -chardev \ +usb-ccid,id=ccid0,bus=usb.0,port=1 -usb -chardev \ socket,id=charsmartcard0,host=127.0.0.1,port=2001,server,nowait \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args index 3e2b293..bb0c845 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args @@ -14,5 +14,5 @@ id=sound6-codec0,bus=sound6.0,cad=0 \ -device ich9-intel-hda,id=sound7,bus=pci.0,addr=0x8 \ -device hda-micro,id=sound7-codec0,bus=sound7.0,cad=0 \ -device hda-duplex,id=sound7-codec1,bus=sound7.0,cad=1 \ --device usb-audio,id=sound8 \ +-device usb-audio,id=sound8,bus=usb.0,port=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x9 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args new file mode 100644 index 0000000..d61cb29 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.args @@ -0,0 +1,15 @@ +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 \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-device usb-hub,id=hub0,bus=usb.0,port=1 \ +-device usb-hub,id=hub1,bus=usb.0,port=2 \ +-device usb-mouse,id=input0,bus=usb.0,port=1.1 \ +-device usb-mouse,id=input1,bus=usb.0,port=1.2 \ +-device usb-mouse,id=input2,bus=usb.0,port=1.3 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml new file mode 100644 index 0000000..a2fe34e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-port-autoassign.xml @@ -0,0 +1,27 @@ +<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'> + </input> + <hub type='usb'> + </hub> + <input type='mouse' bus='usb'> + </input> + <input type='mouse' bus='usb'> + </input> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index feb1d85..39ce7e2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -678,7 +678,8 @@ mymain(void) QEMU_CAPS_BOOT_MENU, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DRIVE_AIO, QEMU_CAPS_CCID_PASSTHRU, QEMU_CAPS_CHARDEV, - QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE, QEMU_CAPS_HDA_DUPLEX); + QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE, QEMU_CAPS_HDA_DUPLEX, + QEMU_CAPS_USB_HUB); DO_TEST("eoi-disabled", NONE); DO_TEST("eoi-enabled", NONE); DO_TEST("pv-spinlock-disabled", NONE); @@ -1177,6 +1178,9 @@ mymain(void) DO_TEST("usb-ports", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); + DO_TEST("usb-port-autoassign", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB, + QEMU_CAPS_NODEFCONFIG); DO_TEST("usb-redir", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_USB_HUB,

On 08/12/2015 10:52 AM, Ján Tomko wrote:

USB disks, redirected devices, host devices and serial devices are supported. --- src/conf/domain_addr.c | 34 +++++++++++++++++++ src/conf/domain_addr.h | 4 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 5 +++ src/qemu/qemu_hotplug.c | 39 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+disk-usb.xml | 1 + 6 files changed, 84 insertions(+)

I ran all the changes through my Coverity checker - figured I'd post this data first...

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 15f4753..42c0837 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1522,3 +1522,37 @@ virDomainUSBAddressReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, VIR_FREE(portstr); return ret; } + + +int +virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + 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); + VIR_DEBUG("Releasing USB addr bus=%u port=%s", info->addr.usb.bus, portstr); + + if (virDomainUSBAddressFindPort(&port, addrs, info) < 0) + goto cleanup; + + if (!*port) { + if ((*port)->nports) {

Coverity complains here ... checking (!*port) and then using (*port) is counter-productive. I assume you meant "if (!port)"

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("usb-hub hotunplug is not yet implemented")); + goto cleanup; + } + VIR_FREE(*port); + } + + ret = 0; + + cleanup: + VIR_FREE(portstr); + return ret; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 8ecf588..fbd9557 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -281,4 +281,8 @@ virDomainUSBAddressReserve(virDomainDefPtr def, virDomainDeviceInfoPtr info, void *data) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int +virDomainUSBAddressRelease(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 048ec68..9c53b13 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -109,6 +109,7 @@ virDomainPCIAddressValidate; virDomainUSBAddressAssign; virDomainUSBAddressGetPortBuf; virDomainUSBAddressGetPortString; +virDomainUSBAddressRelease; virDomainUSBAddressReserve; virDomainUSBAddressSetAddControllers; virDomainUSBAddressSetCreate; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9df542e..277e431 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2106,6 +2106,11 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, info) < 0) VIR_WARN("Unable to release virtio-serial address on %s", NULLSTR(devstr)); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && + priv->usbaddrs && + virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) + VIR_WARN("Unable to release usb address on %s", + NULLSTR(devstr)); }

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8e38153..e72237a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -669,6 +669,7 @@ qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn, char *devstr = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); + bool releaseaddr;

Not initialized for all paths to cleanup - if initialized to false will remove 2 Coverity issues...

for (i = 0; i < vm->def->ndisks; i++) { if (STREQ(vm->def->disks[i]->dst, disk->dst)) { @@ -678,6 +679,16 @@ qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn, } }

+ if (priv->usbaddrs) {

Because you check for priv->usbaddrs here...

+ if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + if (virDomainUSBAddressReserve(NULL, NULL, &disk->info, priv->usbaddrs) < 0) + goto cleanup; + } else if (virDomainUSBAddressAssign(priv->usbaddrs, &disk->info, false) < 0) { + goto cleanup; + } + releaseaddr = true; + } + if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup;

@@ -728,6 +739,8 @@ qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn, virDomainDiskInsertPreAlloced(vm->def, disk);

cleanup: + if (ret < 0 && releaseaddr) + virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);

... coverity complains here since priv->usbaddrs was checked for NULL, but if releaseaddr was initialized to false, then both issues are cleared.

VIR_FREE(devstr); VIR_FREE(drivestr); virObjectUnref(cfg); @@ -1536,6 +1549,16 @@ qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv, return -1; return 1;

+ } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && + chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) { + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + if (virDomainUSBAddressReserve(NULL, NULL, &chr->info, priv->usbaddrs) < 0) + return -1; + } else if (virDomainUSBAddressAssign(priv->usbaddrs, &chr->info, false) < 0) { + return -1; + } + return 1; + } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, @@ -1845,11 +1868,22 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; + bool releaseaddr = false; bool added = false; bool teardowncgroup = false; bool teardownlabel = false; int ret = -1;

+ if (priv->usbaddrs) { + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + if (virDomainUSBAddressReserve(NULL, NULL, hostdev->info, priv->usbaddrs) < 0) + return -1; + } else if (virDomainUSBAddressAssign(priv->usbaddrs, hostdev->info, false) < 0) { + goto cleanup; + } + releaseaddr = true; + } + if (qemuPrepareHostUSBDevices(driver, vm->def->name, &hostdev, 1, 0) < 0) goto cleanup;

@@ -1902,6 +1936,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (added) qemuDomainReAttachHostUSBDevices(driver, vm->def->name, &hostdev, 1); + if (releaseaddr) + virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info); } VIR_FREE(devstr); return ret; @@ -2856,6 +2892,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); + if (priv->usbaddrs) + virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);

virDomainDiskDefFree(disk); return 0; @@ -2949,6 +2987,7 @@ qemuDomainRemoveUSBHostDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev) { qemuDomainReAttachHostUSBDevices(driver, vm->def->name, &hostdev, 1); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); }

static void diff --git a/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-usb.xml b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-usb.xml index 7904c4f..ac69490 100644 --- a/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-usb.xml +++ b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-usb.xml @@ -25,6 +25,7 @@ <target dev='sdq' bus='usb'/> <readonly/> <shareable/> + <address type='usb' bus='0' port='1'/> </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>