[PATCH v3 00/13] migration: objectify MigrationState

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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


Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
CC: Markus Armbruster <[hidden email]>
CC: Marcel Apfelbaum <[hidden email]>
Signed-off-by: Peter Xu <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

[PATCH v3 08/13] trace: add qdev_global_prop_apply

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

[PATCH v3 09/13] migration: let MigrationState be a qdev

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

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

No functional change at all.

Reviewed-by: Juan Quintela <[hidden email]>
Signed-off-by: Peter Xu <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by Peter Xu
Put it into MigrationState then we can use the properties to specify
whether to enable storing global state.

Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
for x86/power, and accel_register_prop() for xen_init().

Signed-off-by: Peter Xu <[hidden email]>
---
 hw/i386/pc_piix.c                | 1 -
 hw/ppc/spapr.c                   | 1 -
 hw/xen/xen-common.c              | 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


Reply | Threaded
Open this post in threaded view
|

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

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

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

  -global migration.only-migratable=true

Currently still keep the old interface.

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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Xu
In reply to this post by Peter Xu
It was in SaveState but now moved to MigrationState altogether, 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 <[hidden email]>
---
 hw/i386/pc_piix.c        |  1 -
 hw/ppc/spapr.c           |  1 -
 hw/xen/xen-common.c      |  2 +-
 include/hw/compat.h      |  4 ++++
 include/migration/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


Reply | Threaded
Open this post in threaded view
|

[PATCH v3 13/13] migration: move skip_section_footers

Peter Xu
In reply to this post by 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 <[hidden email]>
---
 hw/i386/pc_piix.c        |  1 -
 hw/ppc/spapr.c           |  1 -
 hw/xen/xen-common.c      |  2 +-
 include/hw/compat.h      |  4 ++++
 include/migration/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


Reply | Threaded
Open this post in threaded view
|

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

Laurent Vivier-5
In reply to this post by Peter Xu
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 <[hidden email]>
> CC: Markus Armbruster <[hidden email]>
> CC: Marcel Apfelbaum <[hidden email]>
> Signed-off-by: Peter Xu <[hidden email]>

Reviewed-by: Laurent Vivier <[hidden email]>

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


Reply | Threaded
Open this post in threaded view
|

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

Eduardo Habkost-2
In reply to this post by Peter Xu
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 <[hidden email]>

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

Reply | Threaded
Open this post in threaded view
|

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

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
>
> Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
> for x86/power, and accel_register_prop() for xen_init().
>

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

Reply | Threaded
Open this post in threaded view
|

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

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

> One less global variable, and it does only matter with migration.
>
> We keep the old "--only-migratable" option, but also now we support:
>
>   -global migration.only-migratable=true
>
> Currently still keep the old interface.
>
> 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 <[hidden email]>

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

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.

Reply | Threaded
Open this post in threaded view
|

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

Juan Quintela
In reply to this post by Peter Xu
Peter Xu <[hidden email]> wrote:
> It was in SaveState but now moved to MigrationState altogether, 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 <[hidden email]>

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


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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 13/13] migration: move skip_section_footers

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

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

12