Re: [PATCH QEMU HCI-8.0 v7 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] usb-host: cpr_handed_off set before migration outcome is known, skips cleanup on failure
- [Medium] usb-host: cpr_is_incoming() in realize() stops global event reaping with no restart
- [Medium] usb-host: unrealize() clears handoff flag without restarting the global event reaping
- [Low] usb-host: unrealize() forces full reset even if the target may already own the device
- [Low] usb-host: commit message does not match where reaping is stopped and restarted
--

commit d5825316f822699756346d45e052e4ba605a47fe
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: hand off the device across cpr-transfer #VSTOR-137800

This patch hands the usbfs FD and URB reaping off from the source QEMU to
the target QEMU during cpr-transfer. The source stops reaping at quiesce
time and skips device reset, interface release and driver rebind on exit.
The target stops reaping in realize() and restarts it in post_load().

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> +static void usb_host_stop_events(void)
> +{
> +    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
> +
> +    libusb_set_pollfd_notifiers(ctx, NULL, NULL, NULL);
> +    if (poll) {
> +        for (int i = 0; poll[i] != NULL; i++) {
> +            usb_host_del_fd(poll[i]->fd, ctx);
> +        }
> +        free(poll);
> +    }
> +}

[ ... ]

>  static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
>  {
>      if (usb_host_cpr_drain_xfers(s) < 0) {
>          return -1;
>      }
> +    if (mode == MIG_MODE_CPR_TRANSFER) {
> +        usb_host_stop_events();
> +        s->cpr_handed_off = true;
> +    }
>      return 0;
>  }

[Severity: Medium]
Is it safe to set cpr_handed_off here, before the outcome of the migration
is known?

The flag is cleared only in usb_host_cpr_reissue(), which runs on a running
transition, and in usb_host_unrealize().

Consider the case where usb_host_pre_save() calls usb_host_cpr_quiesce() as
the fallback for an already-paused VM, and the migration then fails.  Nothing
resumes the VM, so cpr_handed_off stays true.

usb_host_exit_notifier() then skips the following on every later quit and
only calls libusb_close():

    usb_host_abort_xfers(s);
    usb_host_release_interfaces(s);
    libusb_reset_device(s->dh);
    usb_host_attach_kernel(s);

Can this leave the device without a reset and without the host kernel
driver rebound after a failed or cancelled cpr-transfer?

The commit message says the skip happens only "On success", but the exit
notifier cannot tell success from failure.

[ ... ]

> @@ -1371,16 +1402,19 @@ static int usb_host_close(USBHostDevice *s)
>
>      trace_usb_host_close(s->bus_num, s->addr);
>
> -    usb_host_abort_xfers(s);
> -    usb_host_iso_free_all(s);
> +    if (!s->cpr_handed_off) {
> +        usb_host_abort_xfers(s);
> +        usb_host_iso_free_all(s);
>
> -    if (udev->attached) {
> -        usb_device_detach(udev);
> +        if (udev->attached) {
> +            usb_device_detach(udev);
> +        }
> +
> +        usb_host_release_interfaces(s);
> +        libusb_reset_device(s->dh);
> +        usb_host_attach_kernel(s);
>      }
>
> -    usb_host_release_interfaces(s);
> -    libusb_reset_device(s->dh);
> -    usb_host_attach_kernel(s);
>      libusb_close(s->dh);
>      s->dh = NULL;
>      s->dev = NULL;

[Severity: Medium]
The cpr_handed_off gate here also skips the QEMU-local steps
usb_host_abort_xfers(), usb_host_iso_free_all() and usb_device_detach(),
not only the reset, release and rebind that must not run under the target.

If usb_host_close() is reached while the flag is stale, for example via
usb_host_nodev_bh() or usb_host_auto_check(), s->dh and s->dev are still set
to NULL.  Can that leave a guest-attached device with no libusb handle?

Should the gate cover only the device-affecting calls (release, reset,
rebind)?

[ ... ]

> @@ -1487,6 +1528,14 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
> +        if (cpr_is_incoming()) {
> +            /*
> +             * The source still owns the shared fd until it hands off.  Don't
> +             * reap URBs on it yet, or we would steal the source's in-flight
> +             * completions; .post_load() restarts events once we take over.
> +             */
> +            usb_host_stop_events();
> +        }

[Severity: Medium]
Can this stop event reaping permanently for a device that is not migrated?

cpr_is_incoming() is "incoming_mode != MIG_MODE_NONE", and incoming_mode
does not appear to be reset after the migration completes.

If a usb-host device with hostdevice= is hotplugged on the target after the
CPR migration, usb_host_realize() takes this branch and calls
usb_host_stop_events(), which clears the pollfd notifiers and fd handlers on
the shared libusb ctx.

usb_host_post_load() is the only place that restarts events, and a hotplugged
device has no vmstate section to load.  Would URB completions then stop being
reaped for that device and for every other usb-host device?

[Severity: Low]
This isn't a functional problem, but the commit message and the code differ
in a few places:

    The target doesn't reap until it takes over: it stops events right
    after opening the FD in .realize() and restarts in .post_load().

The condition used is cpr_is_incoming(), which is true for any incoming CPR
mode, including cpr-exec, where the fd is not shared.

    The source stops reaping in .pre_save() and, since it stays alive,
    restarts reaping from the vm-state handler's resume branch on failure.

The stop happens in usb_host_cpr_quiesce(), which usb_host_cpr_vm_state()
calls on the FINISH_MIGRATE transition.  usb_host_pre_save() reaches it only
as a fallback for an already-paused VM.  Could the commit message be updated
to describe this?

[ ... ]

> @@ -1550,6 +1599,12 @@ static void usb_host_unrealize(USBDevice *udev)
> +    /*
> +     * An explicit device_del means we still own the device (a successful
> +     * cpr-transfer handoff exits the process instead), so clear the handoff
> +     * flag to force a full reset/release in usb_host_close().
> +     */
> +    s->cpr_handed_off = false;
>      usb_host_close(s);
>      migrate_del_blocker(&s->cpr_blocker);
>      if (s->cpr_vmse) {
>          qemu_del_vm_change_state_handler(s->cpr_vmse);
>      }

[Severity: Medium]
Does this path leave event reaping stopped for the other usb-host devices?

Suppose usb_host_cpr_quiesce() has called usb_host_stop_events() on the whole
ctx, and the migration then fails or is cancelled while the VM is paused.  No
running transition occurs, so usb_host_cpr_reissue() never calls
usb_host_start_events().

A later device_del reaches usb_host_unrealize(), which sets
cpr_handed_off = false and removes cpr_vmse without calling
usb_host_start_events().  usb_host_init() returns early once ctx exists, so
nothing else restarts reaping.

Can transfers on the other usb-host devices (hostdevice=, autoscan or
bus/addr) hang here, since the stop applies to the shared context?  The
commit message says only one hostdevice= device is supported, but nothing in
the code appears to enforce that.

[Severity: Low]
Can this unconditional clearing of cpr_handed_off cause a reset of a device
the target already owns?

The comment assumes that unrealize means the source still owns the device.
During the handoff window the source has cpr_handed_off = true and the target
may already be reaping.  If a device_del or guest eject reaches
usb_host_unrealize() then, usb_host_close() runs usb_host_abort_xfers(),
usb_host_release_interfaces(), libusb_reset_device() and
usb_host_attach_kernel() on the shared fd.

Is it intended that this disrupts the target's session?

[ ... ]

> @@ -2033,6 +2088,7 @@ static int usb_host_post_load(void *opaque, int version_id)
>      if (cpr_is_incoming()) {
> +        usb_host_start_events();
>          return 0;
>      }

This is the only restart point on the target, and it runs only for devices
whose vmstate section is loaded, as noted in the usb_host_realize() comment
above.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v7-0-044f668789a1@virtuozzo.com?part=10