Re: [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: cpr_is_incoming() in post_load stays true for the whole process lifetime
--

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

usb-host: preserve hostdev FD during CPR migration #VSTOR-137800

This patch saves the host device FD in the CPR registry at realize time
and reuses it on the CPR target. It also deletes the FD from the registry
in the usb_host_open() failure path and in usb_host_close(). Finally, it
makes usb_host_post_load() skip the detach/rescan bottom half when CPR is
incoming.

> 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

[ ... ]

> @@ -1764,6 +1785,16 @@ static int usb_host_post_load(void *opaque, int version_id)
>  {
>      USBHostDevice *dev = opaque;
>  
> +    /*
> +     * For CPR migration, device wasn't released/reset, and the guest
> +     * is unaware of the switchover.  The detach/rescan performed in
> +     * usb_host_post_load_bh() only exists for cross-host migration.
> +     * Skip it for CPR.
> +     */
> +    if (cpr_is_incoming()) {
> +        return 0;
> +    }
> +

[Severity: Low]
Does cpr_is_incoming() stay true for the whole lifetime of a CPR target
process, rather than only during the CPR vmstate load?

cpr_is_incoming() is defined in migration/cpr.c as:

    return incoming_mode != MIG_MODE_NONE;

incoming_mode is set via cpr_set_incoming_mode() in cpr_state_load(), and
nothing in migration/cpr.c appears to reset it to MIG_MODE_NONE afterwards
(checked at the end of the series).

If a later, non-CPR vmstate load of a usb-host device happens in the same
process (for example loadvm of a snapshot), usb_host_post_load() would
return early. The detach/rescan emulation in usb_host_post_load_bh() would
then be skipped, and in the final series state the CPR reclaim path would
run instead.

This is speculative, since it depends on a snapshot or other vmstate load
occurring in a CPR target process, and I did not confirm that this is a
supported flow. Is there a way to restrict this check to the CPR load
itself?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=4