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

[Bharata B Rao]

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