[Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Igor Mammedov]

Changes since v2:
 - rebase on top of hte lates spapr cpu hotpug series
 - add 'vcpus-count' field, pkrempa@redhat.com
 - s/CpuInstanceProps/CpuInstanceProperties/
 - use '#optional' marker
 - make "props" as always present even if it's empty
 - fix JSON examples
 - fix minor typos
 - drop pre_plug spapr impl out of series as not related to QMP command
 - drop generic pre hotplug callback as not related to QMP command

Changes since RFC:
 - drop arch_id
 - move CPU properties into separate structure
 - target implements its own qmp callback version
 - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
                      https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
    - convert slot name to core id hack
    - drop links
    - add generic pre hotplug callback
    - implement query-hotpluggable-cpus

The first patch (QMP API) in this series could go in first
allowing individual targets to post their hotplug
implementation independently on top of it.

Igor Mammedov (2):
  QMP: add query-hotpluggable-cpus
  spapr: implement query-hotpluggable-cpus QMP command

 hw/ppc/spapr.c                      | 32 +++++++++++++++++++++++++++
 qapi-schema.json                    | 41 +++++++++++++++++++++++++++++++++++
 qmp-commands.hx                     | 43 +++++++++++++++++++++++++++++++++++++
 stubs/Makefile.objs                 |  1 +
 stubs/qmp_query_hotpluggable_cpus.c |  9 ++++++++
 5 files changed, 126 insertions(+)
 create mode 100644 stubs/qmp_query_hotpluggable_cpus.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/2] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

it will allow mgmt to query present and hotpluggable
CPU objects, it is required from a target platform that
wish to support command to implement
 qmp_query_hotpluggable_cpus()
function, which will return a list of possible CPU objects
with options that would be needed for hotplugging possible
CPU objects.

There are:
'type': 'str' - QOM CPU object type for usage with device_add
'vcpus-count': 'int' - number of logical VCPU threads per
                        CPU object (mgmt needs to know)

and a set of optional fields that are to used for hotplugging
a CPU objects and would allows mgmt tools to know what/where
it could be hotplugged;
[node],[socket],[core],[thread]

For present CPUs there is a 'qom-path' field which
would allow mgmt to inspect whatever object/abstraction
the target platform considers as CPU object.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 - add 'vcpus-count' field, pkrempa@redhat.com
 - s/CpuInstanceProps/CpuInstanceProperties/
 - use '#optional' marker
 - make "props" as always present even if it's empty
 - fix JSON examples
 - fix minor typos
---
 qapi-schema.json                    | 41 +++++++++++++++++++++++++++++++++++
 qmp-commands.hx                     | 43 +++++++++++++++++++++++++++++++++++++
 stubs/Makefile.objs                 |  1 +
 stubs/qmp_query_hotpluggable_cpus.c |  9 ++++++++
 4 files changed, 94 insertions(+)
 create mode 100644 stubs/qmp_query_hotpluggable_cpus.c

diff --git a/qapi-schema.json b/qapi-schema.json
index 362c9d8..105511d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4122,3 +4122,44 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# CpuInstanceProperties
+#
+# @node: NUMA node ID the CPU belongs to, optional
+# @socket: #optional socket number within node/board the CPU belongs to
+# @core: #optional core number within socket the CPU belongs to
+# @thread: #optional thread number within core the CPU belongs to
+#
+# Since: 2.7
+{ 'struct': 'CpuInstanceProperties',
+  'data': { '*node': 'int',
+            '*socket': 'int',
+            '*core': 'int',
+            '*thread': 'int'
+  }
+}
+
+##
+# @HotpluggableCPU
+#
+# @type: CPU object type for usage with device_add command
+# @props: list of properties to be used for hotplugging CPU
+# @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
+# @qom-path: #optional link to existing CPU object if CPU is present or
+#            omitted if CPU is not present.
+#
+# Since: 2.7
+{ 'struct': 'HotpluggableCPU',
+  'data': { 'type': 'str',
+            'vcpus-count': 'int',
+            'props': 'CpuInstanceProperties',
+            '*qom-path': 'str'
+          }
+}
+
+##
+# @query-hotpluggable-cpus
+#
+# Since: 2.7
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b629673..41a00b3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4853,3 +4853,46 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+    {
+        .name       = "query-hotpluggable-cpus",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
+    },
+
+SQMP
+Show  existing/possible CPUs
+-------------------------------
+
+Arguments: None.
+
+Example for x86 target started with -smp 2,sockets=2,cores=1,threads=3,maxcpus=6:
+
+-> { "execute": "query-hotpluggable-cpus" }
+<- {"return": [
+     { "props": { "core": 0, "socket": 1, "thread": 2},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1 },
+     { "props": { "core": 0, "socket": 1, "thread": 1},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1 },
+     { "props": { "core": 0, "socket": 1, "thread": 0},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1 },
+     { "props": { "core": 0, "socket": 0, "thread": 2},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1 },
+     { "props": { "core": 0, "socket": 0, "thread": 1},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+       "qom-path": "/machine/unattached/device[3]"},
+     { "props": { "core": 0, "socket": 0, "thread": 0},
+       "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+       "qom-path": "/machine/unattached/device[0]"}
+   ]}'
+
+Example for SPAPR target started with -smp 2,cores=2,maxcpus=4:
+
+-> { "execute": "query-hotpluggable-cpus" }
+<- {"return": [
+     { "props": { "core": 1 }, "type": "spapr-cpu-core", "vcpus-count": 1 },
+     { "props": { "core": 0 }, "type": "spapr-cpu-core", "vcpus-count": 1,
+       "qom-path": "/machine/unattached/device[0]"}
+   ]}'
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index e922de9..0011173 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += vhost.o
+stub-obj-y += qmp_query_hotpluggable_cpus.o
diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
new file mode 100644
index 0000000..21a75a3
--- /dev/null
+++ b/stubs/qmp_query_hotpluggable_cpus.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qapi/qmp/qerror.h"
+#include "qmp-commands.h"
+
+HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
+    return NULL;
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/2] QMP: add query-hotpluggable-cpus

[Eduardo Habkost]

On Tue, Mar 15, 2016 at 02:24:07PM +0100, Igor Mammedov wrote:
[...]
> diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
> new file mode 100644
> index 0000000..21a75a3
> --- /dev/null
> +++ b/stubs/qmp_query_hotpluggable_cpus.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qmp-commands.h"
> +
> +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> +{
> +    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> +    return NULL;
> +}

Sorry if this was discussed in previous threads that I haven't
read, but: isn't this supposed to be a MachineClass method?  I
remember David saying once that we have the habit of assuming
that a single QEMU binary can run only one family of machines
that are very similar (like x86), but that's not always true.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Fri, 18 Mar 2016 16:26:28 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Mar 15, 2016 at 02:24:07PM +0100, Igor Mammedov wrote:
> [...]
> > diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
> > new file mode 100644
> > index 0000000..21a75a3
> > --- /dev/null
> > +++ b/stubs/qmp_query_hotpluggable_cpus.c
> > @@ -0,0 +1,9 @@
> > +#include "qemu/osdep.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "qmp-commands.h"
> > +
> > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > +{
> > +    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> > +    return NULL;
> > +}  
> 
> Sorry if this was discussed in previous threads that I haven't
> read, but: isn't this supposed to be a MachineClass method?  I
> remember David saying once that we have the habit of assuming
> that a single QEMU binary can run only one family of machines
> that are very similar (like x86), but that's not always true.
Stub approach works for current qemu with one target per binary
but it won't for multi-target binary.

I've been trying to not clutter MachineClass with hooks
that not must have right now but I don't have a strong opinion
on this so if MachineClass method is preferred way,
I can rewrite it this patch to use it on respin.

Re: [Qemu-devel] [PATCH v3 1/2] QMP: add query-hotpluggable-cpus

[David Gibson]

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

On Mon, 21 Mar 2016 11:53:23 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 18 Mar 2016 16:26:28 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Mar 15, 2016 at 02:24:07PM +0100, Igor Mammedov wrote:
> > [...]  
> > > diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
> > > new file mode 100644
> > > index 0000000..21a75a3
> > > --- /dev/null
> > > +++ b/stubs/qmp_query_hotpluggable_cpus.c
> > > @@ -0,0 +1,9 @@
> > > +#include "qemu/osdep.h"
> > > +#include "qapi/qmp/qerror.h"
> > > +#include "qmp-commands.h"
> > > +
> > > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > > +{
> > > +    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> > > +    return NULL;
> > > +}    
> > 
> > Sorry if this was discussed in previous threads that I haven't
> > read, but: isn't this supposed to be a MachineClass method?  I
> > remember David saying once that we have the habit of assuming
> > that a single QEMU binary can run only one family of machines
> > that are very similar (like x86), but that's not always true.  
> Stub approach works for current qemu with one target per binary
> but it won't for multi-target binary.

This approach won't work even now.  We have draft implementations of
the hook for spapr, but those are absolutely wrong for mac99 or the
many other ppc machine classes.

> I've been trying to not clutter MachineClass with hooks
> that not must have right now but I don't have a strong opinion
> on this so if MachineClass method is preferred way,
> I can rewrite it this patch to use it on respin.
> 
> 


-- 
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 v3 1/2] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Tue, 22 Mar 2016 10:39:28 +1100
David Gibson <dgibson@redhat.com> wrote:

> On Mon, 21 Mar 2016 11:53:23 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 18 Mar 2016 16:26:28 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Mar 15, 2016 at 02:24:07PM +0100, Igor Mammedov wrote:
> > > [...]    
> > > > diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
> > > > new file mode 100644
> > > > index 0000000..21a75a3
> > > > --- /dev/null
> > > > +++ b/stubs/qmp_query_hotpluggable_cpus.c
> > > > @@ -0,0 +1,9 @@
> > > > +#include "qemu/osdep.h"
> > > > +#include "qapi/qmp/qerror.h"
> > > > +#include "qmp-commands.h"
> > > > +
> > > > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > > > +{
> > > > +    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> > > > +    return NULL;
> > > > +}      
> > > 
> > > Sorry if this was discussed in previous threads that I haven't
> > > read, but: isn't this supposed to be a MachineClass method?  I
> > > remember David saying once that we have the habit of assuming
> > > that a single QEMU binary can run only one family of machines
> > > that are very similar (like x86), but that's not always true.    
> > Stub approach works for current qemu with one target per binary
> > but it won't for multi-target binary.  
> 
> This approach won't work even now.  We have draft implementations of
> the hook for spapr, but those are absolutely wrong for mac99 or the
> many other ppc machine classes.
Ok, I'll respin it as MachineClass method.

> 
> > I've been trying to not clutter MachineClass with hooks
> > that not must have right now but I don't have a strong opinion
> > on this so if MachineClass method is preferred way,
> > I can rewrite it this patch to use it on respin.
> > 
> >   
> 
> 

[Qemu-devel] [RFC v3 2/2] spapr: implement query-hotpluggable-cpus QMP command

[Igor Mammedov]

it returns a list of present/possible to hotplug CPU
objects with a list of properties to use with
device_add.

in spapr case returned list would looks like:
-> { "execute": "query-hotpluggable-cpus" }
<- {"return": [
     { "props": { "core": 1 }, "type": "spapr-cpu-core",
       "vcpus-count": 2 },
     { "props": { "core": 0 }, "type": "spapr-cpu-core",
       "vcpus-count": 2,
       "qom-path": "/machine/unattached/device[0]"}
   ]}'

TODO:
  add 'node' property for core <-> numa node mapping

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
it's only compile tested
---
 hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b1e9ba2..e1ce983 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -65,6 +65,7 @@
 #include "hw/compat.h"
 #include "qemu-common.h"
 #include "hw/ppc/spapr_cpu_core.h"
+#include "qmp-commands.h"
 
 #include <libfdt.h>
 
@@ -2399,6 +2400,37 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
     return cpu_index / smp_threads / smp_cores;
 }
 
+HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
+{
+    int i;
+    HotpluggableCPUList *head = NULL;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    int spapr_max_cores = max_cpus / smp_threads;
+
+    for (i = 0; i < spapr_max_cores; i++) {
+        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
+        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
+        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
+
+        cpu_item->type = g_strdup(TYPE_SPAPR_CPU_CORE);
+        cpu_item->vcpus_count = smp_threads;
+        cpu_props->has_core = true;
+        cpu_props->core = i;
+        /* TODO: add 'has_node/node' here to describe
+           to which node core belongs */
+
+        cpu_item->props = cpu_props;
+        if (spapr->cores[i]) {
+            cpu_item->has_qom_path = true;
+            cpu_item->qom_path = object_get_canonical_path(spapr->cores[i]);
+        }
+        list_item->value = cpu_item;
+        list_item->next = head;
+        head = list_item;
+    }
+    return head;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v3 2/2] spapr: implement query-hotpluggable-cpus QMP command

[Bharata B Rao]

On Tue, Mar 15, 2016 at 02:24:08PM +0100, Igor Mammedov wrote:
> it returns a list of present/possible to hotplug CPU
> objects with a list of properties to use with
> device_add.
> 
> in spapr case returned list would looks like:
> -> { "execute": "query-hotpluggable-cpus" }
> <- {"return": [
>      { "props": { "core": 1 }, "type": "spapr-cpu-core",
>        "vcpus-count": 2 },
>      { "props": { "core": 0 }, "type": "spapr-cpu-core",
>        "vcpus-count": 2,
>        "qom-path": "/machine/unattached/device[0]"}
>    ]}'
> 
> TODO:
>   add 'node' property for core <-> numa node mapping
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> it's only compile tested
> ---
>  hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b1e9ba2..e1ce983 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -65,6 +65,7 @@
>  #include "hw/compat.h"
>  #include "qemu-common.h"
>  #include "hw/ppc/spapr_cpu_core.h"
> +#include "qmp-commands.h"
> 
>  #include <libfdt.h>
> 
> @@ -2399,6 +2400,37 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
>      return cpu_index / smp_threads / smp_cores;
>  }
> 
> +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> +{
> +    int i;
> +    HotpluggableCPUList *head = NULL;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    int spapr_max_cores = max_cpus / smp_threads;
> +
> +    for (i = 0; i < spapr_max_cores; i++) {
> +        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> +        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
> +        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
> +
> +        cpu_item->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> +        cpu_item->vcpus_count = smp_threads;

Shouldn't this be fetched from "threads" property of the core device
instead of directly using smp_threads ? But again, what that would mean
for not-yet-plugged in cores and how to get that for them is a question.

Regards,
Bharata.

Re: [Qemu-devel] [RFC v3 2/2] spapr: implement query-hotpluggable-cpus QMP command

[David Gibson]

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

On Wed, 16 Mar 2016 10:49:41 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Mar 15, 2016 at 02:24:08PM +0100, Igor Mammedov wrote:
> > it returns a list of present/possible to hotplug CPU
> > objects with a list of properties to use with
> > device_add.
> > 
> > in spapr case returned list would looks like:  
> > -> { "execute": "query-hotpluggable-cpus" }  
> > <- {"return": [
> >      { "props": { "core": 1 }, "type": "spapr-cpu-core",
> >        "vcpus-count": 2 },
> >      { "props": { "core": 0 }, "type": "spapr-cpu-core",
> >        "vcpus-count": 2,
> >        "qom-path": "/machine/unattached/device[0]"}
> >    ]}'
> > 
> > TODO:
> >   add 'node' property for core <-> numa node mapping
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > it's only compile tested
> > ---
> >  hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b1e9ba2..e1ce983 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -65,6 +65,7 @@
> >  #include "hw/compat.h"
> >  #include "qemu-common.h"
> >  #include "hw/ppc/spapr_cpu_core.h"
> > +#include "qmp-commands.h"
> > 
> >  #include <libfdt.h>
> > 
> > @@ -2399,6 +2400,37 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> >      return cpu_index / smp_threads / smp_cores;
> >  }
> > 
> > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > +{
> > +    int i;
> > +    HotpluggableCPUList *head = NULL;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    int spapr_max_cores = max_cpus / smp_threads;
> > +
> > +    for (i = 0; i < spapr_max_cores; i++) {
> > +        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> > +        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
> > +        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
> > +
> > +        cpu_item->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > +        cpu_item->vcpus_count = smp_threads;  
> 
> Shouldn't this be fetched from "threads" property of the core device
> instead of directly using smp_threads ? But again, what that would mean
> for not-yet-plugged in cores and how to get that for them is a question.

Yeah, I think Igor's patch is correct here.  The information flow goes
the other direction: the machine type code advertises smp_threads here,
which management then passes back to the device_add spapr-core in the
threads property.

-- 
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] [RFC v3 2/2] spapr: implement query-hotpluggable-cpus QMP command

[Igor Mammedov]

On Wed, 16 Mar 2016 10:49:41 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Mar 15, 2016 at 02:24:08PM +0100, Igor Mammedov wrote:
> > it returns a list of present/possible to hotplug CPU
> > objects with a list of properties to use with
> > device_add.
> > 
> > in spapr case returned list would looks like:  
> > -> { "execute": "query-hotpluggable-cpus" }  
> > <- {"return": [
> >      { "props": { "core": 1 }, "type": "spapr-cpu-core",
> >        "vcpus-count": 2 },
> >      { "props": { "core": 0 }, "type": "spapr-cpu-core",
> >        "vcpus-count": 2,
> >        "qom-path": "/machine/unattached/device[0]"}
> >    ]}'
> > 
> > TODO:
> >   add 'node' property for core <-> numa node mapping
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > it's only compile tested
> > ---
> >  hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b1e9ba2..e1ce983 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -65,6 +65,7 @@
> >  #include "hw/compat.h"
> >  #include "qemu-common.h"
> >  #include "hw/ppc/spapr_cpu_core.h"
> > +#include "qmp-commands.h"
> > 
> >  #include <libfdt.h>
> > 
> > @@ -2399,6 +2400,37 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> >      return cpu_index / smp_threads / smp_cores;
> >  }
> > 
> > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > +{
> > +    int i;
> > +    HotpluggableCPUList *head = NULL;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    int spapr_max_cores = max_cpus / smp_threads;
> > +
> > +    for (i = 0; i < spapr_max_cores; i++) {
> > +        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> > +        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
> > +        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
> > +
> > +        cpu_item->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > +        cpu_item->vcpus_count = smp_threads;  
> 
> Shouldn't this be fetched from "threads" property of the core device
> instead of directly using smp_threads ? But again, what that would mean
> for not-yet-plugged in cores and how to get that for them is a question.
my understanding is that libvirt wants to know that info for unplugged
CPUs as well and machine model + core(cpu_model) combo would fix it
at machine startup time for current use case.

> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Christian Borntraeger]

On 03/15/2016 02:24 PM, Igor Mammedov wrote:
> Changes since v2:
>  - rebase on top of hte lates spapr cpu hotpug series
>  - add 'vcpus-count' field, pkrempa@redhat.com
>  - s/CpuInstanceProps/CpuInstanceProperties/
>  - use '#optional' marker
>  - make "props" as always present even if it's empty
>  - fix JSON examples
>  - fix minor typos
>  - drop pre_plug spapr impl out of series as not related to QMP command
>  - drop generic pre hotplug callback as not related to QMP command
> 
> Changes since RFC:
>  - drop arch_id
>  - move CPU properties into separate structure
>  - target implements its own qmp callback version
>  - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
>                       https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
>     - convert slot name to core id hack
>     - drop links
>     - add generic pre hotplug callback
>     - implement query-hotpluggable-cpus
> 
> The first patch (QMP API) in this series could go in first
> allowing individual targets to post their hotplug
> implementation independently on top of it.
> 
> Igor Mammedov (2):
>   QMP: add query-hotpluggable-cpus
>   spapr: implement query-hotpluggable-cpus QMP command
> 
>  hw/ppc/spapr.c                      | 32 +++++++++++++++++++++++++++

i might have just missed that, do we also have the x86 implementation already available as
RFC somewhere?

Re: [Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Wed, 16 Mar 2016 20:29:59 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03/15/2016 02:24 PM, Igor Mammedov wrote:
> > Changes since v2:
> >  - rebase on top of hte lates spapr cpu hotpug series
> >  - add 'vcpus-count' field, pkrempa@redhat.com
> >  - s/CpuInstanceProps/CpuInstanceProperties/
> >  - use '#optional' marker
> >  - make "props" as always present even if it's empty
> >  - fix JSON examples
> >  - fix minor typos
> >  - drop pre_plug spapr impl out of series as not related to QMP command
> >  - drop generic pre hotplug callback as not related to QMP command
> > 
> > Changes since RFC:
> >  - drop arch_id
> >  - move CPU properties into separate structure
> >  - target implements its own qmp callback version
> >  - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
> >                       https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
> >     - convert slot name to core id hack
> >     - drop links
> >     - add generic pre hotplug callback
> >     - implement query-hotpluggable-cpus
> > 
> > The first patch (QMP API) in this series could go in first
> > allowing individual targets to post their hotplug
> > implementation independently on top of it.
> > 
> > Igor Mammedov (2):
> >   QMP: add query-hotpluggable-cpus
> >   spapr: implement query-hotpluggable-cpus QMP command
> > 
> >  hw/ppc/spapr.c                      | 32 +++++++++++++++++++++++++++  
> 
> i might have just missed that, do we also have the x86 implementation already available as
> RFC somewhere?
device_add x86-cpu, hasn't been posted yet, but I'm working on it.
If you are asking about query-hotpluggable-cpus command with
x86 backend then it's been posted as v1 of this RFC
https://patchwork.ozlabs.org/patch/583036/

and current v3 is what QMP interface evolved to based on feedback
for QEMU and libvirt developers.

x86 backend shouldn't be hard to respin as prerequisite patches
for it were just merged, I plan to stick that patch in
device_add enablement series.

Re: [Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> Changes since v2:
>  - rebase on top of hte lates spapr cpu hotpug series
>  - add 'vcpus-count' field, pkrempa@redhat.com
>  - s/CpuInstanceProps/CpuInstanceProperties/
>  - use '#optional' marker
>  - make "props" as always present even if it's empty
>  - fix JSON examples
>  - fix minor typos
>  - drop pre_plug spapr impl out of series as not related to QMP command
>  - drop generic pre hotplug callback as not related to QMP command
>
> Changes since RFC:
>  - drop arch_id
>  - move CPU properties into separate structure
>  - target implements its own qmp callback version
>  - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
>                       https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
>     - convert slot name to core id hack
>     - drop links
>     - add generic pre hotplug callback
>     - implement query-hotpluggable-cpus
>
> The first patch (QMP API) in this series could go in first
> allowing individual targets to post their hotplug
> implementation independently on top of it.

We discussed the need for a yet another ad hoc query command.  Can you
please summarize the discussion and its conclusion?  Explaining *why* a
feature is needed is always a good idea.  Cover letter and commit
messages are good places for it.

Re: [Qemu-devel] [PATCH v3 0/2] spapr: QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Tue, 22 Mar 2016 15:22:59 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > Changes since v2:
> >  - rebase on top of hte lates spapr cpu hotpug series
> >  - add 'vcpus-count' field, pkrempa@redhat.com
> >  - s/CpuInstanceProps/CpuInstanceProperties/
> >  - use '#optional' marker
> >  - make "props" as always present even if it's empty
> >  - fix JSON examples
> >  - fix minor typos
> >  - drop pre_plug spapr impl out of series as not related to QMP command
> >  - drop generic pre hotplug callback as not related to QMP command
> >
> > Changes since RFC:
> >  - drop arch_id
> >  - move CPU properties into separate structure
> >  - target implements its own qmp callback version
> >  - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
> >                       https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
> >     - convert slot name to core id hack
> >     - drop links
> >     - add generic pre hotplug callback
> >     - implement query-hotpluggable-cpus
> >
> > The first patch (QMP API) in this series could go in first
> > allowing individual targets to post their hotplug
> > implementation independently on top of it.  
> 
> We discussed the need for a yet another ad hoc query command.  Can you
> please summarize the discussion and its conclusion?  Explaining *why* a
> feature is needed is always a good idea.  Cover letter and commit
> messages are good places for it.
Summary would be:

that yet another ad hoc query QMP command is
 1. a single / atomic command 
 2. well documented
 3. fixed at compile time/staic so it's easy for mgmt to discover it.

while a considered alternative QOM interface via qom-get(path) is
 1. not atomic, i.e. requires a lot of qom-get commands over the wire
    to traverse QOM tree and fetch information
 2. not documented
 3. dynamically generated and would require more complicated coding
    even to create a simplistic interface similar to proposed QMP command
    (for example: possible_cpu QOM objects with a related properties)

I hope, I haven't missed anything.