Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Igor Mammedov]

On Mon, 25 Mar 2013 14:22:36 -0600
Eric Blake <address@hidden> wrote:

> On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> > On Mon, 25 Mar 2013 16:35:11 +0100
> > Igor Mammedov <address@hidden> wrote:
> > 
> 
> >> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> >> +{
> >> +    if (online) {
> >> +        do_cpu_hot_add(id, errp);
> >> +    } else {
> >> +        error_setg(errp, "Unplug is not implemented");
> >> +    }
> >> +}
> > 
> > As a general rule, we don't allow command extensions to be done this
> > way because this is not queriable. A client would have to try online=off
> > to see if QEMU version X supports it, worse: the client would have to
> > parse the error message to be sure the failure actually corresponds
> > to unplug not implemented.
> > 
> > The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> > verbose, but doesn't have issues.
It looks like better as way to go. Later on we could keep them for
compatibility sake and map it to device_add/device_del commands when they are
ready.

> > 
> > Eric, what do you think?
> 
> Good point.  What is the likelihood of getting offline working before
> 1.5 is released?  If we are certain that offline cpu support won't make
> this release, then having separate commands would indeed be easier to query.
It's not likely to be ready for 1.5, since "offline" depends on code
introduced in this series and lacks VCPU hot-remove on KVM side. Some time
ago there were patches on list to introduce VCPU hot-remove in KVM, but it
didn't go well I think due to lack of VCPU hot-add and common infrastructure
it introduces.

> 
> On the other hand, why aren't we mirroring the QMP behavior to be
> similar to what Lazlo already did for qga:
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01031.html
Looks like terms online/offline are confusing, it would be better if
"cpu-set id=xx online=xxx" is replaced with cpu_add/cpu_del commands
following pattern of drive_add/del, pci_add/del ...

> That is, by having a way to query details on the set of all possible
> cpus (via a new command that returns an array with max cpus elements,
> rather than just the single int of query-cpu-max in
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg04441.html),
> with information in that query including a second boolean stating
> whether that cpu can be taken offline, would be sufficiently queryable.
>  The initial implementation would then state that an offline cpu can be
> taken online, but that an online cpu must remain in that state.
All of it wouldn't be necessary if we have only cpu_add without cpu_del
implemented.

As for querying present VCPUs, one could use qom to enumerate
/machine/unattached/icc-bus childs, looking for childs with type == *-cpu
It's possible to pre-allocate empty shortcut links like /machine/cpu[id] for
all VCPUs and populate corresponding links in x86_cpu_realizefn() when it
is going complete successfully. But query-ability of all possible CPUs could
be a bit out of scope of this series and requires changes to qdev, so I've
left it out for another time.

> And while typing that, I also realize that I like Lazlo's approach for
> another reason - guest-set-vcpus takes an array of actions, and performs
> as many as possible in one transaction; whereas your current cpu-set
> command has to be called multiple times to take multiple cpus online.
Yep, it has to be called for each CPU since it provides low level API at
QEMU level, allowing management to implement higher level ops like
transactions and all the logic associated with it (it is like device_add/del
commands towards which CPUs are moving slowly).

Considering all said above, would it be acceptable to replace cpu-set with
"cpu_add id=xx" command?