<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 6, 2018 at 6:47 AM John Ferlan <<a href="mailto:jferlan@redhat.com">jferlan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
$SUBJ:<br>
<br>
s/implement/Implement<br>
<br>
On 10/12/18 4:50 AM, Han Han wrote:<br>
> <a href="https://bugzilla.redhat.com/show_bug.cgi?id=1375423" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/show_bug.cgi?id=1375423</a><br>
> <br>
<br>
Add the infrastructure to allow a USB Hub device to be hot unplugged.<br>
<br>
> Signed-off-by: Han Han <<a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a>><br>
> ---<br>
> src/qemu/qemu_driver.c | 5 ++-<br>
> src/qemu/qemu_hotplug.c | 74 ++++++++++++++++++++++++++++++++++++++++-<br>
> src/qemu/qemu_hotplug.h | 4 +++<br>
> 3 files changed, 81 insertions(+), 2 deletions(-)<br>
> <br>
<br>
This is where things get a bit dicey. Are you sure we can allow this<br>
given that we automagically create hub devices when there's too many USB<br>
devices, see:<br>
<br>
tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.args<br>
tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.xml<br>
<br>
and in the code qemuDomainUSBAddressAddHubs?<br>
<br>
Note that the test XML doesn't have a hub device defined, but yet some<br>
are created. If someone decides to hot unplug one that has something<br>
plugged into it (e.g. in the case of that test XML, some USB Input<br>
device), then what happens? <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Can you test that and ensure the results that you get?<br></blockquote><div>Currently, if you hot-unplug a hub with usb devices attached, these attached usb</div><div>device will be removed, too. e.g: <br></div><div><br></div><div>Start a VM with a hub. 8 usb mouse attached to the hub(Port 3.1~3.8):</div><div># virsh qemu-monitor-command rhel7 --hmp info usb<br> Device 0.2, Port 3, Speed 12 Mb/s, Product QEMU USB Hub, ID: hub0<br> Device 0.2, Port 1, Speed 480 Mb/s, Product QEMU USB Tablet, ID: input0<br> Device 0.3, Port 3.1, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input3<br> Device 0.4, Port 3.2, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input4<br> Device 0.5, Port 3.3, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input5<br> Device 0.6, Port 3.4, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input6<br> Device 0.7, Port 3.5, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input7<br> Device 0.8, Port 3.6, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input8<br> Device 0.9, Port 3.7, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input9<br> Device 0.10, Port 3.8, Speed 12 Mb/s, Product QEMU USB Mouse, ID: input10<br> Device 0.0, Port 2, Speed 1.5 Mb/s, Product USB Redirection Device, ID: redir0</div><div><br></div><div>Then hot-unplug the hub by hmp command:</div><div># virsh qemu-monitor-command rhel7 --hmp device_del hub0</div><div><br></div><div>All usb mouse devices attached to hub disappeared in the live xml. And no error</div><div>in the qemu VM log. However, I am not sure if other usb devices attached to hub</div><div>will be removed without error...<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I see you've essentially copied the steps that an Input device would<br>
take; however, I'd suspect a Hub device is a bit more special and we may<br>
need to process the various USB devices to make sure there isn't one<br>
using the Hub device by port... Even worse if it's port=1 and there's a<br>
port=1.1 around that equates to more ports (like from the test).<br>
<br>
The question becomes - can we determine which port a USB device is using<br>
via the guest status/live XML? Would the qemu del device error out or<br>
happily accept deleting a hub with attached ports? Or would it just drop<br>
all those attached devices?<br></blockquote><div> </div><div><div>I think that is not the expected result above. It better to refer to the scsi controller in libvirt.</div><div>When you hot-unplug a scsi controller with scsi disk attached, you will get:</div><div># virsh detach-device rhel7 /tmp/scsi.xml<br>error: Failed to detach device from /tmp/scsi.xml<br>error: operation failed: device cannot be detached: device is busy<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
</blockquote> </div><div>Thanks for your review and valuable advice.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Logically what's here would appear to work and is essentially similar to<br>
the Input devices code, so from that aspect things look OK - it's this<br>
one (hah) minor detail.<br>
<br>
Tks -<br>
<br>
John<br>
<br>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c<br>
> index de764a7f1c..c8a6d98dc0 100644<br>
> --- a/src/qemu/qemu_driver.c<br>
> +++ b/src/qemu/qemu_driver.c<br>
> @@ -7822,11 +7822,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,<br>
> ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);<br>
> break;<br>
> <br>
> + case VIR_DOMAIN_DEVICE_HUB:<br>
> + ret = qemuDomainDetachHubDevice(vm, dev->data.hub, async);<br>
> + break;<br>
> +<br>
> case VIR_DOMAIN_DEVICE_FS:<br>
> case VIR_DOMAIN_DEVICE_SOUND:<br>
> case VIR_DOMAIN_DEVICE_VIDEO:<br>
> case VIR_DOMAIN_DEVICE_GRAPHICS:<br>
> - case VIR_DOMAIN_DEVICE_HUB:<br>
> case VIR_DOMAIN_DEVICE_SMARTCARD:<br>
> case VIR_DOMAIN_DEVICE_MEMBALLOON:<br>
> case VIR_DOMAIN_DEVICE_NVRAM:<br>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c<br>
> index 1b6cc36bc8..87749ec011 100644<br>
> --- a/src/qemu/qemu_hotplug.c<br>
> +++ b/src/qemu/qemu_hotplug.c<br>
> @@ -4965,6 +4965,32 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,<br>
> }<br>
> <br>
> <br>
> +static int<br>
> +qemuDomainRemoveHubDevice(virDomainObjPtr vm,<br>
> + virDomainHubDefPtr dev)<br>
> +{<br>
> + qemuDomainObjPrivatePtr priv = vm->privateData;<br>
> + virQEMUDriverPtr driver = priv->driver;<br>
> + virObjectEventPtr event = NULL;<br>
> + size_t i;<br>
> +<br>
> + VIR_DEBUG("Removing hub device %s from domain %p %s",<br>
> + dev->info.alias, vm, vm->def->name);<br>
> +<br>
> + event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);<br>
> + virObjectEventStateQueue(driver->domainEventState, event);<br>
> + for (i = 0; i < vm->def->nhubs; i++) {<br>
> + if (vm->def->hubs[i] == dev)<br>
> + break;<br>
> + }<br>
> + qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);<br>
> +<br>
> + virDomainHubDefFree(vm->def->hubs[i]);<br>
> + VIR_DELETE_ELEMENT(vm->def->hubs, i, vm->def->nhubs);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +<br>
> int<br>
> qemuDomainRemoveDevice(virQEMUDriverPtr driver,<br>
> virDomainObjPtr vm,<br>
> @@ -5016,13 +5042,16 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,<br>
> ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock);<br>
> break;<br>
> <br>
> + case VIR_DOMAIN_DEVICE_HUB:<br>
> + ret = qemuDomainRemoveHubDevice(vm, dev->data.hub);<br>
> + break;<br>
> +<br>
> case VIR_DOMAIN_DEVICE_NONE:<br>
> case VIR_DOMAIN_DEVICE_LEASE:<br>
> case VIR_DOMAIN_DEVICE_FS:<br>
> case VIR_DOMAIN_DEVICE_SOUND:<br>
> case VIR_DOMAIN_DEVICE_VIDEO:<br>
> case VIR_DOMAIN_DEVICE_GRAPHICS:<br>
> - case VIR_DOMAIN_DEVICE_HUB:<br>
> case VIR_DOMAIN_DEVICE_SMARTCARD:<br>
> case VIR_DOMAIN_DEVICE_MEMBALLOON:<br>
> case VIR_DOMAIN_DEVICE_NVRAM:<br>
> @@ -6955,3 +6984,46 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,<br>
> qemuDomainResetDeviceRemoval(vm);<br>
> return ret;<br>
> }<br>
> +<br>
> +<br>
> +int<br>
> +qemuDomainDetachHubDevice(virDomainObjPtr vm,<br>
> + virDomainHubDefPtr def,<br>
> + bool async)<br>
> +{<br>
> + qemuDomainObjPrivatePtr priv = vm->privateData;<br>
> + virQEMUDriverPtr driver = priv->driver;<br>
> + virDomainHubDefPtr hub;<br>
> + int ret = -1;<br>
> + int idx;<br>
> +<br>
> + if ((idx = virDomainHubDefFind(vm->def, def)) < 0) {<br>
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",<br>
> + _("matching hub device not found"));<br>
> + return -1;<br>
> + }<br>
> + hub = vm->def->hubs[idx];<br>
> +<br>
> + if (!async)<br>
> + qemuDomainMarkDeviceForRemoval(vm, &hub->info);<br>
> +<br>
> + qemuDomainObjEnterMonitor(driver, vm);<br>
> + if (qemuMonitorDelDevice(priv->mon, hub->info.alias)) {<br>
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));<br>
> + goto cleanup;<br>
> + }<br>
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)<br>
> + goto cleanup;<br>
> +<br>
> + if (async) {<br>
> + ret = 0;<br>
> + } else {<br>
> + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)<br>
> + ret = qemuDomainRemoveHubDevice(vm, hub);<br>
> + }<br>
> +<br>
> + cleanup:<br>
> + if (!async)<br>
> + qemuDomainResetDeviceRemoval(vm);<br>
> + return ret;<br>
> +}<br>
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h<br>
> index 444333c4df..0f205ff54b 100644<br>
> --- a/src/qemu/qemu_hotplug.h<br>
> +++ b/src/qemu/qemu_hotplug.h<br>
> @@ -208,4 +208,8 @@ int qemuDomainDetachInputDevice(virDomainObjPtr vm,<br>
> int qemuDomainDetachVsockDevice(virDomainObjPtr vm,<br>
> virDomainVsockDefPtr dev,<br>
> bool async);<br>
> +<br>
> +int qemuDomainDetachHubDevice(virDomainObjPtr vm,<br>
> + virDomainHubDefPtr def,<br>
> + bool async);<br>
> #endif /* __QEMU_HOTPLUG_H__ */<br>
> <br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">-----------------------------------<br></div><div dir="ltr">Han Han<br>Quality Engineer<br>Redhat.<br><br>Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a><br>Phone: +861065339333<br></div></div></div></div></div></div></div></div></div></div></div>