[Qemu-devel] [PATCH v6 0/7] Allow hotplug of s390 CPUs

[Qemu-devel] [PATCH v6 0/7] Allow hotplug of s390 CPUs

[Matthew Rosato]

Changes from v5->v6:

* Patch #5 - Remove unnecessary unplug handler (Igor)
* Patch #6 - New patch - move last_cpu to qom/cpu.h, rename one existing
  variable to 'cpu_last' to avoid conflicts. (Igor)
* Patch #7 - Move s390_cpu_hot_add to s390-virtio-ccw & make static. (Igor)

**************

As discussed in the KVM call, we will go ahead with cpu_add for 
s390x to get cpu hotplug functionality in s390x now, until
architectures that require a more robust hotplug interface
settle on a design.

To configure a guest with 2 CPUs online at 
boot and 4 maximum:

qemu -smp 2,maxcpus=4

Or, when using libvirt:
  <domain>
    ...
    <vcpu current="2">4</vcpu>
    ...
  </domain> 


To subsequently hotplug a CPU:

Issue 'cpu-add <id>' from qemu monitor, or use virsh setvcpus --count <n> 
<domain>, where <n> is the total number of desired guest CPUs.

At this point, the guest must bring the CPU online for use -- This can be 
achieved via "echo 1 > /sys/devices/system/cpu/cpuX/online" or via a management 
tool like cpuplugd.

This patch set is based on work previously done by Jason Herne.

Matthew Rosato (7):
  s390x/cpu: Cleanup init in preparation for hotplug
  s390x/cpu: Set initial CPU state in common routine
  s390x/cpu: Move some CPU initialization into realize
  s390x/cpu: Add CPU property links
  s390/virtio-ccw: Add hotplug handler
  cpu: Add a last_cpu macro
  s390x/cpu: Allow hotplug of CPUs

 hw/intc/openpic.c          | 12 ++++----
 hw/s390x/s390-virtio-ccw.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     | 36 +++++++++++++-----------
 hw/s390x/s390-virtio.h     |  2 +-
 include/qom/cpu.h          |  1 +
 target-s390x/cpu.c         | 18 ++++++++++--
 6 files changed, 110 insertions(+), 27 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v6 1/7] s390x/cpu: Cleanup init in preparation for hotplug

[Matthew Rosato]

Ensure a valid cpu_model is set upfront by setting the
default value directly into the MachineState when none is
specified.  This is needed to ensure hotplugged CPUs share
the same cpu_model.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 2 +-
 hw/s390x/s390-virtio.c     | 8 ++++----
 hw/s390x/s390-virtio.h     | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 89f5d0d..b05ed8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
     virtio_ccw_register_hcalls();
 
     /* init CPUs */
-    s390_init_cpus(machine->cpu_model);
+    s390_init_cpus(machine);
 
     if (kvm_enabled()) {
         kvm_s390_enable_css_support(s390_cpu_addr2state(0));
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index c320878..b576811 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -95,12 +95,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model)
+void s390_init_cpus(MachineState *machine)
 {
     int i;
 
-    if (cpu_model == NULL) {
-        cpu_model = "host";
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = "host";
     }
 
     ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
@@ -109,7 +109,7 @@ void s390_init_cpus(const char *cpu_model)
         S390CPU *cpu;
         CPUState *cs;
 
-        cpu = cpu_s390x_init(cpu_model);
+        cpu = cpu_s390x_init(machine->cpu_model);
         cs = CPU(cpu);
 
         ipi_states[i] = cpu;
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index eebce8e..ffd014c 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -19,7 +19,7 @@
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
-void s390_init_cpus(const char *cpu_model);
+void s390_init_cpus(MachineState *machine);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
-- 
1.9.1

[Qemu-devel] [PATCH v6 2/7] s390x/cpu: Set initial CPU state in common routine

[Matthew Rosato]

Both initial and hotplugged CPUs need to set the same initial
state.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c | 4 ----
 target-s390x/cpu.c     | 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index b576811..b3707f4 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -107,14 +107,10 @@ void s390_init_cpus(MachineState *machine)
 
     for (i = 0; i < smp_cpus; i++) {
         S390CPU *cpu;
-        CPUState *cs;
 
         cpu = cpu_s390x_init(machine->cpu_model);
-        cs = CPU(cpu);
 
         ipi_states[i] = cpu;
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
     }
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 73a910d..603c2a1 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -219,6 +219,8 @@ static void s390_cpu_initfn(Object *obj)
 #endif
 
     cs->env_ptr = env;
+    cs->halted = 1;
+    cs->exception_index = EXCP_HLT;
     cpu_exec_init(cs, &error_abort);
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
-- 
1.9.1

[Qemu-devel] [PATCH v6 3/7] s390x/cpu: Move some CPU initialization into realize

[Matthew Rosato]

In preparation for hotplug, defer some CPU initialization
until the device is actually being realized.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 target-s390x/cpu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 603c2a1..8dfd063 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -195,7 +195,13 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+    S390CPU *cpu = S390_CPU(dev);
+    CPUS390XState *env = &cpu->env;
 
+#if !defined(CONFIG_USER_ONLY)
+    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
+#endif
+    env->cpu_num = cs->cpu_index;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -213,7 +219,6 @@ static void s390_cpu_initfn(Object *obj)
     S390CPU *cpu = S390_CPU(obj);
     CPUS390XState *env = &cpu->env;
     static bool inited;
-    static int cpu_num = 0;
 #if !defined(CONFIG_USER_ONLY)
     struct tm tm;
 #endif
@@ -223,7 +228,6 @@ static void s390_cpu_initfn(Object *obj)
     cs->exception_index = EXCP_HLT;
     cpu_exec_init(cs, &error_abort);
 #if !defined(CONFIG_USER_ONLY)
-    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
                       (time2tod(mktimegm(&tm)) * 1000000000ULL);
@@ -232,7 +236,6 @@ static void s390_cpu_initfn(Object *obj)
     env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
     s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
 #endif
-    env->cpu_num = cpu_num++;
 
     if (tcg_enabled() && !inited) {
         inited = true;
-- 
1.9.1

[Qemu-devel] [PATCH v6 4/7] s390x/cpu: Add CPU property links

[Matthew Rosato]

Link each CPUState as property machine/cpu[n] during initialization.
Additionally, maintain an array of state pointers indexed by CPU
id for fast lookup during interrupt handling.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index b3707f4..6bd9803 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -60,15 +60,16 @@
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **ipi_states;
+static S390CPU **cpu_states;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= smp_cpus) {
+    if (cpu_addr >= max_cpus) {
         return NULL;
     }
 
-    return ipi_states[cpu_addr];
+    /* Fast lookup via CPU ID */
+    return cpu_states[cpu_addr];
 }
 
 void s390_init_ipl_dev(const char *kernel_filename,
@@ -98,19 +99,26 @@ void s390_init_ipl_dev(const char *kernel_filename,
 void s390_init_cpus(MachineState *machine)
 {
     int i;
+    gchar *name;
 
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "host";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
+    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
-    for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
-
-        cpu = cpu_s390x_init(machine->cpu_model);
+    for (i = 0; i < max_cpus; i++) {
+        name = g_strdup_printf("cpu[%i]", i);
+        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
+                                 (Object **) &cpu_states[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+        g_free(name);
+    }
 
-        ipi_states[i] = cpu;
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_s390x_init(machine->cpu_model);
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH v6 5/7] s390/virtio-ccw: Add hotplug handler

[Matthew Rosato]

First consumer will be CPU hotplug, to update machine/cpu[n]
links during cpu plug.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b05ed8b..3090e76 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -156,10 +156,40 @@ static void ccw_init(MachineState *machine)
                     gtod_save, gtod_load, kvm_state);
 }
 
+static void s390_cpu_plug(HotplugHandler *hotplug_dev,
+                        DeviceState *dev, Error **errp)
+{
+    gchar *name;
+    S390CPU *cpu = S390_CPU(dev);
+    CPUState *cs = CPU(dev);
+
+    name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+    object_property_set_link(OBJECT(qdev_get_machine()), OBJECT(cs), name,
+                             errp);
+}
+
+static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        s390_cpu_plug(hotplug_dev, dev, errp);
+    }
+}
+
+static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
+                                                DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+    return NULL;
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
@@ -171,6 +201,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 255;
+    mc->get_hotplug_handler = s390_get_hotplug_handler;
+    hc->plug = s390_machine_device_plug;
     nc->nmi_monitor_handler = s390_nmi;
 }
 
@@ -232,6 +264,7 @@ static const TypeInfo ccw_machine_info = {
     .class_init    = ccw_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_NMI },
+        { TYPE_HOTPLUG_HANDLER},
         { }
     },
 };
-- 
1.9.1

[Qemu-devel] [PATCH v6 6/7] cpu: Add a last_cpu macro

[Matthew Rosato]

Add last_cpu to grab last CPU in the queue.  Rename one existing
use of last_cpu as a variable name.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/intc/openpic.c | 12 ++++++------
 include/qom/cpu.h |  1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 903888c..0dd908e 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -217,7 +217,7 @@ typedef struct IRQSource {
     uint32_t ivpr;  /* IRQ vector/priority register */
     uint32_t idr;   /* IRQ destination register */
     uint32_t destmask; /* bitmap of CPU destinations */
-    int last_cpu;
+    int cpu_last;
     int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
     int pending;    /* TRUE if IRQ is pending */
     IRQType type;
@@ -476,9 +476,9 @@ static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
         return;
     }
 
-    if (src->destmask == (1 << src->last_cpu)) {
+    if (src->destmask == (1 << src->cpu_last)) {
         /* Only one CPU is allowed to receive this IRQ */
-        IRQ_local_pipe(opp, src->last_cpu, n_IRQ, active, was_active);
+        IRQ_local_pipe(opp, src->cpu_last, n_IRQ, active, was_active);
     } else if (!(src->ivpr & IVPR_MODE_MASK)) {
         /* Directed delivery mode */
         for (i = 0; i < opp->nb_cpus; i++) {
@@ -488,13 +488,13 @@ static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
         }
     } else {
         /* Distributed delivery mode */
-        for (i = src->last_cpu + 1; i != src->last_cpu; i++) {
+        for (i = src->cpu_last + 1; i != src->cpu_last; i++) {
             if (i == opp->nb_cpus) {
                 i = 0;
             }
             if (src->destmask & (1 << i)) {
                 IRQ_local_pipe(opp, i, n_IRQ, active, was_active);
-                src->last_cpu = i;
+                src->cpu_last = i;
                 break;
             }
         }
@@ -1444,7 +1444,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
         VMSTATE_UINT32(ivpr, IRQSource),
         VMSTATE_UINT32(idr, IRQSource),
         VMSTATE_UINT32(destmask, IRQSource),
-        VMSTATE_INT32(last_cpu, IRQSource),
+        VMSTATE_INT32(cpu_last, IRQSource),
         VMSTATE_INT32(pending, IRQSource),
         VMSTATE_END_OF_LIST()
     }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index ff54600..3645c19 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -364,6 +364,7 @@ extern struct CPUTailQ cpus;
 #define CPU_FOREACH_REVERSE(cpu) \
     QTAILQ_FOREACH_REVERSE(cpu, &cpus, CPUTailQ, node)
 #define first_cpu QTAILQ_FIRST(&cpus)
+#define last_cpu QTAILQ_LAST(&cpus, CPUTailQ)
 
 extern __thread CPUState *current_cpu;
 
-- 
1.9.1

[Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs

[Matthew Rosato]

Implement cpu hotplug routine and add the machine hook.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
 target-s390x/cpu.c         |  7 +++++++
 2 files changed, 40 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3090e76..ea007e8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -22,6 +22,7 @@
 #include "s390-pci-bus.h"
 #include "hw/s390x/storage-keys.h"
 #include "hw/compat.h"
+#include "qom/cpu.h"
 
 #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
 
@@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
+static void s390_hot_add_cpu(const int64_t id, Error **errp)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+
+    if (id < 0) {
+        error_setg(errp, "Invalid CPU id: %" PRIi64, id);
+        return;
+    }
+
+    if (cpu_exists(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;
+    }
+
+    if (id != (last_cpu->cpu_index + 1)) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", The next available id is %d", id,
+                   (last_cpu->cpu_index + 1));
+        return;
+    }
+
+    cpu_s390x_init(machine->cpu_model);
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -193,6 +225,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
 
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
+    mc->hot_add_cpu = s390_hot_add_cpu;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->no_floppy = 1;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 8dfd063..1a4977e 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -32,6 +32,7 @@
 #include "trace.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -211,6 +212,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #endif
 
     scc->parent_realize(dev, errp);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (dev->hotplugged) {
+        raise_irq_cpu_hotplug();
+    }
+#endif
 }
 
 static void s390_cpu_initfn(Object *obj)
-- 
1.9.1

Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
>  target-s390x/cpu.c         |  7 +++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..ea007e8 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -22,6 +22,7 @@
>  #include "s390-pci-bus.h"
>  #include "hw/s390x/storage-keys.h"
>  #include "hw/compat.h"
> +#include "qom/cpu.h"
>  
>  #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
>  
> @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>      return NULL;
>  }
>  
> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    if (id < 0) {
> +        error_setg(errp, "Invalid CPU id: %" PRIi64, id);
> +        return;
> +    }
> +
> +    if (cpu_exists(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;
> +    }
> +
> +    if (id != (last_cpu->cpu_index + 1)) {

This assumption about the order of the CPU list looks dangerous to me.
Aren't there global int fields used by x86 already for the number of
CPUs that you could use instead? That will allow you to drop the ugly
preceding renaming patch. Please avoid messing with the CPU list
directly from target code.

> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", The next available id is %d", id,
> +                   (last_cpu->cpu_index + 1));
> +        return;
> +    }
> +
> +    cpu_s390x_init(machine->cpu_model);

No proper error handling - that's why blindly reusing these old helpers
is a bad idea.

Regards,
Andreas

> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -193,6 +225,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  
>      mc->init = ccw_init;
>      mc->reset = s390_machine_reset;
> +    mc->hot_add_cpu = s390_hot_add_cpu;
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
>      mc->no_floppy = 1;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 8dfd063..1a4977e 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -32,6 +32,7 @@
>  #include "trace.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "hw/s390x/sclp.h"
>  #endif
>  
>  #define CR0_RESET       0xE0UL
> @@ -211,6 +212,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>  #endif
>  
>      scc->parent_realize(dev, errp);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    if (dev->hotplugged) {
> +        raise_irq_cpu_hotplug();
> +    }
> +#endif
>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 6/7] cpu: Add a last_cpu macro

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Add last_cpu to grab last CPU in the queue.  Rename one existing
> use of last_cpu as a variable name.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/intc/openpic.c | 12 ++++++------
>  include/qom/cpu.h |  1 +
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 903888c..0dd908e 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -217,7 +217,7 @@ typedef struct IRQSource {
>      uint32_t ivpr;  /* IRQ vector/priority register */
>      uint32_t idr;   /* IRQ destination register */
>      uint32_t destmask; /* bitmap of CPU destinations */
> -    int last_cpu;
> +    int cpu_last;

If we do need to rename this, what about last_cpu_index? cpu_last reads
really ugly.

>      int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
>      int pending;    /* TRUE if IRQ is pending */
>      IRQType type;
> @@ -476,9 +476,9 @@ static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
>          return;
>      }
>  
> -    if (src->destmask == (1 << src->last_cpu)) {
> +    if (src->destmask == (1 << src->cpu_last)) {
>          /* Only one CPU is allowed to receive this IRQ */
> -        IRQ_local_pipe(opp, src->last_cpu, n_IRQ, active, was_active);
> +        IRQ_local_pipe(opp, src->cpu_last, n_IRQ, active, was_active);
>      } else if (!(src->ivpr & IVPR_MODE_MASK)) {
>          /* Directed delivery mode */
>          for (i = 0; i < opp->nb_cpus; i++) {
> @@ -488,13 +488,13 @@ static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
>          }
>      } else {
>          /* Distributed delivery mode */
> -        for (i = src->last_cpu + 1; i != src->last_cpu; i++) {
> +        for (i = src->cpu_last + 1; i != src->cpu_last; i++) {
>              if (i == opp->nb_cpus) {
>                  i = 0;
>              }
>              if (src->destmask & (1 << i)) {
>                  IRQ_local_pipe(opp, i, n_IRQ, active, was_active);
> -                src->last_cpu = i;
> +                src->cpu_last = i;
>                  break;
>              }
>          }
> @@ -1444,7 +1444,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
>          VMSTATE_UINT32(ivpr, IRQSource),
>          VMSTATE_UINT32(idr, IRQSource),
>          VMSTATE_UINT32(destmask, IRQSource),
> -        VMSTATE_INT32(last_cpu, IRQSource),
> +        VMSTATE_INT32(cpu_last, IRQSource),

This name change shows up in the VMState description, e.g. in the JSON.
Migration will still work, but we should avoid it for debugging
cross-version migration.

>          VMSTATE_INT32(pending, IRQSource),
>          VMSTATE_END_OF_LIST()
>      }
[actual change snipped]

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 3/7] s390x/cpu: Move some CPU initialization into realize

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  target-s390x/cpu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Looks reasonable on a brief sight,

Reviewed-by: Andreas Färber <afaerber@suse.de>

What is env->cpu_num used for? In particular, have you thought about
linux-user creating multiple CPUs and possibly destroying them again?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 1/7] s390x/cpu: Cleanup init in preparation for hotplug

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Ensure a valid cpu_model is set upfront by setting the
> default value directly into the MachineState when none is
> specified.  This is needed to ensure hotplugged CPUs share
> the same cpu_model.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  hw/s390x/s390-virtio.c     | 8 ++++----
>  hw/s390x/s390-virtio.h     | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 89f5d0d..b05ed8b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
>      virtio_ccw_register_hcalls();
>  
>      /* init CPUs */
> -    s390_init_cpus(machine->cpu_model);
> +    s390_init_cpus(machine);
>  
>      if (kvm_enabled()) {
>          kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c320878..b576811 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -95,12 +95,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
>      qdev_init_nofail(dev);
>  }
>  
> -void s390_init_cpus(const char *cpu_model)
> +void s390_init_cpus(MachineState *machine)
>  {
>      int i;
>  
> -    if (cpu_model == NULL) {
> -        cpu_model = "host";
> +    if (machine->cpu_model == NULL) {
> +        machine->cpu_model = "host";

When/why is cpu_model == NULL? Could you simply set it as a default in
your machine's instance_init?

Regards,
Andreas

>      }
>  
>      ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> @@ -109,7 +109,7 @@ void s390_init_cpus(const char *cpu_model)
>          S390CPU *cpu;
>          CPUState *cs;
>  
> -        cpu = cpu_s390x_init(cpu_model);
> +        cpu = cpu_s390x_init(machine->cpu_model);
>          cs = CPU(cpu);
>  
>          ipi_states[i] = cpu;
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index eebce8e..ffd014c 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -19,7 +19,7 @@
>  typedef int (*s390_virtio_fn)(const uint64_t *args);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>  
> -void s390_init_cpus(const char *cpu_model);
> +void s390_init_cpus(MachineState *machine);
>  void s390_init_ipl_dev(const char *kernel_filename,
>                         const char *kernel_cmdline,
>                         const char *initrd_filename,
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 1/7] s390x/cpu: Cleanup init in preparation for hotplug

[Matthew Rosato]

On 02/22/2016 12:46 PM, Andreas Färber wrote:
> Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
>> Ensure a valid cpu_model is set upfront by setting the
>> default value directly into the MachineState when none is
>> specified.  This is needed to ensure hotplugged CPUs share
>> the same cpu_model.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 2 +-
>>  hw/s390x/s390-virtio.c     | 8 ++++----
>>  hw/s390x/s390-virtio.h     | 2 +-
>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 89f5d0d..b05ed8b 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
>>      virtio_ccw_register_hcalls();
>>  
>>      /* init CPUs */
>> -    s390_init_cpus(machine->cpu_model);
>> +    s390_init_cpus(machine);
>>  
>>      if (kvm_enabled()) {
>>          kvm_s390_enable_css_support(s390_cpu_addr2state(0));
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index c320878..b576811 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -95,12 +95,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
>>      qdev_init_nofail(dev);
>>  }
>>  
>> -void s390_init_cpus(const char *cpu_model)
>> +void s390_init_cpus(MachineState *machine)
>>  {
>>      int i;
>>  
>> -    if (cpu_model == NULL) {
>> -        cpu_model = "host";
>> +    if (machine->cpu_model == NULL) {
>> +        machine->cpu_model = "host";
> 
> When/why is cpu_model == NULL? Could you simply set it as a default in
> your machine's instance_init?
> 

Yes, we definitely did this in an earlier version. I'll set this during
machine instance_init.

Matt

Re: [Qemu-devel] [PATCH v6 3/7] s390x/cpu: Move some CPU initialization into realize

[Matthew Rosato]

On 02/22/2016 12:35 PM, Andreas Färber wrote:
> Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
>> In preparation for hotplug, defer some CPU initialization
>> until the device is actually being realized.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  target-s390x/cpu.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> Looks reasonable on a brief sight,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> What is env->cpu_num used for? In particular, have you thought about
> linux-user creating multiple CPUs and possibly destroying them again?
> 

env->cpu_num is intended to map to the s390x architecture concept of a
cpu address, which is a numeric ID used to differentiate between CPUs
when a targeted event occurs (like an unsolicited interrupt that must be
handled on behalf of a particular CPU).  s390x architecture currently
cannot tolerate CPU destruction.

Matt

Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs

[Matthew Rosato]

On 02/22/2016 12:22 PM, Andreas Färber wrote:
> Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
>> Implement cpu hotplug routine and add the machine hook.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
>>  target-s390x/cpu.c         |  7 +++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3090e76..ea007e8 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -22,6 +22,7 @@
>>  #include "s390-pci-bus.h"
>>  #include "hw/s390x/storage-keys.h"
>>  #include "hw/compat.h"
>> +#include "qom/cpu.h"
>>  
>>  #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
>>  
>> @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>>      return NULL;
>>  }
>>  
>> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
>> +{
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +
>> +    if (id < 0) {
>> +        error_setg(errp, "Invalid CPU id: %" PRIi64, id);
>> +        return;
>> +    }
>> +
>> +    if (cpu_exists(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;
>> +    }
>> +
>> +    if (id != (last_cpu->cpu_index + 1)) {
> 
> This assumption about the order of the CPU list looks dangerous to me.
> Aren't there global int fields used by x86 already for the number of
> CPUs that you could use instead? That will allow you to drop the ugly
> preceding renaming patch. Please avoid messing with the CPU list
> directly from target code.
> 

Not that I can find.  We were keeping track of it with our own int for
s390x, I just switched to this approach per review suggestion -- Sounds
like you'd rather I bring back the int and inspect that, that's fine by
me.

>> +        error_setg(errp, "Unable to add CPU: %" PRIi64
>> +                   ", The next available id is %d", id,
>> +                   (last_cpu->cpu_index + 1));
>> +        return;
>> +    }
>> +
>> +    cpu_s390x_init(machine->cpu_model);
> 
> No proper error handling - that's why blindly reusing these old helpers
> is a bad idea.
> 

Good point -- Let me re-work this into something more like f(model, id,
local_err).

Matt