Re: [PATCH QEMU HCI-8.0 v6 09/12] usb: migrate the interface altsetting #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: altsetting subsection in generic vmstate_usb_device has no consumer for emulated devices
--

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

usb: migrate the interface altsetting #VSTOR-137800

This commit adds a vmstate subsection to the generic vmstate_usb_device
that migrates the USBDevice altsetting[] array. The subsection is emitted
only in the CPR migration modes and only when some interface is on a
non-zero altsetting.

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

[ ... ]

> +static bool usb_altsetting_needed(void *opaque)
> +{
> +    USBDevice *dev = opaque;
> +    int i;
> +
> +    /*
> +     * Only the CPR target consumes this, to rebuild its endpoints; other
> +     * migrations reopen the device and reconstruct the altsetting.
> +     */
> +    if (!cpr_incoming_needed(NULL)) {
> +        return false;
> +    }
> +    for (i = 0; i < USB_MAX_INTERFACES; i++) {
> +        if (dev->altsetting[i]) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +static const VMStateDescription vmstate_usb_device_altsetting = {
> +    .name = "USBDevice/altsetting",

[Severity: Low]
Is this gate specific enough to the devices that consume the state?
usb_altsetting_needed() checks only cpr_incoming_needed() and whether
some dev->altsetting[i] is non-zero. It does not check whether the
device is a usb-host device.

The subsection lives in the generic vmstate_usb_device, so an emulated
USB device that supports SET_INTERFACE and sits on a non-zero altsetting
(an audio-style device, for example) will also emit and load
altsetting[] in CPR modes.

The generic usb_device_post_load() only recomputes dev->attached:

    if (dev->state == USB_STATE_NOTATTACHED) {
        dev->attached = false;
    } else {
        dev->attached = true;
    }

Nothing there rebuilds dev->ifaces[] or calls usb_desc_ep_init() from
the loaded altsetting[]. On the CPR target such a device could report
the restored altsetting via GET_INTERFACE while its endpoints still
follow altsetting 0.

No emulated device that triggers this today has been identified, so this
is speculative.

The later patch in the series, "usb-host: reconfigure endpoints on CPR
incoming", adds usb_host_cpr_reclaim_interfaces(), which range-checks the
migrated altsetting. It also adds a usb_host_ep_update() call in
usb_host_post_load() and changes the assert in usb_host_ep_update() to
check alt >= 0. That covers the usb-host consumer and the unvalidated
index concern. The series adds no consumer for non-usb-host devices.

Should the subsection be restricted to usb-host devices, or should the
generic post_load rebuild the interface and endpoint state from
altsetting[]?

> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = usb_altsetting_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_INT32_ARRAY(altsetting, USBDevice, USB_MAX_INTERFACES),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};

[ ... ]

> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_usb_device_altsetting,
> +        NULL
>      }
>  };

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