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

[Bharata B Rao]

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 <address@hidden> 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 <address@hidden>
> > ---
> >  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.