[Qemu-devel] [PATCH 0/4] s390 cpu-hotplug implementation

[Qemu-devel] [PATCH 0/4] s390 cpu-hotplug implementation

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Improvements and modifications to cpu initialization and the qom cpu model
were required to implement hotplug.

Hotplugged cpus are created in the configured state and can be used by the
guest after the guest onlines the cpu by: 
"echo 1 > /sys/bus/cpu/devices/cpuN/online"

Hot unplug is not currently implemented. 

Jason J. Herne (4):
  s390: Storage key global access
  s390-cpu: ipi_states enhancements
  s390-cpu: s390 cpu init improvements for hotplug
  s390-hotplug: Implement hot_add_cpu hook

 hw/s390x/s390-virtio-ccw.c |  4 ++--
 hw/s390x/s390-virtio.c     | 37 +++++++++++++++++++++----------------
 hw/s390x/s390-virtio.h     |  2 +-
 target-s390x/cpu.c         | 43 +++++++++++++++++++++++++++++++++++++++++--
 target-s390x/cpu.h         |  6 ++++++
 5 files changed, 71 insertions(+), 21 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/4] s390: Storage key global access

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Introduces global access to storage key data so we can set it for each cpu in
the S390 cpu initialization routine.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 3 +--
 hw/s390x/s390-virtio.c     | 6 +++---
 hw/s390x/s390-virtio.h     | 2 +-
 target-s390x/cpu.h         | 3 +++
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 733d988..62319b9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     int ret;
     VirtualCssBus *css_bus;
 
@@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args)
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     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 9eeda97..00807e7 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -53,6 +53,7 @@
 
 static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
+uint8_t *storage_keys;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -176,7 +177,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
+void s390_init_cpus(const char *cpu_model)
 {
     int i;
 
@@ -231,7 +232,6 @@ static void s390_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     void *virtio_region;
     hwaddr virtio_region_len;
     hwaddr virtio_region_start;
@@ -273,7 +273,7 @@ static void s390_init(QEMUMachineInitArgs *args)
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index 5c405e7..c1cb042 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -20,7 +20,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, uint8_t *storage_keys);
+void s390_init_cpus(const char *cpu_model);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 96c2b4a..1f4ca4f 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -369,6 +369,9 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 {
 }
 #endif
+
+extern uint8_t *storage_keys;
+
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/4] s390-cpu: ipi_states enhancements

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
above smp_cpus.  Hotplug requires this capability.

Also add s390_cpu_set_state function to allow modification of ipi_state entries
during hotplug.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio.c | 9 +++++----
 target-s390x/cpu.h     | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 00807e7..d47fecf 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -55,12 +55,13 @@ static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
 uint8_t *storage_keys;
 
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
 {
-    if (cpu_addr >= smp_cpus) {
-        return NULL;
-    }
+    ipi_states[cpu_addr] = state;
+}
 
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
     return ipi_states[cpu_addr];
 }
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 1f4ca4f..79f47b4 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -372,6 +372,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 
 extern uint8_t *storage_keys;
 
+void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state);
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 2/4] s390-cpu: ipi_states enhancements

[Andreas Färber]

Am 26.02.2014 18:18, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
> above smp_cpus.  Hotplug requires this capability.
> 
> Also add s390_cpu_set_state function to allow modification of ipi_state entries
> during hotplug.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>

This patch is *still* present despite your previous response that you
incorporated my review comments. You are adding a custom s390x API here
rather than reusing the generic QOM API as requested. I don't even read
further in such cases - like I pointed out, a pure resend is not helping
address review feedback!

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/4] s390-cpu: ipi_states enhancements

[Jason J. Herne]

On 02/28/2014 11:30 AM, Andreas Färber wrote:
> Am 26.02.2014 18:18, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
>> above smp_cpus.  Hotplug requires this capability.
>>
>> Also add s390_cpu_set_state function to allow modification of ipi_state entries
>> during hotplug.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>
> This patch is *still* present despite your previous response that you
> incorporated my review comments. You are adding a custom s390x API here
> rather than reusing the generic QOM API as requested. I don't even read
> further in such cases - like I pointed out, a pure resend is not helping
> address review feedback!
>
> Andreas
>

Andreas,

My apologies, I am not trying to ruffle any feathers and I certainly 
have not intended to make you feel like I've ignored your review 
comments. I believe, if you look at patches 3 and 4 you will find that 
I've address the majority of your comments. I've even completed the work 
that adds the link property as you have suggested.

The reason the ipi_array still exists in my patch set is because I was 
under the impression that we needed it to still exist. This belief 
stemmed from a misunderstanding I had related to an earlier e-mail 
discussion. I apologize for getting that detail wrong. I had assumed we 
would make use of the link property when possible but still keep the 
array around too.

At this point, I only have one concern about removing the array. 
ipi_states is accessed from some high frequency code paths dealing with 
interrupt handling. I have not yet looked into it myself, but I trust I 
would find that retrieving the link property value would not 
significantly affect performance?

Thank you for taking the time to read this patch set. Please let me know 
if you have any more concerns. I'm happy to work with you to address them.

P.S. Sorry for taking so long to respond, I was away on vacation for a 
few days and I was not checking mail.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 2/4] s390-cpu: ipi_states enhancements

[Jason J. Herne]

On 02/28/2014 11:30 AM, Andreas Färber wrote:
> Am 26.02.2014 18:18, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
>> above smp_cpus.  Hotplug requires this capability.
>>
>> Also add s390_cpu_set_state function to allow modification of ipi_state entries
>> during hotplug.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>
> This patch is *still* present despite your previous response that you
> incorporated my review comments. You are adding a custom s390x API here
> rather than reusing the generic QOM API as requested. I don't even read
> further in such cases - like I pointed out, a pure resend is not helping
> address review feedback!
>
> Andreas
>

Just a quick follow on note. Even though the link property has been 
added by this patch set, I just realized this part of the code is not 
yet complete. I'll work up a complete patch just to address this issue 
and post it separate from Hotplug. I think that makes the most sense 
because this is really a qom patch.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

[Qemu-devel] [PATCH 3/4] s390-cpu: s390 cpu init improvements for hotplug

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
object given a cpuid and a model string.

All actual cpu initialization code is moved from boot time specific functions to
s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to allow us
to use the same basic code path for a cpu created at boot time and one created
during a hotplug operation.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio.c | 28 ++++++++++++++++------------
 target-s390x/cpu.c     | 14 ++++++++++++--
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index d47fecf..99a10d8 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -57,11 +57,16 @@ uint8_t *storage_keys;
 
 void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
 {
-    ipi_states[cpu_addr] = state;
+    if (cpu_addr < max_cpus) {
+        ipi_states[cpu_addr] = state;
+    }
 }
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
     return ipi_states[cpu_addr];
 }
 
@@ -181,24 +186,23 @@ void s390_init_ipl_dev(const char *kernel_filename,
 void s390_init_cpus(const char *cpu_model)
 {
     int i;
+    gchar *name;
 
     if (cpu_model == NULL) {
         cpu_model = "host";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
-
-    for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
-        CPUState *cs;
+    ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
-        cpu = cpu_s390x_init(cpu_model);
-        cs = CPU(cpu);
+    for (i = 0; i < max_cpus; i++) {
+        name = g_strdup_printf("cpu[%i]", i);
+        object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
+                                 (Object **)&ipi_states[i], NULL);
+    g_free(name);
+    }
 
-        ipi_states[i] = cpu;
-        cs->halted = 1;
-        cpu->env.exception_index = EXCP_HLT;
-        cpu->env.storage_keys = storage_keys;
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_s390x_init(cpu_model);
     }
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index ff57b80..a6fa82c 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -34,6 +34,8 @@
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
 
+int next_cpu_num;
+
 /* generate CPU information for cpu -? */
 void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
@@ -150,6 +152,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 
     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)
@@ -158,13 +166,13 @@ 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
 
     cs->env_ptr = env;
     cpu_exec_init(env);
+    env->cpu_num = next_cpu_num++;
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
@@ -177,8 +185,10 @@ static void s390_cpu_initfn(Object *obj)
      * cpu counter in s390_cpu_reset to a negative number at
      * initial ipl */
     cs->halted = 1;
+    cpu->env.exception_index = EXCP_HLT;
+    env->storage_keys = storage_keys;
+    s390_cpu_set_ipistate(env->cpu_num, cpu);
 #endif
-    env->cpu_num = cpu_num++;
     env->ext_index = -1;
 
     if (tcg_enabled() && !inited) {
-- 
1.8.3.2

[Qemu-devel] [PATCH 4/4] s390-hotplug: Implement hot_add_cpu hook

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Implement hot_add_cpu for S390 to allow hot plugging of cpus.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |  1 +
 target-s390x/cpu.c         | 29 +++++++++++++++++++++++++++++
 target-s390x/cpu.h         |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62319b9..8b89886 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -132,6 +132,7 @@ static QEMUMachine ccw_machine = {
     .alias = "s390-ccw",
     .desc = "VirtIO-ccw based S390 machine",
     .init = ccw_init,
+    .hot_add_cpu = ccw_hot_add_cpu,
     .block_default_type = IF_VIRTIO,
     .no_cdrom = 1,
     .no_floppy = 1,
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index a6fa82c..fd667d9 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -27,6 +27,8 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "hw/hw.h"
+#include "hw/s390x/sclp.h"
+#include "sysemu/sysemu.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -206,6 +208,33 @@ static void s390_cpu_finalize(Object *obj)
 #endif
 }
 
+#if !defined(CONFIG_USER_ONLY)
+void ccw_hot_add_cpu(const int64_t id, Error **errp)
+{
+    S390CPU *new_cpu;
+    CPUState *cpu;
+    int cpu_count = 0;
+
+    CPU_FOREACH(cpu) {
+        cpu_count++;
+    }
+
+    if (cpu_count == max_cpus) {
+        error_setg(errp, "Maximum number of cpus already defined");
+        return;
+    }
+
+    if (id != next_cpu_num) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", The next available id is %d", id, next_cpu_num);
+        return;
+    }
+
+    new_cpu = S390_CPU(object_new(TYPE_S390_CPU));
+    object_property_set_bool(OBJECT(new_cpu), true, "realized", NULL);
+}
+#endif
+
 static const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .unmigratable = 1,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 79f47b4..af199e5 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -377,6 +377,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
 
+void ccw_hot_add_cpu(const int64_t id, Error **errp);
+
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
 
-- 
1.8.3.2