Re: [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: paused-VM CPR drain replays fully completed URBs, not just partial OUTs
--

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

usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

This commit queues the packets suppressed during the CPR drain on a new
cpr_inflight list. If CPR is aborted, the vm-state handler's resume branch
re-issues them on the preserved fd. Control transfers are completed with
USB_RET_IOERROR instead.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 413ddc12ee43..9cc7d72aa63a 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -433,9 +435,10 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
>  /*
>   * Classify a reap during the CPR drain (r->p is still set).  The paused-VM
>   * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
> - * reap for the target to replay; otherwise one that moved no data is suppressed
> - * and one that moved some is kept as a short transfer.  Returns true if
> - * suppressed, so the caller frees the request.
> + * reap; otherwise one that moved no data is suppressed and one that moved some
> + * is kept as a short transfer.  Suppressed packets are queued on cpr_inflight
> + * for the target to replay.  Returns true if suppressed, so the caller frees
> + * the request.
>   */
>  static bool usb_host_req_cpr_replay(USBHostRequest *r,
>                                      struct libusb_transfer *xfer)
> @@ -462,6 +465,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
>              return false;
>          }
>      }
> +    if (r->p->state == USB_PACKET_ASYNC) {
> +        r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
> +    }
>      return true;
>  }

[Severity: Medium]
When cpr_replay_all is set by usb_host_pre_save() (paused-VM fallback), the
whole "if (!r->host->cpr_replay_all) { ... }" block is skipped. Control
then reaches the g_slist_append() for every reaped URB, whatever
xfer->status or xfer->actual_length is.

That includes URBs that finished with LIBUSB_TRANSFER_COMPLETED.

> [ ... ]
>
> +static void usb_host_cpr_reissue(USBHostDevice *s)
> +{

[ ... ]

> +    for (it = s->cpr_inflight; it; it = it->next) {
> +        p = it->data;
> +        /*
> +         * Replay from the start of the TD.  A transfer that already moved some
> +         * data is re-run in full, so an OUT fragment the device consumed may be
> +         * sent twice.  Only the paused-VM pre_save fallback queues such partial
> +         * transfers here; the normal drain delivers them as short transfers.
> +         */
> +        p->actual_length = 0;
> +        if (p->ep->nr == 0) {
> +            /* control endpoint: can't be reconstructed, so error it out */
> +            p->status = USB_RET_IOERROR;
> +            usb_generic_async_ctrl_complete(udev, p);
> +        } else {
> +            usb_host_handle_data(udev, p);

Is the comment here, and the commit message, accurate about which
transfers get replayed?

The commit message says "an OUT fragment already consumed by the device may
be sent twice. Only the paused-VM .pre_save() fallback queues partial
transfers for re-issue". The code comment makes the same claim.

The path is usb_host_pre_save() setting cpr_replay_all = true, then
usb_host_cpr_quiesce(), then usb_host_req_cpr_replay() queuing every reap.
In that path fully completed URBs are queued as well, not only partial
ones.

On resume after an aborted CPR, usb_host_cpr_reissue() then handles them
as follows:

- A completed OUT transfer is sent to the device a second time.

- A completed IN transfer is resubmitted with actual_length reset to 0,
  so the data already read is lost and the device will not resend it.

- A completed control transfer, for example SET_CONFIGURATION or another
  SET_* request, is reported to the guest as USB_RET_IOERROR through
  usb_generic_async_ctrl_complete().

Should the commit message and the comment describe this wider scope? Or
should usb_host_req_cpr_replay() deliver URBs with xfer->status ==
LIBUSB_TRANSFER_COMPLETED normally, even in cpr_replay_all mode, instead
of queueing them for replay?

> [ ... ]

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