Re: [PATCH QEMU HCI-8.0 v6 11/12] 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 fakes a missed-service error and drops the TD without fixing iso state
- [Low] usb-xhci: cpr_iso_resync flag is never cleared on idle, stop or reset, so a later in-sync iso TD is dropped
--

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

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

This patch adds a cpr_iso_resync flag to XHCIEPContext. usb_xhci_post_load() sets it for running isochronous endpoints when CPR is incoming. The next isochronous TD submitted through xhci_submit() is then completed with CC_MISSED_SERVICE_ERROR without being sent to the device. The aim is to make the guest driver resync after resume.

> 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

[ ... ]

> @@ -1811,6 +1813,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 this early return leave the isochronous scheduling state unrebuilt?

The branch returns before xhci_calc_iso_kick() and
xhci_check_intr_iso_kick(), and before xhci_setup_packet() and
usb_handle_packet(). So epctx->mfindex_last is not updated for the
dropped TD and stays at its initial zero value. The TD is also never
passed to the device. An ISO OUT frame is lost, and an ISO IN buffer is
never filled.

Does the resync then depend entirely on the guest driver reacting to
CC_MISSED_SERVICE_ERROR? The commit message only describes the Linux
behaviour. Would other guests handle the synthetic error the same way?

[Severity: Low]
Can the flag stay set for longer than "the first isochronous transfer
after a CPR resume"?

The flag is consumed only here in xhci_submit(), on whichever isochronous
TD is submitted next. Consider xhci_kick_epctx():

    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 event
        }
        break;
    }

If the ring is empty at resume time, the loop breaks before xhci_submit().
The same happens if xhci_slot_ok() fails or the endpoint is halted.
The flag then survives the resume kick. The stop, reset and set-dequeue
paths do not clear it either.

Can a later isochronous TD that is already in sync then be dropped with a
spurious CC_MISSED_SERVICE_ERROR?

[ ... ]

> @@ -3510,6 +3525,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;
> +                }

[Severity: Medium]
Why is the resync restricted to cpr_is_incoming()?

usb_xhci_post_load() rebuilds the endpoint contexts through
xhci_alloc_epctx() and xhci_init_epctx() in the same way for ordinary
live migration. The commit message says that "we rebuild the controller's
transient transfer and scheduling state out of guest memory" across CPR.
A normal-migration destination gets the same rebuilt state but no resync.

Could the commit message explain why only CPR is affected?

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