[PATCH QEMU HCI-8.0 v7 00/14] usb-host: support CPR migration

[PATCH QEMU HCI-8.0 v7 00/14] usb-host: support CPR migration

[Andrey Drobyshev]

v6 -> v7:

No major logical/functional changes.

Mostly code deduplication, refactoring and minor hardening for issues
reported by Sashiko:
https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v6-0-8688ec9576d1@virtuozzo.com

v6: http://lore.virtuozzo.com/20261006-usb-host_v3-v6-0-8688ec9576d1@virtuozzo.com

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
Andrey Drobyshev (14):
      usb-host: don't leak hostdev FD on open failure #VSTOR-137800
      usb-host: don't leak config descriptor on claim 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: factor out usb_host_start_events() #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    |  25 +++
 hw/usb/host-libusb.c | 575 ++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 583 insertions(+), 53 deletions(-)
---
base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8
change-id: 20260914-usb-host_v3-bbfea86d1dad

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

[PATCH QEMU HCI-8.0 v7 01/14] 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..d4a8c68e6d4 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -961,6 +961,11 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
         goto fail;
     }
 
+    /* Track the caller-owned FD so the fail: path frees it. */
+    if (!dev) {
+        s->hostfd = hostfd;
+    }
+
     if (dev) {
         bus_num = libusb_get_bus_number(dev);
         addr = libusb_get_device_address(dev);
@@ -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 v7 02/14] usb-host: don't leak config descriptor on claim failure #VSTOR-137800

[Andrey Drobyshev]

usb_host_claim_interfaces() fetches the active config descriptor but
frees it only on the success path.  When not every interface can be
claimed it returns USB_RET_STALL without freeing it, leaking the
descriptor.

Free it before the error return.

Fixes: 896b6757f9c4 ("usb-host: support devices with sparse/non-sequential USB interfaces")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/usb/host-libusb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index d4a8c68e6d4..7c8b65ad5af 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1381,6 +1381,7 @@ static int usb_host_claim_interfaces(USBHostDevice *s, int configuration)
         }
     }
     if (claimed != conf->bNumInterfaces) {
+        libusb_free_config_descriptor(conf);
         return USB_RET_STALL;
     }
 

-- 
2.47.1

[PATCH QEMU HCI-8.0 v7 03/14] 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 7c8b65ad5af..e3bd7bcf749 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 v7 04/14] 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 e3bd7bcf749..beab8edc619 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;
         }
@@ -1764,6 +1785,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 v7 05/14] 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 beab8edc619..1a79cdf9e4d 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 v7 06/14] 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 | 236 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 209 insertions(+), 27 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 1a79cdf9e4d..e631b683ed1 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,29 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
     return NULL;
 }
 
+/*
+ * Classify a reap during the CPR drain (r->p is still set).  The paused-VM
+ * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
+ * reap for the target to replay; otherwise one that moved no data is suppressed
+ * and one that moved some is kept as a short transfer.  Returns true if
+ * suppressed, so the caller frees the request.
+ */
+static bool usb_host_req_cpr_replay(USBHostRequest *r,
+                                    struct libusb_transfer *xfer)
+{
+    if (!r->host->cpr_replay_all) {
+        if (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
+            return false;
+        }
+        if (xfer->actual_length != 0) {
+            /* 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 +462,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 +515,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 +538,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 +1141,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 +1162,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 +1181,117 @@ 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 the in-flight
+ * transfers before the device/irqchip snapshot, so completions raised during
+ * the drain are migrated too.
+ */
+static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
+{
+    if (usb_host_cpr_drain_xfers(s) < 0) {
+        return -1;
+    }
+    return 0;
+}
+
+/*
+ * Run the CPR quiesce from the vm-state handler, on the stop-for-CPR
+ * transition before the device/irqchip snapshot.
+ */
+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_quiesce(s, mode) < 0) {
+        migration_file_set_error(-EIO, NULL);
+    }
+}
+
 static int usb_host_close(USBHostDevice *s)
 {
     USBDevice *udev = USB_DEVICE(s);
@@ -1301,6 +1459,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 +1483,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)
@@ -1792,10 +1955,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;
@@ -1810,10 +1971,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_quiesce(s, mode) < 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 v7 07/14] 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.

A re-issued transfer is replayed from the start of the TD, so a partial
transfer loses its progress -- an OUT fragment already consumed by the
device may be sent twice.  Only the paused-VM .pre_save() fallback queues
partial transfers for re-issue; the normal running-VM drain delivers them
as short transfers and is unaffected.

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

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index e631b683ed1..d90117e4292 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;
 
@@ -169,6 +170,7 @@ static void usb_host_release_interfaces(USBHostDevice *s);
 static void usb_host_nodev(USBHostDevice *s);
 static void usb_host_detach_kernel(USBHostDevice *s);
 static void usb_host_attach_kernel(USBHostDevice *s);
+static void usb_host_handle_data(USBDevice *udev, USBPacket *p);
 
 /* ------------------------------------------------------------------------ */
 
@@ -433,9 +435,10 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
 /*
  * Classify a reap during the CPR drain (r->p is still set).  The paused-VM
  * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
- * reap for the target to replay; otherwise one that moved no data is suppressed
- * and one that moved some is kept as a short transfer.  Returns true if
- * suppressed, so the caller frees the request.
+ * reap; otherwise one that moved no data is suppressed and one that moved some
+ * is kept as a short transfer.  Suppressed packets are queued on cpr_inflight
+ * for the target to replay.  Returns true if suppressed, so the caller frees
+ * the request.
  */
 static bool usb_host_req_cpr_replay(USBHostRequest *r,
                                     struct libusb_transfer *xfer)
@@ -450,6 +453,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
             return false;
         }
     }
+    if (r->p->state == USB_PACKET_ASYNC) {
+        r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
+    }
     return true;
 }
 
@@ -1271,16 +1277,74 @@ static int usb_host_cpr_quiesce(USBHostDevice *s, MigMode mode)
     return 0;
 }
 
+/*
+ * 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.  A transfer that already moved some
+         * data is re-run in full, so an OUT fragment the device consumed may be
+         * sent twice.  Only the paused-VM pre_save fallback queues such partial
+         * transfers here; the normal drain delivers them as short transfers.
+         */
+        p->actual_length = 0;
+        if (p->ep->nr == 0) {
+            /* control endpoint: can't be reconstructed, so error it out */
+            p->status = USB_RET_IOERROR;
+            usb_generic_async_ctrl_complete(udev, p);
+        } else {
+            usb_host_handle_data(udev, p);
+            if (p->status != USB_RET_ASYNC) {
+                /* completed synchronously with an error */
+                if (usb_host_use_combining(p->ep)) {
+                    usb_combined_input_packet_complete(udev, p);
+                } else {
+                    usb_packet_complete(udev, p);
+                }
+            }
+        }
+    }
+    g_slist_free(s->cpr_inflight);
+    s->cpr_inflight = NULL;
+    s->cpr_replay_all = false;
+    s->cpr_drained = false;
+}
+
 /*
  * Run the CPR quiesce from the vm-state handler, on the stop-for-CPR
- * transition before the device/irqchip snapshot.
+ * transition before the device/irqchip snapshot, and re-issue the drained
+ * packets 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 (running) {
+        /*
+         * Resume: re-issue drained packets and reset state, even if the
+         * device was lost between the drain and now (dh == NULL).
+         */
+        usb_host_cpr_reissue(s);
+        return;
+    }
+    if (!s->dh) {
+        return;
+    }
+    if (state != RUN_STATE_FINISH_MIGRATE) {
         return;
     }
     mode = migrate_mode();
@@ -1486,6 +1550,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)
@@ -1493,6 +1559,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 v7 08/14] 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 d90117e4292..271456abb0a 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1111,7 +1111,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 v7 09/14] usb-host: factor out usb_host_start_events() #VSTOR-137800

[Andrey Drobyshev]

usb_host_init() registers the libusb pollfd notifiers and hands libusb's
current fds to the QEMU main loop so URB completions get reaped.  Move
that into a helper so the CPR hand-off code can reuse it to resume reaping
after a switchover.  No functional change.

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

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 271456abb0a..09ffb5d48f7 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -265,6 +265,24 @@ static void usb_host_del_fd(int fd, void *user_data)
     qemu_set_fd_handler(fd, NULL, NULL, NULL);
 }
 
+/*
+ * Register the libusb event callbacks and hand its current fds to the QEMU
+ * main loop, so URB completions start getting reaped.
+ */
+static void usb_host_start_events(void)
+{
+    const struct libusb_pollfd **poll;
+
+    libusb_set_pollfd_notifiers(ctx, usb_host_add_fd, usb_host_del_fd, ctx);
+    poll = libusb_get_pollfds(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 QEMUTimer *poll_timer;
@@ -292,9 +310,6 @@ static void usb_host_timer(void *opaque)
 
 static int usb_host_init(void)
 {
-#ifndef CONFIG_WIN32
-    const struct libusb_pollfd **poll;
-#endif
     int rc;
 
     if (ctx) {
@@ -313,17 +328,7 @@ static int usb_host_init(void)
     poll_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, usb_host_timer, NULL);
     usb_host_timer_kick();
 #else
-    libusb_set_pollfd_notifiers(ctx, usb_host_add_fd,
-                                usb_host_del_fd,
-                                ctx);
-    poll = libusb_get_pollfds(ctx);
-    if (poll) {
-        int i;
-        for (i = 0; poll[i] != NULL; i++) {
-            usb_host_add_fd(poll[i]->fd, poll[i]->events, ctx);
-        }
-    }
-    free(poll);
+    usb_host_start_events();
 #endif
     return 0;
 }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v7 10/14] 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.

Reaping is stopped and restarted on the whole libusb context, which is
shared by all usb-host devices, so cpr-transfer currently supports a
single hostdevice= usb-host at a time.

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

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 09ffb5d48f7..387040d6e1a 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;
@@ -283,6 +284,23 @@ static void usb_host_start_events(void)
     }
 }
 
+/*
+ * cpr-transfer shares the usbfs fd with the new QEMU, so stop reaping URBs on
+ * it here -- only one process may drive the device.
+ */
+static void usb_host_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);
+    }
+}
+
 #else
 
 static QEMUTimer *poll_timer;
@@ -306,6 +324,9 @@ static void usb_host_timer(void *opaque)
     usb_host_timer_kick();
 }
 
+static void usb_host_start_events(void) {}
+static void usb_host_stop_events(void) {}
+
 #endif /* !CONFIG_WIN32 */
 
 static int usb_host_init(void)
@@ -1272,13 +1293,18 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
 /*
  * Quiesce the URB data plane around a CPR switchover: drain the in-flight
  * transfers before the device/irqchip snapshot, so completions raised during
- * the drain are migrated too.
+ * the drain are migrated too.  For cpr-transfer, also hand reaping of the
+ * shared fd off to the target by stopping events on this side.
  */
 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_stop_events();
+        s->cpr_handed_off = true;
+    }
     return 0;
 }
 
@@ -1297,6 +1323,11 @@ static void usb_host_cpr_reissue(USBHostDevice *s)
         return;
     }
 
+    if (s->cpr_handed_off) {
+        usb_host_start_events();
+        s->cpr_handed_off = false;
+    }
+
     for (it = s->cpr_inflight; it; it = it->next) {
         p = it->data;
         /*
@@ -1371,16 +1402,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;
@@ -1415,10 +1449,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);
     }
 }
@@ -1487,6 +1528,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_stop_events();
+        }
     } else
 #endif
     if (s->match.addr && s->match.bus_num &&
@@ -1550,6 +1599,12 @@ static void usb_host_unrealize(USBDevice *udev)
     if (s->needs_autoscan) {
         QTAILQ_REMOVE(&hostdevs, s, next);
     }
+    /*
+     * An explicit device_del means we still own the device (a successful
+     * cpr-transfer handoff exits the process instead), so clear the handoff
+     * flag to force a full reset/release in usb_host_close().
+     */
+    s->cpr_handed_off = false;
     usb_host_close(s);
     migrate_del_blocker(&s->cpr_blocker);
     if (s->cpr_vmse) {
@@ -2033,6 +2088,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_start_events();
         return 0;
     }
 

-- 
2.47.1

[PATCH QEMU HCI-8.0 v7 11/14] 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 v7 12/14] 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 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 387040d6e1a..cbe1b91c46d 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -976,7 +976,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];
         }
 
@@ -2079,6 +2079,69 @@ 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 rc;
+
+    rc = libusb_get_active_config_descriptor(s->dev, &conf);
+    if (rc == LIBUSB_ERROR_NOT_FOUND) {
+        /* device is unconfigured (address state), nothing to reclaim */
+        return 0;
+    }
+    if (rc != 0) {
+        usb_host_libusb_error("libusb_get_active_config_descriptor", rc);
+        return -1;
+    }
+
+    /*
+     * Validate all migrated interface/altsetting numbers before touching the
+     * device, so garbage migration data can't leave it half-claimed.
+     */
+    for (int i = 0; i < conf->bNumInterfaces; i++) {
+        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
+        int alt;
+
+        if (ifnum >= USB_MAX_INTERFACES) {
+            error_report("usb-host: interface number %d out of range", ifnum);
+            libusb_free_config_descriptor(conf);
+            return -1;
+        }
+        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;
+        }
+    }
+
+    /* Data is sane: claim each interface on the fd and record it. */
+    for (int i = 0; i < conf->bNumInterfaces; i++) {
+        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
+
+        rc = libusb_claim_interface(s->dh, ifnum);
+        if (rc != 0) {
+            usb_host_libusb_error("libusb_claim_interface", rc);
+            libusb_free_config_descriptor(conf);
+            return -1;
+        }
+        s->ifs[ifnum].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;
@@ -2088,6 +2151,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_start_events();
         return 0;
     }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v7 13/14] 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 292c378bfc9..421494fc379 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;
@@ -1047,6 +1049,11 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
     ctx[0] &= ~EP_STATE_MASK;
     ctx[0] |= state;
 
+    if (state != EP_RUNNING) {
+        /* a stop/reset before the first kick ends the post-CPR resync window */
+        epctx->cpr_iso_resync = false;
+    }
+
     /* update ring dequeue ptr */
     if (epctx->nr_pstreams) {
         if (sctx != NULL) {
@@ -1811,6 +1818,19 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
         xfer->pkts = 1;
         xfer->iso_xfer = true;
         xfer->timed_xfer = true;
+        if (epctx->cpr_iso_resync) {
+            /*
+             * First iso TD after a CPR resume: our transfer state was
+             * rebuilt from guest memory and no longer lines up with what
+             * the guest consumed.  Report a missed service, like real HW,
+             * to kick the driver's skip logic into resyncing.
+             */
+            epctx->cpr_iso_resync = false;
+            xfer->status = CC_MISSED_SERVICE_ERROR;
+            xhci_xfer_report(xfer);
+            xfer->complete = 1;
+            return 0;
+        }
         mfindex = xhci_mfindex_get(xhci);
         xhci_calc_iso_kick(xhci, xfer, epctx, mfindex);
         xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
@@ -3510,6 +3530,11 @@ static int usb_xhci_post_load(void *opaque, int version_id)
             xhci_init_epctx(epctx, pctx, ep_ctx);
             epctx->state = state;
             if (state == EP_RUNNING) {
+                if (cpr_is_incoming() &&
+                    (epctx->type == ET_ISO_IN || epctx->type == ET_ISO_OUT)) {
+                    /* only iso streams need resyncing; see xhci_submit() */
+                    epctx->cpr_iso_resync = true;
+                }
                 /* kick endpoint after vmload is finished */
                 timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
             }

-- 
2.47.1

[PATCH QEMU HCI-8.0 v7 14/14] 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 cbe1b91c46d..d86657f5eb3 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1563,16 +1563,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 v7 02/14] usb-host: don't leak config descriptor on claim failure #VSTOR-137800

[no-reply]

> usb_host_claim_interfaces() fetches the active config descriptor but
> frees it only on the success path.  When not every interface can be
> claimed it returns USB_RET_STALL without freeing it, leaking the
> descriptor.
> 
> Free it before the error return.
> 
> Fixes: 896b6757f9c4 ("usb-host: support devices with sparse/non-sequential USB interfaces")
> 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-v7-0-044f668789a1@virtuozzo.com?part=2

Re: [PATCH QEMU HCI-8.0 v7 03/14] 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-v7-0-044f668789a1@virtuozzo.com?part=3

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

[no-reply]

> 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>

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

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

Re: [PATCH QEMU HCI-8.0 v7 05/14] 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-v7-0-044f668789a1@virtuozzo.com?part=5

Re: [PATCH QEMU HCI-8.0 v7 04/14] 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-v7-0-044f668789a1@virtuozzo.com?part=4

Re: [PATCH QEMU HCI-8.0 v7 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: CPR drain suppresses completed URBs and reports short OUT as full
- [Medium] usb-host: iso drain cancels in-flight URBs once, so resubmitted URBs survive
- [Medium] usb-host: timeout path completes packets mid-iteration then drops live URBs
- [Medium] usb-host: usb_host_req_cpr_replay() not gated on an active CPR drain
--

commit e57108ef005a6c96c6eda4382d78b25d2524c377
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 usb-host URBs when the VM stops for CPR. It also adds a
.pre_save() fallback for a VM that is already paused, and cancels and
reaps isochronous URBs on their own rings. Transfers 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).  The paused-VM
> + * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
> + * reap for the target to replay; otherwise one that moved no data is suppressed
> + * and one that moved some is kept as a short transfer.  Returns true if
> + * suppressed, so the caller frees the request.
> + */
> +static bool usb_host_req_cpr_replay(USBHostRequest *r,
> +                                    struct libusb_transfer *xfer)
> +{
> +    if (!r->host->cpr_replay_all) {
> +        if (xfer->status != LIBUSB_TRANSFER_CANCELLED) {
> +            return false;
> +        }
> +        if (xfer->actual_length != 0) {
> +            /* deliver as a short transfer */
> +            xfer->status = LIBUSB_TRANSFER_COMPLETED;
> +            return false;
> +        }
> +    }
> +    return true;
> +}

[Severity: Medium]
Does this function lose data or duplicate side effects across CPR?

In the paused-VM fallback, usb_host_pre_save() sets cpr_replay_all = true.
usb_host_req_cpr_replay() then skips its status check and returns true
for every reap, including LIBUSB_TRANSFER_COMPLETED ones.

The packet is suppressed and replayed by the target.  An IN URB that had
already received data loses it, since the device will not resend.  A bulk
OUT or control URB that already executed is sent again.

This also seems to contradict the commit message statement "A transfer
that already completed in the kernel is delivered normally".

The final series state still has this behaviour.  The later patch
"usb-host: re-issue drained URBs if CPR is aborted" only documents it in a
comment ("an OUT fragment the device consumed may be sent twice") and does
not change it.

On the normal vm_change_state path, a CANCELLED URB with actual_length != 0
is rewritten to COMPLETED:

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

usb_host_req_complete_data() only trims the packet length for IN:

    r->p->status = status_map[xfer->status];
    if (r->in && xfer->actual_length) {
        usb_packet_copy(r->p, r->buffer, xfer->actual_length);
    }

For an OUT packet, p->actual_length was already set to the full length
when the guest data was copied at submit time.  Can the guest be told that
the whole OUT TD succeeded when the device received only
xfer->actual_length bytes, with nothing replaying the remainder?

[Severity: Medium]
Separately, is usb_host_req_cpr_replay() meant to apply outside an active
CPR drain?

usb_host_req_complete_ctrl() and usb_host_req_complete_data() now call it
unconditionally for every completion with r->p != NULL.  With
cpr_replay_all false, nothing checks cpr_drained or any other drain-active
state.

A transfer that arrives with LIBUSB_TRANSFER_CANCELLED outside a CPR drain
would behave differently:

- actual_length == 0: the request is freed and the packet is never
  completed, which could leave it in USB_PACKET_ASYNC.
- actual_length != 0: the status is rewritten to COMPLETED.

Previously such a transfer was mapped by status_map[] to USB_RET_IOERROR.
Guest-initiated cancels clear r->p in usb_host_cancel_packet() and
usb_host_req_abort(), so they are not affected.

I could not confirm that libusb reports CANCELLED for a transfer with a
live packet outside CPR, so this one is speculative.  Should the logic be
gated on an active drain?

[ ... ]

> +static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> +{

[ ... ]

> +        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;
> +        }
> +    }

[Severity: Medium]
Can this timeout path leave live, uncancelled URBs behind?

The call chain is:

usb_host_cpr_drain_xfers()
  usb_host_req_complete_err()
    usb_packet_complete()
      usb_process_one()
        usb_host_handle_data()
          libusb_submit_transfer()

For a non-ep0 packet, usb_packet_complete() pops the next queued packet on
the endpoint and processes it.  That can allocate a new request, append it
to s->requests, and submit a new URB while QTAILQ_FOREACH_SAFE is running.

usb_host_drop_requests() then unlinks every request, including the newly
submitted ones, without cancelling them.  Would such a URB stay on the
preserved fd with no tracking request, and later complete into a callback
with stale r->p or r->host?

This needs the 100-iteration timeout plus a queued packet behind the failed
one on the same endpoint, which xHCI can have.

[ ... ]

> +    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;
> +}

[Severity: Medium]
Can iso URBs be resubmitted while this reap loop runs, and then escape
cancellation?

The iso URBs on ring->inflight are cancelled in a single pass, and the
reap loop does not cancel again.  For an IN endpoint,
usb_host_req_complete_iso() moves the xfer to ring->copy and calls
usb_wakeup():

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

usb_wakeup() reaches xhci_wakeup_endpoint(), which calls xhci_kick_ep()
and xhci_kick_epctx() without checking the VM run state.  The kick can
reprocess an iso IN packet through usb_host_handle_data() and
usb_host_iso_data_in(), which moves ring->unused entries to inflight with
libusb_submit_transfer().

If that happens, the reap loop either runs out its limit and returns -1,
or it exits with new iso URBs from the old process still submitted on the
preserved fd.  The control/bulk loop above re-cancels on every iteration,
but this one does not.

I did not prove that a guest iso TD is always available at that point.
Should the iso phase re-cancel inflight URBs on each iteration as well?

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: replay_all queues completed URBs, so aborted CPR re-sends or drops data
--

commit 2947a35eaf543430e9227a58d6c2b6610a15e890
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
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 instead of being re-issued.

> 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

[ ... ]

> @@ -450,6 +453,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
>              return false;
>          }
>      }
> +    if (r->p->state == USB_PACKET_ASYNC) {
> +        r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
> +    }
>      return true;
>  }

[Severity: Medium]
When cpr_replay_all is set by the paused-VM usb_host_pre_save() fallback,
the status and actual_length checks above are skipped. Does
usb_host_req_cpr_replay() then queue every reaped URB on cpr_inflight,
including URBs that completed successfully?

The request is then freed without its data being copied into the packet.

[ ... ]

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

If CPR aborts after that fallback, usb_host_cpr_reissue() re-submits these
fully completed transfers from the start:

 - A completed OUT transfer can be sent to the physical device a second
   time.
 - A completed IN transfer has its data discarded, and a new read is
   issued.
 - A completed control transfer is failed with USB_RET_IOERROR, even
   though the device already executed it.

The commit message and the comment above only mention a partial OUT
fragment being sent twice. Could they, or the queueing in
usb_host_req_cpr_replay(), be changed to cover fully completed transfers
too?

This could matter for non-idempotent streams such as mass storage, serial
or printer traffic.

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

Re: [PATCH QEMU HCI-8.0 v7 09/14] usb-host: factor out usb_host_start_events() #VSTOR-137800

[no-reply]

> usb_host_init() registers the libusb pollfd notifiers and hands libusb's
> current fds to the QEMU main loop so URB completions get reaped.  Move
> that into a helper so the CPR hand-off code can reuse it to resume reaping
> after a switchover.  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-v7-0-044f668789a1@virtuozzo.com?part=9

Re: [PATCH QEMU HCI-8.0 v7 08/14] 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 drops product string for any open
- [Low] usb-host: only the string read is skipped; other control I/O in open still runs
--

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

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

This patch skips the libusb product string descriptor read in
usb_host_open() when cpr_is_incoming() is true. 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 d90117e4292d..271456abb0a1 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -1111,7 +1111,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 too broad? cpr_is_incoming() is defined as
"incoming_mode != MIG_MODE_NONE", so it is true for the whole
incoming-migration phase. It is not a per-device check.

usb_host_open() is reached from several paths:

    usb_host_realize()    (hostfd and bus/addr paths)
    usb_host_auto_check() (autoscan)

Any device opened while the flag is set takes the synthetic branch. That
includes autoscan or hotplug matches of devices that are not being handed
over.

product_desc is set once in usb_host_open() and is not refreshed. Can
those devices end up keeping the "host:bus.addr" name after the migration
completes?

[Severity: Low]
Is the product string read the only call in usb_host_open() that can block
during the handover? The neighbouring calls still run unguarded:

    usb_host_detach_kernel(s);
    ...
    usb_host_speed_compat(s);    /* libusb_get_config_descriptor() */

The fail: path also calls libusb_reset_device().

If synchronous control transfers can block on the CPR target, could the
incoming migration still hang in one of these calls? The commit message
doesn't say what causes the blocking, so it is unclear whether skipping
only the string read is enough.

>          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-v7-0-044f668789a1@virtuozzo.com?part=8

Re: [PATCH QEMU HCI-8.0 v7 11/14] 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 c0dc143b080568d7b0883600b0776ce496b368ca
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb: migrate the interface altsetting #VSTOR-137800

This patch adds a vmstate subsection to the shared USBDevice vmstate that
carries the altsetting[] array. The subsection is emitted only in the CPR
migration modes and only when some altsetting[] entry is non-zero.

> 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;
> +}

[Severity: Low]
Is this gate too broad? usb_altsetting_needed() checks only
cpr_incoming_needed() and whether any dev->altsetting[i] is non-zero. It
does not check the device type, but the stated reason (the CPR target
rebuilds its endpoints) seems to hold only for usb-host.

[ ... ]

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

The subsection is attached to the shared vmstate_usb_device, so it is
emitted and loaded for every USB device type, including emulated devices.

For an emulated device, usb_desc_set_interface() in hw/usb/desc.c updates
the three together:

    old = dev->altsetting[index];
    dev->altsetting[index] = value;
    dev->ifaces[index] = iface;
    usb_desc_ep_init(dev);

After a CPR load, only altsetting[] is restored. ifaces[] and the endpoint
table stay at altsetting 0.

Can GET_INTERFACE then report the guest's non-zero altsetting while the
device model behaves as if altsetting 0 were active? Before this patch the
state was wrong but self-consistent. It is still wrong after the patch, so
emulated devices gain nothing from it.

The consumer in this series is only in usb_host_post_load() in
hw/usb/host-libusb.c, which calls usb_host_cpr_reclaim_interfaces() and
usb_host_ep_update(). Nothing re-applies ifaces[] or calls
usb_desc_ep_init() for emulated devices.

I did not verify which emulated devices have multiple altsettings or can be
CPR-migrated, so this may be of limited reach. Would it make sense to limit
the subsection to usb-host, or to restore ifaces[] and the endpoints in a
generic post_load?

Separately, the commit message says "The active altsetting of each
interface is host controller state". Is that accurate? altsetting[] is a
field of USBDevice, not of the host controller.

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

Re: [PATCH QEMU HCI-8.0 v7 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800

[no-reply]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] usb-host: cpr_handed_off set before migration outcome is known, skips cleanup on failure
- [Medium] usb-host: cpr_is_incoming() in realize() stops global event reaping with no restart
- [Medium] usb-host: unrealize() clears handoff flag without restarting the global event reaping
- [Low] usb-host: unrealize() forces full reset even if the target may already own the device
- [Low] usb-host: commit message does not match where reaping is stopped and restarted
--

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

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

This patch hands the usbfs FD and URB reaping off from the source QEMU to
the target QEMU during cpr-transfer. The source stops reaping at quiesce
time and skips device reset, interface release and driver rebind on exit.
The target stops reaping in realize() and restarts it in post_load().

> 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 void usb_host_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 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_stop_events();
> +        s->cpr_handed_off = true;
> +    }
>      return 0;
>  }

[Severity: Medium]
Is it safe to set cpr_handed_off here, before the outcome of the migration
is known?

The flag is cleared only in usb_host_cpr_reissue(), which runs on a running
transition, and in usb_host_unrealize().

Consider the case where usb_host_pre_save() calls usb_host_cpr_quiesce() as
the fallback for an already-paused VM, and the migration then fails.  Nothing
resumes the VM, so cpr_handed_off stays true.

usb_host_exit_notifier() then skips the following on every later quit and
only calls libusb_close():

    usb_host_abort_xfers(s);
    usb_host_release_interfaces(s);
    libusb_reset_device(s->dh);
    usb_host_attach_kernel(s);

Can this leave the device without a reset and without the host kernel
driver rebound after a failed or cancelled cpr-transfer?

The commit message says the skip happens only "On success", but the exit
notifier cannot tell success from failure.

[ ... ]

> @@ -1371,16 +1402,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;

[Severity: Medium]
The cpr_handed_off gate here also skips the QEMU-local steps
usb_host_abort_xfers(), usb_host_iso_free_all() and usb_device_detach(),
not only the reset, release and rebind that must not run under the target.

If usb_host_close() is reached while the flag is stale, for example via
usb_host_nodev_bh() or usb_host_auto_check(), s->dh and s->dev are still set
to NULL.  Can that leave a guest-attached device with no libusb handle?

Should the gate cover only the device-affecting calls (release, reset,
rebind)?

[ ... ]

> @@ -1487,6 +1528,14 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
> +        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_stop_events();
> +        }

[Severity: Medium]
Can this stop event reaping permanently for a device that is not migrated?

cpr_is_incoming() is "incoming_mode != MIG_MODE_NONE", and incoming_mode
does not appear to be reset after the migration completes.

If a usb-host device with hostdevice= is hotplugged on the target after the
CPR migration, usb_host_realize() takes this branch and calls
usb_host_stop_events(), which clears the pollfd notifiers and fd handlers on
the shared libusb ctx.

usb_host_post_load() is the only place that restarts events, and a hotplugged
device has no vmstate section to load.  Would URB completions then stop being
reaped for that device and for every other usb-host device?

[Severity: Low]
This isn't a functional problem, but the commit message and the code differ
in a few places:

    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 condition used is cpr_is_incoming(), which is true for any incoming CPR
mode, including cpr-exec, where the fd is not shared.

    The source stops reaping in .pre_save() and, since it stays alive,
    restarts reaping from the vm-state handler's resume branch on failure.

The stop happens in usb_host_cpr_quiesce(), which usb_host_cpr_vm_state()
calls on the FINISH_MIGRATE transition.  usb_host_pre_save() reaches it only
as a fallback for an already-paused VM.  Could the commit message be updated
to describe this?

[ ... ]

> @@ -1550,6 +1599,12 @@ static void usb_host_unrealize(USBDevice *udev)
> +    /*
> +     * An explicit device_del means we still own the device (a successful
> +     * cpr-transfer handoff exits the process instead), so clear the handoff
> +     * flag to force a full reset/release in usb_host_close().
> +     */
> +    s->cpr_handed_off = false;
>      usb_host_close(s);
>      migrate_del_blocker(&s->cpr_blocker);
>      if (s->cpr_vmse) {
>          qemu_del_vm_change_state_handler(s->cpr_vmse);
>      }

[Severity: Medium]
Does this path leave event reaping stopped for the other usb-host devices?

Suppose usb_host_cpr_quiesce() has called usb_host_stop_events() on the whole
ctx, and the migration then fails or is cancelled while the VM is paused.  No
running transition occurs, so usb_host_cpr_reissue() never calls
usb_host_start_events().

A later device_del reaches usb_host_unrealize(), which sets
cpr_handed_off = false and removes cpr_vmse without calling
usb_host_start_events().  usb_host_init() returns early once ctx exists, so
nothing else restarts reaping.

Can transfers on the other usb-host devices (hostdevice=, autoscan or
bus/addr) hang here, since the stop applies to the shared context?  The
commit message says only one hostdevice= device is supported, but nothing in
the code appears to enforce that.

[Severity: Low]
Can this unconditional clearing of cpr_handed_off cause a reset of a device
the target already owns?

The comment assumes that unrealize means the source still owns the device.
During the handoff window the source has cpr_handed_off = true and the target
may already be reaping.  If a device_del or guest eject reaches
usb_host_unrealize() then, usb_host_close() runs usb_host_abort_xfers(),
usb_host_release_interfaces(), libusb_reset_device() and
usb_host_attach_kernel() on the shared fd.

Is it intended that this disrupts the target's session?

[ ... ]

> @@ -2033,6 +2088,7 @@ static int usb_host_post_load(void *opaque, int version_id)
>      if (cpr_is_incoming()) {
> +        usb_host_start_events();
>          return 0;
>      }

This is the only restart point on the target, and it runs only for devices
whose vmstate section is loaded, as noted in the usb_host_realize() comment
above.

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

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

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

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

This patch sets a per-endpoint flag in usb_xhci_post_load() for running
isochronous endpoints when a CPR resume is incoming. The next isochronous
TD seen by xhci_submit() is then completed with CC_MISSED_SERVICE_ERROR
instead of being sent to the device. The aim is to make the guest driver
skip ahead and resync.

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

[ ... ]

> @@ -1047,6 +1049,11 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
>      ctx[0] &= ~EP_STATE_MASK;
>      ctx[0] |= state;
>  
> +    if (state != EP_RUNNING) {
> +        /* a stop/reset before the first kick ends the post-CPR resync window */
> +        epctx->cpr_iso_resync = false;
> +    }
> +

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

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

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

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

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

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

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

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

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

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

[ ... ]

> @@ -1811,6 +1818,19 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
>          xfer->pkts = 1;
>          xfer->iso_xfer = true;
>          xfer->timed_xfer = true;
> +        if (epctx->cpr_iso_resync) {
> +            /*
> +             * First iso TD after a CPR resume: our transfer state was
> +             * rebuilt from guest memory and no longer lines up with what
> +             * the guest consumed.  Report a missed service, like real HW,
> +             * to kick the driver's skip logic into resyncing.
> +             */
> +            epctx->cpr_iso_resync = false;
> +            xfer->status = CC_MISSED_SERVICE_ERROR;
> +            xhci_xfer_report(xfer);
> +            xfer->complete = 1;
> +            return 0;
> +        }

[Severity: Medium]
Is a fabricated CC_MISSED_SERVICE_ERROR the right way to handle this?

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

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

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

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

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

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

[ ... ]

> @@ -3510,6 +3530,11 @@ static int usb_xhci_post_load(void *opaque, int version_id)
>              xhci_init_epctx(epctx, pctx, ep_ctx);
>              epctx->state = state;
>              if (state == EP_RUNNING) {
> +                if (cpr_is_incoming() &&
> +                    (epctx->type == ET_ISO_IN || epctx->type == ET_ISO_OUT)) {
> +                    /* only iso streams need resyncing; see xhci_submit() */
> +                    epctx->cpr_iso_resync = true;
> +                }
>                  /* kick endpoint after vmload is finished */
>                  timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>              }

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

Re: [PATCH QEMU HCI-8.0 v7 14/14] 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-v7-0-044f668789a1@virtuozzo.com?part=14

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] usb-host: failed CPR post_load makes target exit notifier reset the shared device
- [Medium] usb-host: CPR reclaim claims all interfaces and sets configuration even if guest never configured
- [Medium] usb-host: CPR post_load dereferences NULL dev/dh when device was not opened
- [Low] usb-host: usb_host_ep_update() on CPR target resets endpoint halt state
- [Low] usb-host: reclaim comment claims no half-claimed state but claim loop can fail midway
--

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

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

This patch makes the CPR target call usb_host_ep_update() after vmload, so
endpoints for a non-zero altsetting are rebuilt. It also re-claims the
interfaces through libusb so libusb's view matches the preserved fd. The
new helper usb_host_cpr_reclaim_interfaces() validates the migrated
altsetting values before claiming.

> 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 rc;
> +
> +    rc = libusb_get_active_config_descriptor(s->dev, &conf);

[Severity: Medium]
Can s->dev (and later s->dh in libusb_claim_interface()) be NULL here?

usb_host_post_load() takes the CPR branch whenever cpr_is_incoming(), and
neither usb_host_cpr_reclaim_interfaces() nor usb_host_ep_update() checks
dev->dh or dev->dev first.

On the target, only the hostdevice= path opens the device in realize().
With autoscan (hostvendor/hostproduct/hostport), usb_host_auto_check()
only calls usb_host_open() under runstate_is_running().

If the target command line differs from the source's, a NULL pointer would
be passed to libusb.  This is somewhat speculative.  Would an
"if (!dev->dh)" guard make sense?

[ ... ]

> +    /* Data is sane: claim each interface on the fd and record it. */
> +    for (int i = 0; i < conf->bNumInterfaces; i++) {
> +        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
> +
> +        rc = libusb_claim_interface(s->dh, ifnum);
> +        if (rc != 0) {
> +            usb_host_libusb_error("libusb_claim_interface", rc);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +        s->ifs[ifnum].claimed = true;
> +    }
> +
> +    udev->ninterfaces = conf->bNumInterfaces;
> +    udev->configuration = conf->bConfigurationValue;

[Severity: Medium]
Does this claim every interface and set udev->configuration even when the
guest never configured the device on the source?

On the source, claims are only made in usb_host_claim_interfaces(), which
is reached from usb_host_set_config().  usb_host_open() only detaches
kernel drivers.

If the guest had not issued SET_CONFIGURATION at CPR time,
libusb_get_active_config_descriptor() can still succeed for a
kernel-autoconfigured device.  The target would then create claims the
source never made.  It would also set configuration and ninterfaces from
the host's active config, so the guest would see a configured device.

If migration fails and the source resumes, its ifs[].claimed flags and
configuration would disagree with the kernel state.

The commit message says the re-claim is "a no-op at the usbfs level
anyway".  Does that hold when the source never claimed the interfaces?  I
could not confirm that this case is reachable in practice.

> +    libusb_free_config_descriptor(conf);
> +
> +    return 0;
> +}

[Severity: Low]
The comment above this function and the one in the validation loop say
validation happens first so "garbage migration data can't leave it
half-claimed".  However, the claim loop can still fail partway through
libusb_claim_interface().

In that case the function returns -1 with the earlier interfaces claimed
and marked in s->ifs[].claimed, and nothing rolls that back.  Could the
comment be adjusted, or the claims be undone on failure?

>  static int usb_host_post_load(void *opaque, int version_id)
>  {
>      USBHostDevice *dev = opaque;
[ ... ]
>      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;
> +        }

[Severity: Medium]
Can this new failure return cause the target's exit notifier to reset the
shared device?

If usb_host_cpr_reclaim_interfaces() returns -1, the incoming migration
fails and the target process exits through usb_host_exit_notifier().  On
the target, s->dh is set and s->cpr_handed_off is false, because it is
only set by usb_host_cpr_quiesce() on the source:

usb_host_exit_notifier() {
    ...
    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);
    }
    ...
}

That would reset and release a device on an fd that the still-live source
expects to keep.  Any s->ifs[].claimed flags set by a partial reclaim also
feed usb_host_release_interfaces().

The notifier still gates this only on cpr_handed_off, and the later patch
"usb-host: make CPR migration blocker conditional" does not change that.
Should the target be marked as not owning the device when post_load fails?

> +        usb_host_ep_update(dev);

[Severity: Low]
Does usb_host_ep_update() here reset per-endpoint runtime state that the
guest changed on the source?

It calls usb_ep_reset() and then usb_ep_set_halted(udev, pid, ep, 0) for
every endpoint, and the vmstate carries only the altsetting.  A halted
endpoint would be un-halted without CLEAR_FEATURE(ENDPOINT_HALT), and
max_streams would be rebuilt in default form.

>          usb_host_start_events();
>          return 0;
>      }

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