| Message ID | 1366063976-4909-17-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > 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). > +# > +# 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?
On Mon, 15 Apr 2013 16:20:15 -0600 Eric Blake <eblake@redhat.com> 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 <imammedo@redhat.com> > > --- > > 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 >
On 04/16/2013 02:04 PM, Igor Mammedov wrote: >>> +# >>> +# 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. Yes, that's pretty much how libvirt already handles hotplug for xen: libvirt currently maintains its own count of plugged cpus, and hot[un]plugs the next id so that the online cpus are always contiguous ids starting from 0. We are discussing on the libvirt lists ways of improving libvirt API to expose hotplug of arbitrary ids (after all, a 4-cpu bare metal machine can hotunplug cpu 1 while still leaving 2 and 3 online) - but that can come later, perhaps as qemu 1.6 gains better support for NUMA layout with better control of what ids should be used. > > 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. Agreed that deferring more powerful id enumeration for a later patch series is fine, and shouldn't hold up getting basic hotplug into 1.5.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a5c19a2..f09e44b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -53,6 +53,7 @@ #include "qemu/bitmap.h" #include "qemu/config-file.h" #include "hw/i386/icc_bus.h" +#include "hw/boards.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ @@ -911,6 +912,25 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp) return cpu; } +static void do_cpu_hot_add(const int64_t id, Error **errp) +{ + int64_t apic_id = x86_cpu_apic_id_from_index(id); + + if (cpu_exists(apic_id)) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", it already exists", id); + return; + } + + if (id >= max_cpus) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", max allowed: %d", id, max_cpus - 1); + return; + } + + pc_new_cpu(machine_args->cpu_model, apic_id, errp); +} + void pc_cpus_init(const char *cpu_model) { int i; @@ -925,7 +945,9 @@ void pc_cpus_init(const char *cpu_model) #else cpu_model = "qemu32"; #endif + machine_args->cpu_model = cpu_model; } + current_machine->hot_add_cpu = do_cpu_hot_add; ib = SYS_BUS_DEVICE(object_resolve_path_type("icc-bridge", TYPE_ICC_BRIDGE, NULL)); diff --git a/include/hw/boards.h b/include/hw/boards.h index 425bdc7..de8f92a 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -18,6 +18,8 @@ typedef struct QEMUMachineInitArgs { const char *cpu_model; } QEMUMachineInitArgs; +extern QEMUMachineInitArgs *machine_args; + typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args); typedef void QEMUMachineResetFunc(void); @@ -43,6 +45,7 @@ typedef struct QEMUMachine { GlobalProperty *compat_props; struct QEMUMachine *next; const char *hw_version; + void (*hot_add_cpu)(const int64_t id, Error **errp); } QEMUMachine; int qemu_register_machine(QEMUMachine *m); diff --git a/qapi-schema.json b/qapi-schema.json index 751d3c2..34d3e84 100644 --- a/qapi-schema.json +++ 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 +# +# Returns: Nothing on success +## +{ 'command': 'cpu-add', 'data': {'id': 'int'} } + +## # @memsave: # # Save a portion of guest memory to a file. diff --git a/qmp-commands.hx b/qmp-commands.hx index 4d65422..1e5d299 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -385,6 +385,29 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command. EQMP { + .name = "cpu-add", + .args_type = "id:i", + .mhandler.cmd_new = qmp_marshal_input_cpu_add, + }, + +SQMP +cpu-add +------- + +Adds virtual cpu + +Arguments: + +- "id": cpu id (json-int) + +Example: + +-> { "execute": "cpu-add", "arguments": { "id": 2 } } +<- { "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..8a9fd9e 100644 --- a/qmp.c +++ b/qmp.c @@ -24,6 +24,7 @@ #include "hw/qdev.h" #include "sysemu/blockdev.h" #include "qom/qom-qobject.h" +#include "hw/boards.h" NameInfo *qmp_query_name(Error **errp) { @@ -108,6 +109,15 @@ void qmp_cpu(int64_t index, Error **errp) /* Just do nothing */ } +void qmp_cpu_add(int64_t id, Error **errp) +{ + if (current_machine->hot_add_cpu) { + current_machine->hot_add_cpu(id, errp); + } else { + error_setg(errp, "Not supported"); + } +} + #ifndef CONFIG_VNC /* If VNC support is enabled, the "true" query-vnc command is defined in the VNC subsystem */ diff --git a/vl.c b/vl.c index bc9c016..8d87d97 100644 --- a/vl.c +++ b/vl.c @@ -179,6 +179,8 @@ int main(int argc, char **argv) #define MAX_VIRTIO_CONSOLES 1 #define MAX_SCLP_CONSOLES 1 +QEMUMachineInitArgs *machine_args; + static const char *data_dir[16]; static int data_dir_idx; const char *bios_name = NULL; @@ -4306,13 +4308,15 @@ int main(int argc, char **argv, char **envp) .kernel_cmdline = kernel_cmdline, .initrd_filename = initrd_filename, .cpu_model = cpu_model }; + machine_args = &args; + current_machine = machine; + machine->init(&args); cpu_synchronize_all_post_init(); set_numa_modes(); - current_machine = machine; /* init USB devices */ if (usb_enabled(false)) {
... 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) Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v5: * accept id=[0..max_cpus) range in cpu-add command v4: * merge "qmp: add cpu-add qmp command" & "target-i386: implement CPU hot-add" patches * move notifier call to CPUCLass.realize() * add hook cpu_hot_add to QEMUMachine * make QEMUMachineInitArgs global and keep default cpu_model there v3: * it appears that 'online/offline' in cpu-set are confusing people with what command actually does and users might have to distinguish if 'offline' is not implemented by parsing error message. To simplify things replace cpu-set with cpu-add command to show more clear what command does and just add cpu-del when CPU remove is implemented. 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> --- hw/i386/pc.c | 22 ++++++++++++++++++++++ include/hw/boards.h | 3 +++ qapi-schema.json | 11 +++++++++++ qmp-commands.hx | 23 +++++++++++++++++++++++ qmp.c | 10 ++++++++++ vl.c | 6 +++++- 6 files changed, 74 insertions(+), 1 deletion(-)