* [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:31 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 02/14] usb-host: don't leak config descriptor on claim " Andrey Drobyshev
` (12 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:31 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:31 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: usb_host_open() fail path may double-close wrapped hostfd after libusb_close()
--
commit e4ff0c7f70381f2bead74b81a45f4480bcb0c76b
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: don't leak hostdev FD on open failure #VSTOR-137800
This patch stores the caller-supplied hostfd in s->hostfd before
libusb_wrap_sys_device() is called. It also closes that fd on the fail:
path of usb_host_open(), so a failed open no longer leaks it.
> 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
[ ... ]
> @@ -1066,6 +1070,10 @@ fail:
> s->dh = NULL;
> s->dev = NULL;
> }
> + if (s->hostfd != -1) {
> + close(s->hostfd);
> + s->hostfd = -1;
> + }
> return -1;
> }
[Severity: Low]
Can this close() be a double close of the wrapped fd?
Consider the path where usb_host_open() is called with dev == NULL,
libusb_wrap_sys_device() succeeds, and a later step such as
usb_device_attach() fails. Control reaches fail: and runs:
libusb_close(s->dh);
s->dh = NULL;
s->dev = NULL;
}
if (s->hostfd != -1) {
close(s->hostfd);
Because s->hostfd is now set before the wrap call, this close() runs
right after libusb_close(s->dh).
If the libusb version in use closes a wrapped fd in libusb_close(), the fd
is closed twice. The second close() could then hit an unrelated descriptor
if another thread reused the number in between.
I could not check this against libusb sources. Current Linux backends mark
wrapped fds with fd_keep and do not close them, but older releases such as
1.0.23 may behave differently.
usb_host_close() already uses the same libusb_close() then close() order,
so the new error path relies on the same assumption. Is that assumption
valid for all libusb versions that QEMU supports?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 02/14] usb-host: don't leak config descriptor on claim failure #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:29 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 03/14] usb-host: add migration blocker for CPR modes #VSTOR-137800 Andrey Drobyshev
` (11 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH QEMU HCI-8.0 v8 03/14] usb-host: add migration blocker for CPR modes #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800 Andrey Drobyshev
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 02/14] usb-host: don't leak config descriptor on claim " Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:30 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800 Andrey Drobyshev
` (10 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (2 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 03/14] usb-host: add migration blocker for CPR modes #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:38 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 05/14] usb-host: factor out usb_host_reap_xfers() #VSTOR-137800 Andrey Drobyshev
` (9 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:38 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:38 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: cpr_is_incoming() in post_load stays true for the whole process lifetime
--
commit 09775f4cee7af08e9fba2832c78f5e0afc56f940
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: preserve hostdev FD during CPR migration #VSTOR-137800
This patch saves the host device FD in the CPR registry at realize time
and reuses it on the CPR target. It also deletes the FD from the registry
in the usb_host_open() failure path and in usb_host_close(). Finally, it
makes usb_host_post_load() skip the detach/rescan bottom half when CPR is
incoming.
> 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
[ ... ]
> @@ -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;
> + }
> +
[Severity: Low]
Does cpr_is_incoming() stay true for the whole lifetime of a CPR target
process, rather than only during the CPR vmstate load?
cpr_is_incoming() is defined in migration/cpr.c as:
return incoming_mode != MIG_MODE_NONE;
incoming_mode is set via cpr_set_incoming_mode() in cpr_state_load(), and
nothing in migration/cpr.c appears to reset it to MIG_MODE_NONE afterwards
(checked at the end of the series).
If a later, non-CPR vmstate load of a usb-host device happens in the same
process (for example loadvm of a snapshot), usb_host_post_load() would
return early. The detach/rescan emulation in usb_host_post_load_bh() would
then be skipped, and in the final series state the CPR reclaim path would
run instead.
This is speculative, since it depends on a snapshot or other vmstate load
occurring in a CPR target process, and I did not confirm that this is a
supported flow. Is there a way to restrict this check to the CPR load
itself?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 05/14] usb-host: factor out usb_host_reap_xfers() #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (3 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 04/14] usb-host: preserve hostdev FD during CPR migration #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:31 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800 Andrey Drobyshev
` (8 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (4 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 05/14] usb-host: factor out usb_host_reap_xfers() #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:38 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800 Andrey Drobyshev
` (7 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 | 249 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 222 insertions(+), 27 deletions(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 1a79cdf9e4d..413ddc12ee4 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,41 @@ 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_drained) {
+ /* no CPR drain in progress: normal completion */
+ return false;
+ }
+ 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;
+ if (!r->in) {
+ /*
+ * complete_data() only trims the length for IN; for OUT
+ * report what the device actually consumed rather than
+ * the full submit-time size.
+ */
+ r->p->actual_length = xfer->actual_length;
+ }
+ return false;
+ }
+ }
+ return true;
+}
+
static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer *xfer)
{
USBHostRequest *r = xfer->user_data;
@@ -432,6 +474,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 +527,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 +550,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 +1153,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 +1174,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 +1193,118 @@ 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.
+ * Reaping an iso IN URB wakes the endpoint, which can make xHCI submit
+ * fresh iso URBs mid-drain, so re-cancel here on every iteration too.
+ *
+ * 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 };
+
+ QTAILQ_FOREACH(ring, &s->isorings, next) {
+ QTAILQ_FOREACH(xfer, &ring->inflight, next) {
+ libusb_cancel_transfer(xfer->xfer);
+ }
+ }
+ 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 +1472,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 +1496,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 +1968,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 +1984,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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:38 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:38 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb-host: cpr_replay_all drops already-completed URBs, losing IN data
- [Low] usb-host: drain timeout path drops newly queued URBs without cancelling them
--
commit 5a6ed65d6723a2887c0ae5c9eb6d765f5c531174
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 a CPR switchover. It also adds
a pre_save fallback for VMs that were already paused, and cancels
isochronous URBs on their own rings.
> 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 bool usb_host_req_cpr_replay(USBHostRequest *r,
> + struct libusb_transfer *xfer)
> +{
> + if (!r->host->cpr_drained) {
> + /* no CPR drain in progress: normal completion */
> + return false;
> + }
> + 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;
> + if (!r->in) {
> + r->p->actual_length = xfer->actual_length;
> + }
> + return false;
> + }
> + }
> + return true;
> +}
[Severity: Medium]
When cpr_replay_all is set, the status and actual_length checks are
skipped and usb_host_req_cpr_replay() returns true for every reaped URB.
Does this also discard URBs that the kernel already completed with
LIBUSB_TRANSFER_COMPLETED and actual_length > 0?
For an IN endpoint, the data the device already returned would be dropped.
The target then replays the TD from its start, so that data appears to be
lost. A completed OUT or control request could be sent a second time.
The commit message says "A transfer that already completed in the kernel is
delivered normally". That only holds when cpr_replay_all is false. The
message also says the fallback suppresses every URB, so the two statements
seem to disagree. Could the commit message be made consistent with the code,
or could completed URBs be delivered in the replay_all case too?
The path is: the VM is already paused when a CPR migration starts, so the
FINISH_MIGRATE handler never runs. usb_host_pre_save() then sets
cpr_replay_all and calls usb_host_cpr_quiesce(). A URB that completed in
the kernel but has not been reaped yet then goes through
usb_host_req_complete_data() or usb_host_req_complete_ctrl() and is
suppressed.
Later patches in the series do not change this. The final
usb_host_req_cpr_replay() still returns true for every reap when
cpr_replay_all is set. The later patch "usb-host: re-issue drained URBs if
CPR is aborted" only adds replay for an aborted migration. Its own comment
says the paused-VM fallback queues partially transferred OUT URBs and may
send them twice.
[ ... ]
> +static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> +{
[ ... ]
> + 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;
> + }
> + }
[Severity: Low]
On the 100-iteration timeout, can this path leave URBs live that were never
cancelled? usb_host_req_complete_err() calls usb_packet_complete(), which
can start the next queued packet:
usb_host_cpr_drain_xfers()
usb_host_req_complete_err()
usb_packet_complete()
usb_process_one()
usb_host_handle_data()
usb_host_req_alloc() /* new request appended to s->requests */
usb_host_drop_requests() then unlinks every request on s->requests,
including the newly queued ones. Those were never passed to
libusb_cancel_transfer(), and their packets stay in USB_PACKET_ASYNC.
The URBs would stay live in the kernel on the preserved fd, which is what
this patch is meant to prevent. Their late completion callbacks would run
with r->host still set, so they could touch stale state if the device is
closed or unrealized first.
This is an error path on a migration that is already failing. It mirrors
the existing leak in usb_host_reap_xfers().
Later patches, including "usb-host: re-issue drained URBs if CPR is
aborted", do not change this timeout path. The final code still calls
usb_host_req_complete_err() followed by usb_host_drop_requests().
[ ... ]
> + limit = 2 * s->iso_urb_count * s->iso_urb_frames;
> + while (usb_host_iso_inflight(s)) {
> + struct timeval tv = { .tv_usec = 1000 };
> +
> + QTAILQ_FOREACH(ring, &s->isorings, next) {
> + QTAILQ_FOREACH(xfer, &ring->inflight, next) {
> + libusb_cancel_transfer(xfer->xfer);
> + }
> + }
> + libusb_handle_events_timeout(ctx, &tv);
> + if (limit-- == 0) {
> + return -1;
> + }
> + }
[Severity: Low]
Does the iso timeout path also return with iso URBs still in flight and the
rings still allocated? On limit-- == 0, usb_host_cpr_drain_xfers() returns
-1 before usb_host_iso_free_all(). The cancelled iso URBs may remain on
ring->inflight.
[ ... ]
> + if (usb_host_cpr_quiesce(s, mode) < 0) {
> + migration_file_set_error(-EIO, NULL);
> + }
[Severity: Low]
In usb_host_cpr_vm_state(), the drain failure is reported with
migration_file_set_error(-EIO, NULL). Since the Error is NULL, no
diagnostic reaches the user. Could an Error describing the usb-host drain
failure be passed instead?
[ ... ]
> + 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;
> + }
> + }
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=6
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (5 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 06/14] usb-host: drain in-flight URBs across CPR #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:44 ` no-reply
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800 Andrey Drobyshev
` (6 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 413ddc12ee4..9cc7d72aa63 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)
@@ -462,6 +465,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
return false;
}
}
+ if (r->p->state == USB_PACKET_ASYNC) {
+ r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
+ }
return true;
}
@@ -1284,16 +1290,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();
@@ -1499,6 +1563,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)
@@ -1506,6 +1572,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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:44 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:44 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: paused-VM CPR drain replays fully completed URBs, not just partial OUTs
--
commit 9334effcb0d32b4173bab08303e8d2fef677d756
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800
This commit queues the packets suppressed during the CPR drain on a new
cpr_inflight list. If CPR is aborted, the vm-state handler's resume branch
re-issues them on the preserved fd. Control transfers are completed with
USB_RET_IOERROR instead.
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 413ddc12ee43..9cc7d72aa63a 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
[ ... ]
> @@ -433,9 +435,10 @@ static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
> /*
> * Classify a reap during the CPR drain (r->p is still set). The paused-VM
> * fallback (cpr_replay_all) can't deliver completions, so it suppresses every
> - * reap for the target to replay; otherwise one that moved no data is suppressed
> - * and one that moved some is kept as a short transfer. Returns true if
> - * suppressed, so the caller frees the request.
> + * reap; otherwise one that moved no data is suppressed and one that moved some
> + * is kept as a short transfer. Suppressed packets are queued on cpr_inflight
> + * for the target to replay. Returns true if suppressed, so the caller frees
> + * the request.
> */
> static bool usb_host_req_cpr_replay(USBHostRequest *r,
> struct libusb_transfer *xfer)
> @@ -462,6 +465,9 @@ static bool usb_host_req_cpr_replay(USBHostRequest *r,
> return false;
> }
> }
> + if (r->p->state == USB_PACKET_ASYNC) {
> + r->host->cpr_inflight = g_slist_append(r->host->cpr_inflight, r->p);
> + }
> return true;
> }
[Severity: Medium]
When cpr_replay_all is set by usb_host_pre_save() (paused-VM fallback), the
whole "if (!r->host->cpr_replay_all) { ... }" block is skipped. Control
then reaches the g_slist_append() for every reaped URB, whatever
xfer->status or xfer->actual_length is.
That includes URBs that finished with LIBUSB_TRANSFER_COMPLETED.
> [ ... ]
>
> +static void usb_host_cpr_reissue(USBHostDevice *s)
> +{
[ ... ]
> + for (it = s->cpr_inflight; it; it = it->next) {
> + p = it->data;
> + /*
> + * Replay from the start of the TD. A transfer that already moved some
> + * data is re-run in full, so an OUT fragment the device consumed may be
> + * sent twice. Only the paused-VM pre_save fallback queues such partial
> + * transfers here; the normal drain delivers them as short transfers.
> + */
> + p->actual_length = 0;
> + if (p->ep->nr == 0) {
> + /* control endpoint: can't be reconstructed, so error it out */
> + p->status = USB_RET_IOERROR;
> + usb_generic_async_ctrl_complete(udev, p);
> + } else {
> + usb_host_handle_data(udev, p);
Is the comment here, and the commit message, accurate about which
transfers get replayed?
The commit message says "an OUT fragment already consumed by the device may
be sent twice. Only the paused-VM .pre_save() fallback queues partial
transfers for re-issue". The code comment makes the same claim.
The path is usb_host_pre_save() setting cpr_replay_all = true, then
usb_host_cpr_quiesce(), then usb_host_req_cpr_replay() queuing every reap.
In that path fully completed URBs are queued as well, not only partial
ones.
On resume after an aborted CPR, usb_host_cpr_reissue() then handles them
as follows:
- A completed OUT transfer is sent to the device a second time.
- A completed IN transfer is resubmitted with actual_length reset to 0,
so the data already read is lost and the device will not resend it.
- A completed control transfer, for example SET_CONFIGURATION or another
SET_* request, is reported to the guest as USB_RET_IOERROR through
usb_generic_async_ctrl_complete().
Should the commit message and the comment describe this wider scope? Or
should usb_host_req_cpr_replay() deliver URBs with xfer->status ==
LIBUSB_TRANSFER_COMPLETED normally, even in cpr_replay_all mode, instead
of queueing them for replay?
> [ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=7
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (6 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 07/14] usb-host: re-issue drained URBs if CPR is aborted #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:27 ` Andrey Drobyshev
2026-10-06 16:41 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 09/14] usb-host: factor out usb_host_start_events() #VSTOR-137800 Andrey Drobyshev
` (5 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:27 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 9cc7d72aa63..6c00ce528b1 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1123,7 +1123,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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:41 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:41 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] usb-host: CPR-incoming workaround skips only one blocking call, with no root cause
- [Low] usb-host: cpr_is_incoming() is a global check, so unrelated opens lose product name
--
commit 49b39041283392e4d6c44ac4bd1c34c893215179
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: skip product-string read on CPR incoming #VSTOR-137800
This patch skips the libusb_get_string_descriptor_ascii() call in
usb_host_open() when cpr_is_incoming() is true. In that case product_desc
falls back to the synthetic "host:bus.addr" name. The stated reason is that
the synchronous control transfer can block during CPR handover and hang the
incoming migration.
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 9cc7d72aa63a..6c00ce528b1e 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -1123,7 +1123,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));
[Severity: Low]
Is skipping only this one transfer enough to avoid the hang? The commit
message says the transfer "can block there, hanging the incoming
migration", but no root cause is given.
If the cause is the device handover state, other calls in usb_host_open()
during CPR incoming could block in the same way:
usb_host_open()
libusb_wrap_sys_device() / libusb_open()
usb_host_detach_kernel()
libusb_get_device_descriptor()
usb_host_speed_compat()
libusb_get_config_descriptor()
ioctl(hostfd, USBDEVFS_GET_SPEED, NULL)
usb_device_attach()
Could the root cause be described, and a comment added at this guard
explaining why only the product string read is skipped?
Also, nothing appears to re-read the real product string after CPR
completes. Does product_desc then stay as "host:bus.addr" where the source
showed the real name?
[Severity: Low]
This isn't a functional bug, but cpr_is_incoming() is a process-global check
(incoming_mode != MIG_MODE_NONE), not a per-device indicator that the device
was inherited via CPR.
incoming_mode is cleared only by cpr_set_incoming_mode(MIG_MODE_NONE) in
migration_incoming_state_destroy(). So the window is wider than the device
handover.
Can this also affect other opens during that window? For example,
usb_host_open() reached from usb_host_auto_check() or from the realize
paths would skip the product read for a device that was not handed over by
CPR. Such a device would get the synthetic "host:bus.addr" product_desc,
which is visible in info usb, in log messages and in traces.
The commit message does not mention this wider effect. Would a per-device
condition be more appropriate?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=8
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 09/14] usb-host: factor out usb_host_start_events() #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (7 preceding siblings ...)
2026-10-06 16:27 ` [PATCH QEMU HCI-8.0 v8 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:41 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800 Andrey Drobyshev
` (4 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 6c00ce528b1..70ae0a38167 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (8 preceding siblings ...)
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 09/14] usb-host: factor out usb_host_start_events() #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:50 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 11/14] usb: migrate the interface altsetting #VSTOR-137800 Andrey Drobyshev
` (3 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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().
Until then it must not touch the device at all: if the incoming
migration fails, the target exits while the source resumes, so the
target's exit must not reset the device or release the interfaces
underneath the source.
* The source stops reaping when it quiesces the device on the
stop-for-CPR transition (or in the .pre_save() fallback for an
already-paused VM) 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 | 91 +++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 79 insertions(+), 12 deletions(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 70ae0a38167..1412f92c12e 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)
@@ -1285,13 +1306,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;
}
@@ -1310,6 +1336,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;
/*
@@ -1384,16 +1415,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;
@@ -1428,10 +1462,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);
}
}
@@ -1500,6 +1541,18 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
error_setg(errp, "failed to open host usb device %s", s->hostdevice);
return;
}
+ if (cpr_get_incoming_mode() == MIG_MODE_CPR_TRANSFER) {
+ /*
+ * 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, and don't reset or release the device on an early
+ * exit either -- if the incoming migration fails, the source
+ * resumes with it. .post_load() restarts events and takes
+ * ownership once the source has handed the fd off.
+ */
+ usb_host_stop_events();
+ s->cpr_handed_off = true;
+ }
} else
#endif
if (s->match.addr && s->match.bus_num &&
@@ -1563,6 +1616,17 @@ 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): restart the reaping
+ * stopped by the handoff and clear the flag to force a full
+ * reset/release in usb_host_close(). Except on a CPR target that
+ * hasn't taken over yet -- there the source still drives the fd.
+ */
+ if (s->cpr_handed_off && !cpr_is_incoming()) {
+ usb_host_start_events();
+ s->cpr_handed_off = false;
+ }
usb_host_close(s);
migrate_del_blocker(&s->cpr_blocker);
if (s->cpr_vmse) {
@@ -2046,6 +2110,9 @@ 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();
+ /* the takeover is complete: from here on this process owns the fd */
+ dev->cpr_handed_off = false;
return 0;
}
--
2.47.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:50 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:50 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: cpr-transfer target takes fd ownership in post_load() before migration commits
- [Medium] usb-host: target usb_host_open() fail path resets shared device before cpr_handed_off is set
- [Low] usb-host: cpr_handed_off left set on source after failed cpr-transfer with VM paused
- [Low] usb-host: cpr_handed_off guard in usb_host_close() also skips logical detach and cleanup
--
commit 642a3db85ee636f9e50d1ff27121f39b2c1b9026
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: hand off the device across cpr-transfer #VSTOR-137800
This patch hands reaping of the shared usbfs fd from the cpr-transfer source to the target. The target stops events in realize() and restarts them in post_load(). The source stops events when it quiesces and restarts them on resume. On success the source's exit path leaves the device untouched.
> 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
[ ... ]
> @@ -1285,13 +1306,18 @@ static int usb_host_cpr_drain_xfers(USBHostDevice *s)
> 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: Low]
cpr_handed_off is set here before the outcome of the handoff is known. It
is cleared only on VM resume (usb_host_cpr_reissue()) or device_del
(usb_host_unrealize()). The same applies to the usb_host_pre_save()
fallback for an already-paused VM, which also calls usb_host_cpr_quiesce().
Suppose the cpr-transfer migration fails or is cancelled and the VM stays
paused. The resume branch never runs, so the flag stays set and reaping
stays stopped for the whole libusb context.
A later QEMU quit then reaches 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);
}
libusb_close(s->dh);
Can this path tell a failed handoff from a successful one? As written, it
skips the reset and the host driver rebind even though no handoff happened.
[ ... ]
> @@ -1310,6 +1336,11 @@ static void usb_host_cpr_reissue(USBHostDevice *s)
> return;
> }
>
> + if (s->cpr_handed_off) {
> + usb_host_start_events();
> + s->cpr_handed_off = false;
> + }
[Severity: Medium]
Following the failed-migration path, the source resumes and restarts
reaping here. If the target already called usb_host_start_events() in
usb_host_post_load(), both processes now reap URBs on the shared fd. See
the comment on usb_host_post_load() below.
> @@ -1384,16 +1415,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);
> }
[Severity: Low]
This guard covers more than the physical reset, release and rebind. It
also skips usb_host_abort_xfers(), usb_host_iso_free_all() and
usb_device_detach(). Should only the physical operations be gated?
Two callers can reach this with the flag set:
usb_host_realize()
usb_host_open() -> usb_device_attach()
migrate_add_blocker_modes() fails
usb_host_close() -> usb_device_detach() skipped
On the target, the attach done by usb_host_open() is never paired with a
detach. usb_qdev_realize() only calls usb_release_port() on failure, so
the port is left with a stale attach.
usb_host_nodev_bh()
usb_host_close()
If a surprise removal occurs between the source's quiesce and resume, the
handle is closed without detaching the device from the guest. The guest
then sees NODEV errors instead of a detach, while s->dh is NULL.
[ ... ]
> @@ -1500,6 +1541,18 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
> error_setg(errp, "failed to open host usb device %s", s->hostdevice);
> return;
> }
> + if (cpr_get_incoming_mode() == MIG_MODE_CPR_TRANSFER) {
> + [ ... ]
> + usb_host_stop_events();
> + s->cpr_handed_off = true;
> + }
[Severity: Medium]
The flag and the event stop are applied only after usb_host_open() has
succeeded. Can the failure path inside usb_host_open() reset the device
under the live source?
usb_host_realize()
cpr_open_fd()
usb_host_open(s, NULL, fd)
...
fail:
usb_host_release_interfaces(s);
libusb_reset_device(s->dh);
usb_host_attach_kernel(s);
libusb_close(s->dh);
usb_host_open() does real work on the shared fd:
libusb_wrap_sys_device(), usb_host_detach_kernel(), usb_host_ep_update()
and usb_device_attach(). If a later step fails, for example
usb_device_attach() rejecting a speed mismatch, the fail: label runs
unconditionally and is not gated by cpr_handed_off. This conflicts with
the commit message statement that the target "must not touch the device at
all" until it takes over.
[ ... ]
> @@ -1563,6 +1616,17 @@ static void usb_host_unrealize(USBDevice *udev)
> [ ... ]
> + if (s->cpr_handed_off && !cpr_is_incoming()) {
> + usb_host_start_events();
> + s->cpr_handed_off = false;
> + }
> usb_host_close(s);
[Severity: Medium]
migration_incoming_state_destroy() calls cpr_set_incoming_mode(MIG_MODE_NONE),
so cpr_is_incoming() becomes false once an incoming migration has failed.
Consider a target that survives the failure, for example with exit-on-error
off, and still has cpr_handed_off == true. A device_del then takes the "we
own it" branch. It restarts events and runs the full reset and release
while the source has resumed with the device. Is cpr_is_incoming() a
reliable test for "the source still drives the fd" here?
[ ... ]
> @@ -2046,6 +2110,9 @@ static int usb_host_post_load(void *opaque, int version_id)
> */
> if (cpr_is_incoming()) {
> + usb_host_start_events();
> + /* the takeover is complete: from here on this process owns the fd */
> + dev->cpr_handed_off = false;
> return 0;
> }
[Severity: Medium]
Ownership is taken here when this device's vmstate section is loaded, not
when the whole migration has succeeded. Later device sections, the end of
the stream and the source-side completion all come after this point.
If the migration fails after this section loads, for example from a later
device's load error, a broken stream or a cancel, the source resumes. Then
usb_host_cpr_reissue() restarts its own reaping:
target: usb_host_post_load()
usb_host_start_events();
dev->cpr_handed_off = false;
source: usb_host_cpr_vm_state() -> usb_host_cpr_reissue()
usb_host_start_events();
Both processes then reap URBs on the same usbfs fd. This is the situation
the commit message says must be avoided, since one process can reap the
other's in-flight isochronous completions.
When the target then exits, usb_host_exit_notifier() sees
cpr_handed_off == false and runs:
usb_host_abort_xfers(s);
usb_host_release_interfaces(s);
libusb_reset_device(s->dh);
usb_host_attach_kernel(s);
This resets the device and rebinds the host driver under the resumed
source, which the commit message says the target's exit must not do.
Could the takeover be deferred until the migration has committed, so a
late failure leaves the target's flag set and its events stopped?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=10
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 11/14] usb: migrate the interface altsetting #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (9 preceding siblings ...)
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 10/14] usb-host: hand off the device across cpr-transfer #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:46 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800 Andrey Drobyshev
` (2 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (10 preceding siblings ...)
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 11/14] usb: migrate the interface altsetting #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:52 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800 Andrey Drobyshev
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 14/14] usb-host: make CPR migration blocker conditional #VSTOR-137800 Andrey Drobyshev
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 1412f92c12e..076ba08533b 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -988,7 +988,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];
}
@@ -2101,6 +2101,75 @@ 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;
+
+ if (!s->dh) {
+ /* e.g. a target whose command line doesn't open the device */
+ error_report("usb-host: CPR target has no open host device");
+ return -1;
+ }
+
+ 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;
@@ -2110,6 +2179,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();
/* the takeover is complete: from here on this process owns the fd */
dev->cpr_handed_off = false;
--
2.47.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:52 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:52 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev, den
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: CPR reclaim claims interfaces and sets configuration for guests that never configured the device
--
commit 4d875277381be99a72691b7786690c4fc8a9ce77
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800
On CPR incoming, this patch rebuilds the libusb endpoints with
usb_host_ep_update(). It also adds usb_host_cpr_reclaim_interfaces(), which
re-claims interfaces so that libusb's handle matches the preserved fd.
> 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)
> +{
[ ... ]
> + /* 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: Low]
Can this claim interfaces and set udev->configuration for a guest that never
configured the device?
In usb_host_open(), interfaces are not claimed, and udev->ninterfaces and
udev->configuration are not set. They are only set through
usb_host_set_config()->usb_host_claim_interfaces() when the guest issues
SET_CONFIGURATION.
usb_host_cpr_reclaim_interfaces() instead derives its work from
libusb_get_active_config_descriptor(), which reflects the host kernel's
active configuration. It then stores:
udev->ninterfaces = conf->bNumInterfaces;
udev->configuration = conf->bConfigurationValue;
Nothing from the migrated state is consulted, such as the source's
s->ifs[].claimed. If the guest left the device in the address state while the
kernel's active configuration is non-zero, the target could claim interfaces
the source never claimed. It could also report a non-zero configuration that
the guest never selected.
So the commit message statement "on the preserved FD this is a no-op at the
usbfs level anyway" seems to hold only when the guest had already configured
the device. I did not trace the guest-side GET_CONFIGURATION handling.
Separately, if libusb_claim_interface() fails partway through the loop, for
example with EBUSY, the function returns -1 after earlier interfaces have
already been claimed and marked in s->ifs[ifnum].claimed:
rc = libusb_claim_interface(s->dh, ifnum);
if (rc != 0) {
...
return -1;
}
s->ifs[ifnum].claimed = true;
There is no rollback. This seems to contradict the comment above the
validation loop, "so garbage migration data can't leave it half-claimed".
Would it make sense to release the earlier claims on this error path?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=12
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (11 preceding siblings ...)
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 12/14] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:53 ` no-reply
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 14/14] usb-host: make CPR migration blocker conditional #VSTOR-137800 Andrey Drobyshev
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:53 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:53 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb-xhci: cpr_iso_resync flag is not consumed by an empty-ring kick and drops a later valid TD
- [Medium] usb-xhci: CPR iso resync relies on a fabricated CC_MISSED_SERVICE_ERROR that only Linux guests may handle
--
commit b3cbde381fc3538ebb617db181b3540f42917e1d
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 CPR is incoming. The first isochronous TD
that reaches 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 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;
> + }
> +
[ ... ]
> @@ -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]
Can a later, correctly ordered isochronous TD be dropped here?
The comment in xhci_set_ep_state() says the resync window ends at the
first kick. The code ends it only at the first isochronous submit or at
the first non-running state change.
The post-load kick runs xhci_kick_epctx(). That function can return
without reaching xhci_submit():
- it returns on !xhci_slot_ok()
- it returns on !ring->dequeue
- it breaks out of the loop when xhci_ring_chain_length() <= 0,
which for ET_ISO_OUT/ET_ISO_IN only reports CC_RING_OVERRUN or
CC_RING_UNDERRUN
xhci_kick_epctx() then calls xhci_set_ep_state(..., EP_RUNNING). The new
clearing code only fires for state != EP_RUNNING, so cpr_iso_resync
stays set.
The next isochronous TD the guest queues may arrive long after resume
and be in sync. It would hit this block in xhci_submit(), never be sent
to the device, and the guest would get a CC_MISSED_SERVICE_ERROR. For
iso OUT this loses one chunk of data and makes the guest run its skip
logic needlessly.
Should cpr_iso_resync be cleared when the post-load kick finishes,
rather than only on the first iso submit?
[Severity: Medium]
Does this approach depend on guest-specific handling of
CC_MISSED_SERVICE_ERROR?
The commit message says the error "arms the guest driver's skip logic".
That matches Linux xhci-hcd behaviour (ep->skip). Nothing in the patch
shows how other guests would handle this fabricated event. If a guest
treats it as a hard error or ignores it, the endpoint could stay wedged,
and the first TD's data is dropped anyway.
> @@ -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;
> + }
Also, usb_xhci_post_load() rebuilds the same transient state, including
mfindex_last via xhci_init_epctx(), for ordinary incoming migration.
Only the cpr_is_incoming() case gets the resync here.
Is the difference between CPR and regular incoming migration
intentional? If so, could the commit message explain it?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=13
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH QEMU HCI-8.0 v8 14/14] usb-host: make CPR migration blocker conditional #VSTOR-137800
2026-10-06 16:27 [PATCH QEMU HCI-8.0 v8 00/14] usb-host: support CPR migration Andrey Drobyshev
` (12 preceding siblings ...)
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 13/14] hcd-xhci: resync isochronous endpoints after CPR #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:28 ` Andrey Drobyshev
2026-10-06 16:53 ` no-reply
13 siblings, 1 reply; 29+ messages in thread
From: Andrey Drobyshev @ 2026-10-06 16:28 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
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 076ba08533b..f88e6cde611 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1580,16 +1580,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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH QEMU HCI-8.0 v8 14/14] usb-host: make CPR migration blocker conditional #VSTOR-137800
2026-10-06 16:28 ` [PATCH QEMU HCI-8.0 v8 14/14] usb-host: make CPR migration blocker conditional #VSTOR-137800 Andrey Drobyshev
@ 2026-10-06 16:53 ` no-reply
0 siblings, 0 replies; 29+ messages in thread
From: no-reply @ 2026-10-06 16:53 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: den, 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>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=14
^ permalink raw reply [flat|nested] 29+ messages in thread