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

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

[Peter Xu]

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                    |  10 ++++
 hmp.c                            |   3 +
 hw/core/machine.c                |  13 -----
 hw/core/qdev-properties.c        |  13 +++++
 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     |   3 +
 include/migration/global_state.h |   1 -
 include/migration/misc.h         |   6 +-
 include/sysemu/accel.h           |  10 ++++
 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, 246 insertions(+), 90 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 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    | 13 +++++++++++++
 include/hw/qdev-properties.h |  3 +++
 3 files changed, 16 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 9f1a497..6ff1ac3 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1047,6 +1047,19 @@ 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);
+    /* 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);
+}
+
 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 d206fc9..1722ca4 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,6 +201,9 @@ void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
+void register_compat_prop(const char *driver, const char *property,
+                          const char *value);
+
 /**
  * qdev_property_add_static:
  * @dev: Device to add the property to.
-- 
2.7.4

[Qemu-devel] [PATCH v4 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.

Use the newly exported register_compat_prop() to pass the accelerator
global properties to the global_props list.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/accel.c          | 10 ++++++++++
 include/sysemu/accel.h | 10 ++++++++++
 vl.c                   |  1 +
 3 files changed, 21 insertions(+)

diff --git a/accel/accel.c b/accel/accel.c
index 7c079a5..212581c 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -120,6 +120,16 @@ void configure_accelerator(MachineState *ms)
     }
 }
 
+void accel_register_compat_props(AccelState *accel)
+{
+    AccelClass *class = ACCEL_GET_CLASS(accel);
+    GlobalProperty *prop;
+
+    for (prop = class->global_props; prop && prop->driver; prop++) {
+        register_compat_prop(prop->driver, prop->property, prop->value);
+    }
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 15944c1..8a01e51 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,13 @@ typedef struct AccelClass {
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
     bool *allowed;
+    /*
+     * Array of gobal properties that would be applied when specific
+     * accelerator is chosen. It works just like
+     * MachineClass.compat_props but it's for accelerators not
+     * machines.
+     */
+    GlobalProperty *global_props;
 } AccelClass;
 
 #define TYPE_ACCEL "accel"
@@ -57,5 +65,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

[Qemu-devel] [PATCH v4 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..cdd2ec8 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_compat_props(void)
+{
+    qemu_opts_foreach(qemu_find_opts("global"),
+                      global_init_func, NULL, NULL);
+}
+
+/*
+ * Note: we should see that these compat 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.
+ */
+static void register_global_properties(MachineState *ms)
+{
+    accel_register_compat_props(ms->accelerator);
+    machine_register_compat_props(ms);
+    user_register_compat_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

[Qemu-devel] [PATCH v4 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.

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 cdd2ec8..9b04ba7 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

[Qemu-devel] [PATCH v4 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.

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 e877d45..edbdbfd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,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..9163a0a 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;
 }
 
+GlobalProperty xen_compat_props[] = {
+    {
+        .driver = "migration",
+        .property = "store-global-state",
+        .value = "off",
+    },
+    { .driver = NULL, .property = NULL, .value = NULL },
+};
+
 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

[Qemu-devel] [PATCH v4 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 6bfd489..917ab05 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2314,7 +2314,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 9b04ba7..57ff065 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

[Qemu-devel] [PATCH v4 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().

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 edbdbfd..29fac1b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,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 9163a0a..6e52fb0 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 @@ GlobalProperty xen_compat_props[] = {
         .property = "store-global-state",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-configuration",
+        .value = "off",
+    },
     { .driver = NULL, .property = NULL, .value = NULL },
 };
 
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 917ab05..f1e144d 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

[Qemu-devel] [PATCH v4 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().

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 29fac1b..7c40fdf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3579,7 +3579,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 6e52fb0..04512f4 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 @@ GlobalProperty xen_compat_props[] = {
         .property = "send-configuration",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-section-footer",
+        .value = "off",
+    },
     { .driver = NULL, .property = NULL, .value = NULL },
 };
 
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 f1e144d..28fa1d5 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

[Qemu-devel] [PATCH v4 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).

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 28fa1d5..6b1a9a5 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

[Qemu-devel] [PATCH v4 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)

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 v4 04/10] migration: let MigrationState be a qdev

[Juan Quintela]

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

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
>
> Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
> for x86/power, and AccelClass.global_props for Xen.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Nice!!!

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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)
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

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

[Eduardo Habkost]

On Wed, Jun 21, 2017 at 03:52:00PM +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.
> 
> Use the newly exported register_compat_prop() to pass the accelerator
> global properties to the global_props list.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  accel/accel.c          | 10 ++++++++++
>  include/sysemu/accel.h | 10 ++++++++++
>  vl.c                   |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 7c079a5..212581c 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -120,6 +120,16 @@ void configure_accelerator(MachineState *ms)
>      }
>  }
>  
> +void accel_register_compat_props(AccelState *accel)
> +{
> +    AccelClass *class = ACCEL_GET_CLASS(accel);
> +    GlobalProperty *prop;
> +
> +    for (prop = class->global_props; prop && prop->driver; prop++) {
> +        register_compat_prop(prop->driver, prop->property, prop->value);
> +    }
> +}

I suggest adding a generic register_compat_props_array(GlobalProperty *props)
helper, so we can reuse it at machine_register_compat_props()
later (once we fix global property ordering in
qdev_prop_set_globals() and make the object_class_foreach() hack
in machine_register_compat_props() unnecessary).

> +
>  static void register_accel_types(void)
>  {
>      type_register_static(&accel_type);
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 15944c1..8a01e51 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,13 @@ typedef struct AccelClass {
>      int (*available)(void);
>      int (*init_machine)(MachineState *ms);
>      bool *allowed;
> +    /*
> +     * Array of gobal properties that would be applied when specific

"global"

> +     * accelerator is chosen. It works just like
> +     * MachineClass.compat_props but it's for accelerators not
> +     * machines.
> +     */
> +    GlobalProperty *global_props;
>  } AccelClass;
>  
>  #define TYPE_ACCEL "accel"
> @@ -57,5 +65,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);
>      }
>  

I suggest a comment here warning about global property
registration ordering.  e.g.:

 /*
  * Ordering of global property registration matters:
  * - machine compat_props should override accelerator-specific
  *   globals.
  * - user-provided globals (-global, and global properties
  *   derived from other command-line options like -cpu) should
  *   override machine compat_props and accelerator-specific
  *   globals.
  */

> +    accel_register_compat_props(current_machine->accelerator);
>      machine_register_compat_props(current_machine);
>  
>      qemu_opts_foreach(qemu_find_opts("global"),

-- 
Eduardo

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

[Peter Xu]

On Wed, Jun 21, 2017 at 09:23:11AM -0300, Eduardo Habkost wrote:
> On Wed, Jun 21, 2017 at 03:52:00PM +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.
> > 
> > Use the newly exported register_compat_prop() to pass the accelerator
> > global properties to the global_props list.
> > 
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  accel/accel.c          | 10 ++++++++++
> >  include/sysemu/accel.h | 10 ++++++++++
> >  vl.c                   |  1 +
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/accel/accel.c b/accel/accel.c
> > index 7c079a5..212581c 100644
> > --- a/accel/accel.c
> > +++ b/accel/accel.c
> > @@ -120,6 +120,16 @@ void configure_accelerator(MachineState *ms)
> >      }
> >  }
> >  
> > +void accel_register_compat_props(AccelState *accel)
> > +{
> > +    AccelClass *class = ACCEL_GET_CLASS(accel);
> > +    GlobalProperty *prop;
> > +
> > +    for (prop = class->global_props; prop && prop->driver; prop++) {
> > +        register_compat_prop(prop->driver, prop->property, prop->value);
> > +    }
> > +}
> 
> I suggest adding a generic register_compat_props_array(GlobalProperty *props)
> helper, so we can reuse it at machine_register_compat_props()
> later (once we fix global property ordering in
> qdev_prop_set_globals() and make the object_class_foreach() hack
> in machine_register_compat_props() unnecessary).

Sure.

> 
> > +
> >  static void register_accel_types(void)
> >  {
> >      type_register_static(&accel_type);
> > diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> > index 15944c1..8a01e51 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,13 @@ typedef struct AccelClass {
> >      int (*available)(void);
> >      int (*init_machine)(MachineState *ms);
> >      bool *allowed;
> > +    /*
> > +     * Array of gobal properties that would be applied when specific
> 
> "global"

Will fix.

> 
> > +     * accelerator is chosen. It works just like
> > +     * MachineClass.compat_props but it's for accelerators not
> > +     * machines.
> > +     */
> > +    GlobalProperty *global_props;
> >  } AccelClass;
> >  
> >  #define TYPE_ACCEL "accel"
> > @@ -57,5 +65,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);
> >      }
> >  
> 
> I suggest a comment here warning about global property
> registration ordering.  e.g.:
> 
>  /*
>   * Ordering of global property registration matters:
>   * - machine compat_props should override accelerator-specific
>   *   globals.
>   * - user-provided globals (-global, and global properties
>   *   derived from other command-line options like -cpu) should
>   *   override machine compat_props and accelerator-specific
>   *   globals.
>   */

I have a standalone patch (next one) to unify these three places and
added some comment. Would that suite?

Thanks,

-- 
Peter Xu

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

[Eduardo Habkost]

On Thu, Jun 22, 2017 at 12:25:30PM +0800, Peter Xu wrote:
> On Wed, Jun 21, 2017 at 09:23:11AM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 21, 2017 at 03:52:00PM +0800, Peter Xu wrote:
[...]
> > > 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);
> > >      }
> > >  
> > 
> > I suggest a comment here warning about global property
> > registration ordering.  e.g.:
> > 
> >  /*
> >   * Ordering of global property registration matters:
> >   * - machine compat_props should override accelerator-specific
> >   *   globals.
> >   * - user-provided globals (-global, and global properties
> >   *   derived from other command-line options like -cpu) should
> >   *   override machine compat_props and accelerator-specific
> >   *   globals.
> >   */
> 
> I have a standalone patch (next one) to unify these three places and
> added some comment. Would that suite?

Sorry, I didn't see that patch when I was reviewing this one.  It looks
good to me.  Thanks!

-- 
Eduardo

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

[Peter Xu]

On Thu, Jun 22, 2017 at 02:28:52PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 22, 2017 at 12:25:30PM +0800, Peter Xu wrote:
> > On Wed, Jun 21, 2017 at 09:23:11AM -0300, Eduardo Habkost wrote:
> > > On Wed, Jun 21, 2017 at 03:52:00PM +0800, Peter Xu wrote:
> [...]
> > > > 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);
> > > >      }
> > > >  
> > > 
> > > I suggest a comment here warning about global property
> > > registration ordering.  e.g.:
> > > 
> > >  /*
> > >   * Ordering of global property registration matters:
> > >   * - machine compat_props should override accelerator-specific
> > >   *   globals.
> > >   * - user-provided globals (-global, and global properties
> > >   *   derived from other command-line options like -cpu) should
> > >   *   override machine compat_props and accelerator-specific
> > >   *   globals.
> > >   */
> > 
> > I have a standalone patch (next one) to unify these three places and
> > added some comment. Would that suite?
> 
> Sorry, I didn't see that patch when I was reviewing this one.  It looks
> good to me.  Thanks!

Got it. Let me respin to fix the other places.  Thanks!

-- 
Peter Xu