Re: [PATCH v2 7/8] qemu: implement usb hub device hotunplug

[Han Han <hhan@xxxxxxxxxx>]

On Tue, Nov 6, 2018 at 6:47 AM John Ferlan <jferlan@xxxxxxxxxx> 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 <hhan@xxxxxxxxxx>
> ---
>  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: hhan@xxxxxxxxxx
Phone: +861065339333

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list