| Message ID | 1364225711-32566-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 25 Mar 2013 16:35:11 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > * s/cpu_set/cpu-set/ > * qmp doc style fix > * use bool type instead of opencodding online/offline string > suggested-by: Eric Blake <eblake@redhat.com> > changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP > > --- > include/sysemu/sysemu.h | 2 ++ > qapi-schema.json | 12 ++++++++++++ > qmp-commands.hx | 24 ++++++++++++++++++++++++ > qmp.c | 9 +++++++++ > stubs/Makefile.objs | 1 + > stubs/do_cpu_hot_add.c | 7 +++++++ > 6 files changed, 55 insertions(+), 0 deletions(-) > create mode 100644 stubs/do_cpu_hot_add.c > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 4b8f721..8bcaf26 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict); > void qemu_register_cpu_add_notifier(Notifier *notifier); > void qemu_system_cpu_hotplug_request(uint32_t id); > > +void do_cpu_hot_add(const int64_t id, Error **errp); > + > /* pcie aer error injection */ > void pcie_aer_inject_error_print(Monitor *mon, const QObject *data); > int do_pcie_aer_inject_error(Monitor *mon, > diff --git a/qapi-schema.json b/qapi-schema.json > index 088f4e1..aa5f3dc 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1385,6 +1385,18 @@ > { 'command': 'cpu', 'data': {'index': 'int'} } > > ## > +# @cpu-set > +# > +# Sets specified cpu to online/offline mode > +# > +# @id: cpu id to be updated > +# > +# @online: true to put the cpu online, false to take it offline > +# > +## > +{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} } > + > +## > # @memsave: > # > # Save a portion of guest memory to a file. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index b370060..2f9c256 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -385,6 +385,30 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command. > EQMP > > { > + .name = "cpu-set", > + .args_type = "id:i,online:b", > + .mhandler.cmd_new = qmp_marshal_input_cpu_set, > + }, > + > +SQMP > +cpu-set > +------- > + > +Sets virtual cpu to online/ofline mode > + > +Arguments: > + > +- "id": cpu id (json-int) > +- "online": true to put the cpu online, false to take it offline (json-bool) > + > +Example: > + > +-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } } > +<- { "return": {} } > + > +EQMP > + > + { > .name = "memsave", > .args_type = "val:l,size:i,filename:s,cpu:i?", > .mhandler.cmd_new = qmp_marshal_input_memsave, > diff --git a/qmp.c b/qmp.c > index 55b056b..c211da5 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -108,6 +108,15 @@ void qmp_cpu(int64_t index, Error **errp) > /* Just do nothing */ > } > > +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. Eric, what do you think? > + > #ifndef CONFIG_VNC > /* If VNC support is enabled, the "true" query-vnc command is > defined in the VNC subsystem */ > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > index 6a492f5..4154a2b 100644 > --- a/stubs/Makefile.objs > +++ b/stubs/Makefile.objs > @@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o > stub-obj-y += resume_vcpu.o > stub-obj-y += get_icc_bus.o > stub-obj-y += qemu_system_cpu_hotplug_request.o > +stub-obj-y += do_cpu_hot_add.o > diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c > new file mode 100644 > index 0000000..1f6d7a6 > --- /dev/null > +++ b/stubs/do_cpu_hot_add.c > @@ -0,0 +1,7 @@ > +#include "qapi/error.h" > +#include "sysemu/sysemu.h" > + > +void do_cpu_hot_add(const int64_t id, Error **errp) > +{ > + error_setg(errp, "Not implemented"); > +}
On 03/25/2013 02:09 PM, Luiz Capitulino wrote: > On Mon, 25 Mar 2013 16:35:11 +0100 > Igor Mammedov <imammedo@redhat.com> 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. > > 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. 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 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. 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.
On Mon, 25 Mar 2013 14:22:36 -0600 Eric Blake <eblake@redhat.com> wrote: > On 03/25/2013 02:09 PM, Luiz Capitulino wrote: > > On Mon, 25 Mar 2013 16:35:11 +0100 > > Igor Mammedov <imammedo@redhat.com> 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?
On Tue, 26 Mar 2013 14:43:01 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 25 Mar 2013 14:22:36 -0600 > Eric Blake <eblake@redhat.com> wrote: > > > On 03/25/2013 02:09 PM, Luiz Capitulino wrote: > > > On Mon, 25 Mar 2013 16:35:11 +0100 > > > Igor Mammedov <imammedo@redhat.com> 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. I find the batch API to be a bit overkill (I'd add it in the future if really needed), but I won't oppose to it.
On 03/26/2013 07:43 AM, Igor Mammedov wrote: >>> 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. > > Considering all said above, would it be acceptable to replace cpu-set with > "cpu_add id=xx" command? cpu_add id=xx from HMP, cpu-add from QMP is fine for getting things started; it still leaves room for a future 'cpu-set' batching command if batching turns out to be useful.
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 4b8f721..8bcaf26 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict); void qemu_register_cpu_add_notifier(Notifier *notifier); void qemu_system_cpu_hotplug_request(uint32_t id); +void do_cpu_hot_add(const int64_t id, Error **errp); + /* pcie aer error injection */ void pcie_aer_inject_error_print(Monitor *mon, const QObject *data); int do_pcie_aer_inject_error(Monitor *mon, diff --git a/qapi-schema.json b/qapi-schema.json index 088f4e1..aa5f3dc 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1385,6 +1385,18 @@ { 'command': 'cpu', 'data': {'index': 'int'} } ## +# @cpu-set +# +# Sets specified cpu to online/offline mode +# +# @id: cpu id to be updated +# +# @online: true to put the cpu online, false to take it offline +# +## +{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} } + +## # @memsave: # # Save a portion of guest memory to a file. diff --git a/qmp-commands.hx b/qmp-commands.hx index b370060..2f9c256 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -385,6 +385,30 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command. EQMP { + .name = "cpu-set", + .args_type = "id:i,online:b", + .mhandler.cmd_new = qmp_marshal_input_cpu_set, + }, + +SQMP +cpu-set +------- + +Sets virtual cpu to online/ofline mode + +Arguments: + +- "id": cpu id (json-int) +- "online": true to put the cpu online, false to take it offline (json-bool) + +Example: + +-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } } +<- { "return": {} } + +EQMP + + { .name = "memsave", .args_type = "val:l,size:i,filename:s,cpu:i?", .mhandler.cmd_new = qmp_marshal_input_memsave, diff --git a/qmp.c b/qmp.c index 55b056b..c211da5 100644 --- a/qmp.c +++ b/qmp.c @@ -108,6 +108,15 @@ void qmp_cpu(int64_t index, Error **errp) /* Just do nothing */ } +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"); + } +} + #ifndef CONFIG_VNC /* If VNC support is enabled, the "true" query-vnc command is defined in the VNC subsystem */ diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 6a492f5..4154a2b 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o stub-obj-y += resume_vcpu.o stub-obj-y += get_icc_bus.o stub-obj-y += qemu_system_cpu_hotplug_request.o +stub-obj-y += do_cpu_hot_add.o diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c new file mode 100644 index 0000000..1f6d7a6 --- /dev/null +++ b/stubs/do_cpu_hot_add.c @@ -0,0 +1,7 @@ +#include "qapi/error.h" +#include "sysemu/sysemu.h" + +void do_cpu_hot_add(const int64_t id, Error **errp) +{ + error_setg(errp, "Not implemented"); +}
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: * s/cpu_set/cpu-set/ * qmp doc style fix * use bool type instead of opencodding online/offline string suggested-by: Eric Blake <eblake@redhat.com> changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP --- include/sysemu/sysemu.h | 2 ++ qapi-schema.json | 12 ++++++++++++ qmp-commands.hx | 24 ++++++++++++++++++++++++ qmp.c | 9 +++++++++ stubs/Makefile.objs | 1 + stubs/do_cpu_hot_add.c | 7 +++++++ 6 files changed, 55 insertions(+), 0 deletions(-) create mode 100644 stubs/do_cpu_hot_add.c