Re: [PATCH QEMU HCI-8.0 v6 06/12] 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 3 potential issue(s) to consider:
- [Medium] usb-host: cpr_reissue() completes failed combined IN packets with plain usb_packet_complete()
- [Medium] usb-host: cpr_vm_state() returns on NULL dh before reissue, leaving CPR state stale
- [Medium] usb-host: replay from offset 0 under cpr_replay_all duplicates OUT data and loses IN data
--

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

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

This patch records 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 an error so that the guest driver retries them.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -446,6 +448,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
>          xfer->status = LIBUSB_TRANSFER_COMPLETED;
>          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(), a cancelled URB with
a non-zero actual_length is also suppressed and queued here:

    if (xfer->actual_length != 0 && !r->host->cpr_replay_all) {
        ...
    }

If CPR is then aborted, usb_host_cpr_reissue() restarts that packet from
offset 0:

    /* Replay from the start of the TD */
    p->actual_length = 0;

Can this duplicate data? For an OUT endpoint, the part already sent to
the device would be sent again, since usb_host_handle_data() copies the
whole payload and resubmits it. For an IN endpoint, the bytes already
read from the device would be discarded, so the guest would never see
them.

Also, cpr_inflight is filled in libusb reap order. Is that the same as
guest submission order when several URBs are pipelined on one endpoint?
I could not confirm this from the code.

The commit message does not mention this limitation.

[ ... ]

> @@ -1254,17 +1259,26 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
>  static void usb_host_cpr_vm_state(void *opaque, bool running, RunState state)
>  {
>      USBHostDevice *s = opaque;
>      MigMode mode;
>  
> -    if (running || !s->dh || state != RUN_STATE_FINISH_MIGRATE) {
> +    if (!s->dh) {
> +        return;
> +    }
> +    if (running) {
> +        usb_host_cpr_reissue(s);
> +        return;
> +    }
> +    if (state != RUN_STATE_FINISH_MIGRATE) {
>          return;
>      }

[Severity: Medium]
Can this early return on a NULL dh leave stale CPR state? Before this
patch the !s->dh check only guarded the drain. It now also skips
usb_host_cpr_reissue() on resume.

Consider this sequence:

usb_host_cpr_drain_xfers()
    suppresses requests and queues their packets on cpr_inflight
usb_host_nodev_bh() or unplug
    usb_host_close()
        s->dh = NULL
resume
    usb_host_cpr_vm_state(running)
        if (!s->dh) return;

Here cpr_drained and cpr_replay_all stay set. A later CPR attempt then
skips the drain, because usb_host_pre_save() tests
"s->dh && !s->cpr_drained".

In the base patch, usb_host_close() calls usb_device_detach(). The HCD
then cancels the packets through usb_host_cancel_packet(), which drops
them from cpr_inflight, so the packets are not stranded in that case.

With the later patch "usb-host: hand off the device across
cpr-transfer", usb_host_close() skips usb_host_abort_xfers() and
usb_device_detach() when cpr_handed_off is set. In cpr-transfer mode, a
device loss between the drain and the resume would then leave the guest
packets ASYNC with no request behind them and nothing to complete them.
cpr_handed_off would also stay true.

Should the resume branch run the reissue and the state reset even when
dh is NULL?

[ ... ]

> @@ -1356,6 +1370,45 @@ static libusb_device *usb_host_find_ref(int bus, int addr)
> +static void usb_host_cpr_reissue(USBHostDevice *s)
> +{
[ ... ]
> +        } else {
> +            usb_host_handle_data(udev, p);
> +            if (p->status != USB_RET_ASYNC) {
> +                /* Completed synchronously with error */
> +                usb_packet_complete(udev, p);
> +            }
> +        }

[Severity: Medium]
Does this completion handle combined packets correctly?
usb_host_handle_data() can return a non-ASYNC status synchronously:

- USB_RET_NODEV when libusb_submit_transfer() fails
- USB_RET_STALL on a halted endpoint
- USB_RET_STALL when streams are unsupported

For a bulk IN endpoint where usb_host_use_combining() is true, the
packet on cpr_inflight is the first packet of a USBCombinedPacket. A
plain usb_packet_complete() then completes only that first packet.

The normal completion path in usb_host_req_complete_data() does this
instead:

    if (usb_host_use_combining(r->p->ep)) {
        usb_combined_input_packet_complete(USB_DEVICE(s), r->p);
    } else {
        usb_packet_complete(USB_DEVICE(s), r->p);
    }

Can the other sub-packets be left in USB_PACKET_ASYNC with p->combined
still set, and combined->first pointing at an already completed packet?
That could wedge the pipelined bulk-IN endpoint.

The existing usb_host_req_complete_err() uses the same plain call, but
this reissue error path is new in this patch. The later patch "usb-host:
make CPR migration blocker conditional" does not change this code.

[ ... ]

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