Re: [Qemu-devel] [RFC] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Mon, 15 Feb 2016 10:44:00 -0700
Eric Blake <address@hidden> wrote:

> On 02/15/2016 10:10 AM, Igor Mammedov wrote:
> > it will allow mgmt to query present and possible to hotplug CPUs
> > it is required from a target platform that wish to support
> > command to set board specific MachineClass.possible_cpus() hook,
> > which will return a list of possible CPUs with options
> > that would be needed for hotplugging possible CPUs.
> > 
> > For RFC there are:
> >    'arch_id': 'int' - mandatory unique CPU number,
> >                       for x86 it's APIC ID for ARM it's MPIDR
> >    'type': 'str' - CPU object type for usage with device_add
> > 
> > and a set of optional fields that would allows mgmt tools
> > to know at what granularity and where a new CPU could be
> > hotplugged;
> > [node],[socket],[core],[thread]
> > Hopefully that should cover needs for CPU hotplug porposes for
> > magor targets and we can extend structure in future adding
> > more fields if it will be needed.
> > 
> > also for present CPUs there is a 'cpu_link' field which
> > would allow mgmt inspect whatever object/abstraction
> > the target platform considers as CPU object.
> > 
> > For RFC purposes implements only for x86 target so far.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---  
> 
> Just an interface review for now:
> 
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,33 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @HotpluggableCPU
> > +#
> > +# @type: CPU object tyep for usage with device_add command  
> 
> s/tyep/type/
> 
> > +# @arch_id: unique number designating the CPU within board  
> 
> Please use '-' rather than '_' in new interfaces (this should be 'arch-id')
> 
> > +# @node: NUMA node ID the CPU belongs to, optional  
> 
> Most optional fields are marked with a prefix of '#optional', not an
> unmarked suffix. This will matter once we get to Marc-Andre's patches
> for automated documentation.
> 
> > +# @socket: socket number within node/board the CPU belongs to, optional
> > +# @core: core number within socket the CPU belongs to, optional
> > +# @thread: thread number within core the CPU belongs to, optional
> > +# @cpu_link: link to existing CPU object is CPU is present or  
> 
> Again, 'cpu-link'.
> 
> > +#            omitted if CPU is not present.
> > +#
> > +# Since: 2.6  
> 
> Missing '##' marker line.
> 
> > +{ 'struct': 'HotpluggableCPU',
> > +  'data': { 'type': 'str',
> > +            'arch_id': 'int',
> > +            '*node': 'int',
> > +            '*socket': 'int',
> > +            '*core': 'int',
> > +            '*thread': 'int',
> > +            '*cpu_link': 'str'
> > +          }
> > +}
> > +
> > +##
> > +# @query-hotpluggable-cpus
> > +#
> > +# Since: 2.6  
> 
> Missing '##' terminator, and also lacking on details.
> 
> > +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }  
> 
> Why do we need a new command?  Why can't the existing 'CpuInfo' be
> expanded to provide the new information as part of the existing
> 'query-cpus'?
CpuInfo is representing a existing CPU thread instance, so it won't work
possible CPUs, maybe query-cpus could be extended to show possible CPUs
as well but it still be at thread granularity so it might work for
x86 target but not for others.

In context of CPU hotplug different targets need a different level
of granularity at which CPU device could be hotplugged it might be
a thread, core, socket or something else so this command is
an attempt to provide interface that will allow a target specify
at what granularity it supports CPU hotplug and which/where 
CPUs could be hotplugged in relatively target independent way.

> 
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 020e5ee..cbe0ba4 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -4818,3 +4818,29 @@ Example:
> >                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
> >                    "pop-vlan": 1, "id": 251658240}
> >     ]}
> > +
> > +EQMP
> > +
> > +    {
> > +        .name       = "query-hotpluggable-cpus",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
> > +    },
> > +
> > +SQMP
> > +Show  existing/possible CPUs
> > +-------------------------------  
> 
> Why two spaces? --- separator line should be same length as the line above.
> 
> > +
> > +Arguments: None.
> > +
> > +Example for x86 target started with -smp 
> > 2,sockets=2,cores=1,threads=3,maxcpus=6:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     {"core": 0, "socket": 1, "thread": 2, "arch_id": 6, "type": 
> > "qemu64-x86_64-cpu"},
> > +     {"core": 0, "socket": 1, "thread": 1, "arch_id": 5, "type": 
> > "qemu64-x86_64-cpu"},
> > +     {"core": 0, "socket": 1, "thread": 0, "arch_id": 4, "type": 
> > "qemu64-x86_64-cpu"},
> > +     {"core": 0, "socket": 0, "thread": 2, "arch_id": 2, "type": 
> > "qemu64-x86_64-cpu"},
> > +     {"core": 0, "arch_id": 1, "socket": 0, "thread": 1, "type": 
> > "qemu64-x86_64-cpu", "cpu_link": "/machine/unattached/device[3]"},
> > +     {"core": 0, "arch_id": 0, "socket": 0, "thread": 0, "type": 
> > "qemu64-x86_64-cpu", "cpu_link": "/machine/unattached/device[0]"}  
> 
> Long line. Please wrap the example to fit in 80 columns (we've already
> added stylistic whitespace beyond the single-line JSON output that we
> really get from QMP).
> 
>