Patchwork [16/16] add cpu-add qmp command and implement CPU hot-add for target-i386

login
register
mail settings
Submitter Igor Mammedov
Date April 15, 2013, 10:12 p.m.
Message ID <1366063976-4909-17-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/236745/
State New
Headers show

Comments

Igor Mammedov - April 15, 2013, 10:12 p.m.
... 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(-)
Eric Blake - April 15, 2013, 10:20 p.m.
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?
Igor Mammedov - April 16, 2013, 8:04 p.m.
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
>
Eric Blake - April 23, 2013, 4:17 p.m.
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.

Patch

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)) {