[PATCH 0/2] migration: Extend blockers to CPR and Xen device-state saves

[PATCH 0/2] migration: Extend blockers to CPR and Xen device-state saves

[Akihiko Odaki]

Migration blockers do not currently cover every device-state save
path. The TPM CRB blocker only covers normal migration even though CPR
cannot preserve unsupported chunk state. xen-save-devices-state also
operates outside the MigMode machinery and ignores blockers entirely.

Make the TPM CRB blocker apply to every migration class and add a
Xen-specific class that xen-save-devices-state checks. Keep
vhost-scsi's same-host CPR exemption while extending its default blocker
to Xen device-state saves.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Akihiko Odaki (2):
      hw/tpm: Block CPR with unsupported CRB chunk migration
      migration: Implement Xen migration blocker

 include/migration/blocker.h | 10 ++++++-
 include/qemu/accel.h        |  2 +-
 migration/migration.h       |  2 ++
 accel/accel-system.c        | 25 +++++++++++++-----
 hw/scsi/vhost-scsi.c        |  5 +++-
 hw/tpm/tpm_crb.c            |  2 +-
 migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c          |  4 +++
 system/vl.c                 |  9 ++++---
 9 files changed, 100 insertions(+), 22 deletions(-)
---
base-commit: 209b2afaface001c7d4d981e38f186afe7b24a50
change-id: 20260731-xen-bf7d034abc99

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH 1/2] hw/tpm: Block CPR with unsupported CRB chunk migration

[Akihiko Odaki]

Currently only the normal migration mode is blocked with unsupported CRB
chunk migration. However, all CPR modes stop the VM before saving.
Stopping only freezes an outstanding chunk transaction; it does not
clear it.

tpm_crb_pre_save() waits for the backend request to finish but may leave
command_buffer and response_buffer with some state. Those fields are
then omitted when allow_chunk_migration is false. CPR preserves guest
RAM or selected kernel resources, not these GByteArray heap objects.
Block the CPR modes too.

Fixes: a8aa1b220c71 ("hw/tpm: Add support for VM migration with TPM CRB chunking")
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/tpm/tpm_crb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 54fa2042b5a8..943809103b63 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -482,7 +482,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
         error_setg(&s->migration_blocker,
                    "The tpm-crb device does not support chunk migration with "
                    "machine version less than 11.1");
-        ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
+        ret = migrate_add_blocker(&s->migration_blocker, errp);
         if (ret < 0) {
             return;
         }

-- 
2.55.0

Re: [PATCH 1/2] hw/tpm: Block CPR with unsupported CRB chunk migration

[Stefan Berger]

On 9/12/26 1:31 AM, Akihiko Odaki wrote:
> Currently only the normal migration mode is blocked with unsupported CRB
> chunk migration. However, all CPR modes stop the VM before saving.
> Stopping only freezes an outstanding chunk transaction; it does not
> clear it.
> 
> tpm_crb_pre_save() waits for the backend request to finish but may leave
> command_buffer and response_buffer with some state. Those fields are
> then omitted when allow_chunk_migration is false. CPR preserves guest
> RAM or selected kernel resources, not these GByteArray heap objects.
> Block the CPR modes too.
> 
> Fixes: a8aa1b220c71 ("hw/tpm: Add support for VM migration with TPM CRB chunking")
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   hw/tpm/tpm_crb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 54fa2042b5a8..943809103b63 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -482,7 +482,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>           error_setg(&s->migration_blocker,
>                      "The tpm-crb device does not support chunk migration with "
>                      "machine version less than 11.1");
> -        ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> +        ret = migrate_add_blocker(&s->migration_blocker, errp);
>           if (ret < 0) {
>               return;
>           }
> 

[PATCH 2/2] migration: Implement Xen migration blocker

[Akihiko Odaki]

xen-save-devices-state saves device VMState outside QEMU's normal
migration-mode machinery, so it currently ignores migration blockers.
Add a separate blocker class for this command.

Blockers registered with migrate_add_blocker() or
migrate_add_blocker_internal() now include this class. Existing
mode-specific blocker masks remain unchanged, except that vhost-scsi's
default blocker now covers normal migration and Xen device-state
saving. Its CPR exemption assumes that the replacement QEMU runs on
the same host and not concurrently with the old QEMU; Xen save and
restore do not guarantee those conditions. Users can set
migratable=true when shared storage or orchestrator-migrated target
state makes migration safe.

Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/migration/blocker.h | 10 ++++++-
 include/qemu/accel.h        |  2 +-
 migration/migration.h       |  2 ++
 accel/accel-system.c        | 25 +++++++++++++-----
 hw/scsi/vhost-scsi.c        |  5 +++-
 migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c          |  4 +++
 system/vl.c                 |  9 ++++---
 8 files changed, 99 insertions(+), 21 deletions(-)

diff --git a/include/migration/blocker.h b/include/migration/blocker.h
index 80b75ad5cbdb..0fbaaac83ce8 100644
--- a/include/migration/blocker.h
+++ b/include/migration/blocker.h
@@ -16,6 +16,14 @@
 
 #include "qapi/qapi-types-migration.h"
 
+/**
+ * define MIG_XEN - identifier for Xen migration
+ *
+ * Xen migration is distinct from those expressed with &typedef MigMode.
+ * Use this for migration blockers that affect Xen.
+ */
+#define MIG_XEN MIG_MODE__MAX
+
 /**
  * @migrate_add_blocker - prevent all modes of migration from proceeding
  *
@@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
  *
  * @reasonp - address of an error to be returned whenever migration is attempted
  *
- * @modes - the migration modes to be blocked, a bit set of MigMode
+ * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
  *
  * @errp - [out] The reason (if any) we cannot block migration right now.
  *
diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 6f65f1cb89a0..b6b456a82fc3 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -47,7 +47,7 @@ const char *current_accel_name(void);
 
 void accel_init_interfaces(AccelClass *ac);
 
-int accel_init_machine(AccelState *accel, MachineState *ms);
+int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
 
 /* Called just before os_setup_post (ie just before drop OS privs) */
 void accel_setup_post(MachineState *ms);
diff --git a/migration/migration.h b/migration/migration.h
index 683bc7bdd521..f53fdb2df107 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -568,7 +568,9 @@ void migration_start_incoming(void);
 int migration_call_notifiers(MigrationEventType type, Error **errp);
 
 int migrate_init(MigrationState *s, Error **errp);
+bool enforce_only_migratable(Error **errp);
 bool migration_is_blocked(Error **errp);
+bool xen_migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
 bool migration_postcopy_is_alive(MigrationStatus state);
diff --git a/accel/accel-system.c b/accel/accel-system.c
index 1325b23864c8..75a691cb3cfe 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -26,7 +26,9 @@
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
 #include "qom/compat-properties.h"
+#include "qapi/error.h"
 #include "qapi/qapi-commands-accelerator.h"
+#include "migration/migration.h"
 #include "monitor/monitor.h"
 #include "monitor/hmp.h"
 #include "hw/core/boards.h"
@@ -37,20 +39,31 @@
 #include "qemu/error-report.h"
 #include "accel-internal.h"
 
-int accel_init_machine(AccelState *accel, MachineState *ms)
+int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
 {
     AccelClass *acc = ACCEL_GET_CLASS(accel);
     int ret;
     ms->accelerator = accel;
     *(acc->allowed) = true;
+
+    if (!enforce_only_migratable(errp)) {
+        ret = -EACCES;
+        goto fail;
+    }
+
     ret = acc->init_machine(accel, ms);
     if (ret < 0) {
-        ms->accelerator = NULL;
-        *(acc->allowed) = false;
-        object_unref(OBJECT(accel));
-    } else {
-        object_set_accelerator_compat_props(acc->compat_props);
+        error_setg(errp, "%s", strerror(-ret));
+        goto fail;
     }
+
+    object_set_accelerator_compat_props(acc->compat_props);
+    return ret;
+
+fail:
+    ms->accelerator = NULL;
+    *(acc->allowed) = false;
+    object_unref(OBJECT(accel));
     return ret;
 }
 
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 657403cad011..f9847b5e7096 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     }
 
     if (!vsc->migratable) {
+        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
+
         error_setg(&vsc->migration_blocker,
                 "vhost-scsi does not support migration in all cases. "
                 "When external environment supports it (Orchestrator migrates "
                 "target SCSI device state or use shared storage over network), "
                 "set 'migratable' property to true to enable migration.");
-        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
+        if (migrate_add_blocker_modes(&vsc->migration_blocker,
+                                      modes, errp) < 0) {
             goto free_virtio;
         }
     }
diff --git a/migration/migration.c b/migration/migration.c
index d0b864a76066..c177dc52a1c1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -23,6 +23,7 @@
 #include "system/runstate.h"
 #include "system/system.h"
 #include "system/cpu-throttle.h"
+#include "system/xen.h"
 #include "ram.h"
 #include "migration/cpr.h"
 #include "migration/global_state.h"
@@ -94,7 +95,8 @@ enum mig_rp_message_type {
 static MigrationState *current_migration;
 static MigrationIncomingState *current_incoming;
 
-static GSList *migration_blockers[MIG_MODE__MAX];
+static GSList *migration_blockers[MIG_MODE__MAX + 1];
+static bool enforcing_only_migratable;
 
 static bool migration_object_check(MigrationState *ms, Error **errp);
 static bool migration_switchover_start(MigrationState *s, Error **errp);
@@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
     return false;
 }
 
+static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
+{
+    error_propagate_prepend(errp, *reasonp,
+                            "disallowing migration blocker "
+                            "(--only-migratable) for: ");
+    *reasonp = NULL;
+}
+
 static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
 {
+    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
+
     ERRP_GUARD();
 
-    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
-        error_propagate_prepend(errp, *reasonp,
-                                "disallowing migration blocker "
-                                "(--only-migratable) for: ");
-        *reasonp = NULL;
+    if (enforcing_only_migratable && (modes & BIT(mode))) {
+        propagate_disallowed_blocker(errp, reasonp);
         return true;
     }
     return false;
@@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
 
 static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
 {
-    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
+    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
         if (modes & BIT(mode)) {
             assert(g_slist_index(migration_blockers[mode],
                                  *reasonp) == -1);
@@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
 
 int migrate_add_blocker_internal(Error **reasonp, Error **errp)
 {
-    unsigned modes = BIT(MIG_MODE__MAX) - 1;
+    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
 
     if (is_busy(reasonp, errp)) {
         return -EBUSY;
@@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
 void migrate_del_blocker(Error **reasonp)
 {
     if (*reasonp) {
-        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
+        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
             migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
                                                       *reasonp);
         }
@@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
                "during postcopy-active or postcopy-recover state");
 }
 
+bool enforce_only_migratable(Error **errp)
+{
+    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
+    GSList *blockers = migration_blockers[mode];
+    Error *reason;
+
+    if (!only_migratable) {
+        return true;
+    }
+
+    if (blockers) {
+        reason = error_copy(blockers->data);
+        propagate_disallowed_blocker(errp, &reason);
+        return false;
+    }
+
+    enforcing_only_migratable = true;
+    return true;
+}
+
 bool migration_is_blocked(Error **errp)
 {
     GSList *blockers = migration_blockers[migrate_mode()];
@@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+bool xen_migration_is_blocked(Error **errp)
+{
+    GSList *blockers = migration_blockers[MIG_XEN];
+
+    if (qemu_savevm_state_blocked(errp)) {
+        return true;
+    }
+
+    if (blockers) {
+        error_propagate(errp, error_copy(blockers->data));
+        return true;
+    }
+
+    return false;
+}
+
 /* Returns true if continue to migrate, or false if error detected */
 static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
 {
diff --git a/migration/savevm.c b/migration/savevm.c
index 4b590ea672d2..4e5e1af1b202 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
     int saved_vm_running;
     int ret;
 
+    if (xen_migration_is_blocked(errp)) {
+        return;
+    }
+
     if (!has_live) {
         /* live default to true so old version of Xen tool stack can have a
          * successful live migration */
diff --git a/system/vl.c b/system/vl.c
index 9bd7664b85c7..21fa810637c3 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
 
 static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
 {
+    Error *local_err = NULL;
     bool *p_init_failed = opaque;
     const char *acc = qemu_opt_get(opts, "accel");
     AccelClass *ac = accel_find(acc);
@@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
                      accel,
                      &error_fatal);
 
-    ret = accel_init_machine(accel, current_machine);
+    ret = accel_init_machine(accel, current_machine, &local_err);
     if (ret < 0) {
-        if (!qtest_with_kvm || ret != -ENOENT) {
-            error_report("failed to initialize %s: %s", acc, strerror(-ret));
+        if (qtest_with_kvm && ret == -ENOENT) {
+            error_free(local_err);
+        } else {
+            error_reportf_err(local_err, "failed to initialize %s: ", acc);
         }
         goto bad;
     }

-- 
2.55.0

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Fabiano Rosas]

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

> xen-save-devices-state saves device VMState outside QEMU's normal
> migration-mode machinery, so it currently ignores migration blockers.
> Add a separate blocker class for this command.
>
> Blockers registered with migrate_add_blocker() or
> migrate_add_blocker_internal() now include this class. Existing
> mode-specific blocker masks remain unchanged, except that vhost-scsi's
> default blocker now covers normal migration and Xen device-state
> saving. Its CPR exemption assumes that the replacement QEMU runs on
> the same host and not concurrently with the old QEMU; Xen save and
> restore do not guarantee those conditions. Users can set
> migratable=true when shared storage or orchestrator-migrated target
> state makes migration safe.
>

So one thing is rejecting the xen-save-devices-state command if there
are blockers. This could very well just use the normal mode (default),
no? Or all modes, like qmp_dump_guest_memory() does?

Another thing (complementary, could be in the same patch) is rejecting
the command line if there are blocked devices, including any ones used
along with Xen acceleration. Again, why does this need a new blocker
category? I'm not seeing it.

> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  include/migration/blocker.h | 10 ++++++-
>  include/qemu/accel.h        |  2 +-
>  migration/migration.h       |  2 ++
>  accel/accel-system.c        | 25 +++++++++++++-----
>  hw/scsi/vhost-scsi.c        |  5 +++-
>  migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>  migration/savevm.c          |  4 +++
>  system/vl.c                 |  9 ++++---
>  8 files changed, 99 insertions(+), 21 deletions(-)
>
> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
> index 80b75ad5cbdb..0fbaaac83ce8 100644
> --- a/include/migration/blocker.h
> +++ b/include/migration/blocker.h
> @@ -16,6 +16,14 @@
>  
>  #include "qapi/qapi-types-migration.h"
>  
> +/**
> + * define MIG_XEN - identifier for Xen migration

Hm.

> + *
> + * Xen migration is distinct from those expressed with &typedef MigMode.
> + * Use this for migration blockers that affect Xen.
> + */
> +#define MIG_XEN MIG_MODE__MAX

This could be fine, Xen migration is pre-existing and 'mode' is part of
QAPI, we don't want to create a new mode for Xen.

> +
>  /**
>   * @migrate_add_blocker - prevent all modes of migration from proceeding
>   *
> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>   *
>   * @reasonp - address of an error to be returned whenever migration is attempted
>   *
> - * @modes - the migration modes to be blocked, a bit set of MigMode
> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN

But it's not a mode, so we shouldn't re-use @modes. We just want a place
to store the Xen blockers, let's not introduce the confusion that this
could be a mode somehow.

We could just add migrate_add_blocker_xen and call add_blockers directly
since you're changing that function to use the array size instead of
MIG_MODE__MAX.

>   *
>   * @errp - [out] The reason (if any) we cannot block migration right now.
>   *
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 6f65f1cb89a0..b6b456a82fc3 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>  
>  void accel_init_interfaces(AccelClass *ac);
>  
> -int accel_init_machine(AccelState *accel, MachineState *ms);
> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>  
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
> diff --git a/migration/migration.h b/migration/migration.h
> index 683bc7bdd521..f53fdb2df107 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>  int migration_call_notifiers(MigrationEventType type, Error **errp);
>  
>  int migrate_init(MigrationState *s, Error **errp);
> +bool enforce_only_migratable(Error **errp);
>  bool migration_is_blocked(Error **errp);
> +bool xen_migration_is_blocked(Error **errp);
>  /* True if outgoing migration has entered postcopy phase */
>  bool migration_in_postcopy(void);
>  bool migration_postcopy_is_alive(MigrationStatus state);
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index 1325b23864c8..75a691cb3cfe 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -26,7 +26,9 @@
>  #include "qemu/osdep.h"
>  #include "qemu/accel.h"
>  #include "qom/compat-properties.h"
> +#include "qapi/error.h"
>  #include "qapi/qapi-commands-accelerator.h"
> +#include "migration/migration.h"
>  #include "monitor/monitor.h"
>  #include "monitor/hmp.h"
>  #include "hw/core/boards.h"
> @@ -37,20 +39,31 @@
>  #include "qemu/error-report.h"
>  #include "accel-internal.h"
>  
> -int accel_init_machine(AccelState *accel, MachineState *ms)
> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>  {
>      AccelClass *acc = ACCEL_GET_CLASS(accel);
>      int ret;
>      ms->accelerator = accel;
>      *(acc->allowed) = true;
> +
> +    if (!enforce_only_migratable(errp)) {
> +        ret = -EACCES;
> +        goto fail;
> +    }

Here I think you want to fail the initialization if the accelerator is
Xen, -only-migratable is set and there are blockers. At this point is
not clear whether these blockers really need a xen-specific category.

Isn't this too soon anyway? What blockers will be registered at this
point and if there are blockers on the list, why the
is_only_migratable() check at migrate_add_blocker_modes() hasn't already
rejected the cmdline?

> +
>      ret = acc->init_machine(accel, ms);
>      if (ret < 0) {
> -        ms->accelerator = NULL;
> -        *(acc->allowed) = false;
> -        object_unref(OBJECT(accel));
> -    } else {
> -        object_set_accelerator_compat_props(acc->compat_props);
> +        error_setg(errp, "%s", strerror(-ret));
> +        goto fail;
>      }
> +
> +    object_set_accelerator_compat_props(acc->compat_props);
> +    return ret;
> +
> +fail:
> +    ms->accelerator = NULL;
> +    *(acc->allowed) = false;
> +    object_unref(OBJECT(accel));
>      return ret;
>  }
>  
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 657403cad011..f9847b5e7096 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (!vsc->migratable) {
> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
> +
>          error_setg(&vsc->migration_blocker,
>                  "vhost-scsi does not support migration in all cases. "
>                  "When external environment supports it (Orchestrator migrates "
>                  "target SCSI device state or use shared storage over network), "
>                  "set 'migratable' property to true to enable migration.");
> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
> +                                      modes, errp) < 0) {
>              goto free_virtio;
>          }
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index d0b864a76066..c177dc52a1c1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -23,6 +23,7 @@
>  #include "system/runstate.h"
>  #include "system/system.h"
>  #include "system/cpu-throttle.h"
> +#include "system/xen.h"
>  #include "ram.h"
>  #include "migration/cpr.h"
>  #include "migration/global_state.h"
> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>  static MigrationState *current_migration;
>  static MigrationIncomingState *current_incoming;
>  
> -static GSList *migration_blockers[MIG_MODE__MAX];
> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
> +static bool enforcing_only_migratable;
>  
>  static bool migration_object_check(MigrationState *ms, Error **errp);
>  static bool migration_switchover_start(MigrationState *s, Error **errp);
> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>      return false;
>  }
>  
> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
> +{
> +    error_propagate_prepend(errp, *reasonp,
> +                            "disallowing migration blocker "
> +                            "(--only-migratable) for: ");
> +    *reasonp = NULL;
> +}
> +
>  static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>  {
> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
> +
>      ERRP_GUARD();
>  
> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
> -        error_propagate_prepend(errp, *reasonp,
> -                                "disallowing migration blocker "
> -                                "(--only-migratable) for: ");
> -        *reasonp = NULL;
> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
> +        propagate_disallowed_blocker(errp, reasonp);
>          return true;
>      }
>      return false;
> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>  
>  static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>  {
> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {

Not a MigMode anymore.

>          if (modes & BIT(mode)) {
>              assert(g_slist_index(migration_blockers[mode],
>                                   *reasonp) == -1);
> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>  
>  int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>  {
> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>  
>      if (is_busy(reasonp, errp)) {
>          return -EBUSY;
> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>  void migrate_del_blocker(Error **reasonp)
>  {
>      if (*reasonp) {
> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {

Same here, just use int i.

>              migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>                                                        *reasonp);
>          }
> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>                 "during postcopy-active or postcopy-recover state");
>  }
>  
> +bool enforce_only_migratable(Error **errp)
> +{
> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
> +    GSList *blockers = migration_blockers[mode];

For Xen, what blockers are those? Is it the case that existing blockers
are simply not taking effect for Xen because the QMP command never
checks blockers? Or do you intend to add xen-specific blockers?

> +    Error *reason;
> +
> +    if (!only_migratable) {
> +        return true;
> +    }
> +
> +    if (blockers) {
> +        reason = error_copy(blockers->data);
> +        propagate_disallowed_blocker(errp, &reason);
> +        return false;
> +    }
> +
> +    enforcing_only_migratable = true;
> +    return true;
> +}
> +
>  bool migration_is_blocked(Error **errp)
>  {
>      GSList *blockers = migration_blockers[migrate_mode()];
> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>      return false;
>  }
>  
> +bool xen_migration_is_blocked(Error **errp)
> +{
> +    GSList *blockers = migration_blockers[MIG_XEN];
> +
> +    if (qemu_savevm_state_blocked(errp)) {
> +        return true;
> +    }
> +
> +    if (blockers) {
> +        error_propagate(errp, error_copy(blockers->data));
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  /* Returns true if continue to migrate, or false if error detected */
>  static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>  {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4b590ea672d2..4e5e1af1b202 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>      int saved_vm_running;
>      int ret;
>  
> +    if (xen_migration_is_blocked(errp)) {
> +        return;
> +    }
> +
>      if (!has_live) {
>          /* live default to true so old version of Xen tool stack can have a
>           * successful live migration */
> diff --git a/system/vl.c b/system/vl.c
> index 9bd7664b85c7..21fa810637c3 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>  
>  static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>  {
> +    Error *local_err = NULL;
>      bool *p_init_failed = opaque;
>      const char *acc = qemu_opt_get(opts, "accel");
>      AccelClass *ac = accel_find(acc);
> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>                       accel,
>                       &error_fatal);
>  
> -    ret = accel_init_machine(accel, current_machine);
> +    ret = accel_init_machine(accel, current_machine, &local_err);
>      if (ret < 0) {
> -        if (!qtest_with_kvm || ret != -ENOENT) {
> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
> +        if (qtest_with_kvm && ret == -ENOENT) {
> +            error_free(local_err);
> +        } else {
> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>          }
>          goto bad;
>      }

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Akihiko Odaki]

On 2026/09/22 23:28, Fabiano Rosas wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> xen-save-devices-state saves device VMState outside QEMU's normal
>> migration-mode machinery, so it currently ignores migration blockers.
>> Add a separate blocker class for this command.
>>
>> Blockers registered with migrate_add_blocker() or
>> migrate_add_blocker_internal() now include this class. Existing
>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>> default blocker now covers normal migration and Xen device-state
>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>> the same host and not concurrently with the old QEMU; Xen save and
>> restore do not guarantee those conditions. Users can set
>> migratable=true when shared storage or orchestrator-migrated target
>> state makes migration safe.
>>
> 
> So one thing is rejecting the xen-save-devices-state command if there
> are blockers. This could very well just use the normal mode (default),
> no? Or all modes, like qmp_dump_guest_memory() does?

In general, blockers registered via migrate_add_blocker_normal() are 
specific to standard QEMU live migration and do not apply to Xen. For 
example, block formats like qcow or vhd register normal migration 
blockers, but Xen explicitly supports migration with them [1]. 
Unconditionally applying normal-mode blockers to Xen would cause 
regressions. vhost-scsi is the exception here and is handled specifically.

Checking every mode would lead to the same regression. Note that 
qmp_dump_guest_memory() registers an all-mode blocker during execution; 
it does not check all existing blocker lists when invoked.

[1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback

> 
> Another thing (complementary, could be in the same patch) is rejecting
> the command line if there are blocked devices, including any ones used
> along with Xen acceleration. Again, why does this need a new blocker
> category? I'm not seeing it.
You can use -only-migratable.

> 
>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> ---
>>   include/migration/blocker.h | 10 ++++++-
>>   include/qemu/accel.h        |  2 +-
>>   migration/migration.h       |  2 ++
>>   accel/accel-system.c        | 25 +++++++++++++-----
>>   hw/scsi/vhost-scsi.c        |  5 +++-
>>   migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>>   migration/savevm.c          |  4 +++
>>   system/vl.c                 |  9 ++++---
>>   8 files changed, 99 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>> --- a/include/migration/blocker.h
>> +++ b/include/migration/blocker.h
>> @@ -16,6 +16,14 @@
>>   
>>   #include "qapi/qapi-types-migration.h"
>>   
>> +/**
>> + * define MIG_XEN - identifier for Xen migration
> 
> Hm.
> 
>> + *
>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>> + * Use this for migration blockers that affect Xen.
>> + */
>> +#define MIG_XEN MIG_MODE__MAX
> 
> This could be fine, Xen migration is pre-existing and 'mode' is part of
> QAPI, we don't want to create a new mode for Xen.
> 
>> +
>>   /**
>>    * @migrate_add_blocker - prevent all modes of migration from proceeding
>>    *
>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>>    *
>>    * @reasonp - address of an error to be returned whenever migration is attempted
>>    *
>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
> 
> But it's not a mode, so we shouldn't re-use @modes. We just want a place
> to store the Xen blockers, let's not introduce the confusion that this
> could be a mode somehow.
> 
> We could just add migrate_add_blocker_xen and call add_blockers directly
> since you're changing that function to use the array size instead of
> MIG_MODE__MAX.

I would prefer to keep a single function for adding blockers across all 
migration types so that the full scope of a blocker can be seen at a 
single call site (see the vhost-scsi changes for an example).

Practically speaking, I think the risk of reusing @modes is minimal 
because there is no alternative selective-blocking API to misuse. It 
would indeed be cleaner to have a parameter name that encompasses both 
MigMode and MIG_XEN, but I haven't come up with a better name yet; 
suggestions are welcome.

> 
>>    *
>>    * @errp - [out] The reason (if any) we cannot block migration right now.
>>    *
>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>> index 6f65f1cb89a0..b6b456a82fc3 100644
>> --- a/include/qemu/accel.h
>> +++ b/include/qemu/accel.h
>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>   
>>   void accel_init_interfaces(AccelClass *ac);
>>   
>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>   
>>   /* Called just before os_setup_post (ie just before drop OS privs) */
>>   void accel_setup_post(MachineState *ms);
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 683bc7bdd521..f53fdb2df107 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>   int migration_call_notifiers(MigrationEventType type, Error **errp);
>>   
>>   int migrate_init(MigrationState *s, Error **errp);
>> +bool enforce_only_migratable(Error **errp);
>>   bool migration_is_blocked(Error **errp);
>> +bool xen_migration_is_blocked(Error **errp);
>>   /* True if outgoing migration has entered postcopy phase */
>>   bool migration_in_postcopy(void);
>>   bool migration_postcopy_is_alive(MigrationStatus state);
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index 1325b23864c8..75a691cb3cfe 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -26,7 +26,9 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/accel.h"
>>   #include "qom/compat-properties.h"
>> +#include "qapi/error.h"
>>   #include "qapi/qapi-commands-accelerator.h"
>> +#include "migration/migration.h"
>>   #include "monitor/monitor.h"
>>   #include "monitor/hmp.h"
>>   #include "hw/core/boards.h"
>> @@ -37,20 +39,31 @@
>>   #include "qemu/error-report.h"
>>   #include "accel-internal.h"
>>   
>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>   {
>>       AccelClass *acc = ACCEL_GET_CLASS(accel);
>>       int ret;
>>       ms->accelerator = accel;
>>       *(acc->allowed) = true;
>> +
>> +    if (!enforce_only_migratable(errp)) {
>> +        ret = -EACCES;
>> +        goto fail;
>> +    }
> 
> Here I think you want to fail the initialization if the accelerator is
> Xen, -only-migratable is set and there are blockers. At this point is
> not clear whether these blockers really need a xen-specific category.

The category reflects the specific requirements of the blocker itself. 
Looking at vhost-scsi (which registers a Xen-affecting blocker) 
clarifies why this distinction is necessary; this code merely enforces 
registered blockers.

> 
> Isn't this too soon anyway? What blockers will be registered at this
> point and if there are blockers on the list, why the
> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
> rejected the cmdline?

This check is placed here because qemu_create_early_backends() runs 
before configure_accelerators(). Command-line backends (such as qcow or 
vmdk) can register blockers before xen_enabled() accurately reflects the 
selected accelerator.

> 
>> +
>>       ret = acc->init_machine(accel, ms);
>>       if (ret < 0) {
>> -        ms->accelerator = NULL;
>> -        *(acc->allowed) = false;
>> -        object_unref(OBJECT(accel));
>> -    } else {
>> -        object_set_accelerator_compat_props(acc->compat_props);
>> +        error_setg(errp, "%s", strerror(-ret));
>> +        goto fail;
>>       }
>> +
>> +    object_set_accelerator_compat_props(acc->compat_props);
>> +    return ret;
>> +
>> +fail:
>> +    ms->accelerator = NULL;
>> +    *(acc->allowed) = false;
>> +    object_unref(OBJECT(accel));
>>       return ret;
>>   }
>>   
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 657403cad011..f9847b5e7096 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>>       }
>>   
>>       if (!vsc->migratable) {
>> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>> +
>>           error_setg(&vsc->migration_blocker,
>>                   "vhost-scsi does not support migration in all cases. "
>>                   "When external environment supports it (Orchestrator migrates "
>>                   "target SCSI device state or use shared storage over network), "
>>                   "set 'migratable' property to true to enable migration.");
>> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
>> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
>> +                                      modes, errp) < 0) {
>>               goto free_virtio;
>>           }
>>       }
>> diff --git a/migration/migration.c b/migration/migration.c
>> index d0b864a76066..c177dc52a1c1 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -23,6 +23,7 @@
>>   #include "system/runstate.h"
>>   #include "system/system.h"
>>   #include "system/cpu-throttle.h"
>> +#include "system/xen.h"
>>   #include "ram.h"
>>   #include "migration/cpr.h"
>>   #include "migration/global_state.h"
>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>   static MigrationState *current_migration;
>>   static MigrationIncomingState *current_incoming;
>>   
>> -static GSList *migration_blockers[MIG_MODE__MAX];
>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>> +static bool enforcing_only_migratable;
>>   
>>   static bool migration_object_check(MigrationState *ms, Error **errp);
>>   static bool migration_switchover_start(MigrationState *s, Error **errp);
>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>       return false;
>>   }
>>   
>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>> +{
>> +    error_propagate_prepend(errp, *reasonp,
>> +                            "disallowing migration blocker "
>> +                            "(--only-migratable) for: ");
>> +    *reasonp = NULL;
>> +}
>> +
>>   static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>   {
>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>> +
>>       ERRP_GUARD();
>>   
>> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>> -        error_propagate_prepend(errp, *reasonp,
>> -                                "disallowing migration blocker "
>> -                                "(--only-migratable) for: ");
>> -        *reasonp = NULL;
>> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
>> +        propagate_disallowed_blocker(errp, reasonp);
>>           return true;
>>       }
>>       return false;
>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>   
>>   static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>   {
>> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
> 
> Not a MigMode anymore.

Good catch. I will fix the type in the next version.

> 
>>           if (modes & BIT(mode)) {
>>               assert(g_slist_index(migration_blockers[mode],
>>                                    *reasonp) == -1);
>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>>   
>>   int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>   {
>> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
>> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>   
>>       if (is_busy(reasonp, errp)) {
>>           return -EBUSY;
>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>   void migrate_del_blocker(Error **reasonp)
>>   {
>>       if (*reasonp) {
>> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
> 
> Same here, just use int i.

Will update the variable name and type accordingly.

> 
>>               migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>>                                                         *reasonp);
>>           }
>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>                  "during postcopy-active or postcopy-recover state");
>>   }
>>   
>> +bool enforce_only_migratable(Error **errp)
>> +{
>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>> +    GSList *blockers = migration_blockers[mode];
> 
> For Xen, what blockers are those? Is it the case that existing blockers
> are simply not taking effect for Xen because the QMP command never
> checks blockers? Or do you intend to add xen-specific blockers?

The former.

Existing migration blockers currently do not take effect for Xen, which 
this patch fixes. However, applying all blockers from an existing 
MigMode could cause a regression for the reasons stated above. 
Therefore, this patch ensures that only blockers relevant to Xen are 
applied.

Specifically, it enforces the blockers added via migrate_add_blocker() 
and the vhost-scsi blocker. The remaining blockers have not been proven 
incompatible with Xen migration, so applying them unconditionally could 
cause regressions.

Regards,
Akihiko Odaki

> 
>> +    Error *reason;
>> +
>> +    if (!only_migratable) {
>> +        return true;
>> +    }
>> +
>> +    if (blockers) {
>> +        reason = error_copy(blockers->data);
>> +        propagate_disallowed_blocker(errp, &reason);
>> +        return false;
>> +    }
>> +
>> +    enforcing_only_migratable = true;
>> +    return true;
>> +}
>> +
>>   bool migration_is_blocked(Error **errp)
>>   {
>>       GSList *blockers = migration_blockers[migrate_mode()];
>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>       return false;
>>   }
>>   
>> +bool xen_migration_is_blocked(Error **errp)
>> +{
>> +    GSList *blockers = migration_blockers[MIG_XEN];
>> +
>> +    if (qemu_savevm_state_blocked(errp)) {
>> +        return true;
>> +    }
>> +
>> +    if (blockers) {
>> +        error_propagate(errp, error_copy(blockers->data));
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   /* Returns true if continue to migrate, or false if error detected */
>>   static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>   {
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 4b590ea672d2..4e5e1af1b202 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>>       int saved_vm_running;
>>       int ret;
>>   
>> +    if (xen_migration_is_blocked(errp)) {
>> +        return;
>> +    }
>> +
>>       if (!has_live) {
>>           /* live default to true so old version of Xen tool stack can have a
>>            * successful live migration */
>> diff --git a/system/vl.c b/system/vl.c
>> index 9bd7664b85c7..21fa810637c3 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>   
>>   static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>   {
>> +    Error *local_err = NULL;
>>       bool *p_init_failed = opaque;
>>       const char *acc = qemu_opt_get(opts, "accel");
>>       AccelClass *ac = accel_find(acc);
>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>                        accel,
>>                        &error_fatal);
>>   
>> -    ret = accel_init_machine(accel, current_machine);
>> +    ret = accel_init_machine(accel, current_machine, &local_err);
>>       if (ret < 0) {
>> -        if (!qtest_with_kvm || ret != -ENOENT) {
>> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
>> +        if (qtest_with_kvm && ret == -ENOENT) {
>> +            error_free(local_err);
>> +        } else {
>> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>           }
>>           goto bad;
>>       }

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Fabiano Rosas]

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

> On 2026/09/22 23:28, Fabiano Rosas wrote:
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>> 
>>> xen-save-devices-state saves device VMState outside QEMU's normal
>>> migration-mode machinery, so it currently ignores migration blockers.
>>> Add a separate blocker class for this command.
>>>
>>> Blockers registered with migrate_add_blocker() or
>>> migrate_add_blocker_internal() now include this class. Existing
>>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>>> default blocker now covers normal migration and Xen device-state
>>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>>> the same host and not concurrently with the old QEMU; Xen save and
>>> restore do not guarantee those conditions. Users can set
>>> migratable=true when shared storage or orchestrator-migrated target
>>> state makes migration safe.
>>>
>> 
>> So one thing is rejecting the xen-save-devices-state command if there
>> are blockers. This could very well just use the normal mode (default),
>> no? Or all modes, like qmp_dump_guest_memory() does?
>
> In general, blockers registered via migrate_add_blocker_normal() are 
> specific to standard QEMU live migration and do not apply to Xen. For 
> example, block formats like qcow or vhd register normal migration 
> blockers, but Xen explicitly supports migration with them [1]. 
> Unconditionally applying normal-mode blockers to Xen would cause 
> regressions. vhost-scsi is the exception here and is handled specifically.
>
> Checking every mode would lead to the same regression. Note that 
> qmp_dump_guest_memory() registers an all-mode blocker during execution; 
> it does not check all existing blocker lists when invoked.
>
> [1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback
>
>> 
>> Another thing (complementary, could be in the same patch) is rejecting
>> the command line if there are blocked devices, including any ones used
>> along with Xen acceleration. Again, why does this need a new blocker
>> category? I'm not seeing it.
> You can use -only-migratable.
>
>> 
>>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> ---
>>>   include/migration/blocker.h | 10 ++++++-
>>>   include/qemu/accel.h        |  2 +-
>>>   migration/migration.h       |  2 ++
>>>   accel/accel-system.c        | 25 +++++++++++++-----
>>>   hw/scsi/vhost-scsi.c        |  5 +++-
>>>   migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>>>   migration/savevm.c          |  4 +++
>>>   system/vl.c                 |  9 ++++---
>>>   8 files changed, 99 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>>> --- a/include/migration/blocker.h
>>> +++ b/include/migration/blocker.h
>>> @@ -16,6 +16,14 @@
>>>   
>>>   #include "qapi/qapi-types-migration.h"
>>>   
>>> +/**
>>> + * define MIG_XEN - identifier for Xen migration
>> 
>> Hm.
>> 
>>> + *
>>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>>> + * Use this for migration blockers that affect Xen.
>>> + */
>>> +#define MIG_XEN MIG_MODE__MAX
>> 
>> This could be fine, Xen migration is pre-existing and 'mode' is part of
>> QAPI, we don't want to create a new mode for Xen.
>> 
>>> +
>>>   /**
>>>    * @migrate_add_blocker - prevent all modes of migration from proceeding
>>>    *
>>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>>>    *
>>>    * @reasonp - address of an error to be returned whenever migration is attempted
>>>    *
>>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>>> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
>> 
>> But it's not a mode, so we shouldn't re-use @modes. We just want a place
>> to store the Xen blockers, let's not introduce the confusion that this
>> could be a mode somehow.
>> 
>> We could just add migrate_add_blocker_xen and call add_blockers directly
>> since you're changing that function to use the array size instead of
>> MIG_MODE__MAX.
>
> I would prefer to keep a single function for adding blockers across all 
> migration types so that the full scope of a blocker can be seen at a 
> single call site (see the vhost-scsi changes for an example).
>

Fair.

> Practically speaking, I think the risk of reusing @modes is minimal 
> because there is no alternative selective-blocking API to misuse. It 
> would indeed be cleaner to have a parameter name that encompasses both 
> MigMode and MIG_XEN, but I haven't come up with a better name yet; 
> suggestions are welcome.
>

Keep the @mode, but let's do away with any mentions of MigMode. This is
a "blocker mode", not "migration mode". wink wink

>> 
>>>    *
>>>    * @errp - [out] The reason (if any) we cannot block migration right now.
>>>    *
>>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>>> index 6f65f1cb89a0..b6b456a82fc3 100644
>>> --- a/include/qemu/accel.h
>>> +++ b/include/qemu/accel.h
>>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>>   
>>>   void accel_init_interfaces(AccelClass *ac);
>>>   
>>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>>   
>>>   /* Called just before os_setup_post (ie just before drop OS privs) */
>>>   void accel_setup_post(MachineState *ms);
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 683bc7bdd521..f53fdb2df107 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>>   int migration_call_notifiers(MigrationEventType type, Error **errp);
>>>   
>>>   int migrate_init(MigrationState *s, Error **errp);
>>> +bool enforce_only_migratable(Error **errp);
>>>   bool migration_is_blocked(Error **errp);
>>> +bool xen_migration_is_blocked(Error **errp);
>>>   /* True if outgoing migration has entered postcopy phase */
>>>   bool migration_in_postcopy(void);
>>>   bool migration_postcopy_is_alive(MigrationStatus state);
>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>> index 1325b23864c8..75a691cb3cfe 100644
>>> --- a/accel/accel-system.c
>>> +++ b/accel/accel-system.c
>>> @@ -26,7 +26,9 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qemu/accel.h"
>>>   #include "qom/compat-properties.h"
>>> +#include "qapi/error.h"
>>>   #include "qapi/qapi-commands-accelerator.h"
>>> +#include "migration/migration.h"
>>>   #include "monitor/monitor.h"
>>>   #include "monitor/hmp.h"
>>>   #include "hw/core/boards.h"
>>> @@ -37,20 +39,31 @@
>>>   #include "qemu/error-report.h"
>>>   #include "accel-internal.h"
>>>   
>>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>>   {
>>>       AccelClass *acc = ACCEL_GET_CLASS(accel);
>>>       int ret;
>>>       ms->accelerator = accel;
>>>       *(acc->allowed) = true;
>>> +
>>> +    if (!enforce_only_migratable(errp)) {
>>> +        ret = -EACCES;
>>> +        goto fail;
>>> +    }
>> 
>> Here I think you want to fail the initialization if the accelerator is
>> Xen, -only-migratable is set and there are blockers. At this point is
>> not clear whether these blockers really need a xen-specific category.
>
> The category reflects the specific requirements of the blocker itself. 
> Looking at vhost-scsi (which registers a Xen-affecting blocker) 
> clarifies why this distinction is necessary; this code merely enforces 
> registered blockers.
>
>> 
>> Isn't this too soon anyway? What blockers will be registered at this
>> point and if there are blockers on the list, why the
>> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
>> rejected the cmdline?
>
> This check is placed here because qemu_create_early_backends() runs 
> before configure_accelerators(). Command-line backends (such as qcow or 
> vmdk) can register blockers before xen_enabled() accurately reflects the 
> selected accelerator.
>

Ok, so we want to validate that the blockers registered so far don't
affect the current accel. This information is important to have in a
comment somewhere.

Now, in the bad case we're going to fail the startup anyway, so wouldn't
it be better to avoid all the error handling and just make this a
generic check after configure_accelerators?

void qemu_init(int argc, char **argv)
{
    configure_accelerators(argv[0]);
    phase_advance(PHASE_ACCEL_CREATED);

    enforce_only_migratable();
    ...

>> 
>>> +
>>>       ret = acc->init_machine(accel, ms);
>>>       if (ret < 0) {
>>> -        ms->accelerator = NULL;
>>> -        *(acc->allowed) = false;
>>> -        object_unref(OBJECT(accel));
>>> -    } else {
>>> -        object_set_accelerator_compat_props(acc->compat_props);
>>> +        error_setg(errp, "%s", strerror(-ret));
>>> +        goto fail;
>>>       }
>>> +
>>> +    object_set_accelerator_compat_props(acc->compat_props);
>>> +    return ret;
>>> +
>>> +fail:
>>> +    ms->accelerator = NULL;
>>> +    *(acc->allowed) = false;
>>> +    object_unref(OBJECT(accel));
>>>       return ret;
>>>   }
>>>   
>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>> index 657403cad011..f9847b5e7096 100644
>>> --- a/hw/scsi/vhost-scsi.c
>>> +++ b/hw/scsi/vhost-scsi.c
>>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>>>       }
>>>   
>>>       if (!vsc->migratable) {
>>> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>>> +
>>>           error_setg(&vsc->migration_blocker,
>>>                   "vhost-scsi does not support migration in all cases. "
>>>                   "When external environment supports it (Orchestrator migrates "
>>>                   "target SCSI device state or use shared storage over network), "
>>>                   "set 'migratable' property to true to enable migration.");
>>> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
>>> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
>>> +                                      modes, errp) < 0) {
>>>               goto free_virtio;
>>>           }
>>>       }
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index d0b864a76066..c177dc52a1c1 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -23,6 +23,7 @@
>>>   #include "system/runstate.h"
>>>   #include "system/system.h"
>>>   #include "system/cpu-throttle.h"
>>> +#include "system/xen.h"
>>>   #include "ram.h"
>>>   #include "migration/cpr.h"
>>>   #include "migration/global_state.h"
>>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>>   static MigrationState *current_migration;
>>>   static MigrationIncomingState *current_incoming;
>>>   
>>> -static GSList *migration_blockers[MIG_MODE__MAX];
>>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>>> +static bool enforcing_only_migratable;

I think I missed the need for this variable. Isn't only_migratable
enough?

>>>   
>>>   static bool migration_object_check(MigrationState *ms, Error **errp);
>>>   static bool migration_switchover_start(MigrationState *s, Error **errp);
>>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>>       return false;
>>>   }
>>>   
>>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>>> +{
>>> +    error_propagate_prepend(errp, *reasonp,
>>> +                            "disallowing migration blocker "
>>> +                            "(--only-migratable) for: ");
>>> +    *reasonp = NULL;
>>> +}
>>> +
>>>   static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>   {
>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>> +

Hmm, the original code is actually special casing CPR isn't it? I'd
prefer to be explicit instead:

    unsigned int ignore_only_migratable = (BIT(MIG_MODE_CPR_TRANSFER) |
                                           BIT(MIG_MODE_CPR_EXEC) |
                                           BIT(MIG_MODE_CPR_REBOOT));
    unsigned int cur_mode = blocker_mode() & ~ignore_only_migratable;

Where blocker_mode is:

/*
 * Migration blockers are scoped by migration mode. Nonetheless it's
 * still possible for an accelerator to have particularities that
 * require blocking migration, independently of mode. Use this helper
 * when indexing into migration_blockers to ensure the accelerator is
 * taken into account.
 */
static unsigned int *blocker_mode(void)
{
    if (phase_check(PHASE_ACCEL_CREATED)) {
        if (xen_enabled()) {
            return MIG_BLOCK_XEN;
        }
    }

    return migrate_mode();
}

>>>       ERRP_GUARD();
>>>   
>>> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>>> -        error_propagate_prepend(errp, *reasonp,
>>> -                                "disallowing migration blocker "
>>> -                                "(--only-migratable) for: ");
>>> -        *reasonp = NULL;
>>> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
>>> +        propagate_disallowed_blocker(errp, reasonp);
>>>           return true;
>>>       }
>>>       return false;
>>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>   
>>>   static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>>   {
>>> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>> 
>> Not a MigMode anymore.
>
> Good catch. I will fix the type in the next version.
>
>> 
>>>           if (modes & BIT(mode)) {
>>>               assert(g_slist_index(migration_blockers[mode],
>>>                                    *reasonp) == -1);
>>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>>>   
>>>   int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>   {
>>> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
>>> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>>   
>>>       if (is_busy(reasonp, errp)) {
>>>           return -EBUSY;
>>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>   void migrate_del_blocker(Error **reasonp)
>>>   {
>>>       if (*reasonp) {
>>> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>> 
>> Same here, just use int i.
>
> Will update the variable name and type accordingly.
>
>> 
>>>               migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>>>                                                         *reasonp);
>>>           }
>>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>>                  "during postcopy-active or postcopy-recover state");
>>>   }
>>>   
>>> +bool enforce_only_migratable(Error **errp)
>>> +{
>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>> +    GSList *blockers = migration_blockers[mode];
>> 
>> For Xen, what blockers are those? Is it the case that existing blockers
>> are simply not taking effect for Xen because the QMP command never
>> checks blockers? Or do you intend to add xen-specific blockers?
>
> The former.
>
> Existing migration blockers currently do not take effect for Xen, which 
> this patch fixes. However, applying all blockers from an existing 
> MigMode could cause a regression for the reasons stated above. 
> Therefore, this patch ensures that only blockers relevant to Xen are 
> applied.
>
> Specifically, it enforces the blockers added via migrate_add_blocker() 
> and the vhost-scsi blocker. The remaining blockers have not been proven 
> incompatible with Xen migration, so applying them unconditionally could 
> cause regressions.
>
> Regards,
> Akihiko Odaki
>
>> 
>>> +    Error *reason;
>>> +
>>> +    if (!only_migratable) {
>>> +        return true;
>>> +    }
>>> +
>>> +    if (blockers) {
>>> +        reason = error_copy(blockers->data);
>>> +        propagate_disallowed_blocker(errp, &reason);
>>> +        return false;
>>> +    }
>>> +
>>> +    enforcing_only_migratable = true;
>>> +    return true;
>>> +}

We could reduce the scope of migration_is_blocked so
enforce_only_migrate can call it instead of checking the blockers
itself:

// replaces migration_is_blocked
bool can_migrate(Error **errp)
{
    if (qemu_savevm_state_blocked(errp)) {
        return false;
    }

    if (migration_is_blocked(errp) {
        return false;
    }
    return true;
}

static bool migration_is_blocked(Error **errp)
{
    GSList *blockers;

    assert(phase_check(PHASE_ACCEL_CREATED));

    blockers = migration_blockers[blocker_mode()];
    if (blockers) {
        error_propagate(errp, error_copy(blockers->data));
        return true;
    }

    return false;
}

>>> +
>>>   bool migration_is_blocked(Error **errp)
>>>   {
>>>       GSList *blockers = migration_blockers[migrate_mode()];
>>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>>       return false;
>>>   }
>>>   
>>> +bool xen_migration_is_blocked(Error **errp)
>>> +{
>>> +    GSList *blockers = migration_blockers[MIG_XEN];

Does this need to use MIG_XEN? If we use something like the
blocker_mode() I suggested, then this function can be replaced with
can_migrate() and we make all the points that start migration uniform.

What do you think? Let me know if I missed any details.

Thanks

>>> +
>>> +    if (qemu_savevm_state_blocked(errp)) {
>>> +        return true;
>>> +    }
>>> +
>>> +    if (blockers) {
>>> +        error_propagate(errp, error_copy(blockers->data));
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   /* Returns true if continue to migrate, or false if error detected */
>>>   static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>>   {
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 4b590ea672d2..4e5e1af1b202 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>>>       int saved_vm_running;
>>>       int ret;
>>>   
>>> +    if (xen_migration_is_blocked(errp)) {
>>> +        return;
>>> +    }
>>> +
>>>       if (!has_live) {
>>>           /* live default to true so old version of Xen tool stack can have a
>>>            * successful live migration */
>>> diff --git a/system/vl.c b/system/vl.c
>>> index 9bd7664b85c7..21fa810637c3 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>>   
>>>   static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>   {
>>> +    Error *local_err = NULL;
>>>       bool *p_init_failed = opaque;
>>>       const char *acc = qemu_opt_get(opts, "accel");
>>>       AccelClass *ac = accel_find(acc);
>>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>                        accel,
>>>                        &error_fatal);
>>>   
>>> -    ret = accel_init_machine(accel, current_machine);
>>> +    ret = accel_init_machine(accel, current_machine, &local_err);
>>>       if (ret < 0) {
>>> -        if (!qtest_with_kvm || ret != -ENOENT) {
>>> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
>>> +        if (qtest_with_kvm && ret == -ENOENT) {
>>> +            error_free(local_err);
>>> +        } else {
>>> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>>           }
>>>           goto bad;
>>>       }

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Akihiko Odaki]

On 2026/09/24 6:35, Fabiano Rosas wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> On 2026/09/22 23:28, Fabiano Rosas wrote:
>>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>>
>>>> xen-save-devices-state saves device VMState outside QEMU's normal
>>>> migration-mode machinery, so it currently ignores migration blockers.
>>>> Add a separate blocker class for this command.
>>>>
>>>> Blockers registered with migrate_add_blocker() or
>>>> migrate_add_blocker_internal() now include this class. Existing
>>>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>>>> default blocker now covers normal migration and Xen device-state
>>>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>>>> the same host and not concurrently with the old QEMU; Xen save and
>>>> restore do not guarantee those conditions. Users can set
>>>> migratable=true when shared storage or orchestrator-migrated target
>>>> state makes migration safe.
>>>>
>>>
>>> So one thing is rejecting the xen-save-devices-state command if there
>>> are blockers. This could very well just use the normal mode (default),
>>> no? Or all modes, like qmp_dump_guest_memory() does?
>>
>> In general, blockers registered via migrate_add_blocker_normal() are
>> specific to standard QEMU live migration and do not apply to Xen. For
>> example, block formats like qcow or vhd register normal migration
>> blockers, but Xen explicitly supports migration with them [1].
>> Unconditionally applying normal-mode blockers to Xen would cause
>> regressions. vhost-scsi is the exception here and is handled specifically.
>>
>> Checking every mode would lead to the same regression. Note that
>> qmp_dump_guest_memory() registers an all-mode blocker during execution;
>> it does not check all existing blocker lists when invoked.
>>
>> [1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback
>>
>>>
>>> Another thing (complementary, could be in the same patch) is rejecting
>>> the command line if there are blocked devices, including any ones used
>>> along with Xen acceleration. Again, why does this need a new blocker
>>> category? I'm not seeing it.
>> You can use -only-migratable.
>>
>>>
>>>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>> ---
>>>>    include/migration/blocker.h | 10 ++++++-
>>>>    include/qemu/accel.h        |  2 +-
>>>>    migration/migration.h       |  2 ++
>>>>    accel/accel-system.c        | 25 +++++++++++++-----
>>>>    hw/scsi/vhost-scsi.c        |  5 +++-
>>>>    migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>>>>    migration/savevm.c          |  4 +++
>>>>    system/vl.c                 |  9 ++++---
>>>>    8 files changed, 99 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>>>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>>>> --- a/include/migration/blocker.h
>>>> +++ b/include/migration/blocker.h
>>>> @@ -16,6 +16,14 @@
>>>>    
>>>>    #include "qapi/qapi-types-migration.h"
>>>>    
>>>> +/**
>>>> + * define MIG_XEN - identifier for Xen migration
>>>
>>> Hm.
>>>
>>>> + *
>>>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>>>> + * Use this for migration blockers that affect Xen.
>>>> + */
>>>> +#define MIG_XEN MIG_MODE__MAX
>>>
>>> This could be fine, Xen migration is pre-existing and 'mode' is part of
>>> QAPI, we don't want to create a new mode for Xen.
>>>
>>>> +
>>>>    /**
>>>>     * @migrate_add_blocker - prevent all modes of migration from proceeding
>>>>     *
>>>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>>>>     *
>>>>     * @reasonp - address of an error to be returned whenever migration is attempted
>>>>     *
>>>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>>>> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
>>>
>>> But it's not a mode, so we shouldn't re-use @modes. We just want a place
>>> to store the Xen blockers, let's not introduce the confusion that this
>>> could be a mode somehow.
>>>
>>> We could just add migrate_add_blocker_xen and call add_blockers directly
>>> since you're changing that function to use the array size instead of
>>> MIG_MODE__MAX.
>>
>> I would prefer to keep a single function for adding blockers across all
>> migration types so that the full scope of a blocker can be seen at a
>> single call site (see the vhost-scsi changes for an example).
>>
> 
> Fair.
> 
>> Practically speaking, I think the risk of reusing @modes is minimal
>> because there is no alternative selective-blocking API to misuse. It
>> would indeed be cleaner to have a parameter name that encompasses both
>> MigMode and MIG_XEN, but I haven't come up with a better name yet;
>> suggestions are welcome.
>>
> 
> Keep the @mode, but let's do away with any mentions of MigMode. This is
> a "blocker mode", not "migration mode". wink wink

Good idea. That will also fix the documentation comments, along with the 
variable and function names. I will include this change in the next version.

> 
>>>
>>>>     *
>>>>     * @errp - [out] The reason (if any) we cannot block migration right now.
>>>>     *
>>>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>>>> index 6f65f1cb89a0..b6b456a82fc3 100644
>>>> --- a/include/qemu/accel.h
>>>> +++ b/include/qemu/accel.h
>>>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>>>    
>>>>    void accel_init_interfaces(AccelClass *ac);
>>>>    
>>>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>>>    
>>>>    /* Called just before os_setup_post (ie just before drop OS privs) */
>>>>    void accel_setup_post(MachineState *ms);
>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>> index 683bc7bdd521..f53fdb2df107 100644
>>>> --- a/migration/migration.h
>>>> +++ b/migration/migration.h
>>>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>>>    int migration_call_notifiers(MigrationEventType type, Error **errp);
>>>>    
>>>>    int migrate_init(MigrationState *s, Error **errp);
>>>> +bool enforce_only_migratable(Error **errp);
>>>>    bool migration_is_blocked(Error **errp);
>>>> +bool xen_migration_is_blocked(Error **errp);
>>>>    /* True if outgoing migration has entered postcopy phase */
>>>>    bool migration_in_postcopy(void);
>>>>    bool migration_postcopy_is_alive(MigrationStatus state);
>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>> index 1325b23864c8..75a691cb3cfe 100644
>>>> --- a/accel/accel-system.c
>>>> +++ b/accel/accel-system.c
>>>> @@ -26,7 +26,9 @@
>>>>    #include "qemu/osdep.h"
>>>>    #include "qemu/accel.h"
>>>>    #include "qom/compat-properties.h"
>>>> +#include "qapi/error.h"
>>>>    #include "qapi/qapi-commands-accelerator.h"
>>>> +#include "migration/migration.h"
>>>>    #include "monitor/monitor.h"
>>>>    #include "monitor/hmp.h"
>>>>    #include "hw/core/boards.h"
>>>> @@ -37,20 +39,31 @@
>>>>    #include "qemu/error-report.h"
>>>>    #include "accel-internal.h"
>>>>    
>>>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>>>    {
>>>>        AccelClass *acc = ACCEL_GET_CLASS(accel);
>>>>        int ret;
>>>>        ms->accelerator = accel;
>>>>        *(acc->allowed) = true;
>>>> +
>>>> +    if (!enforce_only_migratable(errp)) {
>>>> +        ret = -EACCES;
>>>> +        goto fail;
>>>> +    }
>>>
>>> Here I think you want to fail the initialization if the accelerator is
>>> Xen, -only-migratable is set and there are blockers. At this point is
>>> not clear whether these blockers really need a xen-specific category.
>>
>> The category reflects the specific requirements of the blocker itself.
>> Looking at vhost-scsi (which registers a Xen-affecting blocker)
>> clarifies why this distinction is necessary; this code merely enforces
>> registered blockers.
>>
>>>
>>> Isn't this too soon anyway? What blockers will be registered at this
>>> point and if there are blockers on the list, why the
>>> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
>>> rejected the cmdline?
>>
>> This check is placed here because qemu_create_early_backends() runs
>> before configure_accelerators(). Command-line backends (such as qcow or
>> vmdk) can register blockers before xen_enabled() accurately reflects the
>> selected accelerator.
>>
> 
> Ok, so we want to validate that the blockers registered so far don't
> affect the current accel. This information is important to have in a
> comment somewhere.
> 
> Now, in the bad case we're going to fail the startup anyway, so wouldn't
> it be better to avoid all the error handling and just make this a
> generic check after configure_accelerators?
> 
> void qemu_init(int argc, char **argv)
> {
>      configure_accelerators(argv[0]);
>      phase_advance(PHASE_ACCEL_CREATED);
> 
>      enforce_only_migratable();
>      ...

I agree. We can also add a comment here explaining that
-only-migratable needs to be enforced at this point.

> 
>>>
>>>> +
>>>>        ret = acc->init_machine(accel, ms);
>>>>        if (ret < 0) {
>>>> -        ms->accelerator = NULL;
>>>> -        *(acc->allowed) = false;
>>>> -        object_unref(OBJECT(accel));
>>>> -    } else {
>>>> -        object_set_accelerator_compat_props(acc->compat_props);
>>>> +        error_setg(errp, "%s", strerror(-ret));
>>>> +        goto fail;
>>>>        }
>>>> +
>>>> +    object_set_accelerator_compat_props(acc->compat_props);
>>>> +    return ret;
>>>> +
>>>> +fail:
>>>> +    ms->accelerator = NULL;
>>>> +    *(acc->allowed) = false;
>>>> +    object_unref(OBJECT(accel));
>>>>        return ret;
>>>>    }
>>>>    
>>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>> index 657403cad011..f9847b5e7096 100644
>>>> --- a/hw/scsi/vhost-scsi.c
>>>> +++ b/hw/scsi/vhost-scsi.c
>>>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>>>>        }
>>>>    
>>>>        if (!vsc->migratable) {
>>>> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>>>> +
>>>>            error_setg(&vsc->migration_blocker,
>>>>                    "vhost-scsi does not support migration in all cases. "
>>>>                    "When external environment supports it (Orchestrator migrates "
>>>>                    "target SCSI device state or use shared storage over network), "
>>>>                    "set 'migratable' property to true to enable migration.");
>>>> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
>>>> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
>>>> +                                      modes, errp) < 0) {
>>>>                goto free_virtio;
>>>>            }
>>>>        }
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index d0b864a76066..c177dc52a1c1 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -23,6 +23,7 @@
>>>>    #include "system/runstate.h"
>>>>    #include "system/system.h"
>>>>    #include "system/cpu-throttle.h"
>>>> +#include "system/xen.h"
>>>>    #include "ram.h"
>>>>    #include "migration/cpr.h"
>>>>    #include "migration/global_state.h"
>>>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>>>    static MigrationState *current_migration;
>>>>    static MigrationIncomingState *current_incoming;
>>>>    
>>>> -static GSList *migration_blockers[MIG_MODE__MAX];
>>>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>>>> +static bool enforcing_only_migratable;
> 
> I think I missed the need for this variable. Isn't only_migratable
> enough?

enforcing_only_migratable is initially false, allowing blockers to be 
registered without immediate enforcement.

Once enforce_only_migratable() is called, the existing blockers are 
validated, and this flag becomes true. Any blocker added later are then 
checked immediately. Together, this ensures all blockers are 
consistently enforced at runtime.

> 
>>>>    
>>>>    static bool migration_object_check(MigrationState *ms, Error **errp);
>>>>    static bool migration_switchover_start(MigrationState *s, Error **errp);
>>>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>>>        return false;
>>>>    }
>>>>    
>>>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>>>> +{
>>>> +    error_propagate_prepend(errp, *reasonp,
>>>> +                            "disallowing migration blocker "
>>>> +                            "(--only-migratable) for: ");
>>>> +    *reasonp = NULL;
>>>> +}
>>>> +
>>>>    static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>    {
>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>> +
> 
> Hmm, the original code is actually special casing CPR isn't it? I'd
> prefer to be explicit instead:
> 
>      unsigned int ignore_only_migratable = (BIT(MIG_MODE_CPR_TRANSFER) |
>                                             BIT(MIG_MODE_CPR_EXEC) |
>                                             BIT(MIG_MODE_CPR_REBOOT));
>      unsigned int cur_mode = blocker_mode() & ~ignore_only_migratable;

blocker_mode() returns an index, whereas ignore_only_migratable is a 
bitmask, so we'll need to adapt the bitwise operations accordingly to 
handle them properly.

> 
> Where blocker_mode is:
> 
> /*
>   * Migration blockers are scoped by migration mode. Nonetheless it's
>   * still possible for an accelerator to have particularities that
>   * require blocking migration, independently of mode. Use this helper
>   * when indexing into migration_blockers to ensure the accelerator is
>   * taken into account.
>   */
> static unsigned int *blocker_mode(void)
> {
>      if (phase_check(PHASE_ACCEL_CREATED)) {

It would be safer to use assert(phase_check(PHASE_ACCEL_CREATED)); here; 
otherwise, the returned value may not be valid.

>          if (xen_enabled()) {
>              return MIG_BLOCK_XEN;
>          }
>      }
> 
>      return migrate_mode();
> }

Aside from the minor adjustments mentioned above, explicitly 
special-casing CPR looks good to me overall.

> 
>>>>        ERRP_GUARD();
>>>>    
>>>> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>>>> -        error_propagate_prepend(errp, *reasonp,
>>>> -                                "disallowing migration blocker "
>>>> -                                "(--only-migratable) for: ");
>>>> -        *reasonp = NULL;
>>>> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
>>>> +        propagate_disallowed_blocker(errp, reasonp);
>>>>            return true;
>>>>        }
>>>>        return false;
>>>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>    
>>>>    static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>>>    {
>>>> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>
>>> Not a MigMode anymore.
>>
>> Good catch. I will fix the type in the next version.
>>
>>>
>>>>            if (modes & BIT(mode)) {
>>>>                assert(g_slist_index(migration_blockers[mode],
>>>>                                     *reasonp) == -1);
>>>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>>>>    
>>>>    int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>    {
>>>> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
>>>> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>>>    
>>>>        if (is_busy(reasonp, errp)) {
>>>>            return -EBUSY;
>>>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>    void migrate_del_blocker(Error **reasonp)
>>>>    {
>>>>        if (*reasonp) {
>>>> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>
>>> Same here, just use int i.
>>
>> Will update the variable name and type accordingly.
>>
>>>
>>>>                migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>>>>                                                          *reasonp);
>>>>            }
>>>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>>>                   "during postcopy-active or postcopy-recover state");
>>>>    }
>>>>    
>>>> +bool enforce_only_migratable(Error **errp)
>>>> +{
>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>> +    GSList *blockers = migration_blockers[mode];
>>>
>>> For Xen, what blockers are those? Is it the case that existing blockers
>>> are simply not taking effect for Xen because the QMP command never
>>> checks blockers? Or do you intend to add xen-specific blockers?
>>
>> The former.
>>
>> Existing migration blockers currently do not take effect for Xen, which
>> this patch fixes. However, applying all blockers from an existing
>> MigMode could cause a regression for the reasons stated above.
>> Therefore, this patch ensures that only blockers relevant to Xen are
>> applied.
>>
>> Specifically, it enforces the blockers added via migrate_add_blocker()
>> and the vhost-scsi blocker. The remaining blockers have not been proven
>> incompatible with Xen migration, so applying them unconditionally could
>> cause regressions.
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>>> +    Error *reason;
>>>> +
>>>> +    if (!only_migratable) {
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    if (blockers) {
>>>> +        reason = error_copy(blockers->data);
>>>> +        propagate_disallowed_blocker(errp, &reason);
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    enforcing_only_migratable = true;
>>>> +    return true;
>>>> +}
> 
> We could reduce the scope of migration_is_blocked so
> enforce_only_migrate can call it instead of checking the blockers
> itself:
> 
> // replaces migration_is_blocked
> bool can_migrate(Error **errp)
> {
>      if (qemu_savevm_state_blocked(errp)) {
>          return false;
>      }
> 
>      if (migration_is_blocked(errp) {
>          return false;
>      }
>      return true;
> }
> 
> static bool migration_is_blocked(Error **errp)
> {
>      GSList *blockers;
> 
>      assert(phase_check(PHASE_ACCEL_CREATED));
> 
>      blockers = migration_blockers[blocker_mode()];
>      if (blockers) {
>          error_propagate(errp, error_copy(blockers->data));
>          return true;
>      }
> 
>      return false;
> }

This might be bikeshedding, but I'd rather extract the logic to check 
migration_blockers to a new function, perhaps named 
has_migration_blockers().

I feel can_migrate() is a bit confusing since migrate_prepare() actually 
has more checks and it does not clearly differentiate itself from 
migration_is_blocked() well. Updating migration/savevm.c would also be 
slightly cumbersome.

While has_migration_blockers() doesn't perfectly differentiate itself 
from migration_is_blocked() either, aligning it with the existing 
"migration_blockers" variable clarifies its exact intent.

> 
>>>> +
>>>>    bool migration_is_blocked(Error **errp)
>>>>    {
>>>>        GSList *blockers = migration_blockers[migrate_mode()];
>>>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>>>        return false;
>>>>    }
>>>>    
>>>> +bool xen_migration_is_blocked(Error **errp)
>>>> +{
>>>> +    GSList *blockers = migration_blockers[MIG_XEN];
> 
> Does this need to use MIG_XEN? If we use something like the
> blocker_mode() I suggested, then this function can be replaced with
> can_migrate() and we make all the points that start migration uniform.
> 
> What do you think? Let me know if I missed any details.

That makes sense to me. I'll incorporate this approach in the next version.

Regards,
Akihiko Odaki

> 
> Thanks
> 
>>>> +
>>>> +    if (qemu_savevm_state_blocked(errp)) {
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    if (blockers) {
>>>> +        error_propagate(errp, error_copy(blockers->data));
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>>    /* Returns true if continue to migrate, or false if error detected */
>>>>    static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>>>    {
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index 4b590ea672d2..4e5e1af1b202 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>>>>        int saved_vm_running;
>>>>        int ret;
>>>>    
>>>> +    if (xen_migration_is_blocked(errp)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>>        if (!has_live) {
>>>>            /* live default to true so old version of Xen tool stack can have a
>>>>             * successful live migration */
>>>> diff --git a/system/vl.c b/system/vl.c
>>>> index 9bd7664b85c7..21fa810637c3 100644
>>>> --- a/system/vl.c
>>>> +++ b/system/vl.c
>>>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>>>    
>>>>    static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>    {
>>>> +    Error *local_err = NULL;
>>>>        bool *p_init_failed = opaque;
>>>>        const char *acc = qemu_opt_get(opts, "accel");
>>>>        AccelClass *ac = accel_find(acc);
>>>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>                         accel,
>>>>                         &error_fatal);
>>>>    
>>>> -    ret = accel_init_machine(accel, current_machine);
>>>> +    ret = accel_init_machine(accel, current_machine, &local_err);
>>>>        if (ret < 0) {
>>>> -        if (!qtest_with_kvm || ret != -ENOENT) {
>>>> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
>>>> +        if (qtest_with_kvm && ret == -ENOENT) {
>>>> +            error_free(local_err);
>>>> +        } else {
>>>> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>>>            }
>>>>            goto bad;
>>>>        }

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Fabiano Rosas]

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

> On 2026/09/24 6:35, Fabiano Rosas wrote:
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>> 
>>> On 2026/09/22 23:28, Fabiano Rosas wrote:
>>>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>>>
>>>>> xen-save-devices-state saves device VMState outside QEMU's normal
>>>>> migration-mode machinery, so it currently ignores migration blockers.
>>>>> Add a separate blocker class for this command.
>>>>>
>>>>> Blockers registered with migrate_add_blocker() or
>>>>> migrate_add_blocker_internal() now include this class. Existing
>>>>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>>>>> default blocker now covers normal migration and Xen device-state
>>>>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>>>>> the same host and not concurrently with the old QEMU; Xen save and
>>>>> restore do not guarantee those conditions. Users can set
>>>>> migratable=true when shared storage or orchestrator-migrated target
>>>>> state makes migration safe.
>>>>>
>>>>
>>>> So one thing is rejecting the xen-save-devices-state command if there
>>>> are blockers. This could very well just use the normal mode (default),
>>>> no? Or all modes, like qmp_dump_guest_memory() does?
>>>
>>> In general, blockers registered via migrate_add_blocker_normal() are
>>> specific to standard QEMU live migration and do not apply to Xen. For
>>> example, block formats like qcow or vhd register normal migration
>>> blockers, but Xen explicitly supports migration with them [1].
>>> Unconditionally applying normal-mode blockers to Xen would cause
>>> regressions. vhost-scsi is the exception here and is handled specifically.
>>>
>>> Checking every mode would lead to the same regression. Note that
>>> qmp_dump_guest_memory() registers an all-mode blocker during execution;
>>> it does not check all existing blocker lists when invoked.
>>>
>>> [1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback
>>>
>>>>
>>>> Another thing (complementary, could be in the same patch) is rejecting
>>>> the command line if there are blocked devices, including any ones used
>>>> along with Xen acceleration. Again, why does this need a new blocker
>>>> category? I'm not seeing it.
>>> You can use -only-migratable.
>>>
>>>>
>>>>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>>>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>>> ---
>>>>>    include/migration/blocker.h | 10 ++++++-
>>>>>    include/qemu/accel.h        |  2 +-
>>>>>    migration/migration.h       |  2 ++
>>>>>    accel/accel-system.c        | 25 +++++++++++++-----
>>>>>    hw/scsi/vhost-scsi.c        |  5 +++-
>>>>>    migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>>>>>    migration/savevm.c          |  4 +++
>>>>>    system/vl.c                 |  9 ++++---
>>>>>    8 files changed, 99 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>>>>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>>>>> --- a/include/migration/blocker.h
>>>>> +++ b/include/migration/blocker.h
>>>>> @@ -16,6 +16,14 @@
>>>>>    
>>>>>    #include "qapi/qapi-types-migration.h"
>>>>>    
>>>>> +/**
>>>>> + * define MIG_XEN - identifier for Xen migration
>>>>
>>>> Hm.
>>>>
>>>>> + *
>>>>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>>>>> + * Use this for migration blockers that affect Xen.
>>>>> + */
>>>>> +#define MIG_XEN MIG_MODE__MAX
>>>>
>>>> This could be fine, Xen migration is pre-existing and 'mode' is part of
>>>> QAPI, we don't want to create a new mode for Xen.
>>>>
>>>>> +
>>>>>    /**
>>>>>     * @migrate_add_blocker - prevent all modes of migration from proceeding
>>>>>     *
>>>>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>>>>>     *
>>>>>     * @reasonp - address of an error to be returned whenever migration is attempted
>>>>>     *
>>>>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>>>>> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
>>>>
>>>> But it's not a mode, so we shouldn't re-use @modes. We just want a place
>>>> to store the Xen blockers, let's not introduce the confusion that this
>>>> could be a mode somehow.
>>>>
>>>> We could just add migrate_add_blocker_xen and call add_blockers directly
>>>> since you're changing that function to use the array size instead of
>>>> MIG_MODE__MAX.
>>>
>>> I would prefer to keep a single function for adding blockers across all
>>> migration types so that the full scope of a blocker can be seen at a
>>> single call site (see the vhost-scsi changes for an example).
>>>
>> 
>> Fair.
>> 
>>> Practically speaking, I think the risk of reusing @modes is minimal
>>> because there is no alternative selective-blocking API to misuse. It
>>> would indeed be cleaner to have a parameter name that encompasses both
>>> MigMode and MIG_XEN, but I haven't come up with a better name yet;
>>> suggestions are welcome.
>>>
>> 
>> Keep the @mode, but let's do away with any mentions of MigMode. This is
>> a "blocker mode", not "migration mode". wink wink
>
> Good idea. That will also fix the documentation comments, along with the 
> variable and function names. I will include this change in the next version.
>
>> 
>>>>
>>>>>     *
>>>>>     * @errp - [out] The reason (if any) we cannot block migration right now.
>>>>>     *
>>>>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>>>>> index 6f65f1cb89a0..b6b456a82fc3 100644
>>>>> --- a/include/qemu/accel.h
>>>>> +++ b/include/qemu/accel.h
>>>>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>>>>    
>>>>>    void accel_init_interfaces(AccelClass *ac);
>>>>>    
>>>>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>>>>    
>>>>>    /* Called just before os_setup_post (ie just before drop OS privs) */
>>>>>    void accel_setup_post(MachineState *ms);
>>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>>> index 683bc7bdd521..f53fdb2df107 100644
>>>>> --- a/migration/migration.h
>>>>> +++ b/migration/migration.h
>>>>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>>>>    int migration_call_notifiers(MigrationEventType type, Error **errp);
>>>>>    
>>>>>    int migrate_init(MigrationState *s, Error **errp);
>>>>> +bool enforce_only_migratable(Error **errp);
>>>>>    bool migration_is_blocked(Error **errp);
>>>>> +bool xen_migration_is_blocked(Error **errp);
>>>>>    /* True if outgoing migration has entered postcopy phase */
>>>>>    bool migration_in_postcopy(void);
>>>>>    bool migration_postcopy_is_alive(MigrationStatus state);
>>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>>> index 1325b23864c8..75a691cb3cfe 100644
>>>>> --- a/accel/accel-system.c
>>>>> +++ b/accel/accel-system.c
>>>>> @@ -26,7 +26,9 @@
>>>>>    #include "qemu/osdep.h"
>>>>>    #include "qemu/accel.h"
>>>>>    #include "qom/compat-properties.h"
>>>>> +#include "qapi/error.h"
>>>>>    #include "qapi/qapi-commands-accelerator.h"
>>>>> +#include "migration/migration.h"
>>>>>    #include "monitor/monitor.h"
>>>>>    #include "monitor/hmp.h"
>>>>>    #include "hw/core/boards.h"
>>>>> @@ -37,20 +39,31 @@
>>>>>    #include "qemu/error-report.h"
>>>>>    #include "accel-internal.h"
>>>>>    
>>>>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>>>>    {
>>>>>        AccelClass *acc = ACCEL_GET_CLASS(accel);
>>>>>        int ret;
>>>>>        ms->accelerator = accel;
>>>>>        *(acc->allowed) = true;
>>>>> +
>>>>> +    if (!enforce_only_migratable(errp)) {
>>>>> +        ret = -EACCES;
>>>>> +        goto fail;
>>>>> +    }
>>>>
>>>> Here I think you want to fail the initialization if the accelerator is
>>>> Xen, -only-migratable is set and there are blockers. At this point is
>>>> not clear whether these blockers really need a xen-specific category.
>>>
>>> The category reflects the specific requirements of the blocker itself.
>>> Looking at vhost-scsi (which registers a Xen-affecting blocker)
>>> clarifies why this distinction is necessary; this code merely enforces
>>> registered blockers.
>>>
>>>>
>>>> Isn't this too soon anyway? What blockers will be registered at this
>>>> point and if there are blockers on the list, why the
>>>> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
>>>> rejected the cmdline?
>>>
>>> This check is placed here because qemu_create_early_backends() runs
>>> before configure_accelerators(). Command-line backends (such as qcow or
>>> vmdk) can register blockers before xen_enabled() accurately reflects the
>>> selected accelerator.
>>>
>> 
>> Ok, so we want to validate that the blockers registered so far don't
>> affect the current accel. This information is important to have in a
>> comment somewhere.
>> 
>> Now, in the bad case we're going to fail the startup anyway, so wouldn't
>> it be better to avoid all the error handling and just make this a
>> generic check after configure_accelerators?
>> 
>> void qemu_init(int argc, char **argv)
>> {
>>      configure_accelerators(argv[0]);
>>      phase_advance(PHASE_ACCEL_CREATED);
>> 
>>      enforce_only_migratable();
>>      ...
>
> I agree. We can also add a comment here explaining that
> -only-migratable needs to be enforced at this point.
>

See if you can structure these changes in a way that we first change the
behavior regarding when only_migratable is enforced, then later add the
Xen support. So we make it clear in the commit log how this affects the
normal mode.

>> 
>>>>
>>>>> +
>>>>>        ret = acc->init_machine(accel, ms);
>>>>>        if (ret < 0) {
>>>>> -        ms->accelerator = NULL;
>>>>> -        *(acc->allowed) = false;
>>>>> -        object_unref(OBJECT(accel));
>>>>> -    } else {
>>>>> -        object_set_accelerator_compat_props(acc->compat_props);
>>>>> +        error_setg(errp, "%s", strerror(-ret));
>>>>> +        goto fail;
>>>>>        }
>>>>> +
>>>>> +    object_set_accelerator_compat_props(acc->compat_props);
>>>>> +    return ret;
>>>>> +
>>>>> +fail:
>>>>> +    ms->accelerator = NULL;
>>>>> +    *(acc->allowed) = false;
>>>>> +    object_unref(OBJECT(accel));
>>>>>        return ret;
>>>>>    }
>>>>>    
>>>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>>> index 657403cad011..f9847b5e7096 100644
>>>>> --- a/hw/scsi/vhost-scsi.c
>>>>> +++ b/hw/scsi/vhost-scsi.c
>>>>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>>>>>        }
>>>>>    
>>>>>        if (!vsc->migratable) {
>>>>> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>>>>> +
>>>>>            error_setg(&vsc->migration_blocker,
>>>>>                    "vhost-scsi does not support migration in all cases. "
>>>>>                    "When external environment supports it (Orchestrator migrates "
>>>>>                    "target SCSI device state or use shared storage over network), "
>>>>>                    "set 'migratable' property to true to enable migration.");
>>>>> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
>>>>> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
>>>>> +                                      modes, errp) < 0) {
>>>>>                goto free_virtio;
>>>>>            }
>>>>>        }
>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>> index d0b864a76066..c177dc52a1c1 100644
>>>>> --- a/migration/migration.c
>>>>> +++ b/migration/migration.c
>>>>> @@ -23,6 +23,7 @@
>>>>>    #include "system/runstate.h"
>>>>>    #include "system/system.h"
>>>>>    #include "system/cpu-throttle.h"
>>>>> +#include "system/xen.h"
>>>>>    #include "ram.h"
>>>>>    #include "migration/cpr.h"
>>>>>    #include "migration/global_state.h"
>>>>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>>>>    static MigrationState *current_migration;
>>>>>    static MigrationIncomingState *current_incoming;
>>>>>    
>>>>> -static GSList *migration_blockers[MIG_MODE__MAX];
>>>>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>>>>> +static bool enforcing_only_migratable;
>> 
>> I think I missed the need for this variable. Isn't only_migratable
>> enough?
>
> enforcing_only_migratable is initially false, allowing blockers to be 
> registered without immediate enforcement.
>
> Once enforce_only_migratable() is called, the existing blockers are 
> validated, and this flag becomes true. Any blocker added later are then 
> checked immediately. Together, this ensures all blockers are 
> consistently enforced at runtime.
>

I see, otherwise we'll reject a blocker for MIG_MODE_NORMAL when in
reality the vm could be running Xen and therefore not want to reject
that blocker. 

>> 
>>>>>    
>>>>>    static bool migration_object_check(MigrationState *ms, Error **errp);
>>>>>    static bool migration_switchover_start(MigrationState *s, Error **errp);
>>>>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>>>>        return false;
>>>>>    }
>>>>>    
>>>>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>>>>> +{
>>>>> +    error_propagate_prepend(errp, *reasonp,
>>>>> +                            "disallowing migration blocker "
>>>>> +                            "(--only-migratable) for: ");
>>>>> +    *reasonp = NULL;
>>>>> +}
>>>>> +
>>>>>    static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>>    {
>>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>>> +
>> 
>> Hmm, the original code is actually special casing CPR isn't it? I'd
>> prefer to be explicit instead:
>> 
>>      unsigned int ignore_only_migratable = (BIT(MIG_MODE_CPR_TRANSFER) |
>>                                             BIT(MIG_MODE_CPR_EXEC) |
>>                                             BIT(MIG_MODE_CPR_REBOOT));
>>      unsigned int cur_mode = blocker_mode() & ~ignore_only_migratable;
>
> blocker_mode() returns an index, whereas ignore_only_migratable is a 
> bitmask, so we'll need to adapt the bitwise operations accordingly to 
> handle them properly.
>

Well spotted.

>> 
>> Where blocker_mode is:
>> 
>> /*
>>   * Migration blockers are scoped by migration mode. Nonetheless it's
>>   * still possible for an accelerator to have particularities that
>>   * require blocking migration, independently of mode. Use this helper
>>   * when indexing into migration_blockers to ensure the accelerator is
>>   * taken into account.
>>   */
>> static unsigned int *blocker_mode(void)
>> {
>>      if (phase_check(PHASE_ACCEL_CREATED)) {
>
> It would be safer to use assert(phase_check(PHASE_ACCEL_CREATED)); here; 
> otherwise, the returned value may not be valid.
>

If we want to allow blockers to be registered before accel being
configured, then we cannot assert.

>>          if (xen_enabled()) {
>>              return MIG_BLOCK_XEN;
>>          }
>>      }
>> 
>>      return migrate_mode();
>> }
>
> Aside from the minor adjustments mentioned above, explicitly 
> special-casing CPR looks good to me overall.
>
>> 
>>>>>        ERRP_GUARD();
>>>>>    
>>>>> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>>>>> -        error_propagate_prepend(errp, *reasonp,
>>>>> -                                "disallowing migration blocker "
>>>>> -                                "(--only-migratable) for: ");
>>>>> -        *reasonp = NULL;
>>>>> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
>>>>> +        propagate_disallowed_blocker(errp, reasonp);
>>>>>            return true;
>>>>>        }
>>>>>        return false;
>>>>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>>    
>>>>>    static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>>>>    {
>>>>> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>>> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>>
>>>> Not a MigMode anymore.
>>>
>>> Good catch. I will fix the type in the next version.
>>>
>>>>
>>>>>            if (modes & BIT(mode)) {
>>>>>                assert(g_slist_index(migration_blockers[mode],
>>>>>                                     *reasonp) == -1);
>>>>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>>>>>    
>>>>>    int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>>    {
>>>>> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
>>>>> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>>>>    
>>>>>        if (is_busy(reasonp, errp)) {
>>>>>            return -EBUSY;
>>>>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>>    void migrate_del_blocker(Error **reasonp)
>>>>>    {
>>>>>        if (*reasonp) {
>>>>> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>>> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>>
>>>> Same here, just use int i.
>>>
>>> Will update the variable name and type accordingly.
>>>
>>>>
>>>>>                migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>>>>>                                                          *reasonp);
>>>>>            }
>>>>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>>>>                   "during postcopy-active or postcopy-recover state");
>>>>>    }
>>>>>    
>>>>> +bool enforce_only_migratable(Error **errp)
>>>>> +{
>>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>>> +    GSList *blockers = migration_blockers[mode];
>>>>
>>>> For Xen, what blockers are those? Is it the case that existing blockers
>>>> are simply not taking effect for Xen because the QMP command never
>>>> checks blockers? Or do you intend to add xen-specific blockers?
>>>
>>> The former.
>>>
>>> Existing migration blockers currently do not take effect for Xen, which
>>> this patch fixes. However, applying all blockers from an existing
>>> MigMode could cause a regression for the reasons stated above.
>>> Therefore, this patch ensures that only blockers relevant to Xen are
>>> applied.
>>>
>>> Specifically, it enforces the blockers added via migrate_add_blocker()
>>> and the vhost-scsi blocker. The remaining blockers have not been proven
>>> incompatible with Xen migration, so applying them unconditionally could
>>> cause regressions.
>>>
>>> Regards,
>>> Akihiko Odaki
>>>
>>>>
>>>>> +    Error *reason;
>>>>> +
>>>>> +    if (!only_migratable) {
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    if (blockers) {
>>>>> +        reason = error_copy(blockers->data);
>>>>> +        propagate_disallowed_blocker(errp, &reason);
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    enforcing_only_migratable = true;
>>>>> +    return true;
>>>>> +}
>> 
>> We could reduce the scope of migration_is_blocked so
>> enforce_only_migrate can call it instead of checking the blockers
>> itself:
>> 
>> // replaces migration_is_blocked
>> bool can_migrate(Error **errp)
>> {
>>      if (qemu_savevm_state_blocked(errp)) {
>>          return false;
>>      }
>> 
>>      if (migration_is_blocked(errp) {
>>          return false;
>>      }
>>      return true;
>> }
>> 
>> static bool migration_is_blocked(Error **errp)
>> {
>>      GSList *blockers;
>> 
>>      assert(phase_check(PHASE_ACCEL_CREATED));
>> 
>>      blockers = migration_blockers[blocker_mode()];
>>      if (blockers) {
>>          error_propagate(errp, error_copy(blockers->data));
>>          return true;
>>      }
>> 
>>      return false;
>> }
>
> This might be bikeshedding, but I'd rather extract the logic to check 
> migration_blockers to a new function, perhaps named 
> has_migration_blockers().
>
> I feel can_migrate() is a bit confusing since migrate_prepare() actually 
> has more checks and it does not clearly differentiate itself from 
> migration_is_blocked() well. Updating migration/savevm.c would also be 
> slightly cumbersome.
>
> While has_migration_blockers() doesn't perfectly differentiate itself 
> from migration_is_blocked() either, aligning it with the existing 
> "migration_blockers" variable clarifies its exact intent.
>

Sure, go ahead. Maybe migration_active_blockers or some other word to
indicate the blockers affect the mode/accel currently in effect.

>> 
>>>>> +
>>>>>    bool migration_is_blocked(Error **errp)
>>>>>    {
>>>>>        GSList *blockers = migration_blockers[migrate_mode()];
>>>>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>>>>        return false;
>>>>>    }
>>>>>    
>>>>> +bool xen_migration_is_blocked(Error **errp)
>>>>> +{
>>>>> +    GSList *blockers = migration_blockers[MIG_XEN];
>> 
>> Does this need to use MIG_XEN? If we use something like the
>> blocker_mode() I suggested, then this function can be replaced with
>> can_migrate() and we make all the points that start migration uniform.
>> 
>> What do you think? Let me know if I missed any details.
>
> That makes sense to me. I'll incorporate this approach in the next version.
>

Thanks! See you in v2.

> Regards,
> Akihiko Odaki
>
>> 
>> Thanks
>> 
>>>>> +
>>>>> +    if (qemu_savevm_state_blocked(errp)) {
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    if (blockers) {
>>>>> +        error_propagate(errp, error_copy(blockers->data));
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>    /* Returns true if continue to migrate, or false if error detected */
>>>>>    static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>>>>    {
>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>> index 4b590ea672d2..4e5e1af1b202 100644
>>>>> --- a/migration/savevm.c
>>>>> +++ b/migration/savevm.c
>>>>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>>>>>        int saved_vm_running;
>>>>>        int ret;
>>>>>    
>>>>> +    if (xen_migration_is_blocked(errp)) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>        if (!has_live) {
>>>>>            /* live default to true so old version of Xen tool stack can have a
>>>>>             * successful live migration */
>>>>> diff --git a/system/vl.c b/system/vl.c
>>>>> index 9bd7664b85c7..21fa810637c3 100644
>>>>> --- a/system/vl.c
>>>>> +++ b/system/vl.c
>>>>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>>>>    
>>>>>    static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>    {
>>>>> +    Error *local_err = NULL;
>>>>>        bool *p_init_failed = opaque;
>>>>>        const char *acc = qemu_opt_get(opts, "accel");
>>>>>        AccelClass *ac = accel_find(acc);
>>>>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>                         accel,
>>>>>                         &error_fatal);
>>>>>    
>>>>> -    ret = accel_init_machine(accel, current_machine);
>>>>> +    ret = accel_init_machine(accel, current_machine, &local_err);
>>>>>        if (ret < 0) {
>>>>> -        if (!qtest_with_kvm || ret != -ENOENT) {
>>>>> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
>>>>> +        if (qtest_with_kvm && ret == -ENOENT) {
>>>>> +            error_free(local_err);
>>>>> +        } else {
>>>>> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>>>>            }
>>>>>            goto bad;
>>>>>        }

Re: [PATCH 2/2] migration: Implement Xen migration blocker

[Akihiko Odaki]

On 2026/09/24 21:38, Fabiano Rosas wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> On 2026/09/24 6:35, Fabiano Rosas wrote:
>>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>>
>>>> On 2026/09/22 23:28, Fabiano Rosas wrote:
>>>>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>>>>
>>>>>> xen-save-devices-state saves device VMState outside QEMU's normal
>>>>>> migration-mode machinery, so it currently ignores migration blockers.
>>>>>> Add a separate blocker class for this command.
>>>>>>
>>>>>> Blockers registered with migrate_add_blocker() or
>>>>>> migrate_add_blocker_internal() now include this class. Existing
>>>>>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>>>>>> default blocker now covers normal migration and Xen device-state
>>>>>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>>>>>> the same host and not concurrently with the old QEMU; Xen save and
>>>>>> restore do not guarantee those conditions. Users can set
>>>>>> migratable=true when shared storage or orchestrator-migrated target
>>>>>> state makes migration safe.
>>>>>>
>>>>>
>>>>> So one thing is rejecting the xen-save-devices-state command if there
>>>>> are blockers. This could very well just use the normal mode (default),
>>>>> no? Or all modes, like qmp_dump_guest_memory() does?
>>>>
>>>> In general, blockers registered via migrate_add_blocker_normal() are
>>>> specific to standard QEMU live migration and do not apply to Xen. For
>>>> example, block formats like qcow or vhd register normal migration
>>>> blockers, but Xen explicitly supports migration with them [1].
>>>> Unconditionally applying normal-mode blockers to Xen would cause
>>>> regressions. vhost-scsi is the exception here and is handled specifically.
>>>>
>>>> Checking every mode would lead to the same regression. Note that
>>>> qmp_dump_guest_memory() registers an all-mode blocker during execution;
>>>> it does not check all existing blocker lists when invoked.
>>>>
>>>> [1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback
>>>>
>>>>>
>>>>> Another thing (complementary, could be in the same patch) is rejecting
>>>>> the command line if there are blocked devices, including any ones used
>>>>> along with Xen acceleration. Again, why does this need a new blocker
>>>>> category? I'm not seeing it.
>>>> You can use -only-migratable.
>>>>
>>>>>
>>>>>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>>>>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>>>> ---
>>>>>>     include/migration/blocker.h | 10 ++++++-
>>>>>>     include/qemu/accel.h        |  2 +-
>>>>>>     migration/migration.h       |  2 ++
>>>>>>     accel/accel-system.c        | 25 +++++++++++++-----
>>>>>>     hw/scsi/vhost-scsi.c        |  5 +++-
>>>>>>     migration/migration.c       | 63 ++++++++++++++++++++++++++++++++++++++-------
>>>>>>     migration/savevm.c          |  4 +++
>>>>>>     system/vl.c                 |  9 ++++---
>>>>>>     8 files changed, 99 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>>>>>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>>>>>> --- a/include/migration/blocker.h
>>>>>> +++ b/include/migration/blocker.h
>>>>>> @@ -16,6 +16,14 @@
>>>>>>     
>>>>>>     #include "qapi/qapi-types-migration.h"
>>>>>>     
>>>>>> +/**
>>>>>> + * define MIG_XEN - identifier for Xen migration
>>>>>
>>>>> Hm.
>>>>>
>>>>>> + *
>>>>>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>>>>>> + * Use this for migration blockers that affect Xen.
>>>>>> + */
>>>>>> +#define MIG_XEN MIG_MODE__MAX
>>>>>
>>>>> This could be fine, Xen migration is pre-existing and 'mode' is part of
>>>>> QAPI, we don't want to create a new mode for Xen.
>>>>>
>>>>>> +
>>>>>>     /**
>>>>>>      * @migrate_add_blocker - prevent all modes of migration from proceeding
>>>>>>      *
>>>>>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>>>>>>      *
>>>>>>      * @reasonp - address of an error to be returned whenever migration is attempted
>>>>>>      *
>>>>>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>>>>>> + * @modes - the migration modes to be blocked, a bit set of MigMode and %MIG_XEN
>>>>>
>>>>> But it's not a mode, so we shouldn't re-use @modes. We just want a place
>>>>> to store the Xen blockers, let's not introduce the confusion that this
>>>>> could be a mode somehow.
>>>>>
>>>>> We could just add migrate_add_blocker_xen and call add_blockers directly
>>>>> since you're changing that function to use the array size instead of
>>>>> MIG_MODE__MAX.
>>>>
>>>> I would prefer to keep a single function for adding blockers across all
>>>> migration types so that the full scope of a blocker can be seen at a
>>>> single call site (see the vhost-scsi changes for an example).
>>>>
>>>
>>> Fair.
>>>
>>>> Practically speaking, I think the risk of reusing @modes is minimal
>>>> because there is no alternative selective-blocking API to misuse. It
>>>> would indeed be cleaner to have a parameter name that encompasses both
>>>> MigMode and MIG_XEN, but I haven't come up with a better name yet;
>>>> suggestions are welcome.
>>>>
>>>
>>> Keep the @mode, but let's do away with any mentions of MigMode. This is
>>> a "blocker mode", not "migration mode". wink wink
>>
>> Good idea. That will also fix the documentation comments, along with the
>> variable and function names. I will include this change in the next version.
>>
>>>
>>>>>
>>>>>>      *
>>>>>>      * @errp - [out] The reason (if any) we cannot block migration right now.
>>>>>>      *
>>>>>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>>>>>> index 6f65f1cb89a0..b6b456a82fc3 100644
>>>>>> --- a/include/qemu/accel.h
>>>>>> +++ b/include/qemu/accel.h
>>>>>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>>>>>     
>>>>>>     void accel_init_interfaces(AccelClass *ac);
>>>>>>     
>>>>>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>>>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>>>>>     
>>>>>>     /* Called just before os_setup_post (ie just before drop OS privs) */
>>>>>>     void accel_setup_post(MachineState *ms);
>>>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>>>> index 683bc7bdd521..f53fdb2df107 100644
>>>>>> --- a/migration/migration.h
>>>>>> +++ b/migration/migration.h
>>>>>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>>>>>     int migration_call_notifiers(MigrationEventType type, Error **errp);
>>>>>>     
>>>>>>     int migrate_init(MigrationState *s, Error **errp);
>>>>>> +bool enforce_only_migratable(Error **errp);
>>>>>>     bool migration_is_blocked(Error **errp);
>>>>>> +bool xen_migration_is_blocked(Error **errp);
>>>>>>     /* True if outgoing migration has entered postcopy phase */
>>>>>>     bool migration_in_postcopy(void);
>>>>>>     bool migration_postcopy_is_alive(MigrationStatus state);
>>>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>>>> index 1325b23864c8..75a691cb3cfe 100644
>>>>>> --- a/accel/accel-system.c
>>>>>> +++ b/accel/accel-system.c
>>>>>> @@ -26,7 +26,9 @@
>>>>>>     #include "qemu/osdep.h"
>>>>>>     #include "qemu/accel.h"
>>>>>>     #include "qom/compat-properties.h"
>>>>>> +#include "qapi/error.h"
>>>>>>     #include "qapi/qapi-commands-accelerator.h"
>>>>>> +#include "migration/migration.h"
>>>>>>     #include "monitor/monitor.h"
>>>>>>     #include "monitor/hmp.h"
>>>>>>     #include "hw/core/boards.h"
>>>>>> @@ -37,20 +39,31 @@
>>>>>>     #include "qemu/error-report.h"
>>>>>>     #include "accel-internal.h"
>>>>>>     
>>>>>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>>>>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>>>>>     {
>>>>>>         AccelClass *acc = ACCEL_GET_CLASS(accel);
>>>>>>         int ret;
>>>>>>         ms->accelerator = accel;
>>>>>>         *(acc->allowed) = true;
>>>>>> +
>>>>>> +    if (!enforce_only_migratable(errp)) {
>>>>>> +        ret = -EACCES;
>>>>>> +        goto fail;
>>>>>> +    }
>>>>>
>>>>> Here I think you want to fail the initialization if the accelerator is
>>>>> Xen, -only-migratable is set and there are blockers. At this point is
>>>>> not clear whether these blockers really need a xen-specific category.
>>>>
>>>> The category reflects the specific requirements of the blocker itself.
>>>> Looking at vhost-scsi (which registers a Xen-affecting blocker)
>>>> clarifies why this distinction is necessary; this code merely enforces
>>>> registered blockers.
>>>>
>>>>>
>>>>> Isn't this too soon anyway? What blockers will be registered at this
>>>>> point and if there are blockers on the list, why the
>>>>> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
>>>>> rejected the cmdline?
>>>>
>>>> This check is placed here because qemu_create_early_backends() runs
>>>> before configure_accelerators(). Command-line backends (such as qcow or
>>>> vmdk) can register blockers before xen_enabled() accurately reflects the
>>>> selected accelerator.
>>>>
>>>
>>> Ok, so we want to validate that the blockers registered so far don't
>>> affect the current accel. This information is important to have in a
>>> comment somewhere.
>>>
>>> Now, in the bad case we're going to fail the startup anyway, so wouldn't
>>> it be better to avoid all the error handling and just make this a
>>> generic check after configure_accelerators?
>>>
>>> void qemu_init(int argc, char **argv)
>>> {
>>>       configure_accelerators(argv[0]);
>>>       phase_advance(PHASE_ACCEL_CREATED);
>>>
>>>       enforce_only_migratable();
>>>       ...
>>
>> I agree. We can also add a comment here explaining that
>> -only-migratable needs to be enforced at this point.
>>
> 
> See if you can structure these changes in a way that we first change the
> behavior regarding when only_migratable is enforced, then later add the
> Xen support. So we make it clear in the commit log how this affects the
> normal mode.

I will split this patch into two as suggested: first adjusting the 
only_migratable enforcement, and then adding the Xen support.

> 
>>>
>>>>>
>>>>>> +
>>>>>>         ret = acc->init_machine(accel, ms);
>>>>>>         if (ret < 0) {
>>>>>> -        ms->accelerator = NULL;
>>>>>> -        *(acc->allowed) = false;
>>>>>> -        object_unref(OBJECT(accel));
>>>>>> -    } else {
>>>>>> -        object_set_accelerator_compat_props(acc->compat_props);
>>>>>> +        error_setg(errp, "%s", strerror(-ret));
>>>>>> +        goto fail;
>>>>>>         }
>>>>>> +
>>>>>> +    object_set_accelerator_compat_props(acc->compat_props);
>>>>>> +    return ret;
>>>>>> +
>>>>>> +fail:
>>>>>> +    ms->accelerator = NULL;
>>>>>> +    *(acc->allowed) = false;
>>>>>> +    object_unref(OBJECT(accel));
>>>>>>         return ret;
>>>>>>     }
>>>>>>     
>>>>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>>>> index 657403cad011..f9847b5e7096 100644
>>>>>> --- a/hw/scsi/vhost-scsi.c
>>>>>> +++ b/hw/scsi/vhost-scsi.c
>>>>>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>>>>>>         }
>>>>>>     
>>>>>>         if (!vsc->migratable) {
>>>>>> +        unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>>>>>> +
>>>>>>             error_setg(&vsc->migration_blocker,
>>>>>>                     "vhost-scsi does not support migration in all cases. "
>>>>>>                     "When external environment supports it (Orchestrator migrates "
>>>>>>                     "target SCSI device state or use shared storage over network), "
>>>>>>                     "set 'migratable' property to true to enable migration.");
>>>>>> -        if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
>>>>>> +        if (migrate_add_blocker_modes(&vsc->migration_blocker,
>>>>>> +                                      modes, errp) < 0) {
>>>>>>                 goto free_virtio;
>>>>>>             }
>>>>>>         }
>>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>>> index d0b864a76066..c177dc52a1c1 100644
>>>>>> --- a/migration/migration.c
>>>>>> +++ b/migration/migration.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>     #include "system/runstate.h"
>>>>>>     #include "system/system.h"
>>>>>>     #include "system/cpu-throttle.h"
>>>>>> +#include "system/xen.h"
>>>>>>     #include "ram.h"
>>>>>>     #include "migration/cpr.h"
>>>>>>     #include "migration/global_state.h"
>>>>>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>>>>>     static MigrationState *current_migration;
>>>>>>     static MigrationIncomingState *current_incoming;
>>>>>>     
>>>>>> -static GSList *migration_blockers[MIG_MODE__MAX];
>>>>>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>>>>>> +static bool enforcing_only_migratable;
>>>
>>> I think I missed the need for this variable. Isn't only_migratable
>>> enough?
>>
>> enforcing_only_migratable is initially false, allowing blockers to be
>> registered without immediate enforcement.
>>
>> Once enforce_only_migratable() is called, the existing blockers are
>> validated, and this flag becomes true. Any blocker added later are then
>> checked immediately. Together, this ensures all blockers are
>> consistently enforced at runtime.
>>
> 
> I see, otherwise we'll reject a blocker for MIG_MODE_NORMAL when in
> reality the vm could be running Xen and therefore not want to reject
> that blocker.
> 
>>>
>>>>>>     
>>>>>>     static bool migration_object_check(MigrationState *ms, Error **errp);
>>>>>>     static bool migration_switchover_start(MigrationState *s, Error **errp);
>>>>>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>>>>>         return false;
>>>>>>     }
>>>>>>     
>>>>>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>>>>>> +{
>>>>>> +    error_propagate_prepend(errp, *reasonp,
>>>>>> +                            "disallowing migration blocker "
>>>>>> +                            "(--only-migratable) for: ");
>>>>>> +    *reasonp = NULL;
>>>>>> +}
>>>>>> +
>>>>>>     static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>>>     {
>>>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>>>> +
>>>
>>> Hmm, the original code is actually special casing CPR isn't it? I'd
>>> prefer to be explicit instead:
>>>
>>>       unsigned int ignore_only_migratable = (BIT(MIG_MODE_CPR_TRANSFER) |
>>>                                              BIT(MIG_MODE_CPR_EXEC) |
>>>                                              BIT(MIG_MODE_CPR_REBOOT));
>>>       unsigned int cur_mode = blocker_mode() & ~ignore_only_migratable;
>>
>> blocker_mode() returns an index, whereas ignore_only_migratable is a
>> bitmask, so we'll need to adapt the bitwise operations accordingly to
>> handle them properly.
>>
> 
> Well spotted.
> 
>>>
>>> Where blocker_mode is:
>>>
>>> /*
>>>    * Migration blockers are scoped by migration mode. Nonetheless it's
>>>    * still possible for an accelerator to have particularities that
>>>    * require blocking migration, independently of mode. Use this helper
>>>    * when indexing into migration_blockers to ensure the accelerator is
>>>    * taken into account.
>>>    */
>>> static unsigned int *blocker_mode(void)
>>> {
>>>       if (phase_check(PHASE_ACCEL_CREATED)) {
>>
>> It would be safer to use assert(phase_check(PHASE_ACCEL_CREATED)); here;
>> otherwise, the returned value may not be valid.
>>
> 
> If we want to allow blockers to be registered before accel being
> configured, then we cannot assert.

Calling blocker_mode() is unnecessary before the accelerator is 
configured, as blockers are unconditionally registered at that stage.

By the way, while implementing this change, I realized that 
"special-casing CPR" isn't an entirely accurate description of the 
behavior. In reality, prior to this patch, -only-migratable always 
enforced normal mode blockers, even when the current migration mode was CPR.

I suspect this is a limitation of the -only-migratable command-line 
option: it cannot predict which migration mode will ultimately be used 
since the mode can be changed later via QMP. I will preserve the 
existing behavior for non-Xen scenarios.

> 
>>>           if (xen_enabled()) {
>>>               return MIG_BLOCK_XEN;
>>>           }
>>>       }
>>>
>>>       return migrate_mode();
>>> }
>>
>> Aside from the minor adjustments mentioned above, explicitly
>> special-casing CPR looks good to me overall.
>>
>>>
>>>>>>         ERRP_GUARD();
>>>>>>     
>>>>>> -    if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>>>>>> -        error_propagate_prepend(errp, *reasonp,
>>>>>> -                                "disallowing migration blocker "
>>>>>> -                                "(--only-migratable) for: ");
>>>>>> -        *reasonp = NULL;
>>>>>> +    if (enforcing_only_migratable && (modes & BIT(mode))) {
>>>>>> +        propagate_disallowed_blocker(errp, reasonp);
>>>>>>             return true;
>>>>>>         }
>>>>>>         return false;
>>>>>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
>>>>>>     
>>>>>>     static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>>>>>     {
>>>>>> -    for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>>>> +    for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>>>
>>>>> Not a MigMode anymore.
>>>>
>>>> Good catch. I will fix the type in the next version.
>>>>
>>>>>
>>>>>>             if (modes & BIT(mode)) {
>>>>>>                 assert(g_slist_index(migration_blockers[mode],
>>>>>>                                      *reasonp) == -1);
>>>>>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>>>>>>     
>>>>>>     int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>>>     {
>>>>>> -    unsigned modes = BIT(MIG_MODE__MAX) - 1;
>>>>>> +    unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>>>>>     
>>>>>>         if (is_busy(reasonp, errp)) {
>>>>>>             return -EBUSY;
>>>>>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>>>>>     void migrate_del_blocker(Error **reasonp)
>>>>>>     {
>>>>>>         if (*reasonp) {
>>>>>> -        for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>>>> +        for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>>>>
>>>>> Same here, just use int i.
>>>>
>>>> Will update the variable name and type accordingly.
>>>>
>>>>>
>>>>>>                 migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>>>>>>                                                           *reasonp);
>>>>>>             }
>>>>>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>>>>>                    "during postcopy-active or postcopy-recover state");
>>>>>>     }
>>>>>>     
>>>>>> +bool enforce_only_migratable(Error **errp)
>>>>>> +{
>>>>>> +    unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>>>>> +    GSList *blockers = migration_blockers[mode];
>>>>>
>>>>> For Xen, what blockers are those? Is it the case that existing blockers
>>>>> are simply not taking effect for Xen because the QMP command never
>>>>> checks blockers? Or do you intend to add xen-specific blockers?
>>>>
>>>> The former.
>>>>
>>>> Existing migration blockers currently do not take effect for Xen, which
>>>> this patch fixes. However, applying all blockers from an existing
>>>> MigMode could cause a regression for the reasons stated above.
>>>> Therefore, this patch ensures that only blockers relevant to Xen are
>>>> applied.
>>>>
>>>> Specifically, it enforces the blockers added via migrate_add_blocker()
>>>> and the vhost-scsi blocker. The remaining blockers have not been proven
>>>> incompatible with Xen migration, so applying them unconditionally could
>>>> cause regressions.
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>>> +    Error *reason;
>>>>>> +
>>>>>> +    if (!only_migratable) {
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (blockers) {
>>>>>> +        reason = error_copy(blockers->data);
>>>>>> +        propagate_disallowed_blocker(errp, &reason);
>>>>>> +        return false;
>>>>>> +    }
>>>>>> +
>>>>>> +    enforcing_only_migratable = true;
>>>>>> +    return true;
>>>>>> +}
>>>
>>> We could reduce the scope of migration_is_blocked so
>>> enforce_only_migrate can call it instead of checking the blockers
>>> itself:
>>>
>>> // replaces migration_is_blocked
>>> bool can_migrate(Error **errp)
>>> {
>>>       if (qemu_savevm_state_blocked(errp)) {
>>>           return false;
>>>       }
>>>
>>>       if (migration_is_blocked(errp) {
>>>           return false;
>>>       }
>>>       return true;
>>> }
>>>
>>> static bool migration_is_blocked(Error **errp)
>>> {
>>>       GSList *blockers;
>>>
>>>       assert(phase_check(PHASE_ACCEL_CREATED));
>>>
>>>       blockers = migration_blockers[blocker_mode()];
>>>       if (blockers) {
>>>           error_propagate(errp, error_copy(blockers->data));
>>>           return true;
>>>       }
>>>
>>>       return false;
>>> }
>>
>> This might be bikeshedding, but I'd rather extract the logic to check
>> migration_blockers to a new function, perhaps named
>> has_migration_blockers().
>>
>> I feel can_migrate() is a bit confusing since migrate_prepare() actually
>> has more checks and it does not clearly differentiate itself from
>> migration_is_blocked() well. Updating migration/savevm.c would also be
>> slightly cumbersome.
>>
>> While has_migration_blockers() doesn't perfectly differentiate itself
>> from migration_is_blocked() either, aligning it with the existing
>> "migration_blockers" variable clarifies its exact intent.
>>
> 
> Sure, go ahead. Maybe migration_active_blockers or some other word to
> indicate the blockers affect the mode/accel currently in effect.

Because -only-migratable enforces normal mode migration blockers even 
when the current migration mode is CPR, this function won't strictly be 
checking the presence of "active" blockers. For that reason, I would 
prefer to keep the name has_migration_blockers(). I'll update the 
function to take a parameter specifying which blocker mode to check.

Regards,
Akihiko Odaki

> 
>>>
>>>>>> +
>>>>>>     bool migration_is_blocked(Error **errp)
>>>>>>     {
>>>>>>         GSList *blockers = migration_blockers[migrate_mode()];
>>>>>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>>>>>         return false;
>>>>>>     }
>>>>>>     
>>>>>> +bool xen_migration_is_blocked(Error **errp)
>>>>>> +{
>>>>>> +    GSList *blockers = migration_blockers[MIG_XEN];
>>>
>>> Does this need to use MIG_XEN? If we use something like the
>>> blocker_mode() I suggested, then this function can be replaced with
>>> can_migrate() and we make all the points that start migration uniform.
>>>
>>> What do you think? Let me know if I missed any details.
>>
>> That makes sense to me. I'll incorporate this approach in the next version.
>>
> 
> Thanks! See you in v2.
> 
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>>>>> +
>>>>>> +    if (qemu_savevm_state_blocked(errp)) {
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (blockers) {
>>>>>> +        error_propagate(errp, error_copy(blockers->data));
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    return false;
>>>>>> +}
>>>>>> +
>>>>>>     /* Returns true if continue to migrate, or false if error detected */
>>>>>>     static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>>>>>     {
>>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>>> index 4b590ea672d2..4e5e1af1b202 100644
>>>>>> --- a/migration/savevm.c
>>>>>> +++ b/migration/savevm.c
>>>>>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>>>>>>         int saved_vm_running;
>>>>>>         int ret;
>>>>>>     
>>>>>> +    if (xen_migration_is_blocked(errp)) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>>         if (!has_live) {
>>>>>>             /* live default to true so old version of Xen tool stack can have a
>>>>>>              * successful live migration */
>>>>>> diff --git a/system/vl.c b/system/vl.c
>>>>>> index 9bd7664b85c7..21fa810637c3 100644
>>>>>> --- a/system/vl.c
>>>>>> +++ b/system/vl.c
>>>>>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>>>>>     
>>>>>>     static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>>     {
>>>>>> +    Error *local_err = NULL;
>>>>>>         bool *p_init_failed = opaque;
>>>>>>         const char *acc = qemu_opt_get(opts, "accel");
>>>>>>         AccelClass *ac = accel_find(acc);
>>>>>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>>                          accel,
>>>>>>                          &error_fatal);
>>>>>>     
>>>>>> -    ret = accel_init_machine(accel, current_machine);
>>>>>> +    ret = accel_init_machine(accel, current_machine, &local_err);
>>>>>>         if (ret < 0) {
>>>>>> -        if (!qtest_with_kvm || ret != -ENOENT) {
>>>>>> -            error_report("failed to initialize %s: %s", acc, strerror(-ret));
>>>>>> +        if (qtest_with_kvm && ret == -ENOENT) {
>>>>>> +            error_free(local_err);
>>>>>> +        } else {
>>>>>> +            error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>>>>>             }
>>>>>>             goto bad;
>>>>>>         }