Re: [PATCH QEMU HCI-8.0 v7 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 stays armed if post-CPR kick submits no ISO TD
- [Medium] usb-xhci: CPR iso desync worked around with fake missed-service error
--

commit 775b0f619a3e5117de7a9bf189a6e7024d14f114
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 a CPR resume is incoming. The next isochronous
TD seen by 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 ahead 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;
> +    }
> +

[Severity: Medium]
Can cpr_iso_resync stay armed after the post-CPR kick?

usb_xhci_post_load() sets the flag for every EP_RUNNING isochronous
endpoint, without checking whether a TD is pending, and then arms
kick_timer.

The kick reaches xhci_kick_epctx(), which calls
xhci_set_ep_state(..., EP_RUNNING). The clear above only runs when
state != EP_RUNNING, so the flag survives.

xhci_kick_epctx() can also leave without calling xhci_submit(). It returns
early when xhci_slot_ok() fails, when the endpoint is halted, or when
!ring->dequeue. It breaks out of the loop when
xhci_ring_chain_length() <= 0:

    length = xhci_ring_chain_length(xhci, ring);
    if (length <= 0) {
        if (epctx->type == ET_ISO_OUT || epctx->type == ET_ISO_IN) {
            ... CC_RING_OVERRUN : CC_RING_UNDERRUN;
        }
        break;
    }

It also breaks out when xhci_ep_alloc_xfer() returns NULL. None of these
paths clear the flag.

If the guest queues an isochronous TD later, long after resume, will that
TD hit the resync branch in xhci_submit()? It would then be completed with
CC_MISSED_SERVICE_ERROR and never sent to the device.

This differs from the comment and the commit message, which describe the
window as the "first kick" or the "first isochronous transfer after a CPR
resume". As written, the window ends at the first isochronous TD submitted,
however late it arrives.

An isochronous endpoint that is EP_RUNNING but idle across CPR, for
example an opened USB audio device with no TDs queued yet, would lose one
valid TD this way.

Should the flag be cleared at the end of the post-CPR kick, whether or not
a TD was submitted?

[ ... ]

> @@ -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]
Is a fabricated CC_MISSED_SERVICE_ERROR the right way to handle this?

In xhci_submit(), the first isochronous TD per endpoint is completed with
an error and never sent to the device. For ISO IN the data is lost, and for
ISO OUT the data is never sent.

Recovery depends on the guest driver's skip logic. The commit message
describes Linux behaviour. Will other guests such as Windows or BSD resync
from a Missed Service event? That is not established by the patch.

The commit message says the controller's "transient transfer and scheduling
state" no longer matches what the guest consumed, but it does not say which
state diverges. It also does not say why the dequeue pointer in the endpoint
context is insufficient.

In usb_xhci_post_load(), the endpoint is rebuilt only through
xhci_alloc_epctx() and xhci_init_epctx():

    xhci_init_epctx(epctx, pctx, ep_ctx);
    epctx->state = state;

After this, mfindex_last is 0 and the transfer list is empty. Could
restoring or re-deriving that scheduling state be a better fix than
discarding the TD?

[ ... ]

> @@ -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;
> +                }
>                  /* kick endpoint after vmload is finished */
>                  timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>              }

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