Re: [Qemu-ppc] [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Bharata B Rao]

On Fri, Jan 29, 2016 at 04:45:06PM +0100, Igor Mammedov wrote:
> On Thu, 28 Jan 2016 11:19:54 +0530
> Bharata B Rao <address@hidden> wrote:
> 
> > Show the details of PPC CPU cores via a new QMP command.
> > 
> > TODO: update qmp-commands.hx with example
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  hw/ppc/cpu-core.c               | 77 
> > +++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json                | 31 +++++++++++++++++
> >  qmp-commands.hx                 | 51 +++++++++++++++++++++++++++
> >  stubs/Makefile.objs             |  1 +
> >  stubs/qmp_query_ppc_cpu_cores.c | 10 ++++++
> >  5 files changed, 170 insertions(+)
> >  create mode 100644 stubs/qmp_query_ppc_cpu_cores.c
> > 
> > diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> > index aa96e79..652a5aa 100644
> > --- a/hw/ppc/cpu-core.c
> > +++ b/hw/ppc/cpu-core.c
> > @@ -9,7 +9,84 @@
> >  #include "hw/ppc/cpu-core.h"
> >  #include "hw/boards.h"
> >  #include <sysemu/cpus.h>
> > +#include <sysemu/kvm.h>
> >  #include "qemu/error-report.h"
> > +#include "qmp-commands.h"
> > +
> > +/*
> > + * QMP: info ppc-cpu-cores
> > + */
> > +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> > +{
> > +    CpuInfoList ***prev = opaque;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU)) {
> > +        CpuInfoList *elem = g_new0(CpuInfoList, 1);
> > +        CpuInfo *s = g_new0(CpuInfo, 1);
> > +        CPUState *cs = CPU(obj);
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +        CPUPPCState *env = &cpu->env;
> > +
> > +        cpu_synchronize_state(cs);
> > +        s->arch = CPU_INFO_ARCH_PPC;
> > +        s->current = (cs == first_cpu);
> > +        s->CPU = cs->cpu_index;
> > +        s->qom_path = object_get_canonical_path(obj);
> > +        s->halted = cs->halted;
> > +        s->thread_id = cs->thread_id;
> > +        s->u.ppc = g_new0(CpuInfoPPC, 1);
> > +        s->u.ppc->nip = env->nip;
> > +
> > +        elem->value = s;
> > +        elem->next = NULL;
> > +        **prev = elem;
> > +        *prev = &elem->next;
> > +    }
> > +    object_child_foreach(obj, qmp_ppc_cpu_list, opaque);
> > +    return 0;
> > +}
> > +
> > +static int qmp_ppc_cpu_core_list(Object *obj, void *opaque)
> > +{
> > +    PPCCPUCoreList ***prev = opaque;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU_CORE)) {
> > +        DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +        DeviceState *dev = DEVICE(obj);
> > +
> > +        if (dev->realized) {
> > +            PPCCPUCoreList *elem = g_new0(PPCCPUCoreList, 1);
> > +            PPCCPUCore *s = g_new0(PPCCPUCore, 1);
> > +            CpuInfoList *cpu_head = NULL;
> > +            CpuInfoList **cpu_prev = &cpu_head;
> > +
> > +            if (dev->id) {
> > +                s->has_id = true;
> > +                s->id = g_strdup(dev->id);
> > +            }
> > +            s->hotplugged = dev->hotplugged;
> > +            s->hotpluggable = dc->hotpluggable;
> > +            qmp_ppc_cpu_list(obj, &cpu_prev);
> > +            s->threads = cpu_head;
> > +            elem->value = s;
> > +            elem->next = NULL;
> > +            **prev = elem;
> > +            *prev = &elem->next;
> > +        }
> > +    }
> > +
> > +    object_child_foreach(obj, qmp_ppc_cpu_core_list, opaque);
> > +    return 0;
> > +}
> > +
> > +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> > +{
> > +    PPCCPUCoreList *head = NULL;
> > +    PPCCPUCoreList **prev = &head;
> > +
> > +    qmp_ppc_cpu_core_list(qdev_get_machine(), &prev);
> > +    return head;
> > +}
> >  
> >  static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> >  {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 8d04897..0902697 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,34 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @PPCCPUCore:
> > +#
> > +# Information about PPC CPU core devices
> > +#
> > +# @hotplugged: true if device was hotplugged
> > +#
> > +# @hotpluggable: true if device if could be added/removed while machine is 
> > running
> > +#
> > +# Since: 2.6
> > +##
> > +
> > +{ 'struct': 'PPCCPUCore',
> > +  'data': { '*id': 'str',
> > +            'hotplugged': 'bool',
> > +            'hotpluggable': 'bool',
> > +            'threads' : ['CpuInfo']
> > +          }
> > +}
> Could it be made more arch independent?

Except that it is called PPCCPUCore, the fields are actually arch
neutral with 'threads' element defined as CpuInfo that gets interpreted
as arch-specific CpuInfo based on the target architecture.

In any case, this patchset adds PowerPC specific CPU core device that
sPAPR target implements. This is kept arch-specific in order to make it
more acceptable in short term in case arch-neutral, generic CPU hotplug
solutions take long time for reaching consensus.

> Perhaps it might make sense to replace 'threads'
> with qom-path so tools could inspect it in more detail
> if needed?

Hmm 'threads' is of CpuInfo type which already has qom-path inside it.
Did I get your question right ?

> 
> Also looking from cpu hotplug pov it would be nice
> to have at top level
>   - device type that tools could use with device_add
>   - display supported least granularity from topology pov
>     like node,socket[,core,[thread]] 'address' parameters
>   - display in CPU list also possible CPUs where only
>     'type' and 'address' parameters are present.
> 
> so above could look like:
> { 'struct': 'CPU',
>   'data': {
>             'type': 'str'
>             'node': 'int',
>             'socket': 'int',
>             '*core' : 'int',
>             '*thread' : 'int',
>             '*id': 'str',
>             '*hotplugged': 'bool',
>             '*hotpluggable': 'bool',
>             '*qom-path' : 'str'
>           }
> }

This is PowerPC specific where only core granularity hotplug makes
sense, but if it helps libvirt or other tools, I could add those fields.

Regards,
Bharata.