[RFC,v0,5/6] qmp, spapr: Show hot-plugged/pluggable CPU slots in the Machine

Implement query cpu-slots that provides information about hot-plugged
as well as hot-pluggable CPU slots that the machine supports.

TODO: As Eric suggested use enum for type instead of str.
TODO: @hotplug-granularity probably isn't required.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/core/machine.c   |  19 +++++++++
 hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h |   4 ++
 qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx     |  47 ++++++++++++++++++++++
 5 files changed, 267 insertions(+)

On Thu, Feb 25, 2016 at 09:52:41PM +0530, Bharata B Rao wrote:
> Implement query cpu-slots that provides information about hot-plugged
> as well as hot-pluggable CPU slots that the machine supports.
> 
> TODO: As Eric suggested use enum for type instead of str.
> TODO: @hotplug-granularity probably isn't required.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/core/machine.c   |  19 +++++++++
>  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |   4 ++
>  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     |  47 ++++++++++++++++++++++
>  5 files changed, 267 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6d1a0d8..3055ef8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -17,6 +17,25 @@
>  #include "hw/sysbus.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "qmp-commands.h"
> +
> +/*
> + * QMP: query-cpu-slots
> + *
> + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> + */
> +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (!mc->cpu_slots) {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +        return NULL;
> +    }
> +
> +    return mc->cpu_slots(ms);
> +}
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 780cd00..b76ed85 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
>      return cpu_index / smp_threads / smp_cores;
>  }
>  
> +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    CPUInfoList ***prev = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> +        CPUInfo *s = g_new0(CPUInfo, 1);
> +        CPUState *cpu = CPU(obj);
> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);

Since you're assuming a POWERPC_CPU here, you should probably check
that in the object_dynamic_cast() above.

> +
> +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> +        s->type = g_strdup(object_get_typename(obj));
> +        s->thread = cpu->cpu_index;
> +        s->has_thread = true;

Unlike Igor's patch, you are attaching this thread information to,
well, a thread, not a cpu slot.  So I don't think the has_thread,
has_core etc. booleans are useful any more.

> +        s->core = cpu->cpu_index / smp_threads;
> +        s->has_core = true;
> +        if (mc->cpu_index_to_socket_id) {
> +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> +        } else {
> +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> +        }
> +        s->has_socket = true;
> +        s->node = cpu->numa_node;
> +        s->has_node = true;
> +        s->qom_path = object_get_canonical_path(obj);
> +        s->has_qom_path = true;
> +
> +        elem->value = s;
> +        elem->next = NULL;
> +        **prev = elem;
> +        *prev = &elem->next;

Hmm.. the dt_id is the only power specific thing, here, wonder if
there's a way we could make this generic.

> +    }
> +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);

Hmm.. IIUC this can be called either on a core, or on a thread, with
somewhat different behaviour.  That seems dangerously subtle.

> +    return 0;
> +}
> +
> +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> +{
> +    CPUSlotInfoList *head = NULL;
> +    CPUSlotInfoList **prev = &head;
> +    Object *root_container;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    /*
> +     * TODO: There surely must be a better/easier way to walk all
> +     * the link properties of an object ?
> +     */
> +    root_container = container_get(object_get_root(), "/machine");
> +    object_property_iter_init(&iter, root_container);
> +
> +    while ((prop = object_property_iter_next(&iter))) {
> +        Object *obj;
> +        DeviceState *dev;
> +        CPUSlotInfoList *elem;
> +        CPUSlotInfo *s;
> +        CPUInfoList *cpu_head = NULL;
> +        CPUInfoList **cpu_prev = &cpu_head;
> +        sPAPRCPUCore *core;
> +
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }

Hmm.. we really out to have an is_link() helper, but that's not your
problem.

> +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> +            continue;
> +        }
> +
> +        elem = g_new0(CPUSlotInfoList, 1);
> +        s = g_new0(CPUSlotInfo, 1);
> +
> +        obj = object_property_get_link(root_container, prop->name, NULL);
> +        if (obj) {
> +            /* Slot populated */
> +            dev = DEVICE(obj);
> +            core = SPAPR_CPU_CORE(obj);
> +
> +            if (dev->id) {
> +                s->has_id = true;
> +                s->id = g_strdup(dev->id);
> +            }
> +            s->realized = object_property_get_bool(obj, "realized", NULL);
> +            s->nr_cpus = core->nr_threads;
> +            s->has_nr_cpus = true;
> +            s->qom_path = object_get_canonical_path(obj);
> +            s->has_qom_path = true;
> +            if (s->realized) {
> +                spapr_cpuinfo_list(obj, &cpu_prev);
> +            }
> +            s->has_cpus = true;
> +        } else {
> +            /* Slot empty */
> +            s->has_id = false;
> +            s->has_nr_cpus = false;
> +            s->has_qom_path = false;
> +            s->has_cpus = false;
> +            s->realized = false;

Um.. doesn't this leave out the cpu model and nr_threads for a
non-populated slot?  Which is crucial information for mgmt to populate
the slot.

I can't actually see that this interface gives you any information you
can't already see from the qom tree.

> +        }
> +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> +        s->slot_id = g_strdup(prop->name);
> +        s->cpus = cpu_head;
> +        elem->value = s;
> +        elem->next = NULL;
> +        *prev = elem;
> +        prev = &elem->next;

I also wonder if there's a way to use the visit_* interfaces to avoid
having to fully construct this list-of-lists.

I can't see any code that frees the info data structure either...

> +    }
> +    return head;
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> +    mc->cpu_slots = spapr_cpu_slots;
>  
>      smc->dr_lmb_enabled = true;
>      smc->dr_cpu_enabled = true;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..d888a02 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
>   *    Set only by old machines because they need to keep
>   *    compatibility on code that exposed QEMU_VERSION to guests in
>   *    the past (and now use qemu_hw_version()).
> + * @cpu_slots:
> + *    Provides information about populated and yet-to-be populated
> + *    CPU slots in the machine. Used by QMP query-cpu-slots.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -99,6 +102,7 @@ struct MachineClass {
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
>      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
>  };
>  
>  /**
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8d04897..e9a52a2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4083,3 +4083,88 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @CPUInfo:
> +#
> +# Information about CPUs
> +#
> +# @arch-id: Arch-specific ID for the CPU.
> +#
> +# @type: QOM type of the CPU.
> +#
> +# @thread: Thread ID of the CPU.
> +#
> +# @core: Core ID of the CPU.
> +#
> +# @socket: Socket ID of the CPU.
> +#
> +# @node: NUMA node to which the CPU belongs.
> +#
> +# @qom-path: QOM path of the CPU object
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'CPUInfo',
> +  'data': { 'arch-id': 'int',
> +            'type': 'str',
> +            '*thread': 'int',
> +            '*core': 'int',
> +            '*socket' : 'int',
> +            '*node' : 'int',
> +            '*qom-path': 'str'
> +          }
> +}
> +
> +##
> +# @CPUSlotInfo:
> +#
> +# Information about CPU Slots
> +#
> +# @id: Device ID of the CPU composite object that occupies the slot.
> +#
> +# @type: QOM type of the CPU composite object that occupies the slot.
> +#
> +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> +# can be thread, core or socket.
> +#
> +# @slot-id: Slot's identifier.
> +#
> +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> +#
> +# @realized: A boolean that indicates whether the slot is filled or empty.
> +#
> +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> +# this slot.
> +#
> +# @cpus: An array of @CPUInfo elements where each element describes one
> +# CPU that is part of this slot's CPU composite object.
> +#
> +# @type: QOM type
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'CPUSlotInfo',
> +  'data': { '*id': 'str',
> +            'type': 'str',
> +            'hotplug-granularity' : 'str',
> +            'slot-id' : 'str',
> +            '*qom-path': 'str',
> +            'realized': 'bool',
> +            '*nr-cpus': 'int',
> +            '*cpus' : ['CPUInfo']
> +          }
> +}
> +
> +##
> +# @query-cpu-slots:
> +#
> +# Returns information for all CPU slots
> +#
> +# Returns: a list of @CPUSlotInfo
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-cpu-slots', 'returns': ['CPUSlotInfo'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 085dc7d..185aa13 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4825,3 +4825,50 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +    {
> +        .name       = "query-cpu-slots",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_cpu_slots,
> +    },
> +
> +SQMP
> +@query-cpu-slots
> +--------------------
> +
> +Show CPU slots information
> +
> +Example:
> +-> { "execute": "query-cpu-slots" }
> +<- {"return": [{
> +                "slot-id": "core[2]",
> +                "hotplug-granularity": "core",
> +                "realized": false,
> +                "type": "spapr-cpu-core"
> +               },
> +               {
> +                "slot-id": "core[1]",
> +                "qom-path": "/machine/unattached/device[2]",
> +                "hotplug-granularity": "core",
> +                "realized": true,
> +                "type": "spapr-cpu-core"
> +                 "nr-cpus": 2,
> +                 "cpus": [
> +                   {"thread": 8,
> +                    "socket": 0,
> +                    "core": 1,
> +                    "arch-id": 8,
> +                    "qom-path": "/machine/unattached/device[2]/thread[0]",
> +                    "node": 0,
> +                    "type": "host-powerpc64-cpu"},
> +                   {"thread": 9,
> +                    "socket": 0,
> +                    "core": 1,
> +                    "arch-id": 9,
> +                    "qom-path": "/machine/unattached/device[2]/thread[2]",
> +                    "node": 0,
> +                    "type": "host-powerpc64-cpu"}]
> +               }
> +   ]}

David, I am responding to just one comment here and will respond to others
in detail later.

On Fri, Feb 26, 2016 at 03:03:17PM +1100, David Gibson wrote:
> 
> > +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > +            continue;
> > +        }
> > +
> > +        elem = g_new0(CPUSlotInfoList, 1);
> > +        s = g_new0(CPUSlotInfo, 1);
> > +
> > +        obj = object_property_get_link(root_container, prop->name, NULL);
> > +        if (obj) {
> > +            /* Slot populated */
> > +            dev = DEVICE(obj);
> > +            core = SPAPR_CPU_CORE(obj);
> > +
> > +            if (dev->id) {
> > +                s->has_id = true;
> > +                s->id = g_strdup(dev->id);
> > +            }
> > +            s->realized = object_property_get_bool(obj, "realized", NULL);
> > +            s->nr_cpus = core->nr_threads;
> > +            s->has_nr_cpus = true;
> > +            s->qom_path = object_get_canonical_path(obj);
> > +            s->has_qom_path = true;
> > +            if (s->realized) {
> > +                spapr_cpuinfo_list(obj, &cpu_prev);
> > +            }
> > +            s->has_cpus = true;
> > +        } else {
> > +            /* Slot empty */
> > +            s->has_id = false;
> > +            s->has_nr_cpus = false;
> > +            s->has_qom_path = false;
> > +            s->has_cpus = false;
> > +            s->realized = false;
> 
> Um.. doesn't this leave out the cpu model and nr_threads for a
> non-populated slot?  Which is crucial information for mgmt to populate
> the slot.

Actually if we don't allow heterogenous configurations, management currently
knows how to get model and threads that have already been specified with
-cpu and -smp global options respectively.

If we want to support heterogenous configurations, should we then have
supported model types and thread values listed here ? At the moment I
am not sure what those supported types and thread values could be.

Regards,
Bharata.

On 02/25/2016 09:22 AM, Bharata B Rao wrote:
> Implement query cpu-slots that provides information about hot-plugged
> as well as hot-pluggable CPU slots that the machine supports.
> 
> TODO: As Eric suggested use enum for type instead of str.
> TODO: @hotplug-granularity probably isn't required.

I guess this is still marked TODO because the series is still RFC?

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---

> +++ b/qapi-schema.json
> @@ -4083,3 +4083,88 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @CPUInfo:
> +#
> +# Information about CPUs
> +#
> +# @arch-id: Arch-specific ID for the CPU.
> +#
> +# @type: QOM type of the CPU.
> +#
> +# @thread: Thread ID of the CPU.
> +#
> +# @core: Core ID of the CPU.
> +#
> +# @socket: Socket ID of the CPU.
> +#
> +# @node: NUMA node to which the CPU belongs.

Please add the '#optional' tag to the fields which are not always present.

> +#
> +# @qom-path: QOM path of the CPU object
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'CPUInfo',
> +  'data': { 'arch-id': 'int',
> +            'type': 'str',

The TODO in the commit message mentions that this should be converted to
an enum.

> +            '*thread': 'int',
> +            '*core': 'int',
> +            '*socket' : 'int',
> +            '*node' : 'int',
> +            '*qom-path': 'str'
> +          }

But looking better than the previous round.

On 25.02.2016 17:22, Bharata B Rao wrote:
> Implement query cpu-slots that provides information about hot-plugged
> as well as hot-pluggable CPU slots that the machine supports.
> 
> TODO: As Eric suggested use enum for type instead of str.
> TODO: @hotplug-granularity probably isn't required.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/core/machine.c   |  19 +++++++++
>  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |   4 ++
>  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     |  47 ++++++++++++++++++++++
>  5 files changed, 267 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6d1a0d8..3055ef8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -17,6 +17,25 @@
>  #include "hw/sysbus.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "qmp-commands.h"
> +
> +/*
> + * QMP: query-cpu-slots
> + *
> + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> + */
> +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (!mc->cpu_slots) {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +        return NULL;
> +    }
> +
> +    return mc->cpu_slots(ms);
> +}
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 780cd00..b76ed85 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
>      return cpu_index / smp_threads / smp_cores;
>  }
>  
> +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    CPUInfoList ***prev = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> +        CPUInfo *s = g_new0(CPUInfo, 1);
> +        CPUState *cpu = CPU(obj);
> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> +
> +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> +        s->type = g_strdup(object_get_typename(obj));
> +        s->thread = cpu->cpu_index;
> +        s->has_thread = true;
> +        s->core = cpu->cpu_index / smp_threads;
> +        s->has_core = true;
> +        if (mc->cpu_index_to_socket_id) {
> +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> +        } else {
> +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> +        }
> +        s->has_socket = true;
> +        s->node = cpu->numa_node;
> +        s->has_node = true;
> +        s->qom_path = object_get_canonical_path(obj);
> +        s->has_qom_path = true;
> +
> +        elem->value = s;
> +        elem->next = NULL;
> +        **prev = elem;
> +        *prev = &elem->next;
> +    }
> +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> +    return 0;
> +}
> +
> +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> +{
> +    CPUSlotInfoList *head = NULL;
> +    CPUSlotInfoList **prev = &head;
> +    Object *root_container;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    /*
> +     * TODO: There surely must be a better/easier way to walk all
> +     * the link properties of an object ?
> +     */
> +    root_container = container_get(object_get_root(), "/machine");
> +    object_property_iter_init(&iter, root_container);
> +
> +    while ((prop = object_property_iter_next(&iter))) {
> +        Object *obj;
> +        DeviceState *dev;
> +        CPUSlotInfoList *elem;
> +        CPUSlotInfo *s;
> +        CPUInfoList *cpu_head = NULL;
> +        CPUInfoList **cpu_prev = &cpu_head;
> +        sPAPRCPUCore *core;
> +
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }
> +
> +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> +            continue;
> +        }
> +
> +        elem = g_new0(CPUSlotInfoList, 1);
> +        s = g_new0(CPUSlotInfo, 1);
> +
> +        obj = object_property_get_link(root_container, prop->name, NULL);
> +        if (obj) {
> +            /* Slot populated */
> +            dev = DEVICE(obj);
> +            core = SPAPR_CPU_CORE(obj);
> +
> +            if (dev->id) {
> +                s->has_id = true;
> +                s->id = g_strdup(dev->id);
> +            }
> +            s->realized = object_property_get_bool(obj, "realized", NULL);
> +            s->nr_cpus = core->nr_threads;
> +            s->has_nr_cpus = true;
> +            s->qom_path = object_get_canonical_path(obj);
> +            s->has_qom_path = true;
> +            if (s->realized) {
> +                spapr_cpuinfo_list(obj, &cpu_prev);
> +            }
> +            s->has_cpus = true;
> +        } else {
> +            /* Slot empty */
> +            s->has_id = false;
> +            s->has_nr_cpus = false;
> +            s->has_qom_path = false;
> +            s->has_cpus = false;
> +            s->realized = false;
> +        }

I think you could drop that whole else-path since you've already used
g_new0 to make sure that the memory of the new CPUSlotInfo is cleared.

> +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> +        s->slot_id = g_strdup(prop->name);
> +        s->cpus = cpu_head;
> +        elem->value = s;
> +        elem->next = NULL;
> +        *prev = elem;
> +        prev = &elem->next;
> +    }
> +    return head;
> +}

 Thomas

On Fri, Feb 26, 2016 at 08:58:05AM -0700, Eric Blake wrote:
> On 02/25/2016 09:22 AM, Bharata B Rao wrote:
> > Implement query cpu-slots that provides information about hot-plugged
> > as well as hot-pluggable CPU slots that the machine supports.
> > 
> > TODO: As Eric suggested use enum for type instead of str.
> > TODO: @hotplug-granularity probably isn't required.
> 
> I guess this is still marked TODO because the series is still RFC?

Yes.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,88 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @CPUInfo:
> > +#
> > +# Information about CPUs
> > +#
> > +# @arch-id: Arch-specific ID for the CPU.
> > +#
> > +# @type: QOM type of the CPU.
> > +#
> > +# @thread: Thread ID of the CPU.
> > +#
> > +# @core: Core ID of the CPU.
> > +#
> > +# @socket: Socket ID of the CPU.
> > +#
> > +# @node: NUMA node to which the CPU belongs.
> 
> Please add the '#optional' tag to the fields which are not always present.
> 

Sure.

> > +#
> > +# @qom-path: QOM path of the CPU object
> > +#
> > +# Since: 2.6
> > +##
> > +
> > +{ 'struct': 'CPUInfo',
> > +  'data': { 'arch-id': 'int',
> > +            'type': 'str',
> 
> The TODO in the commit message mentions that this should be converted to
> an enum.
> 
> > +            '*thread': 'int',
> > +            '*core': 'int',
> > +            '*socket' : 'int',
> > +            '*node' : 'int',
> > +            '*qom-path': 'str'
> > +          }
> 
> But looking better than the previous round.

Thanks for the review. Do you have any comments on the applicability/suitability
of this interface from libvirt point of view for performing device_add based
CPU hotplug ?

Regards,
Bharata.

On Thu, 25 Feb 2016 21:52:41 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Implement query cpu-slots that provides information about hot-plugged
> as well as hot-pluggable CPU slots that the machine supports.
> 
> TODO: As Eric suggested use enum for type instead of str.
> TODO: @hotplug-granularity probably isn't required.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/core/machine.c   |  19 +++++++++
>  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |   4 ++
>  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     |  47 ++++++++++++++++++++++
>  5 files changed, 267 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6d1a0d8..3055ef8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -17,6 +17,25 @@
>  #include "hw/sysbus.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "qmp-commands.h"
> +
> +/*
> + * QMP: query-cpu-slots
> + *
> + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> + */
> +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (!mc->cpu_slots) {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +        return NULL;
> +    }
> +
> +    return mc->cpu_slots(ms);
> +}
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 780cd00..b76ed85 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
>      return cpu_index / smp_threads / smp_cores;
>  }
>  
> +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    CPUInfoList ***prev = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> +        CPUInfo *s = g_new0(CPUInfo, 1);
> +        CPUState *cpu = CPU(obj);
> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> +
> +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> +        s->type = g_strdup(object_get_typename(obj));
> +        s->thread = cpu->cpu_index;
> +        s->has_thread = true;
> +        s->core = cpu->cpu_index / smp_threads;
> +        s->has_core = true;
> +        if (mc->cpu_index_to_socket_id) {
> +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> +        } else {
> +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> +        }
> +        s->has_socket = true;
> +        s->node = cpu->numa_node;
> +        s->has_node = true;
> +        s->qom_path = object_get_canonical_path(obj);
> +        s->has_qom_path = true;
> +
> +        elem->value = s;
> +        elem->next = NULL;
> +        **prev = elem;
> +        *prev = &elem->next;
> +    }
> +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> +    return 0;
> +}
> +
> +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> +{
> +    CPUSlotInfoList *head = NULL;
> +    CPUSlotInfoList **prev = &head;
> +    Object *root_container;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    /*
> +     * TODO: There surely must be a better/easier way to walk all
> +     * the link properties of an object ?
> +     */
> +    root_container = container_get(object_get_root(), "/machine");
> +    object_property_iter_init(&iter, root_container);
> +
> +    while ((prop = object_property_iter_next(&iter))) {
> +        Object *obj;
> +        DeviceState *dev;
> +        CPUSlotInfoList *elem;
> +        CPUSlotInfo *s;
> +        CPUInfoList *cpu_head = NULL;
> +        CPUInfoList **cpu_prev = &cpu_head;
> +        sPAPRCPUCore *core;
> +
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }
> +
> +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> +            continue;
> +        }
> +
> +        elem = g_new0(CPUSlotInfoList, 1);
> +        s = g_new0(CPUSlotInfo, 1);
> +
> +        obj = object_property_get_link(root_container, prop->name, NULL);
> +        if (obj) {
> +            /* Slot populated */
> +            dev = DEVICE(obj);
> +            core = SPAPR_CPU_CORE(obj);
> +
> +            if (dev->id) {
> +                s->has_id = true;
> +                s->id = g_strdup(dev->id);
> +            }
> +            s->realized = object_property_get_bool(obj, "realized", NULL);
> +            s->nr_cpus = core->nr_threads;
> +            s->has_nr_cpus = true;
> +            s->qom_path = object_get_canonical_path(obj);
> +            s->has_qom_path = true;
> +            if (s->realized) {
> +                spapr_cpuinfo_list(obj, &cpu_prev);
> +            }
> +            s->has_cpus = true;
> +        } else {
> +            /* Slot empty */
> +            s->has_id = false;
> +            s->has_nr_cpus = false;
> +            s->has_qom_path = false;
> +            s->has_cpus = false;
> +            s->realized = false;
> +        }
> +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> +        s->slot_id = g_strdup(prop->name);
> +        s->cpus = cpu_head;
> +        elem->value = s;
> +        elem->next = NULL;
> +        *prev = elem;
> +        prev = &elem->next;
> +    }
> +    return head;
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> +    mc->cpu_slots = spapr_cpu_slots;
>  
>      smc->dr_lmb_enabled = true;
>      smc->dr_cpu_enabled = true;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..d888a02 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
>   *    Set only by old machines because they need to keep
>   *    compatibility on code that exposed QEMU_VERSION to guests in
>   *    the past (and now use qemu_hw_version()).
> + * @cpu_slots:
> + *    Provides information about populated and yet-to-be populated
> + *    CPU slots in the machine. Used by QMP query-cpu-slots.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -99,6 +102,7 @@ struct MachineClass {
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
>      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
>  };
>  
>  /**
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8d04897..e9a52a2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4083,3 +4083,88 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @CPUInfo:
> +#
> +# Information about CPUs
> +#
> +# @arch-id: Arch-specific ID for the CPU.
> +#
> +# @type: QOM type of the CPU.
> +#
> +# @thread: Thread ID of the CPU.
> +#
> +# @core: Core ID of the CPU.
> +#
> +# @socket: Socket ID of the CPU.
> +#
> +# @node: NUMA node to which the CPU belongs.
> +#
> +# @qom-path: QOM path of the CPU object
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'CPUInfo',
> +  'data': { 'arch-id': 'int',
> +            'type': 'str',
> +            '*thread': 'int',
> +            '*core': 'int',
> +            '*socket' : 'int',
> +            '*node' : 'int',
> +            '*qom-path': 'str'
> +          }
> +}
> +
> +##
> +# @CPUSlotInfo:
> +#
> +# Information about CPU Slots
> +#
> +# @id: Device ID of the CPU composite object that occupies the slot.
> +#
> +# @type: QOM type of the CPU composite object that occupies the slot.
> +#
> +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> +# can be thread, core or socket.
> +#
> +# @slot-id: Slot's identifier.
> +#
> +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> +#
> +# @realized: A boolean that indicates whether the slot is filled or empty.
> +#
> +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> +# this slot.
> +#
> +# @cpus: An array of @CPUInfo elements where each element describes one
> +# CPU that is part of this slot's CPU composite object.
> +#
> +# @type: QOM type
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'CPUSlotInfo',
> +  'data': { '*id': 'str',
> +            'type': 'str',
> +            'hotplug-granularity' : 'str',
Does it convey any useful info, if yes how it will be used by mgmt?

> +            'slot-id' : 'str',
> +            '*qom-path': 'str',
> +            'realized': 'bool',
field's redundant, presence of qom-path answers this question

> +            '*nr-cpus': 'int',
> +            '*cpus' : ['CPUInfo']
I'd suggest to drop above 2 fields as it's impl dependent,
qom-path already point's to socket/core/thread object and
its internal composition can be explored by other means if needed.

Moreover 'CPUInfo' is confusing and sort of conflicts with existing
'CpuInfo'.
I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
existing thread enumeration's already implemented query-cpus.
 
> +          }
> +}
What I miss here is that CPUSlotInfo doesn't provide any
information to about where CPU might be hotplugged to.

Maybe use following tuple instead of slot-id?

{ 'struct': 'CPUSlotProperties',
  'data': { '*node': 'int',
            '*socket': 'int',
            '*core': 'int',
            '*thread': 'int'
  }
}

it's generic for the most targets, which should work for spapr, s390, x86, ARM
and could be extended for other cases adding other board specific
properties if it's needed.
The all mgmt would have to do for hotplug is to execute:
 device_add ${type},${CPUSlotProperties}


> +
> +##
> +# @query-cpu-slots:
> +#
> +# Returns information for all CPU slots
> +#
> +# Returns: a list of @CPUSlotInfo
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-cpu-slots', 'returns': ['CPUSlotInfo'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 085dc7d..185aa13 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4825,3 +4825,50 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +    {
> +        .name       = "query-cpu-slots",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_cpu_slots,
> +    },
> +
> +SQMP
> +@query-cpu-slots
> +--------------------
> +
> +Show CPU slots information
> +
> +Example:
> +-> { "execute": "query-cpu-slots" }
> +<- {"return": [{
> +                "slot-id": "core[2]",
> +                "hotplug-granularity": "core",
> +                "realized": false,
> +                "type": "spapr-cpu-core"
> +               },
> +               {
> +                "slot-id": "core[1]",
> +                "qom-path": "/machine/unattached/device[2]",
> +                "hotplug-granularity": "core",
> +                "realized": true,
> +                "type": "spapr-cpu-core"
> +                 "nr-cpus": 2,
> +                 "cpus": [
> +                   {"thread": 8,
> +                    "socket": 0,
> +                    "core": 1,
> +                    "arch-id": 8,
> +                    "qom-path": "/machine/unattached/device[2]/thread[0]",
> +                    "node": 0,
> +                    "type": "host-powerpc64-cpu"},
> +                   {"thread": 9,
> +                    "socket": 0,
> +                    "core": 1,
> +                    "arch-id": 9,
> +                    "qom-path": "/machine/unattached/device[2]/thread[2]",
> +                    "node": 0,
> +                    "type": "host-powerpc64-cpu"}]
> +               }
> +   ]}

On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:
> On Thu, 25 Feb 2016 21:52:41 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Implement query cpu-slots that provides information about hot-plugged
> > as well as hot-pluggable CPU slots that the machine supports.
> > 
> > TODO: As Eric suggested use enum for type instead of str.
> > TODO: @hotplug-granularity probably isn't required.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/core/machine.c   |  19 +++++++++
> >  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h |   4 ++
> >  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx     |  47 ++++++++++++++++++++++
> >  5 files changed, 267 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 6d1a0d8..3055ef8 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -17,6 +17,25 @@
> >  #include "hw/sysbus.h"
> >  #include "sysemu/sysemu.h"
> >  #include "qemu/error-report.h"
> > +#include "qmp-commands.h"
> > +
> > +/*
> > + * QMP: query-cpu-slots
> > + *
> > + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> > + */
> > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > +    if (!mc->cpu_slots) {
> > +        error_setg(errp, QERR_UNSUPPORTED);
> > +        return NULL;
> > +    }
> > +
> > +    return mc->cpu_slots(ms);
> > +}
> >  
> >  static char *machine_get_accel(Object *obj, Error **errp)
> >  {
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 780cd00..b76ed85 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> >      return cpu_index / smp_threads / smp_cores;
> >  }
> >  
> > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > +    CPUInfoList ***prev = opaque;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> > +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > +        CPUInfo *s = g_new0(CPUInfo, 1);
> > +        CPUState *cpu = CPU(obj);
> > +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > +
> > +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > +        s->type = g_strdup(object_get_typename(obj));
> > +        s->thread = cpu->cpu_index;
> > +        s->has_thread = true;
> > +        s->core = cpu->cpu_index / smp_threads;
> > +        s->has_core = true;
> > +        if (mc->cpu_index_to_socket_id) {
> > +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > +        } else {
> > +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > +        }
> > +        s->has_socket = true;
> > +        s->node = cpu->numa_node;
> > +        s->has_node = true;
> > +        s->qom_path = object_get_canonical_path(obj);
> > +        s->has_qom_path = true;
> > +
> > +        elem->value = s;
> > +        elem->next = NULL;
> > +        **prev = elem;
> > +        *prev = &elem->next;
> > +    }
> > +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > +    return 0;
> > +}
> > +
> > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > +{
> > +    CPUSlotInfoList *head = NULL;
> > +    CPUSlotInfoList **prev = &head;
> > +    Object *root_container;
> > +    ObjectProperty *prop;
> > +    ObjectPropertyIterator iter;
> > +
> > +    /*
> > +     * TODO: There surely must be a better/easier way to walk all
> > +     * the link properties of an object ?
> > +     */
> > +    root_container = container_get(object_get_root(), "/machine");
> > +    object_property_iter_init(&iter, root_container);
> > +
> > +    while ((prop = object_property_iter_next(&iter))) {
> > +        Object *obj;
> > +        DeviceState *dev;
> > +        CPUSlotInfoList *elem;
> > +        CPUSlotInfo *s;
> > +        CPUInfoList *cpu_head = NULL;
> > +        CPUInfoList **cpu_prev = &cpu_head;
> > +        sPAPRCPUCore *core;
> > +
> > +        if (!strstart(prop->type, "link<", NULL)) {
> > +            continue;
> > +        }
> > +
> > +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > +            continue;
> > +        }
> > +
> > +        elem = g_new0(CPUSlotInfoList, 1);
> > +        s = g_new0(CPUSlotInfo, 1);
> > +
> > +        obj = object_property_get_link(root_container, prop->name, NULL);
> > +        if (obj) {
> > +            /* Slot populated */
> > +            dev = DEVICE(obj);
> > +            core = SPAPR_CPU_CORE(obj);
> > +
> > +            if (dev->id) {
> > +                s->has_id = true;
> > +                s->id = g_strdup(dev->id);
> > +            }
> > +            s->realized = object_property_get_bool(obj, "realized", NULL);
> > +            s->nr_cpus = core->nr_threads;
> > +            s->has_nr_cpus = true;
> > +            s->qom_path = object_get_canonical_path(obj);
> > +            s->has_qom_path = true;
> > +            if (s->realized) {
> > +                spapr_cpuinfo_list(obj, &cpu_prev);
> > +            }
> > +            s->has_cpus = true;
> > +        } else {
> > +            /* Slot empty */
> > +            s->has_id = false;
> > +            s->has_nr_cpus = false;
> > +            s->has_qom_path = false;
> > +            s->has_cpus = false;
> > +            s->realized = false;
> > +        }
> > +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > +        s->slot_id = g_strdup(prop->name);
> > +        s->cpus = cpu_head;
> > +        elem->value = s;
> > +        elem->next = NULL;
> > +        *prev = elem;
> > +        prev = &elem->next;
> > +    }
> > +    return head;
> > +}
> > +
> >  static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >      hc->plug = spapr_machine_device_plug;
> >      hc->unplug = spapr_machine_device_unplug;
> >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > +    mc->cpu_slots = spapr_cpu_slots;
> >  
> >      smc->dr_lmb_enabled = true;
> >      smc->dr_cpu_enabled = true;
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 0f30959..d888a02 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> >   *    Set only by old machines because they need to keep
> >   *    compatibility on code that exposed QEMU_VERSION to guests in
> >   *    the past (and now use qemu_hw_version()).
> > + * @cpu_slots:
> > + *    Provides information about populated and yet-to-be populated
> > + *    CPU slots in the machine. Used by QMP query-cpu-slots.
> >   */
> >  struct MachineClass {
> >      /*< private >*/
> > @@ -99,6 +102,7 @@ struct MachineClass {
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >                                             DeviceState *dev);
> >      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> >  };
> >  
> >  /**
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 8d04897..e9a52a2 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,88 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @CPUInfo:
> > +#
> > +# Information about CPUs
> > +#
> > +# @arch-id: Arch-specific ID for the CPU.
> > +#
> > +# @type: QOM type of the CPU.
> > +#
> > +# @thread: Thread ID of the CPU.
> > +#
> > +# @core: Core ID of the CPU.
> > +#
> > +# @socket: Socket ID of the CPU.
> > +#
> > +# @node: NUMA node to which the CPU belongs.
> > +#
> > +# @qom-path: QOM path of the CPU object
> > +#
> > +# Since: 2.6
> > +##
> > +
> > +{ 'struct': 'CPUInfo',
> > +  'data': { 'arch-id': 'int',
> > +            'type': 'str',
> > +            '*thread': 'int',
> > +            '*core': 'int',
> > +            '*socket' : 'int',
> > +            '*node' : 'int',
> > +            '*qom-path': 'str'
> > +          }
> > +}
> > +
> > +##
> > +# @CPUSlotInfo:
> > +#
> > +# Information about CPU Slots
> > +#
> > +# @id: Device ID of the CPU composite object that occupies the slot.
> > +#
> > +# @type: QOM type of the CPU composite object that occupies the slot.
> > +#
> > +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> > +# can be thread, core or socket.
> > +#
> > +# @slot-id: Slot's identifier.
> > +#
> > +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> > +#
> > +# @realized: A boolean that indicates whether the slot is filled or empty.
> > +#
> > +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> > +# this slot.
> > +#
> > +# @cpus: An array of @CPUInfo elements where each element describes one
> > +# CPU that is part of this slot's CPU composite object.
> > +#
> > +# @type: QOM type
> > +#
> > +# Since: 2.6
> > +##
> > +
> > +{ 'struct': 'CPUSlotInfo',
> > +  'data': { '*id': 'str',
> > +            'type': 'str',
> > +            'hotplug-granularity' : 'str',
> Does it convey any useful info, if yes how it will be used by mgmt?

As I noted in the patch desc, this field is useless, will remove.

> 
> > +            'slot-id' : 'str',
> > +            '*qom-path': 'str',
> > +            'realized': 'bool',
> field's redundant, presence of qom-path answers this question

Ok, makes sense.

> 
> > +            '*nr-cpus': 'int',
> > +            '*cpus' : ['CPUInfo']
> I'd suggest to drop above 2 fields as it's impl dependent,
> qom-path already point's to socket/core/thread object and
> its internal composition can be explored by other means if needed.
> 
> Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> 'CpuInfo'.

Ah I see, should change the naming if we eventually stick with this
implementation.

> I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> existing thread enumeration's already implemented query-cpus.

Ok.

> 
> > +          }
> > +}
> What I miss here is that CPUSlotInfo doesn't provide any
> information to about where CPU might be hotplugged to.
> 
> Maybe use following tuple instead of slot-id?
> 
> { 'struct': 'CPUSlotProperties',
>   'data': { '*node': 'int',
>             '*socket': 'int',
>             '*core': 'int',
>             '*thread': 'int'
>   }
> }

Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
place where the CPU sits. Machine determines the type of the slot and it
can be socket slot, core slot or thread slot based on the granularity
of the hotplug supported by the machine. With this I don't see why
anything else apart from slot-id/slot-name is required to figure out where
to hoplug CPU.

In the current implementation, sPAPR hotplug is at core granularity.
CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
threads in the core and the CPU model of the threads are either machine
default or specified by user during hotplug time.

I believe NUMA node should be the property of the slot. The information
about which slots are part of which NUMA node should be known beforehand
at machine init time. Hotplugging CPU thread, core or socket to a slot will
make that thread, core or socket part of that NUMA node.

> it's generic for the most targets, which should work for spapr, s390, x86, ARM
> and could be extended for other cases adding other board specific
> properties if it's needed.

I don't about ARM, but s390 doesn't care about topology iiuc.

For sPAPR, as I described above, we don't care about sockets and hence
I believe we can live with hotplugging a core into a machine slot by
referring to the slot-id.

I see that CPUSlotProperties that you are suggesting here  would make sense
for x86 depending on the granularity at which hotplug is done. I know there
has been discussion on thread vs socket hotplug granularity for x86, has
there been a consensus on what it is going to be ?

Regards,
Bharata.

On Tue, 1 Mar 2016 14:39:51 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:
> > On Thu, 25 Feb 2016 21:52:41 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > Implement query cpu-slots that provides information about hot-plugged
> > > as well as hot-pluggable CPU slots that the machine supports.
> > > 
> > > TODO: As Eric suggested use enum for type instead of str.
> > > TODO: @hotplug-granularity probably isn't required.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/core/machine.c   |  19 +++++++++
> > >  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/boards.h |   4 ++
> > >  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
> > >  qmp-commands.hx     |  47 ++++++++++++++++++++++
> > >  5 files changed, 267 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 6d1a0d8..3055ef8 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -17,6 +17,25 @@
> > >  #include "hw/sysbus.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "qemu/error-report.h"
> > > +#include "qmp-commands.h"
> > > +
> > > +/*
> > > + * QMP: query-cpu-slots
> > > + *
> > > + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> > > + */
> > > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > +
> > > +    if (!mc->cpu_slots) {
> > > +        error_setg(errp, QERR_UNSUPPORTED);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return mc->cpu_slots(ms);
> > > +}
> > >  
> > >  static char *machine_get_accel(Object *obj, Error **errp)
> > >  {
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 780cd00..b76ed85 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> > >      return cpu_index / smp_threads / smp_cores;
> > >  }
> > >  
> > > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > > +{
> > > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > +    CPUInfoList ***prev = opaque;
> > > +
> > > +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> > > +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > > +        CPUInfo *s = g_new0(CPUInfo, 1);
> > > +        CPUState *cpu = CPU(obj);
> > > +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > > +
> > > +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > > +        s->type = g_strdup(object_get_typename(obj));
> > > +        s->thread = cpu->cpu_index;
> > > +        s->has_thread = true;
> > > +        s->core = cpu->cpu_index / smp_threads;
> > > +        s->has_core = true;
> > > +        if (mc->cpu_index_to_socket_id) {
> > > +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > > +        } else {
> > > +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > > +        }
> > > +        s->has_socket = true;
> > > +        s->node = cpu->numa_node;
> > > +        s->has_node = true;
> > > +        s->qom_path = object_get_canonical_path(obj);
> > > +        s->has_qom_path = true;
> > > +
> > > +        elem->value = s;
> > > +        elem->next = NULL;
> > > +        **prev = elem;
> > > +        *prev = &elem->next;
> > > +    }
> > > +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > > +    return 0;
> > > +}
> > > +
> > > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > > +{
> > > +    CPUSlotInfoList *head = NULL;
> > > +    CPUSlotInfoList **prev = &head;
> > > +    Object *root_container;
> > > +    ObjectProperty *prop;
> > > +    ObjectPropertyIterator iter;
> > > +
> > > +    /*
> > > +     * TODO: There surely must be a better/easier way to walk all
> > > +     * the link properties of an object ?
> > > +     */
> > > +    root_container = container_get(object_get_root(), "/machine");
> > > +    object_property_iter_init(&iter, root_container);
> > > +
> > > +    while ((prop = object_property_iter_next(&iter))) {
> > > +        Object *obj;
> > > +        DeviceState *dev;
> > > +        CPUSlotInfoList *elem;
> > > +        CPUSlotInfo *s;
> > > +        CPUInfoList *cpu_head = NULL;
> > > +        CPUInfoList **cpu_prev = &cpu_head;
> > > +        sPAPRCPUCore *core;
> > > +
> > > +        if (!strstart(prop->type, "link<", NULL)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        elem = g_new0(CPUSlotInfoList, 1);
> > > +        s = g_new0(CPUSlotInfo, 1);
> > > +
> > > +        obj = object_property_get_link(root_container, prop->name, NULL);
> > > +        if (obj) {
> > > +            /* Slot populated */
> > > +            dev = DEVICE(obj);
> > > +            core = SPAPR_CPU_CORE(obj);
> > > +
> > > +            if (dev->id) {
> > > +                s->has_id = true;
> > > +                s->id = g_strdup(dev->id);
> > > +            }
> > > +            s->realized = object_property_get_bool(obj, "realized", NULL);
> > > +            s->nr_cpus = core->nr_threads;
> > > +            s->has_nr_cpus = true;
> > > +            s->qom_path = object_get_canonical_path(obj);
> > > +            s->has_qom_path = true;
> > > +            if (s->realized) {
> > > +                spapr_cpuinfo_list(obj, &cpu_prev);
> > > +            }
> > > +            s->has_cpus = true;
> > > +        } else {
> > > +            /* Slot empty */
> > > +            s->has_id = false;
> > > +            s->has_nr_cpus = false;
> > > +            s->has_qom_path = false;
> > > +            s->has_cpus = false;
> > > +            s->realized = false;
> > > +        }
> > > +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > > +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > > +        s->slot_id = g_strdup(prop->name);
> > > +        s->cpus = cpu_head;
> > > +        elem->value = s;
> > > +        elem->next = NULL;
> > > +        *prev = elem;
> > > +        prev = &elem->next;
> > > +    }
> > > +    return head;
> > > +}
> > > +
> > >  static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      MachineClass *mc = MACHINE_CLASS(oc);
> > > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >      hc->plug = spapr_machine_device_plug;
> > >      hc->unplug = spapr_machine_device_unplug;
> > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > +    mc->cpu_slots = spapr_cpu_slots;
> > >  
> > >      smc->dr_lmb_enabled = true;
> > >      smc->dr_cpu_enabled = true;
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 0f30959..d888a02 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> > >   *    Set only by old machines because they need to keep
> > >   *    compatibility on code that exposed QEMU_VERSION to guests in
> > >   *    the past (and now use qemu_hw_version()).
> > > + * @cpu_slots:
> > > + *    Provides information about populated and yet-to-be populated
> > > + *    CPU slots in the machine. Used by QMP query-cpu-slots.
> > >   */
> > >  struct MachineClass {
> > >      /*< private >*/
> > > @@ -99,6 +102,7 @@ struct MachineClass {
> > >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > >                                             DeviceState *dev);
> > >      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > > +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> > >  };
> > >  
> > >  /**
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 8d04897..e9a52a2 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4083,3 +4083,88 @@
> > >  ##
> > >  { 'enum': 'ReplayMode',
> > >    'data': [ 'none', 'record', 'play' ] }
> > > +
> > > +##
> > > +# @CPUInfo:
> > > +#
> > > +# Information about CPUs
> > > +#
> > > +# @arch-id: Arch-specific ID for the CPU.
> > > +#
> > > +# @type: QOM type of the CPU.
> > > +#
> > > +# @thread: Thread ID of the CPU.
> > > +#
> > > +# @core: Core ID of the CPU.
> > > +#
> > > +# @socket: Socket ID of the CPU.
> > > +#
> > > +# @node: NUMA node to which the CPU belongs.
> > > +#
> > > +# @qom-path: QOM path of the CPU object
> > > +#
> > > +# Since: 2.6
> > > +##
> > > +
> > > +{ 'struct': 'CPUInfo',
> > > +  'data': { 'arch-id': 'int',
> > > +            'type': 'str',
> > > +            '*thread': 'int',
> > > +            '*core': 'int',
> > > +            '*socket' : 'int',
> > > +            '*node' : 'int',
> > > +            '*qom-path': 'str'
> > > +          }
> > > +}
> > > +
> > > +##
> > > +# @CPUSlotInfo:
> > > +#
> > > +# Information about CPU Slots
> > > +#
> > > +# @id: Device ID of the CPU composite object that occupies the slot.
> > > +#
> > > +# @type: QOM type of the CPU composite object that occupies the slot.
> > > +#
> > > +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> > > +# can be thread, core or socket.
> > > +#
> > > +# @slot-id: Slot's identifier.
> > > +#
> > > +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> > > +#
> > > +# @realized: A boolean that indicates whether the slot is filled or empty.
> > > +#
> > > +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> > > +# this slot.
> > > +#
> > > +# @cpus: An array of @CPUInfo elements where each element describes one
> > > +# CPU that is part of this slot's CPU composite object.
> > > +#
> > > +# @type: QOM type
> > > +#
> > > +# Since: 2.6
> > > +##
> > > +
> > > +{ 'struct': 'CPUSlotInfo',
> > > +  'data': { '*id': 'str',
> > > +            'type': 'str',
> > > +            'hotplug-granularity' : 'str',  
> > Does it convey any useful info, if yes how it will be used by mgmt?  
> 
> As I noted in the patch desc, this field is useless, will remove.
> 
> >   
> > > +            'slot-id' : 'str',
> > > +            '*qom-path': 'str',
> > > +            'realized': 'bool',  
> > field's redundant, presence of qom-path answers this question  
> 
> Ok, makes sense.
> 
> >   
> > > +            '*nr-cpus': 'int',
> > > +            '*cpus' : ['CPUInfo']  
> > I'd suggest to drop above 2 fields as it's impl dependent,
> > qom-path already point's to socket/core/thread object and
> > its internal composition can be explored by other means if needed.
> > 
> > Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> > 'CpuInfo'.  
> 
> Ah I see, should change the naming if we eventually stick with this
> implementation.
> 
> > I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> > existing thread enumeration's already implemented query-cpus.  
> 
> Ok.
> 
> >   
> > > +          }
> > > +}  
> > What I miss here is that CPUSlotInfo doesn't provide any
> > information to about where CPU might be hotplugged to.
> > 
> > Maybe use following tuple instead of slot-id?
> > 
> > { 'struct': 'CPUSlotProperties',
> >   'data': { '*node': 'int',
> >             '*socket': 'int',
> >             '*core': 'int',
> >             '*thread': 'int'
> >   }
> > }  
> 
> Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
> place where the CPU sits. Machine determines the type of the slot and it
> can be socket slot, core slot or thread slot based on the granularity
> of the hotplug supported by the machine. With this I don't see why
> anything else apart from slot-id/slot-name is required to figure out where
> to hoplug CPU.
Of cause it's possible to create and keep at board level map of
slot-names to whatever other info needed to create a CPU object
at machine init time, and then make board somehow to apply it
to the new CPU object before realizing it.

But it doesn't give mgmt any information whatsoever about where CPU
is being hotplugged. So for x86/arm we would end up with adding
yet another interface that would tell it. If CPUSlotProperties is used
instead of slot-name, it could convey that information while
keeping interface compatible with a range targets we care about
and extendable to other targets in future.

> In the current implementation, sPAPR hotplug is at core granularity.
> CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
> threads in the core and the CPU model of the threads are either machine
> default or specified by user during hotplug time.
now imagine extending in future sPAPR to support NUMA, which way would
you extend interface, how would mgmt know which CPU belongs to what node?

As you may note in CPUSlotProperties fields are optional so target would
use only ones it needs. For current sPAPR, query result could be
something like this:

{
  {
     type: spapr-cpu-core
     properties: { core: 0 }
     qom-path: /machine/unattached/device[xxx]
  }
  {
     type: spapr-cpu-core
     properties: { core: 1 }
  }
  ...
  {
     type: spapr-cpu-core
     properties: { core: 0 }
  }
}

Later if numa was needed, the board could set 'numa' property as well.
Would that work for sPAPR?

> I believe NUMA node should be the property of the slot. The information
> about which slots are part of which NUMA node should be known beforehand
> at machine init time. Hotplugging CPU thread, core or socket to a slot will
> make that thread, core or socket part of that NUMA node.
As analogy PCI devices also have 'bus' property which is used to tell
device_add machinery where to attach the device. The same goes for DIMM
device which also has 'numa' option so mgmt could say to which node device
is being plugged.

CPUSlotProperties is basically location where CPU is being plugged
and in some cases location includes 'numa' property or some other.
The difference vs slot-name is that CPUSlotProperties is more
verbose and flexible and tries to describe location in natural
for target therms.

> > it's generic for the most targets, which should work for spapr, s390, x86, ARM
> > and could be extended for other cases adding other board specific
> > properties if it's needed.  
> 
> I don't about ARM, but s390 doesn't care about topology iiuc.
> 
> For sPAPR, as I described above, we don't care about sockets and hence
> I believe we can live with hotplugging a core into a machine slot by
> referring to the slot-id.
> 
> I see that CPUSlotProperties that you are suggesting here  would make sense
> for x86 depending on the granularity at which hotplug is done. I know there
> has been discussion on thread vs socket hotplug granularity for x86, has
> there been a consensus on what it is going to be ?
I don't agree with switching to socket model on x86 for a number of reasons:
 - it can't be done without breaking compatibility.
 - from user pov, he/she would like to hotplug CPU at thread level to get
   necessary CPU horsepower without over-paying for over-provisioned threads.
   Limiting it to socket level would be crippling qemu based hypervisors
   in comparison to other hypervisors, which do it currently at thread level.
   Practice show that Linux and Windows handle thread level hotplug well
   enough perhaps because they try match ACPI spec where hotplug happens
   at thread level.

On Tue, Mar 01, 2016 at 02:55:02PM +0100, Igor Mammedov wrote:
> On Tue, 1 Mar 2016 14:39:51 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:
> > > On Thu, 25 Feb 2016 21:52:41 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > Implement query cpu-slots that provides information about hot-plugged
> > > > as well as hot-pluggable CPU slots that the machine supports.
> > > > 
> > > > TODO: As Eric suggested use enum for type instead of str.
> > > > TODO: @hotplug-granularity probably isn't required.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/core/machine.c   |  19 +++++++++
> > > >  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/boards.h |   4 ++
> > > >  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
> > > >  qmp-commands.hx     |  47 ++++++++++++++++++++++
> > > >  5 files changed, 267 insertions(+)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 6d1a0d8..3055ef8 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -17,6 +17,25 @@
> > > >  #include "hw/sysbus.h"
> > > >  #include "sysemu/sysemu.h"
> > > >  #include "qemu/error-report.h"
> > > > +#include "qmp-commands.h"
> > > > +
> > > > +/*
> > > > + * QMP: query-cpu-slots
> > > > + *
> > > > + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> > > > + */
> > > > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > > > +{
> > > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > > > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > > +
> > > > +    if (!mc->cpu_slots) {
> > > > +        error_setg(errp, QERR_UNSUPPORTED);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    return mc->cpu_slots(ms);
> > > > +}
> > > >  
> > > >  static char *machine_get_accel(Object *obj, Error **errp)
> > > >  {
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 780cd00..b76ed85 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> > > >      return cpu_index / smp_threads / smp_cores;
> > > >  }
> > > >  
> > > > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > > > +{
> > > > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > > +    CPUInfoList ***prev = opaque;
> > > > +
> > > > +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> > > > +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > > > +        CPUInfo *s = g_new0(CPUInfo, 1);
> > > > +        CPUState *cpu = CPU(obj);
> > > > +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > > > +
> > > > +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > > > +        s->type = g_strdup(object_get_typename(obj));
> > > > +        s->thread = cpu->cpu_index;
> > > > +        s->has_thread = true;
> > > > +        s->core = cpu->cpu_index / smp_threads;
> > > > +        s->has_core = true;
> > > > +        if (mc->cpu_index_to_socket_id) {
> > > > +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > > > +        } else {
> > > > +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > > > +        }
> > > > +        s->has_socket = true;
> > > > +        s->node = cpu->numa_node;
> > > > +        s->has_node = true;
> > > > +        s->qom_path = object_get_canonical_path(obj);
> > > > +        s->has_qom_path = true;
> > > > +
> > > > +        elem->value = s;
> > > > +        elem->next = NULL;
> > > > +        **prev = elem;
> > > > +        *prev = &elem->next;
> > > > +    }
> > > > +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > > > +{
> > > > +    CPUSlotInfoList *head = NULL;
> > > > +    CPUSlotInfoList **prev = &head;
> > > > +    Object *root_container;
> > > > +    ObjectProperty *prop;
> > > > +    ObjectPropertyIterator iter;
> > > > +
> > > > +    /*
> > > > +     * TODO: There surely must be a better/easier way to walk all
> > > > +     * the link properties of an object ?
> > > > +     */
> > > > +    root_container = container_get(object_get_root(), "/machine");
> > > > +    object_property_iter_init(&iter, root_container);
> > > > +
> > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > +        Object *obj;
> > > > +        DeviceState *dev;
> > > > +        CPUSlotInfoList *elem;
> > > > +        CPUSlotInfo *s;
> > > > +        CPUInfoList *cpu_head = NULL;
> > > > +        CPUInfoList **cpu_prev = &cpu_head;
> > > > +        sPAPRCPUCore *core;
> > > > +
> > > > +        if (!strstart(prop->type, "link<", NULL)) {
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        elem = g_new0(CPUSlotInfoList, 1);
> > > > +        s = g_new0(CPUSlotInfo, 1);
> > > > +
> > > > +        obj = object_property_get_link(root_container, prop->name, NULL);
> > > > +        if (obj) {
> > > > +            /* Slot populated */
> > > > +            dev = DEVICE(obj);
> > > > +            core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +            if (dev->id) {
> > > > +                s->has_id = true;
> > > > +                s->id = g_strdup(dev->id);
> > > > +            }
> > > > +            s->realized = object_property_get_bool(obj, "realized", NULL);
> > > > +            s->nr_cpus = core->nr_threads;
> > > > +            s->has_nr_cpus = true;
> > > > +            s->qom_path = object_get_canonical_path(obj);
> > > > +            s->has_qom_path = true;
> > > > +            if (s->realized) {
> > > > +                spapr_cpuinfo_list(obj, &cpu_prev);
> > > > +            }
> > > > +            s->has_cpus = true;
> > > > +        } else {
> > > > +            /* Slot empty */
> > > > +            s->has_id = false;
> > > > +            s->has_nr_cpus = false;
> > > > +            s->has_qom_path = false;
> > > > +            s->has_cpus = false;
> > > > +            s->realized = false;
> > > > +        }
> > > > +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > > > +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > > > +        s->slot_id = g_strdup(prop->name);
> > > > +        s->cpus = cpu_head;
> > > > +        elem->value = s;
> > > > +        elem->next = NULL;
> > > > +        *prev = elem;
> > > > +        prev = &elem->next;
> > > > +    }
> > > > +    return head;
> > > > +}
> > > > +
> > > >  static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > >  {
> > > >      MachineClass *mc = MACHINE_CLASS(oc);
> > > > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > >      hc->plug = spapr_machine_device_plug;
> > > >      hc->unplug = spapr_machine_device_unplug;
> > > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > > +    mc->cpu_slots = spapr_cpu_slots;
> > > >  
> > > >      smc->dr_lmb_enabled = true;
> > > >      smc->dr_cpu_enabled = true;
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 0f30959..d888a02 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> > > >   *    Set only by old machines because they need to keep
> > > >   *    compatibility on code that exposed QEMU_VERSION to guests in
> > > >   *    the past (and now use qemu_hw_version()).
> > > > + * @cpu_slots:
> > > > + *    Provides information about populated and yet-to-be populated
> > > > + *    CPU slots in the machine. Used by QMP query-cpu-slots.
> > > >   */
> > > >  struct MachineClass {
> > > >      /*< private >*/
> > > > @@ -99,6 +102,7 @@ struct MachineClass {
> > > >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > >                                             DeviceState *dev);
> > > >      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > > > +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> > > >  };
> > > >  
> > > >  /**
> > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > index 8d04897..e9a52a2 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -4083,3 +4083,88 @@
> > > >  ##
> > > >  { 'enum': 'ReplayMode',
> > > >    'data': [ 'none', 'record', 'play' ] }
> > > > +
> > > > +##
> > > > +# @CPUInfo:
> > > > +#
> > > > +# Information about CPUs
> > > > +#
> > > > +# @arch-id: Arch-specific ID for the CPU.
> > > > +#
> > > > +# @type: QOM type of the CPU.
> > > > +#
> > > > +# @thread: Thread ID of the CPU.
> > > > +#
> > > > +# @core: Core ID of the CPU.
> > > > +#
> > > > +# @socket: Socket ID of the CPU.
> > > > +#
> > > > +# @node: NUMA node to which the CPU belongs.
> > > > +#
> > > > +# @qom-path: QOM path of the CPU object
> > > > +#
> > > > +# Since: 2.6
> > > > +##
> > > > +
> > > > +{ 'struct': 'CPUInfo',
> > > > +  'data': { 'arch-id': 'int',
> > > > +            'type': 'str',
> > > > +            '*thread': 'int',
> > > > +            '*core': 'int',
> > > > +            '*socket' : 'int',
> > > > +            '*node' : 'int',
> > > > +            '*qom-path': 'str'
> > > > +          }
> > > > +}
> > > > +
> > > > +##
> > > > +# @CPUSlotInfo:
> > > > +#
> > > > +# Information about CPU Slots
> > > > +#
> > > > +# @id: Device ID of the CPU composite object that occupies the slot.
> > > > +#
> > > > +# @type: QOM type of the CPU composite object that occupies the slot.
> > > > +#
> > > > +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> > > > +# can be thread, core or socket.
> > > > +#
> > > > +# @slot-id: Slot's identifier.
> > > > +#
> > > > +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> > > > +#
> > > > +# @realized: A boolean that indicates whether the slot is filled or empty.
> > > > +#
> > > > +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> > > > +# this slot.
> > > > +#
> > > > +# @cpus: An array of @CPUInfo elements where each element describes one
> > > > +# CPU that is part of this slot's CPU composite object.
> > > > +#
> > > > +# @type: QOM type
> > > > +#
> > > > +# Since: 2.6
> > > > +##
> > > > +
> > > > +{ 'struct': 'CPUSlotInfo',
> > > > +  'data': { '*id': 'str',
> > > > +            'type': 'str',
> > > > +            'hotplug-granularity' : 'str',  
> > > Does it convey any useful info, if yes how it will be used by mgmt?  
> > 
> > As I noted in the patch desc, this field is useless, will remove.
> > 
> > >   
> > > > +            'slot-id' : 'str',
> > > > +            '*qom-path': 'str',
> > > > +            'realized': 'bool',  
> > > field's redundant, presence of qom-path answers this question  
> > 
> > Ok, makes sense.
> > 
> > >   
> > > > +            '*nr-cpus': 'int',
> > > > +            '*cpus' : ['CPUInfo']  
> > > I'd suggest to drop above 2 fields as it's impl dependent,
> > > qom-path already point's to socket/core/thread object and
> > > its internal composition can be explored by other means if needed.
> > > 
> > > Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> > > 'CpuInfo'.  
> > 
> > Ah I see, should change the naming if we eventually stick with this
> > implementation.
> > 
> > > I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> > > existing thread enumeration's already implemented query-cpus.  
> > 
> > Ok.
> > 
> > >   
> > > > +          }
> > > > +}  
> > > What I miss here is that CPUSlotInfo doesn't provide any
> > > information to about where CPU might be hotplugged to.
> > > 
> > > Maybe use following tuple instead of slot-id?
> > > 
> > > { 'struct': 'CPUSlotProperties',
> > >   'data': { '*node': 'int',
> > >             '*socket': 'int',
> > >             '*core': 'int',
> > >             '*thread': 'int'
> > >   }
> > > }  
> > 
> > Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
> > place where the CPU sits. Machine determines the type of the slot and it
> > can be socket slot, core slot or thread slot based on the granularity
> > of the hotplug supported by the machine. With this I don't see why
> > anything else apart from slot-id/slot-name is required to figure out where
> > to hoplug CPU.
> Of cause it's possible to create and keep at board level map of
> slot-names to whatever other info needed to create a CPU object
> at machine init time, and then make board somehow to apply it
> to the new CPU object before realizing it.
> 
> But it doesn't give mgmt any information whatsoever about where CPU
> is being hotplugged. So for x86/arm we would end up with adding
> yet another interface that would tell it. If CPUSlotProperties is used
> instead of slot-name, it could convey that information while
> keeping interface compatible with a range targets we care about
> and extendable to other targets in future.
> 
> > In the current implementation, sPAPR hotplug is at core granularity.
> > CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
> > threads in the core and the CPU model of the threads are either machine
> > default or specified by user during hotplug time.
> now imagine extending in future sPAPR to support NUMA, which way would
> you extend interface, how would mgmt know which CPU belongs to what node?
> 
> As you may note in CPUSlotProperties fields are optional so target would
> use only ones it needs. For current sPAPR, query result could be
> something like this:
> 
> {
>   {
>      type: spapr-cpu-core
>      properties: { core: 0 }
>      qom-path: /machine/unattached/device[xxx]
>   }
>   {
>      type: spapr-cpu-core
>      properties: { core: 1 }
>   }
>   ...
>   {
>      type: spapr-cpu-core
>      properties: { core: 0 }
>   }
> }
> 
> Later if numa was needed, the board could set 'numa' property as well.
> Would that work for sPAPR?

It could work, however I am finding it difficult to reconcile this with
the machine object to core links that I have in this implementation which
I believe is what Andreas originally suggested.  Such a link essentially
represents a slot and it has a name associated with it which identifies the
location where the core is plugged.

Now in order to incorporate CPUSlotProperties, these properties must
be tied to a slot/location object which in the present implementation
doesn't exist. So are you suggesting that we create slot/location
object (abstract ?) that has CPUSlotProperties ? If so, then I guess we
don't need machine object to core object links ?

Regards,
Bharata.

On Thu, 3 Mar 2016 15:00:32 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Mar 01, 2016 at 02:55:02PM +0100, Igor Mammedov wrote:
> > On Tue, 1 Mar 2016 14:39:51 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:  
> > > > On Thu, 25 Feb 2016 21:52:41 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >     
> > > > > Implement query cpu-slots that provides information about hot-plugged
> > > > > as well as hot-pluggable CPU slots that the machine supports.
> > > > > 
> > > > > TODO: As Eric suggested use enum for type instead of str.
> > > > > TODO: @hotplug-granularity probably isn't required.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/core/machine.c   |  19 +++++++++
> > > > >  hw/ppc/spapr.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/boards.h |   4 ++
> > > > >  qapi-schema.json    |  85 +++++++++++++++++++++++++++++++++++++++
> > > > >  qmp-commands.hx     |  47 ++++++++++++++++++++++
> > > > >  5 files changed, 267 insertions(+)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 6d1a0d8..3055ef8 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -17,6 +17,25 @@
> > > > >  #include "hw/sysbus.h"
> > > > >  #include "sysemu/sysemu.h"
> > > > >  #include "qemu/error-report.h"
> > > > > +#include "qmp-commands.h"
> > > > > +
> > > > > +/*
> > > > > + * QMP: query-cpu-slots
> > > > > + *
> > > > > + * TODO: Ascertain if this is the right place to for this arch-neutral routine.
> > > > > + */
> > > > > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > > > > +{
> > > > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > > > > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > > > +
> > > > > +    if (!mc->cpu_slots) {
> > > > > +        error_setg(errp, QERR_UNSUPPORTED);
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    return mc->cpu_slots(ms);
> > > > > +}
> > > > >  
> > > > >  static char *machine_get_accel(Object *obj, Error **errp)
> > > > >  {
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 780cd00..b76ed85 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -2453,6 +2453,117 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
> > > > >      return cpu_index / smp_threads / smp_cores;
> > > > >  }
> > > > >  
> > > > > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > > > > +{
> > > > > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > > > +    CPUInfoList ***prev = opaque;
> > > > > +
> > > > > +    if (object_dynamic_cast(obj, TYPE_CPU)) {
> > > > > +        CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > > > > +        CPUInfo *s = g_new0(CPUInfo, 1);
> > > > > +        CPUState *cpu = CPU(obj);
> > > > > +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > > > > +
> > > > > +        s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > > > > +        s->type = g_strdup(object_get_typename(obj));
> > > > > +        s->thread = cpu->cpu_index;
> > > > > +        s->has_thread = true;
> > > > > +        s->core = cpu->cpu_index / smp_threads;
> > > > > +        s->has_core = true;
> > > > > +        if (mc->cpu_index_to_socket_id) {
> > > > > +            s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > > > > +        } else {
> > > > > +            s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > > > > +        }
> > > > > +        s->has_socket = true;
> > > > > +        s->node = cpu->numa_node;
> > > > > +        s->has_node = true;
> > > > > +        s->qom_path = object_get_canonical_path(obj);
> > > > > +        s->has_qom_path = true;
> > > > > +
> > > > > +        elem->value = s;
> > > > > +        elem->next = NULL;
> > > > > +        **prev = elem;
> > > > > +        *prev = &elem->next;
> > > > > +    }
> > > > > +    object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > > > > +{
> > > > > +    CPUSlotInfoList *head = NULL;
> > > > > +    CPUSlotInfoList **prev = &head;
> > > > > +    Object *root_container;
> > > > > +    ObjectProperty *prop;
> > > > > +    ObjectPropertyIterator iter;
> > > > > +
> > > > > +    /*
> > > > > +     * TODO: There surely must be a better/easier way to walk all
> > > > > +     * the link properties of an object ?
> > > > > +     */
> > > > > +    root_container = container_get(object_get_root(), "/machine");
> > > > > +    object_property_iter_init(&iter, root_container);
> > > > > +
> > > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > > +        Object *obj;
> > > > > +        DeviceState *dev;
> > > > > +        CPUSlotInfoList *elem;
> > > > > +        CPUSlotInfo *s;
> > > > > +        CPUInfoList *cpu_head = NULL;
> > > > > +        CPUInfoList **cpu_prev = &cpu_head;
> > > > > +        sPAPRCPUCore *core;
> > > > > +
> > > > > +        if (!strstart(prop->type, "link<", NULL)) {
> > > > > +            continue;
> > > > > +        }
> > > > > +
> > > > > +        if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > > > > +            continue;
> > > > > +        }
> > > > > +
> > > > > +        elem = g_new0(CPUSlotInfoList, 1);
> > > > > +        s = g_new0(CPUSlotInfo, 1);
> > > > > +
> > > > > +        obj = object_property_get_link(root_container, prop->name, NULL);
> > > > > +        if (obj) {
> > > > > +            /* Slot populated */
> > > > > +            dev = DEVICE(obj);
> > > > > +            core = SPAPR_CPU_CORE(obj);
> > > > > +
> > > > > +            if (dev->id) {
> > > > > +                s->has_id = true;
> > > > > +                s->id = g_strdup(dev->id);
> > > > > +            }
> > > > > +            s->realized = object_property_get_bool(obj, "realized", NULL);
> > > > > +            s->nr_cpus = core->nr_threads;
> > > > > +            s->has_nr_cpus = true;
> > > > > +            s->qom_path = object_get_canonical_path(obj);
> > > > > +            s->has_qom_path = true;
> > > > > +            if (s->realized) {
> > > > > +                spapr_cpuinfo_list(obj, &cpu_prev);
> > > > > +            }
> > > > > +            s->has_cpus = true;
> > > > > +        } else {
> > > > > +            /* Slot empty */
> > > > > +            s->has_id = false;
> > > > > +            s->has_nr_cpus = false;
> > > > > +            s->has_qom_path = false;
> > > > > +            s->has_cpus = false;
> > > > > +            s->realized = false;
> > > > > +        }
> > > > > +        s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > > > > +        s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > > > > +        s->slot_id = g_strdup(prop->name);
> > > > > +        s->cpus = cpu_head;
> > > > > +        elem->value = s;
> > > > > +        elem->next = NULL;
> > > > > +        *prev = elem;
> > > > > +        prev = &elem->next;
> > > > > +    }
> > > > > +    return head;
> > > > > +}
> > > > > +
> > > > >  static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > > >  {
> > > > >      MachineClass *mc = MACHINE_CLASS(oc);
> > > > > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > > >      hc->plug = spapr_machine_device_plug;
> > > > >      hc->unplug = spapr_machine_device_unplug;
> > > > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > > > +    mc->cpu_slots = spapr_cpu_slots;
> > > > >  
> > > > >      smc->dr_lmb_enabled = true;
> > > > >      smc->dr_cpu_enabled = true;
> > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > > index 0f30959..d888a02 100644
> > > > > --- a/include/hw/boards.h
> > > > > +++ b/include/hw/boards.h
> > > > > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> > > > >   *    Set only by old machines because they need to keep
> > > > >   *    compatibility on code that exposed QEMU_VERSION to guests in
> > > > >   *    the past (and now use qemu_hw_version()).
> > > > > + * @cpu_slots:
> > > > > + *    Provides information about populated and yet-to-be populated
> > > > > + *    CPU slots in the machine. Used by QMP query-cpu-slots.
> > > > >   */
> > > > >  struct MachineClass {
> > > > >      /*< private >*/
> > > > > @@ -99,6 +102,7 @@ struct MachineClass {
> > > > >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > > >                                             DeviceState *dev);
> > > > >      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > > > > +    CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > > index 8d04897..e9a52a2 100644
> > > > > --- a/qapi-schema.json
> > > > > +++ b/qapi-schema.json
> > > > > @@ -4083,3 +4083,88 @@
> > > > >  ##
> > > > >  { 'enum': 'ReplayMode',
> > > > >    'data': [ 'none', 'record', 'play' ] }
> > > > > +
> > > > > +##
> > > > > +# @CPUInfo:
> > > > > +#
> > > > > +# Information about CPUs
> > > > > +#
> > > > > +# @arch-id: Arch-specific ID for the CPU.
> > > > > +#
> > > > > +# @type: QOM type of the CPU.
> > > > > +#
> > > > > +# @thread: Thread ID of the CPU.
> > > > > +#
> > > > > +# @core: Core ID of the CPU.
> > > > > +#
> > > > > +# @socket: Socket ID of the CPU.
> > > > > +#
> > > > > +# @node: NUMA node to which the CPU belongs.
> > > > > +#
> > > > > +# @qom-path: QOM path of the CPU object
> > > > > +#
> > > > > +# Since: 2.6
> > > > > +##
> > > > > +
> > > > > +{ 'struct': 'CPUInfo',
> > > > > +  'data': { 'arch-id': 'int',
> > > > > +            'type': 'str',
> > > > > +            '*thread': 'int',
> > > > > +            '*core': 'int',
> > > > > +            '*socket' : 'int',
> > > > > +            '*node' : 'int',
> > > > > +            '*qom-path': 'str'
> > > > > +          }
> > > > > +}
> > > > > +
> > > > > +##
> > > > > +# @CPUSlotInfo:
> > > > > +#
> > > > > +# Information about CPU Slots
> > > > > +#
> > > > > +# @id: Device ID of the CPU composite object that occupies the slot.
> > > > > +#
> > > > > +# @type: QOM type of the CPU composite object that occupies the slot.
> > > > > +#
> > > > > +# @hotplug-granularity: Granularity of CPU composite hotplug for this slot,
> > > > > +# can be thread, core or socket.
> > > > > +#
> > > > > +# @slot-id: Slot's identifier.
> > > > > +#
> > > > > +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> > > > > +#
> > > > > +# @realized: A boolean that indicates whether the slot is filled or empty.
> > > > > +#
> > > > > +# @nr-cpus: Number of CPUs that are part of CPU composite object that occupies
> > > > > +# this slot.
> > > > > +#
> > > > > +# @cpus: An array of @CPUInfo elements where each element describes one
> > > > > +# CPU that is part of this slot's CPU composite object.
> > > > > +#
> > > > > +# @type: QOM type
> > > > > +#
> > > > > +# Since: 2.6
> > > > > +##
> > > > > +
> > > > > +{ 'struct': 'CPUSlotInfo',
> > > > > +  'data': { '*id': 'str',
> > > > > +            'type': 'str',
> > > > > +            'hotplug-granularity' : 'str',    
> > > > Does it convey any useful info, if yes how it will be used by mgmt?    
> > > 
> > > As I noted in the patch desc, this field is useless, will remove.
> > >   
> > > >     
> > > > > +            'slot-id' : 'str',
> > > > > +            '*qom-path': 'str',
> > > > > +            'realized': 'bool',    
> > > > field's redundant, presence of qom-path answers this question    
> > > 
> > > Ok, makes sense.
> > >   
> > > >     
> > > > > +            '*nr-cpus': 'int',
> > > > > +            '*cpus' : ['CPUInfo']    
> > > > I'd suggest to drop above 2 fields as it's impl dependent,
> > > > qom-path already point's to socket/core/thread object and
> > > > its internal composition can be explored by other means if needed.
> > > > 
> > > > Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> > > > 'CpuInfo'.    
> > > 
> > > Ah I see, should change the naming if we eventually stick with this
> > > implementation.
> > >   
> > > > I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> > > > existing thread enumeration's already implemented query-cpus.    
> > > 
> > > Ok.
> > >   
> > > >     
> > > > > +          }
> > > > > +}    
> > > > What I miss here is that CPUSlotInfo doesn't provide any
> > > > information to about where CPU might be hotplugged to.
> > > > 
> > > > Maybe use following tuple instead of slot-id?
> > > > 
> > > > { 'struct': 'CPUSlotProperties',
> > > >   'data': { '*node': 'int',
> > > >             '*socket': 'int',
> > > >             '*core': 'int',
> > > >             '*thread': 'int'
> > > >   }
> > > > }    
> > > 
> > > Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
> > > place where the CPU sits. Machine determines the type of the slot and it
> > > can be socket slot, core slot or thread slot based on the granularity
> > > of the hotplug supported by the machine. With this I don't see why
> > > anything else apart from slot-id/slot-name is required to figure out where
> > > to hoplug CPU.  
> > Of cause it's possible to create and keep at board level map of
> > slot-names to whatever other info needed to create a CPU object
> > at machine init time, and then make board somehow to apply it
> > to the new CPU object before realizing it.
> > 
> > But it doesn't give mgmt any information whatsoever about where CPU
> > is being hotplugged. So for x86/arm we would end up with adding
> > yet another interface that would tell it. If CPUSlotProperties is used
> > instead of slot-name, it could convey that information while
> > keeping interface compatible with a range targets we care about
> > and extendable to other targets in future.
> >   
> > > In the current implementation, sPAPR hotplug is at core granularity.
> > > CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
> > > threads in the core and the CPU model of the threads are either machine
> > > default or specified by user during hotplug time.  
> > now imagine extending in future sPAPR to support NUMA, which way would
> > you extend interface, how would mgmt know which CPU belongs to what node?
> > 
> > As you may note in CPUSlotProperties fields are optional so target would
> > use only ones it needs. For current sPAPR, query result could be
> > something like this:
> > 
> > {
> >   {
> >      type: spapr-cpu-core
> >      properties: { core: 0 }
> >      qom-path: /machine/unattached/device[xxx]
> >   }
> >   {
> >      type: spapr-cpu-core
> >      properties: { core: 1 }
> >   }
> >   ...
> >   {
> >      type: spapr-cpu-core
> >      properties: { core: 0 }
> >   }
> > }
> > 
> > Later if numa was needed, the board could set 'numa' property as well.
> > Would that work for sPAPR?  
> 
> It could work, however I am finding it difficult to reconcile this with
> the machine object to core links that I have in this implementation which
> I believe is what Andreas originally suggested.  Such a link essentially
> represents a slot and it has a name associated with it which identifies the
> location where the core is plugged.
> 
> Now in order to incorporate CPUSlotProperties, these properties must
> be tied to a slot/location object which in the present implementation
> doesn't exist. So are you suggesting that we create slot/location
> object (abstract ?) that has CPUSlotProperties ? If so, then I guess we
> don't need machine object to core object links ?
I consider links as QOM interface to access Existing CPUs and
they links are more or less useless when it comes to possible CPUs.
If you really have to have links, you can associate them with
cores instead of slots, so it would look like:
 /machine/ 
          ->  cpu-core[0]
          ->  cpu-core[1]
          ...

I'm not suggesting anything related to QOM interface or QOM
in general when we discussing QMP interface in this patch.
On contrary this QMP query should be QOM agnostic so each target
could implement it regardless of what QOM model it uses.

So CPUSlotProperties is a pure QMP construct not tied to QOM
and it's up to machine callback to return CPUs layout description
like it has been proposed in https://patchwork.ozlabs.org/patch/583036/

I think in that thread, I've succeed in convincing Eduardo
and David to set aside QOM interface and implement only QMP command.
(Perhaps looking through that thread will convince you too if you
re-read it). I'll repost that patch taking in account comments
on that and this mail-thread using CPUSlotProperties.

That way we separate interface issue from QOM interface, making
QOM modeling a target implementation detail. Then each target
can implement it's own QOM device model (including links if it wishes)
to play with but still use common QMP interface which will not
be affected by it.
So mgmt would get stable well described/documented interface,
and we could eventually move with device_add based CPU hotplug
from stalemate point. QOM interface would be left for future
exercise or as experimental thing until it became at parity
with QMP one and had time settle down more or less.


> 
> Regards,
> Bharata.
>