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

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

[Qemu-devel] [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 <ehabkost@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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

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

[Peter Xu]

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

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

No functional change at all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h | 19 ++++++++++++++
 migration/migration.c         | 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

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

[Peter Xu]

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

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 <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/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

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

[Peter Xu]

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

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

  -global migration.only-migratable=true

Currently still keep the old interface.

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

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 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

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

[Peter Xu]

It was in SaveState but now moved to MigrationState altogether. Again,
using HW_COMPAT_2_3 for old PC/SPAPR machines, and
register_compat_prop() for xen_init().

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

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

[Peter Xu]

Move it into MigrationState, with a property binded to it. Same trick is
played like previous patches.

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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 <ehabkost@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>


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

Later, Juan.

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

[Juan Quintela]

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

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
>
> 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 <peterx@redhat.com>

Yeap, that is what I had in mind.

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>

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

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It was in SaveState but now moved to MigrationState altogether. Again,
> using HW_COMPAT_2_3 for old PC/SPAPR machines, and
> register_compat_prop() for xen_init().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

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.

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

[Juan Quintela]

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

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

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

Later, Juan.

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> 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.

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

[Peter Xu]

On Fri, Jun 09, 2017 at 09:47:09AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Move it into MigrationState, with a property binded to it. Same trick is
> > played like previous patches.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 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

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

[Peter Xu]

On Fri, Jun 09, 2017 at 09:48:32AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> 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

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

[Eric Blake]

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

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 <peterx@redhat.com>
> ---


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


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

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> ---
>  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);

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

[Eduardo Habkost]

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 <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/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

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

[Markus Armbruster]

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?

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

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> 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?

Parameters, capabilities, options for migration.

This was what we discussed on irc.  We want to have a way to control
migration features that depend on versions.

So we disable new features for old machine types (normal thing that we
do).  But right now, creating such a new feature requires creating a
couple of functions to set/clear the feature, adding includes on the
destination side, etc.

This was supposed to be a global property (or a qemu_opt, or whatever).
Just something that could be enabled/disabled easily on machine types,
and if possible independently of machine types.  This (for instance)
would allow that store-global-state is disabled by defaulte for
machine-2.9 (I forgot the exact name for the machine type), but I can
enable it by hand.  That is very handy for testing.

So, I think that the question is, how/where can we set that kind of
properties?

At least for me, being able to *also* set migration
capabilities/parameters with this mechanism on the command line would be
very nice, for instance

   -global migration.xbzrle on

or

   -global migration.max-speed 2G

I don't care about what is the exact syntax, or where we hang them,
i.e. a new device, a new list, somewhere that already exist.  That is
what we ask for advice.

Thanks for the review and the suggestions.

Later, Juan.

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

[Juan Quintela]

Eduardo Habkost <ehabkost@redhat.com> wrote:
D> 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 <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/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.

That is the kind of discussion that I wanted to start.  Thanks.
For the migration point of view, we don't care *how* or *where* it is
implemented, just that it is easy to add new properties (call it
whatever name that you want).

And yes, making xen/kvm/tcg use QOM like everything else looks like a
good idea to me.

Later, Juan.

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

[Peter Xu]

On Fri, Jun 09, 2017 at 05:47:09AM -0500, Eric Blake wrote:
> 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/

Will fix. Thanks Eric. :)

> 
> > played like previous patches.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 




-- 
Peter Xu

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

[Peter Xu]

On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > ---
> >  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?

Yeah. That's the main reason for the series to be (was) RFC.

I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
thing (IIUC you got that already :). I am just curious about why that
is not for QObject from the very beginning, then it'll be easier.

For now, IMHO QDev is okay as well for migration, as long as it's kept
internally inside QEMU. But sure at least I should turn user_creatable
to off. I'll investigate more to see how to make this a safer approach
in next post.

> 
> >      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()?

Yes, but even with object_initialize() I still need that indirection?

Or, I can put the init into main() in some way to avoid the
indirection.

Thanks!

-- 
Peter Xu

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

[Peter Xu]

On Fri, Jun 09, 2017 at 04:02:37PM +0200, Markus Armbruster wrote:
> 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.

Sorry I have missed to check that. I'll definitely verify this in the
following up posts.

> 
> 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?

Hmm, the same answer in the other thread - I may need the compat bits
and "-global", so I am using QDev for migration, not only QObject.

Haven't investigate how difficult would it be to move these features
from QDev into QObject.  For now, do you think user_creatable=false an
workable solution for this? We'll still see this help for sure, but at
least we won't be able to create the device using "-device migration",
also we won't see the migration entry in "-device help".

Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> 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 <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/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.

Sure.

> 
> >  
> > 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.

Hmm, this sounds nice.  Then we can also get rid of the TODO above.

Thanks for your suggestion and review! I'll see whether I can writeup
something as RFC for this.

-- 
Peter Xu

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

[Eduardo Habkost]

On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > > ---
> > >  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?
> 
> Yeah. That's the main reason for the series to be (was) RFC.
> 
> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> thing (IIUC you got that already :). I am just curious about why that
> is not for QObject from the very beginning, then it'll be easier.
> 
> For now, IMHO QDev is okay as well for migration, as long as it's kept
> internally inside QEMU. But sure at least I should turn user_creatable
> to off. I'll investigate more to see how to make this a safer approach
> in next post.

We could allow non-device QOM objects use the global property
system optionally.

We could make qdev_prop_set_globals() work on any Object*, and
let QOM classes call it on post_init if they want to be
configurable by global properties too.

Note that this would break qdev_prop_check_globals(), because it
expects globals to work only on TYPE_DEVICE.  We could address
that by introducing a new interface type to indicate the type
works with -global (something like
INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).

Such a system would probably allow us to replace
default_machine_opts, default_boot_order, default_display,
default_ram_size (and probably many other compat fields) on
MachineClass.  It could also be used to implement -machine and
-accel options by simply translating them to global properties
(like we already do for -cpu).

(This sounds like reinventing a QemuOpts-like system on top of
global properties.  Maybe that's a bad thing, maybe that's a good
thing.  I'm not sure.)

> [...]

-- 
Eduardo

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

[Peter Xu]

On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote:
> On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> > 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 <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/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.
> 
> Sure.
> 
> > 
> > >  
> > > 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.
> 
> Hmm, this sounds nice.  Then we can also get rid of the TODO above.
> 
> Thanks for your suggestion and review! I'll see whether I can writeup
> something as RFC for this.

After a second thought, I see these requirements are slightly
different.

For xen_init(), AccelClass::global_props may help since that's mainly
properties to be setup for TYPE_MIGRATION (let's assume it's okay that
MigrationState can be a QDev).

For kvm_default_props and tcg_default_props, AccelClass::global_props
may not help since they are setting up properties for TYPE_CPU, and
TYPE_CPU cannot really use global property features yet. :(

If finally we can have a new way (e.g., as you suggested in the other
thread, using a new INTERFACE to show that one QObject supports global
qdev properties) to allow TYPE_CPU to use global properties, then
these two requirements can merge. Otherwise IMHO we may still need to
keep the *_default_props in CPU codes.

Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote:
> On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > > Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > > > ---
> > > >  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?
> > 
> > Yeah. That's the main reason for the series to be (was) RFC.
> > 
> > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> > thing (IIUC you got that already :). I am just curious about why that
> > is not for QObject from the very beginning, then it'll be easier.
> > 
> > For now, IMHO QDev is okay as well for migration, as long as it's kept
> > internally inside QEMU. But sure at least I should turn user_creatable
> > to off. I'll investigate more to see how to make this a safer approach
> > in next post.
> 
> We could allow non-device QOM objects use the global property
> system optionally.
> 
> We could make qdev_prop_set_globals() work on any Object*, and
> let QOM classes call it on post_init if they want to be
> configurable by global properties too.
> 
> Note that this would break qdev_prop_check_globals(), because it
> expects globals to work only on TYPE_DEVICE.  We could address
> that by introducing a new interface type to indicate the type
> works with -global (something like
> INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> 
> Such a system would probably allow us to replace
> default_machine_opts, default_boot_order, default_display,
> default_ram_size (and probably many other compat fields) on
> MachineClass.  It could also be used to implement -machine and
> -accel options by simply translating them to global properties
> (like we already do for -cpu).
> 
> (This sounds like reinventing a QemuOpts-like system on top of
> global properties.  Maybe that's a bad thing, maybe that's a good
> thing.  I'm not sure.)

Thanks Eduardo for the reviews and suggestions. 

It'll obviously benefit us a lot if all the objects can use the global
properties and the *_COMPAT_* legacy magic, while the tricky point is
that, QObject will then depend on QDev?

Generally speaking this sounds good to me.  Before I do anything else,
I believe I should wait to see how other QOM developers think about
it.  Thanks,

-- 
Peter Xu

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

[Eduardo Habkost]

On Tue, Jun 13, 2017 at 11:41:02AM +0800, Peter Xu wrote:
> On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote:
> > On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> > > 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 <peterx@redhat.com>
[...]
> > > > 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.
> > 
> > Hmm, this sounds nice.  Then we can also get rid of the TODO above.
> > 
> > Thanks for your suggestion and review! I'll see whether I can writeup
> > something as RFC for this.
> 
> After a second thought, I see these requirements are slightly
> different.
> 
> For xen_init(), AccelClass::global_props may help since that's mainly
> properties to be setup for TYPE_MIGRATION (let's assume it's okay that
> MigrationState can be a QDev).
> 
> For kvm_default_props and tcg_default_props, AccelClass::global_props
> may not help since they are setting up properties for TYPE_CPU, and
> TYPE_CPU cannot really use global property features yet. :(

TYPE_CPU can use global properties, for sure.  TYPE_CPU is a
TYPE_DEVICE subclass, and -cpu is implemented internally using
global properties.

> 
> If finally we can have a new way (e.g., as you suggested in the other
> thread, using a new INTERFACE to show that one QObject supports global
> qdev properties) to allow TYPE_CPU to use global properties, then
> these two requirements can merge. Otherwise IMHO we may still need to
> keep the *_default_props in CPU codes.
> 
> Thanks,
> 
> -- 
> Peter Xu

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Jun 13, 2017 at 11:52:15AM +0800, Peter Xu wrote:
> On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> > > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > > > Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > > > > ---
> > > > >  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?
> > > 
> > > Yeah. That's the main reason for the series to be (was) RFC.
> > > 
> > > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> > > thing (IIUC you got that already :). I am just curious about why that
> > > is not for QObject from the very beginning, then it'll be easier.
> > > 
> > > For now, IMHO QDev is okay as well for migration, as long as it's kept
> > > internally inside QEMU. But sure at least I should turn user_creatable
> > > to off. I'll investigate more to see how to make this a safer approach
> > > in next post.
> > 
> > We could allow non-device QOM objects use the global property
> > system optionally.
> > 
> > We could make qdev_prop_set_globals() work on any Object*, and
> > let QOM classes call it on post_init if they want to be
> > configurable by global properties too.
> > 
> > Note that this would break qdev_prop_check_globals(), because it
> > expects globals to work only on TYPE_DEVICE.  We could address
> > that by introducing a new interface type to indicate the type
> > works with -global (something like
> > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> > 
> > Such a system would probably allow us to replace
> > default_machine_opts, default_boot_order, default_display,
> > default_ram_size (and probably many other compat fields) on
> > MachineClass.  It could also be used to implement -machine and
> > -accel options by simply translating them to global properties
> > (like we already do for -cpu).
> > 
> > (This sounds like reinventing a QemuOpts-like system on top of
> > global properties.  Maybe that's a bad thing, maybe that's a good
> > thing.  I'm not sure.)
> 
> Thanks Eduardo for the reviews and suggestions. 
> 
> It'll obviously benefit us a lot if all the objects can use the global
> properties and the *_COMPAT_* legacy magic, while the tricky point is
> that, QObject will then depend on QDev?

I don't think the dependency would be the main problem.  The qdev
global-property code would be moved outside qdev, to common code.

The main problem I see is: it would make global properties too
powerful and dangerous.  Enabling global properties for all QOM
types unconditionally would expose QEMU internals that are not
supposed to be touched by the user.

> 
> Generally speaking this sounds good to me.  Before I do anything else,
> I believe I should wait to see how other QOM developers think about
> it.  Thanks,
> 
> -- 
> Peter Xu

-- 
Eduardo

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

[Peter Xu]

On Tue, Jun 13, 2017 at 08:16:35AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 13, 2017 at 11:41:02AM +0800, Peter Xu wrote:
> > On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote:
> > > On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> > > > 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 <peterx@redhat.com>
> [...]
> > > > > 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.
> > > 
> > > Hmm, this sounds nice.  Then we can also get rid of the TODO above.
> > > 
> > > Thanks for your suggestion and review! I'll see whether I can writeup
> > > something as RFC for this.
> > 
> > After a second thought, I see these requirements are slightly
> > different.
> > 
> > For xen_init(), AccelClass::global_props may help since that's mainly
> > properties to be setup for TYPE_MIGRATION (let's assume it's okay that
> > MigrationState can be a QDev).
> > 
> > For kvm_default_props and tcg_default_props, AccelClass::global_props
> > may not help since they are setting up properties for TYPE_CPU, and
> > TYPE_CPU cannot really use global property features yet. :(
> 
> TYPE_CPU can use global properties, for sure.  TYPE_CPU is a
> TYPE_DEVICE subclass, and -cpu is implemented internally using
> global properties.

Indeed. :)

Thanks, 

-- 
Peter Xu

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

[Peter Xu]

On Tue, Jun 13, 2017 at 08:16:35AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 13, 2017 at 11:41:02AM +0800, Peter Xu wrote:
> > On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote:
> > > On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> > > > 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 <peterx@redhat.com>
> [...]
> > > > > 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.
> > > 
> > > Hmm, this sounds nice.  Then we can also get rid of the TODO above.
> > > 
> > > Thanks for your suggestion and review! I'll see whether I can writeup
> > > something as RFC for this.
> > 
> > After a second thought, I see these requirements are slightly
> > different.
> > 
> > For xen_init(), AccelClass::global_props may help since that's mainly
> > properties to be setup for TYPE_MIGRATION (let's assume it's okay that
> > MigrationState can be a QDev).
> > 
> > For kvm_default_props and tcg_default_props, AccelClass::global_props
> > may not help since they are setting up properties for TYPE_CPU, and
> > TYPE_CPU cannot really use global property features yet. :(
> 
> TYPE_CPU can use global properties, for sure.  TYPE_CPU is a
> TYPE_DEVICE subclass, and -cpu is implemented internally using
> global properties.

Hi, Eduardo,

I'm working on providing a AccelState.global_props, then let KVM/TCG
to use it to handle X86_CPU properties there.  However, I stuck at a
point where TCG/KVM cannot know what is the macro "TYPE_X86_CPU"
(since it's only there on X86).  The change is something like this (it
cannot be applied directly to master since it's only one patch among
my series, it is used to show what I am doing and the problem):

--8<--
diff --git a/accel.c b/accel.c
index 82b318c..db503b6 100644
--- a/accel.c
+++ b/accel.c
@@ -34,13 +34,34 @@
 #include "sysemu/qtest.h"
 #include "hw/xen/xen.h"
 #include "qom/object.h"
+#if TARGET_I386
+#include "target/i386/cpu-qom.h"
+#endif

 int tcg_tb_size;
 static bool tcg_allowed = true;

+static AccelPropValue x86_tcg_default_props[] = {
+    { "vme", "off" },
+    { NULL, NULL },
+};
+
+static void tcg_register_accel_props(AccelState *accel)
+{
+#if TARGET_I386
+    AccelPropValue *entry;
+
+    for (entry = x86_tcg_default_props; entry->prop; entry++) {
+        global_property_list_register(accel->global_props, TYPE_X86_CPU,
+                                      entry->prop, entry->value, false);
+    }
+#endif
+}
+
 static int tcg_init(MachineState *ms)
 {
     tcg_exec_init(tcg_tb_size * 1024 * 1024);
+    tcg_register_accel_props(ms->accelerator);
     return 0;
 }

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5214fba..89f67b0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1480,17 +1480,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
     },
 };

-typedef struct PropValue {
-    const char *prop, *value;
-} PropValue;
-
-/* TCG-specific defaults that override all CPU models when using TCG
- */
-static PropValue tcg_default_props[] = {
-    { "vme", "off" },
-    { NULL, NULL },
-};
-
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only);

@@ -2262,18 +2251,6 @@ static void x86_cpu_report_filtered_features(X86CPU *cpu)
     }
 }

-static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
-{
-    PropValue *pv;
-    for (pv = props; pv->prop; pv++) {
-        if (!pv->value) {
-            continue;
-        }
-        object_property_parse(OBJECT(cpu), pv->value, pv->prop,
-                              &error_abort);
-    }
-}
-
 /* Load data from X86CPUDefinition into a X86CPU object
  */
 static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
@@ -2300,11 +2277,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
         env->features[w] = def->features[w];
     }

-    /* Special cases not set in the X86CPUDefinition structs: */
-    if (tcg_enabled()) {
-        x86_cpu_apply_props(cpu, tcg_default_props);
-    }
-
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;

     /* sysenter isn't supported in compatibility mode on AMD,
-->8--

What above patch did is something like "moving x86_cpu properties from
x86 CPU codes into tcg" (I am using tcg as example, kvm is more
complex but has similar issue).

Here the general problem is that, there are some properties to be
applied when both conditions match:

- target is X86 (so using X86 cpus)
- accel is tcg

In the old code, it works since in x86 cpu.c codes we can use this:

  if (tcg_enabled()) {
      x86_cpu_apply_props(cpu, tcg_default_props);
  }

to know "whether accel is TCG". However I cannot do similar thing in
TCG code to know "whether target is x86". (I was trying to use
TARGET_I386 but it was poisoned, of course...)

Any thoughts? (Markus/Paolo/others?)

TIA.

-- 
Peter Xu

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

[Eduardo Habkost]

On Fri, Jun 16, 2017 at 03:58:20PM +0800, Peter Xu wrote:
> On Tue, Jun 13, 2017 at 08:16:35AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 13, 2017 at 11:41:02AM +0800, Peter Xu wrote:
> > > On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote:
> > > > On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote:
> > > > > 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 <peterx@redhat.com>
> > [...]
> > > > > > 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.
> > > > 
> > > > Hmm, this sounds nice.  Then we can also get rid of the TODO above.
> > > > 
> > > > Thanks for your suggestion and review! I'll see whether I can writeup
> > > > something as RFC for this.
> > > 
> > > After a second thought, I see these requirements are slightly
> > > different.
> > > 
> > > For xen_init(), AccelClass::global_props may help since that's mainly
> > > properties to be setup for TYPE_MIGRATION (let's assume it's okay that
> > > MigrationState can be a QDev).
> > > 
> > > For kvm_default_props and tcg_default_props, AccelClass::global_props
> > > may not help since they are setting up properties for TYPE_CPU, and
> > > TYPE_CPU cannot really use global property features yet. :(
> > 
> > TYPE_CPU can use global properties, for sure.  TYPE_CPU is a
> > TYPE_DEVICE subclass, and -cpu is implemented internally using
> > global properties.
> 
> Hi, Eduardo,
> 
> I'm working on providing a AccelState.global_props, then let KVM/TCG
> to use it to handle X86_CPU properties there.  However, I stuck at a
> point where TCG/KVM cannot know what is the macro "TYPE_X86_CPU"
> (since it's only there on X86).  The change is something like this (it
> cannot be applied directly to master since it's only one patch among
> my series, it is used to show what I am doing and the problem):

Unfortunately TYPE_X86_CPU macro is arch-specific because it may
be expanded to "x86_64-cpu" or "i386-cpu" depending on the QEMU
target.  So it actually represents two different classes, because
the target/i386/cpu.c code is compiled twice (once for i386 and
once for x86_64).

In this case, you will need two entries on the tcg default
property list:

  x86_64-cpu.vme=off
  i386-cpu.vme=off

Now, we could hardcode the class names, or provide TYPE_I386_CPU
and TYPE_X86_64_CPU macros from a header available to generic
code[1].  I don't know what's the best solution.

[1] i.e. not cpu.h. I was going to suggest including
    "target/i386/cpu-qom.h", but I'm not sure if
    it can be safely included by generic code.

> 
> --8<--
> diff --git a/accel.c b/accel.c
> index 82b318c..db503b6 100644
> --- a/accel.c
> +++ b/accel.c
> @@ -34,13 +34,34 @@
>  #include "sysemu/qtest.h"
>  #include "hw/xen/xen.h"
>  #include "qom/object.h"
> +#if TARGET_I386
> +#include "target/i386/cpu-qom.h"
> +#endif
> 
>  int tcg_tb_size;
>  static bool tcg_allowed = true;
> 
> +static AccelPropValue x86_tcg_default_props[] = {
> +    { "vme", "off" },
> +    { NULL, NULL },

You need a "driver" (type) field too.  I suggest using struct
GlobalProperty like we do on MachineClass.

> +};
> +
> +static void tcg_register_accel_props(AccelState *accel)
> +{
> +#if TARGET_I386
> +    AccelPropValue *entry;
> +
> +    for (entry = x86_tcg_default_props; entry->prop; entry++) {
> +        global_property_list_register(accel->global_props, TYPE_X86_CPU,
> +                                      entry->prop, entry->value, false);
> +    }
> +#endif
> +}
> +
>  static int tcg_init(MachineState *ms)
>  {
>      tcg_exec_init(tcg_tb_size * 1024 * 1024);
> +    tcg_register_accel_props(ms->accelerator);
>      return 0;
>  }
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5214fba..89f67b0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1480,17 +1480,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>      },
>  };
> 
> -typedef struct PropValue {
> -    const char *prop, *value;
> -} PropValue;
> -
> -/* TCG-specific defaults that override all CPU models when using TCG
> - */
> -static PropValue tcg_default_props[] = {
> -    { "vme", "off" },
> -    { NULL, NULL },
> -};
> -
>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                     bool migratable_only);
> 
> @@ -2262,18 +2251,6 @@ static void x86_cpu_report_filtered_features(X86CPU *cpu)
>      }
>  }
> 
> -static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
> -{
> -    PropValue *pv;
> -    for (pv = props; pv->prop; pv++) {
> -        if (!pv->value) {
> -            continue;
> -        }
> -        object_property_parse(OBJECT(cpu), pv->value, pv->prop,
> -                              &error_abort);
> -    }
> -}
> -
>  /* Load data from X86CPUDefinition into a X86CPU object
>   */
>  static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
> @@ -2300,11 +2277,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>          env->features[w] = def->features[w];
>      }
> 
> -    /* Special cases not set in the X86CPUDefinition structs: */
> -    if (tcg_enabled()) {
> -        x86_cpu_apply_props(cpu, tcg_default_props);
> -    }
> -
>      env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
> 
>      /* sysenter isn't supported in compatibility mode on AMD,
> -->8--
> 
> What above patch did is something like "moving x86_cpu properties from
> x86 CPU codes into tcg" (I am using tcg as example, kvm is more
> complex but has similar issue).
> 
> Here the general problem is that, there are some properties to be
> applied when both conditions match:
> 
> - target is X86 (so using X86 cpus)
> - accel is tcg
> 
> In the old code, it works since in x86 cpu.c codes we can use this:
> 
>   if (tcg_enabled()) {
>       x86_cpu_apply_props(cpu, tcg_default_props);
>   }
> 
> to know "whether accel is TCG". However I cannot do similar thing in
> TCG code to know "whether target is x86". (I was trying to use
> TARGET_I386 but it was poisoned, of course...)
> 
> Any thoughts? (Markus/Paolo/others?)

You don't need to make the property conditional on the target, if
you just use the right class name on the "driver' field.

-- 
Eduardo

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

[Peter Xu]

On Fri, Jun 16, 2017 at 11:34:32AM -0300, Eduardo Habkost wrote:
> On Fri, Jun 16, 2017 at 03:58:20PM +0800, Peter Xu wrote:
> > On Tue, Jun 13, 2017 at 08:16:35AM -0300, Eduardo Habkost wrote:

[...]

> > Hi, Eduardo,
> > 
> > I'm working on providing a AccelState.global_props, then let KVM/TCG
> > to use it to handle X86_CPU properties there.  However, I stuck at a
> > point where TCG/KVM cannot know what is the macro "TYPE_X86_CPU"
> > (since it's only there on X86).  The change is something like this (it
> > cannot be applied directly to master since it's only one patch among
> > my series, it is used to show what I am doing and the problem):
> 
> Unfortunately TYPE_X86_CPU macro is arch-specific because it may
> be expanded to "x86_64-cpu" or "i386-cpu" depending on the QEMU
> target.  So it actually represents two different classes, because
> the target/i386/cpu.c code is compiled twice (once for i386 and
> once for x86_64).
> 
> In this case, you will need two entries on the tcg default
> property list:
> 
>   x86_64-cpu.vme=off
>   i386-cpu.vme=off
> 
> Now, we could hardcode the class names, or provide TYPE_I386_CPU
> and TYPE_X86_64_CPU macros from a header available to generic
> code[1].  I don't know what's the best solution.
> 
> [1] i.e. not cpu.h. I was going to suggest including
>     "target/i386/cpu-qom.h", but I'm not sure if
>     it can be safely included by generic code.

Thanks. I'll choose to hard code it for now (with some comments).

> 
> > 
> > --8<--
> > diff --git a/accel.c b/accel.c
> > index 82b318c..db503b6 100644
> > --- a/accel.c
> > +++ b/accel.c
> > @@ -34,13 +34,34 @@
> >  #include "sysemu/qtest.h"
> >  #include "hw/xen/xen.h"
> >  #include "qom/object.h"
> > +#if TARGET_I386
> > +#include "target/i386/cpu-qom.h"
> > +#endif
> > 
> >  int tcg_tb_size;
> >  static bool tcg_allowed = true;
> > 
> > +static AccelPropValue x86_tcg_default_props[] = {
> > +    { "vme", "off" },
> > +    { NULL, NULL },
> 
> You need a "driver" (type) field too.  I suggest using struct
> GlobalProperty like we do on MachineClass.

Yeah, I did it ...

> 
> > +};
> > +
> > +static void tcg_register_accel_props(AccelState *accel)
> > +{
> > +#if TARGET_I386
> > +    AccelPropValue *entry;
> > +
> > +    for (entry = x86_tcg_default_props; entry->prop; entry++) {
> > +        global_property_list_register(accel->global_props, TYPE_X86_CPU,
> > +                                      entry->prop, entry->value, false);

... here, since the driver field is the same (we'll just apply this
property list to both "i386-cpu" and "x86_64-cpu").

> > +    }
> > +#endif
> > +}
> > +

[...]

> > What above patch did is something like "moving x86_cpu properties from
> > x86 CPU codes into tcg" (I am using tcg as example, kvm is more
> > complex but has similar issue).
> > 
> > Here the general problem is that, there are some properties to be
> > applied when both conditions match:
> > 
> > - target is X86 (so using X86 cpus)
> > - accel is tcg
> > 
> > In the old code, it works since in x86 cpu.c codes we can use this:
> > 
> >   if (tcg_enabled()) {
> >       x86_cpu_apply_props(cpu, tcg_default_props);
> >   }
> > 
> > to know "whether accel is TCG". However I cannot do similar thing in
> > TCG code to know "whether target is x86". (I was trying to use
> > TARGET_I386 but it was poisoned, of course...)
> > 
> > Any thoughts? (Markus/Paolo/others?)
> 
> You don't need to make the property conditional on the target, if
> you just use the right class name on the "driver' field.

Yeah, as long as I can use the hard coded "i386-cpu"/"x86_64-cpu" in
accelerator codes, then I'll be fine.  Thanks again!

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jun 09, 2017 at 04:02:37PM +0200, Markus Armbruster wrote:
>> 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.
>
> Sorry I have missed to check that. I'll definitely verify this in the
> following up posts.
>
>> 
>> 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?
>
> Hmm, the same answer in the other thread - I may need the compat bits
> and "-global", so I am using QDev for migration, not only QObject.
>
> Haven't investigate how difficult would it be to move these features
> from QDev into QObject.  For now, do you think user_creatable=false an
> workable solution for this? We'll still see this help for sure, but at
> least we won't be able to create the device using "-device migration",
> also we won't see the migration entry in "-device help".

Hacks that aren't exposed in external interfaces can be tolerable.  In
particular when we have concrete ideas on how to get rid of them later
on.

Don't worry about visibility in "-device migration,help".

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
>> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
>> > Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
>> > > ---
>> > >  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?
>> 
>> Yeah. That's the main reason for the series to be (was) RFC.
>> 
>> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
>> thing (IIUC you got that already :). I am just curious about why that
>> is not for QObject from the very beginning, then it'll be easier.
>> 
>> For now, IMHO QDev is okay as well for migration, as long as it's kept
>> internally inside QEMU. But sure at least I should turn user_creatable
>> to off. I'll investigate more to see how to make this a safer approach
>> in next post.
>
> We could allow non-device QOM objects use the global property
> system optionally.
>
> We could make qdev_prop_set_globals() work on any Object*, and
> let QOM classes call it on post_init if they want to be
> configurable by global properties too.
>
> Note that this would break qdev_prop_check_globals(), because it
> expects globals to work only on TYPE_DEVICE.  We could address
> that by introducing a new interface type to indicate the type
> works with -global (something like
> INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
>
> Such a system would probably allow us to replace
> default_machine_opts, default_boot_order, default_display,
> default_ram_size (and probably many other compat fields) on
> MachineClass.  It could also be used to implement -machine and
> -accel options by simply translating them to global properties
> (like we already do for -cpu).
>
> (This sounds like reinventing a QemuOpts-like system on top of
> global properties.  Maybe that's a bad thing, maybe that's a good
> thing.  I'm not sure.)

Not sure I get the connection to QemuOpts.

Lifting the "adjust property default values on the command line" feature
from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense.

But let's reconsider what exactly we're trying to accomplish for
migration.  The cover letter and commit message are too vague for me gue
figure it out.  Please describe how exactly you intend this series'
features to be used.  You mentioned -global and HW_COMPAT_* briefly;
make sure to cover both.  Concrete examples like "instead of
<inconvenient way to do FOO> we can then do <better way>" would be nice.

>> [...]

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

[Peter Xu]

On Mon, Jun 19, 2017 at 11:09:58AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> >> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> >> > Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> >> > > ---
> >> > >  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?
> >> 
> >> Yeah. That's the main reason for the series to be (was) RFC.
> >> 
> >> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> >> thing (IIUC you got that already :). I am just curious about why that
> >> is not for QObject from the very beginning, then it'll be easier.
> >> 
> >> For now, IMHO QDev is okay as well for migration, as long as it's kept
> >> internally inside QEMU. But sure at least I should turn user_creatable
> >> to off. I'll investigate more to see how to make this a safer approach
> >> in next post.
> >
> > We could allow non-device QOM objects use the global property
> > system optionally.
> >
> > We could make qdev_prop_set_globals() work on any Object*, and
> > let QOM classes call it on post_init if they want to be
> > configurable by global properties too.
> >
> > Note that this would break qdev_prop_check_globals(), because it
> > expects globals to work only on TYPE_DEVICE.  We could address
> > that by introducing a new interface type to indicate the type
> > works with -global (something like
> > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> >
> > Such a system would probably allow us to replace
> > default_machine_opts, default_boot_order, default_display,
> > default_ram_size (and probably many other compat fields) on
> > MachineClass.  It could also be used to implement -machine and
> > -accel options by simply translating them to global properties
> > (like we already do for -cpu).
> >
> > (This sounds like reinventing a QemuOpts-like system on top of
> > global properties.  Maybe that's a bad thing, maybe that's a good
> > thing.  I'm not sure.)
> 
> Not sure I get the connection to QemuOpts.
> 
> Lifting the "adjust property default values on the command line" feature
> from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense.
> 
> But let's reconsider what exactly we're trying to accomplish for
> migration.  The cover letter and commit message are too vague for me gue
> figure it out.  Please describe how exactly you intend this series'
> features to be used.  You mentioned -global and HW_COMPAT_* briefly;
> make sure to cover both.  Concrete examples like "instead of
> <inconvenient way to do FOO> we can then do <better way>" would be nice.

Hi, Markus,

I added some more explaination on the cover letter in the latter
posts (latest v4). Please see whether that'll help, and just let me
know if there is still things unclear in that.  Thanks!

-- 
Peter Xu