[Qemu-devel] [PATCH 0/3] qapi: Fix up cpu hotplug property names and add witness for cpu hotplug support

[Qemu-devel] [PATCH 0/3] qapi: Fix up cpu hotplug property names and add witness for cpu hotplug support

[Peter Krempa]

I started figuring out how to add support for this to libvirt and discovered a
few problems that made it not-easy to use for management.

Patch 1 adds a witness to allow us to properly detect the support for the
feature.

Patches 2 and 3 are two options to fix discrepancy between names that need to
be used with -device and those reported by query-hotpluggable-cpus.

Peter Krempa (3):
  qapi: Report support for -device cpu hotplug in query-machines
  VARIANT 1: qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'
  VARIANT 2: qapi: Change 'core-id' to 'core' in 'struct CPUCore'

 hmp.c                   |  4 ++--
 hw/cpu/core.c           |  6 +++---
 hw/ppc/spapr.c          |  4 ++--
 hw/ppc/spapr_cpu_core.c | 16 ++++++++--------
 include/hw/cpu/core.h   |  7 +++++--
 qapi-schema.json        |  9 ++++++---
 vl.c                    |  1 +
 7 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.8.3

[Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

For management apps it's very useful to know whether the selected
machine type supports cpu hotplug via the new -device approach. Using
the presence of 'query-hotpluggable-cpus' is enough for a withess.

Add a property to 'MachineInfo' called 'hotpluggable-cpus' that will
report the presence of this feature.

Example of output:
    {
        "hotpluggable-cpus": false,
        "name": "mac99",
        "cpu-max": 1
    },
    {
        "hotpluggable-cpus": true,
        "name": "pseries-2.7",
        "is-default": true,
        "cpu-max": 255,
        "alias": "pseries"
    },

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 qapi-schema.json | 5 ++++-
 vl.c             | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 0964eec..24ede28 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2986,11 +2986,14 @@
 # @cpu-max: maximum number of CPUs supported by the machine type
 #           (since 1.5.0)
 #
+# @hotpluggable-cpus: cpu hotplug via -device is supported (since 2.7.0)
+#
 # Since: 1.2.0
 ##
 { 'struct': 'MachineInfo',
   'data': { 'name': 'str', '*alias': 'str',
-            '*is-default': 'bool', 'cpu-max': 'int' } }
+            '*is-default': 'bool', 'cpu-max': 'int',
+            'hotpluggable-cpus': 'bool'} }

 ##
 # @query-machines:
diff --git a/vl.c b/vl.c
index c85833a..4c1f9ae 100644
--- a/vl.c
+++ b/vl.c
@@ -1524,6 +1524,7 @@ MachineInfoList *qmp_query_machines(Error **errp)

         info->name = g_strdup(mc->name);
         info->cpu_max = !mc->max_cpus ? 1 : mc->max_cpus;
+        info->hotpluggable_cpus = !!mc->query_hotpluggable_cpus;

         entry = g_malloc0(sizeof(*entry));
         entry->value = info;
-- 
2.8.3

[Qemu-devel] [PATCH 2/3] [VARIANT 1] qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'

[Peter Krempa]

struct CPUCore uses 'core-id' as the property name. As docs for
query-hotpluggable-cpus state that the cpu core properties should be
passed back to device_add by management in case new members are added
and thus the names for the fields should be kept in sync.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 hmp.c                 | 4 ++--
 hw/ppc/spapr.c        | 4 ++--
 include/hw/cpu/core.h | 3 +++
 qapi-schema.json      | 4 ++--
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hmp.c b/hmp.c
index 997a768..543f087 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2463,8 +2463,8 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
         if (c->has_socket) {
             monitor_printf(mon, "    socket: \"%" PRIu64 "\"\n", c->socket);
         }
-        if (c->has_core) {
-            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n", c->core);
+        if (c->has_core_id) {
+            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n", c->core_id);
         }
         if (c->has_thread) {
             monitor_printf(mon, "    thread: \"%" PRIu64 "\"\n", c->thread);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 778fa25..0b6bb9c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2367,8 +2367,8 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)

         cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
         cpu_item->vcpus_count = smp_threads;
-        cpu_props->has_core = true;
-        cpu_props->core = i * smt;
+        cpu_props->has_core_id = true;
+        cpu_props->core_id = i * smt;
         /* TODO: add 'has_node/node' here to describe
            to which node core belongs */

diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 4540a7d..79ac79c 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -26,6 +26,9 @@ typedef struct CPUCore {
     int nr_threads;
 } CPUCore;

+/* Note: topology field names need to be kept in sync with
+ * 'CpuInstanceProperties' */
+
 #define CPU_CORE_PROP_CORE_ID "core-id"

 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 24ede28..37ef5fd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4271,7 +4271,7 @@
 #
 # @node: #optional NUMA node ID the CPU belongs to
 # @socket: #optional socket number within node/board the CPU belongs to
-# @core: #optional core number within socket the CPU belongs to
+# @core-id: #optional core number within socket the CPU belongs to
 # @thread: #optional thread number within core the CPU belongs to
 #
 # Since: 2.7
@@ -4279,7 +4279,7 @@
 { 'struct': 'CpuInstanceProperties',
   'data': { '*node': 'int',
             '*socket': 'int',
-            '*core': 'int',
+            '*core-id': 'int',
             '*thread': 'int'
   }
 }
-- 
2.8.3

[Qemu-devel] [PATCH 3/3] [VARIANT 2] qapi: Change 'core-id' to 'core' in 'struct CPUCore'

[Peter Krempa]

CpuInstanceProperties uses 'core' as the property name. As docs for
query-hotpluggable-cpus state that the cpu core properties should be
passed back to device_add by management in case new members are added
and thus the names for the fields should be kept in sync.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 hw/cpu/core.c           |  6 +++---
 hw/ppc/spapr_cpu_core.c | 16 ++++++++--------
 include/hw/cpu/core.h   |  4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index eff90c1..8484cab 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -15,7 +15,7 @@ static void core_prop_get_core_id(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
 {
     CPUCore *core = CPU_CORE(obj);
-    int64_t value = core->core_id;
+    int64_t value = core->core;

     visit_type_int(v, name, &value, errp);
 }
@@ -33,7 +33,7 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
         return;
     }

-    core->core_id = value;
+    core->core = value;
 }

 static void core_prop_get_nr_threads(Object *obj, Visitor *v, const char *name,
@@ -65,7 +65,7 @@ static void cpu_core_instance_init(Object *obj)
 {
     CPUCore *core = CPU_CORE(obj);

-    object_property_add(obj, "core-id", "int", core_prop_get_core_id,
+    object_property_add(obj, "core", "int", core_prop_get_core_id,
                         core_prop_set_core_id, NULL, NULL, NULL);
     object_property_add(obj, "nr-threads", "int", core_prop_get_nr_threads,
                         core_prop_set_nr_threads, NULL, NULL, NULL);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3a5da09..74aeda5 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -118,7 +118,7 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
         object_unparent(obj);
     }

-    spapr->cores[cc->core_id / smt] = NULL;
+    spapr->cores[cc->core / smt] = NULL;

     g_free(core->threads);
     object_unparent(OBJECT(dev));
@@ -163,8 +163,8 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     int index;
     int smt = kvmppc_smt_threads();

-    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
-    index = cc->core_id / smt;
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core);
+    index = cc->core / smt;
     spapr->cores[index] = OBJECT(dev);

     if (!smc->dr_cpu_enabled) {
@@ -239,19 +239,19 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }

-    if (cc->core_id % smt) {
-        error_setg(&local_err, "invalid core id %d\n", cc->core_id);
+    if (cc->core % smt) {
+        error_setg(&local_err, "invalid core id %d\n", cc->core);
         goto out;
     }

-    index = cc->core_id / smt;
+    index = cc->core / smt;
     if (index < 0 || index >= spapr_max_cores) {
-        error_setg(&local_err, "core id %d out of range", cc->core_id);
+        error_setg(&local_err, "core id %d out of range", cc->core);
         goto out;
     }

     if (spapr->cores[index]) {
-        error_setg(&local_err, "core %d already populated", cc->core_id);
+        error_setg(&local_err, "core %d already populated", cc->core);
         goto out;
     }

diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 79ac79c..3b00c37 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -22,13 +22,13 @@ typedef struct CPUCore {
     DeviceState parent_obj;

     /*< public >*/
-    int core_id;
+    int core;
     int nr_threads;
 } CPUCore;

 /* Note: topology field names need to be kept in sync with
  * 'CpuInstanceProperties' */

-#define CPU_CORE_PROP_CORE_ID "core-id"
+#define CPU_CORE_PROP_CORE_ID "core"

 #endif
-- 
2.8.3

Re: [Qemu-devel] [PATCH 2/3] [VARIANT 1] qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'

[Igor Mammedov]

On Thu, 23 Jun 2016 22:23:24 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> struct CPUCore uses 'core-id' as the property name. As docs for
> query-hotpluggable-cpus state that the cpu core properties should be
> passed back to device_add by management in case new members are added
> and thus the names for the fields should be kept in sync.
David also prefers core-id,
one nit pls also add -id suffix to socket and thread fields in schema
to be consistent.

> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  hmp.c                 | 4 ++--
>  hw/ppc/spapr.c        | 4 ++--
>  include/hw/cpu/core.h | 3 +++
>  qapi-schema.json      | 4 ++--
>  4 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 997a768..543f087 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2463,8 +2463,8 @@ void hmp_hotpluggable_cpus(Monitor *mon, const
> QDict *qdict) if (c->has_socket) {
>              monitor_printf(mon, "    socket: \"%" PRIu64 "\"\n",
> c->socket); }
> -        if (c->has_core) {
> -            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n",
> c->core);
> +        if (c->has_core_id) {
> +            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n",
> c->core_id); }
>          if (c->has_thread) {
>              monitor_printf(mon, "    thread: \"%" PRIu64 "\"\n",
> c->thread); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa25..0b6bb9c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2367,8 +2367,8 @@ static HotpluggableCPUList
> *spapr_query_hotpluggable_cpus(MachineState *machine)
> 
>          cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
>          cpu_item->vcpus_count = smp_threads;
> -        cpu_props->has_core = true;
> -        cpu_props->core = i * smt;
> +        cpu_props->has_core_id = true;
> +        cpu_props->core_id = i * smt;
>          /* TODO: add 'has_node/node' here to describe
>             to which node core belongs */
> 
> diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> index 4540a7d..79ac79c 100644
> --- a/include/hw/cpu/core.h
> +++ b/include/hw/cpu/core.h
> @@ -26,6 +26,9 @@ typedef struct CPUCore {
>      int nr_threads;
>  } CPUCore;
> 
> +/* Note: topology field names need to be kept in sync with
> + * 'CpuInstanceProperties' */
> +
>  #define CPU_CORE_PROP_CORE_ID "core-id"
> 
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 24ede28..37ef5fd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4271,7 +4271,7 @@
>  #
>  # @node: #optional NUMA node ID the CPU belongs to
>  # @socket: #optional socket number within node/board the CPU belongs
> to -# @core: #optional core number within socket the CPU belongs to
> +# @core-id: #optional core number within socket the CPU belongs to
>  # @thread: #optional thread number within core the CPU belongs to
>  #
>  # Since: 2.7
> @@ -4279,7 +4279,7 @@
>  { 'struct': 'CpuInstanceProperties',
>    'data': { '*node': 'int',
>              '*socket': 'int',
> -            '*core': 'int',
> +            '*core-id': 'int',
>              '*thread': 'int'
>    }
>  }

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Eric Blake]

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

On 06/23/2016 02:23 PM, Peter Krempa wrote:
> For management apps it's very useful to know whether the selected
> machine type supports cpu hotplug via the new -device approach. Using
> the presence of 'query-hotpluggable-cpus' is enough for a withess.

s/is enough/alone is not enough/ ?

s/withess/witness/

> 
> Add a property to 'MachineInfo' called 'hotpluggable-cpus' that will
> report the presence of this feature.
> 
> Example of output:
>     {
>         "hotpluggable-cpus": false,
>         "name": "mac99",
>         "cpu-max": 1
>     },
>     {
>         "hotpluggable-cpus": true,
>         "name": "pseries-2.7",
>         "is-default": true,
>         "cpu-max": 255,
>         "alias": "pseries"
>     },
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qapi-schema.json | 5 ++++-
>  vl.c             | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] [VARIANT 1] qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'

[David Gibson]

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

On Thu, 23 Jun 2016 22:54:18 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 23 Jun 2016 22:23:24 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > struct CPUCore uses 'core-id' as the property name. As docs for
> > query-hotpluggable-cpus state that the cpu core properties should be
> > passed back to device_add by management in case new members are added
> > and thus the names for the fields should be kept in sync.  
> David also prefers core-id,
> one nit pls also add -id suffix to socket and thread fields in schema
> to be consistent.

Heh.  I wrote a patch almost identical to this yesterday, intending to
post it today.  So,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

with the same comment as Igor about changing socket and thread to match.

> 
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  hmp.c                 | 4 ++--
> >  hw/ppc/spapr.c        | 4 ++--
> >  include/hw/cpu/core.h | 3 +++
> >  qapi-schema.json      | 4 ++--
> >  4 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 997a768..543f087 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -2463,8 +2463,8 @@ void hmp_hotpluggable_cpus(Monitor *mon, const
> > QDict *qdict) if (c->has_socket) {
> >              monitor_printf(mon, "    socket: \"%" PRIu64 "\"\n",
> > c->socket); }
> > -        if (c->has_core) {
> > -            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n",
> > c->core);
> > +        if (c->has_core_id) {
> > +            monitor_printf(mon, "    core: \"%" PRIu64 "\"\n",
> > c->core_id); }
> >          if (c->has_thread) {
> >              monitor_printf(mon, "    thread: \"%" PRIu64 "\"\n",
> > c->thread); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 778fa25..0b6bb9c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2367,8 +2367,8 @@ static HotpluggableCPUList
> > *spapr_query_hotpluggable_cpus(MachineState *machine)
> > 
> >          cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
> >          cpu_item->vcpus_count = smp_threads;
> > -        cpu_props->has_core = true;
> > -        cpu_props->core = i * smt;
> > +        cpu_props->has_core_id = true;
> > +        cpu_props->core_id = i * smt;
> >          /* TODO: add 'has_node/node' here to describe
> >             to which node core belongs */
> > 
> > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > index 4540a7d..79ac79c 100644
> > --- a/include/hw/cpu/core.h
> > +++ b/include/hw/cpu/core.h
> > @@ -26,6 +26,9 @@ typedef struct CPUCore {
> >      int nr_threads;
> >  } CPUCore;
> > 
> > +/* Note: topology field names need to be kept in sync with
> > + * 'CpuInstanceProperties' */
> > +
> >  #define CPU_CORE_PROP_CORE_ID "core-id"
> > 
> >  #endif
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 24ede28..37ef5fd 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4271,7 +4271,7 @@
> >  #
> >  # @node: #optional NUMA node ID the CPU belongs to
> >  # @socket: #optional socket number within node/board the CPU belongs
> > to -# @core: #optional core number within socket the CPU belongs to
> > +# @core-id: #optional core number within socket the CPU belongs to
> >  # @thread: #optional thread number within core the CPU belongs to
> >  #
> >  # Since: 2.7
> > @@ -4279,7 +4279,7 @@
> >  { 'struct': 'CpuInstanceProperties',
> >    'data': { '*node': 'int',
> >              '*socket': 'int',
> > -            '*core': 'int',
> > +            '*core-id': 'int',
> >              '*thread': 'int'
> >    }
> >  }  
> 


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

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

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[David Gibson]

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

On Thu, 23 Jun 2016 22:23:23 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> For management apps it's very useful to know whether the selected
> machine type supports cpu hotplug via the new -device approach. Using
> the presence of 'query-hotpluggable-cpus' is enough for a withess.
> 
> Add a property to 'MachineInfo' called 'hotpluggable-cpus' that will
> report the presence of this feature.
> 
> Example of output:
>     {
>         "hotpluggable-cpus": false,
>         "name": "mac99",
>         "cpu-max": 1
>     },
>     {
>         "hotpluggable-cpus": true,
>         "name": "pseries-2.7",
>         "is-default": true,
>         "cpu-max": 255,
>         "alias": "pseries"
>     },

I'd been under the impression that there was a general way of detecting
the availability of a particular qmp command.  Was I mistaken?

> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qapi-schema.json | 5 ++++-
>  vl.c             | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0964eec..24ede28 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2986,11 +2986,14 @@
>  # @cpu-max: maximum number of CPUs supported by the machine type
>  #           (since 1.5.0)
>  #
> +# @hotpluggable-cpus: cpu hotplug via -device is supported (since 2.7.0)
> +#
>  # Since: 1.2.0
>  ##
>  { 'struct': 'MachineInfo',
>    'data': { 'name': 'str', '*alias': 'str',
> -            '*is-default': 'bool', 'cpu-max': 'int' } }
> +            '*is-default': 'bool', 'cpu-max': 'int',
> +            'hotpluggable-cpus': 'bool'} }
> 
>  ##
>  # @query-machines:
> diff --git a/vl.c b/vl.c
> index c85833a..4c1f9ae 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1524,6 +1524,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
> 
>          info->name = g_strdup(mc->name);
>          info->cpu_max = !mc->max_cpus ? 1 : mc->max_cpus;
> +        info->hotpluggable_cpus = !!mc->query_hotpluggable_cpus;
> 
>          entry = g_malloc0(sizeof(*entry));
>          entry->value = info;
> -- 
> 2.8.3
> 


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

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

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Eric Blake]

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

On 06/23/2016 08:56 PM, David Gibson wrote:
> On Thu, 23 Jun 2016 22:23:23 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
>> For management apps it's very useful to know whether the selected
>> machine type supports cpu hotplug via the new -device approach. Using
>> the presence of 'query-hotpluggable-cpus' is enough for a withess.
>>

> 
> I'd been under the impression that there was a general way of detecting
> the availability of a particular qmp command.  Was I mistaken?

You are correct - query-commands says whether 'query-hotpluggable-cpus'
exists as a command.  But that is insufficient.  See my review, or the
v2 patch, where the above poor wording was corrected to say what was
really meant: knowing whether query-hotpluggable-cpus exists is
insufficient to tell you whether a given cpu type can be hotplugged.  So
adding one more piece of witness (for every type of cpu supported, we
also advertise if it is hotpluggable) is enough for libvirt to
efficiently take advantage of the new query-hotpluggable-cpus command.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[David Gibson]

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

On Thu, 23 Jun 2016 21:49:25 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 06/23/2016 08:56 PM, David Gibson wrote:
> > On Thu, 23 Jun 2016 22:23:23 +0200
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> >> For management apps it's very useful to know whether the selected
> >> machine type supports cpu hotplug via the new -device approach. Using
> >> the presence of 'query-hotpluggable-cpus' is enough for a withess.
> >>  
> 
> > 
> > I'd been under the impression that there was a general way of detecting
> > the availability of a particular qmp command.  Was I mistaken?  
> 
> You are correct - query-commands says whether 'query-hotpluggable-cpus'
> exists as a command.  But that is insufficient.  See my review, or the
> v2 patch, where the above poor wording was corrected to say what was
> really meant: knowing whether query-hotpluggable-cpus exists is
> insufficient to tell you whether a given cpu type can be hotplugged.  So
> adding one more piece of witness (for every type of cpu supported, we
> also advertise if it is hotpluggable) is enough for libvirt to
> efficiently take advantage of the new query-hotpluggable-cpus command.

Ah, right.  Or to put it another way, the availability of
query-hotpluggable-cpus is global across qemu, whereas actually being
able to use it for hotplug is per machine type.

Would it be possible to do this instead by attempting to invoke
query-hopluggable-cpus and seeing if it returns any information?


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

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

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Igor Mammedov]

On Fri, 24 Jun 2016 14:56:51 +1000
David Gibson <dgibson@redhat.com> wrote:

> On Thu, 23 Jun 2016 21:49:25 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 06/23/2016 08:56 PM, David Gibson wrote:
> > > On Thu, 23 Jun 2016 22:23:23 +0200
> > > Peter Krempa <pkrempa@redhat.com> wrote:
> > >   
> > >> For management apps it's very useful to know whether the selected
> > >> machine type supports cpu hotplug via the new -device approach.
> > >> Using the presence of 'query-hotpluggable-cpus' is enough for a
> > >> withess. 
> > 
> > > 
> > > I'd been under the impression that there was a general way of
> > > detecting the availability of a particular qmp command.  Was I
> > > mistaken?  
> > 
> > You are correct - query-commands says whether
> > 'query-hotpluggable-cpus' exists as a command.  But that is
> > insufficient.  See my review, or the v2 patch, where the above poor
> > wording was corrected to say what was really meant: knowing whether
> > query-hotpluggable-cpus exists is insufficient to tell you whether
> > a given cpu type can be hotplugged.  So adding one more piece of
> > witness (for every type of cpu supported, we also advertise if it
> > is hotpluggable) is enough for libvirt to efficiently take
> > advantage of the new query-hotpluggable-cpus
> > command.
> 
> Ah, right.  Or to put it another way, the availability of
> query-hotpluggable-cpus is global across qemu, whereas actually being
> able to use it for hotplug is per machine type.
> 
> Would it be possible to do this instead by attempting to invoke
> query-hopluggable-cpus and seeing if it returns any information?
This sounds like a better way, for x86 we can set
query-hotpluggable-cpus hook to NULL for old machine types so that
it would return error that it's not supported.

> 
> 

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:

[...]

> > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > exists as a command.  But that is insufficient.  See my review, or the
> > v2 patch, where the above poor wording was corrected to say what was
> > really meant: knowing whether query-hotpluggable-cpus exists is
> > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > adding one more piece of witness (for every type of cpu supported, we
> > also advertise if it is hotpluggable) is enough for libvirt to
> > efficiently take advantage of the new query-hotpluggable-cpus command.
> 
> Ah, right.  Or to put it another way, the availability of
> query-hotpluggable-cpus is global across qemu, whereas actually being
> able to use it for hotplug is per machine type.
> 
> Would it be possible to do this instead by attempting to invoke
> query-hopluggable-cpus and seeing if it returns any information?

It is not strictly necessary for us to have this in the context of
usability. If the user requests using the new hotplug feature we will
try it unconditionally and call query-hotpluggable-cpus before even
starting guest execution. A failure to query the state will then result
in termination of the VM.

It is necessary though to report the availability of the feature to the
user via our domain capabilities API which some higher layer management
apps use to make decisions.

This would also be necessary if we wanted to switch by default to the
new approach, but that's not really possible as libvirt tries to
guarantee that a config valid on certain version will be still valid
even when it was migrated to a newer version and then back.

My current plan is to start qemu with -smp cpus=1,... and then call
query-hotpluggable-cpus and then hotplug all of them until the requested
configuration is satisfied. This approach is necessary so that we can
query for the model and topology info so that we don't need to
re-implement all the numbering and naming logic from qemu.

Additionally this will require us to mark one CPU as non-hotpluggable as
-smp cpus=0,maxcpus=10 is basically translated to -smp
cpus=10,maxcpus=10.

Peter

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[David Gibson]

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

On Fri, 24 Jun 2016 07:41:11 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:
> 
> [...]
> 
> > > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > > exists as a command.  But that is insufficient.  See my review, or the
> > > v2 patch, where the above poor wording was corrected to say what was
> > > really meant: knowing whether query-hotpluggable-cpus exists is
> > > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > > adding one more piece of witness (for every type of cpu supported, we
> > > also advertise if it is hotpluggable) is enough for libvirt to
> > > efficiently take advantage of the new query-hotpluggable-cpus command.  
> > 
> > Ah, right.  Or to put it another way, the availability of
> > query-hotpluggable-cpus is global across qemu, whereas actually being
> > able to use it for hotplug is per machine type.
> > 
> > Would it be possible to do this instead by attempting to invoke
> > query-hopluggable-cpus and seeing if it returns any information?  
> 
> It is not strictly necessary for us to have this in the context of
> usability. If the user requests using the new hotplug feature we will
> try it unconditionally and call query-hotpluggable-cpus before even
> starting guest execution. A failure to query the state will then result
> in termination of the VM.

Oh.. I wasn't expecting the feature would be enabled at user request -
I thought libvirt would just use it if available.

> It is necessary though to report the availability of the feature to the
> user via our domain capabilities API which some higher layer management
> apps use to make decisions.

Right... what advantage does adding the machine flag have over
attempting the query-hotpluggable-cpus?

> This would also be necessary if we wanted to switch by default to the
> new approach, but that's not really possible as libvirt tries to
> guarantee that a config valid on certain version will be still valid
> even when it was migrated to a newer version and then back.

Sorry, I've lost track of what the "This" is that would be necessary.

> My current plan is to start qemu with -smp cpus=1,... and then call
> query-hotpluggable-cpus and then hotplug all of them until the requested
> configuration is satisfied. This approach is necessary so that we can
> query for the model and topology info so that we don't need to
> re-implement all the numbering and naming logic from qemu.

Um.. why?  What's the problem with just staring with -smp cpus=whatever
and then using query-hotpluggable-cpus?

> Additionally this will require us to mark one CPU as non-hotpluggable as
> -smp cpus=0,maxcpus=10 is basically translated to -smp
> cpus=10,maxcpus=10.

The latter is true, but I'm not clear why it implies the former.

-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

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

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

On Fri, Jun 24, 2016 at 16:56:21 +1000, David Gibson wrote:
> On Fri, 24 Jun 2016 07:41:11 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:
> > 
> > [...]
> > 
> > > > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > > > exists as a command.  But that is insufficient.  See my review, or the
> > > > v2 patch, where the above poor wording was corrected to say what was
> > > > really meant: knowing whether query-hotpluggable-cpus exists is
> > > > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > > > adding one more piece of witness (for every type of cpu supported, we
> > > > also advertise if it is hotpluggable) is enough for libvirt to
> > > > efficiently take advantage of the new query-hotpluggable-cpus command.  
> > > 
> > > Ah, right.  Or to put it another way, the availability of
> > > query-hotpluggable-cpus is global across qemu, whereas actually being
> > > able to use it for hotplug is per machine type.
> > > 
> > > Would it be possible to do this instead by attempting to invoke
> > > query-hopluggable-cpus and seeing if it returns any information?  
> > 
> > It is not strictly necessary for us to have this in the context of
> > usability. If the user requests using the new hotplug feature we will
> > try it unconditionally and call query-hotpluggable-cpus before even
> > starting guest execution. A failure to query the state will then result
> > in termination of the VM.
> 
> Oh.. I wasn't expecting the feature would be enabled at user request -
> I thought libvirt would just use it if available.

Hmm, I think that will be possible to use the feature all the time after
all. I'll need to add multiple states for that though to track it in
the XML so that we can re-create it on migration the right way.

> > It is necessary though to report the availability of the feature to the
> > user via our domain capabilities API which some higher layer management
> > apps use to make decisions.
> 
> Right... what advantage does adding the machine flag have over
> attempting the query-hotpluggable-cpus?

Mostly the ability for oVirt or open stack being able to query whether
it's available for given machine type prior to even attempting to launch
the VM. Otherwise the'll be left to either guessing or re-implementing
the support matrix.

This means that the ability to query it with machine types is not
strictly necessary for implementing the feature in libvirt but a
very-nice-to-have thing to add to our capabilities queries.

> > This would also be necessary if we wanted to switch by default to the
> > new approach, but that's not really possible as libvirt tries to
> > guarantee that a config valid on certain version will be still valid
> > even when it was migrated to a newer version and then back.
> 
> Sorry, I've lost track of what the "This" is that would be necessary.

Never mind on this point. If I design the XML a bit differently it will
be possible to use this this feature if present all the time. Some cpus
just won't be available for unplug.

> > My current plan is to start qemu with -smp cpus=1,... and then call
> > query-hotpluggable-cpus and then hotplug all of them until the requested
> > configuration is satisfied. This approach is necessary so that we can
> > query for the model and topology info so that we don't need to
> > re-implement all the numbering and naming logic from qemu.
> 
> Um.. why?  What's the problem with just staring with -smp cpus=whatever
> and then using query-hotpluggable-cpus?
> 
> > Additionally this will require us to mark one CPU as non-hotpluggable as
> > -smp cpus=0,maxcpus=10 is basically translated to -smp
> > cpus=10,maxcpus=10.
> 
> The latter is true, but I'm not clear why it implies the former.

It necessarily doesn't imply it. I originally wanted to have most cpus
unpluggable but thinking again it is not really necessary I just need to
adapt the interface design.

Let's see how it turns out.

Peter

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[David Gibson]

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

On Fri, 24 Jun 2016 09:21:18 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 16:56:21 +1000, David Gibson wrote:
> > On Fri, 24 Jun 2016 07:41:11 +0200
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> > > On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:
> > > 
> > > [...]
> > >   
> > > > > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > > > > exists as a command.  But that is insufficient.  See my review, or the
> > > > > v2 patch, where the above poor wording was corrected to say what was
> > > > > really meant: knowing whether query-hotpluggable-cpus exists is
> > > > > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > > > > adding one more piece of witness (for every type of cpu supported, we
> > > > > also advertise if it is hotpluggable) is enough for libvirt to
> > > > > efficiently take advantage of the new query-hotpluggable-cpus command.    
> > > > 
> > > > Ah, right.  Or to put it another way, the availability of
> > > > query-hotpluggable-cpus is global across qemu, whereas actually being
> > > > able to use it for hotplug is per machine type.
> > > > 
> > > > Would it be possible to do this instead by attempting to invoke
> > > > query-hopluggable-cpus and seeing if it returns any information?    
> > > 
> > > It is not strictly necessary for us to have this in the context of
> > > usability. If the user requests using the new hotplug feature we will
> > > try it unconditionally and call query-hotpluggable-cpus before even
> > > starting guest execution. A failure to query the state will then result
> > > in termination of the VM.  
> > 
> > Oh.. I wasn't expecting the feature would be enabled at user request -
> > I thought libvirt would just use it if available.  
> 
> Hmm, I think that will be possible to use the feature all the time after
> all. I'll need to add multiple states for that though to track it in
> the XML so that we can re-create it on migration the right way.

Hmm...

At least on Power, our intention is that just the machine type version
determines which approach is in use.  WIth machine type <= 2.6, the old
style initialization will be used and hotplug will not be available.
With machine type >= 2.7, new style will be used, and hotplug will be
theoretically available, whether or not you actually use it and
regardless of anything else on the command line.

So I'm not really understanding why you need to track anything extra
for migration here.

> > > It is necessary though to report the availability of the feature to the
> > > user via our domain capabilities API which some higher layer management
> > > apps use to make decisions.  
> > 
> > Right... what advantage does adding the machine flag have over
> > attempting the query-hotpluggable-cpus?  
> 
> Mostly the ability for oVirt or open stack being able to query whether
> it's available for given machine type prior to even attempting to launch
> the VM. Otherwise the'll be left to either guessing or re-implementing
> the support matrix.
> 
> This means that the ability to query it with machine types is not
> strictly necessary for implementing the feature in libvirt but a
> very-nice-to-have thing to add to our capabilities queries.

Ok, makes sense.  I'll merge the patch.

> > > This would also be necessary if we wanted to switch by default to the
> > > new approach, but that's not really possible as libvirt tries to
> > > guarantee that a config valid on certain version will be still valid
> > > even when it was migrated to a newer version and then back.  
> > 
> > Sorry, I've lost track of what the "This" is that would be necessary.  
> 
> Never mind on this point. If I design the XML a bit differently it will
> be possible to use this this feature if present all the time. Some cpus
> just won't be available for unplug.

Ok, still not seeing what any of this has to do with unplug.

> > > My current plan is to start qemu with -smp cpus=1,... and then call
> > > query-hotpluggable-cpus and then hotplug all of them until the requested
> > > configuration is satisfied. This approach is necessary so that we can
> > > query for the model and topology info so that we don't need to
> > > re-implement all the numbering and naming logic from qemu.  
> > 
> > Um.. why?  What's the problem with just staring with -smp cpus=whatever
> > and then using query-hotpluggable-cpus?
> >   
> > > Additionally this will require us to mark one CPU as non-hotpluggable as
> > > -smp cpus=0,maxcpus=10 is basically translated to -smp
> > > cpus=10,maxcpus=10.  
> > 
> > The latter is true, but I'm not clear why it implies the former.  
> 
> It necessarily doesn't imply it. I originally wanted to have most cpus
> unpluggable but thinking again it is not really necessary I just need to
> adapt the interface design.

Again, not seeing what prevents unplug here.

You do realise that query-hotpluggable-cpus lists already plugged as
well as can-be-plugged packages?  And those already plugged packages
(including ones constructed initially) can be unplugged..?

-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

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