Re: [PATCH QEMU HCI-8.0 v6 08/12] 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 4 potential issue(s) to consider:
- [Medium] usb-host: cpr_handed_off is set before the handoff outcome is known and never cleared on close
- [Medium] usb-host: target restarts URB reaping in post_load() before migration is committed
- [Medium] usb-host: per-device CPR handoff stops and restarts events for the whole libusb context
- [Low] usb-host: commit message says .exit() closes the FD, but exit notifier only calls libusb_close()
--

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

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

This patch makes the usb-host device hand off URB reaping together with the
usbfs fd across cpr-transfer. The target stops reaping until post_load. The
source stops reaping when it quiesces, restarts on resume after a failure,
and skips device teardown at exit after a successful handoff.

> 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 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_cpr_stop_events();
> +        s->cpr_handed_off = true;
> +    }
> +    return 0;
> +}

[Severity: Medium]
Is cpr_handed_off being set too early here? It is set from the vm-state
FINISH_MIGRATE branch or from usb_host_pre_save(), before the outcome of
the migration is known. The flag then means "handed off, awaiting outcome"
rather than "handoff succeeded".

The only place that clears it is usb_host_cpr_reissue(), which runs only
when the VM resumes with cpr_drained set and s->dh non-NULL.

Consider a failed or cancelled cpr-transfer where the VM stays paused, and
the user then quits or does device_del. Both usb_host_exit_notifier() and
usb_host_close() take the skip path:

    if (!s->cpr_handed_off) {
        usb_host_abort_xfers(s);
        ...
        usb_host_attach_kernel(s);
    }

The source is still the only owner of the device in that case. Does this
leave the interfaces claimed, skip libusb_reset_device(), and never rebind
the host driver? In usb_host_close() the guest port also stays attached,
and the iso rings are not freed.

usb_host_close() also sets s->dh = NULL without clearing the flag. After
that, usb_host_cpr_vm_state() returns early on !s->dh, so
usb_host_cpr_reissue() can never clear it. Would a later reopen of the same
USBHostDevice through autoscan then inherit cpr_handed_off == true and
cpr_drained?

Could the flag be cleared in usb_host_close(), or the skip decision be
based on a state that is only set once the migration has completed?

[ ... ]

> @@ -1344,10 +1365,17 @@ static void usb_host_exit_notifier(struct Notifier *n, void *data)
>      USBHostDevice *s = container_of(n, USBHostDevice, exit);
>  
>      if (s->dh) {
> +        /*
> +         * Handed to new QEMU across cpr-transfer: do not reset, release
> +         * interfaces or rebind the host driver underneath it; just close
> +         * our handle.
> +         */
> +        if (!s->cpr_handed_off) {
> +            usb_host_abort_xfers(s);
> +            usb_host_release_interfaces(s);
> +            libusb_reset_device(s->dh);
> +            usb_host_attach_kernel(s);
> +        }
>          libusb_close(s->dh);
>      }
>  }

[Severity: Low]
This isn't a bug, but the commit message says "On success the source's
.exit() leaves the device untouched ... and just closes its own copy of the
FD."

usb_host_exit_notifier() only calls libusb_close(s->dh). That does not
close the application-owned usbfs fd of a libusb_wrap_sys_device() handle.
s->hostfd is closed only in usb_host_close(). Could the message be
corrected?

The message also says "The source stops reaping in .pre_save()". Normally
this happens in the vm-state handler through usb_host_cpr_quiesce().
pre_save only does it as a fallback when the VM was already paused.

[ ... ]

> @@ -2000,6 +2042,7 @@ static int usb_host_post_load(void *opaque, int version_id)
>       * rescan in usb_host_post_load_bh() -- those are for cross-host migration.
>       */
>      if (cpr_is_incoming()) {
> +        usb_host_cpr_restart_events();
>          return 0;
>      }

[Severity: Medium]
Can the target start reaping too early here? The target calls
usb_host_cpr_restart_events() when the device section is loaded, not when
migration has completed.

Suppose another device section fails to load, or the channel breaks, while
the target is still alive. The source then resumes, and usb_host_cpr_reissue()
calls usb_host_cpr_restart_events() on its side:

    if (s->cpr_handed_off) {
        usb_host_cpr_restart_events();
        s->cpr_handed_off = false;
    }

Both processes could then reap URB completions on the shared usbfs fd. That
is the isochronous wedge this patch is meant to prevent. Is anything making
the target's takeover conditional on the source having committed? This
window is narrow and depends on the target surviving a failed migration.

[ ... ]

> +static void usb_host_cpr_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 void usb_host_cpr_restart_events(void)
> +{
> +    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
> +
> +    libusb_set_pollfd_notifiers(ctx, usb_host_add_fd, usb_host_del_fd, ctx);
> +    if (poll) {
> +        for (int i = 0; poll[i] != NULL; i++) {
> +            usb_host_add_fd(poll[i]->fd, poll[i]->events, ctx);
> +        }
> +        free(poll);
> +    }
> +}

[Severity: Medium]
Do these helpers act on more than the one device being handed off? Both
call libusb_set_pollfd_notifiers(ctx, ...) and walk all of
libusb_get_pollfds(ctx) on the static libusb context, while they are driven
from per-device hooks (usb_host_realize(), usb_host_post_load(),
usb_host_pre_save() and the vm-state handler) and the handoff state
(cpr_handed_off) is per device.

With more than one hostdevice= usb-host device:

On the target, the first device's usb_host_post_load() calls
usb_host_cpr_restart_events() and re-enables reaping for every device,
including devices whose post_load has not yet run and whose source side may
not be handed off.

On the source, the first device's usb_host_cpr_reissue() after a failure
restarts events for devices that are still handed off.

While the notifiers are NULL, fds of devices realized or hot-added in that
window are never registered either.

Is the multi-device case intended to be supported? The conditional blocker
added later in the series ("usb-host: make CPR migration blocker
conditional") still seems to allow several hostdevice= devices with ids.

[ ... ]

> @@ -1455,6 +1489,14 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
>              error_setg(errp, "failed to open host usb device %s", s->hostdevice);
>              return;
>          }
> +        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_cpr_stop_events();
> +        }

In usb_host_realize() on the target, does this disable event handling for
the whole libusb context whenever any hostdevice= device is realized? See
the previous comment on usb_host_cpr_stop_events().

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v6-0-8688ec9576d1@virtuozzo.com?part=8