On 11/11/18 10:59 PM, Han Han wrote:

Add this function to check if the a usb address is attached to a hub device.

Signed-off-by: Han Han <hhan@redhat.com> --- src/conf/domain_addr.c | 22 ++++++++++++++++++++++ src/conf/domain_addr.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+)

NB: Patches 1-6 were already Reviewed-by me, so I'll start here...

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index e4ed143b76..722bd2c9fe 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -2155,6 +2155,28 @@ virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, }

+bool +virDomainUSBAddressIsAttachedToHub(virDomainDeviceInfoPtr info, + virDomainHubDefPtr hub) +{ + unsigned int *hub_port = hub->info.addr.usb.port; + unsigned int *device_port = info->addr.usb.port; + size_t i;

These 3 can go inside the if or you could have done the reverse logic to: if (hub->info.addr.usb.bus != info->addr.usb.bus) return false;

+ if (hub->info.addr.usb.bus == info->addr.usb.bus) { + for (i = 0; i < VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH; ++i) { + if (hub_port[i] == device_port[i]) + continue; + else if (hub_port[i] == 0 && device_port[i] != 0) + return true; + else + return false;

perhaps a brief comment about what each check means would help.

+ } + } + + return false; +} + + int virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, virDomainDeviceInfoPtr info) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 2a9af9c00b..b1e0714923 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -285,6 +285,11 @@ virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, virDomainDeviceInfoPtr info) ATTRIBUTE_NONNULL(2);

+bool +virDomainUSBAddressIsAttachedToHub(virDomainDeviceInfoPtr info, + virDomainHubDefPtr hub) + ATTRIBUTE_NONNULL(2);

I'm assuming a cut-n-paste, but since both @info and @hub are referenced without checking - both would get the ATTRIBUTE_NONNULL even though it really only matters if someone tries to pass a NULL to the function. John

+ int virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, virDomainDeviceInfoPtr info) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b29c2bf62b..b45a7b92b4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -131,6 +131,7 @@ virDomainPCIControllerModelToConnectType; virDomainUSBAddressAssign; virDomainUSBAddressCountAllPorts; virDomainUSBAddressEnsure; +virDomainUSBAddressIsAttachedToHub; virDomainUSBAddressPortFormatBuf; virDomainUSBAddressPortIsValid; virDomainUSBAddressPresent;

On 11/11/18 10:59 PM, Han Han wrote:

qemuDomainHubIsBusy is to check whether a usb device are attached to the hub device. It will be used for hotunplugging and live device update of hub device.

Signed-off-by: Han Han <hhan@redhat.com> --- src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)

This would need to be merged with the subsequent patch because the new method is static, but it sure makes it easier to review when it's pulled out. Beyond that I find I see there is a virDomainUSBDeviceDefForeach which perhaps could have be used. This code did miss the nserials The whole assign/remove address space isn't something I know really well, but in reading the code I note: 1. When a new hub is created, virDomainUSBAddressHubNew will create a "hub->portmap" to manage the active ports for the hub. This will be inserted into the priv->usbaddrs address set (something important to understand later). 2. When a device is assigned to a hub, virDomainUSBAddressSetAddHub is called which calls virDomainUSBAddressFindPort to find the targetHub for which a virBitmapSetBit on the portmap is done for the port being used. 3. When a device using a hub is removed, virDomainUSBAddressRelease is called by either qemuDomainReleaseDeviceAddress or various disk hot unplug helpers. This in turn calls virDomainUSBAddressFindPort to return the targetHub which is being unplugged. Then a virBitmapClearBit on done for the port being used. 4. Nothing seems to care that an address set entry has an empty bitmap. That is, if the last port is cleared on the hub, there's no automatic removal, although there is automatic add when space is full. I think that's an important distinction. 5. The only time virDomainUSBAddressSetFree is called to free priv->usbaddrs is when a domain is stopped or the domain object is disposed of (call to privateDataFreeFunc). So what does this all mean, well if this device were removed it would likewise call qemuDomainReleaseDeviceAddress in order to "clear" the targetPort bit on the targetHub that this device was using if by chance it was plugged into another hub (daisy chaining). Still, shouldn't checking whether the device about to be deleted hub has anything attached to be as simple as checking that it's own portmap is clear? If it is clear, then removing it wouldn't remove any other devices in collateral damage. However, there's another gotcha. Recall that when a hub is added the priv->usbaddrs is referenced, VIR_EXPAND_N called, and the hub placed into the address set. If you free something in the address set, then the address set is potentially pointing at memory that it no longer owns. Once a add a device to a hub is called, it's going to attempt to reference that address and "may" or "may not" succeed. It may look like it succeeds, but corruption is sure to follow. And from experience, I can tell you that type of corruption is the absolute most difficult thing to find. So you will need to add code to handle shrinking the priv->usbaddrs since you're essentially about to free something in it. You should be able to use logic from patch7 to become a callback/iter function for virDomainUSBDeviceDefForeach (and it does matter if a hub is plugged into the hub you'd be attempting to remove). Study the existing consumers of virDomainUSBDeviceDefForeach - especially how they add hubdef's and hubaddr's. Then consider how would you safely remove. The logic seems to be intertwined with controllers too. John

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1b6cc36bc8..ca73456260 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5332,6 +5332,70 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } }

+static bool qemuDomainHubIsBusy(virDomainObjPtr vm, + virDomainHubDefPtr detach) +{ + size_t i; + virDomainDiskDefPtr disk; + virDomainHubDefPtr hub; + virDomainInputDefPtr input; + virDomainSoundDefPtr sound; + virDomainHostdevDefPtr hostdev; + virDomainRedirdevDefPtr redirdev; + virDomainControllerDefPtr controller; + + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB && + virDomainUSBAddressIsAttachedToHub(&(disk->info), detach)) + return true; + } + + for (i = 0; i < vm->def->nhubs; i++) { + hub = vm->def->hubs[i]; + if (virDomainUSBAddressIsAttachedToHub(&(hub->info), detach)) + return true; + } + + for (i = 0; i < vm->def->ninputs; i++) { + input = vm->def->inputs[i]; + if (input->bus == VIR_DOMAIN_INPUT_BUS_USB && + virDomainUSBAddressIsAttachedToHub(&(input->info), detach)) + return true; + } + + for (i = 0; i < vm->def->nsounds; i++) { + sound = vm->def->sounds[i]; + if (sound->model == VIR_DOMAIN_SOUND_MODEL_USB && + virDomainUSBAddressIsAttachedToHub(&(sound->info), detach)) + return true; + } + + for (i = 0; i < vm->def->nhostdevs; i++) { + hostdev = vm->def->hostdevs[i]; + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && + virDomainUSBAddressIsAttachedToHub(hostdev->info, detach)) + return true; + } + + for (i = 0; i < vm->def->nredirdevs; i++) { + redirdev = vm->def->redirdevs[i]; + if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && + virDomainUSBAddressIsAttachedToHub(&(redirdev->info), detach)) + return true; + } + + for (i = 0; i < vm->def->ncontrollers; i++) { + controller = vm->def->controllers[i]; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_CCID && + virDomainUSBAddressIsAttachedToHub(&(controller->info), detach)) + return false; + } + + return false; +} + int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev,

On 11/11/18 10:59 PM, Han Han wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1375423

Signed-off-by: Han Han <hhan@redhat.com> --- src/qemu/qemu_driver.c | 5 ++- src/qemu/qemu_hotplug.c | 81 ++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 4 ++ 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 774f6ac8b9..2813a00050 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7822,11 +7822,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async); break;

+ case VIR_DOMAIN_DEVICE_HUB: + ret = qemuDomainDetachHubDevice(vm, dev->data.hub, async); + break; + case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: - case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ca73456260..124703b7b2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4965,6 +4965,32 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, }

+static int +qemuDomainRemoveHubDevice(virDomainObjPtr vm, + virDomainHubDefPtr dev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virObjectEventPtr event = NULL; + size_t i; + + VIR_DEBUG("Removing hub device %s from domain %p %s", + dev->info.alias, vm, vm->def->name); + + event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias); + virObjectEventStateQueue(driver->domainEventState, event); + for (i = 0; i < vm->def->nhubs; i++) { + if (vm->def->hubs[i] == dev) + break; + } + qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL); + + virDomainHubDefFree(vm->def->hubs[i]); + VIR_DELETE_ELEMENT(vm->def->hubs, i, vm->def->nhubs); + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5016,13 +5042,16 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock); break;

+ case VIR_DOMAIN_DEVICE_HUB: + ret = qemuDomainRemoveHubDevice(vm, dev->data.hub); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: - case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: @@ -7019,3 +7048,53 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm, qemuDomainResetDeviceRemoval(vm); return ret; } + + +int +qemuDomainDetachHubDevice(virDomainObjPtr vm, + virDomainHubDefPtr def, + bool async) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virDomainHubDefPtr detach; + int ret = -1; + int idx; + + if ((idx = virDomainHubDefFind(vm->def, def)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching hub device not found")); + return -1; + } + + detach = vm->def->hubs[idx]; + if (qemuDomainHubIsBusy(vm, detach)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be detached: device is busy")); + goto cleanup; + }

This is where either the virDomainUSBDeviceDefForeach logic would need to be called in order to determine whether some other device was attached to this hub or some mechanism to get/search usbaddrs for this hub and determine if its "targetHub->portmap" was (virBitmapIsAllClear). More or less the antecedent to virDomainUSBAddressSetAddHub and potentially virDomainUSBAddressSetAddController. John

+ + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveHubDevice(vm, detach); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 19b8950254..5c860c26ac 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -204,4 +204,8 @@ int qemuDomainDetachInputDevice(virDomainObjPtr vm, int qemuDomainDetachVsockDevice(virDomainObjPtr vm, virDomainVsockDefPtr dev, bool async); + +int qemuDomainDetachHubDevice(virDomainObjPtr vm, + virDomainHubDefPtr def, + bool async); #endif /* __QEMU_HOTPLUG_H__ */

On 11/11/18 10:59 PM, Han Han wrote:

Signed-off-by: Han Han <hhan@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml index 88774c55ae..b677f52efc 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,16 @@ <libvirt> <release version="v4.10.0" date="unreleased"> <section title="New features"> + <change> + <summary> + Qemu: Add active and inactive device add or remove for QEMU + USB Hub devices

Running check fails here because <summary> only expects 1 line. John

+ </summary> + <description> + Add the ability to attach or detach a USB Hub device either + for active or inactive guests. + </description> + </change> </section> <section title="Improvements"> </section>