On Tue, Nov 6, 2018 at 6:47 AM John Ferlan <jfer...@redhat.com> wrote:
> > $SUBJ: > > s/implement/Implement > > On 10/12/18 4:50 AM, Han Han wrote: > > https://bugzilla.redhat.com/show_bug.cgi?id=1375423 > > > > Add the infrastructure to allow a USB Hub device to be hot unplugged. > > > Signed-off-by: Han Han <h...@redhat.com> > > --- > > src/qemu/qemu_driver.c | 5 ++- > > src/qemu/qemu_hotplug.c | 74 ++++++++++++++++++++++++++++++++++++++++- > > src/qemu/qemu_hotplug.h | 4 +++ > > 3 files changed, 81 insertions(+), 2 deletions(-) > > > > This is where things get a bit dicey. Are you sure we can allow this > given that we automagically create hub devices when there's too many USB > devices, see: > > tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.args > tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.xml > > and in the code qemuDomainUSBAddressAddHubs? > > Note that the test XML doesn't have a hub device defined, but yet some > are created. If someone decides to hot unplug one that has something > plugged into it (e.g. in the case of that test XML, some USB Input > device), then what happens? > > Can you test that and ensure the results that you get? > Currently, if you hot-unplug a hub with usb devices attached, these attached usb device will be removed, too. e.g: Start a VM with a hub. 8 usb mouse attached to the hub(Port 3.1~3.8): # virsh qemu-monitor-command rhel7 --hmp info usb Device 0.2, Port 3, Speed 12 Mb/s, Product QEMU USB Hub, ID: hub0 Device 0.2, Port 1, Speed 480 Mb/s, Product QEMU USB Tablet, ID: input0 Device 0.3, Port 3.1, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input3 Device 0.4, Port 3.2, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input4 Device 0.5, Port 3.3, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input5 Device 0.6, Port 3.4, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input6 Device 0.7, Port 3.5, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input7 Device 0.8, Port 3.6, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input8 Device 0.9, Port 3.7, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input9 Device 0.10, Port 3.8, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input10 Device 0.0, Port 2, Speed 1.5 Mb/s, Product USB Redirection Device, ID: redir0 Then hot-unplug the hub by hmp command: # virsh qemu-monitor-command rhel7 --hmp device_del hub0 All usb mouse devices attached to hub disappeared in the live xml. And no error in the qemu VM log. However, I am not sure if other usb devices attached to hub will be removed without error... I see you've essentially copied the steps that an Input device would > take; however, I'd suspect a Hub device is a bit more special and we may > need to process the various USB devices to make sure there isn't one > using the Hub device by port... Even worse if it's port=1 and there's a > port=1.1 around that equates to more ports (like from the test). > > The question becomes - can we determine which port a USB device is using > via the guest status/live XML? Would the qemu del device error out or > happily accept deleting a hub with attached ports? Or would it just drop > all those attached devices? > I think that is not the expected result above. It better to refer to the scsi controller in libvirt. When you hot-unplug a scsi controller with scsi disk attached, you will get: # virsh detach-device rhel7 /tmp/scsi.xml error: Failed to detach device from /tmp/scsi.xml error: operation failed: device cannot be detached: device is busy > Thanks for your review and valuable advice. > > Logically what's here would appear to work and is essentially similar to > the Input devices code, so from that aspect things look OK - it's this > one (hah) minor detail. > > Tks - > > John > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > index de764a7f1c..c8a6d98dc0 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 1b6cc36bc8..87749ec011 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: > > @@ -6955,3 +6984,46 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm, > > qemuDomainResetDeviceRemoval(vm); > > return ret; > > } > > + > > + > > +int > > +qemuDomainDetachHubDevice(virDomainObjPtr vm, > > + virDomainHubDefPtr def, > > + bool async) > > +{ > > + qemuDomainObjPrivatePtr priv = vm->privateData; > > + virQEMUDriverPtr driver = priv->driver; > > + virDomainHubDefPtr hub; > > + 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; > > + } > > + hub = vm->def->hubs[idx]; > > + > > + if (!async) > > + qemuDomainMarkDeviceForRemoval(vm, &hub->info); > > + > > + qemuDomainObjEnterMonitor(driver, vm); > > + if (qemuMonitorDelDevice(priv->mon, hub->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, hub); > > + } > > + > > + cleanup: > > + if (!async) > > + qemuDomainResetDeviceRemoval(vm); > > + return ret; > > +} > > diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h > > index 444333c4df..0f205ff54b 100644 > > --- a/src/qemu/qemu_hotplug.h > > +++ b/src/qemu/qemu_hotplug.h > > @@ -208,4 +208,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__ */ > > > -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list