[Qemu-devel] [PATCH v3 00/13] migration: objectify MigrationState

[Qemu-devel] [PATCH v3 00/13] migration: objectify MigrationState

[Peter Xu]

v3 contains too much new things. So here comes a new cover letter with
richer information.

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

It helps in many use cases.

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

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

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

  -global migration.postcopy=on

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

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

Here's what individual patch does:

patch 1-8: introduce AccelState.global_props list.

      This is an idea suggested by Eduardo in general, it is just more
      complicated than I thought. Here each patch has rich commit
      message to read (especially patch 6). Things to mention:
      - patch 5 fixes a test break
      - patch 8 added a trace so that I can verify all the x86 cpu
        properties are applied correctly on tcg/x86/pc-i440fx-*.

patch 9-13: the original patches for the objectify of MigrationState.

Please kindly review. Thanks.

Peter Xu (13):
  machine: export register_compat_prop()
  qdev: enhance global_prop_list_add()
  qdev: remove qdev_prop_register_global()
  accel: introduce AccelState.global_props
  tests: avoid check GlobalProperty.used
  kvm: let kvm use AccelState.global_props
  tcg: use AccelState.global_props
  trace: add qdev_global_prop_apply
  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

 Makefile.objs                    |  1 +
 accel.c                          | 50 +++++++++++++++++++++++
 hw/core/machine.c                | 20 +++------
 hw/core/qdev-properties.c        | 56 +++++++++++++++++++++++--
 hw/core/trace-events             |  2 +
 hw/i386/pc_piix.c                | 11 +++--
 hw/ppc/spapr.c                   |  3 --
 hw/xen/xen-common.c              |  9 ++--
 include/hw/compat.h              | 12 ++++++
 include/hw/qdev-properties.h     | 17 +++++++-
 include/migration/global_state.h |  1 -
 include/migration/misc.h         |  4 +-
 include/sysemu/accel.h           | 20 +++++++++
 include/sysemu/sysemu.h          |  1 -
 kvm-all.c                        | 30 ++++++++++++++
 migration/global_state.c         |  9 +---
 migration/migration.c            | 88 ++++++++++++++++++++++++++++++----------
 migration/migration.h            | 33 +++++++++++++++
 migration/savevm.c               | 28 ++++---------
 qom/cpu.c                        |  8 +---
 target/i386/cpu.c                | 77 +----------------------------------
 target/i386/cpu.h                | 11 -----
 tests/test-qdev-global-props.c   | 12 ------
 vl.c                             | 54 ++++++++++--------------
 24 files changed, 339 insertions(+), 218 deletions(-)
 create mode 100644 hw/core/trace-events

-- 
2.7.4

[Qemu-devel] [PATCH v3 01/13] 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.

Meanwhile, move it to qdev-properties.c where seems more proper.
Refactor it a bit to provide global_prop_list_add() for further
use (will use it in cases where we need a GlobalProperty list).

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>
---
 accel.c                      |  1 +
 hw/core/machine.c            | 14 +-------------
 hw/core/qdev-properties.c    | 22 ++++++++++++++++++++++
 include/hw/qdev-properties.h |  5 +++++
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/accel.c b/accel.c
index 664bb88..62edd29 100644
--- a/accel.c
+++ b/accel.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/accel.h"
 #include "hw/boards.h"
+#include "hw/qdev-properties.h"
 #include "qemu-common.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2e7e977..b4b3449 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -16,6 +16,7 @@
 #include "qapi-visit.h"
 #include "qapi/visitor.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/numa.h"
 #include "qemu/error-report.h"
@@ -770,19 +771,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     g_free(mc->name);
 }
 
-static void register_compat_prop(const char *driver,
-                                 const char *property,
-                                 const char *value)
-{
-    GlobalProperty *p = g_new0(GlobalProperty, 1);
-    /* Machine compat_props must never cause errors: */
-    p->errp = &error_abort;
-    p->driver = driver;
-    p->property = property;
-    p->value = value;
-    qdev_prop_register_global(p);
-}
-
 static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
 {
     GlobalProperty *p = opaque;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 9f1a497..4b74382 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1042,6 +1042,28 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 
 static GList *global_props;
 
+GList *global_prop_list_add(GList *list, const char *driver,
+                            const char *property, const char *value)
+{
+    GlobalProperty *p = g_new0(GlobalProperty, 1);
+
+    /* These properties cannot fail the apply */
+    p->errp = &error_abort;
+    p->driver = driver;
+    p->property = property;
+    p->value = value;
+
+    return g_list_append(list, p);
+}
+
+void register_compat_prop(const char *driver,
+                          const char *property,
+                          const char *value)
+{
+    global_props = global_prop_list_add(global_props, driver,
+                                        property, value);
+}
+
 void qdev_prop_register_global(GlobalProperty *prop)
 {
     global_props = g_list_append(global_props, prop);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index d206fc9..15ee6ba 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,6 +201,11 @@ void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
+GList *global_prop_list_add(GList *list, const char *driver,
+                            const char *property, const char *value);
+void register_compat_prop(const char *driver, const char *property,
+                          const char *value);
+
 /**
  * qdev_property_add_static:
  * @dev: Device to add the property to.
-- 
2.7.4

[Qemu-devel] [PATCH v3 02/13] qdev: enhance global_prop_list_add()

[Peter Xu]

Originally it can only alloc new entries and insert. Let it be smarter
that it can update existing fields, and even delete elements. This is
preparation of (finally) the replacement of x86_cpu_apply_props(). If
you see x86_cpu_apply_props(), it allows to skip entries when value is
NULL. Here, it works just like deleting an existing entry.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/qdev-properties.c    | 28 +++++++++++++++++++++++++++-
 include/hw/qdev-properties.h | 10 ++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 4b74382..dc3b0ac 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1045,7 +1045,33 @@ static GList *global_props;
 GList *global_prop_list_add(GList *list, const char *driver,
                             const char *property, const char *value)
 {
-    GlobalProperty *p = g_new0(GlobalProperty, 1);
+    GList *l;
+    GlobalProperty *p;
+
+    /* Look up the (property, value) first in the list */
+    for (l = list; l; l = l->next) {
+        p = l->data;
+        if (!strcmp(driver, p->driver) && !strcmp(property, p->property)) {
+            if (value) {
+                /* Modify existing value */
+                p->value = value;
+            } else {
+                /* Delete this entry entirely */
+                list = g_list_remove_link(list, l);
+                g_free(p);
+                g_list_free(l);
+            }
+            return list;
+        }
+    }
+
+    /* Entry not exist. If we are deleting one entry, we're done. */
+    if (!value) {
+        return list;
+    }
+
+    /* If not found and value is set, we try to create a new entry */
+    p = g_new0(GlobalProperty, 1);
 
     /* These properties cannot fail the apply */
     p->errp = &error_abort;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 15ee6ba..55ad507 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,6 +201,16 @@ void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
+/*
+ * Register global property on the list. Elements of list should be
+ * GlobalProperty.
+ *
+ * - If (driver, property) is not existing on the list, create a new
+ *   one and link to the list.
+ * - If (driver, property) exists on the list, then:
+ *   - if value != NULL, update with new value
+ *   - if value == NULL, delete the entry
+ */
 GList *global_prop_list_add(GList *list, const char *driver,
                             const char *property, const char *value);
 void register_compat_prop(const char *driver, const char *property,
-- 
2.7.4

[Qemu-devel] [PATCH v3 03/13] qdev: remove qdev_prop_register_global()

[Peter Xu]

It works very similar to register_compat_prop() but does not need a
malloc, however mostly all the callers of it are doing malloc on their
own. Then it makes little sense to keep it.

Replacing all the callers with register_compat_prop(), meanwhile adding
one more parameter "user_provided" for it to make it even more
powerful (there is only one usage of it to be true).

This greatly reduces tens of LOCs and should have no functional change.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c            |  6 ++++--
 hw/core/qdev-properties.c    | 20 ++++++++++----------
 include/hw/qdev-properties.h |  6 +++---
 qom/cpu.c                    |  8 ++------
 target/i386/cpu.c            |  9 ++-------
 vl.c                         | 39 +++++++++------------------------------
 6 files changed, 30 insertions(+), 58 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index b4b3449..181f69d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -774,7 +774,8 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
 static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
 {
     GlobalProperty *p = opaque;
-    register_compat_prop(object_class_get_name(oc), p->property, p->value);
+    register_compat_prop(object_class_get_name(oc), p->property,
+                         p->value, false);
 }
 
 void machine_register_compat_props(MachineState *machine)
@@ -804,7 +805,8 @@ void machine_register_compat_props(MachineState *machine)
             object_class_foreach(machine_register_compat_for_subclass,
                                  p->driver, false, p);
         } else {
-            register_compat_prop(p->driver, p->property, p->value);
+            register_compat_prop(p->driver, p->property,
+                                 p->value, false);
         }
     }
 }
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index dc3b0ac..e9efb78 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1043,7 +1043,8 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 static GList *global_props;
 
 GList *global_prop_list_add(GList *list, const char *driver,
-                            const char *property, const char *value)
+                            const char *property, const char *value,
+                            bool user_provided)
 {
     GList *l;
     GlobalProperty *p;
@@ -1055,6 +1056,7 @@ GList *global_prop_list_add(GList *list, const char *driver,
             if (value) {
                 /* Modify existing value */
                 p->value = value;
+                p->user_provided = user_provided;
             } else {
                 /* Delete this entry entirely */
                 list = g_list_remove_link(list, l);
@@ -1078,21 +1080,18 @@ GList *global_prop_list_add(GList *list, const char *driver,
     p->driver = driver;
     p->property = property;
     p->value = value;
+    p->user_provided = user_provided;
 
     return g_list_append(list, p);
 }
 
 void register_compat_prop(const char *driver,
                           const char *property,
-                          const char *value)
+                          const char *value,
+                          bool user_provided)
 {
-    global_props = global_prop_list_add(global_props, driver,
-                                        property, value);
-}
-
-void qdev_prop_register_global(GlobalProperty *prop)
-{
-    global_props = g_list_append(global_props, prop);
+    global_props = global_prop_list_add(global_props, driver, property,
+                                        value, user_provided);
 }
 
 void qdev_prop_register_global_list(GlobalProperty *props)
@@ -1100,7 +1099,8 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     int i;
 
     for (i = 0; props[i].driver != NULL; i++) {
-        qdev_prop_register_global(props+i);
+        register_compat_prop(props[i].driver, props[i].property,
+                             props[i].value, props[i].user_provided);
     }
 }
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 55ad507..8a63c2e 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -194,7 +194,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 /* FIXME: Remove opaque pointer properties.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
-void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 int qdev_prop_check_globals(void);
 void qdev_prop_set_globals(DeviceState *dev);
@@ -212,9 +211,10 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
  *   - if value == NULL, delete the entry
  */
 GList *global_prop_list_add(GList *list, const char *driver,
-                            const char *property, const char *value);
+                            const char *property, const char *value,
+                            bool user_provided);
 void register_compat_prop(const char *driver, const char *property,
-                          const char *value);
+                          const char *value, bool user_provided);
 
 /**
  * qdev_property_add_static:
diff --git a/qom/cpu.c b/qom/cpu.c
index 5069876..1027d90 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -342,14 +342,10 @@ static void cpu_common_parse_features(const char *typename, char *features,
     while (featurestr) {
         val = strchr(featurestr, '=');
         if (val) {
-            GlobalProperty *prop = g_new0(typeof(*prop), 1);
             *val = 0;
             val++;
-            prop->driver = typename;
-            prop->property = g_strdup(featurestr);
-            prop->value = g_strdup(val);
-            prop->errp = &error_fatal;
-            qdev_prop_register_global(prop);
+            register_compat_prop(typename, g_strdup(featurestr),
+                                 g_strdup(val), false);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
                        featurestr);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b2b1d20..1bd20e2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2039,7 +2039,6 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
         const char *val = NULL;
         char *eq = NULL;
         char num[32];
-        GlobalProperty *prop;
 
         /* Compatibility syntax: */
         if (featurestr[0] == '+') {
@@ -2091,12 +2090,8 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
             name = "tsc-frequency";
         }
 
-        prop = g_new0(typeof(*prop), 1);
-        prop->driver = typename;
-        prop->property = g_strdup(name);
-        prop->value = g_strdup(val);
-        prop->errp = &error_fatal;
-        qdev_prop_register_global(prop);
+        register_compat_prop(typename, g_strdup(name),
+                             g_strdup(val), false);
     }
 
     if (ambiguous) {
diff --git a/vl.c b/vl.c
index 32db19e..c58ffc0 100644
--- a/vl.c
+++ b/vl.c
@@ -891,13 +891,8 @@ static void configure_rtc(QemuOpts *opts)
     value = qemu_opt_get(opts, "driftfix");
     if (value) {
         if (!strcmp(value, "slew")) {
-            static GlobalProperty slew_lost_ticks = {
-                .driver   = "mc146818rtc",
-                .property = "lost_tick_policy",
-                .value    = "slew",
-            };
-
-            qdev_prop_register_global(&slew_lost_ticks);
+            register_compat_prop("mc146818rtc", "lost_tick_policy",
+                                 "slew", false);
         } else if (!strcmp(value, "none")) {
             /* discard is default */
         } else {
@@ -2945,15 +2940,9 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
 
 static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
-    GlobalProperty *g;
-
-    g = g_malloc0(sizeof(*g));
-    g->driver   = qemu_opt_get(opts, "driver");
-    g->property = qemu_opt_get(opts, "property");
-    g->value    = qemu_opt_get(opts, "value");
-    g->user_provided = true;
-    g->errp = &error_fatal;
-    qdev_prop_register_global(g);
+    register_compat_prop(qemu_opt_get(opts, "driver"),
+                         qemu_opt_get(opts, "property"),
+                         qemu_opt_get(opts, "value"), true);
     return 0;
 }
 
@@ -3689,13 +3678,8 @@ int main(int argc, char **argv, char **envp)
                 win2k_install_hack = 1;
                 break;
             case QEMU_OPTION_rtc_td_hack: {
-                static GlobalProperty slew_lost_ticks = {
-                    .driver   = "mc146818rtc",
-                    .property = "lost_tick_policy",
-                    .value    = "slew",
-                };
-
-                qdev_prop_register_global(&slew_lost_ticks);
+                register_compat_prop("mc146818rtc", "lost_tick_policy",
+                                     "slew", false);
                 break;
             }
             case QEMU_OPTION_acpitable:
@@ -3746,15 +3730,10 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
             case QEMU_OPTION_no_kvm_pit_reinjection: {
-                static GlobalProperty kvm_pit_lost_tick_policy = {
-                    .driver   = "kvm-pit",
-                    .property = "lost_tick_policy",
-                    .value    = "discard",
-                };
-
                 error_report("warning: deprecated, replaced by "
                              "-global kvm-pit.lost_tick_policy=discard");
-                qdev_prop_register_global(&kvm_pit_lost_tick_policy);
+                register_compat_prop("kvm-pit", "lost_tick_policy",
+                                     "discard", false);
                 break;
             }
             case QEMU_OPTION_accel: {
-- 
2.7.4

[Qemu-devel] [PATCH v3 04/13] accel: introduce AccelState.global_props

[Peter Xu]

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

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel.c                | 12 ++++++++++++
 include/sysemu/accel.h | 11 +++++++++++
 vl.c                   |  1 +
 3 files changed, 24 insertions(+)

diff --git a/accel.c b/accel.c
index 62edd29..f747d65 100644
--- a/accel.c
+++ b/accel.c
@@ -130,6 +130,18 @@ void configure_accelerator(MachineState *ms)
     }
 }
 
+static void accel_prop_pass_to_global(gpointer data, gpointer user_data)
+{
+    GlobalProperty *prop = data;
+
+    register_compat_prop(prop->driver, prop->property,
+                         prop->value, false);
+}
+
+void accel_register_compat_props(AccelState *accel)
+{
+    g_list_foreach(accel->global_props, accel_prop_pass_to_global, NULL);
+}
 
 static void tcg_accel_class_init(ObjectClass *oc, void *data)
 {
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 15944c1..83bb60e 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -28,6 +28,15 @@
 typedef struct AccelState {
     /*< private >*/
     Object parent_obj;
+
+    /*< public >*/
+    /*
+     * Global properties that would be applied when specific
+     * accelerator is chosen. It works just like
+     * MachineState.compat_props but it's for accelerators not
+     * machines.
+     */
+    GList *global_props;
 } AccelState;
 
 typedef struct AccelClass {
@@ -57,5 +66,7 @@ typedef struct AccelClass {
 extern int tcg_tb_size;
 
 void configure_accelerator(MachineState *ms);
+/* Register accelerator specific global properties */
+void accel_register_compat_props(AccelState *accel);
 
 #endif
diff --git a/vl.c b/vl.c
index c58ffc0..d3f5594 100644
--- a/vl.c
+++ b/vl.c
@@ -4553,6 +4553,7 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
+    accel_register_compat_props(current_machine->accelerator);
     machine_register_compat_props(current_machine);
 
     qemu_opts_foreach(qemu_find_opts("global"),
-- 
2.7.4

[Qemu-devel] [PATCH v3 05/13] tests: avoid check GlobalProperty.used

[Peter Xu]

After previous patch to let qdev_prop_register_global_list() use
register_compat_prop(), then caller won't be guaranteed that the
GlobalProperty pointer passed in will be the one linked to global_props
list (now we always alloc new GlobalProperty for it). So prop->used will
never be set.

Let's avoid checking this in test codes to make the test pass.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/test-qdev-global-props.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 48e5b73..599bb13 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -218,12 +218,6 @@ static void test_dynamic_globalprop_subprocess(void)
     g_assert_cmpuint(mt->prop2, ==, 102);
     all_used = qdev_prop_check_globals();
     g_assert_cmpuint(all_used, ==, 1);
-    g_assert(props[0].used);
-    g_assert(props[1].used);
-    g_assert(!props[2].used);
-    g_assert(!props[3].used);
-    g_assert(!props[4].used);
-    g_assert(!props[5].used);
 }
 
 static void test_dynamic_globalprop(void)
@@ -263,12 +257,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
     g_assert_cmpuint(mt->prop2, ==, 102);
     all_used = qdev_prop_check_globals();
     g_assert_cmpuint(all_used, ==, 0);
-    g_assert(props[0].used);
-    g_assert(props[1].used);
-    g_assert(!props[2].used);
-    g_assert(!props[3].used);
-    g_assert(!props[4].used);
-    g_assert(!props[5].used);
 }
 
 static void test_dynamic_globalprop_nouser(void)
-- 
2.7.4

[Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Peter Xu]

Let KVM be the first user of the new AccelState.global_props field.
Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
anything else yet.

There will be a change on how these global properties are applied for
TYPE_X86_CPU devices. The general workflow of the global property stuff
for TYPE_X86_CPU devices can be simplied as following (this is a example
routine of KVM that contains both old/new workflow, similar thing apply
to TCG, but even simpler):

   - HW_COMPAT/type_init() magic played before even entering main() [1]
   - main() in vl.c
     - configure_accelerator()
       - AccelClass.init_machine() [2]
         - kvm_init() (for KVM accel)
     - register global properties
       - accel_register_compat_props(): register accel compat props [3]
       - machine_register_compat_props(): register machine compat
         props (here we'll apply all the HW_COMPAT magic into
         global_props) [4]
     - machine init()
       - cpu init() [5]
       - ...

Before this patch, the code setup TYPE_X86_CPU properties at [4] by
keeping the kvm_default_props list and apply them directly to the device
using x86_cpu_apply_props().

After this patch, the code setup the same properties in the sequence of
[1]->[2]->[3][4]->[5]:

  - At [1] we setup machine global properties.  Note: there will be
    properties that with value==NULL but it's okay - when it was applied
    to global list, it'll be used to remove an entry at step [4], it
    works just like the old kvm_default_props, but we just unified it to
    a single place, which is the global_props list.

  - At [2] we setup accel global properties.

  - At [3]/[4] we move machine/accel properties into global property
    list. One thing to mention is that, we do [3] before [4], since we
    have some cross-relation properties, e.g., property that is required
    when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
    properties are still in machine compat properties.

  - At [5] when TYPE_X86_CPU creates, it applies the global property from
    the global property list, which is now a merged list of three: accel
    property list, machine property list, and user specified "-global"
    properties.

So it's getting more complex in workflow, but better modulized.

After the refactoring, x86_cpu_change_kvm_default() is moved directly to
pc_piix.c since that'll be the only place to use it, also it is
rewritten to use the global property APIs.

One defect is that we hard coded TYPE_X86_CPU (both of its definitions,
for 32/64 bits) in accel_register_x86_cpu_props(). Comment explains
itself.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel.c                | 22 ++++++++++++++++++++++
 hw/i386/pc_piix.c      |  8 ++++++++
 include/sysemu/accel.h |  9 +++++++++
 kvm-all.c              | 30 ++++++++++++++++++++++++++++++
 target/i386/cpu.c      | 42 +-----------------------------------------
 target/i386/cpu.h      | 11 -----------
 vl.c                   |  5 +++++
 7 files changed, 75 insertions(+), 52 deletions(-)

diff --git a/accel.c b/accel.c
index f747d65..ca1d0f5 100644
--- a/accel.c
+++ b/accel.c
@@ -130,6 +130,28 @@ void configure_accelerator(MachineState *ms)
     }
 }
 
+void accel_register_prop(AccelState *accel, const char *driver,
+                         const char *prop, const char *value)
+{
+    accel->global_props = global_prop_list_add(accel->global_props,
+                                               driver, prop,
+                                               value, false);
+}
+
+void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
+                                  const char *value)
+{
+    /*
+     * We hard-coded this for x86 cpus, trying to match TYPE_X86_CPU.
+     * We cannot just use TYPE_X86_CPU since that's target-dependent
+     * while accel.c is target-independent. For x86 platform, only one
+     * of below two lines will be used, but it does not hurt to
+     * register both. On non-x86 platforms, none of them are used.
+     */
+    accel_register_prop(accel, "i386-cpu", prop, value);
+    accel_register_prop(accel, "x86_64-cpu", prop, value);
+}
+
 static void accel_prop_pass_to_global(gpointer data, gpointer user_data)
 {
     GlobalProperty *prop = data;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 46a2bc4..d1d8979 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -304,6 +304,14 @@ static void pc_init1(MachineState *machine,
     }
 }
 
+static void x86_cpu_change_kvm_default(const char *prop,
+                                       const char *value)
+{
+    if (kvm_enabled()) {
+        register_compat_prop(TYPE_X86_CPU, prop, value, false);
+    }
+}
+
 /* Looking for a pc_compat_2_4() function? It doesn't exist.
  * pc_compat_*() functions that run on machine-init time and
  * change global QEMU state are deprecated. Please don't create
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 83bb60e..ee2fbad 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -65,8 +65,17 @@ typedef struct AccelClass {
 
 extern int tcg_tb_size;
 
+typedef struct AccelPropValue {
+    const char *prop, *value;
+} AccelPropValue;
+
 void configure_accelerator(MachineState *ms);
 /* Register accelerator specific global properties */
 void accel_register_compat_props(AccelState *accel);
+/* Register single global property to the AccessState property list */
+void accel_register_prop(AccelState *accel, const char *driver,
+                         const char *prop, const char *value);
+void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
+                                  const char *value);
 
 #endif
diff --git a/kvm-all.c b/kvm-all.c
index ab8262f..ef60ddf 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1564,6 +1564,34 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
     return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }
 
+static AccelPropValue x86_kvm_default_props[] = {
+    { "kvmclock", "on" },
+    { "kvm-nopiodelay", "on" },
+    { "kvm-asyncpf", "on" },
+    { "kvm-steal-time", "on" },
+    { "kvm-pv-eoi", "on" },
+    { "kvmclock-stable-bit", "on" },
+    { "x2apic", "on" },
+    { "acpi", "off" },
+    { "monitor", "off" },
+    { "svm", "off" },
+    { NULL, NULL },
+};
+
+static void kvm_register_accel_props(KVMState *kvm)
+{
+    AccelState *accel = ACCEL(kvm);
+    AccelPropValue *entry;
+
+    for (entry = x86_kvm_default_props; entry->prop; entry++) {
+        accel_register_x86_cpu_props(accel, entry->prop, entry->value);
+    }
+
+    if (!kvm_irqchip_in_kernel()) {
+        accel_register_x86_cpu_props(accel, "x2apic", "off");
+    }
+}
+
 static int kvm_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -1776,6 +1804,8 @@ static int kvm_init(MachineState *ms)
 
     cpu_interrupt_handler = kvm_handle_interrupt;
 
+    kvm_register_accel_props(s);
+
     return 0;
 
 err:
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1bd20e2..5214fba 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1484,23 +1484,6 @@ typedef struct PropValue {
     const char *prop, *value;
 } PropValue;
 
-/* KVM-specific features that are automatically added/removed
- * from all CPU models when KVM is enabled.
- */
-static PropValue kvm_default_props[] = {
-    { "kvmclock", "on" },
-    { "kvm-nopiodelay", "on" },
-    { "kvm-asyncpf", "on" },
-    { "kvm-steal-time", "on" },
-    { "kvm-pv-eoi", "on" },
-    { "kvmclock-stable-bit", "on" },
-    { "x2apic", "on" },
-    { "acpi", "off" },
-    { "monitor", "off" },
-    { "svm", "off" },
-    { NULL, NULL },
-};
-
 /* TCG-specific defaults that override all CPU models when using TCG
  */
 static PropValue tcg_default_props[] = {
@@ -1508,23 +1491,6 @@ static PropValue tcg_default_props[] = {
     { NULL, NULL },
 };
 
-
-void x86_cpu_change_kvm_default(const char *prop, const char *value)
-{
-    PropValue *pv;
-    for (pv = kvm_default_props; pv->prop; pv++) {
-        if (!strcmp(pv->prop, prop)) {
-            pv->value = value;
-            break;
-        }
-    }
-
-    /* It is valid to call this function only for properties that
-     * are already present in the kvm_default_props table.
-     */
-    assert(pv->prop);
-}
-
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only);
 
@@ -2335,13 +2301,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     }
 
     /* Special cases not set in the X86CPUDefinition structs: */
-    if (kvm_enabled()) {
-        if (!kvm_irqchip_in_kernel()) {
-            x86_cpu_change_kvm_default("x2apic", "off");
-        }
-
-        x86_cpu_apply_props(cpu, kvm_default_props);
-    } else if (tcg_enabled()) {
+    if (tcg_enabled()) {
         x86_cpu_apply_props(cpu, tcg_default_props);
     }
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index de0551f..93aebfb 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1669,17 +1669,6 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 
-
-/* Change the value of a KVM-specific default
- *
- * If value is NULL, no default will be set and the original
- * value from the CPU model table will be kept.
- *
- * It is valid to call this function only for properties that
- * are already present in the kvm_default_props table.
- */
-void x86_cpu_change_kvm_default(const char *prop, const char *value);
-
 /* mpx_helper.c */
 void cpu_sync_bndcs_hflags(CPUX86State *env);
 
diff --git a/vl.c b/vl.c
index d3f5594..a564139 100644
--- a/vl.c
+++ b/vl.c
@@ -4553,6 +4553,11 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
+    /*
+     * Here we need to first register accelerator compat properties
+     * then machine properties, since cross-relationed properties are
+     * setup in machine compat bits.
+     */
     accel_register_compat_props(current_machine->accelerator);
     machine_register_compat_props(current_machine);
 
-- 
2.7.4

[Qemu-devel] [PATCH v3 07/13] tcg: use AccelState.global_props

[Peter Xu]

Let tcg use the new AccelState.global_props as well (used similar trick
for the kvm convertion in previous patch). Basically we are moving the
tcg_default_props into tcg codes, and link these compat properties onto
newly created AccelState.global_props.

This is much simpler than KVM.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel.c           | 15 +++++++++++++++
 target/i386/cpu.c | 28 ----------------------------
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/accel.c b/accel.c
index ca1d0f5..f4c2b10 100644
--- a/accel.c
+++ b/accel.c
@@ -38,9 +38,24 @@
 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)
+{
+    AccelPropValue *entry;
+
+    for (entry = x86_tcg_default_props; entry->prop; entry++) {
+        accel_register_x86_cpu_props(accel, entry->prop, entry->value);
+    }
+}
+
 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,
-- 
2.7.4

[Qemu-devel] [PATCH v3 08/13] trace: add qdev_global_prop_apply

[Peter Xu]

Trace global property applying. Now there are machine compat properties,
accel compat properties, and user specified global properties. It
provides a way to trace those properties that really applied.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 Makefile.objs             | 1 +
 hw/core/qdev-properties.c | 2 ++
 hw/core/trace-events      | 2 ++
 3 files changed, 5 insertions(+)
 create mode 100644 hw/core/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 0575802..e199f5d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -121,6 +121,7 @@ trace-events-subdirs += migration
 trace-events-subdirs += block
 trace-events-subdirs += backends
 trace-events-subdirs += chardev
+trace-events-subdirs += hw/core
 trace-events-subdirs += hw/block
 trace-events-subdirs += hw/block/dataplane
 trace-events-subdirs += hw/char
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e9efb78..69c3f15 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -10,6 +10,7 @@
 #include "net/hub.h"
 #include "qapi/visitor.h"
 #include "chardev/char.h"
+#include "trace.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -1150,6 +1151,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
+        trace_qdev_global_prop_apply(typename, prop->property, prop->value);
         prop->used = true;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
diff --git a/hw/core/trace-events b/hw/core/trace-events
new file mode 100644
index 0000000..0a0a79f
--- /dev/null
+++ b/hw/core/trace-events
@@ -0,0 +1,2 @@
+# hw/core/qdev-properties.c
+qdev_global_prop_apply(const char *type, const char *prop, const char *value) "type '%s' prop '%s' value '%s'"
-- 
2.7.4

[Qemu-devel] [PATCH v3 09/13] 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.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 69 +++++++++++++++++++++++++++++++++++----------------
 migration/migration.h | 19 ++++++++++++++
 2 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b9d8798..4ba1212 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -101,29 +101,13 @@ enum mig_rp_message_type {
 /* 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)
@@ -1992,3 +1976,46 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = false;
+}
+
+static void migration_instance_init(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+
+    ms->state = MIGRATION_STATUS_NONE;
+    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
+    ms->mbps = -1;
+    ms->parameters = (MigrationParameters) {
+        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
+        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+        .max_bandwidth = MAX_THROTTLE,
+        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
+        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+    };
+    ms->parameters.tls_creds = g_strdup("");
+    ms->parameters.tls_hostname = g_strdup("");
+}
+
+static const TypeInfo migration_type = {
+    .name = TYPE_MIGRATION,
+    .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
+    .class_size = sizeof(MigrationClass),
+    .instance_size = sizeof(MigrationState),
+    .instance_init = migration_instance_init,
+};
+
+static void register_migration_types(void)
+{
+    type_register_static(&migration_type);
+}
+
+type_init(register_migration_types);
diff --git a/migration/migration.h b/migration/migration.h
index d9a268a..3fca364 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -19,6 +19,7 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 /* State for the incoming migration */
 struct MigrationIncomingState {
@@ -62,8 +63,26 @@ struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
+#define TYPE_MIGRATION "migration"
+
+#define MIGRATION_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
+#define MIGRATION_OBJ(obj) \
+    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
+#define MIGRATION_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
+
+typedef struct MigrationClass {
+    /*< private >*/
+    DeviceClass parent_class;
+} MigrationClass;
+
 struct MigrationState
 {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
-- 
2.7.4

[Qemu-devel] [PATCH v3 10/13] migration: move global_state.optional out

[Peter Xu]

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

Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
for x86/power, and accel_register_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              | 5 ++++-
 include/hw/compat.h              | 4 ++++
 include/migration/global_state.h | 1 -
 migration/global_state.c         | 9 ++-------
 migration/migration.c            | 7 +++++++
 migration/migration.h            | 6 ++++++
 8 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index d1d8979..b662618 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -326,7 +326,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e877d45..edbdbfd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index d3fa705..186808b 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/xen/xen_backend.h"
+#include "hw/boards.h"
 #include "qmp-commands.h"
 #include "chardev/char.h"
 #include "sysemu/accel.h"
@@ -119,6 +120,8 @@ static void xen_change_state_handler(void *opaque, int running,
 
 static int xen_init(MachineState *ms)
 {
+    AccelState *accel = ms->accelerator;
+
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
         xen_pv_printf(NULL, 0, "can't open xen interface\n");
@@ -139,7 +142,7 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    global_state_set_optional();
+    accel_register_prop(accel, "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 26cd585..a506a74 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -181,6 +181,10 @@
         .driver   = TYPE_PCI_DEVICE,\
         .property = "x-pcie-lnksta-dllla",\
         .value    = "off",\
+    },{\
+        .driver   = "migration",\
+        .property = "store-global-state",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_2 \
diff --git a/include/migration/global_state.h b/include/migration/global_state.h
index 90faea7..d307de8 100644
--- a/include/migration/global_state.h
+++ b/include/migration/global_state.h
@@ -16,7 +16,6 @@
 #include "sysemu/sysemu.h"
 
 void register_global_state(void);
-void global_state_set_optional(void);
 int global_state_store(void);
 void global_state_store_running(void);
 bool global_state_received(void);
diff --git a/migration/global_state.c b/migration/global_state.c
index f792cf5..dcbbcb2 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -15,12 +15,12 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/util.h"
+#include "migration.h"
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
 typedef struct {
-    bool optional;
     uint32_t size;
     uint8_t runstate[100];
     RunState state;
@@ -57,11 +57,6 @@ RunState global_state_get_runstate(void)
     return global_state.state;
 }
 
-void global_state_set_optional(void)
-{
-    global_state.optional = true;
-}
-
 static bool global_state_needed(void *opaque)
 {
     GlobalState *s = opaque;
@@ -69,7 +64,7 @@ static bool global_state_needed(void *opaque)
 
     /* If it is not optional, it is mandatory */
 
-    if (s->optional == false) {
+    if (migrate_get_current()->store_global_state) {
         return true;
     }
 
diff --git a/migration/migration.c b/migration/migration.c
index 4ba1212..095abd8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1976,11 +1976,18 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static Property migration_properties[] = {
+    DEFINE_PROP_BOOL("store-global-state", MigrationState,
+                     store_global_state, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void migration_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->user_creatable = false;
+    dc->props = migration_properties;
 }
 
 static void migration_instance_init(Object *obj)
diff --git a/migration/migration.h b/migration/migration.h
index 3fca364..4b898e9 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -133,6 +133,12 @@ struct MigrationState
     /* Do we have to clean up -b/-i from old migrate parameters */
     /* This feature is deprecated and will be removed */
     bool must_remove_block_options;
+
+    /*
+     * Global switch on whether we need to store the global state
+     * during migration.
+     */
+    bool store_global_state;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
-- 
2.7.4

[Qemu-devel] [PATCH v3 11/13] migration: move only_migratable to MigrationState

[Peter Xu]

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

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

  -global migration.only-migratable=true

Currently still keep the old interface.

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

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

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 65c7070..23e533f 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -54,4 +54,6 @@ bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
+void migration_only_migratable_set(bool only_migratable);
+
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9841a52..b213696 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -15,7 +15,6 @@
 /* vl.c */
 
 extern const char *bios_name;
-extern int only_migratable;
 extern const char *qemu_name;
 extern QemuUUID qemu_uuid;
 extern bool qemu_uuid_set;
diff --git a/migration/migration.c b/migration/migration.c
index 095abd8..9af5c76 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,6 +110,11 @@ MigrationState *migrate_get_current(void)
     return current_migration;
 }
 
+void migration_only_migratable_set(bool only_migratable)
+{
+    migrate_get_current()->only_migratable = only_migratable;
+}
+
 MigrationIncomingState *migration_incoming_get_current(void)
 {
     static bool once;
@@ -981,7 +986,7 @@ static GSList *migration_blockers;
 
 int migrate_add_blocker(Error *reason, Error **errp)
 {
-    if (only_migratable) {
+    if (migrate_get_current()->only_migratable) {
         error_propagate(errp, error_copy(reason));
         error_prepend(errp, "disallowing migration blocker "
                           "(--only_migratable) for: ");
@@ -1979,6 +1984,7 @@ void migrate_fd_connect(MigrationState *s)
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
+    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 4b898e9..34377dd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -139,6 +139,9 @@ struct MigrationState
      * during migration.
      */
     bool store_global_state;
+
+    /* Whether the VM is only allowing for migratable devices */
+    bool only_migratable;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index f32a82d..d8ea522 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2303,7 +2303,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 a564139..be2a918 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;
 
@@ -3916,7 +3915,13 @@ int main(int argc, char **argv, char **envp)
                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:
-                only_migratable = 1;
+                /*
+                 * TODO: we can remove this option one day, and we
+                 * should all use:
+                 *
+                 * "-global migration.only-migratable=true"
+                 */
+                migration_only_migratable_set(true);
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.7.4

[Qemu-devel] [PATCH v3 12/13] migration: move skip_configuration out

[Peter Xu]

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

Removing savevm_skip_configuration().

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b662618..537de7c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -326,7 +326,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    savevm_skip_configuration();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index edbdbfd..29fac1b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    savevm_skip_configuration();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 186808b..ac6a2b9 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -143,7 +143,7 @@ static int xen_init(MachineState *ms)
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
     accel_register_prop(accel, "migration", "store-global-state", "off");
-    savevm_skip_configuration();
+    accel_register_prop(accel, "migration", "send-configuration", "off");
     savevm_skip_section_footers();
 
     return 0;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index a506a74..1a3fd94 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -183,6 +183,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-configuration",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 23e533f..35b41bc 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -42,7 +42,6 @@ int64_t self_announce_delay(int round)
 
 void dump_vmstate_json_to_file(FILE *out_fp);
 void savevm_skip_section_footers(void);
-void savevm_skip_configuration(void);
 
 /* migration/migration.c */
 void qemu_start_incoming_migration(const char *uri, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index 9af5c76..f69fe28 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1985,6 +1985,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
+    DEFINE_PROP_BOOL("send-configuration", MigrationState,
+                     send_configuration, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 34377dd..4d4ea0d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -142,6 +142,9 @@ struct MigrationState
 
     /* Whether the VM is only allowing for migratable devices */
     bool only_migratable;
+
+    /* Whether we send QEMU_VM_CONFIGURATION during migration */
+    bool send_configuration;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index d8ea522..f678a8a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -287,7 +287,6 @@ typedef struct SaveStateEntry {
 typedef struct SaveState {
     QTAILQ_HEAD(, SaveStateEntry) handlers;
     int global_section_id;
-    bool skip_configuration;
     uint32_t len;
     const char *name;
     uint32_t target_page_bits;
@@ -296,15 +295,8 @@ typedef struct SaveState {
 static SaveState savevm_state = {
     .handlers = QTAILQ_HEAD_INITIALIZER(savevm_state.handlers),
     .global_section_id = 0,
-    .skip_configuration = false,
 };
 
-void savevm_skip_configuration(void)
-{
-    savevm_state.skip_configuration = true;
-}
-
-
 static void configuration_pre_save(void *opaque)
 {
     SaveState *state = opaque;
@@ -970,11 +962,11 @@ void qemu_savevm_state_header(QEMUFile *f)
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (migrate_get_current()->send_configuration ||
+        enforce_config_section()) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
-
 }
 
 void qemu_savevm_state_begin(QEMUFile *f)
@@ -1984,7 +1976,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (migrate_get_current()->send_configuration ||
+        enforce_config_section()) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
-- 
2.7.4

[Qemu-devel] [PATCH v3 13/13] migration: move skip_section_footers

[Peter Xu]

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

Removing savevm_skip_section_footers().

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 537de7c..04e2b2e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -322,7 +322,6 @@ static void x86_cpu_change_kvm_default(const char *prop,
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 29fac1b..7c40fdf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3579,7 +3579,6 @@ DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
 static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
-    savevm_skip_section_footers();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index ac6a2b9..069458c 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -144,7 +144,7 @@ static int xen_init(MachineState *ms)
 
     accel_register_prop(accel, "migration", "store-global-state", "off");
     accel_register_prop(accel, "migration", "send-configuration", "off");
-    savevm_skip_section_footers();
+    accel_register_prop(accel, "migration", "send-section-footer", "off");
 
     return 0;
 }
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 1a3fd94..08f3600 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -187,6 +187,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-section-footer",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 35b41bc..114f032 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -41,7 +41,6 @@ int64_t self_announce_delay(int round)
 /* migration/savevm.c */
 
 void dump_vmstate_json_to_file(FILE *out_fp);
-void savevm_skip_section_footers(void);
 
 /* migration/migration.c */
 void qemu_start_incoming_migration(const char *uri, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index f69fe28..c06c7ff 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1987,6 +1987,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("send-configuration", MigrationState,
                      send_configuration, true),
+    DEFINE_PROP_BOOL("send-section-footer", MigrationState,
+                     send_section_footer, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 4d4ea0d..994b017 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -145,6 +145,8 @@ struct MigrationState
 
     /* Whether we send QEMU_VM_CONFIGURATION during migration */
     bool send_configuration;
+    /* Whether we send section footer during migration */
+    bool send_section_footer;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index f678a8a..df22f90 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -62,8 +62,6 @@
 
 const unsigned int postcopy_ram_discard_version = 0;
 
-static bool skip_section_footers;
-
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
     MIG_CMD_INVALID = 0,   /* Must be 0 */
@@ -761,11 +759,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
     vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
-void savevm_skip_section_footers(void)
-{
-    skip_section_footers = true;
-}
-
 /*
  * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
  */
@@ -793,7 +786,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
  */
 static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
 {
-    if (!skip_section_footers) {
+    if (migrate_get_current()->send_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
     }
@@ -1791,7 +1784,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
     uint8_t read_mark;
     uint32_t read_section_id;
 
-    if (skip_section_footers) {
+    if (!migrate_get_current()->send_section_footer) {
         /* No footer to check */
         return true;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 01/13] machine: export register_compat_prop()

[Laurent Vivier]

On 19/06/2017 14:49, Peter Xu 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.
> 
> Meanwhile, move it to qdev-properties.c where seems more proper.
> Refactor it a bit to provide global_prop_list_add() for further
> use (will use it in cases where we need a GlobalProperty list).
> 
> 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>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  accel.c                      |  1 +
>  hw/core/machine.c            | 14 +-------------
>  hw/core/qdev-properties.c    | 22 ++++++++++++++++++++++
>  include/hw/qdev-properties.h |  5 +++++
>  4 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/accel.c b/accel.c
> index 664bb88..62edd29 100644
> --- a/accel.c
> +++ b/accel.c
> @@ -26,6 +26,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/accel.h"
>  #include "hw/boards.h"
> +#include "hw/qdev-properties.h"
>  #include "qemu-common.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2e7e977..b4b3449 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -16,6 +16,7 @@
>  #include "qapi-visit.h"
>  #include "qapi/visitor.h"
>  #include "hw/sysbus.h"
> +#include "hw/qdev-properties.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/numa.h"
>  #include "qemu/error-report.h"
> @@ -770,19 +771,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>      g_free(mc->name);
>  }
>  
> -static void register_compat_prop(const char *driver,
> -                                 const char *property,
> -                                 const char *value)
> -{
> -    GlobalProperty *p = g_new0(GlobalProperty, 1);
> -    /* Machine compat_props must never cause errors: */
> -    p->errp = &error_abort;
> -    p->driver = driver;
> -    p->property = property;
> -    p->value = value;
> -    qdev_prop_register_global(p);
> -}
> -
>  static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
>  {
>      GlobalProperty *p = opaque;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 9f1a497..4b74382 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1042,6 +1042,28 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
>  
>  static GList *global_props;
>  
> +GList *global_prop_list_add(GList *list, const char *driver,
> +                            const char *property, const char *value)
> +{
> +    GlobalProperty *p = g_new0(GlobalProperty, 1);
> +
> +    /* These properties cannot fail the apply */
> +    p->errp = &error_abort;
> +    p->driver = driver;
> +    p->property = property;
> +    p->value = value;
> +
> +    return g_list_append(list, p);
> +}
> +
> +void register_compat_prop(const char *driver,
> +                          const char *property,
> +                          const char *value)
> +{
> +    global_props = global_prop_list_add(global_props, driver,
> +                                        property, value);
> +}
> +
>  void qdev_prop_register_global(GlobalProperty *prop)
>  {
>      global_props = g_list_append(global_props, prop);
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index d206fc9..15ee6ba 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -201,6 +201,11 @@ void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
> +GList *global_prop_list_add(GList *list, const char *driver,
> +                            const char *property, const char *value);
> +void register_compat_prop(const char *driver, const char *property,
> +                          const char *value);
> +
>  /**
>   * qdev_property_add_static:
>   * @dev: Device to add the property to.
> 

Re: [Qemu-devel] [PATCH v3 02/13] qdev: enhance global_prop_list_add()

[Eduardo Habkost]

On Mon, Jun 19, 2017 at 08:49:37PM +0800, Peter Xu wrote:
> Originally it can only alloc new entries and insert. Let it be smarter
> that it can update existing fields, and even delete elements. This is
> preparation of (finally) the replacement of x86_cpu_apply_props(). If
> you see x86_cpu_apply_props(), it allows to skip entries when value is
> NULL. Here, it works just like deleting an existing entry.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

This makes the global property API much more complex, making the
interactions between different callers much more subtle.

Currently, the global property list was always an append-only
list, and we had only two possible sources of global properties:
 1) MachineClass::compat_props;
 2) User-provided globals (including -global and other
   command-line options that are simply translated to globals).
So it is easy to calculate the results of a given QEMU
configuration: just append both lists.

With this patch, the rules become more complex, and calculating
the actual results of multiple register_compat_prop() calls is
not trivial.  Tracking the actual register_compat_prop() calls
scattered around the code (and figuring out the order in which
they are called) makes it even more difficult.  IMO, this defeats
the purpose of introducing a AccelClass::global_props field.

I believe the main source of this complexity are the
x86_cpu_change_kvm_default() calls in pc_piix.c, and the rules
those calls represent are really more complex.  But I would like
to find a simpler solution to represent those rules inside either
MachineClass::compat_props or AccelClass::global_props.  If we
can't do that, I think we should keep that complexity inside the
PC/x86 code instead of exposing it to all users of the global
properties API.

I suggest we do this, to keep things simple in the first version:

1) Introduce AccelClass::global_props without this patch.
2) Move: kvmclock, kvm-nopiodelay, kvm-asyncpf, kvm-steal-time,
   kvmclock-stable-bit, acpi, monitor to AccelClass::global_props
   in kvm-accel.
3) Move: vme=off to AccelClass::global_props in tcg-accel.
4) Keep svm, x2apic and kvm-pv-eoi inside kvm_default_props in
   x86 code, because their rules are more complex.
5) Look for a simpler way to represent the rules from (4) later.


> ---
>  hw/core/qdev-properties.c    | 28 +++++++++++++++++++++++++++-
>  include/hw/qdev-properties.h | 10 ++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 4b74382..dc3b0ac 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1045,7 +1045,33 @@ static GList *global_props;
>  GList *global_prop_list_add(GList *list, const char *driver,
>                              const char *property, const char *value)
>  {
> -    GlobalProperty *p = g_new0(GlobalProperty, 1);
> +    GList *l;
> +    GlobalProperty *p;
> +
> +    /* Look up the (property, value) first in the list */
> +    for (l = list; l; l = l->next) {
> +        p = l->data;
> +        if (!strcmp(driver, p->driver) && !strcmp(property, p->property)) {
> +            if (value) {
> +                /* Modify existing value */
> +                p->value = value;
> +            } else {
> +                /* Delete this entry entirely */
> +                list = g_list_remove_link(list, l);
> +                g_free(p);
> +                g_list_free(l);
> +            }
> +            return list;
> +        }
> +    }
> +
> +    /* Entry not exist. If we are deleting one entry, we're done. */
> +    if (!value) {
> +        return list;
> +    }
> +
> +    /* If not found and value is set, we try to create a new entry */
> +    p = g_new0(GlobalProperty, 1);
>  
>      /* These properties cannot fail the apply */
>      p->errp = &error_abort;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 15ee6ba..55ad507 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -201,6 +201,16 @@ void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
> +/*
> + * Register global property on the list. Elements of list should be
> + * GlobalProperty.
> + *
> + * - If (driver, property) is not existing on the list, create a new
> + *   one and link to the list.
> + * - If (driver, property) exists on the list, then:
> + *   - if value != NULL, update with new value
> + *   - if value == NULL, delete the entry
> + */
>  GList *global_prop_list_add(GList *list, const char *driver,
>                              const char *property, const char *value);
>  void register_compat_prop(const char *driver, const char *property,
> -- 
> 2.7.4
> 

-- 
Eduardo

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

[Juan Quintela]

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

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

Re: [Qemu-devel] [PATCH v3 11/13] 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.
>
> Hmm, now vl.c has no way to access migrate_get_current(). Export a
> function for it to setup only_migratable.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

If you have to respin

+void migration_only_migratable_set(bool only_migratable);
+

only is used with "true".  I think that it is easier without parameter.

But it is just a question of taste, so, as you are the one doing the
code ....

Later, Juan.

Re: [Qemu-devel] [PATCH v3 12/13] migration: move skip_configuration out

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It was in SaveState but now moved to MigrationState altogether, reverted
> its meaning, then renamed to "send_configuration". Again, using
> HW_COMPAT_2_3 for old PC/SPAPR machines, and accel_register_prop() for
> xen_init().
>
> Removing savevm_skip_configuration().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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


> -    if (!savevm_state.skip_configuration || enforce_config_section()) {
> +    if (migrate_get_current()->send_configuration ||
> +        enforce_config_section()) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
>          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>      }

It is a different problem, but this patch makes enforce_config_section
optional, no?  We can get the same behaviour with this new option,
right?

Looking at the code, it is not clear to me if it is easier to put
enforce_config_option on top of this patch, or let things as they are.

Opinions?

Thanks, Juan.

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Move it into MigrationState, revert its meaning and renaming it to
> send_section_footer, with a property bound to it. Same trick is played
> like previous patches.
>
> Removing savevm_skip_section_footers().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 00/13] migration: objectify MigrationState

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> v3 contains too much new things. So here comes a new cover letter with
> richer information.
>
> The main goal of this series is to let MigrationState be a QDev.
>
> It helps in many use cases.

Hi

I haven't reviewed the qdev part of the patches (it is a landmine for
me).  But I like the result of it.

Well done, Juan.

Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Eduardo Habkost]

On Mon, Jun 19, 2017 at 08:49:41PM +0800, Peter Xu wrote:
> Let KVM be the first user of the new AccelState.global_props field.
> Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
> anything else yet.
> 
> There will be a change on how these global properties are applied for
> TYPE_X86_CPU devices. The general workflow of the global property stuff
> for TYPE_X86_CPU devices can be simplied as following (this is a example
> routine of KVM that contains both old/new workflow, similar thing apply
> to TCG, but even simpler):
> 
>    - HW_COMPAT/type_init() magic played before even entering main() [1]

What do you mean by this?  HW_COMPAT_* is used only in
MachineClass::compat_props[4], and type_init() magic is triggered
by module_call_init(MODULE_INIT_QOM) in main().

>    - main() in vl.c
>      - configure_accelerator()
>        - AccelClass.init_machine() [2]
>          - kvm_init() (for KVM accel)
>      - register global properties
>        - accel_register_compat_props(): register accel compat props [3]
>        - machine_register_compat_props(): register machine compat
>          props (here we'll apply all the HW_COMPAT magic into
>          global_props) [4]
>      - machine init()
>        - cpu init() [5]
>        - ...
> 
> Before this patch, the code setup TYPE_X86_CPU properties at [4] by
> keeping the kvm_default_props list and apply them directly to the device
> using x86_cpu_apply_props().
> 
> After this patch, the code setup the same properties in the sequence of
> [1]->[2]->[3][4]->[5]:
> 
>   - At [1] we setup machine global properties.  Note: there will be
>     properties that with value==NULL but it's okay - when it was applied
>     to global list, it'll be used to remove an entry at step [4], it
>     works just like the old kvm_default_props, but we just unified it to
>     a single place, which is the global_props list.
> 
>   - At [2] we setup accel global properties.

Why isn't AccelClass::global_props set up at class_init(), just
like we do on MachineClass::compat_props?

> 
>   - At [3]/[4] we move machine/accel properties into global property
>     list. One thing to mention is that, we do [3] before [4], since we
>     have some cross-relation properties, e.g., property that is required
>     when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
>     properties are still in machine compat properties.
> 
>   - At [5] when TYPE_X86_CPU creates, it applies the global property from
>     the global property list, which is now a merged list of three: accel
>     property list, machine property list, and user specified "-global"
>     properties.

On which category above would the x86_cpu_change_kvm_default()
calls in pc_piix.c would be?  We would need to ensure they
override the globals registered by the accel code, but they must
not override the user-provided global properties (including
-global and -cpu options).

This is where things get tricky and fragile: the translation from
-cpu to global properties is done very late inside machine init
today, but we should be able to do that much earlier, once we
refactor the -cpu parsing code.

Hence my suggestion is to not touch x86_cpu_change_kvm_default()
and just move the other properties (everything in
kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
AccelClass::global_props field.

> 
> So it's getting more complex in workflow, but better modulized.
> 
> After the refactoring, x86_cpu_change_kvm_default() is moved directly to
> pc_piix.c since that'll be the only place to use it, also it is
> rewritten to use the global property APIs.
> 
> One defect is that we hard coded TYPE_X86_CPU (both of its definitions,
> for 32/64 bits) in accel_register_x86_cpu_props(). Comment explains
> itself.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  accel.c                | 22 ++++++++++++++++++++++
>  hw/i386/pc_piix.c      |  8 ++++++++
>  include/sysemu/accel.h |  9 +++++++++
>  kvm-all.c              | 30 ++++++++++++++++++++++++++++++
>  target/i386/cpu.c      | 42 +-----------------------------------------
>  target/i386/cpu.h      | 11 -----------
>  vl.c                   |  5 +++++
>  7 files changed, 75 insertions(+), 52 deletions(-)
> 
> diff --git a/accel.c b/accel.c
> index f747d65..ca1d0f5 100644
> --- a/accel.c
> +++ b/accel.c
> @@ -130,6 +130,28 @@ void configure_accelerator(MachineState *ms)
>      }
>  }
>  
> +void accel_register_prop(AccelState *accel, const char *driver,
> +                         const char *prop, const char *value)
> +{
> +    accel->global_props = global_prop_list_add(accel->global_props,
> +                                               driver, prop,
> +                                               value, false);
> +}

I don't think we need yet another layer of indirection where
global properties are registered at runtime in AccelState before
being actually registered as global properties.

I expected this to be

  void accel_class_append_global_prop(AccelClass *acc,
                                      const char *driver,
                                      const char *prop,
                                      const char *value);

And the only places calling accel_class_append_global_prop()
would be the accel classes' class_init functions.

I would even consider making AccelClass::global_props an array,
instead of a linked list.  It would help making sure the data is
really static.

> +
> +void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
> +                                  const char *value)
> +{
> +    /*
> +     * We hard-coded this for x86 cpus, trying to match TYPE_X86_CPU.
> +     * We cannot just use TYPE_X86_CPU since that's target-dependent
> +     * while accel.c is target-independent. For x86 platform, only one
> +     * of below two lines will be used, but it does not hurt to
> +     * register both. On non-x86 platforms, none of them are used.
> +     */
> +    accel_register_prop(accel, "i386-cpu", prop, value);
> +    accel_register_prop(accel, "x86_64-cpu", prop, value);

I don't know why we need this helper.  The list of
(driver, property, value) tuples could be hardcoded inside the
initialization of AccelClass::global_props, just like we already
do in MachineClass::compat_props.

> +}
> +
>  static void accel_prop_pass_to_global(gpointer data, gpointer user_data)
>  {
>      GlobalProperty *prop = data;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 46a2bc4..d1d8979 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -304,6 +304,14 @@ static void pc_init1(MachineState *machine,
>      }
>  }
>  
> +static void x86_cpu_change_kvm_default(const char *prop,
> +                                       const char *value)
> +{
> +    if (kvm_enabled()) {
> +        register_compat_prop(TYPE_X86_CPU, prop, value, false);
> +    }

This will be called very late, and can override -cpu options if
we refactor -cpu option parsing in the future.

(It is hard to ensure we have the ordering right if we have too
many register_compat_prop() calls scattered around the code.)


> +}
> +
>  /* Looking for a pc_compat_2_4() function? It doesn't exist.
>   * pc_compat_*() functions that run on machine-init time and
>   * change global QEMU state are deprecated. Please don't create
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 83bb60e..ee2fbad 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -65,8 +65,17 @@ typedef struct AccelClass {
>  
>  extern int tcg_tb_size;
>  
> +typedef struct AccelPropValue {
> +    const char *prop, *value;

Why not add a "driver" field here?

> +} AccelPropValue;
> +
>  void configure_accelerator(MachineState *ms);
>  /* Register accelerator specific global properties */
>  void accel_register_compat_props(AccelState *accel);
> +/* Register single global property to the AccessState property list */
> +void accel_register_prop(AccelState *accel, const char *driver,
> +                         const char *prop, const char *value);
> +void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
> +                                  const char *value);
>  
>  #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index ab8262f..ef60ddf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1564,6 +1564,34 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
>      return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
>  }
>  
> +static AccelPropValue x86_kvm_default_props[] = {
> +    { "kvmclock", "on" },
> +    { "kvm-nopiodelay", "on" },
> +    { "kvm-asyncpf", "on" },
> +    { "kvm-steal-time", "on" },
> +    { "kvm-pv-eoi", "on" },
> +    { "kvmclock-stable-bit", "on" },
> +    { "x2apic", "on" },
> +    { "acpi", "off" },
> +    { "monitor", "off" },
> +    { "svm", "off" },
> +    { NULL, NULL },

If we add a 'driver' field here, we won't need the
accel_register_x86_cpu_props() helper anymore.

> +};
> +
> +static void kvm_register_accel_props(KVMState *kvm)
> +{
> +    AccelState *accel = ACCEL(kvm);
> +    AccelPropValue *entry;
> +
> +    for (entry = x86_kvm_default_props; entry->prop; entry++) {
> +        accel_register_x86_cpu_props(accel, entry->prop, entry->value);
> +    }

I suggest:

* Moving the list inside AccelClass instead of AccelState.
* Doing this inside kvm_accel_class_init() instead.
* Not dealing with x2apic right now, as its rules are more
  complex.

> +
> +    if (!kvm_irqchip_in_kernel()) {
> +        accel_register_x86_cpu_props(accel, "x2apic", "off");
> +    }
> +}
> +
>  static int kvm_init(MachineState *ms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -1776,6 +1804,8 @@ static int kvm_init(MachineState *ms)
>  
>      cpu_interrupt_handler = kvm_handle_interrupt;
>  
> +    kvm_register_accel_props(s);
> +
>      return 0;
>  
>  err:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1bd20e2..5214fba 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1484,23 +1484,6 @@ typedef struct PropValue {
>      const char *prop, *value;
>  } PropValue;
>  
> -/* KVM-specific features that are automatically added/removed
> - * from all CPU models when KVM is enabled.
> - */
> -static PropValue kvm_default_props[] = {
> -    { "kvmclock", "on" },
> -    { "kvm-nopiodelay", "on" },
> -    { "kvm-asyncpf", "on" },
> -    { "kvm-steal-time", "on" },
> -    { "kvm-pv-eoi", "on" },
> -    { "kvmclock-stable-bit", "on" },
> -    { "x2apic", "on" },
> -    { "acpi", "off" },
> -    { "monitor", "off" },
> -    { "svm", "off" },
> -    { NULL, NULL },
> -};
> -
>  /* TCG-specific defaults that override all CPU models when using TCG
>   */
>  static PropValue tcg_default_props[] = {
> @@ -1508,23 +1491,6 @@ static PropValue tcg_default_props[] = {
>      { NULL, NULL },
>  };
>  
> -
> -void x86_cpu_change_kvm_default(const char *prop, const char *value)
> -{
> -    PropValue *pv;
> -    for (pv = kvm_default_props; pv->prop; pv++) {
> -        if (!strcmp(pv->prop, prop)) {
> -            pv->value = value;
> -            break;
> -        }
> -    }
> -
> -    /* It is valid to call this function only for properties that
> -     * are already present in the kvm_default_props table.
> -     */
> -    assert(pv->prop);
> -}
> -
>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                     bool migratable_only);
>  
> @@ -2335,13 +2301,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>      }
>  
>      /* Special cases not set in the X86CPUDefinition structs: */
> -    if (kvm_enabled()) {
> -        if (!kvm_irqchip_in_kernel()) {
> -            x86_cpu_change_kvm_default("x2apic", "off");
> -        }
> -
> -        x86_cpu_apply_props(cpu, kvm_default_props);
> -    } else if (tcg_enabled()) {
> +    if (tcg_enabled()) {
>          x86_cpu_apply_props(cpu, tcg_default_props);
>      }
>  
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index de0551f..93aebfb 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1669,17 +1669,6 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>                                     TPRAccess access);
>  
> -
> -/* Change the value of a KVM-specific default
> - *
> - * If value is NULL, no default will be set and the original
> - * value from the CPU model table will be kept.
> - *
> - * It is valid to call this function only for properties that
> - * are already present in the kvm_default_props table.
> - */
> -void x86_cpu_change_kvm_default(const char *prop, const char *value);
> -
>  /* mpx_helper.c */
>  void cpu_sync_bndcs_hflags(CPUX86State *env);
>  
> diff --git a/vl.c b/vl.c
> index d3f5594..a564139 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4553,6 +4553,11 @@ int main(int argc, char **argv, char **envp)
>              exit (i == 1 ? 1 : 0);
>      }
>  
> +    /*
> +     * Here we need to first register accelerator compat properties
> +     * then machine properties, since cross-relationed properties are
> +     * setup in machine compat bits.
> +     */
>      accel_register_compat_props(current_machine->accelerator);
>      machine_register_compat_props(current_machine);

Note for later: It would be very nice if this was the only place
in the code where qdev_prop_register_global()/register_compat_prop()
is called.  Probably a register_user_provided_compat_props() call
here would make sure the ordering is always right.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 04/13] accel: introduce AccelState.global_props

[Eduardo Habkost]

On Mon, Jun 19, 2017 at 08:49:39PM +0800, Peter Xu wrote:
> Introduce this new field for the accelerator instances so that each
> specific accelerator in the future can register its own global
> properties to be used further by the system.  It works just like how the
> old machine compatible properties do, but only tailored for
> accelerators.
> 
> Use the newly exported register_compat_prop() to pass the accelerator
> global properties to the global_props list.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

As noted on other patches, I believe this should be
AccelClass::global_props (initialized statically in class_init)
instead of AccelState::global_props (built at runtime).
Otherwise, I don't see the benefit of the new field.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 04/13] accel: introduce AccelState.global_props

[Peter Xu]

On Mon, Jun 19, 2017 at 01:17:39PM -0300, Eduardo Habkost wrote:
> On Mon, Jun 19, 2017 at 08:49:39PM +0800, Peter Xu wrote:
> > Introduce this new field for the accelerator instances so that each
> > specific accelerator in the future can register its own global
> > properties to be used further by the system.  It works just like how the
> > old machine compatible properties do, but only tailored for
> > accelerators.
> > 
> > Use the newly exported register_compat_prop() to pass the accelerator
> > global properties to the global_props list.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> As noted on other patches, I believe this should be
> AccelClass::global_props (initialized statically in class_init)
> instead of AccelState::global_props (built at runtime).
> Otherwise, I don't see the benefit of the new field.

The reason is that there is property that can only be defined after
user specified all the parameters (when kvm irqchip is enabled). See:

static void kvm_register_accel_props(KVMState *kvm)
{
    AccelState *accel = ACCEL(kvm);
    AccelPropValue *entry;

    for (entry = x86_kvm_default_props; entry->prop; entry++) {
        accel_register_x86_cpu_props(accel, entry->prop, entry->value);
    }

    if (!kvm_irqchip_in_kernel()) {
        accel_register_x86_cpu_props(accel, "x2apic", "off");
    }
}

So the list is not really static when class init.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 04/13] accel: introduce AccelState.global_props

[Eduardo Habkost]

On Tue, Jun 20, 2017 at 09:20:36PM +0800, Peter Xu wrote:
> On Mon, Jun 19, 2017 at 01:17:39PM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 19, 2017 at 08:49:39PM +0800, Peter Xu wrote:
> > > Introduce this new field for the accelerator instances so that each
> > > specific accelerator in the future can register its own global
> > > properties to be used further by the system.  It works just like how the
> > > old machine compatible properties do, but only tailored for
> > > accelerators.
> > > 
> > > Use the newly exported register_compat_prop() to pass the accelerator
> > > global properties to the global_props list.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > As noted on other patches, I believe this should be
> > AccelClass::global_props (initialized statically in class_init)
> > instead of AccelState::global_props (built at runtime).
> > Otherwise, I don't see the benefit of the new field.
> 
> The reason is that there is property that can only be defined after
> user specified all the parameters (when kvm irqchip is enabled). See:
> 
> static void kvm_register_accel_props(KVMState *kvm)
> {
>     AccelState *accel = ACCEL(kvm);
>     AccelPropValue *entry;
> 
>     for (entry = x86_kvm_default_props; entry->prop; entry++) {
>         accel_register_x86_cpu_props(accel, entry->prop, entry->value);
>     }
> 
>     if (!kvm_irqchip_in_kernel()) {
>         accel_register_x86_cpu_props(accel, "x2apic", "off");
>     }
> }
> 
> So the list is not really static when class init.  Thanks,

I suggest leaving x2apic for later, and address the ones that are
static first.  I'm not sure a dynamic list on AccelState is the
right solution for x2apic.  I still wish to find a way to
represent the x2apic/svm/kvm-pv-eoi rules using static data.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Peter Xu]

On Mon, Jun 19, 2017 at 01:14:03PM -0300, Eduardo Habkost wrote:
> On Mon, Jun 19, 2017 at 08:49:41PM +0800, Peter Xu wrote:
> > Let KVM be the first user of the new AccelState.global_props field.
> > Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
> > anything else yet.
> > 
> > There will be a change on how these global properties are applied for
> > TYPE_X86_CPU devices. The general workflow of the global property stuff
> > for TYPE_X86_CPU devices can be simplied as following (this is a example
> > routine of KVM that contains both old/new workflow, similar thing apply
> > to TCG, but even simpler):
> > 
> >    - HW_COMPAT/type_init() magic played before even entering main() [1]
> 
> What do you mean by this?  HW_COMPAT_* is used only in
> MachineClass::compat_props[4], and type_init() magic is triggered
> by module_call_init(MODULE_INIT_QOM) in main().

Sorry. It should be the thing you mentioned.

> 
> >    - main() in vl.c
> >      - configure_accelerator()
> >        - AccelClass.init_machine() [2]
> >          - kvm_init() (for KVM accel)
> >      - register global properties
> >        - accel_register_compat_props(): register accel compat props [3]
> >        - machine_register_compat_props(): register machine compat
> >          props (here we'll apply all the HW_COMPAT magic into
> >          global_props) [4]
> >      - machine init()
> >        - cpu init() [5]
> >        - ...
> > 
> > Before this patch, the code setup TYPE_X86_CPU properties at [4] by
> > keeping the kvm_default_props list and apply them directly to the device
> > using x86_cpu_apply_props().
> > 
> > After this patch, the code setup the same properties in the sequence of
> > [1]->[2]->[3][4]->[5]:
> > 
> >   - At [1] we setup machine global properties.  Note: there will be
> >     properties that with value==NULL but it's okay - when it was applied
> >     to global list, it'll be used to remove an entry at step [4], it
> >     works just like the old kvm_default_props, but we just unified it to
> >     a single place, which is the global_props list.
> > 
> >   - At [2] we setup accel global properties.
> 
> Why isn't AccelClass::global_props set up at class_init(), just
> like we do on MachineClass::compat_props?

(explained in other thread: there is a property we only need to
 register when split irqchip is used)

> 
> > 
> >   - At [3]/[4] we move machine/accel properties into global property
> >     list. One thing to mention is that, we do [3] before [4], since we
> >     have some cross-relation properties, e.g., property that is required
> >     when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
> >     properties are still in machine compat properties.
> > 
> >   - At [5] when TYPE_X86_CPU creates, it applies the global property from
> >     the global property list, which is now a merged list of three: accel
> >     property list, machine property list, and user specified "-global"
> >     properties.
> 
> On which category above would the x86_cpu_change_kvm_default()
> calls in pc_piix.c would be?  We would need to ensure they
> override the globals registered by the accel code, but they must
> not override the user-provided global properties (including
> -global and -cpu options).

Oh I didn't realize this difference. x86_cpu_change_kvm_default() is
called in [5]. It indeed breaks this rule.

> 
> This is where things get tricky and fragile: the translation from
> -cpu to global properties is done very late inside machine init
> today, but we should be able to do that much earlier, once we
> refactor the -cpu parsing code.
> 
> Hence my suggestion is to not touch x86_cpu_change_kvm_default()
> and just move the other properties (everything in
> kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
> AccelClass::global_props field.

Yes it's fragile and complicated.

How about this:

I introduce AccelClass::global_props, only use it in Xen but nowhere
else? After all, what I really want to do is just let migration codes
start to use "-global" properties and compatibility fields. And if
there is still no good idea to ideally solve this x86 cpu property
issue, I would prefer to keep it (it'll also be simpler for me).

Another thing worries me a bit is that I may make things more
confusing if I separate this list into two (then we'll have part of
the properties in accel code, and the rest ones still in cpu.c).

(then I can also avoid using hard code in accel.c/kvm.c as well, which
 is something I really want to stop from doing. Maybe there can be
 some better idea, but I cannot really figure it out now...)

I'll just hold here to see whether you like above idea before moving
on to further comments.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Eduardo Habkost]

On Tue, Jun 20, 2017 at 09:55:03PM +0800, Peter Xu wrote:
> On Mon, Jun 19, 2017 at 01:14:03PM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 19, 2017 at 08:49:41PM +0800, Peter Xu wrote:
> > > Let KVM be the first user of the new AccelState.global_props field.
> > > Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
> > > anything else yet.
> > > 
> > > There will be a change on how these global properties are applied for
> > > TYPE_X86_CPU devices. The general workflow of the global property stuff
> > > for TYPE_X86_CPU devices can be simplied as following (this is a example
> > > routine of KVM that contains both old/new workflow, similar thing apply
> > > to TCG, but even simpler):
> > > 
> > >    - HW_COMPAT/type_init() magic played before even entering main() [1]
> > 
> > What do you mean by this?  HW_COMPAT_* is used only in
> > MachineClass::compat_props[4], and type_init() magic is triggered
> > by module_call_init(MODULE_INIT_QOM) in main().
> 
> Sorry. It should be the thing you mentioned.
> 
> > 
> > >    - main() in vl.c
> > >      - configure_accelerator()
> > >        - AccelClass.init_machine() [2]
> > >          - kvm_init() (for KVM accel)
> > >      - register global properties
> > >        - accel_register_compat_props(): register accel compat props [3]
> > >        - machine_register_compat_props(): register machine compat
> > >          props (here we'll apply all the HW_COMPAT magic into
> > >          global_props) [4]
> > >      - machine init()
> > >        - cpu init() [5]
> > >        - ...
> > > 
> > > Before this patch, the code setup TYPE_X86_CPU properties at [4] by
> > > keeping the kvm_default_props list and apply them directly to the device
> > > using x86_cpu_apply_props().
> > > 
> > > After this patch, the code setup the same properties in the sequence of
> > > [1]->[2]->[3][4]->[5]:
> > > 
> > >   - At [1] we setup machine global properties.  Note: there will be
> > >     properties that with value==NULL but it's okay - when it was applied
> > >     to global list, it'll be used to remove an entry at step [4], it
> > >     works just like the old kvm_default_props, but we just unified it to
> > >     a single place, which is the global_props list.
> > > 
> > >   - At [2] we setup accel global properties.
> > 
> > Why isn't AccelClass::global_props set up at class_init(), just
> > like we do on MachineClass::compat_props?
> 
> (explained in other thread: there is a property we only need to
>  register when split irqchip is used)
> 
> > 
> > > 
> > >   - At [3]/[4] we move machine/accel properties into global property
> > >     list. One thing to mention is that, we do [3] before [4], since we
> > >     have some cross-relation properties, e.g., property that is required
> > >     when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
> > >     properties are still in machine compat properties.
> > > 
> > >   - At [5] when TYPE_X86_CPU creates, it applies the global property from
> > >     the global property list, which is now a merged list of three: accel
> > >     property list, machine property list, and user specified "-global"
> > >     properties.
> > 
> > On which category above would the x86_cpu_change_kvm_default()
> > calls in pc_piix.c would be?  We would need to ensure they
> > override the globals registered by the accel code, but they must
> > not override the user-provided global properties (including
> > -global and -cpu options).
> 
> Oh I didn't realize this difference. x86_cpu_change_kvm_default() is
> called in [5]. It indeed breaks this rule.
> 
> > 
> > This is where things get tricky and fragile: the translation from
> > -cpu to global properties is done very late inside machine init
> > today, but we should be able to do that much earlier, once we
> > refactor the -cpu parsing code.
> > 
> > Hence my suggestion is to not touch x86_cpu_change_kvm_default()
> > and just move the other properties (everything in
> > kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
> > AccelClass::global_props field.
> 
> Yes it's fragile and complicated.
> 
> How about this:
> 
> I introduce AccelClass::global_props, only use it in Xen but nowhere
> else? After all, what I really want to do is just let migration codes
> start to use "-global" properties and compatibility fields. And if
> there is still no good idea to ideally solve this x86 cpu property
> issue, I would prefer to keep it (it'll also be simpler for me).

Sounds good to me.

> 
> Another thing worries me a bit is that I may make things more
> confusing if I separate this list into two (then we'll have part of
> the properties in accel code, and the rest ones still in cpu.c).
> 
> (then I can also avoid using hard code in accel.c/kvm.c as well, which
>  is something I really want to stop from doing. Maybe there can be
>  some better idea, but I cannot really figure it out now...)
> 
> I'll just hold here to see whether you like above idea before moving
> on to further comments.  Thanks,


Agreed.

When I suggested using accel-provided global properties to
replace kvm_default_props, I forgot x86_cpu_change_kvm_default()
existed, and it makes things much more complex.

I really really want to make the existing x2apic/svm/kvm-pv-eoi
compat stuff be based on static lists of properties.  If we make
them dynamically built at runtime, we still can't introspect them
and it won't be worth the extra complexity.

I believe we can still find a solution to represent the
x2apic/svm/kvm-pv-eoi rules using static lists, but this
shouldn't block the migration work you are doing.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Peter Xu]

On Tue, Jun 20, 2017 at 11:07:34AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 20, 2017 at 09:55:03PM +0800, Peter Xu wrote:
> > On Mon, Jun 19, 2017 at 01:14:03PM -0300, Eduardo Habkost wrote:

[...]

> > > This is where things get tricky and fragile: the translation from
> > > -cpu to global properties is done very late inside machine init
> > > today, but we should be able to do that much earlier, once we
> > > refactor the -cpu parsing code.
> > > 
> > > Hence my suggestion is to not touch x86_cpu_change_kvm_default()
> > > and just move the other properties (everything in
> > > kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
> > > AccelClass::global_props field.
> > 
> > Yes it's fragile and complicated.
> > 
> > How about this:
> > 
> > I introduce AccelClass::global_props, only use it in Xen but nowhere
> > else? After all, what I really want to do is just let migration codes
> > start to use "-global" properties and compatibility fields. And if
> > there is still no good idea to ideally solve this x86 cpu property
> > issue, I would prefer to keep it (it'll also be simpler for me).
> 
> Sounds good to me.

Thanks for the confirmation. Let me cook another simpler series then.

> 
> > 
> > Another thing worries me a bit is that I may make things more
> > confusing if I separate this list into two (then we'll have part of
> > the properties in accel code, and the rest ones still in cpu.c).
> > 
> > (then I can also avoid using hard code in accel.c/kvm.c as well, which
> >  is something I really want to stop from doing. Maybe there can be
> >  some better idea, but I cannot really figure it out now...)
> > 
> > I'll just hold here to see whether you like above idea before moving
> > on to further comments.  Thanks,
> 
> 
> Agreed.
> 
> When I suggested using accel-provided global properties to
> replace kvm_default_props, I forgot x86_cpu_change_kvm_default()
> existed, and it makes things much more complex.
> 
> I really really want to make the existing x2apic/svm/kvm-pv-eoi
> compat stuff be based on static lists of properties.  If we make
> them dynamically built at runtime, we still can't introspect them
> and it won't be worth the extra complexity.
> 
> I believe we can still find a solution to represent the
> x2apic/svm/kvm-pv-eoi rules using static lists, but this
> shouldn't block the migration work you are doing.

One thing I think of is:

Let MachineClass::compat_props be MachCompatProp (rather than
GlobalProperty), then define it as:

struct MachCompatProp {
    GlobalProperty prop;
    bool (*prop_valid)();
};

(We may pass MigrationState *ms into prop_valid(), but for current
 requirements we may not need that, since what we need is basically
 tcg_enabled(), kvm_enabled(), or kvm_irqchip_is_split() checks)

Then this property will only be delivered to the global_props list
only if its prop_valid() check pass.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 11/13] migration: move only_migratable to MigrationState

[Peter Xu]

On Mon, Jun 19, 2017 at 05:50:50PM +0200, Juan Quintela wrote:
> 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.
> >
> > Hmm, now vl.c has no way to access migrate_get_current(). Export a
> > function for it to setup only_migratable.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> If you have to respin

So there will be. :-)

> 
> +void migration_only_migratable_set(bool only_migratable);
> +
> 
> only is used with "true".  I think that it is easier without parameter.
> 
> But it is just a question of taste, so, as you are the one doing the
> code ....

Sure.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 12/13] migration: move skip_configuration out

[Peter Xu]

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

Thanks!

> 
> 
> > -    if (!savevm_state.skip_configuration || enforce_config_section()) {
> > +    if (migrate_get_current()->send_configuration ||
> > +        enforce_config_section()) {
> >          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> >          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
> >      }
> 
> It is a different problem, but this patch makes enforce_config_section
> optional, no?  We can get the same behaviour with this new option,
> right?
> 
> Looking at the code, it is not clear to me if it is easier to put
> enforce_config_option on top of this patch, or let things as they are.
> 
> Opinions?

There can be a very tiny difference. Consider this cmdline:

  qemu -M enforce-config-section=true -global migration.send-configuration=false

For current patch, enforce-config-section=true will make sure this
cmdline will send the config section no matter what we provided in
-global section.

If we remove the enforce_config_section() above and also use
MigrationState.send_configuration, the enforce-config-section=true
will be replaced by the latter send-configuration=false, then config
section will not be sent.

However, I'll vote for your suggestion since after all this cmdline is
not really making much sense (considering it's providing two conflict
confugrations). And anyone who starts to use "-global migration.*"
then he/she should know the possible side effect.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 12/13] migration: move skip_configuration out

[Juan Quintela]

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

>> It is a different problem, but this patch makes enforce_config_section
>> optional, no?  We can get the same behaviour with this new option,
>> right?
>> 
>> Looking at the code, it is not clear to me if it is easier to put
>> enforce_config_option on top of this patch, or let things as they are.
>> 
>> Opinions?
>
> There can be a very tiny difference. Consider this cmdline:
>
>   qemu -M enforce-config-section=true -global migration.send-configuration=false
>
> For current patch, enforce-config-section=true will make sure this
> cmdline will send the config section no matter what we provided in
> -global section.
>
> If we remove the enforce_config_section() above and also use
> MigrationState.send_configuration, the enforce-config-section=true
> will be replaced by the latter send-configuration=false, then config
> section will not be sent.
>
> However, I'll vote for your suggestion since after all this cmdline is
> not really making much sense (considering it's providing two conflict
> confugrations). And anyone who starts to use "-global migration.*"
> then he/she should know the possible side effect.

Agreed.

Thanks for taking this, Juan.

Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Eduardo Habkost]

On Wed, Jun 21, 2017 at 11:00:54AM +0800, Peter Xu wrote:
> On Tue, Jun 20, 2017 at 11:07:34AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 20, 2017 at 09:55:03PM +0800, Peter Xu wrote:
> > > On Mon, Jun 19, 2017 at 01:14:03PM -0300, Eduardo Habkost wrote:
[...]
> > 
> > > 
> > > Another thing worries me a bit is that I may make things more
> > > confusing if I separate this list into two (then we'll have part of
> > > the properties in accel code, and the rest ones still in cpu.c).
> > > 
> > > (then I can also avoid using hard code in accel.c/kvm.c as well, which
> > >  is something I really want to stop from doing. Maybe there can be
> > >  some better idea, but I cannot really figure it out now...)
> > > 
> > > I'll just hold here to see whether you like above idea before moving
> > > on to further comments.  Thanks,
> > 
> > 
> > Agreed.
> > 
> > When I suggested using accel-provided global properties to
> > replace kvm_default_props, I forgot x86_cpu_change_kvm_default()
> > existed, and it makes things much more complex.
> > 
> > I really really want to make the existing x2apic/svm/kvm-pv-eoi
> > compat stuff be based on static lists of properties.  If we make
> > them dynamically built at runtime, we still can't introspect them
> > and it won't be worth the extra complexity.
> > 
> > I believe we can still find a solution to represent the
> > x2apic/svm/kvm-pv-eoi rules using static lists, but this
> > shouldn't block the migration work you are doing.
> 
> One thing I think of is:
> 
> Let MachineClass::compat_props be MachCompatProp (rather than
> GlobalProperty), then define it as:
> 
> struct MachCompatProp {
>     GlobalProperty prop;
>     bool (*prop_valid)();
> };
> 
> (We may pass MigrationState *ms into prop_valid(), but for current
>  requirements we may not need that, since what we need is basically
>  tcg_enabled(), kvm_enabled(), or kvm_irqchip_is_split() checks)
> 
> Then this property will only be delivered to the global_props list
> only if its prop_valid() check pass.  Thanks,

The problem with a ->prop_valid() hook is that it's code, not
data.  It means we can't represent it in the reply of a QMP
introspection command, or allow equivalent behavior to be
reproduced using command-line options.

Device-specific rules like this can be solved by a boolean compat
property implemented by the device itself.  e.g.: a "auto-x2apic"
property in the x86 CPU code.

-- 
Eduardo