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

[Igor Mammedov]

On Mon, 1 Feb 2016 14:13:58 +0530
Bharata B Rao <address@hidden> wrote:

> 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.
Yep it's not easy to agree on but this PPC specific you'd have to
keep around and support pretty much forever once it's released,
I hope it would be easier to agree on CPU hotplug specific qmp/hmp
interface.

Wouldn't 'struct' I've suggested below work for you?
It's more or less generic and would work for x86 as well
so less target specific stuff to do in libvirt would also
be a plus.

> 
> > 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.
then PPC won't display/allow 'thread' parameter, while x86 will
since it allows thread granularity. For targets that allow only
sockets neither core/thread would be shown.

> 
> Regards,
> Bharata.
>