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

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

[Peter Xu]

v5:
- add r-b for Juan on patch 4,5,7-10
- patch 2: introduce register_compat_props_array() that may be further
  used by machine codes [Eduardo]
- patch 2: fix english error [Eduardo]

v4:
- dropped lots of patches related to AccelState.global_props. Now I am
  using AccelClass.global_props, and only use it in Xen codes.
- one patch is added to clean up the global property registerations,
  with some comment to show the ordering.
- one patch is added to dump migration globals in "info migrate"
  unconditionally (last patch).
- changed all the following xen_init() patches to use
  AccelClass.global_props.
- (since most of the patches are either touched-up again, or threw
  away, so no much r-b added, only one for Juan on the only_migratable
  patch, anyway, thanks for reviewing the old codes!)

------------

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

It helps in many use cases.

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

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

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

  -global migration.postcopy=on

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

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

Please kindly review. Thanks.

Peter Xu (10):
  machine: export register_compat_prop()
  accel: introduce AccelClass.global_props
  vl: clean up global property registerations
  migration: let MigrationState be a qdev
  migration: move global_state.optional out
  migration: move only_migratable to MigrationState
  migration: move skip_configuration out
  migration: move skip_section_footers
  migration: merge enforce_config_section somewhat
  migration: hmp: dump globals

 accel/accel.c                    |   6 ++
 hmp.c                            |   3 +
 hw/core/machine.c                |  13 -----
 hw/core/qdev-properties.c        |  20 +++++++
 hw/i386/pc_piix.c                |   3 -
 hw/ppc/spapr.c                   |   3 -
 hw/xen/xen-common.c              |  25 +++++++--
 include/hw/compat.h              |  12 ++++
 include/hw/qdev-properties.h     |   4 ++
 include/migration/global_state.h |   1 -
 include/migration/misc.h         |   6 +-
 include/sysemu/accel.h           |  10 ++++
 include/sysemu/sysemu.h          |   1 -
 migration/global_state.c         |   9 +--
 migration/migration.c            | 118 +++++++++++++++++++++++++++++++--------
 migration/migration.h            |  33 +++++++++++
 migration/savevm.c               |  32 ++---------
 vl.c                             |  41 ++++++++++++--
 18 files changed, 250 insertions(+), 90 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v5 01/10] machine: export register_compat_prop()

[Peter Xu]

We have HW_COMPAT_*, however that's only bound to machines, not other
things (like accelerators).  Behind it, it was register_compat_prop()
that played the trick.  Let's export the function for further use
outside HW_COMPAT_* magic.

Meanwhile, move it to qdev-properties.c where seems more proper (since
it'll be used not only in machine codes).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c            | 13 -------------
 hw/core/qdev-properties.c    | 13 +++++++++++++
 include/hw/qdev-properties.h |  3 +++
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2e7e977..ecb5552 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -770,19 +770,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     g_free(mc->name);
 }
 
-static void register_compat_prop(const char *driver,
-                                 const char *property,
-                                 const char *value)
-{
-    GlobalProperty *p = g_new0(GlobalProperty, 1);
-    /* Machine compat_props must never cause errors: */
-    p->errp = &error_abort;
-    p->driver = driver;
-    p->property = property;
-    p->value = value;
-    qdev_prop_register_global(p);
-}
-
 static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
 {
     GlobalProperty *p = opaque;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 9f1a497..6ff1ac3 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1047,6 +1047,19 @@ void qdev_prop_register_global(GlobalProperty *prop)
     global_props = g_list_append(global_props, prop);
 }
 
+void register_compat_prop(const char *driver,
+                          const char *property,
+                          const char *value)
+{
+    GlobalProperty *p = g_new0(GlobalProperty, 1);
+    /* Machine compat_props must never cause errors: */
+    p->errp = &error_abort;
+    p->driver = driver;
+    p->property = property;
+    p->value = value;
+    qdev_prop_register_global(p);
+}
+
 void qdev_prop_register_global_list(GlobalProperty *props)
 {
     int i;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index d206fc9..1722ca4 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,6 +201,9 @@ void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
+void register_compat_prop(const char *driver, const char *property,
+                          const char *value);
+
 /**
  * qdev_property_add_static:
  * @dev: Device to add the property to.
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 01/10] machine: export register_compat_prop()

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 12:46:37PM +0800, Peter Xu wrote:
> We have HW_COMPAT_*, however that's only bound to machines, not other
> things (like accelerators).  Behind it, it was register_compat_prop()
> that played the trick.  Let's export the function for further use
> outside HW_COMPAT_* magic.
> 
> Meanwhile, move it to qdev-properties.c where seems more proper (since
> it'll be used not only in machine codes).
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/machine.c            | 13 -------------
>  hw/core/qdev-properties.c    | 13 +++++++++++++
>  include/hw/qdev-properties.h |  3 +++
>  3 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2e7e977..ecb5552 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -770,19 +770,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>      g_free(mc->name);
>  }
>  
> -static void register_compat_prop(const char *driver,
> -                                 const char *property,
> -                                 const char *value)
> -{
> -    GlobalProperty *p = g_new0(GlobalProperty, 1);
> -    /* Machine compat_props must never cause errors: */
> -    p->errp = &error_abort;
> -    p->driver = driver;
> -    p->property = property;
> -    p->value = value;
> -    qdev_prop_register_global(p);
> -}
> -
>  static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
>  {
>      GlobalProperty *p = opaque;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 9f1a497..6ff1ac3 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1047,6 +1047,19 @@ void qdev_prop_register_global(GlobalProperty *prop)
>      global_props = g_list_append(global_props, prop);
>  }
>  
> +void register_compat_prop(const char *driver,
> +                          const char *property,
> +                          const char *value)
> +{
> +    GlobalProperty *p = g_new0(GlobalProperty, 1);
> +    /* Machine compat_props must never cause errors: */

Now this won't be used only for machine compat_props, so this comment
needs to be deleted or updated (see suggestion below).


> +    p->errp = &error_abort;
> +    p->driver = driver;
> +    p->property = property;
> +    p->value = value;
> +    qdev_prop_register_global(p);
> +}
> +
>  void qdev_prop_register_global_list(GlobalProperty *props)
>  {
>      int i;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index d206fc9..1722ca4 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -201,6 +201,9 @@ void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  

As this function is very easy to be misused, I suggest including the
following documentation:

/**
 * register_compat_prop:
 *
 * Register internal (not user-provided) global property, changing the
 * default value of a given property in a device type.  This can be used
 * for enabling machine-type compatibility or for enabling
 * accelerator-specific defaults in devices.
 *
 * The property values set using this function must be always valid and
 * never report setter errors, as they property have
 * GlobalProperty::errp set to &error_abort.
 *
 * User-provided global properties should override internal global
 * properties, so callers of this function should ensure that it is
 * called before user-provided global properties are registered.
 *
 * @driver: Device type to be affected
 * @property: Property whose default value is going to be changed
 * @value: New default value for the property
 */

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 01/10] machine: export register_compat_prop()

[Peter Xu]

On Fri, Jun 23, 2017 at 06:24:32PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:37PM +0800, Peter Xu wrote:
> > We have HW_COMPAT_*, however that's only bound to machines, not other
> > things (like accelerators).  Behind it, it was register_compat_prop()
> > that played the trick.  Let's export the function for further use
> > outside HW_COMPAT_* magic.
> > 
> > Meanwhile, move it to qdev-properties.c where seems more proper (since
> > it'll be used not only in machine codes).
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/core/machine.c            | 13 -------------
> >  hw/core/qdev-properties.c    | 13 +++++++++++++
> >  include/hw/qdev-properties.h |  3 +++
> >  3 files changed, 16 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 2e7e977..ecb5552 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -770,19 +770,6 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
> >      g_free(mc->name);
> >  }
> >  
> > -static void register_compat_prop(const char *driver,
> > -                                 const char *property,
> > -                                 const char *value)
> > -{
> > -    GlobalProperty *p = g_new0(GlobalProperty, 1);
> > -    /* Machine compat_props must never cause errors: */
> > -    p->errp = &error_abort;
> > -    p->driver = driver;
> > -    p->property = property;
> > -    p->value = value;
> > -    qdev_prop_register_global(p);
> > -}
> > -
> >  static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
> >  {
> >      GlobalProperty *p = opaque;
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 9f1a497..6ff1ac3 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1047,6 +1047,19 @@ void qdev_prop_register_global(GlobalProperty *prop)
> >      global_props = g_list_append(global_props, prop);
> >  }
> >  
> > +void register_compat_prop(const char *driver,
> > +                          const char *property,
> > +                          const char *value)
> > +{
> > +    GlobalProperty *p = g_new0(GlobalProperty, 1);
> > +    /* Machine compat_props must never cause errors: */
> 
> Now this won't be used only for machine compat_props, so this comment
> needs to be deleted or updated (see suggestion below).

Yes. Fixing.

> 
> 
> > +    p->errp = &error_abort;
> > +    p->driver = driver;
> > +    p->property = property;
> > +    p->value = value;
> > +    qdev_prop_register_global(p);
> > +}
> > +
> >  void qdev_prop_register_global_list(GlobalProperty *props)
> >  {
> >      int i;
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index d206fc9..1722ca4 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -201,6 +201,9 @@ void qdev_prop_set_globals(DeviceState *dev);
> >  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> >                                      Property *prop, const char *value);
> >  
> 
> As this function is very easy to be misused, I suggest including the
> following documentation:
> 
> /**
>  * register_compat_prop:
>  *
>  * Register internal (not user-provided) global property, changing the
>  * default value of a given property in a device type.  This can be used
>  * for enabling machine-type compatibility or for enabling
>  * accelerator-specific defaults in devices.
>  *
>  * The property values set using this function must be always valid and
>  * never report setter errors, as they property have
>  * GlobalProperty::errp set to &error_abort.
>  *
>  * User-provided global properties should override internal global
>  * properties, so callers of this function should ensure that it is
>  * called before user-provided global properties are registered.
>  *
>  * @driver: Device type to be affected
>  * @property: Property whose default value is going to be changed
>  * @value: New default value for the property
>  */

Sure. Will take it directly. Thanks!

> 
> > +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
> > 
> > 
> 
> -- 
> Eduardo

-- 
Peter Xu

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

[Peter Xu]

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

Introduce register_compat_props_array() for it. Export it so that it may
be used in other codes as well in the future.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/accel.c                |  6 ++++++
 hw/core/qdev-properties.c    |  7 +++++++
 include/hw/qdev-properties.h |  1 +
 include/sysemu/accel.h       | 10 ++++++++++
 vl.c                         |  1 +
 5 files changed, 25 insertions(+)

diff --git a/accel/accel.c b/accel/accel.c
index 7c079a5..fa85844 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -120,6 +120,12 @@ void configure_accelerator(MachineState *ms)
     }
 }
 
+void accel_register_compat_props(AccelState *accel)
+{
+    AccelClass *class = ACCEL_GET_CLASS(accel);
+    register_compat_props_array(class->global_props);
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 6ff1ac3..19fa335 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1060,6 +1060,13 @@ void register_compat_prop(const char *driver,
     qdev_prop_register_global(p);
 }
 
+void register_compat_props_array(GlobalProperty *prop)
+{
+    for (; prop && prop->driver; prop++) {
+        register_compat_prop(prop->driver, prop->property, prop->value);
+    }
+}
+
 void qdev_prop_register_global_list(GlobalProperty *props)
 {
     int i;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 1722ca4..2bac6a0 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -203,6 +203,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
 
 void register_compat_prop(const char *driver, const char *property,
                           const char *value);
+void register_compat_props_array(GlobalProperty *prop);
 
 /**
  * qdev_property_add_static:
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 15944c1..effcd7f 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -24,6 +24,7 @@
 #define HW_ACCEL_H
 
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
 
 typedef struct AccelState {
     /*< private >*/
@@ -40,6 +41,13 @@ typedef struct AccelClass {
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
     bool *allowed;
+    /*
+     * Array of global properties that would be applied when specific
+     * accelerator is chosen. It works just like
+     * MachineClass.compat_props but it's for accelerators not
+     * machines.
+     */
+    GlobalProperty *global_props;
 } AccelClass;
 
 #define TYPE_ACCEL "accel"
@@ -57,5 +65,7 @@ typedef struct AccelClass {
 extern int tcg_tb_size;
 
 void configure_accelerator(MachineState *ms);
+/* Register accelerator specific global properties */
+void accel_register_compat_props(AccelState *accel);
 
 #endif
diff --git a/vl.c b/vl.c
index 59fea15..4452d7a 100644
--- a/vl.c
+++ b/vl.c
@@ -4571,6 +4571,7 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
+    accel_register_compat_props(current_machine->accelerator);
     machine_register_compat_props(current_machine);
 
     qemu_opts_foreach(qemu_find_opts("global"),
-- 
2.7.4

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

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 12:46:38PM +0800, Peter Xu wrote:
> Introduce this new field for the accelerator classes so that each
> specific accelerator in the future can register its own global
> properties to be used further by the system. It works just like how the
> old machine compatible properties do, but only tailored for
> accelerators.
> 
> Introduce register_compat_props_array() for it. Export it so that it may
> be used in other codes as well in the future.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  accel/accel.c                |  6 ++++++
>  hw/core/qdev-properties.c    |  7 +++++++
>  include/hw/qdev-properties.h |  1 +
>  include/sysemu/accel.h       | 10 ++++++++++
>  vl.c                         |  1 +
>  5 files changed, 25 insertions(+)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 7c079a5..fa85844 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -120,6 +120,12 @@ void configure_accelerator(MachineState *ms)
>      }
>  }
>  
> +void accel_register_compat_props(AccelState *accel)
> +{
> +    AccelClass *class = ACCEL_GET_CLASS(accel);
> +    register_compat_props_array(class->global_props);
> +}
> +
>  static void register_accel_types(void)
>  {
>      type_register_static(&accel_type);
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 6ff1ac3..19fa335 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1060,6 +1060,13 @@ void register_compat_prop(const char *driver,
>      qdev_prop_register_global(p);
>  }
>  
> +void register_compat_props_array(GlobalProperty *prop)

Note for a later: we need to stop reusing struct GlobalProperty here,
because the 'user_provided', 'used', and 'errp' fields are ignored by
register_compat_prop(), but people may believe the are not.

I suggest defining:

  struct GlobalPropertyValue {
      const char *driver, *property, *value;
  };

and use it instead of GlobalProperty for MachineClass::compat_props,
AccelClass::global_props, and register_compat_prop().

> +{
> +    for (; prop && prop->driver; prop++) {
> +        register_compat_prop(prop->driver, prop->property, prop->value);
> +    }
> +}
> +
>  void qdev_prop_register_global_list(GlobalProperty *props)
>  {
>      int i;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 1722ca4..2bac6a0 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -203,6 +203,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>  
>  void register_compat_prop(const char *driver, const char *property,
>                            const char *value);

I suggest documenting here that this function will call
register_compat_prop(), so it will register internal (not user-provided)
global properties.

> +void register_compat_props_array(GlobalProperty *prop);
>  
>  /**
>   * qdev_property_add_static:
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 15944c1..effcd7f 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -24,6 +24,7 @@
>  #define HW_ACCEL_H
>  
>  #include "qom/object.h"
> +#include "hw/qdev-properties.h"
>  
>  typedef struct AccelState {
>      /*< private >*/
> @@ -40,6 +41,13 @@ typedef struct AccelClass {
>      int (*available)(void);
>      int (*init_machine)(MachineState *ms);
>      bool *allowed;
> +    /*
> +     * Array of global properties that would be applied when specific
> +     * accelerator is chosen. It works just like
> +     * MachineClass.compat_props but it's for accelerators not
> +     * machines.

I suggest adding this:

"Accelerator-provided global properties may be overridden by
machine-type compat_props or user-provided global properties."

> +     */
> +    GlobalProperty *global_props;
>  } AccelClass;
>  
>  #define TYPE_ACCEL "accel"
> @@ -57,5 +65,7 @@ typedef struct AccelClass {
>  extern int tcg_tb_size;
>  
>  void configure_accelerator(MachineState *ms);
> +/* Register accelerator specific global properties */
> +void accel_register_compat_props(AccelState *accel);
>  
>  #endif
> diff --git a/vl.c b/vl.c
> index 59fea15..4452d7a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4571,6 +4571,7 @@ int main(int argc, char **argv, char **envp)
>              exit (i == 1 ? 1 : 0);
>      }
>  
> +    accel_register_compat_props(current_machine->accelerator);
>      machine_register_compat_props(current_machine);
>  
>      qemu_opts_foreach(qemu_find_opts("global"),
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

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

[Peter Xu]

On Fri, Jun 23, 2017 at 06:31:27PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:38PM +0800, Peter Xu wrote:
> > Introduce this new field for the accelerator classes so that each
> > specific accelerator in the future can register its own global
> > properties to be used further by the system. It works just like how the
> > old machine compatible properties do, but only tailored for
> > accelerators.
> > 
> > Introduce register_compat_props_array() for it. Export it so that it may
> > be used in other codes as well in the future.
> > 
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  accel/accel.c                |  6 ++++++
> >  hw/core/qdev-properties.c    |  7 +++++++
> >  include/hw/qdev-properties.h |  1 +
> >  include/sysemu/accel.h       | 10 ++++++++++
> >  vl.c                         |  1 +
> >  5 files changed, 25 insertions(+)
> > 
> > diff --git a/accel/accel.c b/accel/accel.c
> > index 7c079a5..fa85844 100644
> > --- a/accel/accel.c
> > +++ b/accel/accel.c
> > @@ -120,6 +120,12 @@ void configure_accelerator(MachineState *ms)
> >      }
> >  }
> >  
> > +void accel_register_compat_props(AccelState *accel)
> > +{
> > +    AccelClass *class = ACCEL_GET_CLASS(accel);
> > +    register_compat_props_array(class->global_props);
> > +}
> > +
> >  static void register_accel_types(void)
> >  {
> >      type_register_static(&accel_type);
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 6ff1ac3..19fa335 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1060,6 +1060,13 @@ void register_compat_prop(const char *driver,
> >      qdev_prop_register_global(p);
> >  }
> >  
> > +void register_compat_props_array(GlobalProperty *prop)
> 
> Note for a later: we need to stop reusing struct GlobalProperty here,
> because the 'user_provided', 'used', and 'errp' fields are ignored by
> register_compat_prop(), but people may believe the are not.
> 
> I suggest defining:
> 
>   struct GlobalPropertyValue {
>       const char *driver, *property, *value;
>   };
> 
> and use it instead of GlobalProperty for MachineClass::compat_props,
> AccelClass::global_props, and register_compat_prop().

Yeah maybe another definition would be nicer.

Do you mind I skip this change in this series? After all it can be
done on top, and can be separated from current work.

> 
> > +{
> > +    for (; prop && prop->driver; prop++) {
> > +        register_compat_prop(prop->driver, prop->property, prop->value);
> > +    }
> > +}
> > +
> >  void qdev_prop_register_global_list(GlobalProperty *props)
> >  {
> >      int i;
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 1722ca4..2bac6a0 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -203,6 +203,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> >  
> >  void register_compat_prop(const char *driver, const char *property,
> >                            const char *value);
> 
> I suggest documenting here that this function will call
> register_compat_prop(), so it will register internal (not user-provided)
> global properties.

Will do.

> 
> > +void register_compat_props_array(GlobalProperty *prop);
> >  
> >  /**
> >   * qdev_property_add_static:
> > diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> > index 15944c1..effcd7f 100644
> > --- a/include/sysemu/accel.h
> > +++ b/include/sysemu/accel.h
> > @@ -24,6 +24,7 @@
> >  #define HW_ACCEL_H
> >  
> >  #include "qom/object.h"
> > +#include "hw/qdev-properties.h"
> >  
> >  typedef struct AccelState {
> >      /*< private >*/
> > @@ -40,6 +41,13 @@ typedef struct AccelClass {
> >      int (*available)(void);
> >      int (*init_machine)(MachineState *ms);
> >      bool *allowed;
> > +    /*
> > +     * Array of global properties that would be applied when specific
> > +     * accelerator is chosen. It works just like
> > +     * MachineClass.compat_props but it's for accelerators not
> > +     * machines.
> 
> I suggest adding this:
> 
> "Accelerator-provided global properties may be overridden by
> machine-type compat_props or user-provided global properties."

Will do.

Thanks,

-- 
Peter Xu

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

[Eduardo Habkost]

On Mon, Jun 26, 2017 at 10:39:53AM +0800, Peter Xu wrote:
> On Fri, Jun 23, 2017 at 06:31:27PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 23, 2017 at 12:46:38PM +0800, Peter Xu wrote:
> > > Introduce this new field for the accelerator classes so that each
> > > specific accelerator in the future can register its own global
> > > properties to be used further by the system. It works just like how the
> > > old machine compatible properties do, but only tailored for
> > > accelerators.
> > > 
> > > Introduce register_compat_props_array() for it. Export it so that it may
> > > be used in other codes as well in the future.
> > > 
> > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  accel/accel.c                |  6 ++++++
> > >  hw/core/qdev-properties.c    |  7 +++++++
> > >  include/hw/qdev-properties.h |  1 +
> > >  include/sysemu/accel.h       | 10 ++++++++++
> > >  vl.c                         |  1 +
> > >  5 files changed, 25 insertions(+)
> > > 
> > > diff --git a/accel/accel.c b/accel/accel.c
> > > index 7c079a5..fa85844 100644
> > > --- a/accel/accel.c
> > > +++ b/accel/accel.c
> > > @@ -120,6 +120,12 @@ void configure_accelerator(MachineState *ms)
> > >      }
> > >  }
> > >  
> > > +void accel_register_compat_props(AccelState *accel)
> > > +{
> > > +    AccelClass *class = ACCEL_GET_CLASS(accel);
> > > +    register_compat_props_array(class->global_props);
> > > +}
> > > +
> > >  static void register_accel_types(void)
> > >  {
> > >      type_register_static(&accel_type);
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index 6ff1ac3..19fa335 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -1060,6 +1060,13 @@ void register_compat_prop(const char *driver,
> > >      qdev_prop_register_global(p);
> > >  }
> > >  
> > > +void register_compat_props_array(GlobalProperty *prop)
> > 
> > Note for a later: we need to stop reusing struct GlobalProperty here,
> > because the 'user_provided', 'used', and 'errp' fields are ignored by
> > register_compat_prop(), but people may believe the are not.
> > 
> > I suggest defining:
> > 
> >   struct GlobalPropertyValue {
> >       const char *driver, *property, *value;
> >   };
> > 
> > and use it instead of GlobalProperty for MachineClass::compat_props,
> > AccelClass::global_props, and register_compat_prop().
> 
> Yeah maybe another definition would be nicer.
> 
> Do you mind I skip this change in this series? After all it can be
> done on top, and can be separated from current work.
> 

No problem.  This was just a suggestion for later.

-- 
Eduardo

[Qemu-devel] [PATCH v5 03/10] vl: clean up global property registerations

[Peter Xu]

It's not that clear on how the global properties are registered to
global_props (and also its priority relationship). Let's provide a
single function to be called in main() for that, with comment to explain
it a bit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 vl.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 4452d7a..cdd2ec8 100644
--- a/vl.c
+++ b/vl.c
@@ -2969,6 +2969,25 @@ static int qemu_read_default_config_file(void)
     return 0;
 }
 
+static void user_register_compat_props(void)
+{
+    qemu_opts_foreach(qemu_find_opts("global"),
+                      global_init_func, NULL, NULL);
+}
+
+/*
+ * Note: we should see that these compat properties are actually
+ * having a priority: accel < machine < user. This means e.g. when
+ * user specifies something in "-global", it'll always be used with
+ * highest priority.
+ */
+static void register_global_properties(MachineState *ms)
+{
+    accel_register_compat_props(ms->accelerator);
+    machine_register_compat_props(ms);
+    user_register_compat_props();
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -4571,11 +4590,11 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
-    accel_register_compat_props(current_machine->accelerator);
-    machine_register_compat_props(current_machine);
-
-    qemu_opts_foreach(qemu_find_opts("global"),
-                      global_init_func, NULL, NULL);
+    /*
+     * Register all the global properties, including accel properties,
+     * machine properties, and user-specified ones.
+     */
+    register_global_properties(current_machine);
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 03/10] vl: clean up global property registerations

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 12:46:39PM +0800, Peter Xu wrote:
> It's not that clear on how the global properties are registered to
> global_props (and also its priority relationship). Let's provide a
> single function to be called in main() for that, with comment to explain
> it a bit.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  vl.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 4452d7a..cdd2ec8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2969,6 +2969,25 @@ static int qemu_read_default_config_file(void)
>      return 0;
>  }
>  
> +static void user_register_compat_props(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("global"),
> +                      global_init_func, NULL, NULL);

I suggest using "compat props" only when referring to global properties
registered for internal use (because they help us ensure command-line
compatibility).  User-provided global properties are just "global
properties", not "compat props".


> +}
> +
> +/*
> + * Note: we should see that these compat properties are actually
> + * having a priority: accel < machine < user. This means e.g. when
> + * user specifies something in "-global", it'll always be used with
> + * highest priority.
> + */
> +static void register_global_properties(MachineState *ms)
> +{
> +    accel_register_compat_props(ms->accelerator);
> +    machine_register_compat_props(ms);
> +    user_register_compat_props();
> +}
> +
>  int main(int argc, char **argv, char **envp)
>  {
>      int i;
> @@ -4571,11 +4590,11 @@ int main(int argc, char **argv, char **envp)
>              exit (i == 1 ? 1 : 0);
>      }
>  
> -    accel_register_compat_props(current_machine->accelerator);
> -    machine_register_compat_props(current_machine);
> -
> -    qemu_opts_foreach(qemu_find_opts("global"),
> -                      global_init_func, NULL, NULL);
> +    /*
> +     * Register all the global properties, including accel properties,
> +     * machine properties, and user-specified ones.
> +     */
> +    register_global_properties(current_machine);
>  
>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 03/10] vl: clean up global property registerations

[Peter Xu]

On Fri, Jun 23, 2017 at 06:35:29PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:39PM +0800, Peter Xu wrote:
> > It's not that clear on how the global properties are registered to
> > global_props (and also its priority relationship). Let's provide a
> > single function to be called in main() for that, with comment to explain
> > it a bit.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  vl.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 4452d7a..cdd2ec8 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2969,6 +2969,25 @@ static int qemu_read_default_config_file(void)
> >      return 0;
> >  }
> >  
> > +static void user_register_compat_props(void)
> > +{
> > +    qemu_opts_foreach(qemu_find_opts("global"),
> > +                      global_init_func, NULL, NULL);
> 
> I suggest using "compat props" only when referring to global properties
> registered for internal use (because they help us ensure command-line
> compatibility).  User-provided global properties are just "global
> properties", not "compat props".

Agree. Fixing up. Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v5 04/10] migration: let MigrationState be a qdev

[Peter Xu]

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

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

Now we init the MigrationState struct statically in main() to make sure
it's initialized after global properties are applied, since we'll use
them during creation of the object.

No functional change at all.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  1 +
 migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
 migration/migration.h    | 19 ++++++++++++
 vl.c                     |  6 ++++
 4 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 65c7070..2d36cf5 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -45,6 +45,7 @@ void savevm_skip_section_footers(void);
 void savevm_skip_configuration(void);
 
 /* migration/migration.c */
+void migration_object_init(void);
 void qemu_start_incoming_migration(const char *uri, Error **errp);
 bool migration_is_idle(void);
 void add_migration_state_change_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index f588329..2c25927 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -98,32 +98,21 @@ enum mig_rp_message_type {
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
 
+static MigrationState *current_migration;
+
+void migration_object_init(void)
+{
+    /* This can only be called once. */
+    assert(!current_migration);
+    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+}
+
 /* For outgoing */
 MigrationState *migrate_get_current(void)
 {
-    static bool once;
-    static MigrationState current_migration = {
-        .state = MIGRATION_STATUS_NONE,
-        .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
-        .mbps = -1,
-        .parameters = {
-            .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
-            .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-            .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-            .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
-            .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
-            .max_bandwidth = MAX_THROTTLE,
-            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
-            .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
-        },
-    };
-
-    if (!once) {
-        current_migration.parameters.tls_creds = g_strdup("");
-        current_migration.parameters.tls_hostname = g_strdup("");
-        once = true;
-    }
-    return &current_migration;
+    /* This can only be called after the object created. */
+    assert(current_migration);
+    return current_migration;
 }
 
 MigrationIncomingState *migration_incoming_get_current(void)
@@ -1987,3 +1976,46 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = false;
+}
+
+static void migration_instance_init(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+
+    ms->state = MIGRATION_STATUS_NONE;
+    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
+    ms->mbps = -1;
+    ms->parameters = (MigrationParameters) {
+        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
+        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+        .max_bandwidth = MAX_THROTTLE,
+        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
+        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+    };
+    ms->parameters.tls_creds = g_strdup("");
+    ms->parameters.tls_hostname = g_strdup("");
+}
+
+static const TypeInfo migration_type = {
+    .name = TYPE_MIGRATION,
+    .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
+    .class_size = sizeof(MigrationClass),
+    .instance_size = sizeof(MigrationState),
+    .instance_init = migration_instance_init,
+};
+
+static void register_migration_types(void)
+{
+    type_register_static(&migration_type);
+}
+
+type_init(register_migration_types);
diff --git a/migration/migration.h b/migration/migration.h
index d9a268a..3fca364 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -19,6 +19,7 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 /* State for the incoming migration */
 struct MigrationIncomingState {
@@ -62,8 +63,26 @@ struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
+#define TYPE_MIGRATION "migration"
+
+#define MIGRATION_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
+#define MIGRATION_OBJ(obj) \
+    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
+#define MIGRATION_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
+
+typedef struct MigrationClass {
+    /*< private >*/
+    DeviceClass parent_class;
+} MigrationClass;
+
 struct MigrationState
 {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/vl.c b/vl.c
index cdd2ec8..9b04ba7 100644
--- a/vl.c
+++ b/vl.c
@@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
      */
     register_global_properties(current_machine);
 
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
        clock values from the log. */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 04/10] migration: let MigrationState be a qdev

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
> 
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
> 
> Now we init the MigrationState struct statically in main() to make sure
> it's initialized after global properties are applied, since we'll use
> them during creation of the object.
> 
> No functional change at all.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h |  1 +
>  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
>  migration/migration.h    | 19 ++++++++++++
>  vl.c                     |  6 ++++
>  4 files changed, 81 insertions(+), 23 deletions(-)
> 
[...]
> diff --git a/vl.c b/vl.c
> index cdd2ec8..9b04ba7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
>       */
>      register_global_properties(current_machine);
>  
> +    /*
> +     * Migration object can only be created after global properties
> +     * are applied correctly.
> +     */
> +    migration_object_init();
> +

Do we really need this? Can't be we just do:

    if (!current_migration) {
        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
    }

inside migration_get_current()?

>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
>         clock values from the log. */
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 04/10] migration: let MigrationState be a qdev

[Peter Xu]

On Fri, Jun 23, 2017 at 07:18:19PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> > Let the old man "MigrationState" join the object family. Direct benefit
> > is that we can start to use all the property features derived from
> > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > parameters (so will never need to set them up each time using HMP/QMP,
> > this is really, really attractive for test writters), etc.
> > 
> > I see no reason to disallow this happen yet. So let's start from this
> > one, to see whether it would be anything good.
> > 
> > Now we init the MigrationState struct statically in main() to make sure
> > it's initialized after global properties are applied, since we'll use
> > them during creation of the object.
> > 
> > No functional change at all.
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/migration/misc.h |  1 +
> >  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
> >  migration/migration.h    | 19 ++++++++++++
> >  vl.c                     |  6 ++++
> >  4 files changed, 81 insertions(+), 23 deletions(-)
> > 
> [...]
> > diff --git a/vl.c b/vl.c
> > index cdd2ec8..9b04ba7 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
> >       */
> >      register_global_properties(current_machine);
> >  
> > +    /*
> > +     * Migration object can only be created after global properties
> > +     * are applied correctly.
> > +     */
> > +    migration_object_init();
> > +
> 
> Do we really need this? Can't be we just do:
> 
>     if (!current_migration) {
>         current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
>     }
> 
> inside migration_get_current()?

I did this change on purpose (after AccelClass.global_props is
introduced). The comment above tried to explain it but looks like it's
still not clear enough... The reason is that currently the creation of
migration object is depending on the global properties, so we need to
create the object after register_global_properties(), while the old
migrate_get_current() cannot really be sure of this ordering: it just
creates the object on the first call of the function, but the first
call can be even before register_global_properties(). If so, we'll
have a problem (e.g. Xen compat properties will be missing).

Now this restriction is strictly followed if we create the migrate
object here. If anyone calls migrate_get_current() before
register_global_properties(), there will be an assert, and that should
be treated as a programming error.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 04/10] migration: let MigrationState be a qdev

[Eduardo Habkost]

On Mon, Jun 26, 2017 at 10:50:35AM +0800, Peter Xu wrote:
> On Fri, Jun 23, 2017 at 07:18:19PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> > > Let the old man "MigrationState" join the object family. Direct benefit
> > > is that we can start to use all the property features derived from
> > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > > parameters (so will never need to set them up each time using HMP/QMP,
> > > this is really, really attractive for test writters), etc.
> > > 
> > > I see no reason to disallow this happen yet. So let's start from this
> > > one, to see whether it would be anything good.
> > > 
> > > Now we init the MigrationState struct statically in main() to make sure
> > > it's initialized after global properties are applied, since we'll use
> > > them during creation of the object.
> > > 
> > > No functional change at all.
> > > 
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/migration/misc.h |  1 +
> > >  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
> > >  migration/migration.h    | 19 ++++++++++++
> > >  vl.c                     |  6 ++++
> > >  4 files changed, 81 insertions(+), 23 deletions(-)
> > > 
> > [...]
> > > diff --git a/vl.c b/vl.c
> > > index cdd2ec8..9b04ba7 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
> > >       */
> > >      register_global_properties(current_machine);
> > >  
> > > +    /*
> > > +     * Migration object can only be created after global properties
> > > +     * are applied correctly.
> > > +     */
> > > +    migration_object_init();
> > > +
> > 
> > Do we really need this? Can't be we just do:
> > 
> >     if (!current_migration) {
> >         current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> >     }
> > 
> > inside migration_get_current()?
> 
> I did this change on purpose (after AccelClass.global_props is
> introduced). The comment above tried to explain it but looks like it's
> still not clear enough... The reason is that currently the creation of
> migration object is depending on the global properties, so we need to
> create the object after register_global_properties(), while the old
> migrate_get_current() cannot really be sure of this ordering: it just
> creates the object on the first call of the function, but the first
> call can be even before register_global_properties(). If so, we'll
> have a problem (e.g. Xen compat properties will be missing).
> 
> Now this restriction is strictly followed if we create the migrate
> object here. If anyone calls migrate_get_current() before
> register_global_properties(), there will be an assert, and that should
> be treated as a programming error.

Makes sense to me.  Thanks!

-- 
Eduardo

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

[Peter Xu]

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

Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
for x86/power, and AccelClass.global_props for Xen.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc_piix.c                |  1 -
 hw/ppc/spapr.c                   |  1 -
 hw/xen/xen-common.c              | 11 ++++++++++-
 include/hw/compat.h              |  4 ++++
 include/migration/global_state.h |  1 -
 migration/global_state.c         |  9 ++-------
 migration/migration.c            |  7 +++++++
 migration/migration.h            |  6 ++++++
 8 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 46a2bc4..3b51297 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -318,7 +318,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e877d45..edbdbfd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    global_state_set_optional();
     savevm_skip_configuration();
 }
 
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index d3fa705..9163a0a 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -139,19 +139,28 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    global_state_set_optional();
     savevm_skip_configuration();
     savevm_skip_section_footers();
 
     return 0;
 }
 
+GlobalProperty xen_compat_props[] = {
+    {
+        .driver = "migration",
+        .property = "store-global-state",
+        .value = "off",
+    },
+    { .driver = NULL, .property = NULL, .value = NULL },
+};
+
 static void xen_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "Xen";
     ac->init_machine = xen_init;
     ac->allowed = &xen_allowed;
+    ac->global_props = xen_compat_props;
 }
 
 #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 26cd585..a506a74 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -181,6 +181,10 @@
         .driver   = TYPE_PCI_DEVICE,\
         .property = "x-pcie-lnksta-dllla",\
         .value    = "off",\
+    },{\
+        .driver   = "migration",\
+        .property = "store-global-state",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_2 \
diff --git a/include/migration/global_state.h b/include/migration/global_state.h
index 90faea7..d307de8 100644
--- a/include/migration/global_state.h
+++ b/include/migration/global_state.h
@@ -16,7 +16,6 @@
 #include "sysemu/sysemu.h"
 
 void register_global_state(void);
-void global_state_set_optional(void);
 int global_state_store(void);
 void global_state_store_running(void);
 bool global_state_received(void);
diff --git a/migration/global_state.c b/migration/global_state.c
index f792cf5..dcbbcb2 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -15,12 +15,12 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/util.h"
+#include "migration.h"
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
 typedef struct {
-    bool optional;
     uint32_t size;
     uint8_t runstate[100];
     RunState state;
@@ -57,11 +57,6 @@ RunState global_state_get_runstate(void)
     return global_state.state;
 }
 
-void global_state_set_optional(void)
-{
-    global_state.optional = true;
-}
-
 static bool global_state_needed(void *opaque)
 {
     GlobalState *s = opaque;
@@ -69,7 +64,7 @@ static bool global_state_needed(void *opaque)
 
     /* If it is not optional, it is mandatory */
 
-    if (s->optional == false) {
+    if (migrate_get_current()->store_global_state) {
         return true;
     }
 
diff --git a/migration/migration.c b/migration/migration.c
index 2c25927..221b22c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1976,11 +1976,18 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static Property migration_properties[] = {
+    DEFINE_PROP_BOOL("store-global-state", MigrationState,
+                     store_global_state, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void migration_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->user_creatable = false;
+    dc->props = migration_properties;
 }
 
 static void migration_instance_init(Object *obj)
diff --git a/migration/migration.h b/migration/migration.h
index 3fca364..4b898e9 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -133,6 +133,12 @@ struct MigrationState
     /* Do we have to clean up -b/-i from old migrate parameters */
     /* This feature is deprecated and will be removed */
     bool must_remove_block_options;
+
+    /*
+     * Global switch on whether we need to store the global state
+     * during migration.
+     */
+    bool store_global_state;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
-- 
2.7.4

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

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 12:46:41PM +0800, Peter Xu wrote:
> Put it into MigrationState then we can use the properties to specify
> whether to enable storing global state.
> 
> Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
> for x86/power, and AccelClass.global_props for Xen.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/pc_piix.c                |  1 -
>  hw/ppc/spapr.c                   |  1 -
>  hw/xen/xen-common.c              | 11 ++++++++++-
>  include/hw/compat.h              |  4 ++++
>  include/migration/global_state.h |  1 -
>  migration/global_state.c         |  9 ++-------
>  migration/migration.c            |  7 +++++++
>  migration/migration.h            |  6 ++++++
>  8 files changed, 29 insertions(+), 11 deletions(-)
> 
[...]
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index d3fa705..9163a0a 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -139,19 +139,28 @@ static int xen_init(MachineState *ms)
>      }
>      qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
>  
> -    global_state_set_optional();
>      savevm_skip_configuration();
>      savevm_skip_section_footers();
>  
>      return 0;
>  }
>  
> +GlobalProperty xen_compat_props[] = {

This can be static.

> +    {
> +        .driver = "migration",
> +        .property = "store-global-state",
> +        .value = "off",
> +    },
> +    { .driver = NULL, .property = NULL, .value = NULL },

Normally this is written as:

       { /* end of list */ },

> +};
> +
[...]

I didn't review the rest yet, but it seems to be good.

-- 
Eduardo

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

[Peter Xu]

On Fri, Jun 23, 2017 at 07:19:01PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:41PM +0800, Peter Xu wrote:
> > Put it into MigrationState then we can use the properties to specify
> > whether to enable storing global state.
> > 
> > Removing global_state_set_optional() since now we can use HW_COMPAT_2_3
> > for x86/power, and AccelClass.global_props for Xen.
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/pc_piix.c                |  1 -
> >  hw/ppc/spapr.c                   |  1 -
> >  hw/xen/xen-common.c              | 11 ++++++++++-
> >  include/hw/compat.h              |  4 ++++
> >  include/migration/global_state.h |  1 -
> >  migration/global_state.c         |  9 ++-------
> >  migration/migration.c            |  7 +++++++
> >  migration/migration.h            |  6 ++++++
> >  8 files changed, 29 insertions(+), 11 deletions(-)
> > 
> [...]
> > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> > index d3fa705..9163a0a 100644
> > --- a/hw/xen/xen-common.c
> > +++ b/hw/xen/xen-common.c
> > @@ -139,19 +139,28 @@ static int xen_init(MachineState *ms)
> >      }
> >      qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> >  
> > -    global_state_set_optional();
> >      savevm_skip_configuration();
> >      savevm_skip_section_footers();
> >  
> >      return 0;
> >  }
> >  
> > +GlobalProperty xen_compat_props[] = {
> 
> This can be static.

Yep.

> 
> > +    {
> > +        .driver = "migration",
> > +        .property = "store-global-state",
> > +        .value = "off",
> > +    },
> > +    { .driver = NULL, .property = NULL, .value = NULL },
> 
> Normally this is written as:
> 
>        { /* end of list */ },

Let me switch.

> 
> > +};
> > +
> [...]
> 
> I didn't review the rest yet, but it seems to be good.

Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH v5 06/10] migration: move only_migratable to MigrationState

[Peter Xu]

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

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

  -global migration.only-migratable=true

Currently still keep the old interface.

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

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

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 2d36cf5..6ac3307 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -55,4 +55,6 @@ bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
+void migration_only_migratable_set(void);
+
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9841a52..b213696 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -15,7 +15,6 @@
 /* vl.c */
 
 extern const char *bios_name;
-extern int only_migratable;
 extern const char *qemu_name;
 extern QemuUUID qemu_uuid;
 extern bool qemu_uuid_set;
diff --git a/migration/migration.c b/migration/migration.c
index 221b22c..67f9e68 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -115,6 +115,11 @@ MigrationState *migrate_get_current(void)
     return current_migration;
 }
 
+void migration_only_migratable_set(void)
+{
+    migrate_get_current()->only_migratable = true;
+}
+
 MigrationIncomingState *migration_incoming_get_current(void)
 {
     static bool once;
@@ -986,7 +991,7 @@ static GSList *migration_blockers;
 
 int migrate_add_blocker(Error *reason, Error **errp)
 {
-    if (only_migratable) {
+    if (migrate_get_current()->only_migratable) {
         error_propagate(errp, error_copy(reason));
         error_prepend(errp, "disallowing migration blocker "
                           "(--only_migratable) for: ");
@@ -1979,6 +1984,7 @@ void migrate_fd_connect(MigrationState *s)
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
+    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 4b898e9..34377dd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -139,6 +139,9 @@ struct MigrationState
      * during migration.
      */
     bool store_global_state;
+
+    /* Whether the VM is only allowing for migratable devices */
+    bool only_migratable;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index 6bfd489..917ab05 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2314,7 +2314,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
 bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
 {
     /* check needed if --only-migratable is specified */
-    if (!only_migratable) {
+    if (!migrate_get_current()->only_migratable) {
         return true;
     }
 
diff --git a/vl.c b/vl.c
index 9b04ba7..57ff065 100644
--- a/vl.c
+++ b/vl.c
@@ -188,7 +188,6 @@ bool boot_strict;
 uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
-int only_migratable; /* turn it off unless user states otherwise */
 
 int icount_align_option;
 
@@ -3953,7 +3952,13 @@ int main(int argc, char **argv, char **envp)
                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:
-                only_migratable = 1;
+                /*
+                 * TODO: we can remove this option one day, and we
+                 * should all use:
+                 *
+                 * "-global migration.only-migratable=true"
+                 */
+                migration_only_migratable_set();
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.7.4

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

[Peter Xu]

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

Removing savevm_skip_configuration().

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3b51297..488fc0a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -318,7 +318,6 @@ static void pc_compat_2_3(MachineState *machine)
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
-    savevm_skip_configuration();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index edbdbfd..29fac1b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3580,7 +3580,6 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
     savevm_skip_section_footers();
-    savevm_skip_configuration();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 9163a0a..6e52fb0 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -139,7 +139,6 @@ static int xen_init(MachineState *ms)
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
-    savevm_skip_configuration();
     savevm_skip_section_footers();
 
     return 0;
@@ -151,6 +150,11 @@ GlobalProperty xen_compat_props[] = {
         .property = "store-global-state",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-configuration",
+        .value = "off",
+    },
     { .driver = NULL, .property = NULL, .value = NULL },
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index a506a74..1a3fd94 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -183,6 +183,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-configuration",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 6ac3307..f30db4d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -42,7 +42,6 @@ int64_t self_announce_delay(int round)
 
 void dump_vmstate_json_to_file(FILE *out_fp);
 void savevm_skip_section_footers(void);
-void savevm_skip_configuration(void);
 
 /* migration/migration.c */
 void migration_object_init(void);
diff --git a/migration/migration.c b/migration/migration.c
index 67f9e68..414e14d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1985,6 +1985,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
+    DEFINE_PROP_BOOL("send-configuration", MigrationState,
+                     send_configuration, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 34377dd..4d4ea0d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -142,6 +142,9 @@ struct MigrationState
 
     /* Whether the VM is only allowing for migratable devices */
     bool only_migratable;
+
+    /* Whether we send QEMU_VM_CONFIGURATION during migration */
+    bool send_configuration;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index 917ab05..f1e144d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -287,7 +287,6 @@ typedef struct SaveStateEntry {
 typedef struct SaveState {
     QTAILQ_HEAD(, SaveStateEntry) handlers;
     int global_section_id;
-    bool skip_configuration;
     uint32_t len;
     const char *name;
     uint32_t target_page_bits;
@@ -296,15 +295,8 @@ typedef struct SaveState {
 static SaveState savevm_state = {
     .handlers = QTAILQ_HEAD_INITIALIZER(savevm_state.handlers),
     .global_section_id = 0,
-    .skip_configuration = false,
 };
 
-void savevm_skip_configuration(void)
-{
-    savevm_state.skip_configuration = true;
-}
-
-
 static void configuration_pre_save(void *opaque)
 {
     SaveState *state = opaque;
@@ -970,11 +962,11 @@ void qemu_savevm_state_header(QEMUFile *f)
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (migrate_get_current()->send_configuration ||
+        enforce_config_section()) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
-
 }
 
 void qemu_savevm_state_begin(QEMUFile *f)
@@ -1995,7 +1987,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (!savevm_state.skip_configuration || enforce_config_section()) {
+    if (migrate_get_current()->send_configuration ||
+        enforce_config_section()) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
-- 
2.7.4

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

[Peter Xu]

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

Removing savevm_skip_section_footers().

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 488fc0a..22dbef6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -314,7 +314,6 @@ static void pc_init1(MachineState *machine,
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
     }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 29fac1b..7c40fdf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3579,7 +3579,6 @@ DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
 static void spapr_machine_2_3_instance_options(MachineState *machine)
 {
     spapr_machine_2_4_instance_options(machine);
-    savevm_skip_section_footers();
 }
 
 static void spapr_machine_2_3_class_options(MachineClass *mc)
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 6e52fb0..04512f4 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -138,9 +138,6 @@ static int xen_init(MachineState *ms)
         return -1;
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
-
-    savevm_skip_section_footers();
-
     return 0;
 }
 
@@ -155,6 +152,11 @@ GlobalProperty xen_compat_props[] = {
         .property = "send-configuration",
         .value = "off",
     },
+    {
+        .driver = "migration",
+        .property = "send-section-footer",
+        .value = "off",
+    },
     { .driver = NULL, .property = NULL, .value = NULL },
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 1a3fd94..08f3600 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -187,6 +187,10 @@
         .value    = "off",\
     },{\
         .driver   = "migration",\
+        .property = "send-section-footer",\
+        .value    = "off",\
+    },{\
+        .driver   = "migration",\
         .property = "store-global-state",\
         .value    = "off",\
     },
diff --git a/include/migration/misc.h b/include/migration/misc.h
index f30db4d..854c28d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -41,7 +41,6 @@ int64_t self_announce_delay(int round)
 /* migration/savevm.c */
 
 void dump_vmstate_json_to_file(FILE *out_fp);
-void savevm_skip_section_footers(void);
 
 /* migration/migration.c */
 void migration_object_init(void);
diff --git a/migration/migration.c b/migration/migration.c
index 414e14d..96c6412 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1987,6 +1987,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("send-configuration", MigrationState,
                      send_configuration, true),
+    DEFINE_PROP_BOOL("send-section-footer", MigrationState,
+                     send_section_footer, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/migration/migration.h b/migration/migration.h
index 4d4ea0d..994b017 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -145,6 +145,8 @@ struct MigrationState
 
     /* Whether we send QEMU_VM_CONFIGURATION during migration */
     bool send_configuration;
+    /* Whether we send section footer during migration */
+    bool send_section_footer;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index f1e144d..28fa1d5 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -62,8 +62,6 @@
 
 const unsigned int postcopy_ram_discard_version = 0;
 
-static bool skip_section_footers;
-
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
     MIG_CMD_INVALID = 0,   /* Must be 0 */
@@ -761,11 +759,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
     vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
-void savevm_skip_section_footers(void)
-{
-    skip_section_footers = true;
-}
-
 /*
  * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
  */
@@ -793,7 +786,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
  */
 static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
 {
-    if (!skip_section_footers) {
+    if (migrate_get_current()->send_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
     }
@@ -1802,7 +1795,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
     uint8_t read_mark;
     uint32_t read_section_id;
 
-    if (skip_section_footers) {
+    if (!migrate_get_current()->send_section_footer) {
         /* No footer to check */
         return true;
     }
-- 
2.7.4

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

[Peter Xu]

These two parameters:

- MachineState::enforce_config_section
- MigrationState::send_configuration

are playing similar role here. This patch merges the first one into
second, then we'll have a single place to reference whether we need to
send the configuration section.

I didn't remove the MachineState.enforce_config_section field since when
applying that machine property (in machine_set_property()) we haven't
yet initialized global properties and migration object. Then, it's
still not easy to pass that boolean to MigrationState at such an early
time.

A natural benefit for current patch is that now we kept the meaning of
"enforce-config-section" since it'll still have the highest
priority (that's what "enforce" mean I guess).

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 12 ++++++++++++
 migration/savevm.c    | 12 ++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 96c6412..e7e6cf3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -42,6 +42,7 @@
 #include "exec/target_page.h"
 #include "io/channel-buffer.h"
 #include "migration/colo.h"
+#include "hw/boards.h"
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
 
@@ -102,9 +103,20 @@ static MigrationState *current_migration;
 
 void migration_object_init(void)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
+
     /* This can only be called once. */
     assert(!current_migration);
     current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+
+    /*
+     * We cannot really do this in migration_instance_init() since at
+     * that time global properties are not yet applied, then this
+     * value will be definitely replaced by something else.
+     */
+    if (ms->enforce_config_section) {
+        current_migration->send_configuration = true;
+    }
 }
 
 /* For outgoing */
diff --git a/migration/savevm.c b/migration/savevm.c
index 28fa1d5..6b1a9a5 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -943,20 +943,13 @@ bool qemu_savevm_state_blocked(Error **errp)
     return false;
 }
 
-static bool enforce_config_section(void)
-{
-    MachineState *machine = MACHINE(qdev_get_machine());
-    return machine->enforce_config_section;
-}
-
 void qemu_savevm_state_header(QEMUFile *f)
 {
     trace_savevm_state_header();
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (migrate_get_current()->send_configuration ||
-        enforce_config_section()) {
+    if (migrate_get_current()->send_configuration) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
@@ -1980,8 +1973,7 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }
 
-    if (migrate_get_current()->send_configuration ||
-        enforce_config_section()) {
+    if (migrate_get_current()->send_configuration) {
         if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
             error_report("Configuration section missing");
             return -EINVAL;
-- 
2.7.4

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

[Peter Xu]

Now we have some globals that can be configured for migration. Dump them
in HMP info migration for better debugging.

(we can also use this to monitor whether COMPAT fields are applied
correctly on compatible machines)

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hmp.c                    |  3 +++
 include/migration/misc.h |  1 +
 migration/migration.c    | 11 +++++++++++
 3 files changed, 15 insertions(+)

diff --git a/hmp.c b/hmp.c
index 8c72c58..4c41cac 100644
--- a/hmp.c
+++ b/hmp.c
@@ -43,6 +43,7 @@
 #include "exec/ramlist.h"
 #include "hw/intc/intc.h"
 #include "migration/snapshot.h"
+#include "migration/misc.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -164,6 +165,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
     info = qmp_query_migrate(NULL);
     caps = qmp_query_migrate_capabilities(NULL);
 
+    migration_global_dump(mon);
+
     /* do not display parameters during setup */
     if (info->has_status && caps) {
         monitor_printf(mon, "capabilities: ");
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 854c28d..2255121 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -54,5 +54,6 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_only_migratable_set(void);
+void migration_global_dump(Monitor *mon);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index e7e6cf3..b1b0825 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -43,6 +43,7 @@
 #include "io/channel-buffer.h"
 #include "migration/colo.h"
 #include "hw/boards.h"
+#include "monitor/monitor.h"
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
 
@@ -1993,6 +1994,16 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+void migration_global_dump(Monitor *mon)
+{
+    MigrationState *ms = migrate_get_current();
+
+    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
+                   "send-configuration=%d, send-section-footer=%d\n",
+                   ms->store_global_state, ms->only_migratable,
+                   ms->send_configuration, ms->send_section_footer);
+}
+
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
-- 
2.7.4