[Qemu-devel] [PATCH v6 00/10] migration: objectify MigrationState

[Qemu-devel] [PATCH v6 00/10] migration: objectify MigrationState

[Peter Xu]

v6:
- patch 1: fix comment, and add more comment for
  register_compat_props() [Eduardo]
- patch 2: add more comments for register_compat_props_array() and the
  new AccelClass.global_props [Eduardo]
- patch 3: rename user_register_compat_props() into
  user_register_global_props, and some update on comments [Eduardo]
- patch 5: let xen_compat_props be static, and use a better "end of
  list" for it [Eduardo] (since the change is trivial, I didn't remove
  Juan's r-b)

v5:
- add r-b for Juan on patch 4,5,7-10
- patch 2: introduce register_compat_props_array() that may be further
  used by machine codes [Eduardo]
- patch 2: fix english error [Eduardo]

v4:
- dropped lots of patches related to AccelState.global_props. Now I am
  using AccelClass.global_props, and only use it in Xen codes.
- one patch is added to clean up the global property registerations,
  with some comment to show the ordering.
- one patch is added to dump migration globals in "info migrate"
  unconditionally (last patch).
- changed all the following xen_init() patches to use
  AccelClass.global_props.
- (since most of the patches are either touched-up again, or threw
  away, so no much r-b added, only one for Juan on the only_migratable
  patch, anyway, thanks for reviewing the old codes!)

------------

The main goal of this series is to let MigrationState be a QDev.

It helps in many use cases.

First of all, we can remove many legacy tricky functions. To name
some: savevm_skip_section_footers(), savevm_skip_configuration(), etc.
They didn't do much thing but setup a bool value. If MigrationState
can be a QDev, then these things can be setup by the HW_COMPAT_* magic
with some lines like:

{
    .driver   = "migration",
    .property = "send-configuration",
    .value    = "off",
}

Next, if this can be merged and okay, we can move on to convert more
things into properties for migration. A very attractive use case of it
is, we will be able to do this for migration:

  -global migration.postcopy=on

Then we don't need to use either HMP/QMP interface to enable it. It
greatly simplifies the migration test scripts.

Why QDev not QObject? The answer is simple: all the magic that we want
for migration is bound to QDev (HW_COMPAT, "-global"). If one day we
want to move these features from QDev to QObject, that'll be fine and
easy for MigrationState. But before that, let's have MigrationState a
QDev. :-)

Please kindly review. Thanks.

Peter Xu (10):
  machine: export register_compat_prop()
  accel: introduce AccelClass.global_props
  vl: clean up global property registerations
  migration: let MigrationState be a qdev
  migration: move global_state.optional out
  migration: move only_migratable to MigrationState
  migration: move skip_configuration out
  migration: move skip_section_footers
  migration: merge enforce_config_section somewhat
  migration: hmp: dump globals

 accel/accel.c                    |   6 ++
 hmp.c                            |   3 +
 hw/core/machine.c                |  13 -----
 hw/core/qdev-properties.c        |  21 +++++++
 hw/i386/pc_piix.c                |   3 -
 hw/ppc/spapr.c                   |   3 -
 hw/xen/xen-common.c              |  25 +++++++--
 include/hw/compat.h              |  12 ++++
 include/hw/qdev-properties.h     |  29 ++++++++++
 include/migration/global_state.h |   1 -
 include/migration/misc.h         |   6 +-
 include/sysemu/accel.h           |  11 ++++
 include/sysemu/sysemu.h          |   1 -
 migration/global_state.c         |   9 +--
 migration/migration.c            | 118 +++++++++++++++++++++++++++++++--------
 migration/migration.h            |  33 +++++++++++
 migration/savevm.c               |  32 ++---------
 vl.c                             |  41 ++++++++++++--
 18 files changed, 277 insertions(+), 90 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v6 01/10] machine: export register_compat_prop()

[Peter Xu]

We have HW_COMPAT_*, however that's only bound to machines, not other
things (like accelerators).  Behind it, it was register_compat_prop()
that played the trick.  Let's export the function for further use
outside HW_COMPAT_* magic.

Meanwhile, move it to qdev-properties.c where seems more proper (since
it'll be used not only in machine codes).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c            | 13 -------------
 hw/core/qdev-properties.c    | 14 ++++++++++++++
 include/hw/qdev-properties.h | 23 +++++++++++++++++++++++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2e7e977..ecb5552 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -770,19 +770,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     g_free(mc->name);
 }
 
-static void register_compat_prop(const char *driver,
-                                 const char *property,
-                                 const char *value)
-{
-    GlobalProperty *p = g_new0(GlobalProperty, 1);
-    /* Machine compat_props must never cause errors: */
-    p->errp = &error_abort;
-    p->driver = driver;
-    p->property = property;
-    p->value = value;
-    qdev_prop_register_global(p);
-}
-
 static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
 {
     GlobalProperty *p = opaque;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 68cd653..adf62ad 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1084,6 +1084,20 @@ void qdev_prop_register_global(GlobalProperty *prop)
     global_props = g_list_append(global_props, prop);
 }
 
+void register_compat_prop(const char *driver,
+                          const char *property,
+                          const char *value)
+{
+    GlobalProperty *p = g_new0(GlobalProperty, 1);
+
+    /* Any compat_props must never cause error */
+    p->errp = &error_abort;
+    p->driver = driver;
+    p->property = property;
+    p->value = value;
+    qdev_prop_register_global(p);
+}
+
 void qdev_prop_register_global_list(GlobalProperty *props)
 {
     int i;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 1e5c928..4ab4359 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -208,6 +208,29 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
 /**
+ * register_compat_prop:
+ *
+ * Register internal (not user-provided) global property, changing the
+ * default value of a given property in a device type.  This can be used
+ * for enabling machine-type compatibility or for enabling
+ * accelerator-specific defaults in devices.
+ *
+ * The property values set using this function must be always valid and
+ * never report setter errors, as they property have
+ * GlobalProperty::errp set to &error_abort.
+ *
+ * User-provided global properties should override internal global
+ * properties, so callers of this function should ensure that it is
+ * called before user-provided global properties are registered.
+ *
+ * @driver: Device type to be affected
+ * @property: Property whose default value is going to be changed
+ * @value: New default value for the property
+ */
+void register_compat_prop(const char *driver, const char *property,
+                          const char *value);
+
+/**
  * qdev_property_add_static:
  * @dev: Device to add the property to.
  * @prop: The qdev property definition.
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 01/10] machine: export register_compat_prop()

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:10PM +0800, Peter Xu wrote:
[...]
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 1e5c928..4ab4359 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -208,6 +208,29 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
>  /**
> + * register_compat_prop:
> + *
> + * Register internal (not user-provided) global property, changing the
> + * default value of a given property in a device type.  This can be used
> + * for enabling machine-type compatibility or for enabling
> + * accelerator-specific defaults in devices.
> + *
> + * The property values set using this function must be always valid and
> + * never report setter errors, as they property have

Typo (my fault, sorry).  It should be "the property have", or "the
property will have".

With this fixed (which may be done manually by the maintainer when
applying):

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> + * GlobalProperty::errp set to &error_abort.
> + *
> + * User-provided global properties should override internal global
> + * properties, so callers of this function should ensure that it is
> + * called before user-provided global properties are registered.
> + *
> + * @driver: Device type to be affected
> + * @property: Property whose default value is going to be changed
> + * @value: New default value for the property
> + */
> +void register_compat_prop(const char *driver, const char *property,
> +                          const char *value);
> +
> +/**
>   * qdev_property_add_static:
>   * @dev: Device to add the property to.
>   * @prop: The qdev property definition.
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 01/10] machine: export register_compat_prop()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On 06/27/2017 09:55 AM, Eduardo Habkost wrote:

>> + *
>> + * The property values set using this function must be always valid and
>> + * never report setter errors, as they property have
> 
> Typo (my fault, sorry).  It should be "the property have", or "the
> property will have".
> 

"the property will have"

> With this fixed (which may be done manually by the maintainer when
> applying):
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
>> + * GlobalProperty::errp set to &error_abort.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 01/10] machine: export register_compat_prop()

[Peter Xu]

On Tue, Jun 27, 2017 at 10:05:54AM -0500, Eric Blake wrote:
> On 06/27/2017 09:55 AM, Eduardo Habkost wrote:
> 
> >> + *
> >> + * The property values set using this function must be always valid and
> >> + * never report setter errors, as they property have
> > 
> > Typo (my fault, sorry).  It should be "the property have", or "the
> > property will have".
> > 
> 
> "the property will have"

Thanks Eduardo & Eric for reviewing. I suppose this series will go
through Juan's tree if finally proper? Anyway, please anyone let me
know if we need a repost.

> 
> > With this fixed (which may be done manually by the maintainer when
> > applying):
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Peter Xu

[Qemu-devel] [PATCH v6 02/10] accel: introduce AccelClass.global_props

[Peter Xu]

Introduce this new field for the accelerator classes so that each
specific accelerator in the future can register its own global
properties to be used further by the system. It works just like how the
old machine compatible properties do, but only tailored for
accelerators.

Introduce register_compat_props_array() for it. Export it so that it may
be used in other codes as well in the future.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/accel.c                |  6 ++++++
 hw/core/qdev-properties.c    |  7 +++++++
 include/hw/qdev-properties.h |  6 ++++++
 include/sysemu/accel.h       | 11 +++++++++++
 vl.c                         |  1 +
 5 files changed, 31 insertions(+)

diff --git a/accel/accel.c b/accel/accel.c
index 7c079a5..fa85844 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -120,6 +120,12 @@ void configure_accelerator(MachineState *ms)
     }
 }
 
+void accel_register_compat_props(AccelState *accel)
+{
+    AccelClass *class = ACCEL_GET_CLASS(accel);
+    register_compat_props_array(class->global_props);
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index adf62ad..f11d578 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1098,6 +1098,13 @@ void register_compat_prop(const char *driver,
     qdev_prop_register_global(p);
 }
 
+void register_compat_props_array(GlobalProperty *prop)
+{
+    for (; prop && prop->driver; prop++) {
+        register_compat_prop(prop->driver, prop->property, prop->value);
+    }
+}
+
 void qdev_prop_register_global_list(GlobalProperty *props)
 {
     int i;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 4ab4359..0ef7b21 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -229,6 +229,12 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
  */
 void register_compat_prop(const char *driver, const char *property,
                           const char *value);
+/*
+ * register_compat_props_array(): using register_compat_prop(), which
+ * only registers internal global properties (which has lower priority
+ * than user-provided global properties)
+ */
+void register_compat_props_array(GlobalProperty *prop);
 
 /**
  * qdev_property_add_static:
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 15944c1..ecc5c84 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -24,6 +24,7 @@
 #define HW_ACCEL_H
 
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
 
 typedef struct AccelState {
     /*< private >*/
@@ -40,6 +41,14 @@ typedef struct AccelClass {
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
     bool *allowed;
+    /*
+     * Array of global properties that would be applied when specific
+     * accelerator is chosen. It works like MachineClass.compat_props
+     * but it's for accelerators not machines. Accelerator-provided
+     * global properties may be overridden by machine-type
+     * compat_props or user-provided global properties.
+     */
+    GlobalProperty *global_props;
 } AccelClass;
 
 #define TYPE_ACCEL "accel"
@@ -57,5 +66,7 @@ typedef struct AccelClass {
 extern int tcg_tb_size;
 
 void configure_accelerator(MachineState *ms);
+/* Register accelerator specific global properties */
+void accel_register_compat_props(AccelState *accel);
 
 #endif
diff --git a/vl.c b/vl.c
index 59fea15..4452d7a 100644
--- a/vl.c
+++ b/vl.c
@@ -4571,6 +4571,7 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
+    accel_register_compat_props(current_machine->accelerator);
     machine_register_compat_props(current_machine);
 
     qemu_opts_foreach(qemu_find_opts("global"),
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 02/10] accel: introduce AccelClass.global_props

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:11PM +0800, Peter Xu wrote:
> Introduce this new field for the accelerator classes so that each
> specific accelerator in the future can register its own global
> properties to be used further by the system. It works just like how the
> old machine compatible properties do, but only tailored for
> accelerators.
> 
> Introduce register_compat_props_array() for it. Export it so that it may
> be used in other codes as well in the future.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v6 03/10] vl: clean up global property registerations

[Peter Xu]

It's not that clear on how the global properties are registered to
global_props (and also its priority relationship). Let's provide a
single function to be called in main() for that, with comment to explain
it a bit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 vl.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 4452d7a..c0cdb17 100644
--- a/vl.c
+++ b/vl.c
@@ -2969,6 +2969,25 @@ static int qemu_read_default_config_file(void)
     return 0;
 }
 
+static void user_register_global_props(void)
+{
+    qemu_opts_foreach(qemu_find_opts("global"),
+                      global_init_func, NULL, NULL);
+}
+
+/*
+ * Note: we should see that these properties are actually having a
+ * priority: accel < machine < user. This means e.g. when user
+ * specifies something in "-global", it'll always be used with highest
+ * priority than either machine/accelerator compat properties.
+ */
+static void register_global_properties(MachineState *ms)
+{
+    accel_register_compat_props(ms->accelerator);
+    machine_register_compat_props(ms);
+    user_register_global_props();
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -4571,11 +4590,11 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
-    accel_register_compat_props(current_machine->accelerator);
-    machine_register_compat_props(current_machine);
-
-    qemu_opts_foreach(qemu_find_opts("global"),
-                      global_init_func, NULL, NULL);
+    /*
+     * Register all the global properties, including accel properties,
+     * machine properties, and user-specified ones.
+     */
+    register_global_properties(current_machine);
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 03/10] vl: clean up global property registerations

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:12PM +0800, Peter Xu wrote:
> It's not that clear on how the global properties are registered to
> global_props (and also its priority relationship). Let's provide a
> single function to be called in main() for that, with comment to explain
> it a bit.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

On subject: s/registerations/registration/

With this fixed:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v6 04/10] 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.

Now we init the MigrationState struct statically in main() to make sure
it's initialized after global properties are applied, since we'll use
them during creation of the object.

No functional change at all.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  1 +
 migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
 migration/migration.h    | 19 ++++++++++++
 vl.c                     |  6 ++++
 4 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 65c7070..2d36cf5 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -45,6 +45,7 @@ void savevm_skip_section_footers(void);
 void savevm_skip_configuration(void);
 
 /* migration/migration.c */
+void migration_object_init(void);
 void qemu_start_incoming_migration(const char *uri, Error **errp);
 bool migration_is_idle(void);
 void add_migration_state_change_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index f588329..2c25927 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -98,32 +98,21 @@ enum mig_rp_message_type {
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
 
+static MigrationState *current_migration;
+
+void migration_object_init(void)
+{
+    /* This can only be called once. */
+    assert(!current_migration);
+    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+}
+
 /* For outgoing */
 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,
-        },
-    };
-
-    if (!once) {
-        current_migration.parameters.tls_creds = g_strdup("");
-        current_migration.parameters.tls_hostname = g_strdup("");
-        once = true;
-    }
-    return &current_migration;
+    /* This can only be called after the object created. */
+    assert(current_migration);
+    return current_migration;
 }
 
 MigrationIncomingState *migration_incoming_get_current(void)
@@ -1987,3 +1976,46 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = false;
+}
+
+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_init = migration_class_init,
+    .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);
diff --git a/migration/migration.h b/migration/migration.h
index d9a268a..3fca364 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -19,6 +19,7 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 /* State for the incoming migration */
 struct MigrationIncomingState {
@@ -62,8 +63,26 @@ struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
+#define TYPE_MIGRATION "migration"
+
+#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/vl.c b/vl.c
index c0cdb17..f0a0515 100644
--- a/vl.c
+++ b/vl.c
@@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
      */
     register_global_properties(current_machine);
 
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
        clock values from the log. */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:13PM +0800, Peter Xu wrote:
[...]
> +
> +static const TypeInfo migration_type = {
> +    .name = TYPE_MIGRATION,
> +    .parent = TYPE_DEVICE,

As TYPE_MIGRATION isn't really a device and we're using TYPE_DEVICE just
to reuse the global property system, maybe we should add a comment here
explaining that?

It would also be interesting to explain the TYPE_DEVICE features we do
not use.  e.g.: the device is not created using qdev_create(), it is
never attached to any bus, and it is never realized.

(But I don't think this should require a series respin.  The comment may
be sent as a follow-up patch.)

> +    .class_init = migration_class_init,
> +    .class_size = sizeof(MigrationClass),
> +    .instance_size = sizeof(MigrationState),
> +    .instance_init = migration_instance_init,
> +};
> +
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Peter Xu]

On Tue, Jun 27, 2017 at 11:47:18AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 27, 2017 at 12:10:13PM +0800, Peter Xu wrote:
> [...]
> > +
> > +static const TypeInfo migration_type = {
> > +    .name = TYPE_MIGRATION,
> > +    .parent = TYPE_DEVICE,
> 
> As TYPE_MIGRATION isn't really a device and we're using TYPE_DEVICE just
> to reuse the global property system, maybe we should add a comment here
> explaining that?
> 
> It would also be interesting to explain the TYPE_DEVICE features we do
> not use.  e.g.: the device is not created using qdev_create(), it is
> never attached to any bus, and it is never realized.
> 
> (But I don't think this should require a series respin.  The comment may
> be sent as a follow-up patch.)

Sure. Let me prepare another patch for it upon the series. Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:13PM +0800, Peter Xu 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.
> 
> Now we init the MigrationState struct statically in main() to make sure
> it's initialized after global properties are applied, since we'll use
> them during creation of the object.
> 
> No functional change at all.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

On 2017-06-27 06:10, Peter Xu 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.
> 
> Now we init the MigrationState struct statically in main() to make sure
> it's initialized after global properties are applied, since we'll use
> them during creation of the object.
> 
> No functional change at all.

Evidently not quite right because this breaks iotest 055.

Condensed test case:

$ ./qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 64M
Formatting 'foo.vmdk', fmt=vmdk size=67108864 compat6=off
hwversion=undefined subformat=streamOptimized
$ x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.vmdk
qemu-system-x86_64: ./migration/migration.c:114: migrate_get_current:
Assertion `current_migration' failed.
[1]    15453 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64
-drive if=none,file=foo.vmdk

(It just worked before this patch.)

Max

> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h |  1 +
>  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
>  migration/migration.h    | 19 ++++++++++++
>  vl.c                     |  6 ++++
>  4 files changed, 81 insertions(+), 23 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Peter Xu]

On Fri, Jun 30, 2017 at 04:18:56AM +0200, Max Reitz wrote:
> On 2017-06-27 06:10, Peter Xu 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.
> > 
> > Now we init the MigrationState struct statically in main() to make sure
> > it's initialized after global properties are applied, since we'll use
> > them during creation of the object.
> > 
> > No functional change at all.
> 
> Evidently not quite right because this breaks iotest 055.
> 
> Condensed test case:
> 
> $ ./qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 64M
> Formatting 'foo.vmdk', fmt=vmdk size=67108864 compat6=off
> hwversion=undefined subformat=streamOptimized
> $ x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.vmdk
> qemu-system-x86_64: ./migration/migration.c:114: migrate_get_current:
> Assertion `current_migration' failed.
> [1]    15453 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64
> -drive if=none,file=foo.vmdk
> 
> (It just worked before this patch.)

Sorry. Will have a look.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Peter Xu]

On Fri, Jun 30, 2017 at 11:03:21AM +0800, Peter Xu wrote:
> On Fri, Jun 30, 2017 at 04:18:56AM +0200, Max Reitz wrote:
> > On 2017-06-27 06:10, Peter Xu 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.
> > > 
> > > Now we init the MigrationState struct statically in main() to make sure
> > > it's initialized after global properties are applied, since we'll use
> > > them during creation of the object.
> > > 
> > > No functional change at all.
> > 
> > Evidently not quite right because this breaks iotest 055.
> > 
> > Condensed test case:
> > 
> > $ ./qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 64M
> > Formatting 'foo.vmdk', fmt=vmdk size=67108864 compat6=off
> > hwversion=undefined subformat=streamOptimized
> > $ x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.vmdk
> > qemu-system-x86_64: ./migration/migration.c:114: migrate_get_current:
> > Assertion `current_migration' failed.
> > [1]    15453 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64
> > -drive if=none,file=foo.vmdk
> > 
> > (It just worked before this patch.)
> 
> Sorry. Will have a look.

Hello, Max,

The assertion is caused by migrate_add_blocker() called before
initialization of migration object. I'll fix it.

But even with a fix (so I can pass 055 now), I still cannot pass some
of the other tests. Errors I got:

  https://pastebin.com/ACqbXAYd

I am not familiar with iotests. Is above usual? Looks like it still
includes 3 failures, and some output mismatch.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]

On 2017-06-30 09:11, Peter Xu wrote:
> On Fri, Jun 30, 2017 at 11:03:21AM +0800, Peter Xu wrote:
>> On Fri, Jun 30, 2017 at 04:18:56AM +0200, Max Reitz wrote:
>>> On 2017-06-27 06:10, Peter Xu 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.
>>>>
>>>> Now we init the MigrationState struct statically in main() to make sure
>>>> it's initialized after global properties are applied, since we'll use
>>>> them during creation of the object.
>>>>
>>>> No functional change at all.
>>>
>>> Evidently not quite right because this breaks iotest 055.
>>>
>>> Condensed test case:
>>>
>>> $ ./qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 64M
>>> Formatting 'foo.vmdk', fmt=vmdk size=67108864 compat6=off
>>> hwversion=undefined subformat=streamOptimized
>>> $ x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.vmdk
>>> qemu-system-x86_64: ./migration/migration.c:114: migrate_get_current:
>>> Assertion `current_migration' failed.
>>> [1]    15453 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64
>>> -drive if=none,file=foo.vmdk
>>>
>>> (It just worked before this patch.)
>>
>> Sorry. Will have a look.
> 
> Hello, Max,
> 
> The assertion is caused by migrate_add_blocker() called before
> initialization of migration object. I'll fix it.

Thanks!

> But even with a fix (so I can pass 055 now), I still cannot pass some
> of the other tests. Errors I got:
> 
>   https://pastebin.com/ACqbXAYd
> 
> I am not familiar with iotests. Is above usual? Looks like it still
> includes 3 failures, and some output mismatch.

Well, not usual. But 068 just is broken on master currently (Stefan has
sent "virtio: use ioeventfd in TCG and qtest mode" to fix it, and it's
part of his latest pull request). The failure in test 087 is because you
don't have aio=native enabled in your build, as the message says. :-)

I'm not sure about 118. Maybe the os.chmod() doesn't work as intended on
your machine...? Because it tries to open a read-only image as
read/write and wants to see it fail (which it doesn't in your case).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On 06/30/2017 07:33 AM, Max Reitz wrote:

>> The assertion is caused by migrate_add_blocker() called before
>> initialization of migration object. I'll fix it.
> 
> Thanks!
> 
>> But even with a fix (so I can pass 055 now), I still cannot pass some
>> of the other tests. Errors I got:
>>
>>   https://pastebin.com/ACqbXAYd
>>
>> I am not familiar with iotests. Is above usual? Looks like it still
>> includes 3 failures, and some output mismatch.
> 
> Well, not usual. But 068 just is broken on master currently (Stefan has
> sent "virtio: use ioeventfd in TCG and qtest mode" to fix it, and it's
> part of his latest pull request). The failure in test 087 is because you
> don't have aio=native enabled in your build, as the message says. :-)

We could obviously patch 087 to gracefully skip instead of fail when the
build doesn't support running it.

> 
> I'm not sure about 118. Maybe the os.chmod() doesn't work as intended on
> your machine...? Because it tries to open a read-only image as
> read/write and wants to see it fail (which it doesn't in your case).

Maybe a run-as-root issue, where root can write to the file in spite of
permissions?  Ideally, I'm reluctant to run testsuites as root without
good reason (or at least a good sandbox), for fear that a bug in the
testsuite will hose my system.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

On 2017-06-30 15:05, Eric Blake wrote:
> On 06/30/2017 07:33 AM, Max Reitz wrote:
> 
>>> The assertion is caused by migrate_add_blocker() called before
>>> initialization of migration object. I'll fix it.
>>
>> Thanks!
>>
>>> But even with a fix (so I can pass 055 now), I still cannot pass some
>>> of the other tests. Errors I got:
>>>
>>>   https://pastebin.com/ACqbXAYd
>>>
>>> I am not familiar with iotests. Is above usual? Looks like it still
>>> includes 3 failures, and some output mismatch.
>>
>> Well, not usual. But 068 just is broken on master currently (Stefan has
>> sent "virtio: use ioeventfd in TCG and qtest mode" to fix it, and it's
>> part of his latest pull request). The failure in test 087 is because you
>> don't have aio=native enabled in your build, as the message says. :-)
> 
> We could obviously patch 087 to gracefully skip instead of fail when the
> build doesn't support running it.
> 
>>
>> I'm not sure about 118. Maybe the os.chmod() doesn't work as intended on
>> your machine...? Because it tries to open a read-only image as
>> read/write and wants to see it fail (which it doesn't in your case).
> 
> Maybe a run-as-root issue, where root can write to the file in spite of
> permissions?

That's what I had in mind, too.

>               Ideally, I'm reluctant to run testsuites as root without
> good reason (or at least a good sandbox), for fear that a bug in the
> testsuite will hose my system.

I never do. :-)

There is one test which requires it, but well, that just never gets run
on my machine (and I don't feel very sorry about it).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v6 04/10] migration: let MigrationState be a qdev

[Peter Xu]

On Fri, Jun 30, 2017 at 03:57:23PM +0200, Max Reitz wrote:
> On 2017-06-30 15:05, Eric Blake wrote:
> > On 06/30/2017 07:33 AM, Max Reitz wrote:
> > 
> >>> The assertion is caused by migrate_add_blocker() called before
> >>> initialization of migration object. I'll fix it.
> >>
> >> Thanks!

Should be my thanks to you for reporting this. :)

> >>
> >>> But even with a fix (so I can pass 055 now), I still cannot pass some
> >>> of the other tests. Errors I got:
> >>>
> >>>   https://pastebin.com/ACqbXAYd
> >>>
> >>> I am not familiar with iotests. Is above usual? Looks like it still
> >>> includes 3 failures, and some output mismatch.
> >>
> >> Well, not usual. But 068 just is broken on master currently (Stefan has
> >> sent "virtio: use ioeventfd in TCG and qtest mode" to fix it, and it's
> >> part of his latest pull request). The failure in test 087 is because you
> >> don't have aio=native enabled in your build, as the message says. :-)
> > 
> > We could obviously patch 087 to gracefully skip instead of fail when the
> > build doesn't support running it.
> > 
> >>
> >> I'm not sure about 118. Maybe the os.chmod() doesn't work as intended on
> >> your machine...? Because it tries to open a read-only image as
> >> read/write and wants to see it fail (which it doesn't in your case).
> > 
> > Maybe a run-as-root issue, where root can write to the file in spite of
> > permissions?
> 
> That's what I had in mind, too.
> 
> >               Ideally, I'm reluctant to run testsuites as root without
> > good reason (or at least a good sandbox), for fear that a bug in the
> > testsuite will hose my system.
> 
> I never do. :-)

Good reason. I'll definitely switch to non-root even on my dev machine
next time, and also I think I'll also switch to use xfs for my rootfs
(I won't let you know that I'm using btrfs :).

> 
> There is one test which requires it, but well, that just never gets run
> on my machine (and I don't feel very sorry about it).

So looks like I didn't break the rest of the failed tests, good.

Then I'll send the fix soon. Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH v6 05/10] 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.

Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
for x86/power, and AccelClass.global_props for Xen.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c                |  1 -
 hw/ppc/spapr.c                   |  1 -
 hw/xen/xen-common.c              | 11 ++++++++++-
 include/hw/compat.h              |  4 ++++
 include/migration/global_state.h |  1 -
 migration/global_state.c         |  9 ++-------
 migration/migration.c            |  7 +++++++
 migration/migration.h            |  6 ++++++
 8 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 46a2bc4..3b51297 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -318,7 +318,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 ede5167..f07d40a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3581,7 +3581,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/hw/xen/xen-common.c b/hw/xen/xen-common.c
index d3fa705..459e6ab 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -139,19 +139,28 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    global_state_set_optional();
     savevm_skip_configuration();
     savevm_skip_section_footers();
 
     return 0;
 }
 
+static GlobalProperty xen_compat_props[] = {
+    {
+        .driver = "migration",
+        .property = "store-global-state",
+        .value = "off",
+    },
+    { /* end of list */ },
+};
+
 static void xen_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "Xen";
     ac->init_machine = xen_init;
     ac->allowed = &xen_allowed;
+    ac->global_props = xen_compat_props;
 }
 
 #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 26cd585..a506a74 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -181,6 +181,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 \
diff --git a/include/migration/global_state.h b/include/migration/global_state.h
index 90faea7..d307de8 100644
--- a/include/migration/global_state.h
+++ b/include/migration/global_state.h
@@ -16,7 +16,6 @@
 #include "sysemu/sysemu.h"
 
 void register_global_state(void);
-void global_state_set_optional(void);
 int global_state_store(void);
 void global_state_store_running(void);
 bool global_state_received(void);
diff --git a/migration/global_state.c b/migration/global_state.c
index f792cf5..dcbbcb2 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -15,12 +15,12 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/util.h"
+#include "migration.h"
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
 typedef struct {
-    bool optional;
     uint32_t size;
     uint8_t runstate[100];
     RunState state;
@@ -57,11 +57,6 @@ RunState global_state_get_runstate(void)
     return global_state.state;
 }
 
-void global_state_set_optional(void)
-{
-    global_state.optional = true;
-}
-
 static bool global_state_needed(void *opaque)
 {
     GlobalState *s = opaque;
@@ -69,7 +64,7 @@ static bool global_state_needed(void *opaque)
 
     /* If it is not optional, it is mandatory */
 
-    if (s->optional == false) {
+    if (migrate_get_current()->store_global_state) {
         return true;
     }
 
diff --git a/migration/migration.c b/migration/migration.c
index 2c25927..221b22c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1976,11 +1976,18 @@ 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->user_creatable = false;
+    dc->props = migration_properties;
 }
 
 static void migration_instance_init(Object *obj)
diff --git a/migration/migration.h b/migration/migration.h
index 3fca364..4b898e9 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -133,6 +133,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);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 05/10] migration: move global_state.optional out

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:14PM +0800, Peter Xu wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
> 
> Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
> for x86/power, and AccelClass.global_props for Xen.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v6 06/10] 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.

Hmm, now vl.c has no way to access migrate_get_current(). Export a
function for it to setup only_migratable.

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

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 2d36cf5..6ac3307 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -55,4 +55,6 @@ bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
+void migration_only_migratable_set(void);
+
 #endif
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 221b22c..67f9e68 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -115,6 +115,11 @@ MigrationState *migrate_get_current(void)
     return current_migration;
 }
 
+void migration_only_migratable_set(void)
+{
+    migrate_get_current()->only_migratable = true;
+}
+
 MigrationIncomingState *migration_incoming_get_current(void)
 {
     static bool once;
@@ -986,7 +991,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: ");
@@ -1979,6 +1984,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/migration.h b/migration/migration.h
index 4b898e9..34377dd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -139,6 +139,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/migration/savevm.c b/migration/savevm.c
index c7a49c9..1499cd3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2336,7 +2336,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 f0a0515..36ff3f4 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;
 
@@ -3953,7 +3952,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"
+                 */
+                migration_only_migratable_set();
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On 06/26/2017 11:10 PM, Peter Xu wrote:
> 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

Does command line introspection work to see that this new spelling is
available?  If not, what else can we do to let libvirt learn that the
new form is preferred?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote:
> On 06/26/2017 11:10 PM, Peter Xu wrote:
> > 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
> 
> Does command line introspection work to see that this new spelling is
> available?  If not, what else can we do to let libvirt learn that the
> new form is preferred?

"only-migratable" will appear on the output of
"device-list-properties typename=migration".  But although I think
implementing this using global properties internally is nice, I'm unsure
about making -global the recommended interface to configure this.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Peter Xu]

On Tue, Jun 27, 2017 at 10:36:44AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote:
> > On 06/26/2017 11:10 PM, Peter Xu wrote:
> > > 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
> > 
> > Does command line introspection work to see that this new spelling is
> > available?  If not, what else can we do to let libvirt learn that the
> > new form is preferred?
> 
> "only-migratable" will appear on the output of
> "device-list-properties typename=migration".  But although I think
> implementing this using global properties internally is nice, I'm unsure
> about making -global the recommended interface to configure this.

Could I ask why it may not good to use "-global" to configure it?

Actually if we can merge this series, there is planned work to expose
more migration parameters to the user, like: migration.postcopy,
migration.compression, migration.speed, etc. So before I post similar
patches like this to export more things, I would like to know whether
there is possible issue with it in general.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Eduardo Habkost]

On Wed, Jun 28, 2017 at 02:54:34PM +0800, Peter Xu wrote:
> On Tue, Jun 27, 2017 at 10:36:44AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote:
> > > On 06/26/2017 11:10 PM, Peter Xu wrote:
> > > > 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
> > > 
> > > Does command line introspection work to see that this new spelling is
> > > available?  If not, what else can we do to let libvirt learn that the
> > > new form is preferred?
> > 
> > "only-migratable" will appear on the output of
> > "device-list-properties typename=migration".  But although I think
> > implementing this using global properties internally is nice, I'm unsure
> > about making -global the recommended interface to configure this.
> 
> Could I ask why it may not good to use "-global" to configure it?
> 
> Actually if we can merge this series, there is planned work to expose
> more migration parameters to the user, like: migration.postcopy,
> migration.compression, migration.speed, etc. So before I post similar
> patches like this to export more things, I would like to know whether
> there is possible issue with it in general.  Thanks,

I don't see any obvious issue with "-global migration.FOO=BAR", but I
just want to be 100% sure before making it the recommended interface to
configure migration options.  If there are no objections, I'm OK with
that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:15PM +0800, Peter Xu wrote:
> 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.
> 
> Hmm, now vl.c has no way to access migrate_get_current(). Export a
> function for it to setup only_migratable.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
[...]
> +void migration_only_migratable_set(void)
> +{
> +    migrate_get_current()->only_migratable = true;
> +}
> +
[...]
> @@ -3953,7 +3952,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"
> +                 */
> +                migration_only_migratable_set();

This triggers a premature call to migrate_get_current():

  $ qemu-system-x86_64 -only-migratable
  qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/migration/migration.c:127: migrate_get_current: Assertion `current_migration' failed.
  Aborted (core dumped)

What about just doing:

    qemu_global_option("migration.only-migratable=true");

>                  break;
>              case QEMU_OPTION_nodefaults:
>                  has_defaults = 0;
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 06/10] migration: move only_migratable to MigrationState

[Peter Xu]

On Wed, Jun 28, 2017 at 04:13:57PM -0300, Eduardo Habkost wrote:
> On Tue, Jun 27, 2017 at 12:10:15PM +0800, Peter Xu wrote:
> > 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.
> > 
> > Hmm, now vl.c has no way to access migrate_get_current(). Export a
> > function for it to setup only_migratable.
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> [...]
> > +void migration_only_migratable_set(void)
> > +{
> > +    migrate_get_current()->only_migratable = true;
> > +}
> > +
> [...]
> > @@ -3953,7 +3952,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"
> > +                 */
> > +                migration_only_migratable_set();
> 
> This triggers a premature call to migrate_get_current():
> 
>   $ qemu-system-x86_64 -only-migratable
>   qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/migration/migration.c:127: migrate_get_current: Assertion `current_migration' failed.
>   Aborted (core dumped)
> 
> What about just doing:
> 
>     qemu_global_option("migration.only-migratable=true");

Oops... Definitely. Thanks for pointing this out! I'll see whether I
should respin or fix on top.

-- 
Peter Xu

[Qemu-devel] [PATCH v6 07/10] migration: move skip_configuration out

[Peter Xu]

It was in SaveState but now moved to MigrationState altogether, reverted
its meaning, then renamed to "send_configuration". Again, using
HW_COMPAT_2_3 for old PC/SPAPR machines, and accel_register_prop() for
xen_init().

Removing savevm_skip_configuration().

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c        |  1 -
 hw/ppc/spapr.c           |  1 -
 hw/xen/xen-common.c      |  6 +++++-
 include/hw/compat.h      |  4 ++++
 include/migration/misc.h |  1 -
 migration/migration.c    |  2 ++
 migration/migration.h    |  3 +++
 migration/savevm.c       | 15 ++++-----------
 8 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3b51297..488fc0a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -318,7 +318,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 f07d40a..8429187 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3581,7 +3581,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/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 459e6ab..1f3688d 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -139,7 +139,6 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    savevm_skip_configuration();
     savevm_skip_section_footers();
 
     return 0;
@@ -151,6 +150,11 @@ static GlobalProperty xen_compat_props[] = {
         .property = "store-global-state",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-configuration",
+        .value = "off",
+    },
     { /* end of list */ },
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index a506a74..1a3fd94 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -183,6 +183,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-configuration",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 6ac3307..f30db4d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -42,7 +42,6 @@ int64_t self_announce_delay(int round)
 
 void dump_vmstate_json_to_file(FILE *out_fp);
 void savevm_skip_section_footers(void);
-void savevm_skip_configuration(void);
 
 /* migration/migration.c */
 void migration_object_init(void);
diff --git a/migration/migration.c b/migration/migration.c
index 67f9e68..414e14d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1985,6 +1985,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("send-configuration", MigrationState,
+                     send_configuration, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 34377dd..4d4ea0d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -142,6 +142,9 @@ struct MigrationState
 
     /* Whether the VM is only allowing for migratable devices */
     bool only_migratable;
+
+    /* Whether we send QEMU_VM_CONFIGURATION during migration */
+    bool send_configuration;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index 1499cd3..0a8c61f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -287,7 +287,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;
@@ -296,15 +295,8 @@ 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;
-}
-
-
 static void configuration_pre_save(void *opaque)
 {
     SaveState *state = opaque;
@@ -970,11 +962,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()->send_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)
@@ -1995,7 +1987,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (migrate_get_current()->send_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 v6 07/10] migration: move skip_configuration out

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:16PM +0800, Peter Xu wrote:
> It was in SaveState but now moved to MigrationState altogether, reverted
> its meaning, then renamed to "send_configuration". Again, using
> HW_COMPAT_2_3 for old PC/SPAPR machines, and accel_register_prop() for
> xen_init().
> 
> Removing savevm_skip_configuration().
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v6 08/10] migration: move skip_section_footers

[Peter Xu]

Move it into MigrationState, revert its meaning and renaming it to
send_section_footer, with a property bound to it. Same trick is played
like previous patches.

Removing savevm_skip_section_footers().

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 488fc0a..22dbef6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -314,7 +314,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 8429187..ea44358 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,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/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 1f3688d..632a938 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -138,9 +138,6 @@ static int xen_init(MachineState *ms)
         return -1;
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
-
-    savevm_skip_section_footers();
-
     return 0;
 }
 
@@ -155,6 +152,11 @@ static GlobalProperty xen_compat_props[] = {
         .property = "send-configuration",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-section-footer",
+        .value = "off",
+    },
     { /* end of list */ },
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 1a3fd94..08f3600 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -187,6 +187,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-section-footer",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index f30db4d..854c28d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -41,7 +41,6 @@ int64_t self_announce_delay(int round)
 /* migration/savevm.c */
 
 void dump_vmstate_json_to_file(FILE *out_fp);
-void savevm_skip_section_footers(void);
 
 /* migration/migration.c */
 void migration_object_init(void);
diff --git a/migration/migration.c b/migration/migration.c
index 414e14d..96c6412 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1987,6 +1987,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("send-configuration", MigrationState,
                      send_configuration, true),
+    DEFINE_PROP_BOOL("send-section-footer", MigrationState,
+                     send_section_footer, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 4d4ea0d..994b017 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -145,6 +145,8 @@ struct MigrationState
 
     /* Whether we send QEMU_VM_CONFIGURATION during migration */
     bool send_configuration;
+    /* Whether we send section footer during migration */
+    bool send_section_footer;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index 0a8c61f..7b39fb9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -62,8 +62,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 */
@@ -761,11 +759,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
     vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
-void savevm_skip_section_footers(void)
-{
-    skip_section_footers = true;
-}
-
 /*
  * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
  */
@@ -793,7 +786,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()->send_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
     }
@@ -1802,7 +1795,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()->send_section_footer) {
         /* No footer to check */
         return true;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 08/10] migration: move skip_section_footers

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:17PM +0800, Peter Xu wrote:
> Move it into MigrationState, revert its meaning and renaming it to
> send_section_footer, with a property bound to it. Same trick is played
> like previous patches.
> 
> Removing savevm_skip_section_footers().
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Peter Xu]

These two parameters:

- MachineState::enforce_config_section
- MigrationState::send_configuration

are playing similar role here. This patch merges the first one into
second, then we'll have a single place to reference whether we need to
send the configuration section.

I didn't remove the MachineState.enforce_config_section field since when
applying that machine property (in machine_set_property()) we haven't
yet initialized global properties and migration object. Then, it's
still not easy to pass that boolean to MigrationState at such an early
time.

A natural benefit for current patch is that now we kept the meaning of
"enforce-config-section" since it'll still have the highest
priority (that's what "enforce" mean I guess).

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 12 ++++++++++++
 migration/savevm.c    | 12 ++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 96c6412..e7e6cf3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -42,6 +42,7 @@
 #include "exec/target_page.h"
 #include "io/channel-buffer.h"
 #include "migration/colo.h"
+#include "hw/boards.h"
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
 
@@ -102,9 +103,20 @@ static MigrationState *current_migration;
 
 void migration_object_init(void)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
+
     /* This can only be called once. */
     assert(!current_migration);
     current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+
+    /*
+     * We cannot really do this in migration_instance_init() since at
+     * that time global properties are not yet applied, then this
+     * value will be definitely replaced by something else.
+     */
+    if (ms->enforce_config_section) {
+        current_migration->send_configuration = true;
+    }
 }
 
 /* For outgoing */
diff --git a/migration/savevm.c b/migration/savevm.c
index 7b39fb9..be3f885 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -943,20 +943,13 @@ bool qemu_savevm_state_blocked(Error **errp)
     return false;
 }
 
-static bool enforce_config_section(void)
-{
-    MachineState *machine = MACHINE(qdev_get_machine());
-    return machine->enforce_config_section;
-}
-
 void qemu_savevm_state_header(QEMUFile *f)
 {
     trace_savevm_state_header();
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (migrate_get_current()->send_configuration ||
-        enforce_config_section()) {
+    if (migrate_get_current()->send_configuration) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
@@ -1980,8 +1973,7 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (migrate_get_current()->send_configuration ||
-        enforce_config_section()) {
+    if (migrate_get_current()->send_configuration) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:18PM +0800, Peter Xu wrote:
> These two parameters:
> 
> - MachineState::enforce_config_section
> - MigrationState::send_configuration
> 
> are playing similar role here. This patch merges the first one into
> second, then we'll have a single place to reference whether we need to
> send the configuration section.
> 
> I didn't remove the MachineState.enforce_config_section field since when
> applying that machine property (in machine_set_property()) we haven't
> yet initialized global properties and migration object. Then, it's
> still not easy to pass that boolean to MigrationState at such an early
> time.
> 
> A natural benefit for current patch is that now we kept the meaning of
> "enforce-config-section" since it'll still have the highest
> priority (that's what "enforce" mean I guess).
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 12 ++++++++++++
>  migration/savevm.c    | 12 ++----------
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 96c6412..e7e6cf3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -42,6 +42,7 @@
>  #include "exec/target_page.h"
>  #include "io/channel-buffer.h"
>  #include "migration/colo.h"
> +#include "hw/boards.h"
>  
>  #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
>  
> @@ -102,9 +103,20 @@ static MigrationState *current_migration;
>  
>  void migration_object_init(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +
>      /* This can only be called once. */
>      assert(!current_migration);
>      current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> +
> +    /*
> +     * We cannot really do this in migration_instance_init() since at
> +     * that time global properties are not yet applied, then this
> +     * value will be definitely replaced by something else.
> +     */
> +    if (ms->enforce_config_section) {
> +        current_migration->send_configuration = true;
> +    }

So, this is a case where a user-provided config option (-machine
enforce-config-section) should trigger a different default in another
class (migration.send-configuration).

Also, the new default triggered by -machine has a very specific
priority:

* AccelClass::global_props must not override "-machine enforce-config-section=on"
* MachineClass::compat_props must not override
  "-machine enforce-config-section=on"

We must also decide in advance what should be result of:
* "-machine enforce-config-section=on -global migration.send-configuration=off"
* "-machine enforce-config-section=off -global migration.send-configuration=on"
* "-global migration.send-configuration=off -machine enforce-config-section=off"
* "-global migration.send-configuration=on -machine enforce-config-section=on"

I'm not sure what we should decide about these 4 cases above, but I
believe it would be safer to encode that decision at the same place we
handle the priority between accel/machine/user globals:
register_global_properties() at vl.c.


Or maybe this extra complexity is a sign that we shouldn't try to add
extra magic to make -machine affect the "migration" object properties,
and keep the existing machine->enforce_config_section check in the
migration code?  I'm not sure.

>  }
>  
>  /* For outgoing */
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7b39fb9..be3f885 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -943,20 +943,13 @@ bool qemu_savevm_state_blocked(Error **errp)
>      return false;
>  }
>  
> -static bool enforce_config_section(void)
> -{
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    return machine->enforce_config_section;
> -}
> -
>  void qemu_savevm_state_header(QEMUFile *f)
>  {
>      trace_savevm_state_header();
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>      qemu_put_be32(f, QEMU_VM_FILE_VERSION);
>  
> -    if (migrate_get_current()->send_configuration ||
> -        enforce_config_section()) {
> +    if (migrate_get_current()->send_configuration) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
>          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>      }
> @@ -1980,8 +1973,7 @@ int qemu_loadvm_state(QEMUFile *f)
>          return -ENOTSUP;
>      }
>  
> -    if (migrate_get_current()->send_configuration ||
> -        enforce_config_section()) {
> +    if (migrate_get_current()->send_configuration) {
>          if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
>              error_report("Configuration section missing");
>              return -EINVAL;
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Juan Quintela]

Eduardo Habkost <ehabkost@redhat.com> wrote:

>
> So, this is a case where a user-provided config option (-machine
> enforce-config-section) should trigger a different default in another
> class (migration.send-configuration).
>
> Also, the new default triggered by -machine has a very specific
> priority:
>
> * AccelClass::global_props must not override "-machine enforce-config-section=on"
> * MachineClass::compat_props must not override
>   "-machine enforce-config-section=on"
>
> We must also decide in advance what should be result of:
> * "-machine enforce-config-section=on -global migration.send-configuration=off"
> * "-machine enforce-config-section=off -global migration.send-configuration=on"
> * "-global migration.send-configuration=off -machine enforce-config-section=off"
> * "-global migration.send-configuration=on -machine enforce-config-section=on"

BOOM!!!!!

We use old configuration or new one.


>
> I'm not sure what we should decide about these 4 cases above, but I
> believe it would be safer to encode that decision at the same place we
> handle the priority between accel/machine/user globals:
> register_global_properties() at vl.c.
>
>
> Or maybe this extra complexity is a sign that we shouldn't try to add
> extra magic to make -machine affect the "migration" object properties,
> and keep the existing machine->enforce_config_section check in the
> migration code?  I'm not sure.

Not sure there either.  I preffer doing it in a single place, but I am
not the expert here.

Later, Juan.

Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Peter Xu]

On Thu, Jun 29, 2017 at 12:42:56AM +0200, Juan Quintela wrote:
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> >
> > So, this is a case where a user-provided config option (-machine
> > enforce-config-section) should trigger a different default in another
> > class (migration.send-configuration).
> >
> > Also, the new default triggered by -machine has a very specific
> > priority:
> >
> > * AccelClass::global_props must not override "-machine enforce-config-section=on"
> > * MachineClass::compat_props must not override
> >   "-machine enforce-config-section=on"
> >
> > We must also decide in advance what should be result of:
> > * "-machine enforce-config-section=on -global migration.send-configuration=off"
> > * "-machine enforce-config-section=off -global migration.send-configuration=on"
> > * "-global migration.send-configuration=off -machine enforce-config-section=off"
> > * "-global migration.send-configuration=on -machine enforce-config-section=on"

Yes, this is considered before this patch: currently
enforce-config-section will have the highest priority in case if
someone used both of the old & new parameters for it (considering
"enforce-config-section" has the word "enforce" inside, it makes some
sense). While...

> 
> BOOM!!!!!
> 
> We use old configuration or new one.

... I agree more with Juan here, that user should not really specify
these two parameters at the same time. If the user knows the new
parameter, he/she should know that the new one is obsoleting the old
one. And since even for that case this patch can handle it well (will
take -M param), I think it's okay.

> 
> >
> > I'm not sure what we should decide about these 4 cases above, but I
> > believe it would be safer to encode that decision at the same place we
> > handle the priority between accel/machine/user globals:
> > register_global_properties() at vl.c.
> >
> >
> > Or maybe this extra complexity is a sign that we shouldn't try to add
> > extra magic to make -machine affect the "migration" object properties,
> > and keep the existing machine->enforce_config_section check in the
> > migration code?  I'm not sure.
> 
> Not sure there either.  I preffer doing it in a single place, but I am
> not the expert here.

IMHO it is not necessary to introduce such a thing in
register_global_properties(). AFAIU this is the only place where one
machine property can collapse with a global property? And it currently
only happens in migration codes. Actually it is well ordered, since we
init the migration object after register_global_properties(), so
everthing should possibly be fine. Introducing framework-level thing
for this may only make things more complicated imho.

After all we can remove all these one day when we can obsolete the
"enforce-config-section" parameter (maybe we should add one OBSOLETE
warning when the -M parameter is used).  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Eduardo Habkost]

On Thu, Jun 29, 2017 at 11:00:13AM +0800, Peter Xu wrote:
> On Thu, Jun 29, 2017 at 12:42:56AM +0200, Juan Quintela wrote:
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > >
> > > So, this is a case where a user-provided config option (-machine
> > > enforce-config-section) should trigger a different default in another
> > > class (migration.send-configuration).
> > >
> > > Also, the new default triggered by -machine has a very specific
> > > priority:
> > >
> > > * AccelClass::global_props must not override "-machine enforce-config-section=on"
> > > * MachineClass::compat_props must not override
> > >   "-machine enforce-config-section=on"
> > >
> > > We must also decide in advance what should be result of:
> > > * "-machine enforce-config-section=on -global migration.send-configuration=off"
> > > * "-machine enforce-config-section=off -global migration.send-configuration=on"
> > > * "-global migration.send-configuration=off -machine enforce-config-section=off"
> > > * "-global migration.send-configuration=on -machine enforce-config-section=on"
> 
> Yes, this is considered before this patch: currently
> enforce-config-section will have the highest priority in case if
> someone used both of the old & new parameters for it (considering
> "enforce-config-section" has the word "enforce" inside, it makes some
> sense). While...
> 
> > 
> > BOOM!!!!!
> > 
> > We use old configuration or new one.
> 
> ... I agree more with Juan here, that user should not really specify
> these two parameters at the same time. If the user knows the new
> parameter, he/she should know that the new one is obsoleting the old
> one. And since even for that case this patch can handle it well (will
> take -M param), I think it's okay.

If that's the intended result, it's OK to me.  But I think the
relationship between enforce-config-section and
migration.send-configuration should be documented in qemu-options.hx.
And considering that this could break silently in future code
refactoring, an automated test would be interesting (but not critical,
as setting options contradicting each other is not a common scenario).


> 
> > 
> > >
> > > I'm not sure what we should decide about these 4 cases above, but I
> > > believe it would be safer to encode that decision at the same place we
> > > handle the priority between accel/machine/user globals:
> > > register_global_properties() at vl.c.
> > >
> > >
> > > Or maybe this extra complexity is a sign that we shouldn't try to add
> > > extra magic to make -machine affect the "migration" object properties,
> > > and keep the existing machine->enforce_config_section check in the
> > > migration code?  I'm not sure.
> > 
> > Not sure there either.  I preffer doing it in a single place, but I am
> > not the expert here.
> 
> IMHO it is not necessary to introduce such a thing in
> register_global_properties(). AFAIU this is the only place where one
> machine property can collapse with a global property? And it currently
> only happens in migration codes. Actually it is well ordered, since we
> init the migration object after register_global_properties(), so
> everthing should possibly be fine. Introducing framework-level thing
> for this may only make things more complicated imho.

True.  Considering we need to keep the "overrides everything else"
semantics of enforce-config-section, your approach is not bad.

> 
> After all we can remove all these one day when we can obsolete the
> "enforce-config-section" parameter (maybe we should add one OBSOLETE
> warning when the -M parameter is used).  Thanks,

I don't think we need a warning, but a documentation update is
important, IMO.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Peter Xu]

On Thu, Jun 29, 2017 at 04:18:49PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 29, 2017 at 11:00:13AM +0800, Peter Xu wrote:
> > On Thu, Jun 29, 2017 at 12:42:56AM +0200, Juan Quintela wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > >
> > > > So, this is a case where a user-provided config option (-machine
> > > > enforce-config-section) should trigger a different default in another
> > > > class (migration.send-configuration).
> > > >
> > > > Also, the new default triggered by -machine has a very specific
> > > > priority:
> > > >
> > > > * AccelClass::global_props must not override "-machine enforce-config-section=on"
> > > > * MachineClass::compat_props must not override
> > > >   "-machine enforce-config-section=on"
> > > >
> > > > We must also decide in advance what should be result of:
> > > > * "-machine enforce-config-section=on -global migration.send-configuration=off"
> > > > * "-machine enforce-config-section=off -global migration.send-configuration=on"
> > > > * "-global migration.send-configuration=off -machine enforce-config-section=off"
> > > > * "-global migration.send-configuration=on -machine enforce-config-section=on"
> > 
> > Yes, this is considered before this patch: currently
> > enforce-config-section will have the highest priority in case if
> > someone used both of the old & new parameters for it (considering
> > "enforce-config-section" has the word "enforce" inside, it makes some
> > sense). While...
> > 
> > > 
> > > BOOM!!!!!
> > > 
> > > We use old configuration or new one.
> > 
> > ... I agree more with Juan here, that user should not really specify
> > these two parameters at the same time. If the user knows the new
> > parameter, he/she should know that the new one is obsoleting the old
> > one. And since even for that case this patch can handle it well (will
> > take -M param), I think it's okay.
> 
> If that's the intended result, it's OK to me.  But I think the
> relationship between enforce-config-section and
> migration.send-configuration should be documented in qemu-options.hx.
> And considering that this could break silently in future code
> refactoring, an automated test would be interesting (but not critical,
> as setting options contradicting each other is not a common scenario).
> 
> 
> > 
> > > 
> > > >
> > > > I'm not sure what we should decide about these 4 cases above, but I
> > > > believe it would be safer to encode that decision at the same place we
> > > > handle the priority between accel/machine/user globals:
> > > > register_global_properties() at vl.c.
> > > >
> > > >
> > > > Or maybe this extra complexity is a sign that we shouldn't try to add
> > > > extra magic to make -machine affect the "migration" object properties,
> > > > and keep the existing machine->enforce_config_section check in the
> > > > migration code?  I'm not sure.
> > > 
> > > Not sure there either.  I preffer doing it in a single place, but I am
> > > not the expert here.
> > 
> > IMHO it is not necessary to introduce such a thing in
> > register_global_properties(). AFAIU this is the only place where one
> > machine property can collapse with a global property? And it currently
> > only happens in migration codes. Actually it is well ordered, since we
> > init the migration object after register_global_properties(), so
> > everthing should possibly be fine. Introducing framework-level thing
> > for this may only make things more complicated imho.
> 
> True.  Considering we need to keep the "overrides everything else"
> semantics of enforce-config-section, your approach is not bad.
> 
> > 
> > After all we can remove all these one day when we can obsolete the
> > "enforce-config-section" parameter (maybe we should add one OBSOLETE
> > warning when the -M parameter is used).  Thanks,
> 
> I don't think we need a warning, but a documentation update is
> important, IMO.

Since the pull request has been merged already, I'll add some
documentation to qemu-options.hx for this in separate patch (and I'll
temporarily skip the unit test since I really think we should obsolete
the old parameter one day). Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH v6 10/10] migration: hmp: dump globals

[Peter Xu]

Now we have some globals that can be configured for migration. Dump them
in HMP info migration for better debugging.

(we can also use this to monitor whether COMPAT fields are applied
correctly on compatible machines)

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hmp.c                    |  3 +++
 include/migration/misc.h |  1 +
 migration/migration.c    | 11 +++++++++++
 3 files changed, 15 insertions(+)

diff --git a/hmp.c b/hmp.c
index 8c72c58..4c41cac 100644
--- a/hmp.c
+++ b/hmp.c
@@ -43,6 +43,7 @@
 #include "exec/ramlist.h"
 #include "hw/intc/intc.h"
 #include "migration/snapshot.h"
+#include "migration/misc.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -164,6 +165,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
     info = qmp_query_migrate(NULL);
     caps = qmp_query_migrate_capabilities(NULL);
 
+    migration_global_dump(mon);
+
     /* do not display parameters during setup */
     if (info->has_status && caps) {
         monitor_printf(mon, "capabilities: ");
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 854c28d..2255121 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -54,5 +54,6 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_only_migratable_set(void);
+void migration_global_dump(Monitor *mon);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index e7e6cf3..b1b0825 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -43,6 +43,7 @@
 #include "io/channel-buffer.h"
 #include "migration/colo.h"
 #include "hw/boards.h"
+#include "monitor/monitor.h"
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
 
@@ -1993,6 +1994,16 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+void migration_global_dump(Monitor *mon)
+{
+    MigrationState *ms = migrate_get_current();
+
+    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
+                   "send-configuration=%d, send-section-footer=%d\n",
+                   ms->store_global_state, ms->only_migratable,
+                   ms->send_configuration, ms->send_section_footer);
+}
+
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 10/10] migration: hmp: dump globals

[Eduardo Habkost]

On Tue, Jun 27, 2017 at 12:10:19PM +0800, Peter Xu wrote:
> Now we have some globals that can be configured for migration. Dump them
> in HMP info migration for better debugging.
> 
> (we can also use this to monitor whether COMPAT fields are applied
> correctly on compatible machines)
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
[...]
> +void migration_global_dump(Monitor *mon)
> +{
> +    MigrationState *ms = migrate_get_current();
> +
> +    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
> +                   "send-configuration=%d, send-section-footer=%d\n",
> +                   ms->store_global_state, ms->only_migratable,
> +                   ms->send_configuration, ms->send_section_footer);

I wouldn't call them "globals", I would just call them "properties" (or
maybe just "settings").  They happen to be affected by global properties
if the accel/machine/user sets them, but they are just plain old QOM
properties.


> +}
> +
>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 10/10] migration: hmp: dump globals

[Peter Xu]

On Wed, Jun 28, 2017 at 02:12:44PM -0300, Eduardo Habkost wrote:
> On Tue, Jun 27, 2017 at 12:10:19PM +0800, Peter Xu wrote:
> > Now we have some globals that can be configured for migration. Dump them
> > in HMP info migration for better debugging.
> > 
> > (we can also use this to monitor whether COMPAT fields are applied
> > correctly on compatible machines)
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> [...]
> > +void migration_global_dump(Monitor *mon)
> > +{
> > +    MigrationState *ms = migrate_get_current();
> > +
> > +    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
> > +                   "send-configuration=%d, send-section-footer=%d\n",
> > +                   ms->store_global_state, ms->only_migratable,
> > +                   ms->send_configuration, ms->send_section_footer);
> 
> I wouldn't call them "globals", I would just call them "properties" (or
> maybe just "settings").  They happen to be affected by global properties
> if the accel/machine/user sets them, but they are just plain old QOM
> properties.

Yes, "properties" seems better. Considering that the patch is already
merged, I am just afraid it's too trivial to send a single patch to
fix this, so I'll just temporarily skip this one. Anyway, please let
me know if you still want me to do this, I'd be glad to. Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v6 11/10] migration: add comment for TYPE_MIGRATE

[Peter Xu]

It'll be strange that the migration object inherits TYPE_DEVICE. Add
some explanations to it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index b1b0825..2398a9d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2046,6 +2046,14 @@ static void migration_instance_init(Object *obj)
 
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
+    /*
+     * NOTE: "migration" itself is not really a device. We used
+     * TYPE_DEVICE here only to leverage some existing QDev features
+     * like "-global" properties, and HW_COMPAT_* fields (which are
+     * finally applied as global properties as well). If one day the
+     * global property feature can be migrated from QDev to QObject in
+     * general, then we can switch to QObject as well.
+     */
     .parent = TYPE_DEVICE,
     .class_init = migration_class_init,
     .class_size = sizeof(MigrationClass),
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 11/10] migration: add comment for TYPE_MIGRATE

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It'll be strange that the migration object inherits TYPE_DEVICE. Add
> some explanations to it.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b1b0825..2398a9d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2046,6 +2046,14 @@ static void migration_instance_init(Object *obj)
>  
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
> +    /*
> +     * NOTE: "migration" itself is not really a device. We used
> +     * TYPE_DEVICE here only to leverage some existing QDev features
> +     * like "-global" properties, and HW_COMPAT_* fields (which are
> +     * finally applied as global properties as well). If one day the
> +     * global property feature can be migrated from QDev to QObject in
> +     * general, then we can switch to QObject as well.
> +     */
>      .parent = TYPE_DEVICE,
>      .class_init = migration_class_init,
>      .class_size = sizeof(MigrationClass),

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

Re: [Qemu-devel] [PATCH v6 11/10] migration: add comment for TYPE_MIGRATE

[Eduardo Habkost]

On Wed, Jun 28, 2017 at 03:15:44PM +0800, Peter Xu wrote:
> It'll be strange that the migration object inherits TYPE_DEVICE. Add
> some explanations to it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b1b0825..2398a9d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2046,6 +2046,14 @@ static void migration_instance_init(Object *obj)
>  
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
> +    /*
> +     * NOTE: "migration" itself is not really a device. We used

I suggest extending this as:

  TYPE_MIGRATION is not really a device, as the object is not created
  using qdev_create(), it is not attached to the qdev device tree, and
  it is never realized.


> +     * TYPE_DEVICE here only to leverage some existing QDev features
> +     * like "-global" properties, and HW_COMPAT_* fields (which are
> +     * finally applied as global properties as well). If one day the
> +     * global property feature can be migrated from QDev to QObject in
> +     * general, then we can switch to QObject as well.
> +     */

QOM objects (qom/object.h) are not QObject (qapi/qmp/qobject.h).  People
often confuse them with each other.  (It probably doesn't help that both
are described as "QEMU Object Model" at their headers.)

I suggest rewriting it as:

  If one day we allow non-device QOM objects to use the global property
  system, we can switch this from TYPE_DEVICE to TYPE_OBJECT."


>      .parent = TYPE_DEVICE,
>      .class_init = migration_class_init,
>      .class_size = sizeof(MigrationClass),
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 11/10] migration: add comment for TYPE_MIGRATE

[Peter Xu]

On Wed, Jun 28, 2017 at 12:37:04PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 28, 2017 at 03:15:44PM +0800, Peter Xu wrote:
> > It'll be strange that the migration object inherits TYPE_DEVICE. Add
> > some explanations to it.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b1b0825..2398a9d 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2046,6 +2046,14 @@ static void migration_instance_init(Object *obj)
> >  
> >  static const TypeInfo migration_type = {
> >      .name = TYPE_MIGRATION,
> > +    /*
> > +     * NOTE: "migration" itself is not really a device. We used
> 
> I suggest extending this as:
> 
>   TYPE_MIGRATION is not really a device, as the object is not created
>   using qdev_create(), it is not attached to the qdev device tree, and
>   it is never realized.
> 
> 
> > +     * TYPE_DEVICE here only to leverage some existing QDev features
> > +     * like "-global" properties, and HW_COMPAT_* fields (which are
> > +     * finally applied as global properties as well). If one day the
> > +     * global property feature can be migrated from QDev to QObject in
> > +     * general, then we can switch to QObject as well.
> > +     */
> 
> QOM objects (qom/object.h) are not QObject (qapi/qmp/qobject.h).  People
> often confuse them with each other.  (It probably doesn't help that both
> are described as "QEMU Object Model" at their headers.)

Indeed. Sorry I got confused.

> 
> I suggest rewriting it as:
> 
>   If one day we allow non-device QOM objects to use the global property
>   system, we can switch this from TYPE_DEVICE to TYPE_OBJECT."

For the comment for this patch (and patch 10 on "properties" and
"globals"), I both agree. Again, I'll see what's the best way to fix
them.  Thanks,

-- 
Peter Xu