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

[David Gibson <david@gibson.dropbear.id.au>]

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

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 Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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