[PATCH v2 0/6] migration: objectify MigrationState

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 0/6] migration: objectify MigrationState

Peter Xu
v2
- (I didn't add Juan's r-b since I touched the patches)
- remove once parameter in migrate_get_current() since not needed
- add one more patch to export register_compat_prop(), then use it in
  the following patches in xen_init().

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

Currently register_compat_prop() is exported to be used by xen_init().

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

Please review. Thanks.

Peter Xu (6):
  machine: export register_compat_prop()
  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

 hw/core/machine.c             |  6 +--
 hw/i386/pc_piix.c             |  3 --
 hw/ppc/spapr.c                |  3 --
 hw/xen/xen-common.c           | 12 ++++--
 include/hw/boards.h           |  3 ++
 include/hw/compat.h           | 12 ++++++
 include/migration/migration.h | 36 +++++++++++++++--
 include/sysemu/sysemu.h       |  1 -
 migration/migration.c         | 92 +++++++++++++++++++++++++++++--------------
 migration/savevm.c            | 28 ++++---------
 vl.c                          |  9 ++++-
 11 files changed, 136 insertions(+), 69 deletions(-)

--
2.7.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 1/6] machine: export register_compat_prop()

Peter Xu
We have HW_COMPAT_*, however that's only binded 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.

CC: Eduardo Habkost <[hidden email]>
CC: Markus Armbruster <[hidden email]>
CC: Marcel Apfelbaum <[hidden email]>
Signed-off-by: Peter Xu <[hidden email]>
---
 hw/core/machine.c   | 6 +++---
 include/hw/boards.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3adebf1..320486d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -753,9 +753,9 @@ 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)
+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: */
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 76ce021..6e0f5c7 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -41,6 +41,9 @@ int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
+void register_compat_prop(const char *driver,
+                          const char *property,
+                          const char *value);
 void machine_register_compat_props(MachineState *machine);
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
 void machine_set_cpu_numa_node(MachineState *machine,
--
2.7.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 2/6] migration: let MigrationState be a qdev

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

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

No functional change at all.

Signed-off-by: Peter Xu <[hidden email]>
---
 include/migration/migration.h | 19 ++++++++++++++
 migration/migration.c         | 61 ++++++++++++++++++++++++++++---------------
 2 files changed, 59 insertions(+), 21 deletions(-)

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 3/6] migration: move global_state.optional out

Peter Xu
In reply to this post by 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 in general, and the register_compat_prop() for xen_init().

Signed-off-by: Peter Xu <[hidden email]>
---
 hw/i386/pc_piix.c             |  1 -
 hw/ppc/spapr.c                |  1 -
 hw/xen/xen-common.c           |  8 +++++++-
 include/hw/compat.h           |  4 ++++
 include/migration/migration.h |  7 ++++++-
 migration/migration.c         | 24 ++++++++++++++++--------
 6 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2234bd0..c83cec5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -317,7 +317,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab3aab1..3e78bb9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3593,7 +3593,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 0bed577..8240d50 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -138,7 +138,13 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    global_state_set_optional();
+    /*
+     * TODO: make sure global MigrationState has not yet been created
+     * (otherwise the compat trick won't work). For now we are in
+     * configure_accelerator() so we are mostly good. Better to
+     * confirm that in the future.
+     */
+    register_compat_prop("migration", "store-global-state", "off");
     savevm_skip_configuration();
     savevm_skip_section_footers();
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 400c64b..5b5c8de 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -177,6 +177,10 @@
         .driver   = TYPE_PCI_DEVICE,\
         .property = "x-pcie-lnksta-dllla",\
         .value    = "off",\
+    },{\
+        .driver   = "migration",\
+        .property = "store-global-state",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_2 \
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bd0186c..d3ec719 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -162,6 +162,12 @@ struct MigrationState
     /* Do we have to clean up -b/-i from old migrate parameters */
     /* This feature is deprecated and will be removed */
     bool must_remove_block_options;
+
+    /*
+     * Global switch on whether we need to store the global state
+     * during migration.
+     */
+    bool store_global_state;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -240,7 +246,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
 
 void savevm_skip_section_footers(void);
 void register_global_state(void);
-void global_state_set_optional(void);
 void savevm_skip_configuration(void);
 int global_state_store(void);
 void global_state_store_running(void);
diff --git a/migration/migration.c b/migration/migration.c
index 98b77e2..79d886c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -138,13 +138,13 @@ void migration_incoming_state_destroy(void)
 
 
 typedef struct {
-    bool optional;
     uint32_t size;
     uint8_t runstate[100];
     RunState state;
     bool received;
 } GlobalState;
 
+/* This is only used if MigrationState.store_global_state is set. */
 static GlobalState global_state;
 
 int global_state_store(void)
@@ -175,19 +175,13 @@ static 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;
     char *runstate = (char *)s->runstate;
 
     /* If it is not optional, it is mandatory */
-
-    if (s->optional == false) {
+    if (migrate_get_current()->store_global_state) {
         return true;
     }
 
@@ -2107,6 +2101,19 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static Property migration_properties[] = {
+    DEFINE_PROP_BOOL("store-global-state", MigrationState,
+                     store_global_state, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = migration_properties;
+}
+
 static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
@@ -2131,6 +2138,7 @@ static void migration_instance_init(Object *obj)
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
     .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
     .class_size = sizeof(MigrationClass),
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
--
2.7.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 4/6] migration: move only_migratable to MigrationState

Peter Xu
In reply to this post by Peter Xu
One less global variable, and it does only matter with migration.

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

  -global migration.only-migratable=true

Currently still keep the old interface.

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

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 5/6] migration: move skip_configuration out

Peter Xu
In reply to this post by Peter Xu
It was in SaveState but now moved to MigrationState altogether. Again,
using HW_COMPAT_2_3 for old PC/SPAPR machines, and
register_compat_prop() for xen_init().

Signed-off-by: Peter Xu <[hidden email]>
---
 hw/i386/pc_piix.c             |  1 -
 hw/ppc/spapr.c                |  1 -
 hw/xen/xen-common.c           |  2 +-
 include/hw/compat.h           |  4 ++++
 include/migration/migration.h |  4 +++-
 migration/migration.c         |  2 ++
 migration/savevm.c            | 15 ++++-----------
 7 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c83cec5..529018d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -317,7 +317,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    savevm_skip_configuration();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3e78bb9..227b03b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3593,7 +3593,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    savevm_skip_configuration();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 8240d50..a80034f 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -145,7 +145,7 @@ static int xen_init(MachineState *ms)
      * confirm that in the future.
      */
     register_compat_prop("migration", "store-global-state", "off");
-    savevm_skip_configuration();
+    register_compat_prop("migration", "skip-configuration", "on");
     savevm_skip_section_footers();
 
     return 0;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 5b5c8de..4ed2ae7 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -179,6 +179,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "skip-configuration",\
+        .value    = "on",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 27b07ed..5f6861c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -171,6 +171,9 @@ struct MigrationState
 
     /* Whether the VM is only allowing for migratable devices */
     bool only_migratable;
+
+    /* Whether we skip QEMU_VM_CONFIGURATION for migration */
+    bool skip_configuration;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -249,7 +252,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
 
 void savevm_skip_section_footers(void);
 void register_global_state(void);
-void savevm_skip_configuration(void);
 int global_state_store(void);
 void global_state_store_running(void);
 
diff --git a/migration/migration.c b/migration/migration.c
index dbec586..a4ab83d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2105,6 +2105,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
+    DEFINE_PROP_BOOL("skip-configuration", MigrationState,
+                     skip_configuration, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f073027..222af4c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -290,7 +290,6 @@ typedef struct SaveStateEntry {
 typedef struct SaveState {
     QTAILQ_HEAD(, SaveStateEntry) handlers;
     int global_section_id;
-    bool skip_configuration;
     uint32_t len;
     const char *name;
     uint32_t target_page_bits;
@@ -299,15 +298,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;
@@ -989,11 +981,11 @@ void qemu_savevm_state_header(QEMUFile *f)
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (!migrate_get_current()->skip_configuration ||
+        enforce_config_section()) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
-
 }
 
 void qemu_savevm_state_begin(QEMUFile *f)
@@ -2003,7 +1995,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (!migrate_get_current()->skip_configuration ||
+        enforce_config_section()) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
--
2.7.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 6/6] migration: move skip_section_footers

Peter Xu
In reply to this post by Peter Xu
Move it into MigrationState, with a property binded to it. Same trick is
played like previous patches.

Signed-off-by: Peter Xu <[hidden email]>
---
 hw/i386/pc_piix.c             |  1 -
 hw/ppc/spapr.c                |  1 -
 hw/xen/xen-common.c           |  2 +-
 include/hw/compat.h           |  4 ++++
 include/migration/migration.h |  3 ++-
 migration/migration.c         |  2 ++
 migration/savevm.c            | 11 ++---------
 7 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 529018d..1be23e2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -313,7 +313,6 @@ static void pc_init1(MachineState *machine,
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 227b03b..944f829 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3592,7 +3592,6 @@ DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
 static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
-    savevm_skip_section_footers();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index a80034f..e8f08bb 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -146,7 +146,7 @@ static int xen_init(MachineState *ms)
      */
     register_compat_prop("migration", "store-global-state", "off");
     register_compat_prop("migration", "skip-configuration", "on");
-    savevm_skip_section_footers();
+    register_compat_prop("migration", "skip-section-footer", "on");
 
     return 0;
 }
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 4ed2ae7..ef5fbc7 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -183,6 +183,10 @@
         .value    = "on",\
     },{\
         .driver   = "migration",\
+        .property = "skip-section-footer",\
+        .value    = "on",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 5f6861c..dc35567 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -174,6 +174,8 @@ struct MigrationState
 
     /* Whether we skip QEMU_VM_CONFIGURATION for migration */
     bool skip_configuration;
+    /* Whether we skip section footer */
+    bool skip_section_footer;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -250,7 +252,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
                              ram_addr_t offset, size_t size,
                              uint64_t *bytes_sent);
 
-void savevm_skip_section_footers(void);
 void register_global_state(void);
 int global_state_store(void);
 void global_state_store_running(void);
diff --git a/migration/migration.c b/migration/migration.c
index a4ab83d..9f90f7b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2107,6 +2107,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("skip-configuration", MigrationState,
                      skip_configuration, false),
+    DEFINE_PROP_BOOL("skip-section-footer", MigrationState,
+                     skip_section_footer, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 222af4c..06d6986 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -65,8 +65,6 @@
 
 const unsigned int postcopy_ram_discard_version = 0;
 
-static bool skip_section_footers;
-
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
     MIG_CMD_INVALID = 0,   /* Must be 0 */
@@ -780,11 +778,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)
  */
@@ -812,7 +805,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
  */
 static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
 {
-    if (!skip_section_footers) {
+    if (!migrate_get_current()->skip_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
     }
@@ -1810,7 +1803,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
     uint8_t read_mark;
     uint32_t read_section_id;
 
-    if (skip_section_footers) {
+    if (migrate_get_current()->skip_section_footer) {
         /* No footer to check */
         return true;
     }
--
2.7.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 1/6] machine: export register_compat_prop()

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:
> We have HW_COMPAT_*, however that's only binded 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.
>
> CC: Eduardo Habkost <[hidden email]>
> CC: Markus Armbruster <[hidden email]>
> CC: Marcel Apfelbaum <[hidden email]>
> Signed-off-by: Peter Xu <[hidden email]>


I would let others talk about this approach, for me it is ok.

Later, Juan.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/6] migration: let MigrationState be a qdev

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:

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

Reviewed-by: Juan Quintela <[hidden email]>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 3/6] migration: move global_state.optional out

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> 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 in general, and the register_compat_prop() for xen_init().
>
> Signed-off-by: Peter Xu <[hidden email]>

Yeap, that is what I had in mind.

Reviewed-by: Juan Quintela <[hidden email]>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 4/6] migration: move only_migratable to MigrationState

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> 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.
>
> Signed-off-by: Peter Xu <[hidden email]>

Reviewed-by: Juan Quintela <[hidden email]>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 5/6] migration: move skip_configuration out

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:
> It was in SaveState but now moved to MigrationState altogether. Again,
> using HW_COMPAT_2_3 for old PC/SPAPR machines, and
> register_compat_prop() for xen_init().
>
> Signed-off-by: Peter Xu <[hidden email]>

Reviewed-by: Juan Quintela <[hidden email]>

if you had to respin, I think it would be better to change the name to
save_configuration, and change the true/false logic.  I don't like
negative logic, but it was used in this case due to how it was used,
now, it is not needed anymore.

Later, Juan.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] migration: move skip_section_footers

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:
> Move it into MigrationState, with a property binded to it. Same trick is
> played like previous patches.
>
> Signed-off-by: Peter Xu <[hidden email]>

Reviewed-by: Juan Quintela <[hidden email]>

Same here.  Rename it to "section-footers".
For previous one, "global-configuration"?

Later, Juan.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] migration: objectify MigrationState

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:

> v2
> - (I didn't add Juan's r-b since I touched the patches)
> - remove once parameter in migrate_get_current() since not needed
> - add one more patch to export register_compat_prop(), then use it in
>   the following patches in xen_init().
>
> I picked this topic out as suggested by Juan. Also I did what Juan has
> suggested in previous discussions that I moved lots of global
> parameters into MigrationState, and let them be properties. Then we
> can use HW_COMPAT_* and "-global migration.xxx=xxx" formular.
>
> Currently register_compat_prop() is exported to be used by xen_init().
>
> If this can be merged and okay, we can move on to convert more things
> into properties for migration.
>
> Please review. Thanks.

Hi

Nice work, thanks.

One question, once this is accepted, it would be easy to add to this the
migration_capabilities and migration_paramenters, right?

Later, Juan.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] migration: move skip_section_footers

Peter Xu
In reply to this post by Juan Quintela
On Fri, Jun 09, 2017 at 09:47:09AM +0200, Juan Quintela wrote:

> Peter Xu <[hidden email]> wrote:
> > Move it into MigrationState, with a property binded to it. Same trick is
> > played like previous patches.
> >
> > Signed-off-by: Peter Xu <[hidden email]>
>
> Reviewed-by: Juan Quintela <[hidden email]>
>
> Same here.  Rename it to "section-footers".
> For previous one, "global-configuration"?

Let me switch, considering that we always need to keep compatibility
of old interfaces, and we'd better do it right when we started... :)

Though I'll wait for a while for other comments, especially patch 1/2
from QAPI side.

Thanks,

--
Peter Xu

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] migration: objectify MigrationState

Peter Xu
In reply to this post by Juan Quintela
On Fri, Jun 09, 2017 at 09:48:32AM +0200, Juan Quintela wrote:

> Peter Xu <[hidden email]> wrote:
> > v2
> > - (I didn't add Juan's r-b since I touched the patches)
> > - remove once parameter in migrate_get_current() since not needed
> > - add one more patch to export register_compat_prop(), then use it in
> >   the following patches in xen_init().
> >
> > I picked this topic out as suggested by Juan. Also I did what Juan has
> > suggested in previous discussions that I moved lots of global
> > parameters into MigrationState, and let them be properties. Then we
> > can use HW_COMPAT_* and "-global migration.xxx=xxx" formular.
> >
> > Currently register_compat_prop() is exported to be used by xen_init().
> >
> > If this can be merged and okay, we can move on to convert more things
> > into properties for migration.
> >
> > Please review. Thanks.
>
> Hi
>
> Nice work, thanks.
>
> One question, once this is accepted, it would be easy to add to this the
> migration_capabilities and migration_paramenters, right?

Yes. If this series would be merged, I'll work on them soon. Thanks,

--
Peter Xu

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] migration: move skip_section_footers

Eric Blake
In reply to this post by Peter Xu
On 06/08/2017 10:49 PM, Peter Xu wrote:
> Move it into MigrationState, with a property binded to it. Same trick is

s/binded/bound/

> played like previous patches.
>
> Signed-off-by: Peter Xu <[hidden email]>
> ---


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


signature.asc (617 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/6] migration: let MigrationState be a qdev

Markus Armbruster
In reply to this post by Peter Xu
Peter Xu <[hidden email]> writes:

> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
>
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
>
> No functional change at all.
>
> Signed-off-by: Peter Xu <[hidden email]>
> ---
>  include/migration/migration.h | 19 ++++++++++++++
>  migration/migration.c         | 61 ++++++++++++++++++++++++++++---------------
>  2 files changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 79b5484..bd0186c 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -21,6 +21,7 @@
>  #include "qapi-types.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/coroutine_int.h"
> +#include "hw/qdev.h"
>  
>  #define QEMU_VM_FILE_MAGIC           0x5145564d
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> @@ -49,6 +50,8 @@ enum mig_rp_message_type {
>      MIG_RP_MSG_MAX
>  };
>  
> +#define TYPE_MIGRATION "migration"
> +
>  /* State for the incoming migration */
>  struct MigrationIncomingState {
>      QEMUFile *from_src_file;
> @@ -91,8 +94,24 @@ struct MigrationIncomingState {
>  MigrationIncomingState *migration_incoming_get_current(void);
>  void migration_incoming_state_destroy(void);
>  
> +#define MIGRATION_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> +#define MIGRATION_OBJ(obj) \
> +    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> +#define MIGRATION_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> +
> +typedef struct MigrationClass {
> +    /*< private >*/
> +    DeviceClass parent_class;
> +} MigrationClass;
> +
>  struct MigrationState
>  {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/

Turning MigrationState into a QOM object so you can use QOM
infrastructure makes sense.  But why turn it into a device?  It doesn't
feel device-like to me.  Would ObjectClass and Object instead of
DeviceClass and DeviceState do?

>      size_t bytes_xfer;
>      size_t xfer_limit;
>      QemuThread thread;
> diff --git a/migration/migration.c b/migration/migration.c
> index 48c94c9..98b77e2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -93,29 +93,13 @@ static bool deferred_incoming;
>  /* 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,
> -        },
> -    };
> +    static MigrationState *current_migration;

You add an indirection...

>  
> -    if (!once) {
> -        current_migration.parameters.tls_creds = g_strdup("");
> -        current_migration.parameters.tls_hostname = g_strdup("");
> -        once = true;
> +    if (!current_migration) {
> +        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));

... possibly just so you can use object_new().  Have you considered
object_initialize()?

>      }
> -    return &current_migration;
> +
> +    return current_migration;
>  }
>  
>  MigrationIncomingState *migration_incoming_get_current(void)
> @@ -2123,3 +2107,38 @@ void migrate_fd_connect(MigrationState *s)
>      s->migration_thread_running = true;
>  }
>  
> +static void migration_instance_init(Object *obj)
> +{
> +    MigrationState *ms = MIGRATION_OBJ(obj);
> +
> +    ms->state = MIGRATION_STATUS_NONE;
> +    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
> +    ms->mbps = -1;
> +    ms->parameters = (MigrationParameters) {
> +        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> +        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> +        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> +        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
> +        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> +        .max_bandwidth = MAX_THROTTLE,
> +        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
> +        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
> +    };
> +    ms->parameters.tls_creds = g_strdup("");
> +    ms->parameters.tls_hostname = g_strdup("");
> +}
> +
> +static const TypeInfo migration_type = {
> +    .name = TYPE_MIGRATION,
> +    .parent = TYPE_DEVICE,
> +    .class_size = sizeof(MigrationClass),
> +    .instance_size = sizeof(MigrationState),
> +    .instance_init = migration_instance_init,
> +};
> +
> +static void register_migration_types(void)
> +{
> +    type_register_static(&migration_type);
> +}
> +
> +type_init(register_migration_types);

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 3/6] migration: move global_state.optional out

Eduardo Habkost-2
In reply to this post by Peter Xu
On Fri, Jun 09, 2017 at 11:48:59AM +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 in general, and the register_compat_prop() for xen_init().
>
> Signed-off-by: Peter Xu <[hidden email]>
> ---
>  hw/i386/pc_piix.c             |  1 -
>  hw/ppc/spapr.c                |  1 -
>  hw/xen/xen-common.c           |  8 +++++++-
>  include/hw/compat.h           |  4 ++++
>  include/migration/migration.h |  7 ++++++-
>  migration/migration.c         | 24 ++++++++++++++++--------
>  6 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 2234bd0..c83cec5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -317,7 +317,6 @@ static void pc_compat_2_3(MachineState *machine)
>      if (kvm_enabled()) {
>          pcms->smm = ON_OFF_AUTO_OFF;
>      }
> -    global_state_set_optional();
>      savevm_skip_configuration();
>  }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..3e78bb9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3593,7 +3593,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
>  {
>      spapr_machine_2_4_instance_options(machine);
>      savevm_skip_section_footers();
> -    global_state_set_optional();
>      savevm_skip_configuration();
>  }

This is a good thing: makes the migration behavior of the
machine-types introspectable in compat_props.

I suggest moving this part (and all the rest except the new
register_compat_prop() call below) to a separate patch, because
it is an improvement on its own.

>  
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 0bed577..8240d50 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -138,7 +138,13 @@ static int xen_init(MachineState *ms)
>      }
>      qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
>  
> -    global_state_set_optional();
> +    /*
> +     * TODO: make sure global MigrationState has not yet been created
> +     * (otherwise the compat trick won't work). For now we are in
> +     * configure_accelerator() so we are mostly good. Better to
> +     * confirm that in the future.
> +     */

So, this makes accelerator initialization very sensitive to
ordering.

> +    register_compat_prop("migration", "store-global-state", "off");

It's already hard to track down machine-type stuff that is
initialized at machine->init() time but it's not introspectable,
let's not do the same mistake with accelerators.

Can't this be solved by a AcceleratorClass::global_props field,
so we are sure there's a single place in the code where globals
are registered?  (Currently, they are all registered by the few
lines around the machine_register_compat_props() call in main()).

I think I see other use cases for a new
AcceleratorClass::global_props field.  e.g.: replacing
kvm_default_props and tcg_default_props in target/i386/cpu.c.

>      savevm_skip_configuration();
>      savevm_skip_section_footers();
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 400c64b..5b5c8de 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -177,6 +177,10 @@
>          .driver   = TYPE_PCI_DEVICE,\
>          .property = "x-pcie-lnksta-dllla",\
>          .value    = "off",\
> +    },{\
> +        .driver   = "migration",\
> +        .property = "store-global-state",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_2 \
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index bd0186c..d3ec719 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -162,6 +162,12 @@ struct MigrationState
>      /* Do we have to clean up -b/-i from old migrate parameters */
>      /* This feature is deprecated and will be removed */
>      bool must_remove_block_options;
> +
> +    /*
> +     * Global switch on whether we need to store the global state
> +     * during migration.
> +     */
> +    bool store_global_state;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> @@ -240,7 +246,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>  
>  void savevm_skip_section_footers(void);
>  void register_global_state(void);
> -void global_state_set_optional(void);
>  void savevm_skip_configuration(void);
>  int global_state_store(void);
>  void global_state_store_running(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 98b77e2..79d886c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -138,13 +138,13 @@ void migration_incoming_state_destroy(void)
>  
>  
>  typedef struct {
> -    bool optional;
>      uint32_t size;
>      uint8_t runstate[100];
>      RunState state;
>      bool received;
>  } GlobalState;
>  
> +/* This is only used if MigrationState.store_global_state is set. */
>  static GlobalState global_state;
>  
>  int global_state_store(void)
> @@ -175,19 +175,13 @@ static 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;
>      char *runstate = (char *)s->runstate;
>  
>      /* If it is not optional, it is mandatory */
> -
> -    if (s->optional == false) {
> +    if (migrate_get_current()->store_global_state) {
>          return true;
>      }
>  
> @@ -2107,6 +2101,19 @@ void migrate_fd_connect(MigrationState *s)
>      s->migration_thread_running = true;
>  }
>  
> +static Property migration_properties[] = {
> +    DEFINE_PROP_BOOL("store-global-state", MigrationState,
> +                     store_global_state, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void migration_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = migration_properties;
> +}
> +
>  static void migration_instance_init(Object *obj)
>  {
>      MigrationState *ms = MIGRATION_OBJ(obj);
> @@ -2131,6 +2138,7 @@ static void migration_instance_init(Object *obj)
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
>      .parent = TYPE_DEVICE,
> +    .class_init = migration_class_init,
>      .class_size = sizeof(MigrationClass),
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,
> --
> 2.7.4
>
>

--
Eduardo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] migration: objectify MigrationState

Markus Armbruster
In reply to this post by Peter Xu
Test compile gripes:

    hw/xen/xen-common.c: In function ‘xen_init’:
    hw/xen/xen-common.c:147:5: warning: implicit declaration of function ‘register_compat_prop’ [-Wimplicit-function-declaration]
         register_compat_prop("migration", "store-global-state", "off");
         ^~~~~~~~~~~~~~~~~~~~
    hw/xen/xen-common.c:147:5: warning: nested extern declaration of ‘register_compat_prop’ [-Wnested-externs]

You might want to install Xen development packages to catch such issues
earlier.

Test run:

    $ qemu-system-x86_64 -device migration,help
    migration.skip-section-footer=bool
    migration.store-global-state=bool
    migration.only-migratable=bool
    migration.skip-configuration=bool

What would adding this device do?

12
Loading...