[PATCH QEMU HCI-8.1 v6 00/34] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860

[PATCH QEMU HCI-8.1 v6 00/34] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860

[Andrey Drobyshev]

v5 -> v6:

mdev vGPUs are no longer supported, only SR-IOV vGPUs and whole GPUs
(#VSTOR-150598 and #VSTOR-150595 closed).

- Patch 34 ("VZ: vfio/container: lift the CPR blocker for mdev devices")
  dropped: it only let cpr-exec through for mdev devices

- Patch 35 ("VZ: migration: keep the estimate and wait over starved
  windows") dropped: it only mattered for a device without pre-copy;
  with pre-copy the sender isn't starved before the switchover

- Patch 36 -> 34 ("VZ: vfio/migration: don't wait for stop-copy-only
  device state"): reduced to hw/vfio/migration.c: the stop-copy size is
  reported as pending only while the device still has something to
  pre-copy, so once it has drained the rest decides the switchover on
  its own; the savevm and migration.c changes are gone.  Tagged
  #VSTOR-141821 instead of the closed #VSTOR-150595

Fixes of the issues found by Sashiko AI review.

- Patch 27 ("vfio/listener: skip dirty page tracking during CPR")
  replaced by "migration/ram: don't start dirty logging in CPR modes":
  no RAM is migrated in a CPR mode, so the dirty log isn't started at
  all, instead of VFIO alone skipping it; that also closed the gap
  where VFIO tracking stayed off with the log left on by another user

- Patch 34 ("VZ: vfio/migration: don't wait for stop-copy-only device
  state"): a failed pre-copy size query no longer passes for a drained
  device

v5: http://lore.virtuozzo.com/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com
---
Akihiko Odaki (1):
      vfio/pci: Grow buffer in vfio_pci_host_match()

Andrey Drobyshev (20):
      VZ: migration/cpr-exec: fix Error leak in preserve_ram()
      migration/cpr-exec: restore RAM write access if exec fails
      vfio/region: create BAR dma-bufs with O_CLOEXEC
      migration/cpr-exec: require a file or exec migration URI
      migration: close the CPR state if the outgoing channel fails to open
      migration/cpr-exec: close the persisted state if the channel fails to open
      migration: don't refuse a blocker over a migration of another mode
      kvm: tolerate ioctls after kvm_close()
      kvm: fix coalesced MMIO flush racing with kvm_close()
      migration/cpr: add cpr_is_in_progress()
      vfio/migration: don't migrate device state during CPR
      vfio/migration: don't assume the device is running on CPR incoming
      migration/ram: don't start dirty logging in CPR modes
      VZ: vfio/cpr: don't close KVM on cpr-exec
      VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
      VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
      VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released
      VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
      VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
      VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

Arun Menon (1):
      migration: Fix regression of passing error_fatal into vmstate_load_state()

Cédric Le Goater (2):
      vfio/pci: Reject invalid MSI-X Table and PBA BIR values
      vfio/pci: Fix information leak in vfio_rom_read()

Fabiano Rosas (3):
      migration: yank: Move register instance earlier
      migration: Free cpr-transfer MigrationAddress along with gsource
      migration: Move CPR HUP watch to cpr-transfer.c

Igor Mammedov (1):
      q35: Fix migration of SMRAM state

Mario Casquero (1):
      vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

Markus Armbruster (1):
      migration: Plug memory leaks after migrate_set_error()

Peter Xu (1):
      migration: Fix crash on second migration when cancel early

Tycho Andersen (AMD) (1):
      hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

Vladimir Sementsov-Ogievskiy (1):
      migration: vmstate_save_state_v: fix double error_setg

Yanfei Xu (1):
      migration: ensure APIC is loaded prior to VFIO PCI devices

 accel/kvm/kvm-all.c           |  24 +++++++-
 docs/devel/migration/CPR.rst  |   8 +--
 hw/display/virtio-gpu.c       |  22 +++++---
 hw/intc/apic_common.c         |   1 +
 hw/net/vhost_net.c            |  62 ++++++++++++++++++++-
 hw/pci-host/q35.c             |  25 ++++-----
 hw/pci/pci.c                  |  15 ++++-
 hw/s390x/virtio-ccw.c         |  17 +++++-
 hw/scsi/spapr_vscsi.c         |  10 +++-
 hw/vfio/cpr.c                 |  11 ++--
 hw/vfio/device.c              |  29 +++++++---
 hw/vfio/migration-multifd.c   |   2 +
 hw/vfio/migration.c           |  64 +++++++++++++++++++--
 hw/vfio/pci.c                 |  25 ++++++---
 hw/vfio/region.c              |  51 ++++++++++++++++-
 hw/virtio/virtio-mmio.c       |  15 ++++-
 hw/virtio/virtio-pci.c        |  15 ++++-
 hw/virtio/virtio.c            |  10 +++-
 include/hw/vfio/vfio-cpr.h    |   1 +
 include/hw/vfio/vfio-device.h |   2 -
 include/hw/virtio/vhost.h     |   2 +
 include/migration/cpr.h       |   7 +++
 include/migration/vmstate.h   |   1 +
 include/net/vhost_net.h       |   3 +-
 migration/cpr-exec.c          |  44 +++++++++++++--
 migration/cpr-transfer.c      |  33 +++++++++++
 migration/cpr.c               |  22 ++++++++
 migration/migration.c         | 126 ++++++++++++++++++++++++------------------
 migration/migration.h         |   5 ++
 migration/multifd.c           |   2 +
 migration/ram.c               |   7 ++-
 migration/vmstate.c           |   7 ++-
 tests/qtest/q35-test.c        |   6 --
 33 files changed, 534 insertions(+), 140 deletions(-)
---
base-commit: 02f55118bcb0533ce4c6544458c30803d96bea4a
change-id: 20261009-vfio_v1-a176cddd6b64

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

[PATCH QEMU HCI-8.1 v6 01/34] VZ: migration/cpr-exec: fix Error leak in preserve_ram()

[Andrey Drobyshev]

migrate_set_error() stores its own copy of the error, so free ours.

Fixes: a3745506daf9 ("VZ: migration/cpr-exec: mark saved RAM blocks with MADV_DOEXEC before execve")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index edace3f0bd8..9fd3515753a 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -148,6 +148,7 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
         error_report_err(error_copy(err));
         migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
         migrate_set_error(s, err);
+        error_free(err);
 
         return false;
     }

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 02/34] migration/cpr-exec: restore RAM write access if exec fails

[Andrey Drobyshev]

With -cpr-ram-rdonly the RAM blocks are made read-only right before
execve().  If execve() fails, the VM resumes in the old QEMU with
read-only RAM and crashes on the first write.  Make it writable again.

Fixes: 91da25f20471 ("migration/cpr-exec: add '-cpr-ram-rdonly' option to mark RAM blocks as RO #VSTOR-101579")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 9fd3515753a..c4bdf2cb99f 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -224,6 +224,9 @@ static void cpr_exec_cb(void *opaque)
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
     g_clear_pointer(&argv, g_strfreev);
     cpr_exec_unpreserve_fds();
+    if (cpr_ram_rdonly) {
+        cpr_ram_restore_rw();
+    }
 
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 03/34] vfio/region: create BAR dma-bufs with O_CLOEXEC

[Andrey Drobyshev]

The dma-buf fds are created without O_CLOEXEC, so they leak into any
exec'ed program.  With cpr-exec, every update leaks one fd per mmapped
BAR, as new QEMU creates its own.

Fixes: 96163e0dbcb4 ("hw/vfio/region: Create dmabuf for PCI BAR per region")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/region.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 1deaef12fd0..b0332f27fcd 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -258,7 +258,7 @@ static bool vfio_region_create_dma_buf(VFIORegion *region, Error **errp)
     dma_buf = (void *)feature->data;
     *dma_buf = (struct vfio_device_feature_dma_buf) {
         .region_index = region->nr,
-        .open_flags = O_RDWR,
+        .open_flags = O_RDWR | O_CLOEXEC,
         .nr_ranges = region->nr_mmaps,
     };
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 04/34] migration/cpr-exec: require a file or exec migration URI

[Andrey Drobyshev]

cpr-exec writes the device state to the migration URI and new QEMU
reads it back after the exec, so the URI must capture the data: a
file, or an exec command that stores it.  Nothing checks that.  With a
socket URI, e.g. a save or a live migration issued while the 'mode'
parameter is left at cpr-exec after a failed qemu-update, QEMU sends a
stream without the guest RAM to the peer, execs the stale cpr-exec
command, and the VM is lost.

Refuse cpr-exec with anything but a file or exec URI, the two types
the CPR documentation allows.

Fixes: a3eae205c601 ("migration: cpr-exec mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 docs/devel/migration/CPR.rst | 8 ++++----
 migration/migration.c        | 8 ++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
index b6178568a85..2f34a6d7c9d 100644
--- a/docs/devel/migration/CPR.rst
+++ b/docs/devel/migration/CPR.rst
@@ -365,10 +365,10 @@ The VM must be started with the ``-machine aux-ram-share=on`` option.
 Outgoing:
   * Set the migration mode parameter to ``cpr-exec``.
   * Set the ``cpr-exec-command`` parameter.
-  * Issue the ``migrate`` command.  It is recommended that the URI be
-    a ``file`` type, but one can use other types such as ``exec``,
-    provided the command captures all the data from the outgoing side,
-    and provides all the data to the incoming side.
+  * Issue the ``migrate`` command.  The URI must be a ``file`` type,
+    or an ``exec`` type, provided the command captures all the data
+    from the outgoing side, and provides all the data to the incoming
+    side.  Other types are refused.
 
 Incoming:
   * You do not need to explicitly start new QEMU.  It is started as
diff --git a/migration/migration.c b/migration/migration.c
index 2f8ef20c036..39f70328fc7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
         return false;
     }
 
+    if (migrate_mode() == MIG_MODE_CPR_EXEC &&
+        addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
+        addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
+        error_setg(errp,
+                   "cpr-exec mode requires a file or exec migration URI");
+        return false;
+    }
+
     return true;
 }
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 05/34] migration: close the CPR state if the outgoing channel fails to open

[Andrey Drobyshev]

qmp_migrate() saves the CPR state before it opens the migration
channel.  If the channel is a file or an exec command and it can't be
opened, qmp_migrate_finish() returns right away and migration_cleanup()
never runs, so the CPR state file stays open.

Close it there, next to the yank instance, which has the same problem.

Fixes: 624e6e654e11 ("migration: cpr-transfer mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index 39f70328fc7..132f0d403b7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2412,6 +2412,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
         if (!resume_requested) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
+        cpr_state_close();
         migration_connect_set_error(s, local_err);
         error_propagate(errp, local_err);
         return;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 06/34] migration/cpr-exec: close the persisted state if the channel fails to open

[Andrey Drobyshev]

cpr_exec_persist_state() dups the memfd holding the CPR state so that
the dup survives exec(), and remembers it in the environment.  Both
are released by cpr_exec_unpersist_state() alone, which runs only on
MIG_EVENT_PRECOPY_FAILED.  The migration channel failing to open
doesn't raise it, so the dup is leaked and the stale environment
variable is left behind, once per attempt.

Release them there, next to the CPR state.  That path runs in any
migration mode, so make cpr_exec_unpersist_state() a no-op if there's
nothing to release.

Fixes: a3eae205c60 ("migration: cpr-exec mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c  | 4 +++-
 migration/migration.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index c4bdf2cb99f..d4214e6b379 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -83,8 +83,10 @@ void cpr_exec_unpersist_state(void)
     int mfd;
     const char *val = g_getenv(CPR_EXEC_STATE_NAME);
 
+    if (!val) {
+        return;
+    }
     g_unsetenv(CPR_EXEC_STATE_NAME);
-    assert(val);
     assert(!qemu_strtoi(val, NULL, 10, &mfd));
     close(mfd);
 }
diff --git a/migration/migration.c b/migration/migration.c
index 132f0d403b7..7c2fd704890 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2413,6 +2413,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
         cpr_state_close();
+        cpr_exec_unpersist_state();
         migration_connect_set_error(s, local_err);
         error_propagate(errp, local_err);
         return;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 07/34] migration: don't refuse a blocker over a migration of another mode

[Andrey Drobyshev]

migrate_add_blocker_modes() refuses a blocker while a migration is
running, whatever modes the blocker is for.  So, while e.g. a normal
live migration is running, adding a blocker for cpr-transfer only is
refused as well, and whatever operation adds it fails, although the
blocker doesn't concern that migration at all.

Only refuse a blocker if the running migration is of one of its modes.
Snapshots are checked as before.

Fixes: fa3673e497a ("migration: per-mode blockers")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/migration.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7c2fd704890..d5a21da3129 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1866,12 +1866,16 @@ int migrate_init(MigrationState *s, Error **errp)
     return 0;
 }
 
-static bool is_busy(Error **reasonp, Error **errp)
+static bool is_busy(Error **reasonp, Error **errp, int modes)
 {
     ERRP_GUARD();
 
-    /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
-    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
+    /*
+     * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
+     * A running migration only matters if the blocker applies to its mode.
+     */
+    if (runstate_check(RUN_STATE_SAVE_VM) ||
+        (migration_is_running() && (modes & BIT(migrate_mode())))) {
         error_propagate_prepend(errp, *reasonp,
                                 "disallowing migration blocker "
                                 "(migration/snapshot in progress) for: ");
@@ -1942,7 +1946,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
 
     if (is_only_migratable(reasonp, errp, modes)) {
         return -EACCES;
-    } else if (is_busy(reasonp, errp)) {
+    } else if (is_busy(reasonp, errp, modes)) {
         return -EBUSY;
     }
     return add_blockers(reasonp, errp, modes);
@@ -1952,7 +1956,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
 {
     int modes = BIT(MIG_MODE__MAX) - 1;
 
-    if (is_busy(reasonp, errp)) {
+    if (is_busy(reasonp, errp, modes)) {
         return -EBUSY;
     }
     return add_blockers(reasonp, errp, modes);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 08/34] migration: Plug memory leaks after migrate_set_error()

[Andrey Drobyshev]

From: Markus Armbruster <armbru@redhat.com>

migrate_set_error(s, err) stores a copy of @err in @s.  The original
@err is not freed.  Most callers free it immediately.  Some callers
free it later, or pass it on.  And some leak it.  Fix those.

Perhaps migrate_set_error(s, err) should take ownership of @err.  The
callers that free it immediately would become simpler, and avoid a
copy and a deallocation.  The others would have to pass
error_copy(err).

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit 93817ec39656.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Link: https://lore.kernel.org/r/20251115083500.2753895-2-armbru@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/cpr-exec.c | 3 ++-
 migration/multifd.c  | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index d4214e6b379..5a885422154 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -233,11 +233,12 @@ static void cpr_exec_cb(void *opaque)
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);
+    error_free(err);
+    err = NULL;
 
     /* Note, we can go from state COMPLETED to FAILED */
     migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
 
-    err = NULL;
     if (!migration_block_activate(&err)) {
         /* error was already reported */
         error_free(err);
diff --git a/migration/multifd.c b/migration/multifd.c
index 5a723ae25d2..e369421b3ff 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -966,6 +966,7 @@ bool multifd_send_setup(void)
 
         if (!multifd_new_send_channel_create(p, &local_err)) {
             migrate_set_error(s, local_err);
+            error_free(local_err);
             ret = -1;
         }
     }
@@ -990,6 +991,7 @@ bool multifd_send_setup(void)
         ret = multifd_send_state->ops->send_setup(p, &local_err);
         if (ret) {
             migrate_set_error(s, local_err);
+            error_free(local_err);
             goto err;
         }
         assert(p->iov);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 09/34] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

[Andrey Drobyshev]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

While working on savevm/loadvm for a new vfio device, I encountered the
crash below. Since vfio_connect_kvm_msi_virq() didn't check the ->use flag
for the vector, it would pass an unused vector down to
vfio_cpr_load_vector_fd() which would crash.

Fix this by checking the ->use flag along with the virq number to detect
whether a vector is valid or not.

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x0000555555a891ef in vfio_cpr_load_vector_fd (vdev=vdev@entry=0x0,
    name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1) at ../hw/vfio/cpr.c:44
44	    g_autofree char *fdname = STRDUP_VECTOR_FD_NAME(vdev, name);
(gdb) bt
    (vdev=vdev@entry=0x0, name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1)
    at ../hw/vfio/cpr.c:44
    (vdev=0x0, e=e@entry=0x5555586971b4, name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1, errp=errp@entry=0x0) at ../hw/vfio/pci.c:79
    at ../hw/vfio/pci.c:601
    at ../hw/vfio/pci.c:597
    at ../hw/vfio/pci.c:3088
    at ../hw/vfio/migration.c:278
    (f=f@entry=0x5555574a43d0, se=se@entry=0x5555591edd40, errp=errp@entry=0x7fffffffe130)
    at ../migration/savevm.c:971
    (f=f@entry=0x5555574a43d0, type=type@entry=4 '\004', errp=errp@entry=0x7fffffffe130)
    at ../migration/savevm.c:2654
    (f=f@entry=0x5555574a43d0, mis=mis@entry=0x5555571de5a0, errp=0x7fffffffe130,
    errp@entry=0x555557157c10 <error_fatal>) at ../migration/savevm.c:2973
    (f=f@entry=0x5555574a43d0, errp=errp@entry=0x555557157c10 <error_fatal>)
    at ../migration/savevm.c:3058
    (name=0x7fffffffecc9 "foo", vmstate=vmstate@entry=0x0, has_devices=has_devices@entry=false, devices=devices@entry=0x0, errp=errp@entry=0x555557157c10 <error_fatal>) at ../migration/savevm.c:3452

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit f498c21b3595.

Fixes: 30edcb4d4e7a ("vfio-pci: preserve MSI")
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260727150038.2684512-1-tycho@kernel.org
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fd73a0fdf87..5e281b2e1d9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -589,7 +589,7 @@ static void vfio_connect_kvm_msi_virq(VFIOMSIVector *vector, int nr)
 {
     const char *name = "kvm_interrupt";
 
-    if (vector->virq < 0) {
+    if (!vector->use || vector->virq < 0) {
         return;
     }
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 10/34] vfio/pci: Reject invalid MSI-X Table and PBA BIR values

[Andrey Drobyshev]

From: Cédric Le Goater <clg@redhat.com>

The 3-bit MSI-X BIR fields permit values 0-7, but VFIOPCIDevice::bars[]
only contains BAR0-BAR5 (6 entries). An invalid BIR value of 6 or 7
can cause an out-of-bounds array access (CWE-129) in vfio_msix_early_setup().

Add range checks immediately after extracting the BIR values.

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit 9daa7347ff25.

Reported-by: Feifan Qian <bea1e@proton.me>
Fixes: 65501a745dba ("vfio: vfio-pci device assignment driver")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3878
Reviewed-by: Alex Williamson <alex@shazbot.org>
Link: https://lore.kernel.org/qemu-devel/20260706161334.2165482-1-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5e281b2e1d9..d930ef46309 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1791,6 +1791,14 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
     msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
 
+    if (msix->table_bar >= ARRAY_SIZE(vdev->bars) ||
+        msix->pba_bar >= ARRAY_SIZE(vdev->bars)) {
+        error_setg(errp, "invalid MSI-X BIR, table_bar=%d pba_bar=%d",
+                   msix->table_bar, msix->pba_bar);
+        g_free(msix);
+        return false;
+    }
+
     ret = vfio_device_get_irq_info(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
                                    &irq_info);
     if (ret < 0) {

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 11/34] vfio/pci: Fix information leak in vfio_rom_read()

[Andrey Drobyshev]

From: Cédric Le Goater <clg@redhat.com>

vfio_rom_read() can leak uninitialized stack data to the guest when a
read occurs at the end boundary of the ROM BAR.

Fix this by initializing the val union to 0xff, matching the ROM
initialization pattern used in vfio_pci_load_rom().

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit 402fd5a75659.

Reviewed-by: Philippe Mathieu-Daudé <philmd@mailo.com>
Reviewed-by: Mario Casquero <mcasquer@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260602074446.1206892-1-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d930ef46309..fa2ca0d779d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1153,7 +1153,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
         uint16_t word;
         uint32_t dword;
         uint64_t qword;
-    } val;
+    } val = { .qword = ~0ULL };
     uint64_t data = 0;
 
     /* Load the ROM lazily when the guest tries to read it */

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 12/34] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

[Andrey Drobyshev]

From: Mario Casquero <mcasquer@redhat.com>

When vfio_device_get_region_info() fails in vfio_pci_load_rom(),
the function returns without setting vdev->rom_read_failed to true,
and without allocating vdev->rom. This leaves vdev->rom as NULL.

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit c822ecd80661.

Signed-off-by: Mario Casquero <mcasquer@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260602070857.356526-1-mcasquer@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fa2ca0d779d..bea478d521f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1039,7 +1039,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
     }
 }
 
-static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
+static bool vfio_pci_load_rom(VFIOPCIDevice *vdev)
 {
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info = NULL;
@@ -1053,7 +1053,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
 
     if (ret != 0) {
         error_report("vfio: Error getting ROM info: %s", strerror(-ret));
-        return;
+        return false;
     }
 
     trace_vfio_pci_load_rom(vbasedev->name, (unsigned long)reg_info->size,
@@ -1064,12 +1064,11 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
     vdev->rom_offset = reg_info->offset;
 
     if (!vdev->rom_size) {
-        vdev->rom_read_failed = true;
         error_report("vfio-pci: Cannot read device rom at %s", vbasedev->name);
         error_printf("Device option ROM contents are probably invalid "
                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
                     "or load from file with romfile=\n");
-        return;
+        return false;
     }
 
     vdev->rom = g_malloc(size);
@@ -1125,6 +1124,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
             data[6] = -csum;
         }
     }
+
+    return true;
 }
 
 /* "Raw" read of underlying config space. */
@@ -1158,7 +1159,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
 
     /* Load the ROM lazily when the guest tries to read it */
     if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
-        vfio_pci_load_rom(vdev);
+        vdev->rom_read_failed = !vfio_pci_load_rom(vdev);
     }
 
     memcpy(&val, vdev->rom + addr,

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 13/34] vfio/pci: Grow buffer in vfio_pci_host_match()

[Andrey Drobyshev]

From: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Each field of PCIHostDeviceAddress is an unsigned int, therefore
while a valid address is limited to 13 characters, an invalid
address could exceed the specified format, up to:

        ffffffff:ffffffff:ffffffff.ffffffff<NUL>

This requires 36 characters with the terminator.

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit 1785bf9685ec.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>
Message-ID: <20260305-nvme-v4-2-b65b9de1839f@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bea478d521f..50498148275 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2691,7 +2691,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 
 bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-    char tmp[13];
+    char tmp[36];
 
     sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
             addr->bus, addr->slot, addr->function);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 14/34] q35: Fix migration of SMRAM state

[Andrey Drobyshev]

From: Igor Mammedov <imammedo@redhat.com>

When migrating, dst QEMU by default has SMRAM unlocked,
and since wmask is not migrated, the migrated value of
MCH_HOST_BRIDGE_F_SMBASE in config space fall to prey of

  mch_update_smbase_smram()
    ...
    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
        *reg = 0x00;

and is getting cleared and leads to unlocked smram
on dst even if on source it's been locked.

As Andrey has pointed out [1], we should derive wmask
from config and not other way around.

Drop offending chunk and resync wmask based on MCH_HOST_BRIDGE_F_SMBASE
register value. That would preserve the register during
migration and set smram regions into corresponding state.

What that changes is:
that it would let guest write junk values in register
(with no apparent effect) until it's stumbles upon
reserved 0x1 [|] 0x2 values, at which point it
would be only possible to lock register and trigger
switch to SMRAM blackhole in CPU AS.

While at it, fix up test by removing junk discard before negotiation hunk.

PS2:
Instead of adding a dedicated post_load handler for it,
reuse mch_update->mch_update_smbase_smram call chain
that is called on write/reset/post_load to be consistent
with how we handle mch registers.

PS3:
for prosterity here is erro message Andrey got due to this bug:
    qemu: vfio_container_dma_map(0x..., 0x0, 0xa0000, 0x....) = -22 (Invalid argument)
    qemu: hardware error: vfio: DMA mapping failed, unable to continue

1) https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit 66cf169e29b0.

Fixes: f404220e279c ("q35: implement 128K SMRAM at default SMBASE address")
Reported-by: Andrey Ryabinin <arbn@yandex-team.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andrey Ryabinin <arbn@yandex-team.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20251211165454.288476-1-imammedo@redhat.com>
---
 hw/pci-host/q35.c      | 25 +++++++++++--------------
 tests/qtest/q35-test.c |  6 ------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index a708758d361..946342ba585 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
     }
 
     if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
-        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
-            MCH_HOST_BRIDGE_F_SMBASE_LCK;
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
         *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
         return;
     }
 
     /*
-     * default/reset state, discard written value
-     * which will disable SMRAM balackhole at SMBASE
+     * reg value can come from register write/reset/migration source,
+     * update wmask to be in sync with it regardless of source
      */
-    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
-        *reg = 0x00;
+    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+        return;
     }
-
-    memory_region_transaction_begin();
     if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
-        /* disable all writes */
-        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
-            ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
+        /* lock register at 0x2 and disable all writes */
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
         *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
-        lck = true;
-    } else {
-        lck = false;
     }
+
+    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
+    memory_region_transaction_begin();
     memory_region_set_enabled(&mch->smbase_blackhole, lck);
     memory_region_set_enabled(&mch->smbase_window, lck);
     memory_region_transaction_commit();
diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
index 62fff49fc89..4e3a4457f62 100644
--- a/tests/qtest/q35-test.c
+++ b/tests/qtest/q35-test.c
@@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
     qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
     g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
 
-    /* check that writing junk to 0x9c before before negotiating is ignored */
-    for (i = 0; i < 0xff; i++) {
-        qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
-        g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
-    }
-
     /* enable SMRAM at SMBASE */
     qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
     g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 15/34] migration: ensure APIC is loaded prior to VFIO PCI devices

[Andrey Drobyshev]

From: Yanfei Xu <yanfei.xu@bytedance.com>

The load procedure of VFIO PCI devices involves setting up IRT
for each VFIO PCI devices. This requires determining whether an
interrupt is single-destination interrupt to decide between
Posted Interrupt(PI) or remapping mode for the IRTE. However,
determining this may require accessing the VM's APIC registers.

For example:
ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irqs)
  ...
    kvm_arch_irq_bypass_add_producer
      kvm_x86_call(pi_update_irte)
        vmx_pi_update_irte
          kvm_intr_is_single_vcpu

If the LAPIC has not been loaded yet, interrupts will use remapping
mode. To prevent the fallback of interrupt mode, keep APIC is always
loaded prior to VFIO PCI devices.

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit d943cef76090.

Signed-off-by: Yicong Shen <shenyicong.1023@bytedance.com>
Signed-off-by: Yanfei Xu <yanfei.xu@bytedance.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/r/20250818131127.1021648-1-yanfei.xu@bytedance.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/apic_common.c       | 1 +
 include/migration/vmstate.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 37a7a7019d3..394fe020134 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -379,6 +379,7 @@ static const VMStateDescription vmstate_apic_common = {
     .pre_load = apic_pre_load,
     .pre_save = apic_dispatch_pre_save,
     .post_load = apic_dispatch_post_load,
+    .priority = MIG_PRI_APIC,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(apicbase, APICCommonState),
         VMSTATE_UINT8(id, APICCommonState),
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 6839117be87..c3d156c514f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -163,6 +163,7 @@ typedef enum {
     MIG_PRI_IOMMU,              /* Must happen before PCI devices */
     MIG_PRI_PCI_BUS,            /* Must happen before IOMMU */
     MIG_PRI_VIRTIO_MEM,         /* Must happen before IOMMU */
+    MIG_PRI_APIC,               /* Must happen before PCI devices */
     MIG_PRI_GICV3_ITS,          /* Must happen before PCI devices */
     MIG_PRI_GICV3,              /* Must happen before the ITS */
     MIG_PRI_MAX,

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 16/34] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Andrey Drobyshev]

From: Arun Menon <armenon@redhat.com>

error_fatal is passed to vmstate_load_state() and vmstate_save_state()
functions. This was introduced in commit c632ffbd74. This would exit(1)
on error, and therefore does not allow to propagate the error back to
the caller.

To maintain consistency with prior error handling i.e. either propagating
the error to the caller or reporting it, we must set the error within a
local Error object instead of using error_fatal.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Arun Menon <armenon@redhat.com>
Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com
[peterx: always uninit var ret, per Akihiko]
[peterx: touchups on line ordering, spacings etc.]

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 986c3292c6b8,
    adapted for the hci-dev base, which predates the upstream commit
    bcb536cabe10 ("error: Kill @error_warn"): virtio_gpu_load() has no
    'err' local there yet, so it is declared here.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/display/virtio-gpu.c | 22 +++++++++++++++-------
 hw/pci/pci.c            | 15 +++++++++++++--
 hw/s390x/virtio-ccw.c   | 17 +++++++++++++++--
 hw/scsi/spapr_vscsi.c   | 10 ++++++++--
 hw/virtio/virtio-mmio.c | 15 +++++++++++++--
 hw/virtio/virtio-pci.c  | 15 +++++++++++++--
 hw/virtio/virtio.c      | 10 +++++++---
 7 files changed, 84 insertions(+), 20 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 477ec700a13..4bc8c778508 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1223,7 +1223,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
 {
     VirtIOGPU *g = opaque;
     struct virtio_gpu_simple_resource *res;
-    int i;
+    Error *err = NULL;
+    int i, ret;
 
     /* in 2d mode we should never find unprocessed commands here */
     assert(QTAILQ_EMPTY(&g->cmdq));
@@ -1246,8 +1247,12 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
     }
     qemu_put_be32(f, 0); /* end of list */
 
-    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL,
-                              &error_fatal);
+    ret = vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL,
+                             &err);
+    if (ret < 0) {
+        error_report_err(err);
+    }
+    return ret;
 }
 
 static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
@@ -1283,9 +1288,10 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
                            const VMStateField *field)
 {
     VirtIOGPU *g = opaque;
+    Error *err = NULL;
     struct virtio_gpu_simple_resource *res;
     uint32_t resource_id, pformat;
-    int i;
+    int i, ret;
 
     g->hostmem = 0;
 
@@ -1344,9 +1350,11 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
     }
 
     /* load & apply scanout state */
-    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
-
-    return 0;
+    ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
+    if (ret < 0) {
+        error_report_err(err);
+    }
+    return ret;
 }
 
 static int virtio_gpu_blob_save(QEMUFile *f, void *opaque, size_t size,
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 5d7d17f6529..50f062045a9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -926,21 +926,32 @@ const VMStateDescription vmstate_pci_device = {
 
 void pci_device_save(PCIDevice *s, QEMUFile *f)
 {
+    Error *local_err = NULL;
+    int ret;
+
     /* Clear interrupt status bit: it is implicit
      * in irq_state which we are saving.
      * This makes us compatible with old devices
      * which never set or clear this bit. */
     s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
-    vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal);
+    ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
     /* Restore the interrupt status bit. */
     pci_update_irq_status(s);
 }
 
 int pci_device_load(PCIDevice *s, QEMUFile *f)
 {
+    Error *local_err = NULL;
     int ret;
+
     ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
-                             &error_fatal);
+                             &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
     /* Restore the interrupt status bit. */
     pci_update_irq_status(s);
     return ret;
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 4cb1ced001a..4a3ffb84f8f 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1130,13 +1130,26 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
 static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-    vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &error_fatal);
+    Error *local_err = NULL;
+    int ret;
+
+    ret = vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
 }
 
 static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
+    Error *local_err = NULL;
+    int ret;
+
+    ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index f0a7dd2b882..a6591319db7 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -628,10 +628,16 @@ static const VMStateDescription vmstate_spapr_vscsi_req = {
 static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq)
 {
     vscsi_req *req = sreq->hba_private;
-    assert(req->active);
+    Error *local_err = NULL;
+    int rc;
 
-    vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal);
+    assert(req->active);
 
+    rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
+    if (rc < 0) {
+        error_report_err(local_err);
+        return;
+    }
     trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
                                    req->cur_desc_offset);
 }
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index fb58c364527..c05c00bcd4a 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -612,15 +612,26 @@ static const VMStateDescription vmstate_virtio_mmio = {
 static void virtio_mmio_save_extra_state(DeviceState *opaque, QEMUFile *f)
 {
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+    Error *local_err = NULL;
+    int ret;
 
-    vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &error_fatal);
+    ret = vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
 }
 
 static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
 {
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+    Error *local_err = NULL;
+    int ret;
 
-    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
+    ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 static bool virtio_mmio_has_extra_state(DeviceState *opaque)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7273da4c018..c276fbb017c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -189,15 +189,26 @@ static bool virtio_pci_has_extra_state(DeviceState *d)
 static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+    Error *local_err = NULL;
+    int ret;
 
-    vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &error_fatal);
+    ret = vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
 }
 
 static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+    Error *local_err = NULL;
+    int ret;
 
-    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
+    ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a7063d5555e..d4b2e67c274 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3016,7 +3016,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff);
-    int i;
+    int i, ret;
     Error *local_err = NULL;
 
     if (k->save_config) {
@@ -3061,7 +3061,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
     }
 
     if (vdc->vmsd) {
-        int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
+        ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
         if (ret) {
             error_report_err(local_err);
             return ret;
@@ -3069,7 +3069,11 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
     }
 
     /* Subsections */
-    return vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &error_fatal);
+    ret = vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 /* A wrapper for use as a VMState .put function */

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 17/34] migration: vmstate_save_state_v: fix double error_setg

[Andrey Drobyshev]

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

We may call error_setg twice on same errp if inner
vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
crash on assertion in error_setv().

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit d41ce10d0f5a.

Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index aea41c09946..04903131257 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 } else {
                     ret = inner_field->info->put(f, curr_elem, size,
                                                  inner_field, vmdesc_loop);
+                    if (ret < 0) {
+                        error_setg(errp, "put failed");
+                    }
                 }
 
                 written_bytes = qemu_file_transferred(f) - old_offset;
@@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 if (ret) {
-                    error_setg(errp, "Save of field %s/%s failed",
-                                vmsd->name, field->name);
+                    error_prepend(errp, "Save of field %s/%s failed: ",
+                                  vmsd->name, field->name);
                     if (vmsd->post_save) {
                         vmsd->post_save(opaque);
                     }

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 18/34] migration: yank: Move register instance earlier

[Andrey Drobyshev]

From: Fabiano Rosas <farosas@suse.de>

Move the register_instance call to migrate_prepare() so it can be
paired with the unregister_instance at migration_cleanup(). Otherwise,
the cleanup cannot be run when cpr_state_save() fails because the
instance is registered only after it.

When resuming from a paused postcopy migration, migrate_prepare()
returns early, but migration_cleanup() doesn't run, so the yank will
remain paired.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 7e714392437c,
    adapted for the hci-dev base, which predates the upstream
    migration_connect_set_error() -> migration_connect_error_propagate()
    rename (77c82edadbe0): the error propagation calls in qmp_migrate()
    and qmp_migrate_finish() keep the older spelling.

    Also fixed here: upstream left the yank instance registered when
    qmp_migrate_finish() fails before connecting (nothing calls
    migration_cleanup() then), so the next migrate command aborted on
    yank_register_instance(&error_abort); and the unregister at the out:
    label ran on a resume, which never registered the instance.  Upstream
    fixed both in 6b587af5ec9d ("migration: Fold migration_cleanup() into
    migration_connect_error_propagate()"), which needs the above rework;
    this is the equivalent for our base.

    Also register the instance with errp, before migrate_init(), instead
    of with &error_abort: a duplicate instance, e.g. a previous migration
    still being cleaned up, then fails the migrate command instead of
    aborting QEMU.  And refuse migrate while an incoming postcopy
    migration is still in progress: the guest already runs then, but the
    incoming side still holds the yank instance.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d5a21da3129..cc88f70ae59 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2186,6 +2186,15 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         return false;
     }
 
+    /*
+     * The guest already runs during incoming postcopy, but the incoming
+     * migration still holds the yank instance registered below.
+     */
+    if (migration_in_incoming_postcopy()) {
+        error_setg(errp, "Incoming postcopy migration is still in progress");
+        return false;
+    }
+
     if (runstate_check(RUN_STATE_POSTMIGRATE)) {
         error_setg(errp, "Can't migrate the vm that was paused due to "
                    "previous migration");
@@ -2243,15 +2252,19 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         }
     }
 
+    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+        return false;
+    }
+
     if (migrate_init(s, errp)) {
+        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         return false;
     }
 
     return true;
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp);
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
 
 static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
                             void *opaque)
@@ -2276,7 +2289,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
 {
     MigrationAddress *addr = opaque;
 
-    qmp_migrate_finish(addr, false, NULL);
+    qmp_migrate_finish(addr, NULL);
 
     cpr_state_close();
     migrate_hup_delete(migrate_get_current());
@@ -2288,7 +2301,6 @@ void qmp_migrate(const char *uri, bool has_channels,
                  MigrationChannelList *channels, bool has_detach, bool detach,
                  bool has_resume, bool resume, Error **errp)
 {
-    bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     g_autoptr(MigrationChannel) channel = NULL;
@@ -2341,8 +2353,7 @@ void qmp_migrate(const char *uri, bool has_channels,
         return;
     }
 
-    resume_requested = has_resume && resume;
-    if (!migrate_prepare(s, resume_requested, errp)) {
+    if (!migrate_prepare(s, has_resume && resume, errp)) {
         /* Error detected, put into errp */
         return;
     }
@@ -2366,27 +2377,26 @@ void qmp_migrate(const char *uri, bool has_channels,
                         QAPI_CLONE(MigrationAddress, addr));
 
     } else {
-        qmp_migrate_finish(addr, resume_requested, errp);
+        qmp_migrate_finish(addr, errp);
     }
 
 out:
     if (local_err) {
+        /* A resume didn't register it, see migrate_prepare() */
+        if (s->state != MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP) {
+            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+        }
         migration_connect_set_error(s, local_err);
         error_propagate(errp, local_err);
     }
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp)
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
 {
     MigrationState *s = migrate_get_current();
     Error *local_err = NULL;
-
-    if (!resume_requested) {
-        if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
-            return;
-        }
-    }
+    /* Before the start functions below change the state */
+    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
 
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         SocketAddress *saddr = &addr->u.socket;
@@ -2413,7 +2423,8 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
     }
 
     if (local_err) {
-        if (!resume_requested) {
+        /* Nothing connected: migration_cleanup() won't unregister it */
+        if (!resume) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
         cpr_state_close();

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 19/34] migration: Free cpr-transfer MigrationAddress along with gsource

[Andrey Drobyshev]

From: Fabiano Rosas <farosas@suse.de>

When setting a callback on a Glib source and giving it a data pointer,
it's natural to also provide the destructor for the data in question.

Since migrate_hup_add() already needs to clone the MigrationAddress
when setting the qmp_migrate_finish_cb callback, also pass the
qapi_free_MigrationAddress as the GDestroyNotify callback.

With this the address doesn't need to be freed at the callback body,
making the management of that memory slightly simpler.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 6a887119ce18,
    adapted for the hci-dev base: qmp_migrate_finish_cb() here still
    closes the cpr state and deletes the HUP source itself (upstream had
    already moved that into migration_cleanup()), so only the explicit
    qapi_free_MigrationAddress() call goes away from it.

Cc: Mark Kanda <mark.kanda@oracle.com>
Cc: Ben Chaney <bchaney@akamai.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-23-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index cc88f70ae59..b365e413a84 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2269,9 +2269,11 @@ static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
 static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
                             void *opaque)
 {
-        s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
-        g_source_set_callback(s->hup_source, cb, opaque, NULL);
-        g_source_attach(s->hup_source, NULL);
+    s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
+    g_source_set_callback(s->hup_source, cb,
+                          QAPI_CLONE(MigrationAddress, opaque),
+                          (GDestroyNotify)qapi_free_MigrationAddress);
+    g_source_attach(s->hup_source, NULL);
 }
 
 static void migrate_hup_delete(MigrationState *s)
@@ -2293,7 +2295,6 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
 
     cpr_state_close();
     migrate_hup_delete(migrate_get_current());
-    qapi_free_MigrationAddress(addr);
     return G_SOURCE_REMOVE;
 }
 
@@ -2374,7 +2375,7 @@ void qmp_migrate(const char *uri, bool has_channels,
      */
     if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
         migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
-                        QAPI_CLONE(MigrationAddress, addr));
+                        addr);
 
     } else {
         qmp_migrate_finish(addr, errp);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 20/34] migration: Move CPR HUP watch to cpr-transfer.c

[Andrey Drobyshev]

From: Fabiano Rosas <farosas@suse.de>

Move this CPR-specific code into a cpr file. While here, give the
functions more significant names.

This makes the new idea (after cpr-transfer) of having two parts to
qmp_migrate slightly more obvious: either wait for the hangup or
continue directly.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit f523e5aa5f56,
    adapted for the hci-dev base, which predates the upstream qmp_migrate()
    rework around MigrationChannel: the watch is still armed with the
    plain MigrationAddress from qmp_migrate() rather than main_ch->addr,
    and qmp_migrate_finish_cb() keeps closing the cpr state and destroying
    the HUP source itself.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-24-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/migration/cpr.h  |  5 +++++
 migration/cpr-transfer.c | 23 +++++++++++++++++++++++
 migration/migration.c    | 29 ++++-------------------------
 3 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 8fb79eb0a28..ae441322ff5 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -9,6 +9,7 @@
 #define MIGRATION_CPR_H
 
 #include "qapi/qapi-types-migration.h"
+#include "io/channel.h"
 #include "qemu/queue.h"
 
 #define MIG_MODE_NONE           -1
@@ -76,6 +77,10 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index, bool cpr,
 QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp);
 QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
 
+void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
+                                void *opaque);
+void cpr_transfer_source_destroy(MigrationState *s);
+
 void cpr_exec_init(void);
 QEMUFile *cpr_exec_output(Error **errp);
 QEMUFile *cpr_exec_input(Error **errp);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index 00371d17c3a..61d5c9dce2b 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -6,7 +6,10 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-migration.h"
+#include "io/channel.h"
 #include "io/channel-file.h"
 #include "io/channel-socket.h"
 #include "io/net-listener.h"
@@ -72,3 +75,23 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
         return NULL;
     }
 }
+
+void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
+                                void *opaque)
+{
+    s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
+    g_source_set_callback(s->hup_source,
+                          (GSourceFunc)func,
+                          QAPI_CLONE(MigrationAddress, opaque),
+                          (GDestroyNotify)qapi_free_MigrationAddress);
+    g_source_attach(s->hup_source, NULL);
+}
+
+void cpr_transfer_source_destroy(MigrationState *s)
+{
+    if (s->hup_source) {
+        g_source_destroy(s->hup_source);
+        g_source_unref(s->hup_source);
+        s->hup_source = NULL;
+    }
+}
diff --git a/migration/migration.c b/migration/migration.c
index b365e413a84..d892655a21e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,7 +110,6 @@ static bool migration_object_check(MigrationState *ms, Error **errp);
 static bool migration_switchover_start(MigrationState *s, Error **errp);
 static bool close_return_path_on_source(MigrationState *s);
 static void migration_completion_end(MigrationState *s);
-static void migrate_hup_delete(MigrationState *s);
 
 static void migration_downtime_start(MigrationState *s)
 {
@@ -1508,7 +1507,7 @@ static void migration_cleanup(MigrationState *s)
 
     qemu_savevm_state_cleanup();
     cpr_state_close();
-    migrate_hup_delete(s);
+    cpr_transfer_source_destroy(s);
 
     close_return_path_on_source(s);
 
@@ -1669,7 +1668,7 @@ void migration_cancel(void)
         migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                           MIGRATION_STATUS_CANCELLED);
         cpr_state_close();
-        migrate_hup_delete(s);
+        cpr_transfer_source_destroy(s);
     }
 }
 
@@ -2266,25 +2265,6 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
 
 static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
 
-static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
-                            void *opaque)
-{
-    s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
-    g_source_set_callback(s->hup_source, cb,
-                          QAPI_CLONE(MigrationAddress, opaque),
-                          (GDestroyNotify)qapi_free_MigrationAddress);
-    g_source_attach(s->hup_source, NULL);
-}
-
-static void migrate_hup_delete(MigrationState *s)
-{
-    if (s->hup_source) {
-        g_source_destroy(s->hup_source);
-        g_source_unref(s->hup_source);
-        s->hup_source = NULL;
-    }
-}
-
 static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
                                       GIOCondition cond,
                                       void *opaque)
@@ -2294,7 +2274,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
     qmp_migrate_finish(addr, NULL);
 
     cpr_state_close();
-    migrate_hup_delete(migrate_get_current());
+    cpr_transfer_source_destroy(migrate_get_current());
     return G_SOURCE_REMOVE;
 }
 
@@ -2374,8 +2354,7 @@ void qmp_migrate(const char *uri, bool has_channels,
      * connection, so qmp_migrate_finish will fail to connect, and then recover.
      */
     if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
-        migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
-                        addr);
+        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
 
     } else {
         qmp_migrate_finish(addr, errp);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 21/34] migration: Fix crash on second migration when cancel early

[Andrey Drobyshev]

From: Peter Xu <peterx@redhat.com>

Marc-André reported an issue on QEMU crash when retrying a cancelled
migration during early setup phase, see "Link:" for more information, and
also easy way to reproduce.

This patch is a replacement of the prior fix proposed by not only switching
to migration_cleanup(), but also fixing it from CPR side, so that we track
hup_source properly to know if src QEMU is waiting or the HUP signal.

To put it simple: this chunk of special casing in migration_cancel() should
not affect normal migration, but only cpr-transfer migration to cover the
small window when the src QEMU is waiting for a HUP signal on cpr
channel (so that src QEMU can continue the migration on the main channel).

To achieve that, we'll also need to remember to detach the hup_source
whenenver invoked: after that point, we should always be able to cleanup
the migration.

It's not a generic operation to explicitly detach a gsource from its
context while in its dispatch() function.  But it should be safe, because
gsource disptch() will only happen with a boosted refcount for the
dispatcher so that the gsource will not be freed until the callback
completes. It's also safe to return G_SOURCE_REMOVE after the gsource is
detached, as glib will simply ignore the G_SOURCE_REMOVE.

One can refer to latest 2.86.5 glib code in g_main_dispatch() for that:

https://github.com/GNOME/glib/blob/2.86.5/glib/gmain.c#L3592

When at this, add a bunch of assertions to make sure nothing surprises us.

After this patch applied, the 2nd migration will not crash QEMU, instead
it'll be in CANCELLING until the socket connection times out (it will take
~2min on my Fedora default kernel).  During this process no 2nd migration
will be allowed, and after it timed out migration can be restarted.

It's because so far we don't have control over socket_connect_outgoing(),
or anything yet managed by a task executed in qio_task_run_in_thread().
Speeding up the cancellation to be left for future.

I also tested cpr-transfer by only providing cpr channel not the main
channel (with -incoming defer), kickoff migration on source, then cancel it
on source directly without providing the main channel.  It keeps working.

I wanted to add an unit test for that but it'll need to refactor current
cpr-transfer tests first; let's leave it for later.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 4f08b870f8f0,
    adapted for the hci-dev base, which predates the upstream qmp_migrate()
    rework: the HUP callback is still qmp_migrate_finish_cb() rather than
    migration_connect_outgoing_cb(), so the early
    cpr_transfer_source_destroy() call lands there, ahead of
    qmp_migrate_finish() and the cpr_state_close() the callback keeps
    doing itself.

Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/cpr.h  |  1 +
 migration/cpr-transfer.c | 10 ++++++++++
 migration/migration.c    | 32 ++++++++++++++++++++++++--------
 migration/migration.h    |  5 +++++
 4 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index ae441322ff5..4dd397a7c13 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -80,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
 void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
                                 void *opaque);
 void cpr_transfer_source_destroy(MigrationState *s);
+bool cpr_transfer_source_active(MigrationState *s);
 
 void cpr_exec_init(void);
 QEMUFile *cpr_exec_output(Error **errp);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index 61d5c9dce2b..9defe7bad78 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -6,6 +6,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-migration.h"
@@ -79,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
 void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
                                 void *opaque)
 {
+    assert(bql_locked());
     s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
     g_source_set_callback(s->hup_source,
                           (GSourceFunc)func,
@@ -89,9 +91,17 @@ void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
 
 void cpr_transfer_source_destroy(MigrationState *s)
 {
+    assert(bql_locked());
     if (s->hup_source) {
         g_source_destroy(s->hup_source);
         g_source_unref(s->hup_source);
         s->hup_source = NULL;
     }
 }
+
+bool cpr_transfer_source_active(MigrationState *s)
+{
+    /* Whenever the HUP gsource is available, it's active. */
+    assert(bql_locked());
+    return s->hup_source;
+}
diff --git a/migration/migration.c b/migration/migration.c
index d892655a21e..821764afae9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1661,14 +1661,19 @@ void migration_cancel(void)
     }
 
     /*
-     * If qmp_migrate_finish has not been called, then there is no path that
-     * will complete the cancellation.  Do it now.
+     * This is cpr-transfer specific processing.
+     *
+     * If this is true, it means cpr-transfer migration is waiting for the
+     * destination to send HUP event on CPR channel to continue the next
+     * phase.  If so, do the cleanup proactively to avoid get stuck in
+     * CANCELLING state.
      */
-    if (setup && !s->to_dst_file) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
-                          MIGRATION_STATUS_CANCELLED);
-        cpr_state_close();
-        cpr_transfer_source_destroy(s);
+    if (cpr_transfer_source_active(s)) {
+        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
+        assert(setup && !s->to_dst_file);
+        migration_cleanup(s);
+        /* Now all things should have been released */
+        assert(!cpr_transfer_source_active(s));
     }
 }
 
@@ -2270,11 +2275,22 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
                                       void *opaque)
 {
     MigrationAddress *addr = opaque;
+    MigrationState *s = migrate_get_current();
+
+    /*
+     * Detach and release the GSource right after use.  We rely on this to
+     * detect this small cpr-transfer window of "waiting for HUP event".
+     */
+    cpr_transfer_source_destroy(s);
 
     qmp_migrate_finish(addr, NULL);
 
     cpr_state_close();
-    cpr_transfer_source_destroy(migrate_get_current());
+
+    /*
+     * This is redundant as we do cpr_transfer_source_destroy() at the
+     * entry, but it's benign; glib will just skip the detach.
+     */
     return G_SOURCE_REMOVE;
 }
 
diff --git a/migration/migration.h b/migration/migration.h
index a7ea43e925d..ee15ca88461 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -509,6 +509,11 @@ struct MigrationState {
     /* Is this a rdma migration */
     bool rdma_migration;
 
+    /*
+     * When set, it means cpr-transfer is waiting for the HUP signal from
+     * destination to continue the 2nd step of migration via the main
+     * channel.
+     */
     GSource *hup_source;
 };
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 22/34] kvm: tolerate ioctls after kvm_close()

[Andrey Drobyshev]

Since commit 7ed0919119b0 ("migration: close kvm after cpr") a source
QEMU that has handed its VFIO devices over via cpr-transfer (or
cpr-exec) calls kvm_close() once the migration completes, so that the
destination can register its irq bypass consumers.  kvm_close() closes
the VM and vCPU descriptors and sets kvm_state to NULL, but the source
process keeps living with all its device models intact until the
management layer terminates it.

Nothing prevents those device models from raising interrupts in the
meantime, and the regular exit path even guarantees one: net_cleanup()
deletes the netdev behind an emulated virtio-net, the NIC sees the link
go down and calls virtio_notify_config(), MSI-X delivers through the
in-kernel APIC, and kvm_irqchip_send_msi() dereferences the NULL
kvm_state:

  #0  kvm_vm_ioctl (s=0x0, type=KVM_SIGNAL_MSI)   accel/kvm/kvm-all.c
  #1  kvm_irqchip_send_msi (s=0x0)
  #2  kvm_send_msi                                hw/i386/kvm/apic.c
  #3  kvm_apic_mem_write
  ...
  #9  pci_msi_trigger
  #11 msix_notify
  #12 virtio_pci_notify
  #14 virtio_notify_config
  #15 virtio_net_set_link_status
  #16 qemu_del_net_client
  #17 net_cleanup
  #18 qemu_cleanup

Every retired cpr-transfer source with a VFIO device thus dies with
SIGSEGV on SIGTERM/quit and leaves a core dump behind.  Without a VFIO
device kvm_close() never runs and the same exit is harmless.

Make the ioctl wrappers honour the closed state instead of assuming a
live KVMState: keep kvm_state allocated, with its closed descriptors as
the mark, and let kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl()
return 0 once kvm_close() has been called, so any late interrupt or
query on the retired source is a no-op.  Returning an error instead
would still kill it: kvm_set_irq() aborts and kvm_irqchip_commit_routes()
asserts on any failure, and the ioeventfd callers abort on a stale errno.

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 accel/kvm/kvm-all.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a39c1ba260a..8b5d342af52 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -644,7 +644,6 @@ void kvm_close(void)
         close(kvm_state->fd);
         kvm_state->fd = -1;
     }
-    kvm_state = NULL;
 }
 
 /*
@@ -3371,6 +3370,11 @@ int kvm_ioctl(KVMState *s, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: there is no KVM to talk to any more */
+    if (s->fd == -1) {
+        return 0;
+    }
+
     trace_kvm_ioctl(type, arg);
     ret = ioctl(s->fd, type, arg);
     if (ret == -1) {
@@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: there is no VM to talk to any more */
+    if (s->vmfd == -1) {
+        return 0;
+    }
+
     trace_kvm_vm_ioctl(type, arg);
     accel_ioctl_begin();
     ret = ioctl(s->vmfd, type, arg);
@@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: the vCPU fds are gone */
+    if (cpu->kvm_fd == -1) {
+        return 0;
+    }
+
     trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
     accel_cpu_ioctl_begin(cpu);
     ret = ioctl(cpu->kvm_fd, type, arg);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 23/34] kvm: fix coalesced MMIO flush racing with kvm_close()

[Andrey Drobyshev]

kvm_close() destroys the vCPUs, which unmaps their kvm_run areas, and the
coalesced MMIO ring lives in the kvm_run area of the first vCPU.  Nothing
stops the RCU thread from flushing that ring at the same time: finalizing
a memory region of a freed flatview calls memory_region_transaction_begin()
and thus kvm_flush_coalesced_mmio_buffer().  kvm_state is still set at
that point, so the flush reads the ring after it has been unmapped:

  #0  kvm_flush_coalesced_mmio_buffer   accel/kvm/kvm-all.c
  #1  qemu_flush_coalesced_mmio_buffer
  #2  memory_region_transaction_begin
  #3  memory_region_finalize
  ...
  #10 flatview_destroy
  #11 call_rcu_thread

while the main thread is in:

  kvm_close -> cpu_remove_sync
  vfio_kvm_device_close
  vfio_cpr_kvm_close_notifier (MIG_EVENT_PRECOPY_DONE)

Seen as a SIGSEGV of the old QEMU during repeated cpr-exec of a VM with
a VFIO device.

Clear the ring pointer first, so that new flushes skip it, and wait for
the pending RCU callbacks before destroying the vCPUs.

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 accel/kvm/kvm-all.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 8b5d342af52..294a87043ef 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -630,6 +630,14 @@ void kvm_close(void)
         return;
     }
 
+    /*
+     * Destroying vCPUs unmaps the coalesced MMIO ring, while the RCU thread
+     * may be flushing it from a memory region finalizer.  Stop new flushes
+     * and wait for the pending RCU callbacks first.
+     */
+    kvm_state->coalesced_mmio_ring = NULL;
+    drain_call_rcu();
+
     CPU_FOREACH(cpu) {
         cpu_remove_sync(cpu);
         close(cpu->kvm_fd);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 24/34] migration/cpr: add cpr_is_in_progress()

[Andrey Drobyshev]

cpr_incoming_needed() only looks at the migration mode.  That's fine for
vmstate .needed callbacks, which run during a save or load.  But code that
runs at any time, e.g. VM state change handlers, can't tell from it
whether a CPR migration is actually going on: on the source the mode is
just a parameter, set by migrate-set-parameters whether or not a migrate
follows.

Add cpr_is_in_progress(), which is true on the target until the incoming
migration is done, and on the source only while the outgoing one runs.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 include/migration/cpr.h |  1 +
 migration/cpr.c         | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 4dd397a7c13..e73c5da1d9c 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -64,6 +64,7 @@ MigMode cpr_get_incoming_mode(void);
 void cpr_set_incoming_mode(MigMode mode);
 bool cpr_is_incoming(void);
 bool cpr_exec_is_incoming(void);
+bool cpr_is_in_progress(void);
 
 bool cpr_state_save(MigrationChannel *channel, Error **errp);
 int cpr_state_load(MigrationChannel *channel, Error **errp);
diff --git a/migration/cpr.c b/migration/cpr.c
index a149e790c10..3ea5d86bab4 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -549,6 +549,28 @@ bool cpr_exec_is_incoming(void)
     return incoming_mode == MIG_MODE_CPR_EXEC;
 }
 
+bool cpr_is_in_progress(void)
+{
+    /* The incoming mode on the target, the 'mode' parameter on the source */
+    MigMode mode = migrate_mode();
+
+    if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
+        return false;
+    }
+
+    /* Target: the incoming mode is set until the incoming migration is done */
+    if (cpr_is_incoming()) {
+        return true;
+    }
+
+    /*
+     * Source: the parameter alone means nothing, it may just have been set.
+     * An outgoing CPR migration has the CPR state open, from
+     * cpr_state_save() to cpr_state_close().
+     */
+    return cpr_state_file != NULL;
+}
+
 bool cpr_state_save(MigrationChannel *channel, Error **errp)
 {
     int ret;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 25/34] vfio/migration: don't migrate device state during CPR

[Andrey Drobyshev]

cpr-transfer and cpr-exec hand the running VFIO device over to the new
QEMU as is: the device fd, its mappings and interrupts are preserved, and
the device itself is never stopped.

But if the device supports VFIO migration, its migration handlers still
run.  The old QEMU stops the device and saves its state, then the new
QEMU puts the very same device into RESUMING and loads that state back.
On top of that, vfio_pci_load_config() re-programs MSI-X and disables
vector 0.

For an NVIDIA vGPU this breaks the device after qemu-update: restoring
the state into the live vGPU fails and the guest loses the device.
Devices without migration support aren't affected, since no migration
handlers are registered for them.

Skip VFIO migration while a CPR migration is in progress: don't save or
load the device state, and don't change the device state on VM
stop/start.  All of it is gated on cpr_is_in_progress(), not on the mode
alone: the mode may merely be set, and then a plain stop/cont must still
change the device state, and a snapshot must still save it.

Also reset multifd_transfer in vfio_multifd_cleanup(): it's only set in
vfio_multifd_setup(), which CPR now skips, so a stale value from an
earlier migration would still let the multifd save thread run.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration-multifd.c |  2 ++
 hw/vfio/migration.c         | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index e4785031a73..452f772dd7a 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -567,6 +567,8 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev)
     VFIOMigration *migration = vbasedev->migration;
 
     g_clear_pointer(&migration->multifd, vfio_multifd_free);
+    /* Decided anew by vfio_multifd_setup() of the next migration */
+    migration->multifd_transfer = false;
 }
 
 bool vfio_multifd_transfer_supported(void)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4c06e3db936..b142c9f2289 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -19,6 +19,7 @@
 #include "hw/vfio/vfio-device.h"
 #include "hw/vfio/vfio-migration.h"
 #include "migration/misc.h"
+#include "migration/cpr.h"
 #include "migration/savevm.h"
 #include "migration/vmstate.h"
 #include "migration/qemu-file.h"
@@ -513,7 +514,6 @@ static void vfio_save_cleanup(void *opaque)
     Error *local_err = NULL;
     int ret;
 
-    /* Currently a NOP, done for symmetry with load_cleanup() */
     vfio_multifd_cleanup(vbasedev);
 
     /*
@@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
     Error *local_err = NULL;
     int ret;
 
+    /*
+     * .is_active doesn't cover the non-iterable stage, so the section is
+     * still sent: leave it empty, or new QEMU would load the config into
+     * the preserved device and re-program its MSI-X.
+     */
+    if (cpr_is_in_progress()) {
+        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+        return;
+    }
+
     if (vfio_multifd_transfer_enabled(vbasedev)) {
         if (vfio_load_config_after_iter(vbasedev)) {
             qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
@@ -810,6 +820,11 @@ static bool vfio_switchover_ack_needed(void *opaque)
 {
     VFIODevice *vbasedev = opaque;
 
+    /* No device state is sent during CPR, so no ACK to wait for */
+    if (cpr_is_in_progress()) {
+        return false;
+    }
+
     return vfio_precopy_supported(vbasedev);
 }
 
@@ -824,7 +839,19 @@ static int vfio_switchover_start(void *opaque)
     return 0;
 }
 
+/*
+ * cpr-transfer and cpr-exec hand the running device over to the new QEMU
+ * as is: its fds, mappings and interrupts are preserved and it never stops.
+ * Device state migration must stay out of it, else the device is stopped,
+ * its state saved and then resumed into the very same device.
+ */
+static bool vfio_is_active(void *opaque)
+{
+    return !cpr_is_in_progress();
+}
+
 static const SaveVMHandlers savevm_vfio_handlers = {
+    .is_active = vfio_is_active,
     .save_prepare = vfio_save_prepare,
     .save_setup = vfio_save_setup,
     .save_cleanup = vfio_save_cleanup,
@@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
     Error *local_err = NULL;
     int ret;
 
+    /* The device is preserved, not a plain stop/cont: keep its state */
+    if (cpr_is_in_progress()) {
+        return;
+    }
+
     new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
                     VFIO_DEVICE_STATE_PRE_COPY_P2P :
                     VFIO_DEVICE_STATE_RUNNING_P2P;
@@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
     Error *local_err = NULL;
     int ret;
 
+    if (cpr_is_in_progress()) {
+        return;
+    }
+
     if (running) {
         new_state = VFIO_DEVICE_STATE_RUNNING;
     } else {

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 26/34] vfio/migration: don't assume the device is running on CPR incoming

[Andrey Drobyshev]

Since the device descriptor is preserved across CPR, the device keeps
whatever state the source left it in, while vfio_migration_init() on
the target still caches RUNNING.  For a VM paused before CPR, the
device is in STOP, so on 'cont' vfio_migration_set_state() finds the
cached and the requested state equal and skips the ioctl: the guest
runs with a stopped device.

On CPR incoming, cache STOP instead, and have the VM state handler,
which doesn't touch the device during CPR, note where the VM leaves
it: RUNNING once the target starts the VM, STOP if it stays paused.
At CPR the device can only be in one of the two.

Fixes: c29a65ed685 ("vfio/container: preserve descriptors")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index b142c9f2289..04d0de7cef0 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -914,6 +914,14 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
     Error *local_err = NULL;
     int ret;
 
+    /* CPR target only: the device kept its state, note where the VM left it */
+    if (cpr_is_incoming()) {
+        vbasedev->migration->device_state = running ?
+            VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
+        return;
+    }
+
+    /* CPR source only: the device keeps running across the exec */
     if (cpr_is_in_progress()) {
         return;
     }
@@ -1039,7 +1047,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
     vbasedev->migration = g_new0(VFIOMigration, 1);
     migration = vbasedev->migration;
     migration->vbasedev = vbasedev;
-    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
+    /* A device inherited across CPR is in STOP unless the VM gets started */
+    migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
+                                                  VFIO_DEVICE_STATE_RUNNING;
     migration->data_fd = -1;
     migration->mig_flags = mig_flags;
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 27/34] migration/ram: don't start dirty logging in CPR modes

[Andrey Drobyshev]

In the CPR modes guest RAM is preserved in place, not copied:
migrate_ram_is_ignored() is true for every block.  Yet ram_init_bitmaps()
still starts the global dirty log, so every listener starts tracking
pages nobody will send.  KVM write-protects all guest memory, and VFIO
starts DMA logging on the devices; with cpr-exec that logging is never
stopped before the exec, leaving e.g. the DMA page tracker of an NVIDIA
vGPU running after the update.

Don't start the dirty log for a migration that migrates no RAM, as is
already done for background snapshots.  ram_save_cleanup() only stops
the log if it was started.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/ram.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 399b43dd631..0f61942fd3d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2810,8 +2810,11 @@ static bool ram_init_bitmaps(RAMState *rs, Error **errp)
 
     WITH_RCU_READ_LOCK_GUARD() {
         ram_list_init_bitmaps();
-        /* We don't use dirty log with background snapshots */
-        if (!migrate_background_snapshot()) {
+        /*
+         * We don't use dirty log with background snapshots, nor in CPR modes,
+         * where no RAM is migrated (see migrate_ram_is_ignored()).
+         */
+        if (!migrate_background_snapshot() && !cpr_incoming_needed(NULL)) {
             ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
             if (!ret) {
                 goto out_unlock;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 28/34] VZ: vfio/cpr: don't close KVM on cpr-exec

[Andrey Drobyshev]

With VFIO devices, KVM is closed on the source when the migration
completes, so that the new QEMU can create its own KVM VM with the same
devices.  That is done from a migration notifier, which for cpr-exec
runs before cpr_exec_cb().  But cpr_exec_cb() may still give up, when
the memory can't be preserved or execve() fails, and resume the VM in
the old QEMU.  A VM without KVM can't resume: kvm_close() removes the
vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
(KVM_SET_CLOCK in kvmclock_vm_state_change()).

cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
kernel closes them during execve(), before the new QEMU runs, and
kvm_vm_release() tears down the irqfds then.  Keep the notifier for
cpr-transfer only, where the source process lives on.

Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
preserve_ram() and one with a bogus cpr-exec-command both left the VM
running with no lost pings, and a later cpr-exec completed.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/cpr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index db462aabcbf..7ca761d5bf5 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -192,13 +192,16 @@ static int vfio_cpr_kvm_close_notifier(NotifierWithReturn *notifier,
     return 0;
 }
 
+/*
+ * Not for cpr-exec: the exec closes the KVM fds anyway, and until then
+ * cpr_exec_cb() may still give up and resume the VM, which needs KVM.
+ */
 void vfio_cpr_add_kvm_notifier(void)
 {
     if (!kvm_close_notifier.notify) {
-        migration_add_notifier_modes(&kvm_close_notifier,
-                                     vfio_cpr_kvm_close_notifier,
-                                     MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-                                     -1);
+        migration_add_notifier_mode(&kvm_close_notifier,
+                                    vfio_cpr_kvm_close_notifier,
+                                    MIG_MODE_CPR_TRANSFER);
     }
 }
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 29/34] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

[Andrey Drobyshev]

cpr_exec_cb() marks all saved ramblocks with MADV_DOEXEC before exec, but
ignores the result.  If madvise() fails for some block, the walk stops
there and the remaining blocks are not marked, yet execvp() still runs.
The new QEMU then reuses addresses of guest RAM that wasn't preserved,
and the guest is lost.

Treat that as a failed cpr-exec: go to the same failure path as when
execvp() fails, so the VM keeps running in the old QEMU.

On that path also clear MADV_DOEXEC from all saved ramblocks again, just
like the preserved fds get close-on-exec back.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 5a885422154..df11b0c7633 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -157,6 +157,19 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
     return true;
 }
 
+static bool unpreserve_ram(cpr_ramblock_t ram, char *name)
+{
+#ifdef MADV_DONTEXEC
+    qemu_madvise((void *) ram.addr, ram.length, MADV_DONTEXEC);
+#endif
+    return true;
+}
+
+static void cpr_exec_unpreserve_ram(void)
+{
+    cpr_walk_ramblock(unpreserve_ram);
+}
+
 static bool mark_ram_rdonly(cpr_ramblock_t ram, char *name)
 {
     /*
@@ -205,7 +218,10 @@ static void cpr_exec_cb(void *opaque)
      */
     cpr_exec_preserve_fds();
 
-    cpr_walk_ramblock(preserve_ram);
+    if (!cpr_walk_ramblock(preserve_ram)) {
+        /* preserve_ram() has already reported the error and failed migration */
+        goto fail;
+    }
     if (cpr_ram_rdonly) {
         cpr_walk_ramblock(mark_ram_rdonly);
     }
@@ -224,18 +240,20 @@ static void cpr_exec_cb(void *opaque)
      * or the system is very short on resources.
      */
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
-    g_clear_pointer(&argv, g_strfreev);
-    cpr_exec_unpreserve_fds();
-    if (cpr_ram_rdonly) {
-        cpr_ram_restore_rw();
-    }
-
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);
     error_free(err);
     err = NULL;
 
+fail:
+    g_clear_pointer(&argv, g_strfreev);
+    cpr_exec_unpreserve_fds();
+    cpr_exec_unpreserve_ram();
+    if (cpr_ram_rdonly) {
+        cpr_ram_restore_rw();
+    }
+
     /* Note, we can go from state COMPLETED to FAILED */
     migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 30/34] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[Andrey Drobyshev]

Right before execve(), cpr_exec_cb() releases the kernel vhost-net
devices with VHOST_RESET_OWNER, so that the new QEMU can take them.  If
execve() fails, the VM goes on in the old QEMU, but nobody takes the
devices back: the kernel refuses the ioctls of the next
vhost_net_start(), and the guest is left without network.

Take them back with VHOST_SET_OWNER when the exec fails, and set the
vring call and err fds again: VHOST_RESET_OWNER dropped them too, and
vhost_dev_init() registers them only once.  The devices are stopped at
that point, and vhost_net_start() sets up the rest again when the VM
resumes.  Only the devices that were actually released are taken back:
vhost_net_cpr_reset_owners() marks them.  vhost-vsock does the same in
its CPR notifier.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/net/vhost_net.c        | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost.h |  2 ++
 include/net/vhost_net.h   |  1 +
 migration/cpr-exec.c      |  2 ++
 4 files changed, 54 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 56c697f4bcd..fae6b060c6f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -594,6 +594,55 @@ void vhost_net_cpr_reset_owners(void)
             if (r < 0 && r != -ENOSYS) {
                 warn_report("cpr: failed to reset vhost-net owner for %s: %s",
                             nc->name, strerror(-r));
+            } else if (r == 0) {
+                net->cpr_owner_reset = true;
+            }
+        }
+    }
+}
+
+/*
+ * Take the ownership back when the exec fails and the VM goes on in the
+ * old QEMU.  VHOST_RESET_OWNER has reset the kernel device, but the devices
+ * are stopped, and vhost_net_start() sets everything up again when the VM
+ * resumes.
+ */
+void vhost_net_cpr_set_owners(void)
+{
+    NetClientState *nc;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        VHostNetState *net = get_vhost_net(nc);
+
+        if (net && net->cpr_owner_reset) {
+            struct vhost_dev *dev = &net->dev;
+            int i, r = vhost_dev_set_owner(dev);
+
+            if (r < 0) {
+                warn_report("cpr: failed to set vhost-net owner for %s: %s",
+                            nc->name, strerror(-r));
+                continue;
+            }
+            net->cpr_owner_reset = false;
+
+            /* RESET_OWNER also dropped the vring fds set by vhost_dev_init() */
+            for (i = 0; i < dev->nvqs; i++) {
+                const VhostOps *ops = dev->vhost_ops;
+                struct vhost_virtqueue *vq = &dev->vqs[i];
+                struct vhost_vring_file file = {
+                    .index = ops->vhost_get_vq_index(dev, dev->vq_index + i),
+                    .fd = event_notifier_get_wfd(&vq->masked_notifier),
+                };
+
+                r = ops->vhost_set_vring_call(dev, &file);
+                if (r == 0 && ops->vhost_set_vring_err) {
+                    file.fd = event_notifier_get_fd(&vq->error_notifier);
+                    r = ops->vhost_set_vring_err(dev, &file);
+                }
+                if (r < 0) {
+                    warn_report("cpr: failed to set vring fds of %s: %s",
+                                nc->name, strerror(-r));
+                }
             }
         }
     }
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a92439d3443..421f7139cfb 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -151,6 +151,8 @@ struct vhost_net {
     SaveAcketFeatures *save_acked_features;
     bool is_vhost_user;
     NetClientState *nc;
+    /* released for cpr-exec, take back if it fails */
+    bool cpr_owner_reset;
 };
 
 /**
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 39ce7ad0e3d..cdfefa81500 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -90,4 +90,5 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
 void vhost_net_save_acked_features(NetClientState *nc);
 
 void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_set_owners(void);
 #endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index df11b0c7633..e4ea160698c 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -253,6 +253,8 @@ fail:
     if (cpr_ram_rdonly) {
         cpr_ram_restore_rw();
     }
+    /* Still the same mm, take back the vhost-net devices we released */
+    vhost_net_cpr_set_owners();
 
     /* Note, we can go from state COMPLETED to FAILED */
     migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 31/34] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released

[Andrey Drobyshev]

vhost_net_cpr_reset_owners() only warns when VHOST_RESET_OWNER fails for
a device, and the exec goes on.  But the new QEMU can't take such a
device: it is still owned by the old mm, so VHOST_SET_OWNER fails with
EBUSY, and the guest is left without network after the update.

Stop at the first device that can't be released and abort the cpr-exec
instead.  The VM goes on in the old QEMU, with the devices released so
far taken back.  vhost-vsock already does that in its pre_save.

Fixes: 1278e4830052 ("VZ: migration/cpr-exec: vhost-net: send RESET_OWNER before cpr-exec")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/net/vhost_net.c      | 19 +++++++++++++------
 include/net/vhost_net.h |  2 +-
 migration/cpr-exec.c    |  7 ++++++-
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index fae6b060c6f..9110dc43d23 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -579,8 +579,12 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
  * EBUSY.  Called right before execve(), when the VM is stopped and the
  * devices have already been stopped, so nothing is in flight.  The device is
  * fully re-initialized from the migrated state by the new QEMU.
+ *
+ * Stops at the first device that can't be released, since the new QEMU
+ * couldn't take it.  The caller then takes back the ones already released
+ * with vhost_net_cpr_set_owners().
  */
-void vhost_net_cpr_reset_owners(void)
+void vhost_net_cpr_reset_owners(Error **errp)
 {
     NetClientState *nc;
 
@@ -591,12 +595,15 @@ void vhost_net_cpr_reset_owners(void)
             net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
             int r = vhost_dev_reset_owner(&net->dev);
 
-            if (r < 0 && r != -ENOSYS) {
-                warn_report("cpr: failed to reset vhost-net owner for %s: %s",
-                            nc->name, strerror(-r));
-            } else if (r == 0) {
-                net->cpr_owner_reset = true;
+            if (r == -ENOSYS) {
+                continue;
+            }
+            if (r < 0) {
+                error_setg_errno(errp, -r, "cpr: failed to reset vhost-net "
+                                 "owner for %s", nc->name);
+                return;
             }
+            net->cpr_owner_reset = true;
         }
     }
 }
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index cdfefa81500..679edefa1d3 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -89,6 +89,6 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
 
 void vhost_net_save_acked_features(NetClientState *nc);
 
-void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_reset_owners(Error **errp);
 void vhost_net_cpr_set_owners(void);
 #endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index e4ea160698c..b41dcc46494 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -230,7 +230,10 @@ static void cpr_exec_cb(void *opaque)
      * Hand the preserved vhost-net devices over to the new QEMU: they are
      * still owned by this mm, which is about to be replaced by execve().
      */
-    vhost_net_cpr_reset_owners();
+    vhost_net_cpr_reset_owners(&err);
+    if (err) {
+        goto fail_with_err;
+    }
 
     trace_cpr_exec();
     execvp(argv[0], argv);
@@ -240,6 +243,8 @@ static void cpr_exec_cb(void *opaque)
      * or the system is very short on resources.
      */
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
+
+fail_with_err:
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 32/34] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[Andrey Drobyshev]

After cpr-exec, new QEMU mmaps the device BARs again on the preserved
device fd.  Some drivers don't allow that: the NVIDIA vGPU VFIO driver
refuses to map a VF BAR that is still recorded as mapped by old QEMU,
and that record is dropped only when the device fd is closed.  So BAR1
and BAR3 of the vGPU can't be mapped, and the guest loses the device.

Keep the mappings instead.  Save each BAR mapping in the CPR ramblock
list, so cpr_exec_cb() marks it with MADV_DOEXEC together with guest RAM,
and new QEMU finds it there and reuses it instead of calling mmap().
Same find-or-create pattern as for other preserved objects, e.g. vector
eventfds in vfio_notifier_init().  No saved mapping means old QEMU didn't
map the region, e.g. its mmap failed or the region is mapped lazily, so
new QEMU maps it afresh.

The ID is "<device name> region <index> mmaps[<index>]".  It doesn't
depend on the device path, which is gone when the display region is
finalized, so the entry is dropped together with the mapping.

The saved addresses only make sense for cpr-exec, which keeps the
address space.  A cpr-transfer target is another process and would have
to map the regions anew, which the drivers above refuse while the old
mappings exist.  So block cpr-transfer for VFIO devices.

This needs a kernel which supports MADV_DOEXEC on shared PFN mappings of
device memory.  Without it cpr-exec of a VM with VFIO devices fails.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/device.c           | 24 +++++++++++++++++++++--
 hw/vfio/region.c           | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-cpr.h |  1 +
 3 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 5a14a7d5c0a..2a7073ccec8 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
                              TYPE_VFIO_IOMMU_IOMMUFD :
                              TYPE_VFIO_IOMMU_LEGACY;
 
-    return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
-                                            as, errp);
+    /*
+     * Only cpr-exec keeps the address space and so the region mappings,
+     * see vfio_region_mmap().  A cpr-transfer target would have to map the
+     * regions anew, which some drivers (NVIDIA vGPU) refuse.
+     */
+    error_setg(&vbasedev->cpr.mmap_blocker,
+               "cpr-transfer is not supported with VFIO devices; "
+               "use the cpr-exec mode");
+    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
+                                  MIG_MODE_CPR_TRANSFER, -1)) {
+        return false;
+    }
+
+    if (!vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
+                                          as, errp)) {
+        migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+        return false;
+    }
+
+    return true;
 }
 
 void vfio_device_detach(VFIODevice *vbasedev)
 {
+    migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+
     if (!vbasedev->bcontainer) {
         return;
     }
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index b0332f27fcd..86a34b42c57 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -30,6 +30,7 @@
 #include "qemu/units.h"
 #include "monitor/monitor.h"
 #include "system/ramblock.h"
+#include "migration/cpr.h"
 #include "vfio-helpers.h"
 
 /*
@@ -227,8 +228,47 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
     return 0;
 }
 
+static char *vfio_region_cpr_name(VFIORegion *region, int index)
+{
+    return g_strdup_printf("%s region %d mmaps[%d]", region->vbasedev->name,
+                           region->nr, index);
+}
+
+/*
+ * A saved mapping is one preserved by cpr-exec, reuse it.  None means old
+ * QEMU didn't map the region, e.g. its mmap failed or it's mapped lazily,
+ * so map it afresh.
+ */
+static void *vfio_region_cpr_find(VFIORegion *region, int index)
+{
+    g_autofree char *name = vfio_region_cpr_name(region, index);
+    cpr_ramblock_t ram = cpr_find_ramblock(name);
+
+    if (ram.addr && ram.length != region->mmaps[index].size) {
+        error_report("CPR mode=exec is requested, but the mapping of %s "
+                     "can't be reused: addr=%p length=%zu, expected "
+                     "length=%zu. Aborting", name, (void *)ram.addr,
+                     ram.length, region->mmaps[index].size);
+        exit(1);
+    }
+
+    return (void *)(uintptr_t)ram.addr;
+}
+
+static void vfio_region_cpr_save(VFIORegion *region, int index)
+{
+    g_autofree char *name = vfio_region_cpr_name(region, index);
+    void *addr = region->mmaps[index].mmap;
+    size_t size = region->mmaps[index].size;
+
+    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
+}
+
 static void vfio_subregion_unmap(VFIORegion *region, int index)
 {
+    g_autofree char *cpr_name = vfio_region_cpr_name(region, index);
+
+    cpr_delete_ramblock(cpr_name);
     trace_vfio_region_unmap(memory_region_name(&region->mmaps[index].mem),
                             region->mmaps[index].offset,
                             region->mmaps[index].offset +
@@ -314,6 +354,11 @@ int vfio_region_mmap(VFIORegion *region)
         size_t align = MIN(1ULL << ctz64(region->mmaps[i].size), 1 * GiB);
         void *map_base, *map_align;
 
+        region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
+        if (region->mmaps[i].mmap) {
+            goto mapped;
+        }
+
         /*
          * Align the mmap for more efficient mapping in the kernel.  Ideally
          * we'd know the PMD and PUD mapping sizes to use as discrete alignment
@@ -347,7 +392,9 @@ int vfio_region_mmap(VFIORegion *region)
             ret = -errno;
             goto no_mmap;
         }
+        vfio_region_cpr_save(region, i);
 
+mapped:
         name = g_strdup_printf("%s mmaps[%d]",
                                memory_region_name(region->mem), i);
         memory_region_init_ram_device_ptr(&region->mmaps[i].mem,
@@ -427,6 +474,8 @@ void vfio_region_finalize(VFIORegion *region)
 
     for (i = 0; i < region->nr_mmaps; i++) {
         if (region->mmaps[i].mmap) {
+            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
+            cpr_delete_ramblock(cpr_name);
             munmap(region->mmaps[i].mmap, region->mmaps[i].size);
         }
     }
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index 4606da500a7..fa1699857ca 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -34,6 +34,7 @@ typedef struct VFIOContainerCPR {
 typedef struct VFIODeviceCPR {
     Error *mdev_blocker;
     Error *id_blocker;
+    Error *mmap_blocker;
     uint32_t hwpt_id;
     uint32_t ioas_id;
 } VFIODeviceCPR;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 33/34] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

[Andrey Drobyshev]

Commit bb8da93ddfc4 ("VZ: migration/cpr-exec: enable checks prohibiting
cpr-exec #VSTOR-108564") introduced restrictions for cpr-exec migration
mode.  Among them was prohibition to do such migration in case of
presence of any VFIO devices.  Now we want to allow migration with such
devices in scope of VSTOR-103860.  Therefore, let's lift that restiction.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/device.c              | 5 -----
 include/hw/vfio/vfio-device.h | 2 --
 migration/migration.c         | 3 ---
 3 files changed, 10 deletions(-)

diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 2a7073ccec8..d1b595adb71 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -36,11 +36,6 @@
 VFIODeviceList vfio_device_list =
     QLIST_HEAD_INITIALIZER(vfio_device_list);
 
-bool vfio_devices_present(void)
-{
-    return !QLIST_EMPTY(&vfio_device_list);
-}
-
 /*
  * We want to differentiate hot reset of multiple in-use devices vs
  * hot reset of a single in-use device. VFIO_DEVICE_RESET will already
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 935000f0074..fc28f580ba3 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -162,8 +162,6 @@ VFIODevice *vfio_get_vfio_device(Object *obj);
 typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
 extern VFIODeviceList vfio_device_list;
 
-bool vfio_devices_present(void);
-
 #ifdef CONFIG_LINUX
 /*
  * How devices communicate with the server.  The default option is through
diff --git a/migration/migration.c b/migration/migration.c
index 821764afae9..57aa0dea264 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2115,9 +2115,6 @@ static bool migrate_cpr_is_blocked(Error **errp)
     } else if (cbw_filter_present()) {
         error_setg(errp, "CPR is prohibited: active block snapshots (CBW "
                    "filter) present");
-    } else if (vfio_devices_present()) {
-        error_setg(errp, "CPR is prohibited: passthrough (VFIO) "
-                   "devices present");
     } else {
         res = false;
     }

-- 
2.47.1

[PATCH QEMU HCI-8.1 v6 34/34] VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

[Andrey Drobyshev]

A VFIO device reports the state it hands over once the guest is stopped
as pending data, and the migration switches over only when all pending
data fits into bandwidth x downtime-limit.  That state can't get any
smaller by iterating, so when it alone doesn't fit, the migration never
converges until the downtime limit is raised, which Nova does in steps
of minutes.  An NVIDIA vGPU reports 1.5-3 GiB of it, which never fits
the default limits.

Report the stop-copy size as pending only while the device still has
something to pre-copy.  Once it has drained, the rest of the pending
data, which iterating can shrink, decides the switchover on its own.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 04d0de7cef0..eff1172569d 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -569,16 +569,24 @@ static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
     uint64_t stop_copy_size = VFIO_MIG_STOP_COPY_SIZE;
+    int ret;
 
     /*
      * If getting pending migration size fails, VFIO_MIG_STOP_COPY_SIZE is
      * reported so downtime limit won't be violated.
      */
-    vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
-    *must_precopy += stop_copy_size;
+    ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
 
     if (vfio_device_state_is_precopy(vbasedev)) {
-        vfio_query_precopy_size(migration);
+        ret |= vfio_query_precopy_size(migration);
+    }
+
+    /*
+     * Once there's nothing left to pre-copy, the stop-copy state can't
+     * shrink by iterating: don't let it hold up the switchover.
+     */
+    if (ret || migration->precopy_init_size || migration->precopy_dirty_size) {
+        *must_precopy += stop_copy_size;
     }
 
     trace_vfio_state_pending_exact(vbasedev->name, *must_precopy, *can_postcopy,

-- 
2.47.1

Re: [PATCH QEMU HCI-8.1 v6 01/34] VZ: migration/cpr-exec: fix Error leak in preserve_ram()

[no-reply]

> migrate_set_error() stores its own copy of the error, so free ours.
> 
> Fixes: a3745506daf9 ("VZ: migration/cpr-exec: mark saved RAM blocks with MADV_DOEXEC before execve")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=1

Re: [PATCH QEMU HCI-8.1 v6 02/34] migration/cpr-exec: restore RAM write access if exec fails

[no-reply]

> With -cpr-ram-rdonly the RAM blocks are made read-only right before
> execve().  If execve() fails, the VM resumes in the old QEMU with
> read-only RAM and crashes on the first write.  Make it writable again.
> 
> Fixes: 91da25f20471 ("migration/cpr-exec: add '-cpr-ram-rdonly' option to mark RAM blocks as RO #VSTOR-101579")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=2

Re: [PATCH QEMU HCI-8.1 v6 03/34] vfio/region: create BAR dma-bufs with O_CLOEXEC

[no-reply]

> The dma-buf fds are created without O_CLOEXEC, so they leak into any
> exec'ed program.  With cpr-exec, every update leaks one fd per mmapped
> BAR, as new QEMU creates its own.
> 
> Fixes: 96163e0dbcb4 ("hw/vfio/region: Create dmabuf for PCI BAR per region")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=3

Re: [PATCH QEMU HCI-8.1 v6 05/34] migration: close the CPR state if the outgoing channel fails to open

[no-reply]

> qmp_migrate() saves the CPR state before it opens the migration
> channel.  If the channel is a file or an exec command and it can't be
> opened, qmp_migrate_finish() returns right away and migration_cleanup()
> never runs, so the CPR state file stays open.
> 
> Close it there, next to the yank instance, which has the same problem.
> 
> Fixes: 624e6e654e11 ("migration: cpr-transfer mode")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=5

Re: [PATCH QEMU HCI-8.1 v6 04/34] migration/cpr-exec: require a file or exec migration URI

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: cpr-exec URI check also rejects incoming socket/fd/rdma URIs, undocumented
--

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

migration/cpr-exec: require a file or exec migration URI

This commit makes migration_channels_and_transport_compatible() refuse
cpr-exec mode unless the migration URI is a file or exec type. It also
updates CPR.rst to say that other URI types are refused.

> diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
> --- a/docs/devel/migration/CPR.rst
> +++ b/docs/devel/migration/CPR.rst

[ ... ]

> @@ -365,10 +365,10 @@ The VM must be started with the ``-machine aux-ram-share=on`` option.
>  Outgoing:
>    * Set the migration mode parameter to ``cpr-exec``.
>    * Set the ``cpr-exec-command`` parameter.
> -  * Issue the ``migrate`` command.  It is recommended that the URI be
> -    a ``file`` type, but one can use other types such as ``exec``,
> -    provided the command captures all the data from the outgoing side,
> -    and provides all the data to the incoming side.
> +  * Issue the ``migrate`` command.  The URI must be a ``file`` type,
> +    or an ``exec`` type, provided the command captures all the data
> +    from the outgoing side, and provides all the data to the incoming
> +    side.  Other types are refused.
>  
>  Incoming:
>    * You do not need to explicitly start new QEMU.  It is started as

[Severity: Low]
The restriction is documented only under Outgoing, but the check is also
applied on the incoming side.  Should the Incoming section mention it as
well?

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
>          return false;
>      }
>  
> +    if (migrate_mode() == MIG_MODE_CPR_EXEC &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
> +        error_setg(errp,
> +                   "cpr-exec mode requires a file or exec migration URI");
> +        return false;
> +    }
> +
>      return true;
>  }

[Severity: Low]
Is it intended that this check also applies to incoming migration?  The
commit message, the CPR.rst text and the error string describe only the
outgoing side.

migration_channels_and_transport_compatible() is shared with the incoming
path:

qemu_start_incoming_migration()
  migration_transport_compatible()
    migration_channels_and_transport_compatible()

qmp_migrate_incoming() reaches the same helper.

migrate_mode() returns cpr_get_incoming_mode() first, so in the exec'd
QEMU, after cpr_state_load() has set the incoming mode, it returns
cpr-exec:

MigMode migrate_mode(void)
{
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

When the mode is cpr-exec, any incoming URI that is not file or exec is
now refused.  That includes tcp, unix, vsock, rdma and fd:.  An fd: URI
always parses as MIGRATION_ADDRESS_TYPE_SOCKET, even when the descriptor
refers to a regular file.

A plain destination with the mode parameter set to cpr-exec is affected as
well.

Could the commit message and documentation describe the incoming-side
behavior?  Alternatively, should the check be limited to the outgoing path?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=4

Re: [PATCH QEMU HCI-8.1 v6 06/34] migration/cpr-exec: close the persisted state if the channel fails to open

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration/cpr-exec: val used after g_unsetenv() in cpr_exec_unpersist_state()
--

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

migration/cpr-exec: close the persisted state if the channel fails to open

This patch makes qmp_migrate_finish() call cpr_exec_unpersist_state() when
the outgoing channel fails to open. That releases the dup'd memfd and the
environment variable that cpr_exec_persist_state() created. It also makes
cpr_exec_unpersist_state() return early when the variable isn't set.

> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c

[ ... ]

> @@ -83,8 +83,10 @@ void cpr_exec_unpersist_state(void)
>      int mfd;
>      const char *val = g_getenv(CPR_EXEC_STATE_NAME);
>  
> +    if (!val) {
> +        return;
> +    }
>      g_unsetenv(CPR_EXEC_STATE_NAME);
> -    assert(val);
>      assert(!qemu_strtoi(val, NULL, 10, &mfd));
>      close(mfd);
>  }

[Severity: Low]
Is it safe to parse val after g_unsetenv() has removed the entry?

In cpr_exec_unpersist_state(), val comes from g_getenv() and points into
the environment string created by g_setenv() in cpr_exec_persist_state():

    const char *val = g_getenv(CPR_EXEC_STATE_NAME);
    ...
    g_unsetenv(CPR_EXEC_STATE_NAME);
    assert(!qemu_strtoi(val, NULL, 10, &mfd));

GLib documents that the string returned by g_getenv() may be invalidated
by a later g_setenv() or g_unsetenv(). On a libc that frees setenv()
allocated entries in unsetenv(), such as musl, qemu_strtoi() would read
freed memory. The result could be an assert failure or a wrong fd passed
to close(). On glibc the string is only unlinked, so it works there.

This ordering was already in cpr_exec_unpersist_state() and in
cpr_exec_find_state(). The new call in qmp_migrate_finish() runs in the
process that called g_setenv(), which is the case where the string is
heap-allocated. In cpr_exec_find_state() the entry comes from the
inherited environment block after exec(), so it is much less likely to be
freed.

I did not check the musl source, so this is speculative.

Could val be parsed into mfd, or copied with g_strdup(), before calling
g_unsetenv()? The same reordering would apply to cpr_exec_find_state().

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2413,6 +2413,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
>          cpr_state_close();
> +        cpr_exec_unpersist_state();
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=6

Re: [PATCH QEMU HCI-8.1 v6 07/34] migration: don't refuse a blocker over a migration of another mode

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: is_busy() infers running migration mode from mutable migrate_mode()
--

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

migration: don't refuse a blocker over a migration of another mode

This patch changes is_busy() in migration/migration.c so that a running
migration only causes a migration blocker to be refused if the blocker
applies to the running migration's mode. Snapshots (RUN_STATE_SAVE_VM)
are still checked as before.

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> -static bool is_busy(Error **reasonp, Error **errp)
> +static bool is_busy(Error **reasonp, Error **errp, int modes)
>  {
>      ERRP_GUARD();
>  
> -    /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> -    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
> +    /*
> +     * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
> +     * A running migration only matters if the blocker applies to its mode.
> +     */
> +    if (runstate_check(RUN_STATE_SAVE_VM) ||
> +        (migration_is_running() && (modes & BIT(migrate_mode())))) {

[Severity: Low]
Can this test be wrong while a migration is in flight? is_busy() infers
the mode of the running migration from migrate_mode(), which is not a
value recorded when the migration started:

migration/options.c:migrate_mode() {
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

The parameters.mode value can be changed at any time through
qmp_migrate_set_parameters()->migrate_params_apply():

    if (params->has_mode) {
        s->parameters.mode = params->mode;
    }

Unlike qmp_migrate_set_capabilities(), which rejects changes with
"There's a migration process in progress" when migration_is_running() is
true, I could not find a similar guard on the parameters path.

Suppose a normal migration is running and mode is then set to
cpr-transfer. A blocker for MIG_MODE_NORMAL only would compute
BIT(MIG_MODE_NORMAL) & BIT(MIG_MODE_CPR_TRANSFER) == 0 and be accepted
while the normal migration is still running. The reverse case would
wrongly refuse a blocker for the newly set mode.

Also, migrate_mode() returns cpr_get_incoming_mode() when it is set. That
describes the incoming migration, whereas migration_is_running() checks the
outgoing current_migration state. Can the two describe different
migrations here?

Would it be better to record the mode in MigrationState in migrate_init()
(or migrate_prepare()) and test that value in is_busy(), or to reject mode
changes while migration_is_running()?

>          error_propagate_prepend(errp, *reasonp,
>                                  "disallowing migration blocker "
>                                  "(migration/snapshot in progress) for: ");

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=7

Re: [PATCH QEMU HCI-8.1 v6 08/34] migration: Plug memory leaks after migrate_set_error()

[no-reply]

> From: Markus Armbruster <armbru@redhat.com>
> 
> migrate_set_error(s, err) stores a copy of @err in @s.  The original
> @err is not freed.  Most callers free it immediately.  Some callers
> free it later, or pass it on.  And some leak it.  Fix those.
> 
> Perhaps migrate_set_error(s, err) should take ownership of @err.  The
> callers that free it immediately would become simpler, and avoid a
> copy and a deallocation.  The others would have to pass
> error_copy(err).
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit 93817ec39656.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Link: https://lore.kernel.org/r/20251115083500.2753895-2-armbru@redhat.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=8

Re: [PATCH QEMU HCI-8.1 v6 09/34] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

[no-reply]

> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> While working on savevm/loadvm for a new vfio device, I encountered the
> crash below. Since vfio_connect_kvm_msi_virq() didn't check the ->use flag
> for the vector, it would pass an unused vector down to
> vfio_cpr_load_vector_fd() which would crash.
> 
> Fix this by checking the ->use flag along with the virq number to detect
> whether a vector is valid or not.
> 
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x0000555555a891ef in vfio_cpr_load_vector_fd (vdev=vdev@entry=0x0,
>     name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1) at ../hw/vfio/cpr.c:44
> 44	    g_autofree char *fdname = STRDUP_VECTOR_FD_NAME(vdev, name);
> (gdb) bt
> [ ... ]
> Fixes: 30edcb4d4e7a ("vfio-pci: preserve MSI")
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Link: https://lore.kernel.org/qemu-devel/20260727150038.2684512-1-tycho@kernel.org
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=9

Re: [PATCH QEMU HCI-8.1 v6 10/34] vfio/pci: Reject invalid MSI-X Table and PBA BIR values

[no-reply]

> From: Cédric Le Goater <clg@redhat.com>
> 
> The 3-bit MSI-X BIR fields permit values 0-7, but VFIOPCIDevice::bars[]
> only contains BAR0-BAR5 (6 entries). An invalid BIR value of 6 or 7
> can cause an out-of-bounds array access (CWE-129) in vfio_msix_early_setup().
> 
> Add range checks immediately after extracting the BIR values.
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit 9daa7347ff25.
> 
> Reported-by: Feifan Qian <bea1e@proton.me>
> Fixes: 65501a745dba ("vfio: vfio-pci device assignment driver")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3878
> Reviewed-by: Alex Williamson <alex@shazbot.org>
> Link: https://lore.kernel.org/qemu-devel/20260706161334.2165482-1-clg@redhat.com
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=10

Re: [PATCH QEMU HCI-8.1 v6 11/34] vfio/pci: Fix information leak in vfio_rom_read()

[no-reply]

> From: Cédric Le Goater <clg@redhat.com>
> 
> vfio_rom_read() can leak uninitialized stack data to the guest when a
> read occurs at the end boundary of the ROM BAR.
> 
> Fix this by initializing the val union to 0xff, matching the ROM
> initialization pattern used in vfio_pci_load_rom().
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit 402fd5a75659.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@mailo.com>
> Reviewed-by: Mario Casquero <mcasquer@redhat.com>
> Link: https://lore.kernel.org/qemu-devel/20260602074446.1206892-1-clg@redhat.com
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=11

Re: [PATCH QEMU HCI-8.1 v6 13/34] vfio/pci: Grow buffer in vfio_pci_host_match()

[no-reply]

> From: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> 
> Each field of PCIHostDeviceAddress is an unsigned int, therefore
> while a valid address is limited to 13 characters, an invalid
> address could exceed the specified format, up to:
> 
>         ffffffff:ffffffff:ffffffff.ffffffff<NUL>
> 
> This requires 36 characters with the terminator.
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit 1785bf9685ec.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>
> Message-ID: <20260305-nvme-v4-2-b65b9de1839f@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=13

Re: [PATCH QEMU HCI-8.1 v6 12/34] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

[no-reply]

> From: Mario Casquero <mcasquer@redhat.com>
> 
> When vfio_device_get_region_info() fails in vfio_pci_load_rom(),
> the function returns without setting vdev->rom_read_failed to true,
> and without allocating vdev->rom. This leaves vdev->rom as NULL.
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit c822ecd80661.
> 
> Signed-off-by: Mario Casquero <mcasquer@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Link: https://lore.kernel.org/qemu-devel/20260602070857.356526-1-mcasquer@redhat.com
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=12

Re: [PATCH QEMU HCI-8.1 v6 15/34] migration: ensure APIC is loaded prior to VFIO PCI devices

[no-reply]

> From: Yanfei Xu <yanfei.xu@bytedance.com>
> 
> The load procedure of VFIO PCI devices involves setting up IRT
> for each VFIO PCI devices. This requires determining whether an
> interrupt is single-destination interrupt to decide between
> Posted Interrupt(PI) or remapping mode for the IRTE. However,
> determining this may require accessing the VM's APIC registers.
> 
> For example:
> ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irqs)
>   ...
>     kvm_arch_irq_bypass_add_producer
>       kvm_x86_call(pi_update_irte)
>         vmx_pi_update_irte
>           kvm_intr_is_single_vcpu
> 
> If the LAPIC has not been loaded yet, interrupts will use remapping
> mode. To prevent the fallback of interrupt mode, keep APIC is always
> loaded prior to VFIO PCI devices.
> 
> andrey.drobyshev:
>     This commit is cherry-picked as-is, with no modifications, from the
>     upstream commit d943cef76090.
> 
> Signed-off-by: Yicong Shen <shenyicong.1023@bytedance.com>
> Signed-off-by: Yanfei Xu <yanfei.xu@bytedance.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Link: https://lore.kernel.org/r/20250818131127.1021648-1-yanfei.xu@bytedance.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=15

Re: [PATCH QEMU HCI-8.1 v6 14/34] q35: Fix migration of SMRAM state

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] q35: pre-negotiation writes to SMBASE reg can lock it and enable SMRAM blackhole
--

commit e599128e44cfa63b5f0a0828dcf6845ef880af50
Author: Igor Mammedov <imammedo@redhat.com>

q35: Fix migration of SMRAM state

This change drops the wmask == 0xff discard in mch_update_smbase_smram() and
resyncs wmask from the SMBASE config register value, so a locked SMRAM state
survives migration. It also removes the pre-negotiation junk-write check
from test_smram_smbase_lock().

Link: https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c

[ ... ]

> @@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
>      }
>  
>      if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> -            MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
>          return;
>      }
>  
>      /*
> -     * default/reset state, discard written value
> -     * which will disable SMRAM balackhole at SMBASE
> +     * reg value can come from register write/reset/migration source,
> +     * update wmask to be in sync with it regardless of source
>       */
> -    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> -        *reg = 0x00;
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        return;
>      }
> -
> -    memory_region_transaction_begin();
>      if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
> -        /* disable all writes */
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
> -            ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        /* lock register at 0x2 and disable all writes */
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> -        lck = true;
> -    } else {
> -        lck = false;
>      }
> +
> +    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +    memory_region_transaction_begin();
>      memory_region_set_enabled(&mch->smbase_blackhole, lck);
>      memory_region_set_enabled(&mch->smbase_window, lck);
>      memory_region_transaction_commit();

[Severity: Low]
Can a guest lock the SMBASE register without negotiating first?

mch_reset() sets config[MCH_HOST_BRIDGE_F_SMBASE] to 0 and
wmask[MCH_HOST_BRIDGE_F_SMBASE] to 0xff, so after reset every bit of the
register is writable.

A guest config write to offset 0x9c then goes through this path:

pci_default_write_config()
  mch_write_config()
    mch_update_smbase_smram()

The old "wmask == 0xff" check used to set *reg = 0x00 here.  With that
check removed, a written value with bit 0x2 set reaches the
MCH_HOST_BRIDGE_F_SMBASE_LCK branch with no 0xff query step first.  That
branch sets wmask to 0 and *reg to 0x02, and then enables smbase_blackhole
and smbase_window.

A write of exactly 0x01 reaches the MCH_HOST_BRIDGE_F_SMBASE_IN_RAM branch
and sets wmask to MCH_HOST_BRIDGE_F_SMBASE_LCK, also without negotiation.

Can this make the 128K of RAM at the default SMBASE become inaccessible to
the guest CPU address space until the next reset, for a guest that never
negotiated the protocol?  For example, a stray dword config write covering
0x9c could trigger it.

The commit message acknowledges junk writes are accepted ("it would let
guest write junk values in register ... until it's stumbles upon reserved
0x1 [|] 0x2 values").  Is this behavior change intended, given that the
migration fix needs wmask to be derived from the register value?  No later
patch in the series changes mch_update_smbase_smram() or the SMBASE wmask
handling.

> diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
> --- a/tests/qtest/q35-test.c
> +++ b/tests/qtest/q35-test.c
> @@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
>      qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
>      g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
>  
> -    /* check that writing junk to 0x9c before before negotiating is ignored */
> -    for (i = 0; i < 0xff; i++) {
> -        qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
> -        g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
> -    }
> -

Since this junk-write check was removed from test_smram_smbase_lock(), is
there any test that covers the junk-then-lock path, where a pre-negotiation
write with bit 0x2 set enables the SMRAM blackhole?

>      /* enable SMRAM at SMBASE */
>      qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
>      g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=14

Re: [PATCH QEMU HCI-8.1 v6 16/34] migration: Fix regression of passing error_fatal into vmstate_load_state()

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: void vmstate save helpers log failure but do not fail the stream
--

commit d292a4854a94c2740197418260667f472e5aac86
Author: Arun Menon <armenon@redhat.com>

migration: Fix regression of passing error_fatal into vmstate_load_state()

This replaces error_fatal in several vmstate_save_state() and
vmstate_load_state() callers with a local Error object. Failures are now
reported with error_report_err() and, where the function returns an int,
propagated to the caller. This is a cherry-pick of upstream commit
986c3292c6b8.

Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com

> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c

[ ... ]

>  void pci_device_save(PCIDevice *s, QEMUFile *f)
>  {
> +    Error *local_err = NULL;
> +    int ret;
> +
>      /* Clear interrupt status bit: it is implicit
>       * in irq_state which we are saving.
>       * This makes us compatible with old devices
>       * which never set or clear this bit. */
>      s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> -    vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal);
> +    ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
>      /* Restore the interrupt status bit. */
>      pci_update_irq_status(s);
>  }

[Severity: Low]
Can a vmstate_save_state() failure here go unnoticed by the migration
core? pci_device_save() is void, so on ret < 0 the error is only printed
and the function carries on to pci_update_irq_status().

Looking at vmstate_save_state_v() in migration/vmstate.c, the failure
paths only call error_setg() and return ret:

    if (ret) {
        error_setg(errp, "Save of field %s/%s failed",
                    vmsd->name, field->name);
        if (vmsd->post_save) {
            vmsd->post_save(opaque);
        }
        return ret;
    }

It never calls qemu_file_set_error() on the QEMUFile. Before this patch,
error_fatal made QEMU exit(1) in this case. Now the save continues and the
stream is not marked failed.

Could the destination then receive truncated or inconsistent device state?
Would it make sense to call qemu_file_set_error(f, ret) in these void
callers?

The same applies to the other void callers changed here:
virtio_ccw_save_config(), virtio_mmio_save_extra_state(), and
virtio_pci_save_extra_state(). In the last one, virtio_save() keeps
writing the stream after the failure because the save_extra_state callback
cannot report one.

[ ... ]

> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c

[ ... ]

>  static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq)
>  {
>      vscsi_req *req = sreq->hba_private;
> -    assert(req->active);
> +    Error *local_err = NULL;
> +    int rc;
>  
> -    vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal);
> +    assert(req->active);
>  
> +    rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
> +    if (rc < 0) {
> +        error_report_err(local_err);
> +        return;
> +    }
>      trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
>                                     req->cur_desc_offset);
>  }

[Severity: Low]
Similarly, in vscsi_save_request(), the failure is reported and then
swallowed with a bare return. Can the request end up partly or wholly
missing from the stream while the migration is not failed?

The later series patch "migration: vmstate_save_state_v: fix double
error_setg" only changes the error message text. It does not set an error
on the QEMUFile, so this still looks present at the end of the series.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=16

Re: [PATCH QEMU HCI-8.1 v6 17/34] migration: vmstate_save_state_v: fix double error_setg

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() sets Error only for ret < 0 but error_prepend() runs for any ret != 0
--

commit c242dc6e98ee351214e7cbc0d990ad8f5b090974
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

migration: vmstate_save_state_v: fix double error_setg

This patch avoids calling error_setg() twice on the same errp when a nested
vmstate_save_state() or vmstate_save_state_v() fails. It sets an Error only
when put() returns a negative value, and it changes the common failure
branch to use error_prepend().

Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index aea41c099463..04903131257b 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

[ ... ]

> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else {
>                      ret = inner_field->info->put(f, curr_elem, size,
>                                                   inner_field, vmdesc_loop);
> +                    if (ret < 0) {
> +                        error_setg(errp, "put failed");
> +                    }
>                  }

[ ... ]

> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> +                    error_prepend(errp, "Save of field %s/%s failed: ",
> +                                  vmsd->name, field->name);

[Severity: Medium]
Can this error_prepend() run with *errp still NULL? The put() branch sets
an Error only when ret < 0, but this branch is taken for any non-zero ret:

    ret = inner_field->info->put(f, curr_elem, size,
                                 inner_field, vmdesc_loop);
    if (ret < 0) {
        error_setg(errp, "put failed");
    }
    ...
    if (ret) {
        error_prepend(errp, "Save of field %s/%s failed: ", ...);

If put() returns a positive value, error_setg() is skipped. error_prepend()
then calls error_vprepend(), which checks only for a NULL errp and not a
NULL *errp:

    if (!errp) {
        return;
    }
    newmsg = g_string_new(NULL);
    g_string_vprintf(newmsg, fmt, ap);
    g_string_append(newmsg, (*errp)->msg);

That would dereference a NULL Error and crash the migration source.

The nested paths look similar. put_tmp() in vmstate-types.c reports the
inner failure through a local_err and returns the nested ret unchanged:

    ret = vmstate_save_state(f, vmsd, tmp, vmdesc, &local_err);
    if (ret) {
        error_report_err(local_err);
    }
    g_free(tmp);

    return ret;

put_qtailq(), put_qlist() and put_gtree() behave the same way. If the
nested ret is positive, for example from a pre_save callback, it reaches
the outer branch with ret < 0 false and *errp unset.

I could not find an in-tree put() callback that returns a positive value.
The ones I read return 0 or a negative errno. The VMStateInfo.put contract
in include/migration/vmstate.h doesn't say that only negative values are
allowed.

Before this patch, error_setg() ran for any non-zero ret, so this mismatch
is new. The pre_save and post_save paths are not affected, because both
call error_setg() for any non-zero return.

Would it be better to use "if (ret)" instead of "if (ret < 0)" for the
put() case, or to normalise ret to a negative value?

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=17

Re: [PATCH QEMU HCI-8.1 v6 19/34] migration: Free cpr-transfer MigrationAddress along with gsource

[no-reply]

> From: Fabiano Rosas <farosas@suse.de>
> 
> When setting a callback on a Glib source and giving it a data pointer,
> it's natural to also provide the destructor for the data in question.
> 
> Since migrate_hup_add() already needs to clone the MigrationAddress
> when setting the qmp_migrate_finish_cb callback, also pass the
> qapi_free_MigrationAddress as the GDestroyNotify callback.
> 
> With this the address doesn't need to be freed at the callback body,
> making the management of that memory slightly simpler.
> 
> andrey.drobyshev:
>     This commit was cherry-picked from the upstream commit 6a887119ce18,
>     adapted for the hci-dev base: qmp_migrate_finish_cb() here still
>     closes the cpr state and deletes the HUP source itself (upstream had
>     already moved that into migration_cleanup()), so only the explicit
>     qapi_free_MigrationAddress() call goes away from it.
> 
> Cc: Mark Kanda <mark.kanda@oracle.com>
> Cc: Ben Chaney <bchaney@akamai.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-23-farosas@suse.de
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=19

Re: [PATCH QEMU HCI-8.1 v6 20/34] migration: Move CPR HUP watch to cpr-transfer.c

[no-reply]

> From: Fabiano Rosas <farosas@suse.de>
> 
> Move this CPR-specific code into a cpr file. While here, give the
> functions more significant names.
> 
> This makes the new idea (after cpr-transfer) of having two parts to
> qmp_migrate slightly more obvious: either wait for the hangup or
> continue directly.
> 
> andrey.drobyshev:
>     This commit was cherry-picked from the upstream commit f523e5aa5f56,
>     adapted for the hci-dev base, which predates the upstream qmp_migrate()
>     rework around MigrationChannel: the watch is still armed with the
>     plain MigrationAddress from qmp_migrate() rather than main_ch->addr,
>     and qmp_migrate_finish_cb() keeps closing the cpr state and destroying
>     the HUP source itself.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-24-farosas@suse.de
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=20

Re: [PATCH QEMU HCI-8.1 v6 18/34] migration: yank: Move register instance earlier

[no-reply]

> From: Fabiano Rosas <farosas@suse.de>
> 
> Move the register_instance call to migrate_prepare() so it can be
> paired with the unregister_instance at migration_cleanup(). Otherwise,
> the cleanup cannot be run when cpr_state_save() fails because the
> instance is registered only after it.
> 
> When resuming from a paused postcopy migration, migrate_prepare()
> returns early, but migration_cleanup() doesn't run, so the yank will
> remain paired.
> 
> andrey.drobyshev:
>     This commit was cherry-picked from the upstream commit 7e714392437c,
>     adapted for the hci-dev base, which predates the upstream
>     migration_connect_set_error() -> migration_connect_error_propagate()
> [ ... ]
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=18

Re: [PATCH QEMU HCI-8.1 v6 21/34] migration: Fix crash on second migration when cancel early

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: assert(migrate_mode() == CPR_TRANSFER) in migration_cancel() can abort QEMU
--

commit 4600a9bb3fca173c79b21943fa97310ae3af2659
Author: Peter Xu <peterx@redhat.com>

migration: Fix crash on second migration when cancel early

This patch makes migration_cancel() call migration_cleanup() when the
cpr-transfer HUP gsource is still armed, rather than special-casing
the SETUP state. It also detaches hup_source at the start of
qmp_migrate_finish_cb() and adds several assertions.

Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -1661,14 +1661,19 @@ void migration_cancel(void)
>      }
>  
>      /*
> -     * If qmp_migrate_finish has not been called, then there is no path that
> -     * will complete the cancellation.  Do it now.
> +     * This is cpr-transfer specific processing.
> +     *
> +     * If this is true, it means cpr-transfer migration is waiting for the
> +     * destination to send HUP event on CPR channel to continue the next
> +     * phase.  If so, do the cleanup proactively to avoid get stuck in
> +     * CANCELLING state.
>       */
> -    if (setup && !s->to_dst_file) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
> -                          MIGRATION_STATUS_CANCELLED);
> -        cpr_state_close();
> -        cpr_transfer_source_destroy(s);
> +    if (cpr_transfer_source_active(s)) {
> +        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);

[Severity: Medium]
Can this assert abort QEMU?

migrate_mode() is not latched for the running migration. It returns
cpr_get_incoming_mode() if that is not NONE, and otherwise returns the
live s->parameters.mode:

migrate_mode()
    mode = cpr_get_incoming_mode();
    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }

As far as I can tell, qmp_migrate_set_parameters() and
migrate_params_check() do not reject a change of mode while a migration
is in SETUP. Consider this sequence:

1. qmp_migrate() with mode=cpr-transfer runs migrate_prepare(), then
   cpr_state_save(), then cpr_transfer_add_hup_watch(). hup_source is
   now set and the state stays SETUP, waiting for the destination HUP.

2. Before the HUP arrives, the client sends migrate-set-parameters
   with mode=normal.

3. The client sends migrate_cancel, or QEMU quits and
   migration_shutdown() calls migration_cancel().

In that case cpr_transfer_source_active(s) is true but migrate_mode()
is MIG_MODE_NORMAL, so assert(migrate_mode() == MIG_MODE_CPR_TRANSFER)
fails and the whole source QEMU aborts.

The old code in this window did not depend on the mode. Would it be
better to drop this assert, since hup_source being set already implies
that cpr-transfer armed it?

> +        assert(setup && !s->to_dst_file);
> +        migration_cleanup(s);
> +        /* Now all things should have been released */
> +        assert(!cpr_transfer_source_active(s));
>      }
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=21

Re: [PATCH QEMU HCI-8.1 v6 23/34] kvm: fix coalesced MMIO flush racing with kvm_close()

[no-reply]

> kvm_close() destroys the vCPUs, which unmaps their kvm_run areas, and the
> coalesced MMIO ring lives in the kvm_run area of the first vCPU.  Nothing
> stops the RCU thread from flushing that ring at the same time: finalizing
> a memory region of a freed flatview calls memory_region_transaction_begin()
> and thus kvm_flush_coalesced_mmio_buffer().  kvm_state is still set at
> that point, so the flush reads the ring after it has been unmapped:
> 
>   #0  kvm_flush_coalesced_mmio_buffer   accel/kvm/kvm-all.c
>   #1  qemu_flush_coalesced_mmio_buffer
>   #2  memory_region_transaction_begin
>   #3  memory_region_finalize
>   ...
>   #10 flatview_destroy
>   #11 call_rcu_thread
> 
> while the main thread is in:
> 
>   kvm_close -> cpu_remove_sync
>   vfio_kvm_device_close
>   vfio_cpr_kvm_close_notifier (MIG_EVENT_PRECOPY_DONE)
> 
> Seen as a SIGSEGV of the old QEMU during repeated cpr-exec of a VM with
> a VFIO device.
> 
> Clear the ring pointer first, so that new flushes skip it, and wait for
> the pending RCU callbacks before destroying the vCPUs.
> 
> Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=23

Re: [PATCH QEMU HCI-8.1 v6 24/34] migration/cpr: add cpr_is_in_progress()

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration/cpr: cpr_is_in_progress() false on cpr-transfer source mid-migration
--

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

migration/cpr: add cpr_is_in_progress()

This adds cpr_is_in_progress(), a helper meant to report whether a CPR
migration is actually going on. It returns true on the target until the
incoming migration is done. On the source it returns true when the CPR
state file is open.

> diff --git a/migration/cpr.c b/migration/cpr.c
> index a149e790c103..3ea5d86bab48 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c

[ ... ]

> +bool cpr_is_in_progress(void)
> +{
> +    /* The incoming mode on the target, the 'mode' parameter on the source */
> +    MigMode mode = migrate_mode();

[ ... ]

> +    /*
> +     * Source: the parameter alone means nothing, it may just have been set.
> +     * An outgoing CPR migration has the CPR state open, from
> +     * cpr_state_save() to cpr_state_close().
> +     */
> +    return cpr_state_file != NULL;
> +}

[Severity: Medium]
The commit message says the helper is true "on the source only while the
outgoing one runs", and the comment says the state is open "from
cpr_state_save() to cpr_state_close()". Does this hold for cpr-transfer?

On a cpr-transfer source, qmp_migrate() only registers a HUP watch after
cpr_state_save():

migration/migration.c:qmp_migrate() {
    ...
    if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
    } else {
        qmp_migrate_finish(addr, errp);
    ...
}

When the target closes the CPR socket, qmp_migrate_finish_cb() starts the
outgoing connection and then closes the CPR state right away:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
    ...
}

cpr_state_close() sets cpr_state_file to NULL. From then until
migration_cleanup(), the source has cpr_is_incoming() false and
cpr_state_file == NULL. Can cpr_is_in_progress() then return false for the
whole of precopy, the vm_stop at completion, the VM state change handlers
and the device save?

The helper would be true on a cpr-transfer source only between the end of
cpr_state_save() and the target's HUP. Nothing has been migrated and the
VM is still running during that window.

Callers such as VM state change handlers and savevm .is_active hooks would
take the non-CPR path on a cpr-transfer source. That includes the VFIO
callbacks later in the series: vfio_is_active(), vfio_vmstate_change(),
vfio_vmstate_change_prepare(), vfio_switchover_ack_needed() and
vfio_save_state().

Only cpr-exec keeps cpr_state_file open until migration_cleanup(), so the
two CPR modes behave differently. I did not find a later patch in the
series that changes this.

Also, cpr_state_file is assigned only at the end of cpr_state_save(), so
the helper returns false while cpr_state_save() is still running. Is that
intended?

Could the source-side check use a state that lasts for the whole outgoing
migration, rather than cpr_state_file?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=24

Re: [PATCH QEMU HCI-8.1 v6 22/34] kvm: tolerate ioctls after kvm_close()

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: ioctl wrappers return 0 after kvm_close(), faking fd 0 for fd-returning ioctls
- [Medium] kvm: closed-state ioctl guard returns 0 for fd-returning ioctls, handing out fd 0
- [Low] kvm: stale fds left in kvm_parked_vcpus after kvm_close() now stay reachable
--

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

kvm: tolerate ioctls after kvm_close()

This patch keeps kvm_state allocated after kvm_close() instead of setting it
to NULL. It makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0
once the descriptors are closed. The goal is to stop a retired cpr-transfer
source from crashing on late interrupts.

> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c

[ ... ]

> @@ -644,7 +644,6 @@ void kvm_close(void)
>          close(kvm_state->fd);
>          kvm_state->fd = -1;
>      }
> -    kvm_state = NULL;
>  }

[Severity: Low]
kvm_close() runs cpu_remove_sync() for each CPU, which reaches
do_kvm_destroy_vcpu() and then kvm_park_vcpu(). That stores the vCPU fd
number in kvm_state->kvm_parked_vcpus:

    vcpu->kvm_fd = cpu->kvm_fd;
    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);

kvm_close() then runs close(cpu->kvm_fd). Only cpu->kvm_fd is set to -1, and
the parked entry keeps the closed fd number.

Before this patch kvm_state was NULL after kvm_close(), so this list could not
be reached. With kvm_state kept alive, can kvm_create_vcpu() get a stale fd
from kvm_unpark_vcpu() on a vCPU hotplug after kvm_close()?

kvm_create_vcpu()
  kvm_fd = kvm_unpark_vcpu(s, vcpu_id);

kvm_init_vcpu() would then run mmap() and kvm_vcpu_ioctl() on that number.
The kvm_vcpu_ioctl() guard does not catch this, because kvm_create_vcpu()
has just assigned the stale value to cpu->kvm_fd. If the number has been
reused, an unrelated descriptor would be used.

If no parked entry matches, the new guard in kvm_vm_ioctl() returns 0 for
KVM_CREATE_VCPU:

    kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
    if (kvm_fd < 0) {

Here 0 is accepted as the vCPU fd, so cpu->kvm_fd becomes 0.

The later patch in the series, "kvm: fix coalesced MMIO flush racing with
kvm_close()", only clears coalesced_mmio_ring and calls drain_call_rcu(). It
does not purge the parked list or guard kvm_unpark_vcpu().

[ ... ]

> @@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no VM to talk to any more */
> +    if (s->vmfd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Can this blanket return 0 hand out fd 0 to callers of ioctls that return a
new fd? Several of those callers only test for an error return, and the
argument structure is not filled in.

query_stats_cb() and query_stats_schemas_cb() only test for -1:

    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
    if (stats_fd == -1) {
        ...
    }
    query_stats(result, target, names, stats_fd, NULL, errp);
    close(stats_fd);

After kvm_close(), a QMP query-stats or query-stats-schemas command gets
stats_fd == 0. The code then reads from fd 0 and calls close(0), which closes
stdin or whatever owns fd 0.

kvm_create_guest_memfd() tests only fd < 0:

    fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_GUEST_MEMFD, &guest_memfd);
    if (fd < 0) {

A return of 0 is accepted there as a valid guest_memfd.

kvm_create_device() returns the unfilled field as a success:

    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &create_dev);
    if (ret) {
        return ret;
    }
    return test ? 0 : create_dev.fd;

Here create_dev.fd is still -1.

In hw/vfio/helpers.c, vfio_kvm_device_add_fd() has a similar problem:

    if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
        ...
    }
    vfio_kvm_device_fd = cd.fd;

vfio_kvm_device_close() resets vfio_kvm_device_fd to -1, so this path is
taken again on a VFIO hot-add after kvm_close(). cd is initialised with only
.type, so cd.fd is 0 and vfio_kvm_device_fd becomes 0. Later ioctl() and
close() calls then act on stdin. It can be reached through
iommufd_cdev_kvm_device_add() -> vfio_kvm_device_add_fd().

The commit message gives the reason for returning 0 only for interrupt
injection, route commit and ioeventfd:

    Returning an error instead would still kill it: kvm_set_irq() aborts and
    kvm_irqchip_commit_routes() asserts on any failure, and the ioeventfd
    callers abort on a stale errno.

That reasoning does not cover the ioctls that return a descriptor. Should
those return a negative errno in the closed state, or should their callers
check for the closed state?

[ ... ]

> @@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
>      va_end(ap);
>  
> +    /* kvm_close() has been called: the vCPU fds are gone */
> +    if (cpu->kvm_fd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Does this have the same problem for vCPU ioctls that return an fd?

kvm_init_vcpu() sets:

    cpu->kvm_vcpu_stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);

If the vCPU fd is closed, this stores 0 as the stats fd.

The follow-up patches in the series change only kvm_close(). They add no
guard for any of these fd-returning callers.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=22

Re: [PATCH QEMU HCI-8.1 v6 26/34] vfio/migration: don't assume the device is running on CPR incoming

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio/migration: CPR incoming caches STOP even if the device is RUNNING
--

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

vfio/migration: don't assume the device is running on CPR incoming

This patch makes vfio_migration_init() cache STOP instead of RUNNING for a
device inherited across CPR. It also makes vfio_vmstate_change() record
RUNNING or STOP on the CPR target, depending on whether the VM is started.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -914,6 +914,14 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /* CPR target only: the device kept its state, note where the VM left it */
> +    if (cpr_is_incoming()) {
> +        vbasedev->migration->device_state = running ?
> +            VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
> +        return;
> +    }
> +

[Severity: Medium]
Can this handler leave the cached state wrong when the target stays paused?

This branch only runs when vm_state_notify() fires. On the target,
process_incoming_migration_bh() takes this path when the VM is not
started (for example with -S or autostart off):

    } else {
        runstate_set(RUN_STATE_PAUSED);
    }

runstate_set() only validates and records the transition. It does not call
vm_state_notify(), so vfio_vmstate_change() never runs.

If the source VM was running at CPR time, the device was left RUNNING. This
is because vfio_vmstate_change() and vfio_vmstate_change_prepare() return
early under cpr_is_in_progress() on the source. The target is paused but the
device is still RUNNING.

[ ... ]

> @@ -1039,7 +1047,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
>      vbasedev->migration = g_new0(VFIOMigration, 1);
>      migration = vbasedev->migration;
>      migration->vbasedev = vbasedev;
> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> +    /* A device inherited across CPR is in STOP unless the VM gets started */
> +    migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
> +                                                  VFIO_DEVICE_STATE_RUNNING;

[Severity: Medium]
Does this cache STOP for every CPR-incoming device, even when the source
left it RUNNING?

If the target stays paused, the cache stays STOP while the device is really
RUNNING. The commit message says the device is "STOP if it stays paused",
which doesn't hold when the source VM was running.

Is the real state ever queried, for example through
VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE GET? As far as I can tell, no later
patch in the series does that.

The stale value affects these callers:

hw/vfio/migration.c:vfio_migration_set_state() {
    ...
    if (new_state == migration->device_state) {
        return 0;
    }
    ...
}

Any request matching the stale cached state is skipped without consulting
the kernel.

vfio_device_state_is_running() and vfio_device_state_is_precopy() also
return wrong answers. The dirty-tracking check in hw/vfio/listener.c uses
them:

    (vfio_device_state_is_running(vbasedev) ||
     vfio_device_state_is_precopy(vbasedev))

The vfio_save_setup() switch uses the cached state too.

With the target paused, 'cont' sends a redundant STOP->RUNNING request to a
device that is already RUNNING. This is probably benign.

A live migration out of the paused target before 'cont' still issues the
STOP_COPY request, since STOP_COPY differs from the cached STOP. I did not
find a proven data loss on that path.

The stale cache also persists after the incoming migration finishes.
migration_incoming_state_destroy() clears the incoming mode, so the
cpr_is_incoming() branch in vfio_vmstate_change() can no longer correct the
state when 'cont' runs later.

Could the cached state be taken from the device itself, or could the paused
target case be handled explicitly?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=26

Re: [PATCH QEMU HCI-8.1 v6 25/34] vfio/migration: don't migrate device state during CPR

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vfio/migration: cpr_is_in_progress() is false on cpr-transfer source
- [High] migration: cpr_is_in_progress() is false on the cpr-transfer source
- [Medium] vfio/migration: stop/cont ignored for whole CPR outgoing window, not just the handover
--

Commit 54a8e2428cc329d383d08292ed034898e8bf828e
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: don't migrate device state during CPR

This patch gates the VFIO migration handlers on cpr_is_in_progress(). With
the gates in place, device state is neither saved nor loaded during
cpr-transfer or cpr-exec, and the device state is not changed on VM
stop/start. It also resets multifd_transfer in vfio_multifd_cleanup().

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /*
> +     * .is_active doesn't cover the non-iterable stage, so the section is
> +     * still sent: leave it empty, or new QEMU would load the config into
> +     * the preserved device and re-program its MSI-X.
> +     */
> +    if (cpr_is_in_progress()) {
> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> +        return;
> +    }
> +

[Severity: High]
Is cpr_is_in_progress() still true here on a cpr-transfer source?

On the source it returns cpr_state_file != NULL:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate() only registers a HUP watch:

qmp_migrate()
    cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

When the target closes the CPR socket, the callback runs:

qmp_migrate_finish_cb()
    qmp_migrate_finish(addr, NULL);
    cpr_state_close();

qmp_migrate_finish() ends up in socket_start_outgoing_migration(), which
only starts qio_channel_socket_connect_async(). The completion
(socket_outgoing_migration() -> migration_channel_connect()), the migration
thread, qemu_savevm_state_setup(), vm_stop() and vfio_save_state() all run
after the callback has returned, with cpr_state_file already NULL.

If so, cpr_is_in_progress() is false for the whole data migration on a
cpr-transfer source, and the gates in vfio_save_state(),
vfio_switchover_ack_needed(), vfio_vmstate_change_prepare() and
vfio_vmstate_change() are all bypassed. Wouldn't the device then still be
stopped and put into PRE_COPY or STOP_COPY, and its config and data state
still be sent, which is the failure the commit message describes for the
NVIDIA vGPU?

The same question applies to the source-side comment in
cpr_is_in_progress(), which says the state is open "from cpr_state_save()
to cpr_state_close()". For cpr-transfer that window ends before the data
migration starts, whereas for cpr-exec cpr_state_close() runs in
migration_cleanup() after qemu_savevm_state_cleanup().

I did not find a later patch in the series that changes cpr_is_in_progress()
or the cpr_state_close() call sites.

[ ... ]

> +/*
> + * cpr-transfer and cpr-exec hand the running device over to the new QEMU
> + * as is: its fds, mappings and interrupts are preserved and it never stops.
> + * Device state migration must stay out of it, else the device is stopped,
> + * its state saved and then resumed into the very same device.
> + */
> +static bool vfio_is_active(void *opaque)
> +{
> +    return !cpr_is_in_progress();
> +}
> +

[Severity: High]
On a cpr-transfer source, wouldn't vfio_is_active() return true here, given
that cpr_state_file has already been cleared by qmp_migrate_finish_cb()?

In that case save_setup, iterate and complete would all still run for the
device.

On the target, cpr_is_in_progress() is true because the incoming mode is
set, so load_setup is skipped. vfio_load_state() has no CPR gate:

hw/vfio/migration.c:vfio_load_state() {
    ...
    data = qemu_get_be64(f);
    while (data != VFIO_MIG_FLAG_END_OF_STATE) {
    ...
}

Could real DEV_CONFIG_STATE or DEV_DATA_STATE sent by the source then be
loaded into the preserved live device? That would re-program MSI-X, or
write to a data_fd that was never opened. The two sides would also disagree
about whether device state is migrated.

[ ... ]

> @@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>      Error *local_err = NULL;
>      int ret;
>  
> +    /* The device is preserved, not a plain stop/cont: keep its state */
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +

[Severity: Medium]
Can this gate skip a user-issued stop or cont while a CPR migration has been
started but the VM has not yet been stopped for handover?

On the source, cpr_state_save() sets cpr_state_file from qmp_migrate(),
before the migration thread starts. For cpr-transfer, the source then
returns to the main loop and waits for the target's HUP, with
cpr_state_file open the whole time:

qmp_migrate()
    cpr_state_save()
    cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

During that window, a QMP stop or cont reaches vfio_vmstate_change_prepare()
and vfio_vmstate_change(), and both return early on cpr_is_in_progress().

The commit message says that a plain stop/cont must still change the device
state when the mode is merely set. Does that hold once CPR migration has
started but has not yet stopped the VM? A stopped guest could keep a
running, DMA-capable device, and migration->device_state would not be
updated.

The final-state series still has the source-side early return in
vfio_vmstate_change().

> @@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +

[Severity: High]
Is this gate effective on a cpr-transfer source?

With cpr_state_file already cleared in qmp_migrate_finish_cb(), the early
return would not be taken when the migration thread calls vm_stop(). The
device would then still be moved to STOP or STOP_COPY, and the same applies
to vfio_save_state(), vfio_is_active() and vfio_vmstate_change_prepare().

In contrast, cpr-exec appears to work, because cpr_state_close() is called
from migration_cleanup() after qemu_savevm_state_cleanup():

migration/migration.c:migration_cleanup() {
    ...
    qemu_savevm_state_cleanup();
    cpr_state_close();
}

Should the early cpr_state_close() in qmp_migrate_finish_cb() be reconsidered,
or the predicate changed, so that cpr_is_in_progress() holds for the data
migration in cpr-transfer as well? The close in qmp_migrate_finish_cb()
seems to be the defect, rather than any mismatch between stop and start
values.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=25

Re: [PATCH QEMU HCI-8.1 v6 27/34] migration/ram: don't start dirty logging in CPR modes

[no-reply]

> In the CPR modes guest RAM is preserved in place, not copied:
> migrate_ram_is_ignored() is true for every block.  Yet ram_init_bitmaps()
> still starts the global dirty log, so every listener starts tracking
> pages nobody will send.  KVM write-protects all guest memory, and VFIO
> starts DMA logging on the devices; with cpr-exec that logging is never
> stopped before the exec, leaving e.g. the DMA page tracker of an NVIDIA
> vGPU running after the update.
> 
> Don't start the dirty log for a migration that migrates no RAM, as is
> already done for background snapshots.  ram_save_cleanup() only stops
> the log if it was started.
> 
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=27

Re: [PATCH QEMU HCI-8.1 v6 29/34] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

[no-reply]

> cpr_exec_cb() marks all saved ramblocks with MADV_DOEXEC before exec, but
> ignores the result.  If madvise() fails for some block, the walk stops
> there and the remaining blocks are not marked, yet execvp() still runs.
> The new QEMU then reuses addresses of guest RAM that wasn't preserved,
> and the guest is lost.
> 
> Treat that as a failed cpr-exec: go to the same failure path as when
> execvp() fails, so the VM keeps running in the old QEMU.
> 
> On that path also clear MADV_DOEXEC from all saved ramblocks again, just
> like the preserved fds get close-on-exec back.
> 
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=29

Re: [PATCH QEMU HCI-8.1 v6 28/34] VZ: vfio/cpr: don't close KVM on cpr-exec

[no-reply]

> With VFIO devices, KVM is closed on the source when the migration
> completes, so that the new QEMU can create its own KVM VM with the same
> devices.  That is done from a migration notifier, which for cpr-exec
> runs before cpr_exec_cb().  But cpr_exec_cb() may still give up, when
> the memory can't be preserved or execve() fails, and resume the VM in
> the old QEMU.  A VM without KVM can't resume: kvm_close() removes the
> vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
> (KVM_SET_CLOCK in kvmclock_vm_state_change()).
> 
> cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
> kernel closes them during execve(), before the new QEMU runs, and
> kvm_vm_release() tears down the irqfds then.  Keep the notifier for
> cpr-transfer only, where the source process lives on.
> 
> Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
> preserve_ram() and one with a bogus cpr-exec-command both left the VM
> running with no lost pings, and a later cpr-exec completed.
> 
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=28

Re: [PATCH QEMU HCI-8.1 v6 31/34] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released

[no-reply]

> vhost_net_cpr_reset_owners() only warns when VHOST_RESET_OWNER fails for
> a device, and the exec goes on.  But the new QEMU can't take such a
> device: it is still owned by the old mm, so VHOST_SET_OWNER fails with
> EBUSY, and the guest is left without network after the update.
> 
> Stop at the first device that can't be released and abort the cpr-exec
> instead.  The VM goes on in the old QEMU, with the devices released so
> far taken back.  vhost-vsock already does that in its pre_save.
> 
> Fixes: 1278e4830052 ("VZ: migration/cpr-exec: vhost-net: send RESET_OWNER before cpr-exec")
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=31

Re: [PATCH QEMU HCI-8.1 v6 30/34] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[no-reply]

> Right before execve(), cpr_exec_cb() releases the kernel vhost-net
> devices with VHOST_RESET_OWNER, so that the new QEMU can take them.  If
> execve() fails, the VM goes on in the old QEMU, but nobody takes the
> devices back: the kernel refuses the ioctls of the next
> vhost_net_start(), and the guest is left without network.
> 
> Take them back with VHOST_SET_OWNER when the exec fails, and set the
> vring call and err fds again: VHOST_RESET_OWNER dropped them too, and
> vhost_dev_init() registers them only once.  The devices are stopped at
> that point, and vhost_net_start() sets up the rest again when the VM
> resumes.  Only the devices that were actually released are taken back:
> vhost_net_cpr_reset_owners() marks them.  vhost-vsock does the same in
> its CPR notifier.
> 
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=30

Re: [PATCH QEMU HCI-8.1 v6 33/34] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

[no-reply]

> Commit bb8da93ddfc4 ("VZ: migration/cpr-exec: enable checks prohibiting
> cpr-exec #VSTOR-108564") introduced restrictions for cpr-exec migration
> mode.  Among them was prohibition to do such migration in case of
> presence of any VFIO devices.  Now we want to allow migration with such
> devices in scope of VSTOR-103860.  Therefore, let's lift that restiction.
> 
> 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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=33

Re: [PATCH QEMU HCI-8.1 v6 34/34] VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio: stop-copy size dropped from pending data, bypassing the downtime limit
- [Medium] vfio/migration: stop-copy size dropped for devices that never enter PRE_COPY
--

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

VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

This patch changes vfio_state_pending_exact() so that the device's stop-copy
size is added to must_precopy only when a query failed or pre-copy data
(initial or dirty) is still pending. Once pre-copy has drained, the
stop-copy size no longer holds up the switchover decision.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 04d0de7cef0a..eff1172569d3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -569,16 +569,24 @@ static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
>      VFIODevice *vbasedev = opaque;
>      VFIOMigration *migration = vbasedev->migration;
>      uint64_t stop_copy_size = VFIO_MIG_STOP_COPY_SIZE;
> +    int ret;
>  
>      /*
>       * If getting pending migration size fails, VFIO_MIG_STOP_COPY_SIZE is
>       * reported so downtime limit won't be violated.
>       */

[Severity: Medium]
Is this comment still accurate? On the success path the stop-copy size is
now deliberately left out of must_precopy, so the downtime limit is no
longer protected for it. Only the error fallback still reports
VFIO_MIG_STOP_COPY_SIZE.

> -    vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> -    *must_precopy += stop_copy_size;
> +    ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
>  
>      if (vfio_device_state_is_precopy(vbasedev)) {
> -        vfio_query_precopy_size(migration);
> +        ret |= vfio_query_precopy_size(migration);
> +    }
> +
> +    /*
> +     * Once there's nothing left to pre-copy, the stop-copy state can't
> +     * shrink by iterating: don't let it hold up the switchover.
> +     */
> +    if (ret || migration->precopy_init_size || migration->precopy_dirty_size) {
> +        *must_precopy += stop_copy_size;
>      }

[Severity: Medium]
The commit message says the stop-copy size is reported "only while the
device still has something to pre-copy". Can this condition tell a drained
PRE_COPY device apart from a device that never pre-copies?

If the device lacks VFIO_MIGRATION_PRE_COPY, vfio_save_setup() never moves
it to PRE_COPY. In that case vfio_device_state_is_precopy() is false and
vfio_query_precopy_size() is never called:

    if (vfio_device_state_is_precopy(vbasedev)) {
        ret |= vfio_query_precopy_size(migration);
    }

precopy_init_size and precopy_dirty_size then stay at 0. They are zeroed
in vfio_save_cleanup() and are only decremented elsewhere. If
vfio_query_stop_copy_size() succeeds, ret is 0, so this is the result:

    if (ret || migration->precopy_init_size || migration->precopy_dirty_size)

The condition is false and stop_copy_size is not added to *must_precopy.
vfio_state_pending_estimate() also returns early when
!vfio_device_state_is_precopy(), so nothing else accounts for the device's
stop-copy data.

Can migration_iteration_run() then reach this check with the device blob
excluded?

    complete_ready = can_switchover && (pending_size <= s->threshold_size);

If so, the full stop-copy state would be sent after the guest is stopped,
outside the configured downtime limit. Before the patch, the downtime limit
applied to such devices. Should the omission be gated on
vfio_device_state_is_precopy() (or vfio_precopy_supported()) together with
successful queries?

[Severity: Medium]
Even for a PRE_COPY device, can the stop-copy size be dropped too early?
vfio_update_estimated_pending_data() zeroes both counters whenever
vfio_save_block() returns 0 bytes:

    migration->precopy_init_size = 0;
    migration->precopy_dirty_size = 0;

vfio_query_precopy_size() can also report 0/0 transiently. Either case
satisfies the "drained" condition, and nothing re-checks the stop-copy size
afterwards.

The change is also not opt-in. There is no property or capability gate, so
every VFIO migration user now gets this behaviour. Was Nova's stepwise
downtime-limit raise meant to be bypassed for all devices, and not only for
the vGPU case described in the commit message?

>  
>      trace_vfio_state_pending_exact(vbasedev->name, *must_precopy, *can_postcopy,
>                                     stop_copy_size, migration->precopy_init_size,
>                                     migration->precopy_dirty_size);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=34

Re: [PATCH QEMU HCI-8.1 v6 32/34] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] vfio: unconditional cpr-transfer blocker added in vfio_device_attach() regresses working mode
- [Medium] vfio: cpr-transfer blocker is skipped for vfio-user devices
- [Low] vfio: BAR mappings in CPR ramblock list lose PROT_READ-only protection under -cpr-ram-rdonly
--

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

VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

This patch saves each VFIO BAR mapping in the CPR ramblock list, so cpr-exec
carries it over with MADV_DOEXEC and the new QEMU reuses it instead of
calling mmap() again. It also adds a cpr-transfer migration blocker to VFIO
devices in vfio_device_attach().

> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c

[ ... ]

> @@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                               TYPE_VFIO_IOMMU_IOMMUFD :
>                               TYPE_VFIO_IOMMU_LEGACY;
>  
> -    return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
> -                                            as, errp);
> +    /*
> +     * Only cpr-exec keeps the address space and so the region mappings,
> +     * see vfio_region_mmap().  A cpr-transfer target would have to map the
> +     * regions anew, which some drivers (NVIDIA vGPU) refuse.
> +     */
> +    error_setg(&vbasedev->cpr.mmap_blocker,
> +               "cpr-transfer is not supported with VFIO devices; "
> +               "use the cpr-exec mode");
> +    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
> +                                  MIG_MODE_CPR_TRANSFER, -1)) {
> +        return false;
> +    }

[Severity: Medium]
Is this blocker too broad?

It is added in vfio_device_attach() for every VFIO device and every iommu
backend, whether or not a BAR is mmapped. At the base commit, cpr-transfer
with VFIO was not blocked. The vfio_devices_present() check was only
reached under:

    if (migrate_mode() == MIG_MODE_CPR_EXEC && migrate_cpr_is_blocked(errp))

The existing cpr-transfer notifier registration in
vfio_cpr_pci_register_device() and the cpr-legacy and cpr-iommufd transfer
blockers and notifiers also become unreachable for VFIO.

Can this regress cpr-transfer for vfio-pci devices that do not need the BAR
reuse path? Would it be better to add the blocker only when a BAR is
actually mapped?

migrate_add_blocker_modes() can also return -EBUSY here, for example during
savevm or an active cpr-transfer migration. Is it intended that a device
attach or hotplug now fails in those cases?

Also, docs/devel/migration/CPR.rst is not updated to mention the new
restriction.

> +
> +    if (!vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
> +                                          as, errp)) {
> +        migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
> +        return false;
> +    }
> +
> +    return true;
>  }

[ ... ]

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    void *addr = region->mmaps[index].mmap;
> +    size_t size = region->mmaps[index].size;
> +
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

[Severity: Low]
Can the BAR mapping lose its original protection here?

vfio_region_mmap() derives prot from VFIO_REGION_INFO_FLAG_READ and
VFIO_REGION_INFO_FLAG_WRITE, so a region with READ but not WRITE is mapped
PROT_READ. This function records only the address and length in the generic
ramblock list.

With -cpr-ram-rdonly, cpr_exec_cb() runs mark_ram_rdonly() over every entry
including the BARs. mark_ram_rw() then calls qemu_mprotect_rw() on every
entry, both in cpr_ram_restore_rw() and in the new QEMU path.

Could a read-only BAR mapping become writable after cpr-exec, or after a
failed cpr-exec rollback, when this debug option is used?

[ ... ]

> @@ -347,7 +392,9 @@ int vfio_region_mmap(VFIORegion *region)
>              ret = -errno;
>              goto no_mmap;
>          }
> +        vfio_region_cpr_save(region, i);

[Severity: Medium]
Is the cpr-transfer blocker skipped for vfio-user devices?

The blocker is added only in the vfio_device_attach() wrapper.
vfio_user_pci_realize() calls the lower-level function directly:

    if (!vfio_device_attach_by_iommu_type(TYPE_VFIO_IOMMU_USER,
                                          vbasedev->name, vbasedev,
                                          as, errp)) {

It can still reach vfio_bars_register() and then vfio_region_mmap(), and
vfio_user_get_region_info() shows that vfio-user regions can be mmappable.
So vfio_region_cpr_save() records the BAR address in cpr_state.ramblocks
for those devices too.

With cpr-transfer, can vfio_region_cpr_find() in the target process then
return an address that belongs to the source process? That address would be
passed to memory_region_init_ram_device_ptr(). I did not find any cpr or
blocker code under hw/vfio-user/ that would prevent this.

>  
> +mapped:
>          name = g_strdup_printf("%s mmaps[%d]",

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=32

Re: [PATCH QEMU HCI-8.1 v6 21/34] migration: Fix crash on second migration when cancel early

[no-reply]

> From: Peter Xu <peterx@redhat.com>
> 
> Marc-André reported an issue on QEMU crash when retrying a cancelled
> migration during early setup phase, see "Link:" for more information, and
> also easy way to reproduce.
> 
> This patch is a replacement of the prior fix proposed by not only switching
> to migration_cleanup(), but also fixing it from CPR side, so that we track
> hup_source properly to know if src QEMU is waiting or the HUP signal.
> 
> To put it simple: this chunk of special casing in migration_cancel() should
> not affect normal migration, but only cpr-transfer migration to cover the
> small window when the src QEMU is waiting for a HUP signal on cpr
> channel (so that src QEMU can continue the migration on the main channel).
> 
> [ ... ]
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Tested-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261030-vfio_v1-v6-0-45de246bc92e@virtuozzo.com?part=21

[PATCH QEMU HCI-8.1 v6 21/34] migration: Fix crash on second migration when cancel early

[Andrey Drobyshev]

From: Peter Xu <peterx@redhat.com>

Marc-André reported an issue on QEMU crash when retrying a cancelled
migration during early setup phase, see "Link:" for more information, and
also easy way to reproduce.

This patch is a replacement of the prior fix proposed by not only switching
to migration_cleanup(), but also fixing it from CPR side, so that we track
hup_source properly to know if src QEMU is waiting or the HUP signal.

To put it simple: this chunk of special casing in migration_cancel() should
not affect normal migration, but only cpr-transfer migration to cover the
small window when the src QEMU is waiting for a HUP signal on cpr
channel (so that src QEMU can continue the migration on the main channel).

To achieve that, we'll also need to remember to detach the hup_source
whenenver invoked: after that point, we should always be able to cleanup
the migration.

It's not a generic operation to explicitly detach a gsource from its
context while in its dispatch() function.  But it should be safe, because
gsource disptch() will only happen with a boosted refcount for the
dispatcher so that the gsource will not be freed until the callback
completes. It's also safe to return G_SOURCE_REMOVE after the gsource is
detached, as glib will simply ignore the G_SOURCE_REMOVE.

One can refer to latest 2.86.5 glib code in g_main_dispatch() for that:

https://github.com/GNOME/glib/blob/2.86.5/glib/gmain.c#L3592

When at this, add a bunch of assertions to make sure nothing surprises us.

After this patch applied, the 2nd migration will not crash QEMU, instead
it'll be in CANCELLING until the socket connection times out (it will take
~2min on my Fedora default kernel).  During this process no 2nd migration
will be allowed, and after it timed out migration can be restarted.

It's because so far we don't have control over socket_connect_outgoing(),
or anything yet managed by a task executed in qio_task_run_in_thread().
Speeding up the cancellation to be left for future.

I also tested cpr-transfer by only providing cpr channel not the main
channel (with -incoming defer), kickoff migration on source, then cancel it
on source directly without providing the main channel.  It keeps working.

I wanted to add an unit test for that but it'll need to refactor current
cpr-transfer tests first; let's leave it for later.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 4f08b870f8f0,
    adapted for the hci-dev base, which predates the upstream qmp_migrate()
    rework: the HUP callback is still qmp_migrate_finish_cb() rather than
    migration_connect_outgoing_cb(), so the early
    cpr_transfer_source_destroy() call lands there, ahead of
    qmp_migrate_finish() and the cpr_state_close() the callback keeps
    doing itself.

Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/cpr.h  |  1 +
 migration/cpr-transfer.c | 10 ++++++++++
 migration/migration.c    | 32 ++++++++++++++++++++++++--------
 migration/migration.h    |  5 +++++
 4 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index ae441322ff5..4dd397a7c13 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -80,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
 void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
                                 void *opaque);
 void cpr_transfer_source_destroy(MigrationState *s);
+bool cpr_transfer_source_active(MigrationState *s);
 
 void cpr_exec_init(void);
 QEMUFile *cpr_exec_output(Error **errp);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index 61d5c9dce2b..9defe7bad78 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -6,6 +6,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-migration.h"
@@ -79,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
 void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
                                 void *opaque)
 {
+    assert(bql_locked());
     s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
     g_source_set_callback(s->hup_source,
                           (GSourceFunc)func,
@@ -89,9 +91,17 @@ void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
 
 void cpr_transfer_source_destroy(MigrationState *s)
 {
+    assert(bql_locked());
     if (s->hup_source) {
         g_source_destroy(s->hup_source);
         g_source_unref(s->hup_source);
         s->hup_source = NULL;
     }
 }
+
+bool cpr_transfer_source_active(MigrationState *s)
+{
+    /* Whenever the HUP gsource is available, it's active. */
+    assert(bql_locked());
+    return s->hup_source;
+}
diff --git a/migration/migration.c b/migration/migration.c
index d892655a21e..821764afae9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1661,14 +1661,19 @@ void migration_cancel(void)
     }
 
     /*
-     * If qmp_migrate_finish has not been called, then there is no path that
-     * will complete the cancellation.  Do it now.
+     * This is cpr-transfer specific processing.
+     *
+     * If this is true, it means cpr-transfer migration is waiting for the
+     * destination to send HUP event on CPR channel to continue the next
+     * phase.  If so, do the cleanup proactively to avoid get stuck in
+     * CANCELLING state.
      */
-    if (setup && !s->to_dst_file) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
-                          MIGRATION_STATUS_CANCELLED);
-        cpr_state_close();
-        cpr_transfer_source_destroy(s);
+    if (cpr_transfer_source_active(s)) {
+        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
+        assert(setup && !s->to_dst_file);
+        migration_cleanup(s);
+        /* Now all things should have been released */
+        assert(!cpr_transfer_source_active(s));
     }
 }
 
@@ -2270,11 +2275,22 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
                                       void *opaque)
 {
     MigrationAddress *addr = opaque;
+    MigrationState *s = migrate_get_current();
+
+    /*
+     * Detach and release the GSource right after use.  We rely on this to
+     * detect this small cpr-transfer window of "waiting for HUP event".
+     */
+    cpr_transfer_source_destroy(s);
 
     qmp_migrate_finish(addr, NULL);
 
     cpr_state_close();
-    cpr_transfer_source_destroy(migrate_get_current());
+
+    /*
+     * This is redundant as we do cpr_transfer_source_destroy() at the
+     * entry, but it's benign; glib will just skip the detach.
+     */
     return G_SOURCE_REMOVE;
 }
 
diff --git a/migration/migration.h b/migration/migration.h
index a7ea43e925d..ee15ca88461 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -509,6 +509,11 @@ struct MigrationState {
     /* Is this a rdma migration */
     bool rdma_migration;
 
+    /*
+     * When set, it means cpr-transfer is waiting for the HUP signal from
+     * destination to continue the 2nd step of migration via the main
+     * channel.
+     */
     GSource *hup_source;
 };
 

-- 
2.47.1