s390x cleanups and CPU hotplug via device_add

It is a leftover from the days where we had still the !ccw virtio machine. As this one is long gone, let's move everything to> s390-virtio-ccw.c.

Suggested-by Cornelia Huck <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Acked-by Christian Borntraeger <[email protected]>

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

---
 hw/s390x/Makefile.objs     |   1 -
 hw/s390x/s390-virtio-ccw.c | 157 ++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     |  37 -----------
 hw/s390x/s390-virtio.h     |  15 -----
 4 files changed, 156 insertions(+), 54 deletions(-)

hw/s390x/Makefile.objs +0 -1

index 7ee19d3abc..dc704b57d6 100644
@@ -1,4 +1,3 @@
-obj-y += s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o

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

index dd504dd5ae..4c3a3e177b 100644
@@ -2,6 +2,7 @@
  * virtio ccw machine
  *
  * Copyright 2012 IBM Corp.
+ * Copyright (c) 2009 Alexander Graf <[email protected]>
  * Author(s): Cornelia Huck <[email protected]>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or (at

@@ -31,6 +32,46 @@
 #include "hw/s390x/css-bridge.h"
 #include "migration/register.h"
 #include "cpu_models.h"
+#include "qapi/qmp/qerror.h"
+#include "hw/nmi.h"
+
+static S390CPU **cpu_states;
+
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
+
+    /* Fast lookup via CPU ID */
+    return cpu_states[cpu_addr];
+}
+
+static void s390_init_cpus(MachineState *machine)
+{
+    int i;
+    gchar *name;
+
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = s390_default_cpu_model_name();
+    }
+
+    cpu_states = g_new0(S390CPU *, max_cpus);
+
+    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);
+    }
+
+    for (i = 0; i < smp_cpus; i++) {
+        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
+    }
+}
 
 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,

@@ -94,7 +135,7 @@ static void virtio_ccw_register_hcalls(void)
                                    virtio_ccw_hcall_early_printk);
 }
 
-void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(ram_addr_t mem_size)
 {
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);

@@ -109,11 +150,103 @@ void s390_memory_init(ram_addr_t mem_size)
     s390_stattrib_init();
 }
 
+#define S390_TOD_CLOCK_VALUE_MISSING    0x00
+#define S390_TOD_CLOCK_VALUE_PRESENT    0x01
+
+static void gtod_save(QEMUFile *f, void *opaque)
+{
+    uint64_t tod_low;
+    uint8_t tod_high;
+    int r;
+
+    r = s390_get_clock(&tod_high, &tod_low);
+    if (r) {
+        fprintf(stderr, "WARNING: Unable to get guest clock for migration. "
+                        "Error code %d. Guest clock will not be migrated "
+                        "which could cause the guest to hang.\n", r);
+        qemu_put_byte(f, S390_TOD_CLOCK_VALUE_MISSING);
+        return;
+    }
+
+    qemu_put_byte(f, S390_TOD_CLOCK_VALUE_PRESENT);
+    qemu_put_byte(f, tod_high);
+    qemu_put_be64(f, tod_low);
+}
+
+static int gtod_load(QEMUFile *f, void *opaque, int version_id)
+{
+    uint64_t tod_low;
+    uint8_t tod_high;
+    int r;
+
+    if (qemu_get_byte(f) == S390_TOD_CLOCK_VALUE_MISSING) {
+        fprintf(stderr, "WARNING: Guest clock was not migrated. This could "
+                        "cause the guest to hang.\n");
+        return 0;
+    }
+
+    tod_high = qemu_get_byte(f);
+    tod_low = qemu_get_be64(f);
+
+    r = s390_set_clock(&tod_high, &tod_low);
+    if (r) {
+        fprintf(stderr, "WARNING: Unable to set guest clock value. "
+                        "s390_get_clock returned error %d. This could cause "
+                        "the guest to hang.\n", r);
+    }
+
+    return 0;
+}
+
 static SaveVMHandlers savevm_gtod = {
     .save_state = gtod_save,
     .load_state = gtod_load,
 };
 
+static void s390_init_ipl_dev(const char *kernel_filename,
+                              const char *kernel_cmdline,
+                              const char *initrd_filename, const char *firmware,
+                              const char *netboot_fw, bool enforce_bios)
+{
+    Object *new = object_new(TYPE_S390_IPL);
+    DeviceState *dev = DEVICE(new);
+
+    if (kernel_filename) {
+        qdev_prop_set_string(dev, "kernel", kernel_filename);
+    }
+    if (initrd_filename) {
+        qdev_prop_set_string(dev, "initrd", initrd_filename);
+    }
+    qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
+    qdev_prop_set_string(dev, "firmware", firmware);
+    qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
+    qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
+    object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
+                              new, NULL);
+    object_unref(new);
+    qdev_init_nofail(dev);
+}
+
+static void s390_create_virtio_net(BusState *bus, const char *name)
+{
+    int i;
+
+    for (i = 0; i < nb_nics; i++) {
+        NICInfo *nd = &nd_table[i];
+        DeviceState *dev;
+
+        if (!nd->model) {
+            nd->model = g_strdup("virtio");
+        }
+
+        qemu_check_nic_model(nd, "virtio");
+
+        dev = qdev_create(bus, name);
+        qdev_set_nic_properties(dev, nd);
+        qdev_init_nofail(dev);
+    }
+}
+
 static void ccw_init(MachineState *machine)
 {
     int ret;

@@ -177,6 +310,19 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     g_free(name);
 }
 
+static void s390_machine_reset(void)
+{
+    S390CPU *ipl_cpu = S390_CPU(qemu_get_cpu(0));
+
+    s390_cmma_reset();
+    qemu_devices_reset();
+    s390_crypto_reset();
+
+    /* all cpus are stopped - configure and start the ipl cpu only */
+    s390_ipl_prepare_cpu(ipl_cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+}
+
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {

@@ -201,6 +347,15 @@ static void s390_hot_add_cpu(const int64_t id, Error **errp)
     s390x_new_cpu(machine->cpu_model, id, errp);
 }
 
+static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
+{
+    CPUState *cs = qemu_get_cpu(cpu_index);
+
+    if (s390_cpu_restart(S390_CPU(cs))) {
+        error_setg(errp, QERR_UNSUPPORTED);
+    }
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);

hw/s390x/s390-virtio.c +0 -37

index da3f49e80e..9316be1cfc 100644
@@ -48,17 +48,6 @@
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **cpu_states;
-
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
-{
-    if (cpu_addr >= max_cpus) {
-        return NULL;
-    }
-
-    /* Fast lookup via CPU ID */
-    return cpu_states[cpu_addr];
-}
 
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,

@@ -86,32 +75,6 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(MachineState *machine)
-{
-    int i;
-    gchar *name;
-
-    if (machine->cpu_model == NULL) {
-        machine->cpu_model = s390_default_cpu_model_name();
-    }
-
-    cpu_states = g_new0(S390CPU *, max_cpus);
-
-    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);
-    }
-
-    for (i = 0; i < smp_cpus; i++) {
-        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
-    }
-}
-
 
 void s390_create_virtio_net(BusState *bus, const char *name)
 {

hw/s390x/s390-virtio.h +0 -15

index ca97fd6814..d984cd4115 100644
@@ -12,24 +12,9 @@
 #ifndef HW_S390_VIRTIO_H
 #define HW_S390_VIRTIO_H
 
-#include "hw/nmi.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);
-
-void s390_init_cpus(MachineState *machine);
-void s390_init_ipl_dev(const char *kernel_filename,
-                       const char *kernel_cmdline,
-                       const char *initrd_filename,
-                       const char *firmware,
-                       const char *netboot_fw,
-                       bool enforce_bios);
-void s390_create_virtio_net(BusState *bus, const char *name);
-void s390_nmi(NMIState *n, int cpu_index, Error **errp);
-void s390_machine_reset(void);
-void s390_memory_init(ram_addr_t mem_size);
-void gtod_save(QEMUFile *f, void *opaque);
-int gtod_load(QEMUFile *f, void *opaque, int version_id);
 #endif

Quoted 2 lines · David Hildenbrand

It is a leftover from the days where we had still the !ccw virtio machine. As this one is long gone, let's move everything to> s390-virtio-ccw.c.

Whoops, another thing to fix up:

hw/s390x/s390-virtio.c is not compiled any more but not completely deleted.

Quoted 12 lines

Suggested-by Cornelia Huck <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Acked-by Christian Borntraeger <[email protected]>

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

---
 hw/s390x/Makefile.objs     |   1 -
 hw/s390x/s390-virtio-ccw.c | 157 ++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     |  37 -----------
 hw/s390x/s390-virtio.h     |  15 -----
 4 files changed, 156 insertions(+), 54 deletions(-)

This allows us to drop inclusion of cpu_models.h in cpu-qom.h, and prepares for using cpu-qom.h as a s390 specific version of typedefs.h

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

---
 target/s390x/cpu-qom.h    | 5 ++++-
 target/s390x/cpu.h        | 5 +++--
 target/s390x/cpu_models.h | 8 ++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

target/s390x/cpu.h +1 -1

 #define TARGET_LONG_BITS 64
 

@@ -80,7 +81,7 @@ typedef struct MchkQueue {
     uint16_t type;
 } MchkQueue;
 
-typedef struct CPUS390XState {
+struct CPUS390XState {
     uint64_t regs[16];     /* GP registers */
     /*
      * The floating point registers are part of the vector registers.

Quoted 9 lines · David Hildenbrand

This allows us to drop inclusion of cpu_models.h in cpu-qom.h, and prepares for using cpu-qom.h as a s390 specific version of typedefs.h

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

---
 target/s390x/cpu-qom.h    | 5 ++++-
 target/s390x/cpu.h        | 5 +++--
 target/s390x/cpu_models.h | 8 ++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

[...]

Quoted 6 lines

target/s390x/cpu.h +0 -0

index 372f7d8885..4a0a59dfbc 100644
@@ -25,6 +25,7 @@
 

target/s390x/cpu.h line 28

Quoted 3 lines

target/s390x/cpu.h +1 -0

 #include "qemu-common.h"
 #include "cpu-qom.h"
+#include "cpu_models.h"

Do we really need to include cpu_models.h here? Looking at your changes below, it does not seem to be necessary?

Quoted 17 lines · Thomas Huth

Quoted 9 lines · David Hildenbrand

This allows us to drop inclusion of cpu_models.h in cpu-qom.h, and prepares for using cpu-qom.h as a s390 specific version of typedefs.h

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

---
 target/s390x/cpu-qom.h    | 5 ++++-
 target/s390x/cpu.h        | 5 +++--
 target/s390x/cpu_models.h | 8 ++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

[...]

Quoted 6 lines

target/s390x/cpu.h +0 -0

index 372f7d8885..4a0a59dfbc 100644
@@ -25,6 +25,7 @@
 

Quoted 6 lines

target/s390x/cpu.h line 28

Quoted 3 lines

target/s390x/cpu.h +1 -0

 #include "qemu-common.h"
 #include "cpu-qom.h"
+#include "cpu_models.h"

Do we really need to include cpu_models.h here? Looking at your changes below, it does not seem to be necessary?

Yes, due to s390_has_feat(), and I want to avoid cleaning all the other calling places up in this patch.

Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of struct CPUS390XState to fix the two sclp consoles complaining.

include/hw/s390x/sclp.h +2 -0

index a72d096081..847ff32f85 100644
@@ -16,6 +16,7 @@
 
 #include "hw/sysbus.h"
 #include "hw/qdev.h"
+#include "target/s390x/cpu-qom.h"
 
 #define SCLP_CMD_CODE_MASK                      0xffff00ff
 

@@ -242,5 +243,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void);
 sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void);
 void sclp_service_interrupt(uint32_t sccb);
 void raise_irq_cpu_hotplug(void);
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif

target/s390x/cpu.h +0 -1

index 4a0a59dfbc..5295bd3c66 100644
@@ -722,6 +722,5 @@ 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);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif

target/s390x/misc_helper.c +1 -0

index b142db71c6..8b07535b02 100644
@@ -35,6 +35,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ebcdic.h"
 #include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 /* #define DEBUG_HELPER */

Quoted 3 lines · David Hildenbrand

Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of struct CPUS390XState to fix the two sclp consoles complaining.

Is that sentence about the forward declaration still valid? I did not see that anymore here...

Apart from that:

Reviewed-by Thomas Huth <[email protected]>

Quoted 3 lines · David Hildenbrand

Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of struct CPUS390XState to fix the two sclp consoles complaining.

Is that sentence about the forward declaration still valid? I did not see that anymore here...

Apart from that:

Reviewed-by Thomas Huth <[email protected]>

Quoted 7 lines · On Wed, 13 Sep 2017 16:29:23 +0200 Thomas Huth <[email protected]> wrote:

Quoted 3 lines · David Hildenbrand

Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of struct CPUS390XState to fix the two sclp consoles complaining.

Is that sentence about the forward declaration still valid? I did not see that anymore here...

Indeed. I can fix this up to:

"Implemented in sclp.c, so let's move it to the right include file. Also adjust some includes."

?

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.

Tested-by Matthew Rosato <[email protected]>

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

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

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

index f67b4b5d58..417998ec28 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,11 @@ 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 > 1) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (1) on s390x", max_cpus);
+        exit(1);
+    }
 
     cpu_states = g_new0(S390CPU *, max_cpus);
 

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.

Tested-by Matthew Rosato <[email protected]>

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

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

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

index f67b4b5d58..417998ec28 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"

Quoted 4 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.

Quoted 2 lines

Let's bail out if more than 1 is specified, so we don't raise people's hope.

Why does this restriction exist? Without MTTCG enabled -smp > 1 should be safe from any races.

Quoted 7 lines · Alex Bennée

Quoted 4 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.

Quoted 5 lines

Quoted 2 lines

Let's bail out if more than 1 is specified, so we don't raise people's hope.

Why does this restriction exist? Without MTTCG enabled -smp > 1 should be safe from any races.

Because the actual SIGP code (instruction to start/stop ... CPUs) is not implemented yet.

Quoted 8 lines · David Hildenbrand

Quoted 7 lines · Alex Bennée

Quoted 4 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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Let's bail out if more than 1 is specified, so we don't raise people's hope.

Why does this restriction exist? Without MTTCG enabled -smp > 1 should be safe from any races.

Because the actual SIGP code (instruction to start/stop ... CPUs) is not implemented yet.

Ahh OK, I assume something like ARM's PCSI interface then.

When you do get around to implementing just ensure you use the async mechanism to initialise the target processor state to avoid races in MTTCG. Essentially you queue the work up on the target and then it is run before the powered up vCPU starts running code.

--
Alex Bennée

E.g. the following now works:

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

Quoted 2 lines · David Hildenbrand

E.g. the following now works:

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

Hi David,

FYI, this introduced a possibility to crash QEMU with the none machine:

$ s390x-softmmu/qemu-system-s390x -M none -nographic
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) device_add qemu-s390-cpu
Segmentation fault (core dumped)

Thomas

Quoted 2 lines · David Hildenbrand

E.g. the following now works:

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

Hi David,

FYI, this introduced a possibility to crash QEMU with the none machine:

$ s390x-softmmu/qemu-system-s390x -M none -nographic
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) device_add qemu-s390-cpu
Segmentation fault (core dumped)

Thomas

device_del on a CPU will currently do nothing. Let's emmit an error

telling that this is will currently not 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

Reviewed-by Matthew Rosato <[email protected]>

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

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

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

---
 hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json           | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

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

index 3be81d96af..cc64a81321 100644
@@ -50,6 +50,7 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 
 static void s390_init_cpus(MachineState *machine)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     int i;
     gchar *name;
 

@@ -74,6 +75,9 @@ static void s390_init_cpus(MachineState *machine)
         g_free(name);
     }
 
+    /* initialize possible_cpus */
+    mc->possible_cpu_arch_ids(machine);
+
     for (i = 0; i < smp_cpus; i++) {
         s390x_new_cpu(machine->cpu_model, i, &error_fatal);
     }

@@ -307,6 +311,7 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
     gchar *name;
+    MachineState *ms = MACHINE(hotplug_dev);
     S390CPU *cpu = S390_CPU(dev);
     CPUState *cs = CPU(dev);
 

@@ -314,6 +319,9 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
                              errp);
     g_free(name);
+
+    g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
+    ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
 }
 
 static void s390_machine_reset(void)

@@ -346,6 +354,36 @@ static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
     }
 }
 
+static CpuInstanceProperties s390_cpu_index_to_props(MachineState *machine,
+                                                     unsigned cpu_index)
+{
+    g_assert(machine->possible_cpus && cpu_index < machine->possible_cpus->len);
+
+    return machine->possible_cpus->cpus[cpu_index].props;
+}
+
+static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
+{
+    int i;
+
+    if (ms->possible_cpus) {
+        g_assert(ms->possible_cpus && ms->possible_cpus->len == max_cpus);
+        return ms->possible_cpus;
+    }
+
+    ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
+                                  sizeof(CPUArchId) * max_cpus);
+    ms->possible_cpus->len = max_cpus;
+    for (i = 0; i < ms->possible_cpus->len; i++) {
+        ms->possible_cpus->cpus[i].vcpus_count = 1;
+        ms->possible_cpus->cpus[i].arch_id = i;
+        ms->possible_cpus->cpus[i].props.has_core_id = true;
+        ms->possible_cpus->cpus[i].props.core_id = i;
+    }
+
+    return ms->possible_cpus;
+}
+
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {

@@ -393,7 +431,10 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 248;
+    mc->has_hotpluggable_cpus = true;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
+    mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
+    mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;

qapi-schema.json +16 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
 

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

---
 hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json           | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

Quoted 2 lines · On Wed, 13 Sep 2017 15:24:13 +0200 David Hildenbrand <[email protected]> wrote:

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

s/properly wire everything up/wire everything up properly/

?

(fixing while... you get the idea)

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

qapi-schema.json +8 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }

Quoted 4 lines · David Hildenbrand

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

[...]

Quoted 22 lines

qapi-schema.json +14 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }

qapi-schema.json line 3141

Quoted 4 lines

qapi-schema.json +2 -0

+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }  

Please stick in '# Example:' like we do in query-hotpluggable-cpus's doc comment. I expect the generated documentation to be illegible[*] without it.

[*] If you want to perform a quick eye-over, try running "make tests/qapi-schema/doc-good.test.txt" and examine the result.

Quoted 29 lines · On Mon, 02 Oct 2017 09:46:41 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 4 lines · David Hildenbrand

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

[...]

Quoted 22 lines

qapi-schema.json +14 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }

Quoted 8 lines

qapi-schema.json line 3141

Quoted 4 lines

qapi-schema.json +2 -0

+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }  

Please stick in '# Example:' like we do in query-hotpluggable-cpus's doc comment. I expect the generated documentation to be illegible[*] without it.

Should '# Example:' go before each of the examples (instead of the beginning of the example block), then?

Quoted 2 lines

[*] If you want to perform a quick eye-over, try running "make tests/qapi-schema/doc-good.test.txt" and examine the result.

Hmm... I might be missing something in my setup:

[cohuck@gondolin build]$ make tests/qapi-schema/doc-good.test.txt
GEN tests/qapi-schema/doc-good.test.txt
tests/qapi-schema/doc-good.test.texi:79: @subsection seen before @end deftp tests/qapi-schema/doc-good.test.texi:90: unmatched `@end deftp'
make: *** [Makefile:695: tests/qapi-schema/doc-good.test.txt] Error 1

Quoted 41 lines · Cornelia Huck

Quoted 37 lines · On Mon, 02 Oct 2017 09:46:41 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 4 lines · David Hildenbrand

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

[...]

Quoted 16 lines

qapi-schema.json +8 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }

qapi-schema.json line 3141

Quoted 10 lines

qapi-schema.json +8 -0

+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }    

Please stick in '# Example:' like we do in query-hotpluggable-cpus's doc comment. I expect the generated documentation to be illegible[*] without it.

Quoted 2 lines

Should '# Example:' go before each of the examples (instead of the beginning of the example block), then?

Whatever makes the generated documentation look better.

Quoted 43 lines · On Wed, 04 Oct 2017 14:42:55 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 41 lines · Cornelia Huck

Quoted 37 lines · On Mon, 02 Oct 2017 09:46:41 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 4 lines · David Hildenbrand

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

[...]

Quoted 16 lines

qapi-schema.json +8 -0

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }

qapi-schema.json line 3141

Quoted 10 lines

qapi-schema.json +8 -0

+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }    

Please stick in '# Example:' like we do in query-hotpluggable-cpus's doc comment. I expect the generated documentation to be illegible[*] without it.

Quoted 4 lines

Quoted 2 lines

Should '# Example:' go before each of the examples (instead of the beginning of the example block), then?

Whatever makes the generated documentation look better.

The pdf indeed looks ugly, but that's a preexisting problem, and adding more '# Example:' lines does not really help.

The basic issue is that the code expects just one example and no additional text for the '# Example:' tag -- the explaining text for the various statements ends up looking like the qmp examples. That should be improved before doing a patch on top for this command (and I don't really have the skills or time to improve the output, sorry.)

Quoted 57 lines · Cornelia Huck

Quoted 46 lines · On Wed, 04 Oct 2017 14:42:55 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 44 lines · Cornelia Huck

Quoted 40 lines · On Mon, 02 Oct 2017 09:46:41 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 4 lines · David Hildenbrand

CPU hotplug is only possible on a per core basis on s390x. So let's add possible_cpus and properly wire everything up.

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

[...]

Quoted 18 lines

qapi-schema.json +9 -1

index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2
-cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },

qapi-schema.json

Quoted 10 lines

qapi-schema.json +7 -0

+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns':
 ['HotpluggableCPU'] }

Please stick in '# Example:' like we do in query-hotpluggable-cpus's doc
comment. I expect the generated documentation to be illegible[*] without it.

Quoted 4 lines

Quoted 2 lines

Should '# Example:' go before each of the examples (instead of the beginning of the example block), then?

Whatever makes the generated documentation look better.

The pdf indeed looks ugly, but that's a preexisting problem, and adding more '# Example:' lines does not really help.

Quoted 5 lines

The basic issue is that the code expects just one example and no additional text for the '# Example:' tag -- the explaining text for the various statements ends up looking like the qmp examples. That should be improved before doing a patch on top for this command (and I don't really have the skills or time to improve the output, sorry.)

Marc-André, any ideas?

Now that we have possible_cpus, we can get rid of the global variable and rewrite s390_cpu_addr2state() to use it.

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

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

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

index cc64a81321..720f145054 100644
@@ -36,23 +36,26 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/nmi.h"
 
-static S390CPU **cpu_states;
-
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= max_cpus) {
-        return NULL;
+    static MachineState *ms;
+
+    if (!ms) {
+        ms = MACHINE(qdev_get_machine());
+        g_assert(ms->possible_cpus);
     }
 
-    /* Fast lookup via CPU ID */
-    return cpu_states[cpu_addr];
+    /* CPU address corresponds to the core_id and the index */
+    if (cpu_addr >= ms->possible_cpus->len) {
+        return NULL;
+    }
+    return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
 }
 
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     int i;
-    gchar *name;
 
     if (machine->cpu_model == NULL) {
         machine->cpu_model = s390_default_cpu_model_name();

@@ -63,18 +66,6 @@ static void s390_init_cpus(MachineState *machine)
         exit(1);
     }
 
-    cpu_states = g_new0(S390CPU *, max_cpus);
-
-    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);
-    }
-
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 

@@ -310,15 +301,8 @@ static void ccw_init(MachineState *machine)
 static void s390_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
-    gchar *name;
     MachineState *ms = MACHINE(hotplug_dev);
     S390CPU *cpu = S390_CPU(dev);
-    CPUState *cs = CPU(dev);
-
-    name = g_strdup_printf("cpu[%i]", cpu->env.core_id);
-    object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
-                             errp);
-    g_free(name);
 
     g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
     ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);

SCLP correctly indicates the core-id aka. CPU address for each available CPU.

As the core-id corresponds to cpu_index, also a newly created kvm vcpu gets assigned this core-id as vcpu id. So SIGP in the kernel works correctly (it uses the vcpu id to lookup the correct CPU).

So there should be nothing hindering us from hotplugging CPUs in random core-id order.

This now makes sure that the output from "query-hotpluggable-cpus" is completely true. Until now, a specific order is implicit. Performance vice, hotplugging CPUs in non-sequential order might not be the best thing to do, as VCPU lookup inside KVM might be a little slower. But that doesn't hinder us from supporting it.

next_core_id is now used by linux user only.

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

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

target/s390x/cpu.c +1 -7

index 5724dffb88..34538c3ab9 100644
@@ -199,6 +199,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #else
     /* implicitly set for linux-user only */
     cpu->env.core_id = scc->next_core_id;
+    scc->next_core_id++;
 #endif
 
     if (cpu_exists(cpu->env.core_id)) {

@@ -206,12 +207,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    ", it already exists", cpu->env.core_id);
         goto out;
     }
-    if (cpu->env.core_id != scc->next_core_id) {
-        error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
-                   ", the next available core-id is %" PRIu32, cpu->env.core_id,
-                   scc->next_core_id);
-        goto out;
-    }
 
     /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
     cs->cpu_index = env->core_id;

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

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".

On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.

Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places - 152e9f65d66f ("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.

Quoted 7 lines · On Wed, 13 Sep 2017 15:23:55 +0200 David Hildenbrand <[email protected]> wrote:

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".

On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.

Quoted 5 lines

Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places - 152e9f65d66f ("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.

Thanks, applied.

Quoted 16 lines · Cornelia Huck

Quoted 12 lines · On Wed, 13 Sep 2017 15:23:55 +0200 David Hildenbrand <[email protected]> wrote:

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".

On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.

Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places - 152e9f65d66f ("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.

Quoted 1 line

Thanks, applied.

Please squash in the one-liner fix I asked for in my (late!) review of PATCH 18.

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".

On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.

Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places - 152e9f65d66f ("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.