1. Patchew
  2. QEMU
  3. [PATCH 0/4] migration: savevm testing
  4. View patch

[PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

Fabiano Rosas posted 4 patches 5 days, 23 hours ago
[PATCH 1/4] migration/savevm: Add a compatibility check for capabilities
Posted by Fabiano Rosas 5 days, 23 hours ago 
It has always been possible to enable arbitrary migration capabilities
and attempt to take a snapshot of the VM with the savevm/loadvm
commands as well as their QMP counterparts
snapshot-save/snapshot-load.

Most migration capabilities are not meant to be used with snapshots
and there's a risk of crashing QEMU or producing incorrect
behavior. Ideally, every migration capability would either be
implemented for savevm or explicitly rejected.

Add a compatibility check routine and reject the snapshot command if
an incompatible capability is enabled. For now only act on the the two
that actually cause a crash: multifd and mapped-ram.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2881
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 26 ++++++++++++++++++++++++++
 migration/options.h |  1 +
 migration/savevm.c  |  8 ++++++++
 3 files changed, 35 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index b0ac2ea408..8772b98dca 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -443,11 +443,37 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
     MIGRATION_CAPABILITY_VALIDATE_UUID,
     MIGRATION_CAPABILITY_ZERO_COPY_SEND);
 
+/* Snapshot compatibility check list */
+static const
+INITIALIZE_MIGRATE_CAPS_SET(check_caps_savevm,
+                            MIGRATION_CAPABILITY_MULTIFD,
+                            MIGRATION_CAPABILITY_MAPPED_RAM,
+);
+
 static bool migrate_incoming_started(void)
 {
     return !!migration_incoming_get_current()->transport_data;
 }
 
+bool migrate_can_snapshot(Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    int i;
+
+    for (i = 0; i < check_caps_savevm.size; i++) {
+        int incomp_cap = check_caps_savevm.caps[i];
+
+        if (s->capabilities[incomp_cap]) {
+            error_setg(errp,
+                       "Snapshots are not compatible with %s",
+                       MigrationCapability_str(incomp_cap));
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /**
  * @migration_caps_check - check capability compatibility
  *
diff --git a/migration/options.h b/migration/options.h
index 762be4e641..20b71b6f2a 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -58,6 +58,7 @@ bool migrate_tls(void);
 /* capabilities helpers */
 
 bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
+bool migrate_can_snapshot(Error **errp);
 
 /* parameters */
 
diff --git a/migration/savevm.c b/migration/savevm.c
index ce158c3512..3be13bcfe8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -3229,6 +3229,10 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
 
     GLOBAL_STATE_CODE();
 
+    if (!migrate_can_snapshot(errp)) {
+        return false;
+    }
+
     if (migration_is_blocked(errp)) {
         return false;
     }
@@ -3413,6 +3417,10 @@ bool load_snapshot(const char *name, const char *vmstate,
     int ret;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (!migrate_can_snapshot(errp)) {
+        return false;
+    }
+
     if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
         return false;
     }
-- 
2.35.3
Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities
Posted by Marco Cavenati 5 days, 21 hours ago 
Hello Fabiano,

First of all thanks a lot for the quick follow up to my issue!

I just want to point out that with only mapped-ram enabled (without
multifd) savevm/loadvm do not lead to a crash but just to an error
according to my (few) experiments (on upstream).

Ciao

Marco

On Thursday, March 27, 2025 15:39 CET, Fabiano Rosas <farosas@suse.de> wrote:

> It has always been possible to enable arbitrary migration capabilities
> and attempt to take a snapshot of the VM with the savevm/loadvm
> commands as well as their QMP counterparts
> snapshot-save/snapshot-load.
> 
> Most migration capabilities are not meant to be used with snapshots
> and there's a risk of crashing QEMU or producing incorrect
> behavior. Ideally, every migration capability would either be
> implemented for savevm or explicitly rejected.
> 
> Add a compatibility check routine and reject the snapshot command if
> an incompatible capability is enabled. For now only act on the the two
> that actually cause a crash: multifd and mapped-ram.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2881
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 26 ++++++++++++++++++++++++++
>  migration/options.h |  1 +
>  migration/savevm.c  |  8 ++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/migration/options.c b/migration/options.c
> index b0ac2ea408..8772b98dca 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -443,11 +443,37 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>  
> +/* Snapshot compatibility check list */
> +static const
> +INITIALIZE_MIGRATE_CAPS_SET(check_caps_savevm,
> +                            MIGRATION_CAPABILITY_MULTIFD,
> +                            MIGRATION_CAPABILITY_MAPPED_RAM,
> +);
> +
>  static bool migrate_incoming_started(void)
>  {
>      return !!migration_incoming_get_current()->transport_data;
>  }
>  
> +bool migrate_can_snapshot(Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    int i;
> +
> +    for (i = 0; i < check_caps_savevm.size; i++) {
> +        int incomp_cap = check_caps_savevm.caps[i];
> +
> +        if (s->capabilities[incomp_cap]) {
> +            error_setg(errp,
> +                       "Snapshots are not compatible with %s",
> +                       MigrationCapability_str(incomp_cap));
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  /**
>   * @migration_caps_check - check capability compatibility
>   *
> diff --git a/migration/options.h b/migration/options.h
> index 762be4e641..20b71b6f2a 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -58,6 +58,7 @@ bool migrate_tls(void);
>  /* capabilities helpers */
>  
>  bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
> +bool migrate_can_snapshot(Error **errp);
>  
>  /* parameters */
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ce158c3512..3be13bcfe8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3229,6 +3229,10 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>  
>      GLOBAL_STATE_CODE();
>  
> +    if (!migrate_can_snapshot(errp)) {
> +        return false;
> +    }
> +
>      if (migration_is_blocked(errp)) {
>          return false;
>      }
> @@ -3413,6 +3417,10 @@ bool load_snapshot(const char *name, const char *vmstate,
>      int ret;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (!migrate_can_snapshot(errp)) {
> +        return false;
> +    }
> +
>      if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
>          return false;
>      }
> -- 
> 2.35.3
>
Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities
Posted by Fabiano Rosas 5 days, 20 hours ago 
"Marco Cavenati" <Marco.Cavenati@eurecom.fr> writes:

> Hello Fabiano,
>
> First of all thanks a lot for the quick follow up to my issue!
>
> I just want to point out that with only mapped-ram enabled (without
> multifd) savevm/loadvm do not lead to a crash but just to an error
> according to my (few) experiments (on upstream).
>

Yes, absolutely. I used imprecise language. Thanks for the correction,
I'll explain it better in the following versions.
Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities
Posted by Daniel P. Berrangé 5 days, 22 hours ago 
On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
> It has always been possible to enable arbitrary migration capabilities
> and attempt to take a snapshot of the VM with the savevm/loadvm
> commands as well as their QMP counterparts
> snapshot-save/snapshot-load.
> 
> Most migration capabilities are not meant to be used with snapshots
> and there's a risk of crashing QEMU or producing incorrect
> behavior. Ideally, every migration capability would either be
> implemented for savevm or explicitly rejected.

IMHO, this a prime example of why migration config shouldn't be held
as global state, and instead passed as parameters to the commands
that need them.  The snapshot-save/load commands would then only
be able to accept what few settings are actually relevant, instead
of inheriting any/all global migration state.

> Add a compatibility check routine and reject the snapshot command if
> an incompatible capability is enabled. For now only act on the the two
> that actually cause a crash: multifd and mapped-ram.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818

Issue is 2881 not 2818                                   ^^^^^^^

> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 26 ++++++++++++++++++++++++++
>  migration/options.h |  1 +
>  migration/savevm.c  |  8 ++++++++
>  3 files changed, 35 insertions(+)

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities
Posted by Fabiano Rosas 5 days, 22 hours ago 
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>> It has always been possible to enable arbitrary migration capabilities
>> and attempt to take a snapshot of the VM with the savevm/loadvm
>> commands as well as their QMP counterparts
>> snapshot-save/snapshot-load.
>> 
>> Most migration capabilities are not meant to be used with snapshots
>> and there's a risk of crashing QEMU or producing incorrect
>> behavior. Ideally, every migration capability would either be
>> implemented for savevm or explicitly rejected.
>
> IMHO, this a prime example of why migration config shouldn't be held
> as global state, and instead passed as parameters to the commands
> that need them.  The snapshot-save/load commands would then only
> be able to accept what few settings are actually relevant, instead
> of inheriting any/all global migration state.
>

Right, I remember we got caught around the fact that some migration
options are needed during runtime as well... but I don't remember the
details, let try to find that thread.

>> Add a compatibility check routine and reject the snapshot command if
>> an incompatible capability is enabled. For now only act on the the two
>> that actually cause a crash: multifd and mapped-ram.
>> 
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818
>
> Issue is 2881 not 2818                                   ^^^^^^^
>

It seem that was your own C-t =)

>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 26 ++++++++++++++++++++++++++
>>  migration/options.h |  1 +
>>  migration/savevm.c  |  8 ++++++++
>>  3 files changed, 35 insertions(+)
>
> With regards,
> Daniel

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.