s390x cleanups and CPU hotplug via device_add

All but a handfull of filles include exec/cpu-all.h via cpu.h only.

As these files already include cpu.h, let's just drop the additional include.

Signed-off-by David Hildenbrand <[email protected]>

---
 dump.c                            | 1 -
 exec.c                            | 2 --
 target/i386/arch_dump.c           | 1 -
 target/i386/arch_memory_mapping.c | 1 -
 target/i386/svm_helper.c          | 1 -
 target/ppc/arch_dump.c            | 1 -
 target/s390x/arch_dump.c          | 1 -
 7 files changed, 8 deletions(-)

dump.c +0 -1

index a79773d0f7..2ef6a678e8 100644
@@ -15,7 +15,6 @@
 #include "qemu/cutils.h"
 #include "elf.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"

exec.c +0 -2

index d20c34ca83..d109de2014 100644
@@ -23,7 +23,6 @@
 
 #include "qemu/cutils.h"
 #include "cpu.h"
-#include "exec/exec-all.h"
 #include "exec/target_page.h"
 #include "tcg.h"
 #include "hw/qdev-core.h"

@@ -56,7 +55,6 @@
 #endif
 
 #endif
-#include "exec/cpu-all.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
 #include "translate-all.h"

target/i386/arch_dump.c +0 -1

index e682904052..35b55fc200 100644
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "elf.h"
 #include "sysemu/memory_mapping.h"

target/i386/arch_memory_mapping.c +0 -1

index 647cff2829..271cb5e41b 100644
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/memory_mapping.h"
 
 /* PAE Paging or IA-32e Paging */

target/i386/svm_helper.c +0 -1

index 59e8b5091c..f479239875 100644
@@ -19,7 +19,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"

target/ppc/arch_dump.c +0 -1

index 8e9397aa58..beb7268e57 100644
@@ -15,7 +15,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "sysemu/kvm.h"
 

target/s390x/arch_dump.c +0 -1

index 9b0bf92698..6f61ff95af 100644
@@ -15,7 +15,6 @@
 #include "cpu.h"
 #include "internal.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 
 

All but a handfull of filles include exec/cpu-all.h via cpu.h only. As these files already include cpu.h, let's just drop the additional include.

Signed-off-by David Hildenbrand <[email protected]>

---
 dump.c                            | 1 -
 exec.c                            | 2 --
 target/i386/arch_dump.c           | 1 -
 target/i386/arch_memory_mapping.c | 1 -
 target/i386/svm_helper.c          | 1 -
 target/ppc/arch_dump.c            | 1 -
 target/s390x/arch_dump.c          | 1 -
 7 files changed, 8 deletions(-)

dump.c +0 -1

index a79773d0f7..2ef6a678e8 100644
@@ -15,7 +15,6 @@
 #include "qemu/cutils.h"
 #include "elf.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"

exec.c +0 -0

index d20c34ca83..d109de2014 100644

Quoted 3 lines

#include "qemu/cutils.h"
#include "cpu.h"
-#include "exec/exec-all.h"

You should maybe mention exec-all.h in the patch description, too. (I think it can be dropped here because it is included by translate-all.h already).

If you tweak the patch description:

Reviewed-by Thomas Huth <[email protected]>

Quoted 7 lines · Thomas Huth

Quoted 3 lines

#include "qemu/cutils.h"
#include "cpu.h"
-#include "exec/exec-all.h"

You should maybe mention exec-all.h in the patch description, too. (I think it can be dropped here because it is included by translate-all.h already).

Quoted 2 lines

If you tweak the patch description:

Reviewed-by Thomas Huth <[email protected]>

I'll just drop that hunk although it can also be removed.

Yes, this was not included on purpose - these similar names still confuse me :)

The only interface left, so let's properly rename it.

Reviewed-by Thomas Huth <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (77%)

hw/s390x/s390-virtio-ccw.c +1 -1

index 4c3a3e177b..f67b4b5d58 100644
@@ -16,7 +16,7 @@
 #include "cpu.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
-#include "s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"

hw/s390x/s390-virtio-hcall.c +1 -1

index 23d67d6170..ec7cf8beb3 100644
@@ -11,7 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/s390x/s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 
 #define MAX_DIAG_SUBCODES 255
 

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +4 -4

similarity index 77%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..59fcba3a06 100644
@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <[email protected]>

@@ -9,12 +9,12 @@
  * directory.
  */
 
-#ifndef HW_S390_VIRTIO_H
-#define HW_S390_VIRTIO_H
+#ifndef HW_S390_VIRTIO_HCALL_H
+#define HW_S390_VIRTIO_HCALL_H
 
 #include "standard-headers/asm-s390/kvm_virtio.h"
 #include "standard-headers/asm-s390/virtio-ccw.h"
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
-#endif
+#endif /* HW_S390_VIRTIO_HCALL_H */

Let's move it to the palce where struct S390CPU is defined.

Suggested-by Thomas Huth <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu-qom.h | 2 --
 target/s390x/cpu.h     | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

Let's move it to the palce where struct S390CPU is defined.

Suggested-by Thomas Huth <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu-qom.h | 2 --
 target/s390x/cpu.h     | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

Implemented in hw/s390x/s390-virtio-hcall.c, so let's move it to the right header file.

We can also drop the extern. Fix up includes.

Signed-off-by David Hildenbrand <[email protected]>

---
 include/hw/s390x/s390-virtio-ccw.h | 3 +++
 target/s390x/cpu.h                 | 2 --
 target/s390x/diag.c                | 1 +
 target/s390x/interrupt.c           | 3 +++
 4 files changed, 7 insertions(+), 2 deletions(-)

include/hw/s390x/s390-virtio-ccw.h +3 -0

index 41a9d2862b..42f2ccdb43 100644
@@ -56,4 +56,7 @@ bool gs_allowed(void);
  */
 bool css_migration_enabled(void);
 
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
+void subsystem_reset(void);
+
 #endif

target/s390x/cpu.h +0 -2

index 2343f2f1f7..6c0abb9894 100644
@@ -720,8 +720,6 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 
 
 /* outside of target/s390x/ */
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-extern void subsystem_reset(void);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif

target/s390x/diag.c +1 -0

index e6b5e6de37..82a623948d 100644
@@ -20,6 +20,7 @@
 #include "hw/watchdog/wdt_diag288.h"
 #include "sysemu/cpus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 static int modified_clear_reset(S390CPU *cpu)
 {

target/s390x/interrupt.c +0 -0

index 058e219fe5..9aa6b89301 100644

Quoted 10 lines

+++ b/target/s390x/interrupt.c

target/s390x/interrupt.c +3 -0

@@ -15,6 +15,9 @@
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
 #include "hw/s390x/ioinst.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/s390x/s390-virtio-ccw.h"
+#endif
 

target/s390x/interrupt.c line 23

Quoted 3 lines

target/s390x/interrupt.c +0 -0

 /* Ensure to exit the TB after this call! */
 void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)

If I listen to my gut feeling, I still think we should rather move s390_cpu_addr2state() to cpu.c instead. It does not sound as it's really specific to the virtio-ccw machine... Just my 0.02 €

Thomas

Quoted 9 lines

Quoted 9 lines

+++ b/target/s390x/interrupt.c

target/s390x/interrupt.c +3 -0

@@ -15,6 +15,9 @@
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
 #include "hw/s390x/ioinst.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/s390x/s390-virtio-ccw.h"
+#endif
 

Quoted 8 lines

target/s390x/interrupt.c line 23

Quoted 3 lines

target/s390x/interrupt.c +0 -0

 /* Ensure to exit the TB after this call! */
 void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)

If I listen to my gut feeling, I still think we should rather move s390_cpu_addr2state() to cpu.c instead. It does not sound as it's really specific to the virtio-ccw machine... Just my 0.02 €

I prefer to leave it as is for now, so just a header cleanup. E.g. with patch 18 we could implement it directly using ms->possible_cpus.

Fix up includes and rename it to s390x_*.

cpu.h is now free of externally implemented functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/sclp.c            | 51 +---------------------------------------------
 include/hw/s390x/sclp.h    |  1 +
 target/s390x/cpu.h         |  5 +----
 target/s390x/interrupt.c   | 49 ++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/kvm.c         |  2 +-
 target/s390x/misc_helper.c |  2 +-
 6 files changed, 54 insertions(+), 56 deletions(-)

Quoted 1 line · On Mon, 4 Sep 2017 17:43:05 +0200 David Hildenbrand <[email protected]> wrote:

Fix up includes and rename it to s390x_*.

I'm not quite sure whether that is the right direction: servc is just an instruction that does something and then happens to also generate an interrupt on conclusion. I'll think a bit more about it.

Quoted 5 lines · Cornelia Huck

Quoted 1 line · On Mon, 4 Sep 2017 17:43:05 +0200 David Hildenbrand <[email protected]> wrote:

Fix up includes and rename it to s390x_*.

Quoted 4 lines

I'm not quite sure whether that is the right direction: servc is just an instruction that does something and then happens to also generate an interrupt on conclusion. I'll think a bit more about it.

Having CPU related stuff in sclp looks also wrong. Feel free to skip this patch, should be unrelated to the following patches.

Quoted 11 lines · David Hildenbrand

Quoted 9 lines · Cornelia Huck

Quoted 1 line · On Mon, 4 Sep 2017 17:43:05 +0200 David Hildenbrand <[email protected]> wrote:

Fix up includes and rename it to s390x_*.

I'm not quite sure whether that is the right direction: servc is just an instruction that does something and then happens to also generate an interrupt on conclusion. I'll think a bit more about it.

Quoted 2 lines

Having CPU related stuff in sclp looks also wrong. Feel free to skip this patch, should be unrelated to the following patches.

I think having the sclp instruction handler in sclp (as today) is the best compromise.

Quoted 17 lines · Christian Borntraeger

Quoted 13 lines · David Hildenbrand

Quoted 9 lines · Cornelia Huck

Quoted 1 line · On Mon, 4 Sep 2017 17:43:05 +0200 David Hildenbrand <[email protected]> wrote:

Fix up includes and rename it to s390x_*.

I'm not quite sure whether that is the right direction: servc is just an instruction that does something and then happens to also generate an interrupt on conclusion. I'll think a bit more about it.

Having CPU related stuff in sclp looks also wrong. Feel free to skip this patch, should be unrelated to the following patches.

Quoted 2 lines

I think having the sclp instruction handler in sclp (as today) is the best compromise.

+1

But maybe you could at least move the prototype to sclp.h instead?

This looks cleaner. linux-user will not use the ilen field, so setting it doesn't do any harm.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

target/s390x/excp_helper.c +0 -0

index 361f970db3..14d3160e92 100644

Clean it up by reusing program_interrupt(). Add a concern regarding ilen.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

target/s390x/misc_helper.c +2 -6

index 57c02ddf1b..5096286157 100644
@@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
 #ifndef CONFIG_USER_ONLY
 void HELPER(per_check_exception)(CPUS390XState *env)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint32_t ilen;
 
     if (env->per_perc_atmid) {
-        env->int_pgm_code = PGM_PER;
-        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
-
-        cs->exception_index = EXCP_PGM;
-        cpu_loop_exit(cs);
+        /*

Quoted 32 lines · On Mon, 4 Sep 2017 17:43:07 +0200 David Hildenbrand <[email protected]> wrote:

Clean it up by reusing program_interrupt(). Add a concern regarding ilen.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

target/s390x/misc_helper.c +8 -6

index 57c02ddf1b..5096286157 100644
@@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
 #ifndef CONFIG_USER_ONLY
 void HELPER(per_check_exception)(CPUS390XState *env)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint32_t ilen;
 
     if (env->per_perc_atmid) {
-        env->int_pgm_code = PGM_PER;
-        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
-
-        cs->exception_index = EXCP_PGM;
-        cpu_loop_exit(cs);
+        /*
+         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
+         * always has to match the instruction referenced in the PSW. E.g.
+         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
+         * of EXECUTE, while per_address contains the target of EXECUTE.
+         */
+        ilen = get_ilen(cpu_ldub_code(env, env->per_address));

target/s390x/misc_helper.c void HELPER(chsc)(CPUS390XState *env, uint64_t inst) · line 462

Quoted 4 lines

target/s390x/misc_helper.c +1 -0

+        program_interrupt(env, PGM_PER, ilen);
     }
 }
 

Wrapping my head around it: You preserve the current behavior, which should probably be changed to use ILEN_AUTO to handle cases like EXECUTE correctly?

Do you plan to do a follow up? (Should I spend some cycles on it?)

Quoted 35 lines · Cornelia Huck

Quoted 32 lines · On Mon, 4 Sep 2017 17:43:07 +0200 David Hildenbrand <[email protected]> wrote:

Clean it up by reusing program_interrupt(). Add a concern regarding ilen.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

target/s390x/misc_helper.c +8 -6

index 57c02ddf1b..5096286157 100644
@@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
 #ifndef CONFIG_USER_ONLY
 void HELPER(per_check_exception)(CPUS390XState *env)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint32_t ilen;
 
     if (env->per_perc_atmid) {
-        env->int_pgm_code = PGM_PER;
-        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
-
-        cs->exception_index = EXCP_PGM;
-        cpu_loop_exit(cs);
+        /*
+         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
+         * always has to match the instruction referenced in the PSW. E.g.
+         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
+         * of EXECUTE, while per_address contains the target of EXECUTE.
+         */
+        ilen = get_ilen(cpu_ldub_code(env, env->per_address));

Quoted 8 lines

target/s390x/misc_helper.c void HELPER(chsc)(CPUS390XState *env, uint64_t inst) · line 462

Quoted 4 lines

target/s390x/misc_helper.c +1 -0

+        program_interrupt(env, PGM_PER, ilen);
     }
 }
 

Wrapping my head around it: You preserve the current behavior, which should probably be changed to use ILEN_AUTO to handle cases like EXECUTE correctly?

Exactly, e.g. if EXECUTE executes instruction Y, and Y is to generate an IFETCH event, AFAIK

a) per_address points at Y
b) old PGM PSW points at instruction after EXECUTE (ignoring nullifying for now)
c) PGM ilen has to match EXECUTE (so the PSW can properly be rewound)

The case where per_address == (PGM PSW - ilen) (ignoring nullification) should happen without EXECUTE being involved. That's why PER currently works just fine with Linux (e.g. uprobe smoke tests when kernel boots up).

Execute handling with PER is just nasty. And one first has to find out how that plays together with EXECUTE handling in TCG. Therefore, no easy fix (although ILEN_AUTO might most probably really be the right thing to do).

Quoted 4 lines

Do you plan to do a follow up? (Should I spend some cycles on it?)

Somewhere on my list. The next thing (in this area) I want do is write kvm-unit-tests for PER, because also KVM could benefit from that (EXECUTE handling is just nasty).

So sure, go ahead and have a look at it if you have some spare cycles :)

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.

Signed-off-by David Hildenbrand <[email protected]>

---

 hw/s390x/s390-virtio-ccw.c | 7 +++++++
 target/s390x/cpu.h         | 2 ++
 2 files changed, 9 insertions(+)

hw/s390x/s390-virtio-ccw.c +7 -0

index f67b4b5d58..f7ca20d77a 100644
@@ -23,6 +23,7 @@
 #include "hw/s390x/css.h"
 #include "virtio-ccw.h"
 #include "qemu/config-file.h"
+#include "qemu/error-report.h"
 #include "s390-pci-bus.h"
 #include "hw/s390x/storage-keys.h"
 #include "hw/s390x/storage-attributes.h"

@@ -55,6 +56,12 @@ static void s390_init_cpus(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = s390_default_cpu_model_name();
     }
+    if (tcg_enabled() && max_cpus > S390_TCG_MAX_CPUS) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (%d) on s390x", max_cpus,
+                     S390_TCG_MAX_CPUS);
+        exit(1);
+    }
 
     cpu_states = g_new0(S390CPU *, max_cpus);
 

target/s390x/cpu.h +2 -0

index 147aceba28..dca6aa9aae 100644
@@ -209,6 +209,8 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
 
 #define ENV_OFFSET offsetof(S390CPU, env)
 
+#define S390_TCG_MAX_CPUS 1
+
 #ifndef CONFIG_USER_ONLY
 extern const struct VMStateDescription vmstate_s390_cpu;
 #endif

Quoted 7 lines · David Hildenbrand

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.

Quoted 2 lines

Signed-off-by David Hildenbrand <[email protected]>

---

Makes sense. Ran the described environment without this patch (errors) and again with this patch (graceful exit w/ message).

Tested-by Matthew Rosato <[email protected]>

Quoted 11 lines · Matthew Rosato

Quoted 9 lines · David Hildenbrand

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 4 lines

Makes sense. Ran the described environment without this patch (errors) and again with this patch (graceful exit w/ message).

Tested-by Matthew Rosato <[email protected]>

Can someone review

http://patchwork.ozlabs.org/patch/760010/

which does at least start to add the SIGP support.

Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't see the point of the define.

r~

Quoted 12 lines · Richard Henderson

Quoted 11 lines · Matthew Rosato

Quoted 9 lines · David Hildenbrand

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 9 lines

Quoted 4 lines

Makes sense. Ran the described environment without this patch (errors) and again with this patch (graceful exit w/ message).

Tested-by Matthew Rosato <[email protected]>

Can someone review

http://patchwork.ozlabs.org/patch/760010/

Yes, we also discovered that patch during review of v1.

Quoted 5 lines

which does at least start to add the SIGP support.

Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't see the point of the define.

Conny requested a define. So it boils down to

a) no define just as in v1
b) a define like S390_TCG_SMP_SUPPORTED

What do you suggest?

Quoted 4 lines

r~

Quoted 14 lines · Matthew Rosato

Quoted 9 lines · David Hildenbrand

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.

Signed-off-by David Hildenbrand <[email protected]>

---

Makes sense. Ran the described environment without this patch (errors) and again with this patch (graceful exit w/ message).

Quoted 1 line

Tested-by Matthew Rosato <[email protected]>

Can someone review

http://patchwork.ozlabs.org/patch/760010/

which does at least start to add the SIGP support.

Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't see the point of the define.

r~

FWIW, I started factoring out today KVM SIGP code to make it usable by TCG.

I also started adding the missing SIGP instructions the kernel handles for KVM. I dropped the old TCG SIGP handling code and completely reuse the new SIGP code. I already got boot/reboot/shutdown properly running (implementing STOP and RESTART interrupts like KVM has).

But its still quite hacky and there are is a bunch of stuff to clean up, especially:
- external interrupt handling (the queue approach we have right now is no good for external calls and emergency signals)
- floating interrupt support (io interrupts always going to CPU 0 is a hack)

I think I can at least implement SIGP properly and fix the external call stuff. floating interrupts might require more thought.

scc->next_cpu_id is updated when realizing. Setting it just before that point looks cleaner.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

target/s390x/cpu.c +4 -7

index 74b3e4fd0d..5f9315fb16 100644
@@ -194,7 +194,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    ", max allowed: %d", cpu->id, max_cpus - 1);
         goto out;
     }
+#else
+    /* implicitly set for linux-user only */
+    cpu->id = scc->next_cpu_id;
 #endif
+
     if (cpu_exists(cpu->id)) {
         error_setg(&err, "Unable to add CPU: %" PRIi64
                    ", it already exists", cpu->id);

@@ -306,13 +310,6 @@ static void s390_cpu_initfn(Object *obj)
         inited = true;
         s390x_translate_init();
     }
-
-#if defined(CONFIG_USER_ONLY)
-    {
-        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
-        cpu->id = scc->next_cpu_id;
-    }
-#endif
 }
 
 static void s390_cpu_finalize(Object *obj)

Some time ago we discussed that using "id" as property name is not the right thing to do, as it is a reserved property for other devices and will not work with device_add.

Switch to the term "core-id" instead, and use it as an equivalent to "CPU address" mentioned in the PoP. There is no such thing as cpu number, so rename env.cpu_num to env.core_id. We use "core-id" as this is the common term to use for device_add later on (x86 and ppc).

We can get rid of cpu->id now. Keep cpu_index and env->core_id in sync. cpu_index was already implicitly used by e.g. cpu_exists(), so keeping both in sync seems to be the right thing to do.

cpu_index will now no longer automatically get set via cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed in sync.>

Our new cpu property "core-id" can be a static property. Range checks can be avoided by using the correct type and the "setting after realized" check is done implicitly.

device_add will later need the reserved "id" property. Hotplugging a CPU on s390x will then be: "device_add host-s390-cpu,id=cpu2,core-id=2".

Signed-off-by David Hildenbrand <[email protected]>

---

target/s390x/translate.c +0 -0

index 4b0db7b7bd..b8963f2fe2 100644
@@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)
 static ExitStatus op_stap(DisasContext *s, DisasOps *o)

Quoted 16 lines

target/s390x/translate.c +1 -4

index 4b0db7b7bd..b8963f2fe2 100644
@@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)
 static ExitStatus op_stap(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    /* ??? Surely cpu address != cpu number.  In any case the previous
-       version of this stored more than the required half-word, so it
-       is unlikely this has ever been tested.  */
-    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
+    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));
     return NO_EXIT;
 }

Quoted 1 line

Are you sure it's OK to remove this blurb in its entirety? You are certainly collapsing the various CPU identifiers, but you aren't changing the size of the store from when this blurb was put in (411fea3d) So, "the previous version of this stored more than the required half-word" seems to be still relevant -- Unless you've gone ahead and tested it out?

Outside of that nit, I like the changes.

Reviewed-by Matthew Rosato <[email protected]>

Adapt to the new term "core_id". While at it, fix the type and drop the initialization to 0 (which is superfluous).

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

target/s390x/cpu-qom.h +1 -1

index c8fbf8ae03..21ea15e642 100644
@@ -52,7 +52,7 @@ typedef struct S390CPUClass {
     bool is_migration_safe;
     const char *desc;
 
-    int64_t next_cpu_id;
+    uint32_t next_core_id;
 
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);

target/s390x/cpu.c +5 -6

index a2570bbc6b..105ff13034 100644
@@ -197,7 +197,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #else
     /* implicitly set for linux-user only */
-    cpu->env.core_id = scc->next_cpu_id;
+    cpu->env.core_id = scc->next_core_id;
 #endif
 
     if (cpu_exists(cpu->env.core_id)) {

@@ -205,10 +205,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    cpu->env.core_id);
         goto out;
     }
-    if (cpu->env.core_id != scc->next_cpu_id) {
+    if (cpu->env.core_id != scc->next_core_id) {
         error_setg(&err, "Unable to add CPU: %" PRIu32
-                   ", the next available nr is %" PRIi64, cpu->env.core_id,
-                   scc->next_cpu_id);
+                   ", the next available id is %" PRIu32, cpu->env.core_id,
+                   scc->next_core_id);
         goto out;
     }
 

@@ -218,7 +218,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     if (err != NULL) {
         goto out;
     }
-    scc->next_cpu_id++;
+    scc->next_core_id++;
 
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);

@@ -463,7 +463,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(scc);
     DeviceClass *dc = DEVICE_CLASS(oc);
 
-    scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;

Other architectures provide nicely sorted lists, let's do it similarly on s390x.

While at it, clean up the code we have to touch either way.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu_models.c | 63 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 22 deletions(-)

target/s390x/cpu_models.c +1 -16

index 8e20e7637b..092db34258 100644
@@ -270,41 +270,60 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
     return last_compatible;
 }
 
-struct S390PrintCpuListInfo {
-    FILE *f;
-    fprintf_function print;
-};
-
-static void print_cpu_model_list(ObjectClass *klass, void *opaque)
+static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data)
 {
-    struct S390PrintCpuListInfo *info = opaque;
-    S390CPUClass *scc = S390_CPU_CLASS(klass);
-    char *name = g_strdup(object_class_get_name(klass));
-    const char *details = "";
-
-    if (scc->is_static) {
-        details = "(static, migration-safe)";
-    } else if (scc->is_migration_safe) {
-        details = "(migration-safe)";
-    }

target/s390x/cpu_models.c +7 -0

     g_free(name);
 }
 
+static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    const S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *)a);
+    const S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *)b);
+    const char *name_a = object_class_get_name((ObjectClass *)a);
+    const char *name_b = object_class_get_name((ObjectClass *)b);
+

target/s390x/cpu_models.c +13 -3

+
+    /* keep the same order we have in our table (sorted by release date) */
+    if (cc_a->cpu_def != cc_b->cpu_def) {
+        return cc_a->cpu_def - cc_b->cpu_def;
+    }
+
+    /* exact same definition - list base model first */
+    return cc_a->is_static ? -1 : 1;
+}
+
 void s390_cpu_list(FILE *f, fprintf_function print)
 {
-    struct S390PrintCpuListInfo info = {
-        .f = f,
-        .print = print,
+    CPUListState s = {
+        .file = f,
+        .cpu_fprintf = print,
     };
     S390FeatGroup group;
     S390Feat feat;

Quoted 5 lines

target/s390x/cpu_models.c +3 -0

+    CPUListState *s = user_data;
+    const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data);
+    char *name = g_strdup(object_class_get_name((ObjectClass *)data));

target/s390x/cpu_models.c const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, · line 282

Quoted 6 lines

     /* strip off the -s390-cpu */
     g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;

target/s390x/cpu_models.c +2 -2

-    (*info->print)(info->f, "s390 %-15s %-35s %s\n", name, scc->desc,
-                   details);
+    (*s->cpu_fprintf)(s->file, "s390 %-15s %-35s (%smigration-safe)\n", name,
+                      scc->desc, scc->is_static ? "static, " : "");

Hm, that now prints 's390 host' as 'migration safe'...

Quoted 16 lines

     g_free(name);

}

target/s390x/cpu_models.c +12 -0

+static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    const S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *)a);
+    const S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *)b);
+    const char *name_a = object_class_get_name((ObjectClass *)a);
+    const char *name_b = object_class_get_name((ObjectClass *)b);
+
+    /* move qemu and host to the top of the list, qemu first, host second */
+    if (name_a[0] == 'q') {
+        return -1;
+    } else if (name_b[0] == 'q') {
+        return 1;

target/s390x/cpu_models.c const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, · line 302

Quoted 5 lines

target/s390x/cpu_models.c +5 -0

+    } else if (name_a[0] == 'h') {
+        return -1;
+    } else if (name_b[0] == 'h') {
+        return 1;
+    }

Are we sure that there will never be models starting with q or h? But we can worry about that later.

E.g. the following now works:

    device_add host-s390-cpu,id=cpu1,core-id=1

The system will perform the same checks as when using cpu_add:
- If the core_id is already in use
- If the next sequential core_id isn't used
- If core-id >= max_cpu is specified

In addition, mixed CPU models are checked. E.g. if starting with -cpu host and trying to hotplug "qemu-s390-cpu":

    "Mixed CPU models are not supported on s390x."

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c | 1 +
 1 file changed, 1 insertion(+)

target/s390x/cpu.c +1 -0

index 105ff13034..be20c2fb0f 100644
@@ -466,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
+    dc->user_creatable = true;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)

Quoted 8 lines · David Hildenbrand

E.g. the following now works:

    device_add host-s390-cpu,id=cpu1,core-id=1

The system will perform the same checks as when using cpu_add:
- If the core_id is already in use
- If the next sequential core_id isn't used
- If core-id >= max_cpu is specified

Quoted 5 lines

In addition, mixed CPU models are checked. E.g. if starting with -cpu host and trying to hotplug "qemu-s390-cpu":

    "Mixed CPU models are not supported on s390x."

Signed-off-by David Hildenbrand <[email protected]>

As easy as flipping a switch, right?

Tested the new device_add path, the old cpu-add path and mixed cases where I alternated adding cpus via both methods -- looking good.

Reviewed-by Matthew Rosato <[email protected]>

device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

hw/s390x/s390-virtio-ccw.c +10 -0

index 22a8a1b45d..dd149567bb 100644
@@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
+                                               DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        error_setg(errp, "CPU hot unplug not supported on this machine");
+        return;
+    }
+}
+
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {

@@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 248;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
     hc->plug = s390_machine_device_plug;
+    hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
 }
 

Quoted 7 lines · David Hildenbrand

device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Given the fact that I get the question about unplug _every_ time when I give a presentation about KVM on z, I will try to get some architecture folks look at this. Maybe we can define something very simple like "if the CPU is in the stopped state we can remove this and just piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".

So maybe add "currently"

Quoted 14 lines · Christian Borntraeger

Quoted 7 lines · David Hildenbrand

device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Given the fact that I get the question about unplug _every_ time when I give a presentation about KVM on z, I will try to get some architecture folks look at this. Maybe we can define something very simple like "if the CPU is in the stopped state we can remove this and just piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".

Quoted 1 line

So maybe add "currently"

Unfortunately it might not be that easy.

We would have to find a way that existing OS's don't break. If a guest OS is not prepared for CPUs to get removed, we might run into inconsistencies when simply deleting CPUs that are in the STOPPED state.

Especially, these CPUs would still show up in the guest as "offline". Wonder what would then happen trying to "online" these.

But yes, I can add "currently".

Quoted 15 lines · David Hildenbrand

Quoted 14 lines · Christian Borntraeger

Quoted 7 lines · David Hildenbrand

device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Given the fact that I get the question about unplug _every_ time when I give a presentation about KVM on z, I will try to get some architecture folks look at this. Maybe we can define something very simple like "if the CPU is in the stopped state we can remove this and just piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".

Quoted 7 lines

Quoted 1 line

So maybe add "currently"

Unfortunately it might not be that easy.

We would have to find a way that existing OS's don't break. If a guest OS is not prepared for CPUs to get removed, we might run into inconsistencies when simply deleting CPUs that are in the STOPPED state.

Yes, this needs to be validated across all things.

Quoted 3 lines

Especially, these CPUs would still show up in the guest as "offline". Wonder what would then happen trying to "online" these.

The main use case seems to be, that the admin does not want to allow a guest to online back a guest CPU if it was taken away from the configuration. So maybe we could simply fail a SIGP START for those.

Or we might go one level below and only allow an unplug if the CPU is in the deconfigured state and we would then have to forbid the configuration step.

Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in QEMU.

Anyway, we need some architecture agreement here first (something that is also ok with LPAR and z/VM).

Quoted 1 line

But yes, I can add "currently".

Having the cpu in the unconfigured state as a requirement also makes this less likely to break for older OSs, I guess.

Quoted 2 lines

Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in QEMU.

Sounds like something we'd like to have in the future?

(Btw, what does cpuplugd trigger? Offline/online or deconfigure/configure?)

device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

hw/s390x/s390-virtio-ccw.c +10 -0

index 22a8a1b45d..dd149567bb 100644
@@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
+                                               DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        error_setg(errp, "CPU hot unplug not supported on this machine");
+        return;
+    }
+}
+
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {

@@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 248;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
     hc->plug = s390_machine_device_plug;
+    hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
 }
 

CPU hotplug is only possible on a per core basis on s390x.

As we now have ms->possible_cpus, we can get rid of the global variable cpu_states.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 90 +++++++++++++++++++++++++++++++++-------------
 qapi-schema.json           | 16 +++++++++
 2 files changed, 81 insertions(+), 25 deletions(-)

Now that there is only one user of cpu_s390x_create() left, make cpu creation looks like on x86.
- Perform the model/properties split and checks in s390_init_cpus()
- Parse features only once without having to remember if already parsed
- Pass only the typename to s390x_new_cpu()
- Use the typename of an existing CPU for hotplug via cpu-add

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 29 +++++++++++++++++++++++++++--
 target/s390x/cpu.h         |  2 +-
 target/s390x/helper.c      | 45 ++-------------------------------------------
 target/s390x/internal.h    |  1 -
 4 files changed, 30 insertions(+), 47 deletions(-)

The first patches are a bunch of cleanups. I decided to go the extra mile and implement CPU hotplug via "device_add", as well as "query-hotpluggable-cpus". So the last patches in this series differ to v1. I tried to stick as much as possible to x86.

On s390x, only complete cores can be plugged. Cores have to be plugged in the right order (core-id 0, 1, 2 ...). CPU hot unplug is not supported by the architecture.

In addition, we now also print the CPU model list in sorted order and allow only 1 CPU for TCG.

v1 -> v2:
- "exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly" -- added a few instances I missed
- "s390x: get rid of s390-virtio.c"
-- move everything in the first shot and clean the "cpu_states" stuff up
in later patches, when we have ms->possible_cpus.
- Added "target/s390x: move typedef of S390CPU to its definition"
- Added "s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h"
- Added "s390x: move two function declarations to s390-virtio-ccw.h"
- Added "s390x: move sclp_service_call() to interrupt.c"
- "target/s390x: use program_interrupt() in per_check_exception()" -- Add a comment describing my concern
- "s390x: allow only 1 CPU with TCG"
-- Add a #define to easily be able to change it (e.g. for development)
- Added "target/s390x: set cpu->id for linux user when realizing"
- "target/s390x: use "core-id" for cpu number/address/id handling" -- I now use "core-id" as property name, because that will be used by device_add
- "target/s390x: rename next_cpu_id to next_core_id"
-- Also change to the term "core_id"
- Added "s390x: print CPU definitions in sorted order"
- Added "s390x: allow cpu hotplug via device_add"
- Added "s390x: CPU hot unplug via device_del cannot work"
- Added "s390x: implement query-hotpluggable-cpu"
- Added "s390x: get rid of cpu_s390x_create()"

David Hildenbrand (19):
exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly
cpu: drop old comments describing members
s390x: get rid of s390-virtio.c
s390x: rename s390-virtio.h to s390-virtio-hcall.h target/s390x: move typedef of S390CPU to its definition
s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
s390x: move two function declarations to s390-virtio-ccw.h
s390x: move sclp_service_call() to interrupt.c target/s390x: use trigger_pgm_exception() in

    s390_cpu_handle_mmu_fault()

target/s390x: use program_interrupt() in per_check_exception()
s390x: allow only 1 CPU with TCG
target/s390x: set cpu->id for linux user when realizing target/s390x: use "core-id" for cpu number/address/id handling target/s390x: rename next_cpu_id to next_core_id
s390x: print CPU definitions in sorted order
s390x: allow cpu hotplug via device_add
s390x: CPU hot unplug via device_del cannot work
s390x: implement query-hotpluggable-cpus
s390x: get rid of cpu_s390x_create()

 dump.c                             |   1 -
 exec.c                             |   2 -
 hw/s390x/Makefile.objs             |   1 -
 hw/s390x/s390-virtio-ccw.c         | 259 +++++++++++++++++++++++++++++++++++--
 hw/s390x/s390-virtio-hcall.c       |   2 +-
 hw/s390x/s390-virtio-hcall.h       |  21 +++
 hw/s390x/s390-virtio.c             |  37 ------
 hw/s390x/s390-virtio.h             |  35 -----
 hw/s390x/sclp.c                    |  51 +-------
 include/hw/s390x/s390-virtio-ccw.h |   3 +
 include/hw/s390x/sclp.h            |   1 +
 include/qom/cpu.h                  |   6 +-
 qapi-schema.json                   |  16 +++
 target/i386/arch_dump.c            |   1 -
 target/i386/arch_memory_mapping.c  |   1 -
 target/i386/svm_helper.c           |   1 -
 target/ppc/arch_dump.c             |   1 -
 target/s390x/arch_dump.c           |   1 -
 target/s390x/cpu-qom.h             |   4 +-
 target/s390x/cpu.c                 |  86 ++++--------
 target/s390x/cpu.h                 |  19 +--
 target/s390x/cpu_models.c          |  65 ++++++----
 target/s390x/diag.c                |   1 +
 target/s390x/excp_helper.c         |   5 +-
 target/s390x/helper.c              |  47 +------
 target/s390x/internal.h            |   1 -
 target/s390x/interrupt.c           |  52 ++++++++
 target/s390x/kvm.c                 |   3 +-
 target/s390x/misc_helper.c         |  22 ++--
 target/s390x/translate.c           |   5 +-
 30 files changed, 443 insertions(+), 307 deletions(-)
 create mode 100644 hw/s390x/s390-virtio-hcall.h
 delete mode 100644 hw/s390x/s390-virtio.h

I think we certainly want to have device_add as well (as long as the old interface stays for a while).

Adding Matt. Can you have a look at the cpu hotplug bits?

Quoted 4 lines · Christian Borntraeger

I think we certainly want to have device_add as well (as long as the old interface stays for a while).

Adding Matt. Can you have a look at the cpu hotplug bits?

If there is no further feedback, I'll be sending a new version out tomorrow, including the suggested changes. In addition, I'll be including support to hotplug VCPUs in random order (2 additional patches).