[Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Peter Xu]

We have the MigrationState as QDev now (which seems crazy). Let's
continue to benefit.

This series is exporting all migration capabilities/params as global
parameters. Then we can do something like this:

  qemu -global migration.postcopy-ram=true \
       -global migration.max-bandwidth=4096

The values will be inited just like we typed these values into HMP
monitor. It'll simplify lots of migration scripts.

The changes are fairly straightforward. One tiny loss is that we still
don't support:

  -global migration.max-bandwidth=1g

...just like what we did in HMP:

  migrate_set_speed 1g

...while we need to use:

  -global migration.max-bandwidth=1073741824

However that should only be used in scripts, and that's good enough
imho.

These properties should only be used for debugging/testing purpose,
and we should not guarantee any interface compatibility for them (just
like HMP).

Please review. Thanks.

Peter Xu (3):
  qdev: provide DEFINE_PROP_INT64()
  migration: export parameters to props
  migration: export capabilities to props

 hw/core/qdev-properties.c    | 32 +++++++++++++++++++++++++++
 include/hw/qdev-properties.h |  3 +++
 migration/migration.c        | 52 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 77 insertions(+), 10 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] qdev: provide DEFINE_PROP_INT64()

[Peter Xu]

We have merely all the stuff, but this one is missing. Add it in.

Am going to use this new helper for MigrationParameters fields, since
most of them are int64_t.

CC: Markus Armbruster <armbru@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Peter Xu <peterx@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index f11d578..70e14df 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -403,6 +403,31 @@ static void set_uint64(Object *obj, Visitor *v, const char *name,
     visit_type_uint64(v, name, ptr, errp);
 }
 
+static void get_int64(Object *obj, Visitor *v, const char *name,
+                      void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    visit_type_int64(v, name, ptr, errp);
+}
+
+static void set_int64(Object *obj, Visitor *v, const char *name,
+                      void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_int64(v, name, ptr, errp);
+}
+
 PropertyInfo qdev_prop_uint64 = {
     .name  = "uint64",
     .get   = get_uint64,
@@ -410,6 +435,13 @@ PropertyInfo qdev_prop_uint64 = {
     .set_default_value = set_default_value_uint,
 };
 
+PropertyInfo qdev_prop_int64 = {
+    .name  = "int64",
+    .get   = get_int64,
+    .set   = set_int64,
+    .set_default_value = set_default_value_int,
+};
+
 /* --- string --- */
 
 static void release_string(Object *obj, const char *name, void *opaque)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0604c33..2939614 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
+extern PropertyInfo qdev_prop_int64;
 extern PropertyInfo qdev_prop_size;
 extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
@@ -127,6 +128,8 @@ extern PropertyInfo qdev_prop_arraylen;
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
 #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
     DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
+#define DEFINE_PROP_INT64(_n, _s, _f, _d)                      \
+    DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
 #define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
     DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size, uint64_t)
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] migration: export parameters to props

[Peter Xu]

Export migration parameters to qdev properties. Then we can use, for
example:

  -global migration.cpu-throttle-initial=xxx

To specify migration parameters during init.

One thing to mention is that, this usage should only be used for
debugging/testing purpose, and should never be used elsewhere.
Meanwhile, we don't guarantee the compatibility of this interface. It
can alter anytime in the future.

(Ok, it won't change a lot. But it'll just work like HMP - Let's just be
 prepare for any possible change to it)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a0db40d..fd00670 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2009,6 +2009,31 @@ static Property migration_properties[] = {
                      send_configuration, true),
     DEFINE_PROP_BOOL("send-section-footer", MigrationState,
                      send_section_footer, true),
+
+    /* Migration parameters */
+    DEFINE_PROP_INT64("compress-level", MigrationState,
+                      parameters.compress_level,
+                      DEFAULT_MIGRATE_COMPRESS_LEVEL),
+    DEFINE_PROP_INT64("compress-threads", MigrationState,
+                      parameters.compress_threads,
+                      DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
+    DEFINE_PROP_INT64("decompress-threads", MigrationState,
+                      parameters.decompress_threads,
+                      DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
+    DEFINE_PROP_INT64("cpu-throttle-initial", MigrationState,
+                      parameters.cpu_throttle_initial,
+                      DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
+    DEFINE_PROP_INT64("cpu-throttle-increment", MigrationState,
+                      parameters.cpu_throttle_increment,
+                      DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
+    DEFINE_PROP_INT64("max-bandwidth", MigrationState,
+                      parameters.max_bandwidth, MAX_THROTTLE),
+    DEFINE_PROP_INT64("downtime-limit", MigrationState,
+                      parameters.downtime_limit,
+                      DEFAULT_MIGRATE_SET_DOWNTIME),
+    DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
+                      parameters.x_checkpoint_delay,
+                      DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2027,16 +2052,6 @@ static void migration_instance_init(Object *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("");
 }
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] migration: export capabilities to props

[Peter Xu]

Do the same thing to migration capabilities, just like what we did in
previous patch for migration parameters.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index fd00670..7e6f353 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2001,6 +2001,9 @@ void migration_global_dump(Monitor *mon)
                    ms->send_configuration, ms->send_section_footer);
 }
 
+#define DEFINE_PROP_MIG_CAP(name, x)             \
+    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
+
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
@@ -2034,6 +2037,20 @@ static Property migration_properties[] = {
     DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
                       parameters.x_checkpoint_delay,
                       DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
+
+    /* Migration capabilities */
+    DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),
+    DEFINE_PROP_MIG_CAP("rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
+    DEFINE_PROP_MIG_CAP("auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
+    DEFINE_PROP_MIG_CAP("zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
+    DEFINE_PROP_MIG_CAP("compress", MIGRATION_CAPABILITY_COMPRESS),
+    DEFINE_PROP_MIG_CAP("events", MIGRATION_CAPABILITY_EVENTS),
+    DEFINE_PROP_MIG_CAP("postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
+    DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+    DEFINE_PROP_MIG_CAP("release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
+    DEFINE_PROP_MIG_CAP("block", MIGRATION_CAPABILITY_BLOCK),
+    DEFINE_PROP_MIG_CAP("return-path", MIGRATION_CAPABILITY_RETURN_PATH),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] migration: export parameters to props

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Export migration parameters to qdev properties. Then we can use, for
> example:
> 
>   -global migration.cpu-throttle-initial=xxx
> 
> To specify migration parameters during init.
> 
> One thing to mention is that, this usage should only be used for
> debugging/testing purpose, and should never be used elsewhere.
> Meanwhile, we don't guarantee the compatibility of this interface. It
> can alter anytime in the future.
> 
> (Ok, it won't change a lot. But it'll just work like HMP - Let's just be
>  prepare for any possible change to it)
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a0db40d..fd00670 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2009,6 +2009,31 @@ static Property migration_properties[] = {
>                       send_configuration, true),
>      DEFINE_PROP_BOOL("send-section-footer", MigrationState,
>                       send_section_footer, true),
> +
> +    /* Migration parameters */
> +    DEFINE_PROP_INT64("compress-level", MigrationState,
> +                      parameters.compress_level,
> +                      DEFAULT_MIGRATE_COMPRESS_LEVEL),
> +    DEFINE_PROP_INT64("compress-threads", MigrationState,
> +                      parameters.compress_threads,
> +                      DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
> +    DEFINE_PROP_INT64("decompress-threads", MigrationState,
> +                      parameters.decompress_threads,
> +                      DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
> +    DEFINE_PROP_INT64("cpu-throttle-initial", MigrationState,
> +                      parameters.cpu_throttle_initial,
> +                      DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
> +    DEFINE_PROP_INT64("cpu-throttle-increment", MigrationState,
> +                      parameters.cpu_throttle_increment,
> +                      DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
> +    DEFINE_PROP_INT64("max-bandwidth", MigrationState,
> +                      parameters.max_bandwidth, MAX_THROTTLE),
> +    DEFINE_PROP_INT64("downtime-limit", MigrationState,
> +                      parameters.downtime_limit,
> +                      DEFAULT_MIGRATE_SET_DOWNTIME),
> +    DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
> +                      parameters.x_checkpoint_delay,
> +                      DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2027,16 +2052,6 @@ static void migration_instance_init(Object *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("");
>  }
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] migration: export capabilities to props

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Do the same thing to migration capabilities, just like what we did in
> previous patch for migration parameters.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index fd00670..7e6f353 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2001,6 +2001,9 @@ void migration_global_dump(Monitor *mon)
>                     ms->send_configuration, ms->send_section_footer);
>  }
>  
> +#define DEFINE_PROP_MIG_CAP(name, x)             \
> +    DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
> +
>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> @@ -2034,6 +2037,20 @@ static Property migration_properties[] = {
>      DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
>                        parameters.x_checkpoint_delay,
>                        DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
> +
> +    /* Migration capabilities */
> +    DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> +    DEFINE_PROP_MIG_CAP("rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
> +    DEFINE_PROP_MIG_CAP("auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> +    DEFINE_PROP_MIG_CAP("zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
> +    DEFINE_PROP_MIG_CAP("compress", MIGRATION_CAPABILITY_COMPRESS),
> +    DEFINE_PROP_MIG_CAP("events", MIGRATION_CAPABILITY_EVENTS),
> +    DEFINE_PROP_MIG_CAP("postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> +    DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +    DEFINE_PROP_MIG_CAP("release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> +    DEFINE_PROP_MIG_CAP("block", MIGRATION_CAPABILITY_BLOCK),
> +    DEFINE_PROP_MIG_CAP("return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> We have the MigrationState as QDev now (which seems crazy). Let's
> continue to benefit.
> 
> This series is exporting all migration capabilities/params as global
> parameters. Then we can do something like this:
> 
>   qemu -global migration.postcopy-ram=true \
>        -global migration.max-bandwidth=4096
> 
> The values will be inited just like we typed these values into HMP
> monitor. It'll simplify lots of migration scripts.
> 
> The changes are fairly straightforward. One tiny loss is that we still
> don't support:
> 
>   -global migration.max-bandwidth=1g
> 
> ...just like what we did in HMP:
> 
>   migrate_set_speed 1g
> 
> ...while we need to use:
> 
>   -global migration.max-bandwidth=1073741824
> 
> However that should only be used in scripts, and that's good enough
> imho.
> 
> These properties should only be used for debugging/testing purpose,
> and we should not guarantee any interface compatibility for them (just
> like HMP).

I guess the sanity checks in qmp_migrate_set_parameters and
qmp_migrate_set_capabilities aren't run?

Also, have you done any checks on multiple migrations; e.g. it's good
to check that the flags/state are all preserved if we do a migrate, that
migrate fails or is cancelled and then you do a 2nd migrate.

Dave

> Please review. Thanks.
> 
> Peter Xu (3):
>   qdev: provide DEFINE_PROP_INT64()
>   migration: export parameters to props
>   migration: export capabilities to props
> 
>  hw/core/qdev-properties.c    | 32 +++++++++++++++++++++++++++
>  include/hw/qdev-properties.h |  3 +++
>  migration/migration.c        | 52 +++++++++++++++++++++++++++++++++++---------
>  3 files changed, 77 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Eduardo Habkost]

On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
[...]
> These properties should only be used for debugging/testing purpose,
> and we should not guarantee any interface compatibility for them (just
> like HMP).

If we don't guarantee compatibility, the property names need to
be prefixed with "x-".

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] qdev: provide DEFINE_PROP_INT64()

[Marc-André Lureau]

Hi

On Wed, Jul 12, 2017 at 8:55 AM Peter Xu <peterx@redhat.com> wrote:

> We have merely all the stuff, but this one is missing. Add it in.
>
> Am going to use this new helper for MigrationParameters fields, since
> most of them are int64_t.
>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 32 ++++++++++++++++++++++++++++++++
>  include/hw/qdev-properties.h |  3 +++
>  2 files changed, 35 insertions(+)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index f11d578..70e14df 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -403,6 +403,31 @@ static void set_uint64(Object *obj, Visitor *v, const
> char *name,
>      visit_type_uint64(v, name, ptr, errp);
>  }
>
> +static void get_int64(Object *obj, Visitor *v, const char *name,
> +                      void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    visit_type_int64(v, name, ptr, errp);
> +}
> +
> +static void set_int64(Object *obj, Visitor *v, const char *name,
> +                      void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> +
> +    visit_type_int64(v, name, ptr, errp);
> +}
> +
>  PropertyInfo qdev_prop_uint64 = {
>      .name  = "uint64",
>      .get   = get_uint64,
> @@ -410,6 +435,13 @@ PropertyInfo qdev_prop_uint64 = {
>      .set_default_value = set_default_value_uint,
>  };
>
> +PropertyInfo qdev_prop_int64 = {
> +    .name  = "int64",
> +    .get   = get_int64,
> +    .set   = set_int64,
> +    .set_default_value = set_default_value_int,
> +};
> +
>  /* --- string --- */
>
>  static void release_string(Object *obj, const char *name, void *opaque)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0604c33..2939614 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint16;
>  extern PropertyInfo qdev_prop_uint32;
>  extern PropertyInfo qdev_prop_int32;
>  extern PropertyInfo qdev_prop_uint64;
> +extern PropertyInfo qdev_prop_int64;
>  extern PropertyInfo qdev_prop_size;
>  extern PropertyInfo qdev_prop_string;
>  extern PropertyInfo qdev_prop_chr;
> @@ -127,6 +128,8 @@ extern PropertyInfo qdev_prop_arraylen;
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
>  #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
>      DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
> +#define DEFINE_PROP_INT64(_n, _s, _f, _d)                      \
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
>


Make it SIGNED, with that:

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>




>  #define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
>      DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size, uint64_t)
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> --
> 2.7.4
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] qdev: provide DEFINE_PROP_INT64()

[Peter Xu]

On Thu, Jul 13, 2017 at 04:05:32PM +0000, Marc-André Lureau wrote:

[...]

> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 0604c33..2939614 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint16;
> >  extern PropertyInfo qdev_prop_uint32;
> >  extern PropertyInfo qdev_prop_int32;
> >  extern PropertyInfo qdev_prop_uint64;
> > +extern PropertyInfo qdev_prop_int64;
> >  extern PropertyInfo qdev_prop_size;
> >  extern PropertyInfo qdev_prop_string;
> >  extern PropertyInfo qdev_prop_chr;
> > @@ -127,6 +128,8 @@ extern PropertyInfo qdev_prop_arraylen;
> >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
> >  #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
> >      DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
> > +#define DEFINE_PROP_INT64(_n, _s, _f, _d)                      \
> > +    DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
> >
> 
> 
> Make it SIGNED, with that:

Oops, definitely.

> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Peter Xu]

On Wed, Jul 12, 2017 at 08:02:40PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We have the MigrationState as QDev now (which seems crazy). Let's
> > continue to benefit.
> > 
> > This series is exporting all migration capabilities/params as global
> > parameters. Then we can do something like this:
> > 
> >   qemu -global migration.postcopy-ram=true \
> >        -global migration.max-bandwidth=4096
> > 
> > The values will be inited just like we typed these values into HMP
> > monitor. It'll simplify lots of migration scripts.
> > 
> > The changes are fairly straightforward. One tiny loss is that we still
> > don't support:
> > 
> >   -global migration.max-bandwidth=1g
> > 
> > ...just like what we did in HMP:
> > 
> >   migrate_set_speed 1g
> > 
> > ...while we need to use:
> > 
> >   -global migration.max-bandwidth=1073741824
> > 
> > However that should only be used in scripts, and that's good enough
> > imho.
> > 
> > These properties should only be used for debugging/testing purpose,
> > and we should not guarantee any interface compatibility for them (just
> > like HMP).
> 
> I guess the sanity checks in qmp_migrate_set_parameters and
> qmp_migrate_set_capabilities aren't run?

Indeed I missed this point. Before that: Eduardo, do you think below
change would make any sense?

diff --git a/qom/object.c b/qom/object.c
index 5f6fdfa..ca077a9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
 
 static void object_post_init_with_type(Object *obj, TypeImpl *ti)
 {
-    if (ti->instance_post_init) {
-        ti->instance_post_init(obj);
-    }
-
     if (type_has_parent(ti)) {
         object_post_init_with_type(obj, type_get_parent(ti));
     }
+
+    if (ti->instance_post_init) {
+        ti->instance_post_init(obj);
+    }
 }
 
 static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)

IMHO it'll make more sense if we call the parent post_init first (just
like most of the other object hooks). Or is current ordering intended?

If we can have above change, it'll be much easier for me to do the
check that Dave mentioned, after the global properties applied (I can
then just provide post_init hook for the migration object).

> 
> Also, have you done any checks on multiple migrations; e.g. it's good
> to check that the flags/state are all preserved if we do a migrate, that
> migrate fails or is cancelled and then you do a 2nd migrate.

I think so. They'll be applied only when creating the object, and
they'll be there even after a e.g. migrate_cancel.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Peter Xu]

On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> [...]
> > These properties should only be used for debugging/testing purpose,
> > and we should not guarantee any interface compatibility for them (just
> > like HMP).
> 
> If we don't guarantee compatibility, the property names need to
> be prefixed with "x-".

Indeed. Sorry I missed that.

But I'd say it is slightly awkward to add "x-" for all these (for me,
"x-" means more like "this is not stable and experimental, use it
carefully", while this does not suite for this series). Maybe I can
just remove this sentence in commit log (I think I am just a little
bit frightened by the compatibility problems)...

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 12:23:06PM +0800, Peter Xu wrote:
> On Wed, Jul 12, 2017 at 08:02:40PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > We have the MigrationState as QDev now (which seems crazy). Let's
> > > continue to benefit.
> > > 
> > > This series is exporting all migration capabilities/params as global
> > > parameters. Then we can do something like this:
> > > 
> > >   qemu -global migration.postcopy-ram=true \
> > >        -global migration.max-bandwidth=4096
> > > 
> > > The values will be inited just like we typed these values into HMP
> > > monitor. It'll simplify lots of migration scripts.
> > > 
> > > The changes are fairly straightforward. One tiny loss is that we still
> > > don't support:
> > > 
> > >   -global migration.max-bandwidth=1g
> > > 
> > > ...just like what we did in HMP:
> > > 
> > >   migrate_set_speed 1g
> > > 
> > > ...while we need to use:
> > > 
> > >   -global migration.max-bandwidth=1073741824
> > > 
> > > However that should only be used in scripts, and that's good enough
> > > imho.
> > > 
> > > These properties should only be used for debugging/testing purpose,
> > > and we should not guarantee any interface compatibility for them (just
> > > like HMP).
> > 
> > I guess the sanity checks in qmp_migrate_set_parameters and
> > qmp_migrate_set_capabilities aren't run?
> 
> Indeed I missed this point. Before that: Eduardo, do you think below
> change would make any sense?
> 
> diff --git a/qom/object.c b/qom/object.c
> index 5f6fdfa..ca077a9 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
>  
>  static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>  {
> -    if (ti->instance_post_init) {
> -        ti->instance_post_init(obj);
> -    }
> -
>      if (type_has_parent(ti)) {
>          object_post_init_with_type(obj, type_get_parent(ti));
>      }
> +
> +    if (ti->instance_post_init) {
> +        ti->instance_post_init(obj);
> +    }
>  }
>  
>  static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
> 
> IMHO it'll make more sense if we call the parent post_init first (just
> like most of the other object hooks). Or is current ordering intended?

I'm not sure.  The point of post_init was to allow the parent
class to perform actions after all the children classes have done
their tasks.

However, if you can prove this won't break existing code and you
propose an amendment to ObjectClass::instance_post_init
documentation, we can consider changing the ordering.

(A quick look at arm_cpu_post_init() seems to indicate the order
you propose will be more useful for TYPE_ARM_CPU too.)

> 
> If we can have above change, it'll be much easier for me to do the
> check that Dave mentioned, after the global properties applied (I can
> then just provide post_init hook for the migration object).
> 
> > 
> > Also, have you done any checks on multiple migrations; e.g. it's good
> > to check that the flags/state are all preserved if we do a migrate, that
> > migrate fails or is cancelled and then you do a 2nd migrate.
> 
> I think so. They'll be applied only when creating the object, and
> they'll be there even after a e.g. migrate_cancel.  Thanks,
> 
> -- 
> Peter Xu

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote:
> On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> > [...]
> > > These properties should only be used for debugging/testing purpose,
> > > and we should not guarantee any interface compatibility for them (just
> > > like HMP).
> > 
> > If we don't guarantee compatibility, the property names need to
> > be prefixed with "x-".
> 
> Indeed. Sorry I missed that.
> 
> But I'd say it is slightly awkward to add "x-" for all these (for me,
> "x-" means more like "this is not stable and experimental, use it
> carefully", while this does not suite for this series). Maybe I can
> just remove this sentence in commit log (I think I am just a little
> bit frightened by the compatibility problems)...

"x-" in property names doesn't mean "experimental", but just "not
part of the stable interface".  If you have the tiniest doubt
about command-line compatibility, I think it won't hurt to use
"x-".

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote:
> > On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> > > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> > > [...]
> > > > These properties should only be used for debugging/testing purpose,
> > > > and we should not guarantee any interface compatibility for them (just
> > > > like HMP).
> > > 
> > > If we don't guarantee compatibility, the property names need to
> > > be prefixed with "x-".
> > 
> > Indeed. Sorry I missed that.
> > 
> > But I'd say it is slightly awkward to add "x-" for all these (for me,
> > "x-" means more like "this is not stable and experimental, use it
> > carefully", while this does not suite for this series). Maybe I can
> > just remove this sentence in commit log (I think I am just a little
> > bit frightened by the compatibility problems)...
> 
> "x-" in property names doesn't mean "experimental", but just "not
> part of the stable interface".  If you have the tiniest doubt
> about command-line compatibility, I think it won't hurt to use
> "x-".

We have:
    DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),

so we could easily do:
    DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),

to ensure that all of the things we expose as props here have
that added fealing of uncertainty but don't change what migration
parameters see.


(It's a shame we have to have those lists manually, there are so
many manual places for each parameter and capability)

Dave

> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Peter Xu]

On Fri, Jul 14, 2017 at 05:32:10PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote:
> > > On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> > > > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> > > > [...]
> > > > > These properties should only be used for debugging/testing purpose,
> > > > > and we should not guarantee any interface compatibility for them (just
> > > > > like HMP).
> > > > 
> > > > If we don't guarantee compatibility, the property names need to
> > > > be prefixed with "x-".
> > > 
> > > Indeed. Sorry I missed that.
> > > 
> > > But I'd say it is slightly awkward to add "x-" for all these (for me,
> > > "x-" means more like "this is not stable and experimental, use it
> > > carefully", while this does not suite for this series). Maybe I can
> > > just remove this sentence in commit log (I think I am just a little
> > > bit frightened by the compatibility problems)...
> > 
> > "x-" in property names doesn't mean "experimental", but just "not
> > part of the stable interface".  If you have the tiniest doubt
> > about command-line compatibility, I think it won't hurt to use
> > "x-".
> 
> We have:
>     DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> 
> so we could easily do:
>     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> 
> to ensure that all of the things we expose as props here have
> that added fealing of uncertainty but don't change what migration
> parameters see.

I was thinking to ask either you or Juan on what you would like for
this, looks like I got the answer now. :-) Let me add "x-" to them.

> 
> 
> (It's a shame we have to have those lists manually, there are so
> many manual places for each parameter and capability)

Temporarily I think it's still okay. But indeed you are right.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/3] migration: export cap/params to qdev props

[Peter Xu]

On Fri, Jul 14, 2017 at 12:57:15PM -0300, Eduardo Habkost wrote:
> On Fri, Jul 14, 2017 at 12:23:06PM +0800, Peter Xu wrote:
> > On Wed, Jul 12, 2017 at 08:02:40PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (peterx@redhat.com) wrote:
> > > > We have the MigrationState as QDev now (which seems crazy). Let's
> > > > continue to benefit.
> > > > 
> > > > This series is exporting all migration capabilities/params as global
> > > > parameters. Then we can do something like this:
> > > > 
> > > >   qemu -global migration.postcopy-ram=true \
> > > >        -global migration.max-bandwidth=4096
> > > > 
> > > > The values will be inited just like we typed these values into HMP
> > > > monitor. It'll simplify lots of migration scripts.
> > > > 
> > > > The changes are fairly straightforward. One tiny loss is that we still
> > > > don't support:
> > > > 
> > > >   -global migration.max-bandwidth=1g
> > > > 
> > > > ...just like what we did in HMP:
> > > > 
> > > >   migrate_set_speed 1g
> > > > 
> > > > ...while we need to use:
> > > > 
> > > >   -global migration.max-bandwidth=1073741824
> > > > 
> > > > However that should only be used in scripts, and that's good enough
> > > > imho.
> > > > 
> > > > These properties should only be used for debugging/testing purpose,
> > > > and we should not guarantee any interface compatibility for them (just
> > > > like HMP).
> > > 
> > > I guess the sanity checks in qmp_migrate_set_parameters and
> > > qmp_migrate_set_capabilities aren't run?
> > 
> > Indeed I missed this point. Before that: Eduardo, do you think below
> > change would make any sense?
> > 
> > diff --git a/qom/object.c b/qom/object.c
> > index 5f6fdfa..ca077a9 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
> >  
> >  static void object_post_init_with_type(Object *obj, TypeImpl *ti)
> >  {
> > -    if (ti->instance_post_init) {
> > -        ti->instance_post_init(obj);
> > -    }
> > -
> >      if (type_has_parent(ti)) {
> >          object_post_init_with_type(obj, type_get_parent(ti));
> >      }
> > +
> > +    if (ti->instance_post_init) {
> > +        ti->instance_post_init(obj);
> > +    }
> >  }
> >  
> >  static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
> > 
> > IMHO it'll make more sense if we call the parent post_init first (just
> > like most of the other object hooks). Or is current ordering intended?
> 
> I'm not sure.  The point of post_init was to allow the parent
> class to perform actions after all the children classes have done
> their tasks.
> 
> However, if you can prove this won't break existing code and you
> propose an amendment to ObjectClass::instance_post_init
> documentation, we can consider changing the ordering.
> 
> (A quick look at arm_cpu_post_init() seems to indicate the order
> you propose will be more useful for TYPE_ARM_CPU too.)

Exactly. That's one of the reason I asked. I do think it makes more
sense to post_init on parent first, since child always contains extra
things to handle, and it may possibly want to override something of
the parent rather than in a reversed order.

So as you may have already seen, it should not break anything, instead
it may benefit us for at least this series and possibly
arm_cpu_post_init() as well in the future.

Let me propose a patch for it then.  Thanks!

-- 
Peter Xu