Re: [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb-xhci: cpr_iso_resync flag is not consumed by an empty-ring kick and drops a later valid TD
- [Medium] usb-xhci: CPR iso resync relies on a fabricated CC_MISSED_SERVICE_ERROR that only Linux guests may handle
--

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

hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800

This patch sets a per-endpoint flag in usb_xhci_post_load() for running
isochronous endpoints when CPR is incoming. The first isochronous TD
that reaches xhci_submit() is then completed with CC_MISSED_SERVICE_ERROR
instead of being sent to the device. The aim is to make the guest driver
skip and resync.

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

[ ... ]

> @@ -1047,6 +1049,11 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
>      ctx[0] &= ~EP_STATE_MASK;
>      ctx[0] |= state;
>  
> +    if (state != EP_RUNNING) {
> +        /* a stop/reset before the first kick ends the post-CPR resync window */
> +        epctx->cpr_iso_resync = false;
> +    }
> +

[ ... ]

> @@ -1811,6 +1818,19 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
>          xfer->pkts = 1;
>          xfer->iso_xfer = true;
>          xfer->timed_xfer = true;
> +        if (epctx->cpr_iso_resync) {
> +            /*
> +             * First iso TD after a CPR resume: our transfer state was
> +             * rebuilt from guest memory and no longer lines up with what
> +             * the guest consumed.  Report a missed service, like real HW,
> +             * to kick the driver's skip logic into resyncing.
> +             */
> +            epctx->cpr_iso_resync = false;
> +            xfer->status = CC_MISSED_SERVICE_ERROR;
> +            xhci_xfer_report(xfer);
> +            xfer->complete = 1;
> +            return 0;
> +        }

[Severity: Medium]
Can a later, correctly ordered isochronous TD be dropped here?

The comment in xhci_set_ep_state() says the resync window ends at the
first kick. The code ends it only at the first isochronous submit or at
the first non-running state change.

The post-load kick runs xhci_kick_epctx(). That function can return
without reaching xhci_submit():

    - it returns on !xhci_slot_ok()
    - it returns on !ring->dequeue
    - it breaks out of the loop when xhci_ring_chain_length() <= 0,
      which for ET_ISO_OUT/ET_ISO_IN only reports CC_RING_OVERRUN or
      CC_RING_UNDERRUN

xhci_kick_epctx() then calls xhci_set_ep_state(..., EP_RUNNING). The new
clearing code only fires for state != EP_RUNNING, so cpr_iso_resync
stays set.

The next isochronous TD the guest queues may arrive long after resume
and be in sync. It would hit this block in xhci_submit(), never be sent
to the device, and the guest would get a CC_MISSED_SERVICE_ERROR. For
iso OUT this loses one chunk of data and makes the guest run its skip
logic needlessly.

Should cpr_iso_resync be cleared when the post-load kick finishes,
rather than only on the first iso submit?

[Severity: Medium]
Does this approach depend on guest-specific handling of
CC_MISSED_SERVICE_ERROR?

The commit message says the error "arms the guest driver's skip logic".
That matches Linux xhci-hcd behaviour (ep->skip). Nothing in the patch
shows how other guests would handle this fabricated event. If a guest
treats it as a hard error or ignores it, the endpoint could stay wedged,
and the first TD's data is dropped anyway.

> @@ -3510,6 +3530,11 @@ static int usb_xhci_post_load(void *opaque, int version_id)
>              xhci_init_epctx(epctx, pctx, ep_ctx);
>              epctx->state = state;
>              if (state == EP_RUNNING) {
> +                if (cpr_is_incoming() &&
> +                    (epctx->type == ET_ISO_IN || epctx->type == ET_ISO_OUT)) {
> +                    /* only iso streams need resyncing; see xhci_submit() */
> +                    epctx->cpr_iso_resync = true;
> +                }

Also, usb_xhci_post_load() rebuilds the same transient state, including
mfindex_last via xhci_init_epctx(), for ordinary incoming migration.

Only the cpr_is_incoming() case gets the resync here.

Is the difference between CPR and regular incoming migration
intentional? If so, could the commit message explain it?

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