[Qemu-devel] [PATCH 0/6] migration: objectify MigrationState

[Qemu-devel] [PATCH 0/6] migration: objectify MigrationState

[Peter Xu]

I picked this topic out as suggested by Juan. Also I did what Juan has
suggested in previous discussions that I moved lots of global
parameters into MigrationState, and let them be properties. Then we
can use HW_COMPAT_* and "-global migration.xxx=xxx" formular.

If without xen_init(), we can remove more LOC. However there is it, so
I changed the bits as best as I can.

If this can be merged and okay, we can move on to convert more things
into properties for migration.

Please review. Thanks.

Peter Xu (6):
  migration: let MigrationState be a qdev
  migration: move global_state.optional out
  migration: use compat bit for global_state
  migration: move only_migratable to MigrationState
  migration: move skip_configuration out
  migration: move skip_section_footers

 hw/i386/pc_piix.c             |  3 --
 hw/ppc/spapr.c                |  3 --
 include/hw/compat.h           | 12 ++++++
 include/migration/migration.h | 33 +++++++++++++++++
 include/sysemu/sysemu.h       |  1 -
 migration/migration.c         | 85 +++++++++++++++++++++++++++++++------------
 migration/savevm.c            | 22 +++++------
 vl.c                          |  9 ++++-
 8 files changed, 123 insertions(+), 45 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] migration: let MigrationState be a qdev

[Peter Xu]

Let the old man "MigrationState" join the object family. Direct benefit
is that we can start to use all the property features derived from
current QDev, like: HW_COMPAT_* bits, command line setup for migration
parameters (so will never need to set them up each time using HMP/QMP,
this is really, really attractive for test writters), etc.

I see no reason to disallow this happen yet. So let's start from this
one, to see whether it would be anything good.

No functional change at all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h | 19 +++++++++++++++
 migration/migration.c         | 57 +++++++++++++++++++++++++++++--------------
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 79b5484..bd0186c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -21,6 +21,7 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
@@ -49,6 +50,8 @@ enum mig_rp_message_type {
     MIG_RP_MSG_MAX
 };
 
+#define TYPE_MIGRATION "migration"
+
 /* State for the incoming migration */
 struct MigrationIncomingState {
     QEMUFile *from_src_file;
@@ -91,8 +94,24 @@ struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
+#define MIGRATION_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
+#define MIGRATION_OBJ(obj) \
+    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
+#define MIGRATION_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
+
+typedef struct MigrationClass {
+    /*< private >*/
+    DeviceClass parent_class;
+} MigrationClass;
+
 struct MigrationState
 {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/migration/migration.c b/migration/migration.c
index 48c94c9..483b027 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -94,28 +94,14 @@ static bool deferred_incoming;
 MigrationState *migrate_get_current(void)
 {
     static bool once;
-    static MigrationState current_migration = {
-        .state = MIGRATION_STATUS_NONE,
-        .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
-        .mbps = -1,
-        .parameters = {
-            .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
-            .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-            .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-            .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
-            .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
-            .max_bandwidth = MAX_THROTTLE,
-            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
-            .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
-        },
-    };
+    static MigrationState *current_migration;
 
     if (!once) {
-        current_migration.parameters.tls_creds = g_strdup("");
-        current_migration.parameters.tls_hostname = g_strdup("");
+        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
         once = true;
     }
-    return &current_migration;
+
+    return current_migration;
 }
 
 MigrationIncomingState *migration_incoming_get_current(void)
@@ -2123,3 +2109,38 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_instance_init(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+
+    ms->state = MIGRATION_STATUS_NONE;
+    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
+    ms->mbps = -1;
+    ms->parameters = (MigrationParameters) {
+        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
+        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+        .max_bandwidth = MAX_THROTTLE,
+        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
+        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+    };
+    ms->parameters.tls_creds = g_strdup("");
+    ms->parameters.tls_hostname = g_strdup("");
+}
+
+static const TypeInfo migration_type = {
+    .name = TYPE_MIGRATION,
+    .parent = TYPE_DEVICE,
+    .class_size = sizeof(MigrationClass),
+    .instance_size = sizeof(MigrationState),
+    .instance_init = migration_instance_init,
+};
+
+static void register_migration_types(void)
+{
+    type_register_static(&migration_type);
+}
+
+type_init(register_migration_types);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/6] migration: let MigrationState be a qdev

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
>
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
>
> No functional change at all.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

I am not qapi expert, but for the migration bits:

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Peter Xu]

Put it into MigrationState then we can use the properties to specify
whether to enable storing global state.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h |  6 ++++++
 migration/migration.c         | 21 +++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index bd0186c..8aa1ea6 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -162,6 +162,12 @@ struct MigrationState
     /* Do we have to clean up -b/-i from old migrate parameters */
     /* This feature is deprecated and will be removed */
     bool must_remove_block_options;
+
+    /*
+     * Global switch on whether we need to store the global state
+     * during migration.
+     */
+    bool store_global_state;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/migration.c b/migration/migration.c
index 483b027..0653f49 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -140,13 +140,13 @@ void migration_incoming_state_destroy(void)
 
 
 typedef struct {
-    bool optional;
     uint32_t size;
     uint8_t runstate[100];
     RunState state;
     bool received;
 } GlobalState;
 
+/* This is only used if MigrationState.store_global_state is set. */
 static GlobalState global_state;
 
 int global_state_store(void)
@@ -179,7 +179,7 @@ static RunState global_state_get_runstate(void)
 
 void global_state_set_optional(void)
 {
-    global_state.optional = true;
+    migrate_get_current()->store_global_state = false;
 }
 
 static bool global_state_needed(void *opaque)
@@ -188,8 +188,7 @@ static bool global_state_needed(void *opaque)
     char *runstate = (char *)s->runstate;
 
     /* If it is not optional, it is mandatory */
-
-    if (s->optional == false) {
+    if (migrate_get_current()->store_global_state) {
         return true;
     }
 
@@ -2109,6 +2108,19 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static Property migration_properties[] = {
+    DEFINE_PROP_BOOL("store-global-state", MigrationState,
+                     store_global_state, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = migration_properties;
+}
+
 static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
@@ -2133,6 +2145,7 @@ static void migration_instance_init(Object *obj)
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
     .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
     .class_size = sizeof(MigrationClass),
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/migration.h |  6 ++++++
>  migration/migration.c         | 21 +++++++++++++++++----
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index bd0186c..8aa1ea6 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -162,6 +162,12 @@ struct MigrationState
>      /* Do we have to clean up -b/-i from old migrate parameters */
>      /* This feature is deprecated and will be removed */
>      bool must_remove_block_options;
> +
> +    /*
> +     * Global switch on whether we need to store the global state
> +     * during migration.
> +     */
> +    bool store_global_state;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> diff --git a/migration/migration.c b/migration/migration.c
> index 483b027..0653f49 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -140,13 +140,13 @@ void migration_incoming_state_destroy(void)
>  
>  
>  typedef struct {
> -    bool optional;
>      uint32_t size;
>      uint8_t runstate[100];
>      RunState state;
>      bool received;
>  } GlobalState;
>  
> +/* This is only used if MigrationState.store_global_state is set. */
>  static GlobalState global_state;
>  
>  int global_state_store(void)
> @@ -179,7 +179,7 @@ static RunState global_state_get_runstate(void)
>  
>  void global_state_set_optional(void)
>  {
> -    global_state.optional = true;
> +    migrate_get_current()->store_global_state = false;

Part of the advantage (for me) of using qapi was not to have to export
a function to set this.  I.e.  isn't a way to call

qemu_opt_get_bool(migration_opts, "store_global_state", true)

qapi_<magic>_set_bool(migration_opts, "store_global_state",false);
?

So, I don't have to eport global_state_set_optional()?


My goal would be that when I need to add a new propertly, I add it to
migration_properties and to MigrationState and call it a day?

>  static bool global_state_needed(void *opaque)
> @@ -188,8 +188,7 @@ static bool global_state_needed(void *opaque)
>      char *runstate = (char *)s->runstate;
>  
>      /* If it is not optional, it is mandatory */
> -
> -    if (s->optional == false) {
> +    if (migrate_get_current()->store_global_state) {

Being able to query without having a function would also be nice.

Later, Juan.

>          return true;
>      }
>  
> @@ -2109,6 +2108,19 @@ void migrate_fd_connect(MigrationState *s)
>      s->migration_thread_running = true;
>  }
>  
> +static Property migration_properties[] = {
> +    DEFINE_PROP_BOOL("store-global-state", MigrationState,
> +                     store_global_state, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void migration_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = migration_properties;
> +}
> +
>  static void migration_instance_init(Object *obj)
>  {
>      MigrationState *ms = MIGRATION_OBJ(obj);
> @@ -2133,6 +2145,7 @@ static void migration_instance_init(Object *obj)
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
>      .parent = TYPE_DEVICE,
> +    .class_init = migration_class_init,
>      .class_size = sizeof(MigrationClass),
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,

Re: [Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Peter Xu]

On Wed, Jun 07, 2017 at 07:42:57PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Put it into MigrationState then we can use the properties to specify
> > whether to enable storing global state.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/migration/migration.h |  6 ++++++
> >  migration/migration.c         | 21 +++++++++++++++++----
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index bd0186c..8aa1ea6 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -162,6 +162,12 @@ struct MigrationState
> >      /* Do we have to clean up -b/-i from old migrate parameters */
> >      /* This feature is deprecated and will be removed */
> >      bool must_remove_block_options;
> > +
> > +    /*
> > +     * Global switch on whether we need to store the global state
> > +     * during migration.
> > +     */
> > +    bool store_global_state;
> >  };
> >  
> >  void migrate_set_state(int *state, int old_state, int new_state);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 483b027..0653f49 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -140,13 +140,13 @@ void migration_incoming_state_destroy(void)
> >  
> >  
> >  typedef struct {
> > -    bool optional;
> >      uint32_t size;
> >      uint8_t runstate[100];
> >      RunState state;
> >      bool received;
> >  } GlobalState;
> >  
> > +/* This is only used if MigrationState.store_global_state is set. */
> >  static GlobalState global_state;
> >  
> >  int global_state_store(void)
> > @@ -179,7 +179,7 @@ static RunState global_state_get_runstate(void)
> >  
> >  void global_state_set_optional(void)
> >  {
> > -    global_state.optional = true;
> > +    migrate_get_current()->store_global_state = false;
> 
> Part of the advantage (for me) of using qapi was not to have to export
> a function to set this.  I.e.  isn't a way to call
> 
> qemu_opt_get_bool(migration_opts, "store_global_state", true)
> 
> qapi_<magic>_set_bool(migration_opts, "store_global_state",false);
> ?

I didn't catch the comment here... Do you mean e.g.
qemu_opt_set_bool()? Here can we use it in some way?

(I thought we were using the "-global migration.store_global_state"
 parameter, then it'll setup MigrationState.store_global_state, isn't
 that the trick?)

> 
> So, I don't have to eport global_state_set_optional()?
> 

As mentioned in latter patch, xen_init() still uses it, so looks like
we still need it?

I can squash this patch with the next if you like it.

Thanks,

> 
> My goal would be that when I need to add a new propertly, I add it to
> migration_properties and to MigrationState and call it a day?
> 
> >  static bool global_state_needed(void *opaque)
> > @@ -188,8 +188,7 @@ static bool global_state_needed(void *opaque)
> >      char *runstate = (char *)s->runstate;
> >  
> >      /* If it is not optional, it is mandatory */
> > -
> > -    if (s->optional == false) {
> > +    if (migrate_get_current()->store_global_state) {
> 
> Being able to query without having a function would also be nice.
> 
> Later, Juan.
> 
> >          return true;
> >      }
> >  
> > @@ -2109,6 +2108,19 @@ void migrate_fd_connect(MigrationState *s)
> >      s->migration_thread_running = true;
> >  }
> >  
> > +static Property migration_properties[] = {
> > +    DEFINE_PROP_BOOL("store-global-state", MigrationState,
> > +                     store_global_state, true),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void migration_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->props = migration_properties;
> > +}
> > +
> >  static void migration_instance_init(Object *obj)
> >  {
> >      MigrationState *ms = MIGRATION_OBJ(obj);
> > @@ -2133,6 +2145,7 @@ static void migration_instance_init(Object *obj)
> >  static const TypeInfo migration_type = {
> >      .name = TYPE_MIGRATION,
> >      .parent = TYPE_DEVICE,
> > +    .class_init = migration_class_init,
> >      .class_size = sizeof(MigrationClass),
> >      .instance_size = sizeof(MigrationState),
> >      .instance_init = migration_instance_init,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jun 07, 2017 at 07:42:57PM +0200, Juan Quintela wrote:

>> > +/* This is only used if MigrationState.store_global_state is set. */
>> >  static GlobalState global_state;
>> >  
>> >  int global_state_store(void)
>> > @@ -179,7 +179,7 @@ static RunState global_state_get_runstate(void)
>> >  
>> >  void global_state_set_optional(void)
>> >  {
>> > -    global_state.optional = true;
>> > +    migrate_get_current()->store_global_state = false;
>> 
>> Part of the advantage (for me) of using qapi was not to have to export
>> a function to set this.  I.e.  isn't a way to call
>> 
>> qemu_opt_get_bool(migration_opts, "store_global_state", true)
>> 
>> qapi_<magic>_set_bool(migration_opts, "store_global_state",false);
>> ?
>
> I didn't catch the comment here... Do you mean e.g.
> qemu_opt_set_bool()? Here can we use it in some way?
>
> (I thought we were using the "-global migration.store_global_state"
>  parameter, then it'll setup MigrationState.store_global_state, isn't
>  that the trick?)

Yeap.  Althought for me would be the same if that is stored anywhare
else.  I don't really care where it is stored.

>> 
>> So, I don't have to eport global_state_set_optional()?
>> 
>
> As mentioned in latter patch, xen_init() still uses it, so looks like
> we still need it?

Yeap.  I *thought* that there was a way to test/set thing
programatically also so I didn't have to create/export that functions.
My ideal world would be that there were something like that

qemu_opt_get_bool(migration_opts, "store_global_state", true);

so I only have to export migration_opts (or whatever), and just set/read
values from places like xen_init.  Im my ideal world, if I have to
create a new "property", I don't want to have to export a function to
set/read it.  For instance, the case of xen_init().  We haven't been
able to remove global_state_set_optional() because they don't know about
properties.

I still love the patches are they are.  Boing able to set things from
the command line makes things so much better/easier O:-)


> I can squash this patch with the next if you like it.

That is up to you.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Peter Xu]

On Thu, Jun 08, 2017 at 01:12:29PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Jun 07, 2017 at 07:42:57PM +0200, Juan Quintela wrote:
> 
> >> > +/* This is only used if MigrationState.store_global_state is set. */
> >> >  static GlobalState global_state;
> >> >  
> >> >  int global_state_store(void)
> >> > @@ -179,7 +179,7 @@ static RunState global_state_get_runstate(void)
> >> >  
> >> >  void global_state_set_optional(void)
> >> >  {
> >> > -    global_state.optional = true;
> >> > +    migrate_get_current()->store_global_state = false;
> >> 
> >> Part of the advantage (for me) of using qapi was not to have to export
> >> a function to set this.  I.e.  isn't a way to call
> >> 
> >> qemu_opt_get_bool(migration_opts, "store_global_state", true)
> >> 
> >> qapi_<magic>_set_bool(migration_opts, "store_global_state",false);
> >> ?
> >
> > I didn't catch the comment here... Do you mean e.g.
> > qemu_opt_set_bool()? Here can we use it in some way?
> >
> > (I thought we were using the "-global migration.store_global_state"
> >  parameter, then it'll setup MigrationState.store_global_state, isn't
> >  that the trick?)
> 
> Yeap.  Althought for me would be the same if that is stored anywhare
> else.  I don't really care where it is stored.
> 
> >> 
> >> So, I don't have to eport global_state_set_optional()?
> >> 
> >
> > As mentioned in latter patch, xen_init() still uses it, so looks like
> > we still need it?
> 
> Yeap.  I *thought* that there was a way to test/set thing
> programatically also so I didn't have to create/export that functions.
> My ideal world would be that there were something like that
> 
> qemu_opt_get_bool(migration_opts, "store_global_state", true);
> 
> so I only have to export migration_opts (or whatever), and just set/read
> values from places like xen_init.  Im my ideal world, if I have to
> create a new "property", I don't want to have to export a function to
> set/read it.  For instance, the case of xen_init().  We haven't been
> able to remove global_state_set_optional() because they don't know about
> properties.
> 
> I still love the patches are they are.  Boing able to set things from
> the command line makes things so much better/easier O:-)

Oh, looks like what we need to do is just export
register_compat_prop(), then xen_init() can use it.

I'll try that tomorrow. :)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/6] migration: move global_state.optional out

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, Jun 08, 2017 at 01:12:29PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Wed, Jun 07, 2017 at 07:42:57PM +0200, Juan Quintela wrote:

...
>> 
>> Yeap.  I *thought* that there was a way to test/set thing
>> programatically also so I didn't have to create/export that functions.
>> My ideal world would be that there were something like that
>> 
>> qemu_opt_get_bool(migration_opts, "store_global_state", true);
>> 
>> so I only have to export migration_opts (or whatever), and just set/read
>> values from places like xen_init.  Im my ideal world, if I have to
>> create a new "property", I don't want to have to export a function to
>> set/read it.  For instance, the case of xen_init().  We haven't been
>> able to remove global_state_set_optional() because they don't know about
>> properties.
>> 
>> I still love the patches are they are.  Boing able to set things from
>> the command line makes things so much better/easier O:-)
>
> Oh, looks like what we need to do is just export
> register_compat_prop(), then xen_init() can use it.
>
> I'll try that tomorrow. :)

Thanks, Juan.

[Qemu-devel] [PATCH 3/6] migration: use compat bit for global_state

[Peter Xu]

Removing two callers of global_state_set_optional() since now we can use
HW_COMPAT_2_3. However there is still one more caller (xen_init), so we
still need to keep the function until it disappears.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c   | 1 -
 hw/ppc/spapr.c      | 1 -
 include/hw/compat.h | 4 ++++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2234bd0..c83cec5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -317,7 +317,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab3aab1..3e78bb9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3593,7 +3593,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 400c64b..5b5c8de 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -177,6 +177,10 @@
         .driver   = TYPE_PCI_DEVICE,\
         .property = "x-pcie-lnksta-dllla",\
         .value    = "off",\
+    },{\
+        .driver   = "migration",\
+        .property = "store-global-state",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_2 \
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/6] migration: use compat bit for global_state

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Removing two callers of global_state_set_optional() since now we can use
> HW_COMPAT_2_3. However there is still one more caller (xen_init), so we
> still need to keep the function until it disappears.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Ah, you do it here, nice.

[Qemu-devel] [PATCH 4/6] migration: move only_migratable to MigrationState

[Peter Xu]

One less global variable, and it does only matter with migration.

We keep the old "--only-migratable" option, but also now we support:

  -global migration.only-migratable=true

Currently still keep the old interface.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h | 3 +++
 include/sysemu/sysemu.h       | 1 -
 migration/migration.c         | 3 ++-
 migration/savevm.c            | 2 +-
 vl.c                          | 9 +++++++--
 5 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 8aa1ea6..e7d8e32 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -168,6 +168,9 @@ struct MigrationState
      * during migration.
      */
     bool store_global_state;
+
+    /* Whether the VM is only allowing for migratable devices */
+    bool only_migratable;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9841a52..b213696 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -15,7 +15,6 @@
 /* vl.c */
 
 extern const char *bios_name;
-extern int only_migratable;
 extern const char *qemu_name;
 extern QemuUUID qemu_uuid;
 extern bool qemu_uuid_set;
diff --git a/migration/migration.c b/migration/migration.c
index 0653f49..3492365 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1120,7 +1120,7 @@ static GSList *migration_blockers;
 
 int migrate_add_blocker(Error *reason, Error **errp)
 {
-    if (only_migratable) {
+    if (migrate_get_current()->only_migratable) {
         error_propagate(errp, error_copy(reason));
         error_prepend(errp, "disallowing migration blocker "
                           "(--only_migratable) for: ");
@@ -2111,6 +2111,7 @@ void migrate_fd_connect(MigrationState *s)
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
+    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..f073027 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2321,7 +2321,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
 bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
 {
     /* check needed if --only-migratable is specified */
-    if (!only_migratable) {
+    if (!migrate_get_current()->only_migratable) {
         return true;
     }
 
diff --git a/vl.c b/vl.c
index be4dcf2..e842eef 100644
--- a/vl.c
+++ b/vl.c
@@ -188,7 +188,6 @@ bool boot_strict;
 uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
-int only_migratable; /* turn it off unless user states otherwise */
 
 int icount_align_option;
 
@@ -3937,7 +3936,13 @@ int main(int argc, char **argv, char **envp)
                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:
-                only_migratable = 1;
+                /*
+                 * TODO: we can remove this option one day, and we
+                 * should all use:
+                 *
+                 * "-global migration.only-migratable=true"
+                 */
+                migrate_get_current()->only_migratable = true;
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] migration: move skip_configuration out

[Peter Xu]

It was in SaveState but now moved to MigrationState altogether. Again,
using HW_COMPAT_2_3 for old PC/SPAPR machines but still we'll have to
keep savevm_skip_configuration() since used by xen_init().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c             |  1 -
 hw/ppc/spapr.c                |  1 -
 include/hw/compat.h           |  4 ++++
 include/migration/migration.h |  3 +++
 migration/migration.c         |  2 ++
 migration/savevm.c            | 12 +++++-------
 6 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c83cec5..529018d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -317,7 +317,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    savevm_skip_configuration();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3e78bb9..227b03b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3593,7 +3593,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    savevm_skip_configuration();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 5b5c8de..4ed2ae7 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -179,6 +179,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "skip-configuration",\
+        .value    = "on",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/migration.h b/include/migration/migration.h
index e7d8e32..2d8c9f5 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -171,6 +171,9 @@ struct MigrationState
 
     /* Whether the VM is only allowing for migratable devices */
     bool only_migratable;
+
+    /* Whether we skip QEMU_VM_CONFIGURATION for migration */
+    bool skip_configuration;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/migration.c b/migration/migration.c
index 3492365..5d93d06 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2112,6 +2112,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
+    DEFINE_PROP_BOOL("skip-configuration", MigrationState,
+                     skip_configuration, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f073027..e7c18a4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -290,7 +290,6 @@ typedef struct SaveStateEntry {
 typedef struct SaveState {
     QTAILQ_HEAD(, SaveStateEntry) handlers;
     int global_section_id;
-    bool skip_configuration;
     uint32_t len;
     const char *name;
     uint32_t target_page_bits;
@@ -299,15 +298,13 @@ typedef struct SaveState {
 static SaveState savevm_state = {
     .handlers = QTAILQ_HEAD_INITIALIZER(savevm_state.handlers),
     .global_section_id = 0,
-    .skip_configuration = false,
 };
 
 void savevm_skip_configuration(void)
 {
-    savevm_state.skip_configuration = true;
+    migrate_get_current()->skip_configuration = true;
 }
 
-
 static void configuration_pre_save(void *opaque)
 {
     SaveState *state = opaque;
@@ -989,11 +986,11 @@ void qemu_savevm_state_header(QEMUFile *f)
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (!migrate_get_current()->skip_configuration ||
+        enforce_config_section()) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
-
 }
 
 void qemu_savevm_state_begin(QEMUFile *f)
@@ -2003,7 +2000,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (!migrate_get_current()->skip_configuration ||
+        enforce_config_section()) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/6] migration: move skip_configuration out

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It was in SaveState but now moved to MigrationState altogether. Again,
> using HW_COMPAT_2_3 for old PC/SPAPR machines but still we'll have to
> keep savevm_skip_configuration() since used by xen_init().

oops.

>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 6/6] migration: move skip_section_footers

[Peter Xu]

Move it into MigrationState, with a property binded to it.

Same trick played with HW_COMPAT_2_3.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c             | 1 -
 hw/ppc/spapr.c                | 1 -
 include/hw/compat.h           | 4 ++++
 include/migration/migration.h | 2 ++
 migration/migration.c         | 2 ++
 migration/savevm.c            | 8 +++-----
 6 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 529018d..1be23e2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -313,7 +313,6 @@ static void pc_init1(MachineState *machine,
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 227b03b..944f829 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3592,7 +3592,6 @@ DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
 static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
-    savevm_skip_section_footers();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 4ed2ae7..ef5fbc7 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -183,6 +183,10 @@
         .value    = "on",\
     },{\
         .driver   = "migration",\
+        .property = "skip-section-footer",\
+        .value    = "on",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 2d8c9f5..6bdaa97 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -174,6 +174,8 @@ struct MigrationState
 
     /* Whether we skip QEMU_VM_CONFIGURATION for migration */
     bool skip_configuration;
+    /* Whether we skip section footer */
+    bool skip_section_footer;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/migration.c b/migration/migration.c
index 5d93d06..e0e0970 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2114,6 +2114,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("skip-configuration", MigrationState,
                      skip_configuration, false),
+    DEFINE_PROP_BOOL("skip-section-footer", MigrationState,
+                     skip_section_footer, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/savevm.c b/migration/savevm.c
index e7c18a4..9f56d03 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -65,8 +65,6 @@
 
 const unsigned int postcopy_ram_discard_version = 0;
 
-static bool skip_section_footers;
-
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
     MIG_CMD_INVALID = 0,   /* Must be 0 */
@@ -787,7 +785,7 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
 
 void savevm_skip_section_footers(void)
 {
-    skip_section_footers = true;
+    migrate_get_current()->skip_section_footer = true;
 }
 
 /*
@@ -817,7 +815,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
  */
 static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
 {
-    if (!skip_section_footers) {
+    if (!migrate_get_current()->skip_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
     }
@@ -1815,7 +1813,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
     uint8_t read_mark;
     uint32_t read_section_id;
 
-    if (skip_section_footers) {
+    if (migrate_get_current()->skip_section_footer) {
         /* No footer to check */
         return true;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 6/6] migration: move skip_section_footers

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Move it into MigrationState, with a property binded to it.
>
> Same trick played with HW_COMPAT_2_3.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>