[PATCH QEMU HCI-8.0 v6 00/12] usb-host: support CPR migration

[PATCH QEMU HCI-8.0 v6 00/12] usb-host: support CPR migration

[Andrey Drobyshev]

v5 -> v6:

Substantial changes:

  * Drain each usb-host from a vm-state change handler when the VM stops
    for CPR (RUN_STATE_FINISH_MIGRATE), before the device and interrupt
    controller are snapshotted.  A completion raised during the drain now
    lands in the still-live interrupt controller and is migrated, instead
    of being lost as it was when the drain ran from .pre_save() after the
    snapshot.

  * Deliver a cancelled transfer that moved some data as a short transfer
    instead of dropping it; a zero-progress one is suppressed for the
    target to replay from the start of the TD.  A VM already paused when
    migration starts keeps a suppress-all drain in .pre_save() as a
    fallback.

Minor changes (refactoring/cleanup):

  * Group the per-device CPR fields into one struct block; factor the
    drain-and-handoff tail into usb_host_cpr_quiesce(); fold the
    altsetting range check into the interface reclaim (one config
    descriptor read); register the exit notifier and vm-state handler
    only after the blocker, shrinking the realize error path.

  * Commit message and comment rewording, forward-declaration placement,
    and a checkpatch line-length fix.  No functional change.

v5: http://lore.virtuozzo.com/qemu/20260925-usb-host_v3-v5-0-e5b21724db4d@virtuozzo.com/

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
Andrey Drobyshev (12):
      usb-host: don't leak hostdev FD on open failure #VSTOR-137800
      usb-host: add migration blocker for CPR modes #VSTOR-137800
      usb-host: preserve hostdev FD during CPR migration #VSTOR-137800
      usb-host: factor out usb_host_reap_xfers() #VSTOR-137800
      usb-host: drain in-flight URBs across CPR #VSTOR-137800
      usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800
      usb-host: skip product-string read on CPR incoming #VSTOR-137800
      usb-host: hand off the device across cpr-transfer #VSTOR-137800
      usb: migrate the interface altsetting #VSTOR-137800
      usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800
      hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800
      usb-host: make CPR migration blocker conditional #VSTOR-137800

 hw/usb/bus.c         |  36 ++++
 hw/usb/hcd-xhci.c    |  20 ++
 hw/usb/host-libusb.c | 525 +++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 542 insertions(+), 39 deletions(-)
---
base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8
change-id: 20260914-usb-host_v3-bbfea86d1dad

Best regards,
--  
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

[PATCH QEMU HCI-8.0 v6 01/12] usb-host: don't leak hostdev FD on open failure #VSTOR-137800

[Andrey Drobyshev]

The FD passed into usb_host_open() for a wrapped device is only stored in
s->hostfd after libusb_wrap_sys_device() succeeds, and the failure path
doesn't close it.  if wrapping fails, or a later step of the open sequence
fails (e.g. usb_device_attach), the FD is leaked, as usb_host_close() only
runs for fully opened devices, and the realize error path doesn't clean it
up either.

Store the FD in s->hostfd before wrapping and close it on the failure
path.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index b74670ae256..83645fbe0ce 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -954,6 +954,11 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
     int rc;
     Error *local_err = NULL;
 
+    /* Make sure failure path frees caller-owned FD */
+    if (!dev) {
+        s->hostfd = hostfd;
+    }
+
     if (s->bh_postld_pending) {
         return -1;
     }
@@ -978,7 +983,6 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
         if (rc != 0) {
             goto fail;
         }
-        s->hostfd  = hostfd;
         dev = libusb_get_device(s->dh);
         bus_num = libusb_get_bus_number(dev);
         addr = libusb_get_device_address(dev);
@@ -1066,6 +1070,10 @@ fail:
         s->dh = NULL;
         s->dev = NULL;
     }
+    if (s->hostfd != -1) {
+        close(s->hostfd);
+        s->hostfd = -1;
+    }
     return -1;
 }
 

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 02/12] usb-host: add migration blocker for CPR modes #VSTOR-137800

[Andrey Drobyshev]

We're about to add support for CPR migration to usb-host in the upcoming
commits.  Right now CPR is crashing with usb-host devices.  Let's add
migration blocker upfront, for both cpr-exec and cpr-transfer.  The
blocker is added unconditionally, so that we refuse CPR migration instead
of crashing on it.  That is for the sake of bisectability of the upcoming
commits.  It is going to be lifted (conditioned) in a following patch
once CPR support is added.  Also, blocker is added at the end of
usb_host_realize(), so we have to cleanup all the side effects done
earlier in .realize() in case migrate_add_blocker_modes() fails.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 83645fbe0ce..770bdaca4c8 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -46,6 +46,7 @@
 #endif
 
 #include "qapi/error.h"
+#include "migration/blocker.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
@@ -118,6 +119,9 @@ struct USBHostDevice {
     bool                             bh_postld_pending;
     Notifier                         exit;
 
+    /* CPR migration */
+    Error                            *cpr_blocker;
+
     /* request queues */
     QTAILQ_HEAD(, USBHostRequest)    requests;
     QTAILQ_HEAD(, USBHostIsoRing)    isorings;
@@ -1256,6 +1260,18 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
         usb_host_auto_check(NULL);
     }
 
+    error_setg(&s->cpr_blocker, "usb-host device %s does not support CPR",
+               DEVICE(s)->id ?: "(anonymous)");
+    if (migrate_add_blocker_modes(&s->cpr_blocker, errp,
+                                  MIG_MODE_CPR_TRANSFER,
+                                  MIG_MODE_CPR_EXEC, -1) < 0) {
+        if (s->needs_autoscan) {
+            QTAILQ_REMOVE(&hostdevs, s, next);
+        }
+        usb_host_close(s);
+        return;
+    }
+
     s->exit.notify = usb_host_exit_notifier;
     qemu_add_exit_notifier(&s->exit);
 }
@@ -1279,6 +1295,7 @@ static void usb_host_unrealize(USBDevice *udev)
         QTAILQ_REMOVE(&hostdevs, s, next);
     }
     usb_host_close(s);
+    migrate_del_blocker(&s->cpr_blocker);
 }
 
 static void usb_host_cancel_packet(USBDevice *udev, USBPacket *p)

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 03/12] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800

[Andrey Drobyshev]

In .realize(), save freshly opened host device FD into the CPR registry,
and then reuse it on CPR target.  Also, add deletion of that FD from
registry to usb_host_open() / usb_host_close() cleanup paths.

For this to work, we also need to skip the .post-load() code which
closes, detaches the device, and then rescans the host bus to reopen
matching hostdevs.  For the CPR-case migration we don't want any of that
as the hostdev FD stays preserved, and the guest shouldn't notice the
switchover.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 770bdaca4c8..f601b85b4d9 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -47,6 +47,7 @@
 
 #include "qapi/error.h"
 #include "migration/blocker.h"
+#include "migration/cpr.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
@@ -949,6 +950,21 @@ static void usb_host_ep_update(USBHostDevice *s)
     libusb_free_config_descriptor(conf);
 }
 
+static char *usb_host_cpr_fd_name(USBHostDevice *s)
+{
+    /* Prefix the key so it can't clash with another CPR label */
+    return DEVICE(s)->id ? g_strdup_printf("usb-host/%s", DEVICE(s)->id)
+                         : NULL;
+}
+
+static void usb_host_cpr_delete_fd(USBHostDevice *s)
+{
+    g_autofree char *name = usb_host_cpr_fd_name(s);
+    if (name) {
+        cpr_delete_fd(name, 0);
+    }
+}
+
 static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
 {
     USBDevice *udev = USB_DEVICE(s);
@@ -1075,6 +1091,7 @@ fail:
         s->dev = NULL;
     }
     if (s->hostfd != -1) {
+        usb_host_cpr_delete_fd(s);
         close(s->hostfd);
         s->hostfd = -1;
     }
@@ -1136,6 +1153,7 @@ static int usb_host_close(USBHostDevice *s)
     s->dev = NULL;
 
     if (s->hostfd != -1) {
+        usb_host_cpr_delete_fd(s);
         close(s->hostfd);
         s->hostfd = -1;
     }
@@ -1222,9 +1240,12 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
 
 #if LIBUSB_API_VERSION >= 0x01000107 && !defined(CONFIG_WIN32)
     if (s->hostdevice) {
+        g_autofree char *name = usb_host_cpr_fd_name(s);
         int fd;
+
         s->needs_autoscan = false;
-        fd = qemu_open(s->hostdevice, O_RDWR, errp);
+        fd = name ? cpr_open_fd(s->hostdevice, O_RDWR, name, 0, errp)
+                  : qemu_open(s->hostdevice, O_RDWR, errp);
         if (fd < 0) {
             return;
         }
@@ -1763,6 +1784,16 @@ static int usb_host_post_load(void *opaque, int version_id)
 {
     USBHostDevice *dev = opaque;
 
+    /*
+     * For CPR migration, device wasn't released/reset, and the guest
+     * is unaware of the switchover.  The detach/rescan performed in
+     * usb_host_post_load_bh() only exists for cross-host migration.
+     * Skip it for CPR.
+     */
+    if (cpr_is_incoming()) {
+        return 0;
+    }
+
     if (!dev->bh_postld) {
         dev->bh_postld = qemu_bh_new_guarded(usb_host_post_load_bh, dev,
                                              &DEVICE(dev)->mem_reentrancy_guard);

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 04/12] usb-host: factor out usb_host_reap_xfers() #VSTOR-137800

[Andrey Drobyshev]

usb_host_abort_xfers() cancels every pending request and then pumps
libusb events until all of them are reaped, with a bounded wait.  Split
that reap-wait loop out into usb_host_reap_xfers().  No functional
change.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index f601b85b4d9..5dc3308770e 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1098,15 +1098,11 @@ fail:
     return -1;
 }
 
-static void usb_host_abort_xfers(USBHostDevice *s)
+static void usb_host_reap_xfers(USBHostDevice *s)
 {
     USBHostRequest *r, *rtmp;
     int limit = 100;
 
-    QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
-        usb_host_req_abort(r);
-    }
-
     while (QTAILQ_FIRST(&s->requests) != NULL) {
         struct timeval tv;
         memset(&tv, 0, sizeof(tv));
@@ -1128,6 +1124,16 @@ static void usb_host_abort_xfers(USBHostDevice *s)
     }
 }
 
+static void usb_host_abort_xfers(USBHostDevice *s)
+{
+    USBHostRequest *r, *rtmp;
+
+    QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
+        usb_host_req_abort(r);
+    }
+    usb_host_reap_xfers(s);
+}
+
 static int usb_host_close(USBHostDevice *s)
 {
     USBDevice *udev = USB_DEVICE(s);

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 05/12] usb-host: drain in-flight URBs across CPR #VSTOR-137800

[Andrey Drobyshev]

URBs submitted by the old QEMU must not outlive it across a CPR
switchover: the kernel would complete them into buffers of an address
space that is gone after exec, and the new libusb context would reap URB
pointers it never submitted.

Register a vm_change_state handler that, when the VM stops for CPR
(RUN_STATE_FINISH_MIGRATE), cancels and reaps every pending transfer.
It runs before the vCPU/device snapshot, so a completion raised during
the drain lands in the still-live interrupt controller and is migrated
too.  A transfer that already completed in the kernel is delivered
normally; one cancelled after moving some data is delivered as a short
transfer; one that moved nothing is suppressed, and the target replays
it from the start of the TD via usb_xhci_post_load().  A VM already
paused when migration starts never hits that transition, so .pre_save()
drains as a fallback, suppressing every URB for the target to replay.

Isochronous URBs live on their own rings; cancel and reap them too -- the
target reconstructs the stream from the guest's transfer ring.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 220 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 193 insertions(+), 27 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 5dc3308770e..d68fe0df03e 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -48,6 +48,7 @@
 #include "qapi/error.h"
 #include "migration/blocker.h"
 #include "migration/cpr.h"
+#include "migration/misc.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
@@ -122,6 +123,9 @@ struct USBHostDevice {
 
     /* CPR migration */
     Error                            *cpr_blocker;
+    VMChangeStateEntry               *cpr_vmse;
+    bool                             cpr_drained;
+    bool                             cpr_replay_all;
 
     /* request queues */
     QTAILQ_HEAD(, USBHostRequest)    requests;
@@ -405,7 +409,10 @@ static void usb_host_req_free(USBHostRequest *r)
 #ifdef CONFIG_WIN32
     request_count--;
 #endif
-    QTAILQ_REMOVE(&r->host->requests, r, next);
+    /* Skip if a timed-out reap already unlinked it (avoid a double remove) */
+    if (QTAILQ_IN_USE(r, next)) {
+        QTAILQ_REMOVE(&r->host->requests, r, next);
+    }
     libusb_free_transfer(r->xfer);
     g_free(r->buffer);
     g_free(r);
@@ -423,6 +430,25 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
     return NULL;
 }
 
+/*
+ * Classify a reap during the CPR drain (r->p is still set).  One that moved no
+ * data is suppressed for the target to replay; one that moved some is kept as a
+ * short transfer.  Returns true if suppressed, so the caller frees the request.
+ */
+static bool usb_host_req_cpr_replay(USBHostRequest *r,
+                                    struct libusb_transfer *xfer)
+{
+    if (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
+        return false;
+    }
+    if (xfer->actual_length != 0 && !r->host->cpr_replay_all) {
+        /* deliver as a short transfer */
+        xfer->status = LIBUSB_TRANSFER_COMPLETED;
+        return false;
+    }
+    return true;
+}
+
 static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer *xfer)
 {
     USBHostRequest *r = xfer->user_data;
@@ -432,6 +458,9 @@ static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer *xfer)
     if (r->p == NULL) {
         goto out; /* request was canceled */
     }
+    if (usb_host_req_cpr_replay(r, xfer)) {
+        goto out; /* CPR drain: the target will replay this packet */
+    }
 
     r->p->status = status_map[xfer->status];
     r->p->actual_length = xfer->actual_length;
@@ -482,6 +511,9 @@ static void LIBUSB_CALL usb_host_req_complete_data(struct libusb_transfer *xfer)
     if (r->p == NULL) {
         goto out; /* request was canceled */
     }
+    if (usb_host_req_cpr_replay(r, xfer)) {
+        goto out; /* CPR drain: the target will replay this packet */
+    }
 
     r->p->status = status_map[xfer->status];
     if (r->in && xfer->actual_length) {
@@ -502,22 +534,29 @@ out:
     }
 }
 
-static void usb_host_req_abort(USBHostRequest *r)
+/* Fail r's in-flight packet with an error status and detach it from r. */
+static void usb_host_req_complete_err(USBHostRequest *r, int status)
 {
-    USBHostDevice  *s = r->host;
-    bool inflight = (r->p && r->p->state == USB_PACKET_ASYNC);
+    USBHostDevice *s = r->host;
 
-    if (inflight) {
-        r->p->status = USB_RET_NODEV;
-        trace_usb_host_req_complete(s->bus_num, s->addr, r->p,
-                                    r->p->status, r->p->actual_length);
-        if (r->p->ep->nr == 0) {
-            usb_generic_async_ctrl_complete(USB_DEVICE(s), r->p);
-        } else {
-            usb_packet_complete(USB_DEVICE(s), r->p);
-        }
-        r->p = NULL;
+    if (!r->p || r->p->state != USB_PACKET_ASYNC) {
+        return;
+    }
+    r->p->status = status;
+    trace_usb_host_req_complete(s->bus_num, s->addr, r->p,
+                                r->p->status, r->p->actual_length);
+    if (r->p->ep->nr == 0) {
+        usb_generic_async_ctrl_complete(USB_DEVICE(s), r->p);
+    } else {
+        usb_packet_complete(USB_DEVICE(s), r->p);
+    }
+    r->p = NULL;
+}
 
+static void usb_host_req_abort(USBHostRequest *r)
+{
+    if (r->p && r->p->state == USB_PACKET_ASYNC) {
+        usb_host_req_complete_err(r, USB_RET_NODEV);
         libusb_cancel_transfer(r->xfer);
     }
 }
@@ -1098,9 +1137,18 @@ fail:
     return -1;
 }
 
-static void usb_host_reap_xfers(USBHostDevice *s)
+/* libusb still owns cancelled-but-unreaped transfers: unlink, don't free. */
+static void usb_host_drop_requests(USBHostDevice *s)
 {
     USBHostRequest *r, *rtmp;
+
+    QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
+        QTAILQ_REMOVE(&s->requests, r, next);
+    }
+}
+
+static void usb_host_reap_xfers(USBHostDevice *s)
+{
     int limit = 100;
 
     while (QTAILQ_FIRST(&s->requests) != NULL) {
@@ -1110,15 +1158,10 @@ static void usb_host_reap_xfers(USBHostDevice *s)
         libusb_handle_events_timeout(ctx, &tv);
         if (--limit == 0) {
             /*
-             * Don't wait forever for libusb calling the complete
-             * callback (which will unlink and free the request).
-             *
-             * Leaking memory here, to make sure libusb will not
-             * access memory which we have released already.
+             * Don't wait forever for libusb to call the complete callback
+             * (which unlinks and frees the request).
              */
-            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
-                QTAILQ_REMOVE(&s->requests, r, next);
-            }
+            usb_host_drop_requests(s);
             return;
         }
     }
@@ -1134,6 +1177,105 @@ static void usb_host_abort_xfers(USBHostDevice *s)
     usb_host_reap_xfers(s);
 }
 
+static bool usb_host_iso_inflight(USBHostDevice *s)
+{
+    USBHostIsoRing *ring;
+
+    QTAILQ_FOREACH(ring, &s->isorings, next) {
+        if (!QTAILQ_EMPTY(&ring->inflight)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/*
+ * Quiesce the device for a CPR switchover.  Cancel and reap all URBs,
+ * so that no URB submitted by this process can outlive it.
+ */
+static int usb_host_cpr_drain_xfers(USBHostDevice *s)
+{
+    USBHostRequest *r, *rtmp;
+    USBHostIsoRing *ring;
+    USBHostIsoXfer *xfer;
+    int limit = 100;
+
+    s->cpr_drained = true;
+
+    /*
+     * Cancel and reap every URB.  Completing a reaped URB can make xHCI queue
+     * the next TD, so re-cancel whatever reappears until the queue stays empty.
+     */
+    while (!QTAILQ_EMPTY(&s->requests)) {
+        struct timeval tv = { .tv_usec = 2500 };
+
+        QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
+            libusb_cancel_transfer(r->xfer);
+        }
+        libusb_handle_events_timeout(ctx, &tv);
+        if (--limit == 0) {
+            /*
+             * The device won't release these URBs.  Error the packets out so
+             * the resumed guest retries them, leak the requests (libusb still
+             * owns their transfers) and abort the migration.
+             */
+            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
+                usb_host_req_complete_err(r, USB_RET_IOERROR);
+            }
+            usb_host_drop_requests(s);
+            return -1;
+        }
+    }
+
+    /*
+     * Iso URBs are on the rings, not s->requests.  Cancel them: one in
+     * flight when the guest stops may never complete on its own, and none
+     * may be left on the preserved fd.  The target replays from the ring.
+     */
+    QTAILQ_FOREACH(ring, &s->isorings, next) {
+        QTAILQ_FOREACH(xfer, &ring->inflight, next) {
+            libusb_cancel_transfer(xfer->xfer);
+        }
+    }
+
+    /*
+     * Cap the reap at 2x a full ring:
+     * iso_urb_count URBs x iso_urb_frames packets each
+     */
+    limit = 2 * s->iso_urb_count * s->iso_urb_frames;
+    while (usb_host_iso_inflight(s)) {
+        struct timeval tv = { .tv_usec = 1000 };
+        libusb_handle_events_timeout(ctx, &tv);
+        if (limit-- == 0) {
+            return -1;
+        }
+    }
+    usb_host_iso_free_all(s);
+    return 0;
+}
+
+/*
+ * Quiesce the URB data plane around a CPR switchover: drain when the VM stops
+ * for CPR -- before the device/irqchip snapshot, so completions raised during
+ * the drain are migrated too.
+ */
+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) {
+        return;
+    }
+    mode = migrate_mode();
+    if (mode != MIG_MODE_CPR_EXEC && mode != MIG_MODE_CPR_TRANSFER) {
+        return;
+    }
+    if (usb_host_cpr_drain_xfers(s) < 0) {
+        migration_file_set_error(-EIO, NULL);
+    }
+}
+
 static int usb_host_close(USBHostDevice *s)
 {
     USBDevice *udev = USB_DEVICE(s);
@@ -1301,6 +1443,8 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
 
     s->exit.notify = usb_host_exit_notifier;
     qemu_add_exit_notifier(&s->exit);
+
+    s->cpr_vmse = qemu_add_vm_change_state_handler(usb_host_cpr_vm_state, s);
 }
 
 static void usb_host_instance_init(Object *obj)
@@ -1323,6 +1467,9 @@ static void usb_host_unrealize(USBDevice *udev)
     }
     usb_host_close(s);
     migrate_del_blocker(&s->cpr_blocker);
+    if (s->cpr_vmse) {
+        qemu_del_vm_change_state_handler(s->cpr_vmse);
+    }
 }
 
 static void usb_host_cancel_packet(USBDevice *udev, USBPacket *p)
@@ -1791,10 +1938,8 @@ static int usb_host_post_load(void *opaque, int version_id)
     USBHostDevice *dev = opaque;
 
     /*
-     * For CPR migration, device wasn't released/reset, and the guest
-     * is unaware of the switchover.  The detach/rescan performed in
-     * usb_host_post_load_bh() only exists for cross-host migration.
-     * Skip it for CPR.
+     * CPR keeps the device open and the guest unaware, so skip the detach and
+     * rescan in usb_host_post_load_bh() -- those are for cross-host migration.
      */
     if (cpr_is_incoming()) {
         return 0;
@@ -1809,10 +1954,31 @@ static int usb_host_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static int usb_host_pre_save(void *opaque)
+{
+    USBHostDevice *s = opaque;
+    MigMode mode = migrate_mode();
+
+    /*
+     * The vm-state handler drains on the stop-for-CPR transition, but a VM
+     * already paused never hits it.  Drain here instead; the stopped guest
+     * can't take completions, so suppress every URB and let the target replay.
+     */
+    if ((mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER) &&
+        s->dh && !s->cpr_drained) {
+        s->cpr_replay_all = true;
+        if (usb_host_cpr_drain_xfers(s) < 0) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static const VMStateDescription vmstate_usb_host = {
     .name = "usb-host",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = usb_host_pre_save,
     .post_load = usb_host_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_USB_DEVICE(parent_obj, USBHostDevice),

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 06/12] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

[Andrey Drobyshev]

If CPR fails, the source VM resumes with the drained packets still owned
by the host controller as in-flight, and nothing left to complete them.

Remember the packets suppressed during the drain on a list, and re-issue
them on the preserved fd from the vm-state handler's resume branch -- it
runs on any resume but only acts once we have actually drained
(cpr_drained).  Control transfers cannot be reconstructed from the packet
alone, so complete those as errors and let the guest driver retry.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index d68fe0df03e..85509dc2d83 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -124,6 +124,7 @@ struct USBHostDevice {
     /* CPR migration */
     Error                            *cpr_blocker;
     VMChangeStateEntry               *cpr_vmse;
+    GSList                           *cpr_inflight;
     bool                             cpr_drained;
     bool                             cpr_replay_all;
 
@@ -432,8 +433,9 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
 
 /*
  * Classify a reap during the CPR drain (r->p is still set).  One that moved no
- * data is suppressed for the target to replay; one that moved some is kept as a
- * short transfer.  Returns true if suppressed, so the caller frees the request.
+ * data is suppressed and queued on cpr_inflight for the target to replay; one
+ * that moved some is kept as a short transfer.  Returns true if suppressed, so
+ * the caller frees the request.
  */
 static bool usb_host_req_cpr_replay(USBHostRequest *r,
                                     struct libusb_transfer *xfer)
@@ -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;
 }
 
@@ -1254,17 +1259,26 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
     return 0;
 }
 
+static void usb_host_cpr_reissue(USBHostDevice *s);
+
 /*
  * Quiesce the URB data plane around a CPR switchover: drain when the VM stops
  * for CPR -- before the device/irqchip snapshot, so completions raised during
- * the drain are migrated too.
+ * the drain are migrated too -- and re-issue on resume if CPR was aborted.
  */
 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;
     }
     mode = migrate_mode();
@@ -1356,6 +1370,45 @@ static libusb_device *usb_host_find_ref(int bus, int addr)
     return ret;
 }
 
+static void usb_host_handle_data(USBDevice *udev, USBPacket *p);
+
+/*
+ * If CPR is aborted the source resumes with the drained packets still owned
+ * by the controller as in-flight; re-issue them on the preserved fd.  Control
+ * transfers can't be reconstructed from the packet, so error those out.
+ */
+static void usb_host_cpr_reissue(USBHostDevice *s)
+{
+    USBDevice *udev = USB_DEVICE(s);
+    GSList *it;
+    USBPacket *p;
+
+    if (!s->cpr_drained) {
+        return;
+    }
+
+    for (it = s->cpr_inflight; it; it = it->next) {
+        p = it->data;
+        /* Replay from the start of the TD */
+        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);
+            if (p->status != USB_RET_ASYNC) {
+                /* Completed synchronously with error */
+                usb_packet_complete(udev, p);
+            }
+        }
+    }
+    g_slist_free(s->cpr_inflight);
+    s->cpr_inflight = NULL;
+    s->cpr_replay_all = false;
+    s->cpr_drained = false;
+}
+
 static void usb_host_realize(USBDevice *udev, Error **errp)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
@@ -1470,6 +1523,8 @@ static void usb_host_unrealize(USBDevice *udev)
     if (s->cpr_vmse) {
         qemu_del_vm_change_state_handler(s->cpr_vmse);
     }
+    g_slist_free(s->cpr_inflight);
+    s->cpr_inflight = NULL;
 }
 
 static void usb_host_cancel_packet(USBDevice *udev, USBPacket *p)
@@ -1477,6 +1532,9 @@ static void usb_host_cancel_packet(USBDevice *udev, USBPacket *p)
     USBHostDevice *s = USB_HOST_DEVICE(udev);
     USBHostRequest *r;
 
+    /* Packet is being cancelled -> it mustn't be re-issued on failed CPR */
+    s->cpr_inflight = g_slist_remove(s->cpr_inflight, p);
+
     if (p->combined) {
         usb_combined_packet_cancel(udev, p);
         return;

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 07/12] usb-host: skip product-string read on CPR incoming #VSTOR-137800

[Andrey Drobyshev]

usb_host_open() reads the product string descriptor from the device
with a synchronous control transfer.  On the CPR target this runs while
the device is being handed over, and the transfer can block there,
hanging the incoming migration.

The string only fills the cosmetic product_desc, so skip the read when
we're the CPR target and fall back to the synthetic "host:bus.addr"
name.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 85509dc2d83..020a685aa41 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1106,7 +1106,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
     udev->speed = speed_map[libusb_speed];
     usb_host_speed_compat(s);
 
-    if (s->ddesc.iProduct) {
+    if (s->ddesc.iProduct && !cpr_is_incoming()) {
         libusb_get_string_descriptor_ascii(s->dh, s->ddesc.iProduct,
                                            (unsigned char *)udev->product_desc,
                                            sizeof(udev->product_desc));

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 08/12] usb-host: hand off the device across cpr-transfer #VSTOR-137800

[Andrey Drobyshev]

Unlike cpr-exec, cpr-transfer keeps the source QEMU alive: it hands the
usbfs FD to the new QEMU over the migration channel and only exits once
the target is up.  While the handoff is in flight both processes hold the
same FD, and only one may reap URB events on it - if both do, one reaps
the other's in-flight completions and wedges an isochronous transfer that
cannot be retried.  So reaping is handed off along with the FD:

  * The target doesn't reap until it takes over: it stops events right
    after opening the FD in .realize() and restarts in .post_load().
  * The source stops reaping in .pre_save() and, since it stays alive,
    restarts reaping from the vm-state handler's resume branch on failure.
  * On success the source's .exit() leaves the device untouched - no
    reset, interface release or host-driver rebind - and just closes its
    own copy of the FD.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 109 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 96 insertions(+), 13 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 020a685aa41..492f0442974 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -127,6 +127,7 @@ struct USBHostDevice {
     GSList                           *cpr_inflight;
     bool                             cpr_drained;
     bool                             cpr_replay_all;
+    bool                             cpr_handed_off;
 
     /* request queues */
     QTAILQ_HEAD(, USBHostRequest)    requests;
@@ -1260,6 +1261,23 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
 }
 
 static void usb_host_cpr_reissue(USBHostDevice *s);
+static void usb_host_cpr_stop_events(void);
+
+/*
+ * Drain the device for a CPR switchover and, for cpr-transfer, hand reaping of
+ * the shared fd off to the target.  Returns -1 if the drain couldn't complete.
+ */
+static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
+{
+    if (usb_host_cpr_drain_xfers(s) < 0) {
+        return -1;
+    }
+    if (mode == MIG_MODE_CPR_TRANSFER) {
+        usb_host_cpr_stop_events();
+        s->cpr_handed_off = true;
+    }
+    return 0;
+}
 
 /*
  * Quiesce the URB data plane around a CPR switchover: drain when the VM stops
@@ -1285,7 +1303,7 @@ static void usb_host_cpr_vm_state(void *opaque, bool running, RunState state)
     if (mode != MIG_MODE_CPR_EXEC && mode != MIG_MODE_CPR_TRANSFER) {
         return;
     }
-    if (usb_host_cpr_drain_xfers(s) < 0) {
+    if (usb_host_cpr_quiesce(s, mode) < 0) {
         migration_file_set_error(-EIO, NULL);
     }
 }
@@ -1300,16 +1318,19 @@ static int usb_host_close(USBHostDevice *s)
 
     trace_usb_host_close(s->bus_num, s->addr);
 
-    usb_host_abort_xfers(s);
-    usb_host_iso_free_all(s);
+    if (!s->cpr_handed_off) {
+        usb_host_abort_xfers(s);
+        usb_host_iso_free_all(s);
 
-    if (udev->attached) {
-        usb_device_detach(udev);
+        if (udev->attached) {
+            usb_device_detach(udev);
+        }
+
+        usb_host_release_interfaces(s);
+        libusb_reset_device(s->dh);
+        usb_host_attach_kernel(s);
     }
 
-    usb_host_release_interfaces(s);
-    libusb_reset_device(s->dh);
-    usb_host_attach_kernel(s);
     libusb_close(s->dh);
     s->dh = NULL;
     s->dev = NULL;
@@ -1344,10 +1365,17 @@ static void usb_host_exit_notifier(struct Notifier *n, void *data)
     USBHostDevice *s = container_of(n, USBHostDevice, exit);
 
     if (s->dh) {
-        usb_host_abort_xfers(s);
-        usb_host_release_interfaces(s);
-        libusb_reset_device(s->dh);
-        usb_host_attach_kernel(s);
+        /*
+         * Handed to new QEMU across cpr-transfer: do not reset, release
+         * interfaces or rebind the host driver underneath it; just close
+         * our handle.
+         */
+        if (!s->cpr_handed_off) {
+            usb_host_abort_xfers(s);
+            usb_host_release_interfaces(s);
+            libusb_reset_device(s->dh);
+            usb_host_attach_kernel(s);
+        }
         libusb_close(s->dh);
     }
 }
@@ -1371,6 +1399,7 @@ static libusb_device *usb_host_find_ref(int bus, int addr)
 }
 
 static void usb_host_handle_data(USBDevice *udev, USBPacket *p);
+static void usb_host_cpr_restart_events(void);
 
 /*
  * If CPR is aborted the source resumes with the drained packets still owned
@@ -1387,6 +1416,11 @@ static void usb_host_cpr_reissue(USBHostDevice *s)
         return;
     }
 
+    if (s->cpr_handed_off) {
+        usb_host_cpr_restart_events();
+        s->cpr_handed_off = false;
+    }
+
     for (it = s->cpr_inflight; it; it = it->next) {
         p = it->data;
         /* Replay from the start of the TD */
@@ -1455,6 +1489,14 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
             error_setg(errp, "failed to open host usb device %s", s->hostdevice);
             return;
         }
+        if (cpr_is_incoming()) {
+            /*
+             * The source still owns the shared fd until it hands off.  Don't
+             * reap URBs on it yet, or we would steal the source's in-flight
+             * completions; .post_load() restarts events once we take over.
+             */
+            usb_host_cpr_stop_events();
+        }
     } else
 #endif
     if (s->match.addr && s->match.bus_num &&
@@ -2000,6 +2042,7 @@ static int usb_host_post_load(void *opaque, int version_id)
      * rescan in usb_host_post_load_bh() -- those are for cross-host migration.
      */
     if (cpr_is_incoming()) {
+        usb_host_cpr_restart_events();
         return 0;
     }
 
@@ -2012,6 +2055,46 @@ static int usb_host_post_load(void *opaque, int version_id)
     return 0;
 }
 
+#ifndef CONFIG_WIN32
+
+/*
+ * cpr-transfer shares the usbfs fd with the new QEMU, so stop reaping URBs on
+ * it here -- only one process may drive the device.  Restart if migration
+ * fails and the source resumes.
+ */
+static void usb_host_cpr_stop_events(void)
+{
+    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
+
+    libusb_set_pollfd_notifiers(ctx, NULL, NULL, NULL);
+    if (poll) {
+        for (int i = 0; poll[i] != NULL; i++) {
+            usb_host_del_fd(poll[i]->fd, ctx);
+        }
+        free(poll);
+    }
+}
+
+static void usb_host_cpr_restart_events(void)
+{
+    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
+
+    libusb_set_pollfd_notifiers(ctx, usb_host_add_fd, usb_host_del_fd, ctx);
+    if (poll) {
+        for (int i = 0; poll[i] != NULL; i++) {
+            usb_host_add_fd(poll[i]->fd, poll[i]->events, ctx);
+        }
+        free(poll);
+    }
+}
+
+#else
+
+static void usb_host_cpr_stop_events(void) {}
+static void usb_host_cpr_restart_events(void) {}
+
+#endif
+
 static int usb_host_pre_save(void *opaque)
 {
     USBHostDevice *s = opaque;
@@ -2025,7 +2108,7 @@ static int usb_host_pre_save(void *opaque)
     if ((mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER) &&
         s->dh && !s->cpr_drained) {
         s->cpr_replay_all = true;
-        if (usb_host_cpr_drain_xfers(s) < 0) {
+        if (usb_host_cpr_quiesce(s, mode) < 0) {
             return -1;
         }
     }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 09/12] usb: migrate the interface altsetting #VSTOR-137800

[Andrey Drobyshev]

The active altsetting of each interface is host controller state that
the guest selected with SET_INTERFACE, but it isn't part of the
migration stream.  For most devices the target happens to reopen on the
same altsettings, so this went unnoticed.

It matters for CPR of a device whose endpoints only exist on a non-zero
altsetting, such as an isochronous streaming interface.  Migrate the
altsetting array in a subsection, gated to the CPR modes: only the CPR
target consumes it to rebuild its endpoints, while a regular migration
reopens the device and reconstructs the altsetting itself.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/bus.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 8dd2ce415eb..ee13acea658 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -8,6 +8,7 @@
 #include "qemu/module.h"
 #include "system/system.h"
 #include "migration/vmstate.h"
+#include "migration/cpr.h"
 #include "monitor/monitor.h"
 #include "trace.h"
 #include "qemu/cutils.h"
@@ -63,6 +64,37 @@ static int usb_device_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool usb_altsetting_needed(void *opaque)
+{
+    USBDevice *dev = opaque;
+    int i;
+
+    /*
+     * Only the CPR target consumes this, to rebuild its endpoints; other
+     * migrations reopen the device and reconstruct the altsetting.
+     */
+    if (!cpr_incoming_needed(NULL)) {
+        return false;
+    }
+    for (i = 0; i < USB_MAX_INTERFACES; i++) {
+        if (dev->altsetting[i]) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static const VMStateDescription vmstate_usb_device_altsetting = {
+    .name = "USBDevice/altsetting",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = usb_altsetting_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_INT32_ARRAY(altsetting, USBDevice, USB_MAX_INTERFACES),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
 const VMStateDescription vmstate_usb_device = {
     .name = "USBDevice",
     .version_id = 1,
@@ -77,6 +109,10 @@ const VMStateDescription vmstate_usb_device = {
         VMSTATE_INT32(setup_index, USBDevice),
         VMSTATE_UINT8_ARRAY(setup_buf, USBDevice, 8),
         VMSTATE_END_OF_LIST(),
+    },
+    .subsections = (const VMStateDescription * const []) {
+        &vmstate_usb_device_altsetting,
+        NULL
     }
 };
 

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 10/12] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

[Andrey Drobyshev]

On CPR incoming we skip the usb_host_open() rescan, so libusb only sets
up the endpoints from the device's default configuration.  If the guest
had enabled an endpoint by selecting a non-zero altsetting (e.g. an
isochronous streaming endpoint), that endpoint is now missing, and the
guest's transfers to it simply stall.

The altsetting itself is restored by vmload now, so let's call
usb_host_ep_update() on the CPR target to rebuild the endpoints out of
it.

The kernel claims are still there on the preserved FD, but libusb's own
handle knows nothing about them, so a later SET_INTERFACE would fail with
LIBUSB_ERROR_NOT_FOUND.  Let's re-claim the interfaces on the target as
well to bring libusb's view back in sync - on the preserved FD this is a
no-op at the usbfs level anyway.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 492f0442974..b3c46aa8de0 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -946,7 +946,7 @@ static void usb_host_ep_update(USBHostDevice *s)
         alt = udev->altsetting[intf->bInterfaceNumber];
 
         if (alt != 0) {
-            assert(alt < conf->interface[i].num_altsetting);
+            assert(alt >= 0 && alt < conf->interface[i].num_altsetting);
             intf = &conf->interface[i].altsetting[alt];
         }
 
@@ -2033,6 +2033,53 @@ static void usb_host_post_load_bh(void *opaque)
     usb_host_auto_check(NULL);
 }
 
+/*
+ * Reconfigure the CPR target after vmload: reject a migrated altsetting the
+ * device can't honour, then re-claim the interfaces so libusb's view matches
+ * the preserved fd (which already holds the claims).
+ */
+static int usb_host_cpr_reclaim_interfaces(USBHostDevice *s)
+{
+    USBDevice *udev = USB_DEVICE(s);
+    struct libusb_config_descriptor *conf;
+    int n, rc;
+
+    rc = libusb_get_active_config_descriptor(s->dev, &conf);
+    if (rc != 0) {
+        usb_host_libusb_error("libusb_get_active_config_descriptor", rc);
+        return -1;
+    }
+
+    for (int i = 0; i < conf->bNumInterfaces; i++) {
+        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
+        int alt = udev->altsetting[ifnum];
+
+        if (alt < 0 || alt >= conf->interface[i].num_altsetting) {
+            error_report("usb-host: migrated altsetting %d out of range for "
+                         "interface %d", alt, ifnum);
+            libusb_free_config_descriptor(conf);
+            return -1;
+        }
+    }
+
+    n = MIN(conf->bNumInterfaces, USB_MAX_INTERFACES);
+    for (int i = 0; i < n; i++) {
+        rc = libusb_claim_interface(s->dh, i);
+        if (rc != 0) {
+            usb_host_libusb_error("libusb_claim_interface", rc);
+            libusb_free_config_descriptor(conf);
+            return -1;
+        }
+        s->ifs[i].claimed = true;
+    }
+
+    udev->ninterfaces = conf->bNumInterfaces;
+    udev->configuration = conf->bConfigurationValue;
+    libusb_free_config_descriptor(conf);
+
+    return 0;
+}
+
 static int usb_host_post_load(void *opaque, int version_id)
 {
     USBHostDevice *dev = opaque;
@@ -2042,6 +2089,14 @@ static int usb_host_post_load(void *opaque, int version_id)
      * rescan in usb_host_post_load_bh() -- those are for cross-host migration.
      */
     if (cpr_is_incoming()) {
+        /*
+         * The device stayed open, so rebuild libusb's claims and endpoints,
+         * and start reaping now that the source has handed the fd off.
+         */
+        if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
+            return -1;
+        }
+        usb_host_ep_update(dev);
         usb_host_cpr_restart_events();
         return 0;
     }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 11/12] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800

[Andrey Drobyshev]

An isochronous endpoint has no retransmission, so the transfer events
the controller reports have to line up with the TDs the guest driver is
sitting on.  Across CPR we rebuild the controller's transient transfer
and scheduling state out of guest memory, and it no longer matches what
the guest has already consumed - so the very first event after resume
lands on the wrong TD and the guest wedges with "not part of TD".

Let's report CC_MISSED_SERVICE_ERROR on the first isochronous transfer
after a CPR resume, just like a real xHCI does when it misses a service
interval.  That arms the guest driver's skip logic, and it resyncs to
the live stream instead of getting stuck.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/hcd-xhci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 292c378bfc9..0370c784627 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -25,6 +25,7 @@
 #include "qemu/module.h"
 #include "qemu/queue.h"
 #include "migration/vmstate.h"
+#include "migration/cpr.h"
 #include "hw/qdev-properties.h"
 #include "trace.h"
 #include "qapi/error.h"
@@ -282,6 +283,7 @@ struct XHCIEPContext {
     unsigned int max_psize;
     uint32_t state;
     uint32_t kick_active;
+    bool cpr_iso_resync; /* resync iso stream on first kick after CPR resume */
 
     /* streams */
     unsigned int max_pstreams;
@@ -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;
+        }
         mfindex = xhci_mfindex_get(xhci);
         xhci_calc_iso_kick(xhci, xfer, epctx, mfindex);
         xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
@@ -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;
+                }
                 /* kick endpoint after vmload is finished */
                 timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
             }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v6 12/12] usb-host: make CPR migration blocker conditional #VSTOR-137800

[Andrey Drobyshev]

A previous commit added an unconditional blocker for CPR migrations.
That is to refuse CPR gracefully instead of crashing and for the previous
commits to remain bisectable.

Now the restore path is complete: the FD is preserved, in-flight URBs
are drained and replayed, the device is handed off across cpr-transfer,
and the endpoints and isochronous streams are reconstructed on the
target.  So let's lift the CPR blocker for the devices which satisfy the
preconditions.

Still, the remaining preconditions for CPR to work are:

  * We must open host device FD ourselves, so that libusb owns the FD.
    This way we're able to preserve the FD during CPR;
  * Device ID must be provided as a unique stable key for CPR FD registry.

Use them to guard the blocker addition.  Apart from that, prohibit CPR on
WIN32 platform and on hosts with older libusb versions.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index b3c46aa8de0..258e1d5c471 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1524,16 +1524,39 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
         usb_host_auto_check(NULL);
     }
 
-    error_setg(&s->cpr_blocker, "usb-host device %s does not support CPR",
+#ifdef CONFIG_WIN32
+    error_setg(&s->cpr_blocker,
+               "usb-host device %s does not support CPR: "
+               "WIN32 platform is not supported",
                DEVICE(s)->id ?: "(anonymous)");
-    if (migrate_add_blocker_modes(&s->cpr_blocker, errp,
-                                  MIG_MODE_CPR_TRANSFER,
-                                  MIG_MODE_CPR_EXEC, -1) < 0) {
-        if (s->needs_autoscan) {
-            QTAILQ_REMOVE(&hostdevs, s, next);
+#elif LIBUSB_API_VERSION < 0x01000107
+    error_setg(&s->cpr_blocker,
+               "usb-host device %s does not support CPR: "
+               "libusb API version 0x%08x is too old",
+               DEVICE(s)->id ?: "(anonymous)", (unsigned) LIBUSB_API_VERSION);
+#else
+    if (!(s->hostdevice && DEVICE(s)->id)) {
+        /*
+         * CPR needs the host fd owned by libusb (the hostdevice= property)
+         * and a device id to key the CPR fd registry.
+         */
+        error_setg(&s->cpr_blocker,
+                   "usb-host device %s does not support CPR: "
+                   "hostdevice= and a device id are required",
+                   DEVICE(s)->id ?: "(anonymous)");
+    }
+#endif
+
+    if (s->cpr_blocker) {
+        if (migrate_add_blocker_modes(&s->cpr_blocker, errp,
+                                      MIG_MODE_CPR_TRANSFER,
+                                      MIG_MODE_CPR_EXEC, -1) < 0) {
+            if (s->needs_autoscan) {
+                QTAILQ_REMOVE(&hostdevs, s, next);
+            }
+            usb_host_close(s);
+            return;
         }
-        usb_host_close(s);
-        return;
     }
 
     s->exit.notify = usb_host_exit_notifier;

-- 
2.47.1

Re: [PATCH QEMU HCI-8.0 v6 02/12] usb-host: add migration blocker for CPR modes #VSTOR-137800

[no-reply]

> We're about to add support for CPR migration to usb-host in the upcoming
> commits.  Right now CPR is crashing with usb-host devices.  Let's add
> migration blocker upfront, for both cpr-exec and cpr-transfer.  The
> blocker is added unconditionally, so that we refuse CPR migration instead
> of crashing on it.  That is for the sake of bisectability of the upcoming
> commits.  It is going to be lifted (conditioned) in a following patch
> once CPR support is added.  Also, blocker is added at the end of
> usb_host_realize(), so we have to cleanup all the side effects done
> earlier in .realize() in case migrate_add_blocker_modes() fails.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH QEMU HCI-8.0 v6 01/12] usb-host: don't leak hostdev FD on open failure #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: usb_host_open() stores hostfd before early checks that skip the new close
--

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

usb-host: don't leak hostdev FD on open failure #VSTOR-137800

This patch stores the caller-supplied hostfd in s->hostfd at the top of
usb_host_open() when no libusb_device is given. It also closes s->hostfd
on the fail: path, so the FD is released when libusb_wrap_sys_device() or
a later open step fails.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index b74670ae25696..83645fbe0ced0 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -954,6 +954,11 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      int rc;
>      Error *local_err = NULL;
>  
> +    /* Make sure failure path frees caller-owned FD */
> +    if (!dev) {
> +        s->hostfd = hostfd;
> +    }
> +
>      if (s->bh_postld_pending) {
>          return -1;
>      }
>      if (s->dh != NULL) {
>          goto fail;
>      }

[Severity: Medium]
Can this leak the FD when usb_host_open() returns early?

The store of s->hostfd happens before the bh_postld_pending check:

    if (!dev) {
        s->hostfd = hostfd;
    }

    if (s->bh_postld_pending) {
        return -1;
    }

That early return bypasses the fail: label and its new close(s->hostfd)
block. The caller-owned FD is then neither closed nor released, and
s->hostfd keeps holding it.

usb_host_realize() does not close the FD either. It only calls
error_setg() and returns:

    rc = usb_host_open(s, NULL, fd);
    if (rc < 0) {
        error_setg(errp, "failed to open host usb device %s", s->hostdevice);
        return;
    }

The s->dh != NULL case has a related problem. It does jump to fail:, but
s->hostfd has already been overwritten. The FD that s->hostfd previously
owned is lost, and the new caller's FD is closed instead.

Today the only caller that passes a real FD is usb_host_realize(). There
s->hostfd starts at -1 and s->dh is NULL, so both early exits look
unreachable and the problem is latent. I also checked the end of the
series. The store is still placed before the bh_postld_pending check, so
a later patch does not fix this.

Would it be better to assign s->hostfd only after those two early checks?
Alternatively, the early checks could be routed through the fail: path in
a way that does not clobber an FD that is already owned.

> @@ -978,7 +983,6 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>          if (rc != 0) {
>              goto fail;
>          }
> -        s->hostfd  = hostfd;
>          dev = libusb_get_device(s->dh);

[ ... ]

> @@ -1066,6 +1070,10 @@ fail:
>          s->dh = NULL;
>          s->dev = NULL;
>      }
> +    if (s->hostfd != -1) {
> +        close(s->hostfd);
> +        s->hostfd = -1;
> +    }
>      return -1;
>  }

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

Re: [PATCH QEMU HCI-8.0 v6 04/12] usb-host: factor out usb_host_reap_xfers() #VSTOR-137800

[no-reply]

> usb_host_abort_xfers() cancels every pending request and then pumps
> libusb events until all of them are reaped, with a bounded wait.  Split
> that reap-wait loop out into usb_host_reap_xfers().  No functional
> change.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH QEMU HCI-8.0 v6 03/12] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800

[no-reply]

> In .realize(), save freshly opened host device FD into the CPR registry,
> and then reuse it on CPR target.  Also, add deletion of that FD from
> registry to usb_host_open() / usb_host_close() cleanup paths.
> 
> For this to work, we also need to skip the .post-load() code which
> closes, detaches the device, and then rescans the host bus to reopen
> matching hostdevs.  For the CPR-case migration we don't want any of that
> as the hostdev FD stays preserved, and the guest shouldn't notice the
> switchover.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH QEMU HCI-8.0 v6 06/12] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800

[no-reply]

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

Re: [PATCH QEMU HCI-8.0 v6 07/12] usb-host: skip product-string read on CPR incoming #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] usb-host: cpr_is_incoming() guard in usb_host_open() skips product string for all opens
- [Low] usb-host: CPR product-string skip is an uncommented workaround for an unexplained hang
--

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

usb-host: skip product-string read on CPR incoming #VSTOR-137800

This patch skips the synchronous libusb product string read in
usb_host_open() when the process is the CPR target. In that case it uses
the synthetic "host:bus.addr" name for product_desc instead.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 85509dc2d83e..020a685aa419 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -1106,7 +1106,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      udev->speed = speed_map[libusb_speed];
>      usb_host_speed_compat(s);
>  
> -    if (s->ddesc.iProduct) {
> +    if (s->ddesc.iProduct && !cpr_is_incoming()) {

[Severity: Low]
Is this guard broader than the CPR hostfd hand-over it is meant to
cover?

cpr_is_incoming() returns incoming_mode != MIG_MODE_NONE, which is a
process-wide flag that stays set for the whole incoming migration. It
is not a per-device indicator.

Any usb_host_open() call in that window therefore takes the synthetic
name branch. That includes the autoscan path:

usb_host_auto_check()
    usb_host_open(s, devs[i], 0)

It also includes the explicit bus/addr realize path. Neither of these
is the hostfd hand-over described in the commit message.

Since product_desc is only written in usb_host_open(), would those
devices keep the "host:bus.addr" name after migration completes?

Could the check be narrowed to the device that was actually handed over
by CPR, for example by keying off hostfd?

[Severity: Low]
This isn't a functional problem, but could a comment be added here
explaining why the product string read is skipped on the CPR target?

The commit message says only that the transfer "can block". The cause
is not recorded in the code, and other cpr_is_incoming() checks in this
file carry comments.

It is also unclear whether the other libusb calls that still run in
usb_host_open() before usb_device_attach() can block in the same way:

libusb_wrap_sys_device()
usb_host_detach_kernel()
usb_host_ep_update()
usb_host_speed_compat()

>          libusb_get_string_descriptor_ascii(s->dh, s->ddesc.iProduct,
>                                             (unsigned char *)udev->product_desc,
>                                             sizeof(udev->product_desc));

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

Re: [PATCH QEMU HCI-8.0 v6 05/12] usb-host: drain in-flight URBs across CPR #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] usb-host: xHCI replay of suppressed URBs relies on a dequeue pointer that is already advanced
- [Medium] usb-host: pre_save fallback drain can deliver completions after xHCI state is saved
- [Medium] usb-host: iso drain cancels in-flight URBs once but completions can resubmit new ones
- [Medium] usb-host: a cancelled OUT or control URB that moved data is reported as a short success
- [Medium] usb-host: drain timeout path can leave newly submitted URBs live and untracked
--

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

usb-host: drain in-flight URBs across CPR #VSTOR-137800

This patch registers a vm_change_state handler that cancels and reaps all
pending libusb transfers when the VM stops for CPR. It also adds a
.pre_save() fallback for VMs that are already paused, and cancels and
reaps isochronous URBs as well. Cancelled URBs that moved no data are
suppressed so the target can replay 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

[ ... ]

> +/*
> + * Classify a reap during the CPR drain (r->p is still set).  One that moved no
> + * data is suppressed for the target to replay; one that moved some is kept as a
> + * short transfer.  Returns true if suppressed, so the caller frees the request.
> + */
> +static bool usb_host_req_cpr_replay(USBHostRequest *r,
> +                                    struct libusb_transfer *xfer)
> +{
> +    if (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
> +        return false;
> +    }

[Severity: Medium]
The commit message says the .pre_save() fallback suppresses "every URB for
the target to replay". Is that accurate?

In usb_host_req_cpr_replay(), any status other than
LIBUSB_TRANSFER_CANCELLED returns false, so a URB that completed in the
kernel during the fallback drain is still delivered:

    usb_host_req_complete_data()
      -> usb_packet_complete()

Can this run after the xHCI and interrupt controller state has already
been saved? usb_host_pre_save() runs as part of device-state save, not
before the snapshot as the vm-state handler does. If so, event ring,
interrupter and endpoint state changed by that completion might be missing
from the migrated image.

> +    if (xfer->actual_length != 0 && !r->host->cpr_replay_all) {
> +        /* deliver as a short transfer */
> +        xfer->status = LIBUSB_TRANSFER_COMPLETED;
> +        return false;
> +    }
> +    return true;
> +}

[Severity: Medium]
Can this corrupt an OUT stream? The status is rewritten to
LIBUSB_TRANSFER_COMPLETED for every direction and transfer type, with no
check of r->in or the control stage.

For a bulk or interrupt OUT, or a control write, that was cancelled after
moving some bytes, the device has consumed a fragment. The guest is then
told the transfer ended short, and the target can neither replay it nor
see an error. A CBW or a data stage could be affected this way.

Should this conversion be limited to IN transfers?

[Severity: Medium]
When usb_host_req_cpr_replay() returns true, the completion is suppressed
and the request is freed. The replay then relies on the xHCI endpoint
context in guest RAM still pointing at the start of this TD.

Is that the case when several TDs are outstanding on one endpoint?
xhci_kick_epctx() advances ring->dequeue via xhci_ring_fetch() for each TD
it fires. xhci_complete() calls xhci_kick_epctx(), which calls
xhci_set_ep_state(EP_RUNNING) at its start:

    xhci_complete()
      -> xhci_kick_epctx()
           -> xhci_set_ep_state()
                ctx[2] = ring->dequeue | ring->ccs;

That writes the advanced dequeue to guest memory while later TDs are
still queued or in flight. usb_xhci_post_load() rebuilds the ring from
that value. Would a suppressed TD2 then be skipped on the target, so that
its completion event is never generated?

[ ... ]

> +/*
> + * Quiesce the device for a CPR switchover.  Cancel and reap all URBs,
> + * so that no URB submitted by this process can outlive it.
> + */
> +static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> +{

[ ... ]

> +        if (--limit == 0) {
> +            /*
> +             * The device won't release these URBs.  Error the packets out so
> +             * the resumed guest retries them, leak the requests (libusb still
> +             * owns their transfers) and abort the migration.
> +             */
> +            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
> +                usb_host_req_complete_err(r, USB_RET_IOERROR);
> +            }
> +            usb_host_drop_requests(s);
> +            return -1;
> +        }

[Severity: Medium]
On this timeout path, can usb_host_drop_requests() orphan live URBs?

usb_host_req_complete_err() calls usb_packet_complete(), which can process
queued packets on the endpoint:

    usb_host_req_complete_err()
      -> usb_packet_complete()
           -> usb_process_one()
                -> usb_host_handle_data() / control handler
                     -> libusb_submit_transfer()

Those paths append new entries to s->requests. usb_host_drop_requests()
then unlinks every entry without cancelling it, so the new URBs stay live
on the preserved fd and are never reaped.

> +    /*
> +     * Iso URBs are on the rings, not s->requests.  Cancel them: one in
> +     * flight when the guest stops may never complete on its own, and none
> +     * may be left on the preserved fd.  The target replays from the ring.
> +     */
> +    QTAILQ_FOREACH(ring, &s->isorings, next) {
> +        QTAILQ_FOREACH(xfer, &ring->inflight, next) {
> +            libusb_cancel_transfer(xfer->xfer);
> +        }
> +    }
> +
> +    /*
> +     * Cap the reap at 2x a full ring:
> +     * iso_urb_count URBs x iso_urb_frames packets each
> +     */
> +    limit = 2 * s->iso_urb_count * s->iso_urb_frames;
> +    while (usb_host_iso_inflight(s)) {
> +        struct timeval tv = { .tv_usec = 1000 };
> +        libusb_handle_events_timeout(ctx, &tv);
> +        if (limit-- == 0) {
> +            return -1;
> +        }
> +    }

[Severity: Medium]
Are the iso URBs cancelled only once here? Completing a cancelled IN iso
URB runs usb_host_req_complete_iso(), which does:

    QTAILQ_INSERT_TAIL(&xfer->ring->copy, xfer, next);
    usb_wakeup(xfer->ring->ep, 0);

That can reach xhci_wakeup_endpoint(), then a kick, then
usb_host_iso_data_in(), which submits every xfer on ring->unused into
ring->inflight. The loop above never re-cancels those, and the bulk loop
earlier in usb_host_cpr_drain_xfers() has already finished, so it does not
recheck s->requests.

Can this either time out and return -1, which aborts the migration, or
leave freshly submitted iso or bulk URBs live across exec?

[ ... ]

> +static int usb_host_pre_save(void *opaque)
> +{

[ ... ]

> +    if ((mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER) &&
> +        s->dh && !s->cpr_drained) {
> +        s->cpr_replay_all = true;
> +        if (usb_host_cpr_drain_xfers(s) < 0) {
> +            return -1;
> +        }
> +    }

[Severity: Medium]
Is it safe to run the drain from usb_host_pre_save()? Packet completions
raised here can modify xHCI state that may already have been serialized,
depending on the order of the xHCI and usb-host vmstate entries in the
save sequence. Could that ordering be confirmed, or the drain be moved
earlier?

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

Re: [PATCH QEMU HCI-8.0 v6 09/12] usb: migrate the interface altsetting #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: altsetting subsection in generic vmstate_usb_device has no consumer for emulated devices
--

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

usb: migrate the interface altsetting #VSTOR-137800

This commit adds a vmstate subsection to the generic vmstate_usb_device
that migrates the USBDevice altsetting[] array. The subsection is emitted
only in the CPR migration modes and only when some interface is on a
non-zero altsetting.

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

[ ... ]

> +static bool usb_altsetting_needed(void *opaque)
> +{
> +    USBDevice *dev = opaque;
> +    int i;
> +
> +    /*
> +     * Only the CPR target consumes this, to rebuild its endpoints; other
> +     * migrations reopen the device and reconstruct the altsetting.
> +     */
> +    if (!cpr_incoming_needed(NULL)) {
> +        return false;
> +    }
> +    for (i = 0; i < USB_MAX_INTERFACES; i++) {
> +        if (dev->altsetting[i]) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +static const VMStateDescription vmstate_usb_device_altsetting = {
> +    .name = "USBDevice/altsetting",

[Severity: Low]
Is this gate specific enough to the devices that consume the state?
usb_altsetting_needed() checks only cpr_incoming_needed() and whether
some dev->altsetting[i] is non-zero. It does not check whether the
device is a usb-host device.

The subsection lives in the generic vmstate_usb_device, so an emulated
USB device that supports SET_INTERFACE and sits on a non-zero altsetting
(an audio-style device, for example) will also emit and load
altsetting[] in CPR modes.

The generic usb_device_post_load() only recomputes dev->attached:

    if (dev->state == USB_STATE_NOTATTACHED) {
        dev->attached = false;
    } else {
        dev->attached = true;
    }

Nothing there rebuilds dev->ifaces[] or calls usb_desc_ep_init() from
the loaded altsetting[]. On the CPR target such a device could report
the restored altsetting via GET_INTERFACE while its endpoints still
follow altsetting 0.

No emulated device that triggers this today has been identified, so this
is speculative.

The later patch in the series, "usb-host: reconfigure endpoints on CPR
incoming", adds usb_host_cpr_reclaim_interfaces(), which range-checks the
migrated altsetting. It also adds a usb_host_ep_update() call in
usb_host_post_load() and changes the assert in usb_host_ep_update() to
check alt >= 0. That covers the usb-host consumer and the unvalidated
index concern. The series adds no consumer for non-usb-host devices.

Should the subsection be restricted to usb-host devices, or should the
generic post_load rebuild the interface and endpoint state from
altsetting[]?

> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = usb_altsetting_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_INT32_ARRAY(altsetting, USBDevice, USB_MAX_INTERFACES),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};

[ ... ]

> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_usb_device_altsetting,
> +        NULL
>      }
>  };

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

Re: [PATCH QEMU HCI-8.0 v6 08/12] usb-host: hand off the device across cpr-transfer #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: cpr_handed_off is set before the handoff outcome is known and never cleared on close
- [Medium] usb-host: target restarts URB reaping in post_load() before migration is committed
- [Medium] usb-host: per-device CPR handoff stops and restarts events for the whole libusb context
- [Low] usb-host: commit message says .exit() closes the FD, but exit notifier only calls libusb_close()
--

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

usb-host: hand off the device across cpr-transfer #VSTOR-137800

This patch makes the usb-host device hand off URB reaping together with the
usbfs fd across cpr-transfer. The target stops reaping until post_load. The
source stops reaping when it quiesces, restarts on resume after a failure,
and skips device teardown at exit after a successful handoff.

> 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

[ ... ]

> +static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
> +{
> +    if (usb_host_cpr_drain_xfers(s) < 0) {
> +        return -1;
> +    }
> +    if (mode == MIG_MODE_CPR_TRANSFER) {
> +        usb_host_cpr_stop_events();
> +        s->cpr_handed_off = true;
> +    }
> +    return 0;
> +}

[Severity: Medium]
Is cpr_handed_off being set too early here? It is set from the vm-state
FINISH_MIGRATE branch or from usb_host_pre_save(), before the outcome of
the migration is known. The flag then means "handed off, awaiting outcome"
rather than "handoff succeeded".

The only place that clears it is usb_host_cpr_reissue(), which runs only
when the VM resumes with cpr_drained set and s->dh non-NULL.

Consider a failed or cancelled cpr-transfer where the VM stays paused, and
the user then quits or does device_del. Both usb_host_exit_notifier() and
usb_host_close() take the skip path:

    if (!s->cpr_handed_off) {
        usb_host_abort_xfers(s);
        ...
        usb_host_attach_kernel(s);
    }

The source is still the only owner of the device in that case. Does this
leave the interfaces claimed, skip libusb_reset_device(), and never rebind
the host driver? In usb_host_close() the guest port also stays attached,
and the iso rings are not freed.

usb_host_close() also sets s->dh = NULL without clearing the flag. After
that, usb_host_cpr_vm_state() returns early on !s->dh, so
usb_host_cpr_reissue() can never clear it. Would a later reopen of the same
USBHostDevice through autoscan then inherit cpr_handed_off == true and
cpr_drained?

Could the flag be cleared in usb_host_close(), or the skip decision be
based on a state that is only set once the migration has completed?

[ ... ]

> @@ -1344,10 +1365,17 @@ static void usb_host_exit_notifier(struct Notifier *n, void *data)
>      USBHostDevice *s = container_of(n, USBHostDevice, exit);
>  
>      if (s->dh) {
> +        /*
> +         * Handed to new QEMU across cpr-transfer: do not reset, release
> +         * interfaces or rebind the host driver underneath it; just close
> +         * our handle.
> +         */
> +        if (!s->cpr_handed_off) {
> +            usb_host_abort_xfers(s);
> +            usb_host_release_interfaces(s);
> +            libusb_reset_device(s->dh);
> +            usb_host_attach_kernel(s);
> +        }
>          libusb_close(s->dh);
>      }
>  }

[Severity: Low]
This isn't a bug, but the commit message says "On success the source's
.exit() leaves the device untouched ... and just closes its own copy of the
FD."

usb_host_exit_notifier() only calls libusb_close(s->dh). That does not
close the application-owned usbfs fd of a libusb_wrap_sys_device() handle.
s->hostfd is closed only in usb_host_close(). Could the message be
corrected?

The message also says "The source stops reaping in .pre_save()". Normally
this happens in the vm-state handler through usb_host_cpr_quiesce().
pre_save only does it as a fallback when the VM was already paused.

[ ... ]

> @@ -2000,6 +2042,7 @@ static int usb_host_post_load(void *opaque, int version_id)
>       * rescan in usb_host_post_load_bh() -- those are for cross-host migration.
>       */
>      if (cpr_is_incoming()) {
> +        usb_host_cpr_restart_events();
>          return 0;
>      }

[Severity: Medium]
Can the target start reaping too early here? The target calls
usb_host_cpr_restart_events() when the device section is loaded, not when
migration has completed.

Suppose another device section fails to load, or the channel breaks, while
the target is still alive. The source then resumes, and usb_host_cpr_reissue()
calls usb_host_cpr_restart_events() on its side:

    if (s->cpr_handed_off) {
        usb_host_cpr_restart_events();
        s->cpr_handed_off = false;
    }

Both processes could then reap URB completions on the shared usbfs fd. That
is the isochronous wedge this patch is meant to prevent. Is anything making
the target's takeover conditional on the source having committed? This
window is narrow and depends on the target surviving a failed migration.

[ ... ]

> +static void usb_host_cpr_stop_events(void)
> +{
> +    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
> +
> +    libusb_set_pollfd_notifiers(ctx, NULL, NULL, NULL);
> +    if (poll) {
> +        for (int i = 0; poll[i] != NULL; i++) {
> +            usb_host_del_fd(poll[i]->fd, ctx);
> +        }
> +        free(poll);
> +    }
> +}
> +
> +static void usb_host_cpr_restart_events(void)
> +{
> +    const struct libusb_pollfd **poll = libusb_get_pollfds(ctx);
> +
> +    libusb_set_pollfd_notifiers(ctx, usb_host_add_fd, usb_host_del_fd, ctx);
> +    if (poll) {
> +        for (int i = 0; poll[i] != NULL; i++) {
> +            usb_host_add_fd(poll[i]->fd, poll[i]->events, ctx);
> +        }
> +        free(poll);
> +    }
> +}

[Severity: Medium]
Do these helpers act on more than the one device being handed off? Both
call libusb_set_pollfd_notifiers(ctx, ...) and walk all of
libusb_get_pollfds(ctx) on the static libusb context, while they are driven
from per-device hooks (usb_host_realize(), usb_host_post_load(),
usb_host_pre_save() and the vm-state handler) and the handoff state
(cpr_handed_off) is per device.

With more than one hostdevice= usb-host device:

On the target, the first device's usb_host_post_load() calls
usb_host_cpr_restart_events() and re-enables reaping for every device,
including devices whose post_load has not yet run and whose source side may
not be handed off.

On the source, the first device's usb_host_cpr_reissue() after a failure
restarts events for devices that are still handed off.

While the notifiers are NULL, fds of devices realized or hot-added in that
window are never registered either.

Is the multi-device case intended to be supported? The conditional blocker
added later in the series ("usb-host: make CPR migration blocker
conditional") still seems to allow several hostdevice= devices with ids.

[ ... ]

> @@ -1455,6 +1489,14 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
>              error_setg(errp, "failed to open host usb device %s", s->hostdevice);
>              return;
>          }
> +        if (cpr_is_incoming()) {
> +            /*
> +             * The source still owns the shared fd until it hands off.  Don't
> +             * reap URBs on it yet, or we would steal the source's in-flight
> +             * completions; .post_load() restarts events once we take over.
> +             */
> +            usb_host_cpr_stop_events();
> +        }

In usb_host_realize() on the target, does this disable event handling for
the whole libusb context whenever any hostdevice= device is realized? See
the previous comment on usb_host_cpr_stop_events().

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

Re: [PATCH QEMU HCI-8.0 v6 10/12] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: CPR post_load fails on an unconfigured device (NOT_FOUND treated as fatal)
- [Medium] usb-host: re-claim loop uses array index instead of bInterfaceNumber
- [Medium] usb-host: device-supplied bInterfaceNumber indexes altsetting[16] unchecked; ninterfaces unclamped
- [Low] usb-host: usb_host_ep_update() result ignored on CPR path, so partial rebuild reports success
--

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

usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

This commit makes the CPR incoming path in usb_host_post_load() re-claim the
interfaces through libusb and call usb_host_ep_update(). The goal is to
rebuild the endpoints for a non-zero altsetting restored by vmload. It also
adds an "alt >= 0" check to an assert in usb_host_ep_update().

> 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

[ ... ]

> +static int usb_host_cpr_reclaim_interfaces(USBHostDevice *s)
> +{
> +    USBDevice *udev = USB_DEVICE(s);
> +    struct libusb_config_descriptor *conf;
> +    int n, rc;
> +
> +    rc = libusb_get_active_config_descriptor(s->dev, &conf);
> +    if (rc != 0) {
> +        usb_host_libusb_error("libusb_get_active_config_descriptor", rc);
> +        return -1;
> +    }

[Severity: Medium]
Can this abort the whole incoming CPR vmload for a device that has no active
configuration?

For a device in the address state (for example after SET_CONFIGURATION(0) or
a device reset), libusb_get_active_config_descriptor() returns
LIBUSB_ERROR_NOT_FOUND.  Here any rc != 0 returns -1, and
usb_host_post_load() propagates it:

    if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
        return -1;
    }

Before this patch the CPR branch always returned 0.  The existing
usb_host_claim_interfaces() treats the same condition as benign:

    if (rc == LIBUSB_ERROR_NOT_FOUND) {
        /* address state - ignore */
        return USB_RET_SUCCESS;
    }

and usb_host_ep_update() simply returns.  Should NOT_FOUND be tolerated here
as well?

Also, if the host still has an active configuration but the guest never
configured the device, does this function claim every interface and then
overwrite the guest-visible state?

    udev->ninterfaces = conf->bNumInterfaces;
    udev->configuration = conf->bConfigurationValue;

The source had both of these at 0.  The later patch "usb-host: make CPR
migration blocker conditional" only changes the blocker and does not touch
this helper.

> +
> +    for (int i = 0; i < conf->bNumInterfaces; i++) {
> +        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
> +        int alt = udev->altsetting[ifnum];

[Severity: Medium]
Can this read past the end of udev->altsetting[]?

ifnum comes from the device's bInterfaceNumber (a uint8_t, up to 255), while
altsetting[] has USB_MAX_INTERFACES (16) entries.  This validation loop runs
over all conf->bNumInterfaces entries with no bound on ifnum.

The claim loop below is clamped with MIN(conf->bNumInterfaces,
USB_MAX_INTERFACES), but the function then stores the unclamped value and
returns success:

    udev->ninterfaces = conf->bNumInterfaces;

Can a device with more than 16 interfaces then trip this assert in a later
usb_desc_set_config()?

    assert(dev->ninterfaces <= USB_MAX_INTERFACES);

usb_host_claim_interfaces() would have returned a stall in that case.

usb_host_ep_update(), which is now also called from the CPR path, has the
same unchecked access:

    alt = udev->altsetting[intf->bInterfaceNumber];

This isn't a bug introduced by this patch, but the new call site makes it
reachable on the CPR path.  The patch only adds "alt >= 0" to the assert
there.

> +
> +        if (alt < 0 || alt >= conf->interface[i].num_altsetting) {
> +            error_report("usb-host: migrated altsetting %d out of range for "
> +                         "interface %d", alt, ifnum);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +    }
> +
> +    n = MIN(conf->bNumInterfaces, USB_MAX_INTERFACES);
> +    for (int i = 0; i < n; i++) {
> +        rc = libusb_claim_interface(s->dh, i);
> +        if (rc != 0) {
> +            usb_host_libusb_error("libusb_claim_interface", rc);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +        s->ifs[i].claimed = true;
> +    }

[Severity: Medium]
Is the loop index i the right value to pass to libusb_claim_interface()?

i is the position in conf->interface[], but libusb and usbfs expect the
interface number, which is altsetting[0].bInterfaceNumber.  The validation
loop above already uses bInterfaceNumber for this.  The two can differ for
devices whose interface numbers don't start at 0 or aren't contiguous, such
as {0,2} or {1,2}.

Claiming a non-existent number fails, so the function returns -1 and the
migration aborts.  Where the claim does succeed, the real interfaces are not
recorded in libusb's claimed set, so a later SET_INTERFACE can still fail
with LIBUSB_ERROR_NOT_FOUND.  That is the problem this patch sets out to fix.

s->ifs[i].claimed is also indexed by position, so usb_host_release_interfaces()
would act on the wrong numbers.

By contrast, usb_host_claim_interfaces() probes by interface number and
tolerates gaps:

    for (i = 0; i < USB_MAX_INTERFACES; i++) {
        trace_usb_host_claim_interface(s->bus_num, s->addr, configuration, i);
        rc = libusb_claim_interface(s->dh, i);
        if (rc == 0) {
            s->ifs[i].claimed = true;

Should the new code claim by bInterfaceNumber and index ifs[] the same way?
The later patches in the series do not change this.

[ ... ]

>      if (cpr_is_incoming()) {
> +        /*
> +         * The device stayed open, so rebuild libusb's claims and endpoints,
> +         * and start reaping now that the source has handed the fd off.
> +         */
> +        if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
> +            return -1;
> +        }
> +        usb_host_ep_update(dev);
>          usb_host_cpr_restart_events();
>          return 0;
>      }

[Severity: Low]
Should the result of usb_host_ep_update() be checked here?

usb_host_ep_update() is void.  After usb_ep_reset() it returns silently on
"invalid endpoint address", "duplicate endpoint address", or a missing
active configuration, which leaves a partial endpoint table.
usb_host_post_load() then returns 0, so guest transfers to the missing
endpoints would stall without any report.

usb_host_ep_update() also calls usb_ep_set_halted(udev, pid, ep, 0) for every
endpoint, so any halted state the guest had on the source is lost, since it
is not migrated.  This second point may be speculative.  The failure
conditions would normally already have occurred in usb_host_open() on the
same device, so this is probably a rare edge case.

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

Re: [PATCH QEMU HCI-8.0 v6 11/12] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800

[no-reply]

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

Re: [PATCH QEMU HCI-8.0 v6 12/12] usb-host: make CPR migration blocker conditional #VSTOR-137800

[no-reply]

> A previous commit added an unconditional blocker for CPR migrations.
> That is to refuse CPR gracefully instead of crashing and for the previous
> commits to remain bisectable.
> 
> Now the restore path is complete: the FD is preserved, in-flight URBs
> are drained and replayed, the device is handed off across cpr-transfer,
> and the endpoints and isochronous streams are reconstructed on the
> target.  So let's lift the CPR blocker for the devices which satisfy the
> preconditions.
> 
> Still, the remaining preconditions for CPR to work are:
> 
>   * We must open host device FD ourselves, so that libusb owns the FD.
>     This way we're able to preserve the FD during CPR;
>   * Device ID must be provided as a unique stable key for CPR FD registry.
> 
> Use them to guard the blocker addition.  Apart from that, prohibit CPR on
> WIN32 platform and on hosts with older libusb versions.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH QEMU HCI-8.0 v6 00/12] usb-host: support CPR migration

[Andrey Drobyshev]

Some of the issues reported by Sashiko appear to be real, I'll fix them.

On 10/6/26 1:34 PM, Andrey Drobyshev wrote:
> v5 -> v6:
> 
> Substantial changes:
> 
>   * Drain each usb-host from a vm-state change handler when the VM stops
>     for CPR (RUN_STATE_FINISH_MIGRATE), before the device and interrupt
>     controller are snapshotted.  A completion raised during the drain now
>     lands in the still-live interrupt controller and is migrated, instead
>     of being lost as it was when the drain ran from .pre_save() after the
>     snapshot.
> 
>   * Deliver a cancelled transfer that moved some data as a short transfer
>     instead of dropping it; a zero-progress one is suppressed for the
>     target to replay from the start of the TD.  A VM already paused when
>     migration starts keeps a suppress-all drain in .pre_save() as a
>     fallback.
> 
> Minor changes (refactoring/cleanup):
> 
>   * Group the per-device CPR fields into one struct block; factor the
>     drain-and-handoff tail into usb_host_cpr_quiesce(); fold the
>     altsetting range check into the interface reclaim (one config
>     descriptor read); register the exit notifier and vm-state handler
>     only after the blocker, shrinking the realize error path.
> 
>   * Commit message and comment rewording, forward-declaration placement,
>     and a checkpatch line-length fix.  No functional change.
> 
> v5: http://lore.virtuozzo.com/qemu/20260925-usb-host_v3-v5-0-e5b21724db4d@virtuozzo.com/
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> Andrey Drobyshev (12):
>       usb-host: don't leak hostdev FD on open failure #VSTOR-137800
>       usb-host: add migration blocker for CPR modes #VSTOR-137800
>       usb-host: preserve hostdev FD during CPR migration #VSTOR-137800
>       usb-host: factor out usb_host_reap_xfers() #VSTOR-137800
>       usb-host: drain in-flight URBs across CPR #VSTOR-137800
>       usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800
>       usb-host: skip product-string read on CPR incoming #VSTOR-137800
>       usb-host: hand off the device across cpr-transfer #VSTOR-137800
>       usb: migrate the interface altsetting #VSTOR-137800
>       usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800
>       hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800
>       usb-host: make CPR migration blocker conditional #VSTOR-137800
> 
>  hw/usb/bus.c         |  36 ++++
>  hw/usb/hcd-xhci.c    |  20 ++
>  hw/usb/host-libusb.c | 525 +++++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 542 insertions(+), 39 deletions(-)
> ---
> base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8
> change-id: 20260914-usb-host_v3-bbfea86d1dad
> 
> Best regards,
> --  
> Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>