[PATCH 0/2] VFIO device migration parallel state transitions

[PATCH 0/2] VFIO device migration parallel state transitions

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

When multiple VFIO devices are present in a VM the fact that their state
transitions on migration happen sequentially has a visible impact on
migration downtime.

This is because both PRE_COPY -> PRE_COPY_P2P -> STOP_COPY transitions on
the source and RESUMING -> RUNNING_P2P -> RUNNING transitions on the target
happen during the switchover phase.
During this phase the VM is stopped so the downtime is ticking.

These device state transitions are performed by VM state change handlers
registered by the VFIO device migration code.

Instead of performing such state transition synchronously launch a thread
performing the state change in parallel with other VFIO devices and other
VM state change handlers at the particular VFIO device qdev tree depth.
Only wait for this thread to finish *after* all other handlers at this
tree depth finish doing their jobs.

To implement the above allow adjustment of priority for VM state change
handlers - specifically allow registering qdev VM state change handlers
below and above the normal priority level for the registering device qdev
tree depth, but still properly ordered with respect to handlers
registered at other tree depths.

This way these state transitions can happen in parallel not only with
respect to other VFIO device instances but also ordinary (serialized)
handlers for other devices at this qdev tree depth.


Downtime results:
               4 VFs    2 VFs    1 VF
Disabled:    1385 ms   758 ms  497 ms
Enabled:      986 ms   653 ms  493 ms 
IMPROVEMENT:   ~29 %    ~14 %    ~0 %

Test VM shape:
vCPU 12 cores x 2 threads, 15 GiB RAM.

VFIO devices in the source and target machine:
Mellanox ConnectX-7 with 100GbE link and ~100 MiB of device state per VF.


Maciej S. Szmigiero (2):
  system/runstate: Allow adjustment of priority for VM state change
    handlers
  vfio/migration: Parallelize device state transitions

 hw/core/vm-change-state-handler.c |  22 ++--
 hw/vfio/migration.c               | 174 ++++++++++++++++++++++++++++--
 hw/vfio/pci.c                     |   2 +
 hw/vfio/vfio-migration-internal.h |   4 +-
 include/hw/vfio/vfio-device.h     |   1 +
 include/system/runstate.h         |   2 +-
 6 files changed, 189 insertions(+), 16 deletions(-)

[PATCH 1/2] system/runstate: Allow adjustment of priority for VM state change handlers

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

A future patch will need an ability to register qdev VM state change
handlers below and above the normal priority level for the registering
device qdev tree depth, but still properly ordered with respect to handlers
registered at other tree depths.

To implement this split the priority argument passed to
qemu_add_vm_change_state_handler_prio_full() into two parts:
its 15 most significant bits will now carry the actual qdev tree depth
while the 16 least significant bits will now carry the caller provided
priority adjustment value.

Although this will limit the qdev tree to a depth of 32k such high limit
shouldn't be a problem in practice.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 hw/core/vm-change-state-handler.c | 22 +++++++++++++++-------
 hw/vfio/migration.c               |  2 +-
 include/system/runstate.h         |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/core/vm-change-state-handler.c b/hw/core/vm-change-state-handler.c
index 2c111350298d..3db0819984c6 100644
--- a/hw/core/vm-change-state-handler.c
+++ b/hw/core/vm-change-state-handler.c
@@ -19,9 +19,9 @@
 #include "hw/core/qdev.h"
 #include "system/runstate.h"
 
-static int qdev_get_dev_tree_depth(DeviceState *dev)
+static unsigned int qdev_get_dev_tree_depth(DeviceState *dev)
 {
-    int depth;
+    unsigned int depth;
 
     for (depth = 0; dev; depth++) {
         BusState *bus = dev->parent_bus;
@@ -61,20 +61,28 @@ VMChangeStateEntry *qdev_add_vm_change_state_handler(DeviceState *dev,
                                                      void *opaque)
 {
     assert(!cb || !cb_ret);
-    return qdev_add_vm_change_state_handler_full(dev, cb, NULL, cb_ret, opaque);
+    return qdev_add_vm_change_state_handler_full(dev, cb, NULL, cb_ret, opaque, 0);
 }
 
 /*
  * Exactly like qdev_add_vm_change_state_handler() but passes a prepare_cb
- * and the cb_ret arguments too.
+ * and the cb_ret arguments too and allows for adjustment of priority.
  */
 VMChangeStateEntry *qdev_add_vm_change_state_handler_full(
     DeviceState *dev, VMChangeStateHandler *cb, VMChangeStateHandler *prepare_cb,
-    VMChangeStateHandlerWithRet *cb_ret, void *opaque)
+    VMChangeStateHandlerWithRet *cb_ret, void *opaque, int adj)
 {
-    int depth = qdev_get_dev_tree_depth(dev);
+    unsigned int depth = qdev_get_dev_tree_depth(dev);
+    int prio;
+
+    /* 32k depth should be enough for everyone */
+    assert(depth <= INT16_MAX);
+
+    /* encode depth on 15 MSB and adj on 16 LSB */
+    assert(adj >= INT16_MIN && adj <= INT16_MAX);
+    prio = (depth << 16) + (adj - INT16_MIN);
 
     assert(!cb || !cb_ret);
     return qemu_add_vm_change_state_handler_prio_full(cb, prepare_cb, cb_ret,
-                                                      opaque, depth);
+                                                      opaque, prio);
 }
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index dbfd13b83a15..9889b20ad7dd 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -1227,7 +1227,7 @@ static int vfio_migration_init(VFIODevice *vbasedev, Error **errp)
                      vfio_vmstate_change_prepare :
                      NULL;
     migration->vm_state = qdev_add_vm_change_state_handler_full(
-        vbasedev->dev, vfio_vmstate_change, prepare_cb, NULL, vbasedev);
+        vbasedev->dev, vfio_vmstate_change, prepare_cb, NULL, vbasedev, 0);
     migration_add_notifier(&migration->migration_state,
                            vfio_migration_state_notifier);
 
diff --git a/include/system/runstate.h b/include/system/runstate.h
index 929379adae41..306e2684c195 100644
--- a/include/system/runstate.h
+++ b/include/system/runstate.h
@@ -69,7 +69,7 @@ VMChangeStateEntry *qdev_add_vm_change_state_handler(DeviceState *dev,
                                                      void *opaque);
 VMChangeStateEntry *qdev_add_vm_change_state_handler_full(
     DeviceState *dev, VMChangeStateHandler *cb, VMChangeStateHandler *prepare_cb,
-    VMChangeStateHandlerWithRet *cb_ret, void *opaque);
+    VMChangeStateHandlerWithRet *cb_ret, void *opaque, int adj);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 /**
  * vm_state_notify: Notify the state of the VM

[PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

When multiple VFIO devices are present in a VM the fact that their state
transitions on migration happen sequentially has a visible impact on
migration downtime.

This is because both PRE_COPY -> PRE_COPY_P2P -> STOP_COPY transitions on
the source and RESUMING -> RUNNING_P2P -> RUNNING transitions on the target
happen during the switchover phase.
During this phase the VM is stopped so the downtime is ticking.

These device state transitions are performed by VM state change handlers
registered by the VFIO device migration code.

Instead of performing such state transition synchronously use the priority
adjustment mechanism from the previous patch to launch a thread performing
the state change at the priority level *before* all other VM state change
handlers at the particular VFIO device qdev tree depth.
Only wait for this thread to finish at the priority level ordered *after*
all other handlers at this tree depth.

This way these state transitions can happen in parallel not only with
respect to other VFIO device instances but also ordinary (serialized)
handlers for other devices at this qdev tree depth while still being
properly ordered with respect to handlers registered at other tree depths.

Unfortunately, the order in which VM state handlers are called depends
on whether the VM is starting or stopping.
Because of this, one extra layer of indirection is necessary to make the
(first, second) ordering of these handlers constant.

Enable the feature by default since it has no impact on the migration bit
stream protocol - it shouldn't need disabling for anything else but
debugging scenarios.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 hw/vfio/migration.c               | 174 ++++++++++++++++++++++++++++--
 hw/vfio/pci.c                     |   2 +
 hw/vfio/vfio-migration-internal.h |   4 +-
 include/hw/vfio/vfio-device.h     |   1 +
 4 files changed, 173 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 9889b20ad7dd..fd2b37a85f4b 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -1047,6 +1047,135 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
                               mig_state_to_str(new_state));
 }
 
+typedef struct VMStateChangeThreadData {
+    VFIODevice *vbasedev;
+    bool is_prepare;
+    bool running;
+    RunState state;
+} VMStateChangeThreadData;
+
+static void *vfio_vmstate_change_thread(void *opaque)
+{
+    g_autofree VMStateChangeThreadData *data = opaque;
+
+    if (data->is_prepare) {
+        vfio_vmstate_change_prepare(data->vbasedev, data->running, data->state);
+    } else {
+        vfio_vmstate_change(data->vbasedev, data->running, data->state);
+    }
+
+    return NULL;
+}
+
+static void vfio_vmstate_change_thread_launch(VFIODevice *vbasedev,
+                                              bool is_prepare,
+                                              bool running,
+                                              RunState state)
+{
+    VFIOMigration *migration = vbasedev->migration;
+    VMStateChangeThreadData *data = g_new(VMStateChangeThreadData, 1);
+
+    data->vbasedev = vbasedev;
+    data->is_prepare = is_prepare;
+    data->running = running;
+    data->state = state;
+
+    assert(!migration->vm_state_thread_running);
+    migration->vm_state_thread_running = true;
+
+    qemu_thread_create(&migration->vm_state_thread,
+                       is_prepare ? "vfio_vmstate_change_prepare" :
+                       "vfio_vmstate_change",
+                       vfio_vmstate_change_thread, data,
+                       QEMU_THREAD_JOINABLE);
+}
+
+static void vfio_vmstate_change_thread_join(VFIODevice *vbasedev)
+{
+    VFIOMigration *migration = vbasedev->migration;
+
+    assert(migration->vm_state_thread_running);
+
+    qemu_thread_join(&migration->vm_state_thread);
+
+    migration->vm_state_thread_running = false;
+}
+
+/*
+ * The first handler called during a vmstate change at a particular depth -
+ * launch the VFIO device state change thread.
+ */
+static void vfio_vmstate_change_first(VFIODevice *vbasedev,
+                                      bool is_prepare,
+                                      bool running, RunState state)
+{
+    vfio_vmstate_change_thread_launch(vbasedev,
+                                      is_prepare,
+                                      running,
+                                      state);
+}
+
+/*
+ * The last handler called during a vmstate change at a particular depth -
+ * wait for the VFIO device state change thread to finish.
+ */
+static void vfio_vmstate_change_second(VFIODevice *vbasedev)
+{
+    vfio_vmstate_change_thread_join(vbasedev);
+}
+
+/*
+ * Lower priority number handler:
+ * Called before higher number handler when VM is starting
+ * but after higher number handler when VM is stopping.
+ */
+static void vfio_vmstate_change_prepare_lower_prio(void *opaque, bool running,
+                                                   RunState state)
+{
+    if (running) {
+        vfio_vmstate_change_first(opaque, true, running, state);
+    } else {
+        vfio_vmstate_change_second(opaque);
+    }
+}
+
+/*
+ * Higher priority number handler:
+ * Called after lower number handler when VM is starting
+ * but before lower number handler when VM is stopping.
+ */
+static void vfio_vmstate_change_prepare_higher_prio(void *opaque, bool running,
+                                                    RunState state)
+{
+    if (running) {
+        vfio_vmstate_change_second(opaque);
+    } else {
+        vfio_vmstate_change_first(opaque, true, running, state);
+    }
+}
+
+/* Same ordering issues as for vfio_vmstate_change_prepare_lower_prio() */
+static void vfio_vmstate_change_lower_prio(void *opaque, bool running,
+                                           RunState state)
+{
+    if (running) {
+        vfio_vmstate_change_first(opaque, false, running, state);
+    } else {
+        vfio_vmstate_change_second(opaque);
+    }
+}
+
+/* Same ordering issues as for vfio_vmstate_change_prepare_higher_prio() */
+static void vfio_vmstate_change_higher_prio(void *opaque, bool running,
+                                            RunState state)
+{
+    if (running) {
+        vfio_vmstate_change_second(opaque);
+    } else {
+        vfio_vmstate_change_first(opaque, false, running, state);
+    }
+}
+
 static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
                                          MigrationEvent *e, Error **errp)
 {
@@ -1063,6 +1192,8 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
          * MigrationNotifyFunc may not return an error code and an Error
          * object for MIG_EVENT_FAILED. Hence, report the error
          * locally and ignore the errp argument.
+         * This state change is not parallelized as it is not expected to be
+         * performance critical.
          */
         ret = vfio_migration_set_state_or_reset(vbasedev,
                                                 VFIO_DEVICE_STATE_RUNNING,
@@ -1143,7 +1274,7 @@ static int vfio_migration_init(VFIODevice *vbasedev, Error **errp)
     char id[256] = "";
     g_autofree char *path = NULL, *oid = NULL;
     uint64_t mig_flags = 0;
-    VMChangeStateHandler *prepare_cb;
+    VMChangeStateHandler *prepare_cb, *prepare_cb_lower, *prepare_cb_higher;
 
     if (!vbasedev->ops->vfio_get_object) {
         ret = -EINVAL;
@@ -1223,11 +1354,34 @@ static int vfio_migration_init(VFIODevice *vbasedev, Error **errp)
     register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
                          vbasedev);
 
-    prepare_cb = migration->mig_flags & VFIO_MIGRATION_P2P ?
-                     vfio_vmstate_change_prepare :
-                     NULL;
-    migration->vm_state = qdev_add_vm_change_state_handler_full(
-        vbasedev->dev, vfio_vmstate_change, prepare_cb, NULL, vbasedev, 0);
+    if (vbasedev->migration_parallel_states) {
+        /*
+         * Unfortunately, the order in which vmstate handlers are called depends
+         * on whether the VM is starting or stopping.
+         * Because of this, one extra layer of indirection is necessary
+         * to make the (first, second) ordering of these handlers constant.
+         */
+        prepare_cb_lower = migration->mig_flags & VFIO_MIGRATION_P2P ?
+            vfio_vmstate_change_prepare_lower_prio : NULL;
+        prepare_cb_higher = migration->mig_flags & VFIO_MIGRATION_P2P ?
+            vfio_vmstate_change_prepare_higher_prio : NULL;
+        migration->vm_state_lower_prio = qdev_add_vm_change_state_handler_full(
+            vbasedev->dev, vfio_vmstate_change_lower_prio, prepare_cb_lower,
+            NULL, vbasedev, -1);
+        migration->vm_state_higher_prio = qdev_add_vm_change_state_handler_full(
+            vbasedev->dev, vfio_vmstate_change_higher_prio, prepare_cb_higher,
+            NULL, vbasedev, 1);
+    } else {
+        prepare_cb = migration->mig_flags & VFIO_MIGRATION_P2P ?
+            vfio_vmstate_change_prepare : NULL;
+        /* Arbitrarily use lower_prio field to store non-parallel handler */
+        migration->vm_state_lower_prio =
+            qdev_add_vm_change_state_handler_full(vbasedev->dev,
+                                                  vfio_vmstate_change,
+                                                  prepare_cb, NULL,
+                                                  vbasedev, 0);
+    }
+
     migration_add_notifier(&migration->migration_state,
                            vfio_migration_state_notifier);
 
@@ -1302,7 +1456,13 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
     VFIOMigration *migration = vbasedev->migration;
 
     migration_remove_notifier(&migration->migration_state);
-    qemu_del_vm_change_state_handler(migration->vm_state);
+
+    if (vbasedev->migration_parallel_states) {
+        qemu_del_vm_change_state_handler(migration->vm_state_higher_prio);
+    }
+    /* Non-parallel state change uses lower_prio field to store its handler */
+    qemu_del_vm_change_state_handler(migration->vm_state_lower_prio);
+
     unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
     vfio_migration_free(vbasedev);
     vfio_unblock_multiple_devices_migration();
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b2a07f6bb421..fa2411474c9b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3777,6 +3777,8 @@ static const Property vfio_pci_properties[] = {
                             ON_OFF_AUTO_AUTO),
     DEFINE_PROP_SIZE("x-migration-max-queued-buffers-size", VFIOPCIDevice,
                      vbasedev.migration_max_queued_buffers_size, UINT64_MAX),
+    DEFINE_PROP_BOOL("x-migration-parallel-states", VFIOPCIDevice,
+                     vbasedev.migration_parallel_states, true),
     DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
                      vbasedev.migration_events, false),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
index a1c58b112685..9fb00f9f4d7d 100644
--- a/hw/vfio/vfio-migration-internal.h
+++ b/hw/vfio/vfio-migration-internal.h
@@ -38,7 +38,9 @@ typedef struct VFIOMultifd VFIOMultifd;
 
 typedef struct VFIOMigration {
     struct VFIODevice *vbasedev;
-    VMChangeStateEntry *vm_state;
+    VMChangeStateEntry *vm_state_lower_prio, *vm_state_higher_prio;
+    QemuThread vm_state_thread;
+    bool vm_state_thread_running;
     NotifierWithReturn migration_state;
     uint32_t device_state;
     int data_fd;
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 380a55d6e5ea..28004e1e99f4 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -69,6 +69,7 @@ typedef struct VFIODevice {
     OnOffAuto migration_multifd_transfer;
     OnOffAuto migration_load_config_after_iter;
     uint64_t migration_max_queued_buffers_size;
+    bool migration_parallel_states;
     bool migration_events;
     bool use_region_fds;
     VFIODeviceOps *ops;

Re: [PATCH 1/2] system/runstate: Allow adjustment of priority for VM state change handlers

[Fabiano Rosas]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> A future patch will need an ability to register qdev VM state change
> handlers below and above the normal priority level for the registering
> device qdev tree depth, but still properly ordered with respect to handlers
> registered at other tree depths.
>
> To implement this split the priority argument passed to
> qemu_add_vm_change_state_handler_prio_full() into two parts:
> its 15 most significant bits will now carry the actual qdev tree depth
> while the 16 least significant bits will now carry the caller provided
> priority adjustment value.
>
> Although this will limit the qdev tree to a depth of 32k such high limit
> shouldn't be a problem in practice.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Fabiano Rosas]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> When multiple VFIO devices are present in a VM the fact that their state
> transitions on migration happen sequentially has a visible impact on
> migration downtime.
>
> This is because both PRE_COPY -> PRE_COPY_P2P -> STOP_COPY transitions on
> the source and RESUMING -> RUNNING_P2P -> RUNNING transitions on the target
> happen during the switchover phase.
> During this phase the VM is stopped so the downtime is ticking.
>
> These device state transitions are performed by VM state change handlers
> registered by the VFIO device migration code.
>
> Instead of performing such state transition synchronously use the priority
> adjustment mechanism from the previous patch to launch a thread performing
> the state change at the priority level *before* all other VM state change
> handlers at the particular VFIO device qdev tree depth.
> Only wait for this thread to finish at the priority level ordered *after*
> all other handlers at this tree depth.
>
> This way these state transitions can happen in parallel not only with
> respect to other VFIO device instances but also ordinary (serialized)
> handlers for other devices at this qdev tree depth while still being
> properly ordered with respect to handlers registered at other tree depths.
>
> Unfortunately, the order in which VM state handlers are called depends
> on whether the VM is starting or stopping.
> Because of this, one extra layer of indirection is necessary to make the
> (first, second) ordering of these handlers constant.
>
> Enable the feature by default since it has no impact on the migration bit
> stream protocol - it shouldn't need disabling for anything else but
> debugging scenarios.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>  hw/vfio/migration.c               | 174 ++++++++++++++++++++++++++++--
>  hw/vfio/pci.c                     |   2 +
>  hw/vfio/vfio-migration-internal.h |   4 +-
>  include/hw/vfio/vfio-device.h     |   1 +
>  4 files changed, 173 insertions(+), 8 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 9889b20ad7dd..fd2b37a85f4b 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -1047,6 +1047,135 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>                                mig_state_to_str(new_state));
>  }
>  
> +typedef struct VMStateChangeThreadData {
> +    VFIODevice *vbasedev;
> +    bool is_prepare;
> +    bool running;
> +    RunState state;
> +} VMStateChangeThreadData;
> +
> +static void *vfio_vmstate_change_thread(void *opaque)
> +{
> +    g_autofree VMStateChangeThreadData *data = opaque;
> +
> +    if (data->is_prepare) {
> +        vfio_vmstate_change_prepare(data->vbasedev, data->running, data->state);
> +    } else {
> +        vfio_vmstate_change(data->vbasedev, data->running, data->state);
> +    }
> +
> +    return NULL;
> +}
> +
> +static void vfio_vmstate_change_thread_launch(VFIODevice *vbasedev,
> +                                              bool is_prepare,
> +                                              bool running,
> +                                              RunState state)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +    VMStateChangeThreadData *data = g_new(VMStateChangeThreadData, 1);
> +
> +    data->vbasedev = vbasedev;
> +    data->is_prepare = is_prepare;
> +    data->running = running;
> +    data->state = state;
> +
> +    assert(!migration->vm_state_thread_running);
> +    migration->vm_state_thread_running = true;
> +
> +    qemu_thread_create(&migration->vm_state_thread,
> +                       is_prepare ? "vfio_vmstate_change_prepare" :
> +                       "vfio_vmstate_change",
> +                       vfio_vmstate_change_thread, data,
> +                       QEMU_THREAD_JOINABLE);
> +}
> +
> +static void vfio_vmstate_change_thread_join(VFIODevice *vbasedev)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +
> +    assert(migration->vm_state_thread_running);
> +
> +    qemu_thread_join(&migration->vm_state_thread);
> +
> +    migration->vm_state_thread_running = false;
> +}
> +
> +/*
> + * The first handler called during a vmstate change at a particular depth -
> + * launch the VFIO device state change thread.
> + */
> +static void vfio_vmstate_change_first(VFIODevice *vbasedev,
> +                                      bool is_prepare,
> +                                      bool running, RunState state)
> +{
> +    vfio_vmstate_change_thread_launch(vbasedev,
> +                                      is_prepare,
> +                                      running,
> +                                      state);
> +}
> +
> +/*
> + * The last handler called during a vmstate change at a particular depth -
> + * wait for the VFIO device state change thread to finish.
> + */
> +static void vfio_vmstate_change_second(VFIODevice *vbasedev)
> +{
> +    vfio_vmstate_change_thread_join(vbasedev);
> +}
> +
> +/*
> + * Lower priority number handler:
> + * Called before higher number handler when VM is starting
> + * but after higher number handler when VM is stopping.
> + */
> +static void vfio_vmstate_change_prepare_lower_prio(void *opaque, bool running,
> +                                                   RunState state)
> +{
> +    if (running) {
> +        vfio_vmstate_change_first(opaque, true, running, state);
> +    } else {
> +        vfio_vmstate_change_second(opaque);
> +    }
> +}
> +
> +/*
> + * Higher priority number handler:
> + * Called after lower number handler when VM is starting
> + * but before lower number handler when VM is stopping.
> + */
> +static void vfio_vmstate_change_prepare_higher_prio(void *opaque, bool running,
> +                                                    RunState state)
> +{
> +    if (running) {
> +        vfio_vmstate_change_second(opaque);
> +    } else {
> +        vfio_vmstate_change_first(opaque, true, running, state);
> +    }
> +}
> +
> +/* Same ordering issues as for vfio_vmstate_change_prepare_lower_prio() */
> +static void vfio_vmstate_change_lower_prio(void *opaque, bool running,
> +                                           RunState state)
> +{
> +    if (running) {
> +        vfio_vmstate_change_first(opaque, false, running, state);
> +    } else {
> +        vfio_vmstate_change_second(opaque);
> +    }
> +}
> +
> +/* Same ordering issues as for vfio_vmstate_change_prepare_higher_prio() */
> +static void vfio_vmstate_change_higher_prio(void *opaque, bool running,
> +                                            RunState state)
> +{
> +    if (running) {
> +        vfio_vmstate_change_second(opaque);
> +    } else {
> +        vfio_vmstate_change_first(opaque, false, running, state);
> +    }
> +}
> +
>  static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
>                                           MigrationEvent *e, Error **errp)
>  {
> @@ -1063,6 +1192,8 @@ static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
>           * MigrationNotifyFunc may not return an error code and an Error
>           * object for MIG_EVENT_FAILED. Hence, report the error
>           * locally and ignore the errp argument.
> +         * This state change is not parallelized as it is not expected to be
> +         * performance critical.
>           */
>          ret = vfio_migration_set_state_or_reset(vbasedev,
>                                                  VFIO_DEVICE_STATE_RUNNING,
> @@ -1143,7 +1274,7 @@ static int vfio_migration_init(VFIODevice *vbasedev, Error **errp)
>      char id[256] = "";
>      g_autofree char *path = NULL, *oid = NULL;
>      uint64_t mig_flags = 0;
> -    VMChangeStateHandler *prepare_cb;
> +    VMChangeStateHandler *prepare_cb, *prepare_cb_lower, *prepare_cb_higher;
>  
>      if (!vbasedev->ops->vfio_get_object) {
>          ret = -EINVAL;
> @@ -1223,11 +1354,34 @@ static int vfio_migration_init(VFIODevice *vbasedev, Error **errp)
>      register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
>                           vbasedev);
>  
> -    prepare_cb = migration->mig_flags & VFIO_MIGRATION_P2P ?
> -                     vfio_vmstate_change_prepare :
> -                     NULL;
> -    migration->vm_state = qdev_add_vm_change_state_handler_full(
> -        vbasedev->dev, vfio_vmstate_change, prepare_cb, NULL, vbasedev, 0);
> +    if (vbasedev->migration_parallel_states) {
> +        /*
> +         * Unfortunately, the order in which vmstate handlers are called depends
> +         * on whether the VM is starting or stopping.
> +         * Because of this, one extra layer of indirection is necessary
> +         * to make the (first, second) ordering of these handlers constant.
> +         */
> +        prepare_cb_lower = migration->mig_flags & VFIO_MIGRATION_P2P ?
> +            vfio_vmstate_change_prepare_lower_prio : NULL;
> +        prepare_cb_higher = migration->mig_flags & VFIO_MIGRATION_P2P ?
> +            vfio_vmstate_change_prepare_higher_prio : NULL;
> +        migration->vm_state_lower_prio = qdev_add_vm_change_state_handler_full(
> +            vbasedev->dev, vfio_vmstate_change_lower_prio, prepare_cb_lower,
> +            NULL, vbasedev, -1);
> +        migration->vm_state_higher_prio = qdev_add_vm_change_state_handler_full(
> +            vbasedev->dev, vfio_vmstate_change_higher_prio, prepare_cb_higher,
> +            NULL, vbasedev, 1);
> +    } else {
> +        prepare_cb = migration->mig_flags & VFIO_MIGRATION_P2P ?
> +            vfio_vmstate_change_prepare : NULL;
> +        /* Arbitrarily use lower_prio field to store non-parallel handler */
> +        migration->vm_state_lower_prio =
> +            qdev_add_vm_change_state_handler_full(vbasedev->dev,
> +                                                  vfio_vmstate_change,
> +                                                  prepare_cb, NULL,
> +                                                  vbasedev, 0);
> +    }
> +
>      migration_add_notifier(&migration->migration_state,
>                             vfio_migration_state_notifier);
>  
> @@ -1302,7 +1456,13 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
>      VFIOMigration *migration = vbasedev->migration;
>  
>      migration_remove_notifier(&migration->migration_state);
> -    qemu_del_vm_change_state_handler(migration->vm_state);
> +
> +    if (vbasedev->migration_parallel_states) {
> +        qemu_del_vm_change_state_handler(migration->vm_state_higher_prio);
> +    }
> +    /* Non-parallel state change uses lower_prio field to store its handler */
> +    qemu_del_vm_change_state_handler(migration->vm_state_lower_prio);
> +
>      unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
>      vfio_migration_free(vbasedev);
>      vfio_unblock_multiple_devices_migration();
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b2a07f6bb421..fa2411474c9b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3777,6 +3777,8 @@ static const Property vfio_pci_properties[] = {
>                              ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_SIZE("x-migration-max-queued-buffers-size", VFIOPCIDevice,
>                       vbasedev.migration_max_queued_buffers_size, UINT64_MAX),
> +    DEFINE_PROP_BOOL("x-migration-parallel-states", VFIOPCIDevice,
> +                     vbasedev.migration_parallel_states, true),
>      DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>                       vbasedev.migration_events, false),
>      DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
> diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
> index a1c58b112685..9fb00f9f4d7d 100644
> --- a/hw/vfio/vfio-migration-internal.h
> +++ b/hw/vfio/vfio-migration-internal.h
> @@ -38,7 +38,9 @@ typedef struct VFIOMultifd VFIOMultifd;
>  
>  typedef struct VFIOMigration {
>      struct VFIODevice *vbasedev;
> -    VMChangeStateEntry *vm_state;
> +    VMChangeStateEntry *vm_state_lower_prio, *vm_state_higher_prio;
> +    QemuThread vm_state_thread;
> +    bool vm_state_thread_running;
>      NotifierWithReturn migration_state;
>      uint32_t device_state;
>      int data_fd;
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 380a55d6e5ea..28004e1e99f4 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -69,6 +69,7 @@ typedef struct VFIODevice {
>      OnOffAuto migration_multifd_transfer;
>      OnOffAuto migration_load_config_after_iter;
>      uint64_t migration_max_queued_buffers_size;
> +    bool migration_parallel_states;
>      bool migration_events;
>      bool use_region_fds;
>      VFIODeviceOps *ops;

A bit idiosyncratic, but I don't have any better suggestions.

Also, pre-existing but maybe you'll want to fix it in this patch,
'vmstate_change' is the wrong term. This is vm_change_state or
vm_state_change.

Acked-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Mon, Jun 01, 2026 at 10:39:26AM -0300, Fabiano Rosas wrote:
> A bit idiosyncratic, but I don't have any better suggestions.

One thing I don't think proper with the current approach: if it's a real
"priority" it should be invoked always with high->low order, no matter if
running=true or not..  but that's not what will happen if we encode it with
the "depth" field.

That is an implication that we have two demands, and they aren't the same,
but this solution wrongly packed them together.

In 2023, VFIO already introduced VMChangeStateEntry.prepare_cb().  That is
a real impl of prority queues with only two: prepare_cb() always has higher
priority than the cb() itself.

Can VFIO simply leverage this existing interface, instead of hijacking
"depth" in an unwanted way?

I believe the _P2P states still need to happen first for all vfio devices,
then it can do the next step. Logically I think it can also be done
internally within VFIO by proper impl of these two callbacks, to achieve
both:

- Proper transition to P2P states first for all devices, meanwhile,
- Proper concurrency of all VFIO devices on device state transitions

Would that work in a cleaner way?

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 1.06.2026 21:33, Peter Xu wrote:
> On Mon, Jun 01, 2026 at 10:39:26AM -0300, Fabiano Rosas wrote:
>> A bit idiosyncratic, but I don't have any better suggestions.
> 
> One thing I don't think proper with the current approach: if it's a real
> "priority" it should be invoked always with high->low order, no matter if
> running=true or not..  but that's not what will happen if we encode it with
> the "depth" field.
> 
> That is an implication that we have two demands, and they aren't the same,
> but this solution wrongly packed them together.
> 
> In 2023, VFIO already introduced VMChangeStateEntry.prepare_cb().  That is
> a real impl of prority queues with only two: prepare_cb() always has higher
> priority than the cb() itself.
> 
> Can VFIO simply leverage this existing interface, instead of hijacking
> "depth" in an unwanted way?
> 
> I believe the _P2P states still need to happen first for all vfio devices,
> then it can do the next step. Logically I think it can also be done
> internally within VFIO by proper impl of these two callbacks, to achieve
> both:
> 
> - Proper transition to P2P states first for all devices, meanwhile,
> - Proper concurrency of all VFIO devices on device state transitions
> 
> Would that work in a cleaner way?

In the VFIO migration save/source code we have two transitions:
PRE_COPY -> PRE_COPY_P2P done via prepare_cb() and
PRE_COPY_P2P -> STOP_COPY done via cb().
On the load/target side the situation is similar, only states are
different.

For each of these transitions we need two callbacks: one has to launch
per-VFIO device state change thread and the other one has to "collect"
(or join) these threads.

All handlers of the first callback have to be complete before calling
any handler of the second callback since otherwise there would be
VFIO device vs VFIO device serialization.

Moreover, all the prepare_cb()-related transitions have to be done
before any of the cb()-related is started.


I presume what you suggest above is to launch per-VFIO device state
transition thread in prepare_cb() which would then be responsible for
doing *both* state transitions for this VFIO device.
And then collect this thread in cb().

However, this would need some extra interlocking:
- A lock inside VFIO code shared by all VFIO devices making sure
no VFIO device starts the second transition (the cb() one) before
all VFIO devices finish the first transition (the prepare_cb() one).

- Looking at the current QEMU code, there are some non-VFIO cb()
handlers registered via qdev_add_vm_change_state_handler() and
quite a few handlers registered via qemu_add_vm_change_state_handler().

I presume these VFIO prepare_cb()-related transitions need to finish
before calling any of these handlers to maintain the current ordering
so some kind of interlocking in vm_state_notify() between calling
prepare_cb() and cb() may be necessary since any "prio" based
ordering will have the same problem of reverse calling order on VM
start vs stop.


To be clear, I'm not insisting on the implementation from this patch -
any alternative one that preserves the aforementioned ordering rules
and has similar downtime performance is probably okay too.

> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Tue, Jun 02, 2026 at 12:01:32PM +0200, Maciej S. Szmigiero wrote:
> On 1.06.2026 21:33, Peter Xu wrote:
> > On Mon, Jun 01, 2026 at 10:39:26AM -0300, Fabiano Rosas wrote:
> > > A bit idiosyncratic, but I don't have any better suggestions.
> > 
> > One thing I don't think proper with the current approach: if it's a real
> > "priority" it should be invoked always with high->low order, no matter if
> > running=true or not..  but that's not what will happen if we encode it with
> > the "depth" field.
> > 
> > That is an implication that we have two demands, and they aren't the same,
> > but this solution wrongly packed them together.
> > 
> > In 2023, VFIO already introduced VMChangeStateEntry.prepare_cb().  That is
> > a real impl of prority queues with only two: prepare_cb() always has higher
> > priority than the cb() itself.
> > 
> > Can VFIO simply leverage this existing interface, instead of hijacking
> > "depth" in an unwanted way?
> > 
> > I believe the _P2P states still need to happen first for all vfio devices,
> > then it can do the next step. Logically I think it can also be done
> > internally within VFIO by proper impl of these two callbacks, to achieve
> > both:
> > 
> > - Proper transition to P2P states first for all devices, meanwhile,
> > - Proper concurrency of all VFIO devices on device state transitions
> > 
> > Would that work in a cleaner way?
> 
> In the VFIO migration save/source code we have two transitions:
> PRE_COPY -> PRE_COPY_P2P done via prepare_cb() and
> PRE_COPY_P2P -> STOP_COPY done via cb().
> On the load/target side the situation is similar, only states are
> different.
> 
> For each of these transitions we need two callbacks: one has to launch
> per-VFIO device state change thread and the other one has to "collect"
> (or join) these threads.
> 
> All handlers of the first callback have to be complete before calling
> any handler of the second callback since otherwise there would be
> VFIO device vs VFIO device serialization.
> 
> Moreover, all the prepare_cb()-related transitions have to be done
> before any of the cb()-related is started.
> 
> 
> I presume what you suggest above is to launch per-VFIO device state
> transition thread in prepare_cb() which would then be responsible for
> doing *both* state transitions for this VFIO device.
> And then collect this thread in cb().

Yes.

> 
> However, this would need some extra interlocking:
> - A lock inside VFIO code shared by all VFIO devices making sure
> no VFIO device starts the second transition (the cb() one) before
> all VFIO devices finish the first transition (the prepare_cb() one).

Yes, more below.

> 
> - Looking at the current QEMU code, there are some non-VFIO cb()
> handlers registered via qdev_add_vm_change_state_handler() and
> quite a few handlers registered via qemu_add_vm_change_state_handler().
> 
> I presume these VFIO prepare_cb()-related transitions need to finish
> before calling any of these handlers to maintain the current ordering

IIUC, we don't necessarily need to keep this ordering; or do you see any
real demand of that?

What I see is that the order is only introduced due to P2P state of VFIO,
it seems very specific to me.

> so some kind of interlocking in vm_state_notify() between calling
> prepare_cb() and cb() may be necessary since any "prio" based
> ordering will have the same problem of reverse calling order on VM
> start vs stop.
> 
> 
> To be clear, I'm not insisting on the implementation from this patch -
> any alternative one that preserves the aforementioned ordering rules
> and has similar downtime performance is probably okay too.

The problem is, IIUC patch 1 needs better justification on its own, and I
feel like it's not doing the right thing, as discussed previously. So even
if we want to introduce a new priority-based approach, we may want to
rethink patch 1.

IMHO it'll be perfect if we can simply reuse prepare_cb().  It seems
working.

Due to this demand, I got to learn we have pthread_barrier_init(), but..
personally I like a simple atomic+event approach:

vfio thread:

  # Do step 1, ->P2P
  if (qatomic_fetch_inc(&vfio_p2p_done) + 1 == vfio_total_devices) {                                                                  
      qemu_event_set(&vfio_p2p_done_event);
  }                                                                  
  qemu_event_wait(&vfio_p2p_done_event);
  # Do step 2, ->STOPCOPY

cb() only join()s.

Would this work?

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 2.06.2026 22:17, Peter Xu wrote:
> On Tue, Jun 02, 2026 at 12:01:32PM +0200, Maciej S. Szmigiero wrote:
>> On 1.06.2026 21:33, Peter Xu wrote:
>>> On Mon, Jun 01, 2026 at 10:39:26AM -0300, Fabiano Rosas wrote:
>>>> A bit idiosyncratic, but I don't have any better suggestions.
>>>
>>> One thing I don't think proper with the current approach: if it's a real
>>> "priority" it should be invoked always with high->low order, no matter if
>>> running=true or not..  but that's not what will happen if we encode it with
>>> the "depth" field.
>>>
>>> That is an implication that we have two demands, and they aren't the same,
>>> but this solution wrongly packed them together.
>>>
>>> In 2023, VFIO already introduced VMChangeStateEntry.prepare_cb().  That is
>>> a real impl of prority queues with only two: prepare_cb() always has higher
>>> priority than the cb() itself.
>>>
>>> Can VFIO simply leverage this existing interface, instead of hijacking
>>> "depth" in an unwanted way?
>>>
>>> I believe the _P2P states still need to happen first for all vfio devices,
>>> then it can do the next step. Logically I think it can also be done
>>> internally within VFIO by proper impl of these two callbacks, to achieve
>>> both:
>>>
>>> - Proper transition to P2P states first for all devices, meanwhile,
>>> - Proper concurrency of all VFIO devices on device state transitions
>>>
>>> Would that work in a cleaner way?
>>
>> In the VFIO migration save/source code we have two transitions:
>> PRE_COPY -> PRE_COPY_P2P done via prepare_cb() and
>> PRE_COPY_P2P -> STOP_COPY done via cb().
>> On the load/target side the situation is similar, only states are
>> different.
>>
>> For each of these transitions we need two callbacks: one has to launch
>> per-VFIO device state change thread and the other one has to "collect"
>> (or join) these threads.
>>
>> All handlers of the first callback have to be complete before calling
>> any handler of the second callback since otherwise there would be
>> VFIO device vs VFIO device serialization.
>>
>> Moreover, all the prepare_cb()-related transitions have to be done
>> before any of the cb()-related is started.
>>
>>
>> I presume what you suggest above is to launch per-VFIO device state
>> transition thread in prepare_cb() which would then be responsible for
>> doing *both* state transitions for this VFIO device.
>> And then collect this thread in cb().
> 
> Yes.
> 
>>
>> However, this would need some extra interlocking:
>> - A lock inside VFIO code shared by all VFIO devices making sure
>> no VFIO device starts the second transition (the cb() one) before
>> all VFIO devices finish the first transition (the prepare_cb() one).
> 
> Yes, more below.
> 
>>
>> - Looking at the current QEMU code, there are some non-VFIO cb()
>> handlers registered via qdev_add_vm_change_state_handler() and
>> quite a few handlers registered via qemu_add_vm_change_state_handler().
>>
>> I presume these VFIO prepare_cb()-related transitions need to finish
>> before calling any of these handlers to maintain the current ordering
> 
> IIUC, we don't necessarily need to keep this ordering; or do you see any
> real demand of that?
> 
> What I see is that the order is only introduced due to P2P state of VFIO,
> it seems very specific to me.

While I don't have a specific example why relaxing this ordering is unsafe
I am not certain that it is safe either - we can end up introducing subtle
race condition and possible guest memory corruption.

Maybe Alex and Cédric can provide some additional input here.

If nobody can authoritatively say that calling non-VFIO cb()
VM state change handlers before being certain that all VFIO devices
reached PRE_COPY_P2P / RUNNING_P2P state is indeed safe thing to do
I think we'll have to play safe here.

> 
>> so some kind of interlocking in vm_state_notify() between calling
>> prepare_cb() and cb() may be necessary since any "prio" based
>> ordering will have the same problem of reverse calling order on VM
>> start vs stop.
>>
>>
>> To be clear, I'm not insisting on the implementation from this patch -
>> any alternative one that preserves the aforementioned ordering rules
>> and has similar downtime performance is probably okay too.
> 
> The problem is, IIUC patch 1 needs better justification on its own, and I
> feel like it's not doing the right thing, as discussed previously. So even
> if we want to introduce a new priority-based approach, we may want to
> rethink patch 1.
> 
> IMHO it'll be perfect if we can simply reuse prepare_cb().  It seems
> working.
> 
> Due to this demand, I got to learn we have pthread_barrier_init(), but..
> personally I like a simple atomic+event approach:
> 
> vfio thread:
> 
>    # Do step 1, ->P2P
>    if (qatomic_fetch_inc(&vfio_p2p_done) + 1 == vfio_total_devices) {
>        qemu_event_set(&vfio_p2p_done_event);
>    }
>    qemu_event_wait(&vfio_p2p_done_event);
>    # Do step 2, ->STOPCOPY
> 
> cb() only join()s.
> 
> Would this work?

At the first glance it should work - of course, we'd need to introduce
the vfio_total_devices counter too.

However the issue about synchronization with other (non-VFIO) cb()
handlers described above still (possibly) remains.

> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 3.06.2026 23:34, Maciej S. Szmigiero wrote:
> On 2.06.2026 22:17, Peter Xu wrote:
>> On Tue, Jun 02, 2026 at 12:01:32PM +0200, Maciej S. Szmigiero wrote:
>>> On 1.06.2026 21:33, Peter Xu wrote:
>>>> On Mon, Jun 01, 2026 at 10:39:26AM -0300, Fabiano Rosas wrote:
>>>>> A bit idiosyncratic, but I don't have any better suggestions.
>>>>
>>>> One thing I don't think proper with the current approach: if it's a real
>>>> "priority" it should be invoked always with high->low order, no matter if
>>>> running=true or not..  but that's not what will happen if we encode it with
>>>> the "depth" field.
>>>>
>>>> That is an implication that we have two demands, and they aren't the same,
>>>> but this solution wrongly packed them together.
>>>>
>>>> In 2023, VFIO already introduced VMChangeStateEntry.prepare_cb().  That is
>>>> a real impl of prority queues with only two: prepare_cb() always has higher
>>>> priority than the cb() itself.
>>>>
>>>> Can VFIO simply leverage this existing interface, instead of hijacking
>>>> "depth" in an unwanted way?
>>>>
>>>> I believe the _P2P states still need to happen first for all vfio devices,
>>>> then it can do the next step. Logically I think it can also be done
>>>> internally within VFIO by proper impl of these two callbacks, to achieve
>>>> both:
>>>>
>>>> - Proper transition to P2P states first for all devices, meanwhile,
>>>> - Proper concurrency of all VFIO devices on device state transitions
>>>>
>>>> Would that work in a cleaner way?
>>>
>>> In the VFIO migration save/source code we have two transitions:
>>> PRE_COPY -> PRE_COPY_P2P done via prepare_cb() and
>>> PRE_COPY_P2P -> STOP_COPY done via cb().
>>> On the load/target side the situation is similar, only states are
>>> different.
>>>
>>> For each of these transitions we need two callbacks: one has to launch
>>> per-VFIO device state change thread and the other one has to "collect"
>>> (or join) these threads.
>>>
>>> All handlers of the first callback have to be complete before calling
>>> any handler of the second callback since otherwise there would be
>>> VFIO device vs VFIO device serialization.
>>>
>>> Moreover, all the prepare_cb()-related transitions have to be done
>>> before any of the cb()-related is started.
>>>
>>>
>>> I presume what you suggest above is to launch per-VFIO device state
>>> transition thread in prepare_cb() which would then be responsible for
>>> doing *both* state transitions for this VFIO device.
>>> And then collect this thread in cb().
>>
>> Yes.
>>
>>>
>>> However, this would need some extra interlocking:
>>> - A lock inside VFIO code shared by all VFIO devices making sure
>>> no VFIO device starts the second transition (the cb() one) before
>>> all VFIO devices finish the first transition (the prepare_cb() one).
>>
>> Yes, more below.
>>
>>>
>>> - Looking at the current QEMU code, there are some non-VFIO cb()
>>> handlers registered via qdev_add_vm_change_state_handler() and
>>> quite a few handlers registered via qemu_add_vm_change_state_handler().
>>>
>>> I presume these VFIO prepare_cb()-related transitions need to finish
>>> before calling any of these handlers to maintain the current ordering
>>
>> IIUC, we don't necessarily need to keep this ordering; or do you see any
>> real demand of that?
>>
>> What I see is that the order is only introduced due to P2P state of VFIO,
>> it seems very specific to me.
> 
> While I don't have a specific example why relaxing this ordering is unsafe
> I am not certain that it is safe either - we can end up introducing subtle
> race condition and possible guest memory corruption.
> 
> Maybe Alex and Cédric can provide some additional input here.
> 
> If nobody can authoritatively say that calling non-VFIO cb()
> VM state change handlers before being certain that all VFIO devices
> reached PRE_COPY_P2P / RUNNING_P2P state is indeed safe thing to do
> I think we'll have to play safe here.
> 

I looked more deeply into this and even prototyped a similar new design.

However, in the end I think that running VFIO state transitions in
parallel with other non-VFIO cb() handlers just isn't safe.

For example, on the migration source, the second transition is
PRE_COPY_P2P -> STOP_COPY.
PRE_COPY_P2P is the last state during migration that still allows the
device to accept DMA and similar interactions, while STOP_COPY is
already the "quiescent" state.

So if a VFIO has already reached STOP_COPY but a non-VFIO device
callback still hasn't finished, then any DMA/MMIO/P2P transaction from
that "unfinished" device to the VFIO device would now hit a VFIO device
that no longer can accept such transactions.
This may result in hard-to-debug memory corruption during live migration.

For a similar reason I think we need to preserve qdev tree depth based
ordering as its whole point is to make sure these transitions happen in
the correct order between different devices.

This is even more pressing considering that the VFIO hardware needed to
work on these issues isn't very widespread.

What I think we can do do instead, is to use a use an early prepare_cb()
handler registered by a VFIO code to launch a state changing thread
inside VFIO code.

Then a late prepare_cb() handler also registered by a VFIO code would
synchronize with these state changing threads at their first sync point:
completion of the first state change.
This could be achieved for example by doing an event wait similar to the
code snippet you had earlier suggested/provided.

The next sync point will be at an early cb() handler registered by VFIO code -
this will let these waiting state changing VFIO threads start the second
transition.

The final wait and joining of these threads will be at a late cb() handler
registered by the VFIO code.

>>
>>> so some kind of interlocking in vm_state_notify() between calling
>>> prepare_cb() and cb() may be necessary since any "prio" based
>>> ordering will have the same problem of reverse calling order on VM
>>> start vs stop.
>>>
>>>
>>> To be clear, I'm not insisting on the implementation from this patch -
>>> any alternative one that preserves the aforementioned ordering rules
>>> and has similar downtime performance is probably okay too.
>>
>> The problem is, IIUC patch 1 needs better justification on its own, and I
>> feel like it's not doing the right thing, as discussed previously. So even
>> if we want to introduce a new priority-based approach, we may want to
>> rethink patch 1.

Sure, any implementation that preserved the ordering I described above
would be good.
But just using existing single-priority-per-qdev-depth prepare_cb() and
cb() handlers unfortunately won't do it.

I think if there's a good idea how to implement such ordering in the
VM state change handlers then the VFIO side of patch can be adjusted
accordingly.

I also agree that adding the description of the issue being solved as
justification for patch 1 and maybe even as a comment somewhere in the
code would be kind to people looking at this code in the future and
would prevent future issues being accidentally introduced in this area.

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Thu, Jul 02, 2026 at 08:36:32PM +0200, Maciej S. Szmigiero wrote:
> I looked more deeply into this and even prototyped a similar new design.

Sorry for a late response; off work most of last week.

> 
> However, in the end I think that running VFIO state transitions in
> parallel with other non-VFIO cb() handlers just isn't safe.
> 
> For example, on the migration source, the second transition is
> PRE_COPY_P2P -> STOP_COPY.
> PRE_COPY_P2P is the last state during migration that still allows the
> device to accept DMA and similar interactions, while STOP_COPY is
> already the "quiescent" state.
> 
> So if a VFIO has already reached STOP_COPY but a non-VFIO device
> callback still hasn't finished, then any DMA/MMIO/P2P transaction from
> that "unfinished" device to the VFIO device would now hit a VFIO device
> that no longer can accept such transactions.
> This may result in hard-to-debug memory corruption during live migration.

Do you have any solid example of this problem?

AFAIU, P2P in VFIO state's term only means P2P DMAs _between_ host VFIO
devices, nothing to do with emulated.

I am not aware of any P2P that can be initiated from emulated devices, even
if it exists, it is only P2P from guest perspective not host: it will
become host operations finally from processor side.  I really don't think
it's a concern.

My limited understanding of VFIO's P2P state is: when switching to P2P
state, the VFIO device can still accept P2P DMA from other devices, but
never initiating P2P DMAs, and the latter sentence is critical, it means
the real quiecent state of the whole system happens after the last VFIO
device switching to P2P state: then it means none of the VFIO devices can
initiate P2P DMA anymore, whole system is quiesced.  That happens earlier
than reaching any of the cb()s here, because VFIO's prepare_cb() does that
P2P state switch.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 6.07.2026 18:23, Peter Xu wrote:
> On Thu, Jul 02, 2026 at 08:36:32PM +0200, Maciej S. Szmigiero wrote:
>> I looked more deeply into this and even prototyped a similar new design.
> 
> Sorry for a late response; off work most of last week.

Also sorry for my late response - was extremely busy the entire previous week.

>>
>> However, in the end I think that running VFIO state transitions in
>> parallel with other non-VFIO cb() handlers just isn't safe.
>>
>> For example, on the migration source, the second transition is
>> PRE_COPY_P2P -> STOP_COPY.
>> PRE_COPY_P2P is the last state during migration that still allows the
>> device to accept DMA and similar interactions, while STOP_COPY is
>> already the "quiescent" state.
>>
>> So if a VFIO has already reached STOP_COPY but a non-VFIO device
>> callback still hasn't finished, then any DMA/MMIO/P2P transaction from
>> that "unfinished" device to the VFIO device would now hit a VFIO device
>> that no longer can accept such transactions.
>> This may result in hard-to-debug memory corruption during live migration.
> 
> Do you have any solid example of this problem?
> 
> AFAIU, P2P in VFIO state's term only means P2P DMAs _between_ host VFIO
> devices, nothing to do with emulated.

I presume that quiescent VFIO devices can't accept *any* DMA, the spec (vfio.h)
even says that:
> The user should take steps to restrict access to vfio device regions while
> the device is in STOP_COPY or risk corruption of the device migration data
> stream.

Correspondingly, I think the same goes for RESUMING state on the migration
target since it is described there as:
> The device is *stopped* and is loading a new internal state

While you are right that the spec only talks about P2P DMA I don't see how
host-initiated DMA would be different here - after all, it's about changing
internal device state in a state where the device can't accept that anymore/yet.

> I am not aware of any P2P that can be initiated from emulated devices, even
> if it exists, it is only P2P from guest perspective not host: it will
> become host operations finally from processor side.  I really don't think
> it's a concern.

Yeah, it's normal DMA from the VFIO device perspective.
  > My limited understanding of VFIO's P2P state is: when switching to P2P
> state, the VFIO device can still accept P2P DMA from other devices, but
> never initiating P2P DMAs, and the latter sentence is critical, it means
> the real quiecent state of the whole system happens after the last VFIO
> device switching to P2P state: then it means none of the VFIO devices can
> initiate P2P DMA anymore, whole system is quiesced. 

Just having the VFIO devices in the system prevented from being able to
initiate DMA is not enough for them to reach "quiecent" or fully stable
state as long as there are other possible sources of DMA transactions
targeting them in the system.

Glancing at the existing vm_state_change handlers there are some
concerning ones there:
* vhost-net (called via virtio_vmstate_change() in base virtio-net),

In the vhost-net case the vm_state_change handler for this device
ultimately reaches vhost_net_stop() and do_vhost_dev_stop() and these
seem to stop this network device rings at this point.

Now, having vhost-net packet buffers in a VFIO device MMIO BAR would
be rather atypical configuration, but nevertheless letting VFIO
device possibly reach "quiecent" state before this handler finishes
would theoretically be a regression.

* virtio-blk virtio_blk_dma_restart_cb() vm_state_change handler
submits some block requests to its (possibly dedicated) IOThread.

Again, having block device request buffers in a VFIO device MMIO BAR
would not be a typical configuration, however letting this handler
run before making sure that VFIO devices can accept DMA would
theoretically make a regression too.

* same for scsi-bus scsi_dma_restart_cb() that does
scsi_dma_restart_req() for each pending request.


The above is not an exhaustive list, these are only the cases
I was able to identify relatively easily by going through the
code.
And I think that with possible memory corruption issues its better
to be safe than sorry, as they are pain to debug.

The only real non-VFIO cost of being safe about all of this is
~15 code lines addition to vm-change-state-handler.c, all the
remaining complexity is already on the VFIO side of things.
> That happens earlier
> than reaching any of the cb()s here, because VFIO's prepare_cb() does that
> P2P state switch.

I thought your proposed design was to launch (per-VFIO device)
device state changing thread in prepare_cb(), make it do *both*
VFIO device state transitions and collect it in cb()?

In this case the transitions are *NOT* ordered with respect to
other cb() handlers (besides them being finished by the time
the VFIO device qdev tree depth cb() handlers are reached by
virtue of the threads being collected at this point - this is
actually necessary in order to preserve proper ordering with
respect to some types of interrupt controllers on the source).

So on the migration source the thread could already put device
into "quiecent" state by the time other cb() handlers could run
and on the migration target the device could still be
in an "unprepared" state by the time other cb() handlers are called.

> 
> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Mon, Jul 13, 2026 at 10:23:07PM +0200, Maciej S. Szmigiero wrote:
> On 6.07.2026 18:23, Peter Xu wrote:
> > On Thu, Jul 02, 2026 at 08:36:32PM +0200, Maciej S. Szmigiero wrote:
> > > I looked more deeply into this and even prototyped a similar new design.
> > 
> > Sorry for a late response; off work most of last week.
> 
> Also sorry for my late response - was extremely busy the entire previous week.
> 
> > > 
> > > However, in the end I think that running VFIO state transitions in
> > > parallel with other non-VFIO cb() handlers just isn't safe.
> > > 
> > > For example, on the migration source, the second transition is
> > > PRE_COPY_P2P -> STOP_COPY.
> > > PRE_COPY_P2P is the last state during migration that still allows the
> > > device to accept DMA and similar interactions, while STOP_COPY is
> > > already the "quiescent" state.
> > > 
> > > So if a VFIO has already reached STOP_COPY but a non-VFIO device
> > > callback still hasn't finished, then any DMA/MMIO/P2P transaction from
> > > that "unfinished" device to the VFIO device would now hit a VFIO device
> > > that no longer can accept such transactions.
> > > This may result in hard-to-debug memory corruption during live migration.
> > 
> > Do you have any solid example of this problem?
> > 
> > AFAIU, P2P in VFIO state's term only means P2P DMAs _between_ host VFIO
> > devices, nothing to do with emulated.
> 
> I presume that quiescent VFIO devices can't accept *any* DMA, the spec (vfio.h)
> even says that:
> > The user should take steps to restrict access to vfio device regions while
> > the device is in STOP_COPY or risk corruption of the device migration data
> > stream.
> 
> Correspondingly, I think the same goes for RESUMING state on the migration
> target since it is described there as:
> > The device is *stopped* and is loading a new internal state
> 
> While you are right that the spec only talks about P2P DMA I don't see how
> host-initiated DMA would be different here - after all, it's about changing
> internal device state in a state where the device can't accept that anymore/yet.
> 
> > I am not aware of any P2P that can be initiated from emulated devices, even
> > if it exists, it is only P2P from guest perspective not host: it will
> > become host operations finally from processor side.  I really don't think
> > it's a concern.
> 
> Yeah, it's normal DMA from the VFIO device perspective.
>  > My limited understanding of VFIO's P2P state is: when switching to P2P
> > state, the VFIO device can still accept P2P DMA from other devices, but
> > never initiating P2P DMAs, and the latter sentence is critical, it means
> > the real quiecent state of the whole system happens after the last VFIO
> > device switching to P2P state: then it means none of the VFIO devices can
> > initiate P2P DMA anymore, whole system is quiesced.
> 
> Just having the VFIO devices in the system prevented from being able to
> initiate DMA is not enough for them to reach "quiecent" or fully stable
> state as long as there are other possible sources of DMA transactions
> targeting them in the system.
> 
> Glancing at the existing vm_state_change handlers there are some
> concerning ones there:
> * vhost-net (called via virtio_vmstate_change() in base virtio-net),
> 
> In the vhost-net case the vm_state_change handler for this device
> ultimately reaches vhost_net_stop() and do_vhost_dev_stop() and these
> seem to stop this network device rings at this point.
> 
> Now, having vhost-net packet buffers in a VFIO device MMIO BAR would
> be rather atypical configuration, but nevertheless letting VFIO
> device possibly reach "quiecent" state before this handler finishes
> would theoretically be a regression.

If this is an issue, it was an issue for master branch too, right?  Because
the master branch (with prepare_cb()s) already switch to P2P_DMA state for
all VFIO devices before invoking any cb().

It is also an issue then even before P2P_DMA state introduced, because
before that all cb()s can be run at random orders.

What you said might be an issue indeed in that extremely corner case, but I
think it still needs some justification, and it's IMHO orthogonal to what
we're talking about.  If it's an issue, maybe we want to fix it first
before attempting to move further to any concurrent approaches, but is it
really an issue is to be better justified.

Applies to all below.

> 
> * virtio-blk virtio_blk_dma_restart_cb() vm_state_change handler
> submits some block requests to its (possibly dedicated) IOThread.
> 
> Again, having block device request buffers in a VFIO device MMIO BAR
> would not be a typical configuration, however letting this handler
> run before making sure that VFIO devices can accept DMA would
> theoretically make a regression too.
> 
> * same for scsi-bus scsi_dma_restart_cb() that does
> scsi_dma_restart_req() for each pending request.
> 
> 
> The above is not an exhaustive list, these are only the cases
> I was able to identify relatively easily by going through the
> code.
> And I think that with possible memory corruption issues its better
> to be safe than sorry, as they are pain to debug.
> 
> The only real non-VFIO cost of being safe about all of this is
> ~15 code lines addition to vm-change-state-handler.c, all the
> remaining complexity is already on the VFIO side of things.
> > That happens earlier
> > than reaching any of the cb()s here, because VFIO's prepare_cb() does that
> > P2P state switch.
> 
> I thought your proposed design was to launch (per-VFIO device)
> device state changing thread in prepare_cb(), make it do *both*
> VFIO device state transitions and collect it in cb()?

The idea was only about trying to leverage prepare_cb() to achieve real
"prioritized callbacks" for a device, based on patch 1 being problematic.

What prepare_cb() does can be anything.  Certainly, we can make it only do
step 1 then in cb() kick all threads to move on to step 2, and sync again.

> 
> In this case the transitions are *NOT* ordered with respect to
> other cb() handlers (besides them being finished by the time
> the VFIO device qdev tree depth cb() handlers are reached by
> virtue of the threads being collected at this point - this is
> actually necessary in order to preserve proper ordering with
> respect to some types of interrupt controllers on the source).
> 
> So on the migration source the thread could already put device
> into "quiecent" state by the time other cb() handlers could run
> and on the migration target the device could still be
> in an "unprepared" state by the time other cb() handlers are called.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 14.07.2026 19:06, Peter Xu wrote:
> On Mon, Jul 13, 2026 at 10:23:07PM +0200, Maciej S. Szmigiero wrote:
>> On 6.07.2026 18:23, Peter Xu wrote:
>>> On Thu, Jul 02, 2026 at 08:36:32PM +0200, Maciej S. Szmigiero wrote:
>>>> I looked more deeply into this and even prototyped a similar new design.
>>>
>>> Sorry for a late response; off work most of last week.
>>
>> Also sorry for my late response - was extremely busy the entire previous week.
>>
>>>>
>>>> However, in the end I think that running VFIO state transitions in
>>>> parallel with other non-VFIO cb() handlers just isn't safe.
>>>>
>>>> For example, on the migration source, the second transition is
>>>> PRE_COPY_P2P -> STOP_COPY.
>>>> PRE_COPY_P2P is the last state during migration that still allows the
>>>> device to accept DMA and similar interactions, while STOP_COPY is
>>>> already the "quiescent" state.
>>>>
>>>> So if a VFIO has already reached STOP_COPY but a non-VFIO device
>>>> callback still hasn't finished, then any DMA/MMIO/P2P transaction from
>>>> that "unfinished" device to the VFIO device would now hit a VFIO device
>>>> that no longer can accept such transactions.
>>>> This may result in hard-to-debug memory corruption during live migration.
>>>
>>> Do you have any solid example of this problem?
>>>
>>> AFAIU, P2P in VFIO state's term only means P2P DMAs _between_ host VFIO
>>> devices, nothing to do with emulated.
>>
>> I presume that quiescent VFIO devices can't accept *any* DMA, the spec (vfio.h)
>> even says that:
>>> The user should take steps to restrict access to vfio device regions while
>>> the device is in STOP_COPY or risk corruption of the device migration data
>>> stream.
>>
>> Correspondingly, I think the same goes for RESUMING state on the migration
>> target since it is described there as:
>>> The device is *stopped* and is loading a new internal state
>>
>> While you are right that the spec only talks about P2P DMA I don't see how
>> host-initiated DMA would be different here - after all, it's about changing
>> internal device state in a state where the device can't accept that anymore/yet.
>>
>>> I am not aware of any P2P that can be initiated from emulated devices, even
>>> if it exists, it is only P2P from guest perspective not host: it will
>>> become host operations finally from processor side.  I really don't think
>>> it's a concern.
>>
>> Yeah, it's normal DMA from the VFIO device perspective.
>>   > My limited understanding of VFIO's P2P state is: when switching to P2P
>>> state, the VFIO device can still accept P2P DMA from other devices, but
>>> never initiating P2P DMAs, and the latter sentence is critical, it means
>>> the real quiecent state of the whole system happens after the last VFIO
>>> device switching to P2P state: then it means none of the VFIO devices can
>>> initiate P2P DMA anymore, whole system is quiesced.
>>
>> Just having the VFIO devices in the system prevented from being able to
>> initiate DMA is not enough for them to reach "quiecent" or fully stable
>> state as long as there are other possible sources of DMA transactions
>> targeting them in the system.
>>
>> Glancing at the existing vm_state_change handlers there are some
>> concerning ones there:
>> * vhost-net (called via virtio_vmstate_change() in base virtio-net),
>>
>> In the vhost-net case the vm_state_change handler for this device
>> ultimately reaches vhost_net_stop() and do_vhost_dev_stop() and these
>> seem to stop this network device rings at this point.
>>
>> Now, having vhost-net packet buffers in a VFIO device MMIO BAR would
>> be rather atypical configuration, but nevertheless letting VFIO
>> device possibly reach "quiecent" state before this handler finishes
>> would theoretically be a regression.
> 
> If this is an issue, it was an issue for master branch too, right?  Because
> the master branch (with prepare_cb()s) already switch to P2P_DMA state for
> all VFIO devices before invoking any cb().

The VFIO devices in {PRE_COPY,RUNNING}_P2P states can {still,already} accept
DMA so in the master branch by the time prepare_cb() handlers exit and
cb() ones start running the VFIO devices are {still,already} capable of
accepting incoming DMA.

On the other hand, if you meant the state transition into STOP_COPY on the
migration source currently initiated from cb() handler then there can be an
ordering issue in principle.

It looks like the current master branch mostly avoids this by registering
vhost-net vm_state_change handler from VirtIODevice and not from its parent
PCI device, so with the usual PCI topology vhost-net cb() handler will run
before the VFIO PCI device one (and so still hit DMA-capable VFIO device)
due to the vhost-net VirtIODevice being one level deeper in the qdev tree.
It could theoretically be problem with more complex PCI topology though.

> It is also an issue then even before P2P_DMA state introduced, because
> before that all cb()s can be run at random orders.

Technically, the order is based on the device qdev tree depth and within
the same qdev tree depth level it looks like the handler calling order will
be dependent on the order the registering devices were instantiated.

This means that even before these P2P states were introduced
VM configurations that instantiated VFIO/virtio devices in the certain
"right" order or topology weren't affected.

So maybe adding these P2P states mostly fixed this issue too?

> What you said might be an issue indeed in that extremely corner case, but I
> think it still needs some justification, and it's IMHO orthogonal to what
> we're talking about.  If it's an issue, maybe we want to fix it first
> before attempting to move further to any concurrent approaches, but is it
> really an issue is to be better justified.

As I wrote above, I don't think there's a major ordering issue in the
current master branch, at least not for P2P-state-capable VFIO devices.

> Applies to all below.
The above example is about what happens on the migration source, however
the examples below are about the migration target.

Here, in the current master branch these non-VFIO cb() handlers are
guaranteed to hit DMA-capable VFIO device since this switch happened
in its prepare_cb() handler and VFIO device cb() handler does not
change its DMA acceptance qualities so its ordering with respect to
other cb() handlers shouldn't matter.

>>
>> * virtio-blk virtio_blk_dma_restart_cb() vm_state_change handler
>> submits some block requests to its (possibly dedicated) IOThread.
>>
>> Again, having block device request buffers in a VFIO device MMIO BAR
>> would not be a typical configuration, however letting this handler
>> run before making sure that VFIO devices can accept DMA would
>> theoretically make a regression too.
>>
>> * same for scsi-bus scsi_dma_restart_cb() that does
>> scsi_dma_restart_req() for each pending request.
>>
>>
>> The above is not an exhaustive list, these are only the cases
>> I was able to identify relatively easily by going through the
>> code.
>> And I think that with possible memory corruption issues its better
>> to be safe than sorry, as they are pain to debug.
>>
>> The only real non-VFIO cost of being safe about all of this is
>> ~15 code lines addition to vm-change-state-handler.c, all the
>> remaining complexity is already on the VFIO side of things.
>>> That happens earlier
>>> than reaching any of the cb()s here, because VFIO's prepare_cb() does that
>>> P2P state switch.
>>
>> I thought your proposed design was to launch (per-VFIO device)
>> device state changing thread in prepare_cb(), make it do *both*
>> VFIO device state transitions and collect it in cb()?
> 
> The idea was only about trying to leverage prepare_cb() to achieve real
> "prioritized callbacks" for a device, based on patch 1 being problematic.
> 
> What prepare_cb() does can be anything.  Certainly, we can make it only do
> step 1 then in cb() kick all threads to move on to step 2, and sync again.
> 
>>
>> In this case the transitions are *NOT* ordered with respect to
>> other cb() handlers (besides them being finished by the time
>> the VFIO device qdev tree depth cb() handlers are reached by
>> virtue of the threads being collected at this point - this is
>> actually necessary in order to preserve proper ordering with
>> respect to some types of interrupt controllers on the source).
>>
>> So on the migration source the thread could already put device
>> into "quiecent" state by the time other cb() handlers could run
>> and on the migration target the device could still be
>> in an "unprepared" state by the time other cb() handlers are called.
> 
> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Tue, Jul 14, 2026 at 10:23:05PM +0200, Maciej S. Szmigiero wrote:
> The VFIO devices in {PRE_COPY,RUNNING}_P2P states can {still,already} accept
> DMA so in the master branch by the time prepare_cb() handlers exit and
> cb() ones start running the VFIO devices are {still,already} capable of
> accepting incoming DMA.
> 
> On the other hand, if you meant the state transition into STOP_COPY on the
> migration source currently initiated from cb() handler then there can be an
> ordering issue in principle.
> 
> It looks like the current master branch mostly avoids this by registering
> vhost-net vm_state_change handler from VirtIODevice and not from its parent
> PCI device, so with the usual PCI topology vhost-net cb() handler will run
> before the VFIO PCI device one (and so still hit DMA-capable VFIO device)
> due to the vhost-net VirtIODevice being one level deeper in the qdev tree.
> It could theoretically be problem with more complex PCI topology though.

That's an interesting observation, but neither do I think it's intentional,
nor do I think it guarantees the depth ordering. Consider when a VFIO
device is put under a few layers of PCIe switches, or PCI bridges.  In that
case the VFIO device notifier can have larger depth value v.s. the virtio
device when the virtio device is attached to the root complex.

> 
> > It is also an issue then even before P2P_DMA state introduced, because
> > before that all cb()s can be run at random orders.
> 
> Technically, the order is based on the device qdev tree depth and within
> the same qdev tree depth level it looks like the handler calling order will
> be dependent on the order the registering devices were instantiated.
> 
> This means that even before these P2P states were introduced
> VM configurations that instantiated VFIO/virtio devices in the certain
> "right" order or topology weren't affected.
> 
> So maybe adding these P2P states mostly fixed this issue too?

I still think they're two different issues, and I still think with/without
P2P the issue is the same, now I tend to agree this issue exists,
especially if virtio-blk have similar behavior like virtio-net.

For one example of virtio-blk DMA to VFIO MMIO regions:

https://lore.kernel.org/r/20260616052552.389021-1-gshan@redhat.com

Said that, I don't know it's 100% triggerable in this case with notifiers,
but sounds relevant.  For virtio-net and others, I have less idea.

In all cases, I still think there are two issues to fix, and I think we
don't need to fix this problem in one shot. I still don't think they're
directly relevant, at least on the goals.

So with what I suggested previously relying on prepare_cb() I believe we
can still do the concurrency issue you're looking for, making sure when
reaching VFIO's cb() it's still at least accepting DMAs (P2P is fine).

For the other issue you reported, even if existed, I am not yet sure VFIO
is the only special case that got affected.  I think it's possible some
other emulated devices suffer the same even if it has no direct hardware
attached.  Say, there can be device backends got stopped in cb() when VM
stopped, further DMA to it may cause assertions, then it's the same issue
that needs solving, where essentially we may require all such devices to
provide similar P2P states like VFIO to make sure they initiate DMAs and
flush them in a prepare_cb(), then if all devices' prepare_cb() will make
sure no DMA to be initiated anymore, cb()s will have no ordering constraint
on DMAs.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Maciej S. Szmigiero]

On 15.07.2026 17:30, Peter Xu wrote:
> On Tue, Jul 14, 2026 at 10:23:05PM +0200, Maciej S. Szmigiero wrote:
>> The VFIO devices in {PRE_COPY,RUNNING}_P2P states can {still,already} accept
>> DMA so in the master branch by the time prepare_cb() handlers exit and
>> cb() ones start running the VFIO devices are {still,already} capable of
>> accepting incoming DMA.
>>
>> On the other hand, if you meant the state transition into STOP_COPY on the
>> migration source currently initiated from cb() handler then there can be an
>> ordering issue in principle.
>>
>> It looks like the current master branch mostly avoids this by registering
>> vhost-net vm_state_change handler from VirtIODevice and not from its parent
>> PCI device, so with the usual PCI topology vhost-net cb() handler will run
>> before the VFIO PCI device one (and so still hit DMA-capable VFIO device)
>> due to the vhost-net VirtIODevice being one level deeper in the qdev tree.
>> It could theoretically be problem with more complex PCI topology though.
> 
> That's an interesting observation, but neither do I think it's intentional,
> nor do I think it guarantees the depth ordering. Consider when a VFIO
> device is put under a few layers of PCIe switches, or PCI bridges.  In that
> case the VFIO device notifier can have larger depth value v.s. the virtio
> device when the virtio device is attached to the root complex.

Yeah, that's why I wrote that it could be a problem with "more complex PCI topology".

Nevertheless, that's probably uncommon enough setup to not generate visible
complaints about things being broken.

> 
>>
>>> It is also an issue then even before P2P_DMA state introduced, because
>>> before that all cb()s can be run at random orders.
>>
>> Technically, the order is based on the device qdev tree depth and within
>> the same qdev tree depth level it looks like the handler calling order will
>> be dependent on the order the registering devices were instantiated.
>>
>> This means that even before these P2P states were introduced
>> VM configurations that instantiated VFIO/virtio devices in the certain
>> "right" order or topology weren't affected.
>>
>> So maybe adding these P2P states mostly fixed this issue too?
> 
> I still think they're two different issues, and I still think with/without
> P2P the issue is the same, now I tend to agree this issue exists,
> especially if virtio-blk have similar behavior like virtio-net.
> 
> For one example of virtio-blk DMA to VFIO MMIO regions:
> 
> https://lore.kernel.org/r/20260616052552.389021-1-gshan@redhat.com
> 
> Said that, I don't know it's 100% triggerable in this case with notifiers,
> but sounds relevant. 

It does look like it could trigger this issue indeed since it's DMA done
to VFIO device MMIO BAR.
However, how easy would be to reproduce this is another question.

> For virtio-net and others, I have less idea.
> 
> In all cases, I still think there are two issues to fix, and I think we
> don't need to fix this problem in one shot. I still don't think they're
> directly relevant, at least on the goals.

Sure, let's just not make the possible race situation worse.

> So with what I suggested previously relying on prepare_cb() I believe we
> can still do the concurrency issue you're looking for, making sure when
> reaching VFIO's cb() it's still at least accepting DMAs (P2P is fine).

But that's more or less the design of this (original) patch set -
make the state change to RUNNING_P2P on the target finish already in the
prepare_cb() time, make the state change to STOP_COPY on the source begin
only in the cb() time?

> For the other issue you reported, even if existed, I am not yet sure VFIO
> is the only special case that got affected.  I think it's possible some
> other emulated devices suffer the same even if it has no direct hardware
> attached.  Say, there can be device backends got stopped in cb() when VM
> stopped, further DMA to it may cause assertions, then it's the same issue
> that needs solving, where essentially we may require all such devices to
> provide similar P2P states like VFIO to make sure they initiate DMAs and
> flush them in a prepare_cb(), then if all devices' prepare_cb() will make
> sure no DMA to be initiated anymore, cb()s will have no ordering constraint
> on DMAs.

Although the description above is about the migration source the situation
on the migration target is essentially a mirror image of it so I will refer
to both cases below.

Specifically:
* on the source no device should reach non DMA accepting state (like STOP_COPY
for VFIO) before all devices reach non DMA generating state (PRE_COPY_P2P-equivalent).

* on the target no device should reach DMA generating state (RUNNING-equivalent)
before all devices reach DMA accepting state (RUNNING_P2P-like).

That's why even in the case where non-VFIO devices introduce the equivalent of
VFIO P2P states and implement a prepare_cb() callback we still need a (VM-wide)
sync point between these callbacks for the benefit of async state changes/threads
since no cb()-like async state change should start until all prepare_cb()-like
changes finish (including these for other devices).
> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 2/2] vfio/migration: Parallelize device state transitions

[Peter Xu]

On Thu, Jul 16, 2026 at 02:18:34PM +0200, Maciej S. Szmigiero wrote:
> On 15.07.2026 17:30, Peter Xu wrote:
> > On Tue, Jul 14, 2026 at 10:23:05PM +0200, Maciej S. Szmigiero wrote:
> > > The VFIO devices in {PRE_COPY,RUNNING}_P2P states can {still,already} accept
> > > DMA so in the master branch by the time prepare_cb() handlers exit and
> > > cb() ones start running the VFIO devices are {still,already} capable of
> > > accepting incoming DMA.
> > > 
> > > On the other hand, if you meant the state transition into STOP_COPY on the
> > > migration source currently initiated from cb() handler then there can be an
> > > ordering issue in principle.
> > > 
> > > It looks like the current master branch mostly avoids this by registering
> > > vhost-net vm_state_change handler from VirtIODevice and not from its parent
> > > PCI device, so with the usual PCI topology vhost-net cb() handler will run
> > > before the VFIO PCI device one (and so still hit DMA-capable VFIO device)
> > > due to the vhost-net VirtIODevice being one level deeper in the qdev tree.
> > > It could theoretically be problem with more complex PCI topology though.
> > 
> > That's an interesting observation, but neither do I think it's intentional,
> > nor do I think it guarantees the depth ordering. Consider when a VFIO
> > device is put under a few layers of PCIe switches, or PCI bridges.  In that
> > case the VFIO device notifier can have larger depth value v.s. the virtio
> > device when the virtio device is attached to the root complex.
> 
> Yeah, that's why I wrote that it could be a problem with "more complex PCI topology".

Ah yes.

> 
> Nevertheless, that's probably uncommon enough setup to not generate visible
> complaints about things being broken.
> 
> > 
> > > 
> > > > It is also an issue then even before P2P_DMA state introduced, because
> > > > before that all cb()s can be run at random orders.
> > > 
> > > Technically, the order is based on the device qdev tree depth and within
> > > the same qdev tree depth level it looks like the handler calling order will
> > > be dependent on the order the registering devices were instantiated.
> > > 
> > > This means that even before these P2P states were introduced
> > > VM configurations that instantiated VFIO/virtio devices in the certain
> > > "right" order or topology weren't affected.
> > > 
> > > So maybe adding these P2P states mostly fixed this issue too?
> > 
> > I still think they're two different issues, and I still think with/without
> > P2P the issue is the same, now I tend to agree this issue exists,
> > especially if virtio-blk have similar behavior like virtio-net.
> > 
> > For one example of virtio-blk DMA to VFIO MMIO regions:
> > 
> > https://lore.kernel.org/r/20260616052552.389021-1-gshan@redhat.com
> > 
> > Said that, I don't know it's 100% triggerable in this case with notifiers,
> > but sounds relevant.
> 
> It does look like it could trigger this issue indeed since it's DMA done
> to VFIO device MMIO BAR.
> However, how easy would be to reproduce this is another question.
> 
> > For virtio-net and others, I have less idea.
> > 
> > In all cases, I still think there are two issues to fix, and I think we
> > don't need to fix this problem in one shot. I still don't think they're
> > directly relevant, at least on the goals.
> 
> Sure, let's just not make the possible race situation worse.
> 
> > So with what I suggested previously relying on prepare_cb() I believe we
> > can still do the concurrency issue you're looking for, making sure when
> > reaching VFIO's cb() it's still at least accepting DMAs (P2P is fine).
> 
> But that's more or less the design of this (original) patch set -
> make the state change to RUNNING_P2P on the target finish already in the
> prepare_cb() time, make the state change to STOP_COPY on the source begin
> only in the cb() time?

I am not against the idea in general. I'm against patch 1 where it hijacks
"depth" into something even more complicated.

The "depth" itself makes sense to be recorded as a value, but I think it's
already a bit confusing to call it "priority" in VMChangeStateEntry,
because it isn't: when something has higher "priority", it should always be
executed before lower priority items.  Here, the current code invert the
order for !RUN case, so it's in reality "depth", not "priority".

Now patch 1 added yet another extra "adj" concept on top of qdev hierachy's
"depth", which in fact the caller needs "priority" and callers need to
explicitly handle that by checking RUNNING and invert things.

That's the part I'm definitely against.  Such code is IMHO not maintainable.

So my point is if we can implement your idea with existing prepare_cb() we
should just use it.  Even if we want to enhance the priority support for
cb()s, we shouldn't use patch 1.  I never thought further on that because I
still assumed relying on prepare_cb() works.

> 
> > For the other issue you reported, even if existed, I am not yet sure VFIO
> > is the only special case that got affected.  I think it's possible some
> > other emulated devices suffer the same even if it has no direct hardware
> > attached.  Say, there can be device backends got stopped in cb() when VM
> > stopped, further DMA to it may cause assertions, then it's the same issue
> > that needs solving, where essentially we may require all such devices to
> > provide similar P2P states like VFIO to make sure they initiate DMAs and
> > flush them in a prepare_cb(), then if all devices' prepare_cb() will make
> > sure no DMA to be initiated anymore, cb()s will have no ordering constraint
> > on DMAs.
> 
> Although the description above is about the migration source the situation
> on the migration target is essentially a mirror image of it so I will refer
> to both cases below.
> 
> Specifically:
> * on the source no device should reach non DMA accepting state (like STOP_COPY
> for VFIO) before all devices reach non DMA generating state (PRE_COPY_P2P-equivalent).
> 
> * on the target no device should reach DMA generating state (RUNNING-equivalent)
> before all devices reach DMA accepting state (RUNNING_P2P-like).
> 
> That's why even in the case where non-VFIO devices introduce the equivalent of
> VFIO P2P states and implement a prepare_cb() callback we still need a (VM-wide)
> sync point between these callbacks for the benefit of async state changes/threads
> since no cb()-like async state change should start until all prepare_cb()-like
> changes finish (including these for other devices).

IIUC, prepare_cb() + cb() is already that "sync point".  After all
prepare_cb() done, the VM should achieve "DMA quiesced" across the system?

What's missing is we should move all problematic device cb()s "let's flush
pending DMAs" logic into a prepare_cb(), leaving the real stop logic in
cb()s.  That's the 2nd problem which I think we can skip until a reproducer
pops up at least.

VFIO is only special here when it wants to concurrent process of state
changes.  If you create threads in prepare_cb(), then in cb() you have
chance to synchronize everything correctly satisfying the "sync point".
That's not a requirement for most of the rest devices.

Thanks,

-- 
Peter Xu