Re: [Qemu-devel] [PATCH 16/16] add cpu-add qmp command and implement CPU hot-add for target-i386

[Igor Mammedov]

On Mon, 15 Apr 2013 16:20:15 -0600
Eric Blake <address@hidden> wrote:

> On 04/15/2013 04:12 PM, Igor Mammedov wrote:
> > ... via current_machine->cpu_hot_add() hook called by cpu-set QMP command,
> > for x86 target.
> > 
> > cpu-add's "id" argument is a CPU thread number in a range [0..max-cpus - 1)
> 
> Off by one.  It's either [0..max-cpus) or [0..max-cpus - 1] (they mean
> the same thing).
> 
> It might be worth including a sample QMP command in the commit message.
Ok, will ammend it on next respin.

> 
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v5:
> >   * accept id=[0..max_cpus) range in cpu-add command
> 
> This notation is right, unlike the commit message.
> 
> Reviewing just the QMP portion:
> 
> > +++ b/qapi-schema.json
> > @@ -1387,6 +1387,17 @@
> >  { 'command': 'cpu', 'data': {'index': 'int'} }
> >  
> >  ##
> > +# @cpu-add
> > +#
> > +# Adds CPU with specified id
> > +#
> > +# @id: cpu id of CPU to be created
> 
> Here it would be helpful to mention what forms a valid id (your
> [0..max-cpus) notation from the commit message, for example).
ditto

> 
> > +#
> > +# Returns: Nothing on success
> > +##
> > +{ 'command': 'cpu-add', 'data': {'id': 'int'} }
> > +
> 
> Should be usable from libvirt's perspective, even if hot-plugging more
> than one cpu requires more than one QMP call.  Do we have a counterpart
> QMP call to easily determine which cpu ids can still be hotplugged?  If
> so, should we mention that in the documentation of this command?
We do not have it yet. Despite interface allowing to plug arbitrary CPU,
libvirt shouldn't do it in order not to break migration support. Since
migration target should be started with all CPU from source (including
hot-plugged ones). And current command line doesn't have means for this.

I'd propose do implement in libvirt something like:

hotplug_id = current_cpu_count
{ "execute": "cpu-add", "arguments": { "id": hotplug_id } }
current_cpu_count += 1

until arbitrary CPU hotplug and interface for enumerating possible CPUs
settle down.

There was a proposal for enumerating possible CPUs in previous version,
but it was target specific, and I was convinced to drop it for 1.5 and
aim for generic way to expose this information later.
http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02286.html
Opinion from libvirt POV would be nice to have though.

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


-- 
Regards,
  Igor