Re: [PATCH QEMU HCI-8.0 v7 11/14] 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 c0dc143b080568d7b0883600b0776ce496b368ca
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb: migrate the interface altsetting #VSTOR-137800

This patch adds a vmstate subsection to the shared USBDevice vmstate that
carries the altsetting[] array. The subsection is emitted only in the CPR
migration modes and only when some altsetting[] entry is non-zero.

> 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;
> +}

[Severity: Low]
Is this gate too broad? usb_altsetting_needed() checks only
cpr_incoming_needed() and whether any dev->altsetting[i] is non-zero. It
does not check the device type, but the stated reason (the CPR target
rebuilds its endpoints) seems to hold only for usb-host.

[ ... ]

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

The subsection is attached to the shared vmstate_usb_device, so it is
emitted and loaded for every USB device type, including emulated devices.

For an emulated device, usb_desc_set_interface() in hw/usb/desc.c updates
the three together:

    old = dev->altsetting[index];
    dev->altsetting[index] = value;
    dev->ifaces[index] = iface;
    usb_desc_ep_init(dev);

After a CPR load, only altsetting[] is restored. ifaces[] and the endpoint
table stay at altsetting 0.

Can GET_INTERFACE then report the guest's non-zero altsetting while the
device model behaves as if altsetting 0 were active? Before this patch the
state was wrong but self-consistent. It is still wrong after the patch, so
emulated devices gain nothing from it.

The consumer in this series is only in usb_host_post_load() in
hw/usb/host-libusb.c, which calls usb_host_cpr_reclaim_interfaces() and
usb_host_ep_update(). Nothing re-applies ifaces[] or calls
usb_desc_ep_init() for emulated devices.

I did not verify which emulated devices have multiple altsettings or can be
CPR-migrated, so this may be of limited reach. Would it make sense to limit
the subsection to usb-host, or to restore ifaces[] and the endpoints in a
generic post_load?

Separately, the commit message says "The active altsetting of each
interface is host controller state". Is that accurate? altsetting[] is a
field of USBDevice, not of the host controller.

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