[Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[David Hildenbrand]

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.


v4 -> v5:
* Only moving code around, no conceptual changes.
* "s390x: allow only 1 CPU with TCG"
 - Drop the ifdef stuff again.
* "s390x: implement query-hotpluggable-cpus"
 - Don't move s390_cpu_addr2state(), we can clean that up later.
 - Moved dropping "cpu_states" to next patch.
 - Drop s390_find_cpu_slot().
* "s390x: get rid of cpu_states and use possible_cpus instead"
 - Added as requested by Igor.


David Hildenbrand (22):
  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
  s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
  s390x: move subsystem_reset() to s390-virtio-ccw.h
  target/s390x: move some s390x typedefs to cpu-qom.h
  s390x: move sclp_service_call() to sclp.h
  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 for now
  s390x: implement query-hotpluggable-cpus
  s390x: get rid of cpu_states and use possible_cpus instead
  s390x: get rid of cpu_s390x_create()
  s390x: generate sclp cpu information from possible_cpus
  s390x: allow CPU hotplug in random core-id order

 dump.c                             |   1 -
 exec.c                             |   1 -
 hw/s390x/Makefile.objs             |   1 -
 hw/s390x/s390-virtio-ccw.c         | 239 +++++++++++++++++++++++++++++++++++--
 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                    |  34 +++---
 include/hw/s390x/s390-virtio-ccw.h |   2 +
 include/hw/s390x/sclp.h            |   2 +
 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             |   7 +-
 target/s390x/cpu.c                 |  85 ++++---------
 target/s390x/cpu.h                 |  13 +-
 target/s390x/cpu_models.c          |  58 ++++++---
 target/s390x/cpu_models.h          |   8 +-
 target/s390x/diag.c                |   1 +
 target/s390x/excp_helper.c         |   5 +-
 target/s390x/helper.c              |  47 +-------
 target/s390x/internal.h            |   1 -
 target/s390x/kvm.c                 |   1 +
 target/s390x/misc_helper.c         |  21 ++--
 target/s390x/translate.c           |   5 +-
 30 files changed, 388 insertions(+), 266 deletions(-)
 create mode 100644 hw/s390x/s390-virtio-hcall.h
 delete mode 100644 hw/s390x/s390-virtio.h

-- 
2.13.5

[Qemu-devel] [PATCH v5 01/22] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[David Hildenbrand]

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

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 dump.c                            | 1 -
 exec.c                            | 1 -
 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, 7 deletions(-)

diff --git a/dump.c b/dump.c
index a79773d0f7..2ef6a678e8 100644
--- a/dump.c
+++ b/dump.c
@@ -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"
diff --git a/exec.c b/exec.c
index d20c34ca83..a25a4c6018 100644
--- a/exec.c
+++ b/exec.c
@@ -56,7 +56,6 @@
 #endif
 
 #endif
-#include "exec/cpu-all.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
 #include "translate-all.h"
diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
index e682904052..35b55fc200 100644
--- a/target/i386/arch_dump.c
+++ b/target/i386/arch_dump.c
@@ -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"
diff --git a/target/i386/arch_memory_mapping.c b/target/i386/arch_memory_mapping.c
index 647cff2829..271cb5e41b 100644
--- a/target/i386/arch_memory_mapping.c
+++ b/target/i386/arch_memory_mapping.c
@@ -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 */
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index 59e8b5091c..f479239875 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -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"
diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
index 8e9397aa58..beb7268e57 100644
--- a/target/ppc/arch_dump.c
+++ b/target/ppc/arch_dump.c
@@ -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"
 
diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 9b0bf92698..6f61ff95af 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -15,7 +15,6 @@
 #include "cpu.h"
 #include "internal.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 
 
-- 
2.13.5

[Qemu-devel] [PATCH v5 02/22] cpu: drop old comments describing members

[David Hildenbrand]

These comments are obviously stale.

Acked-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qom/cpu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 995a7beeb5..0dc767a753 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -388,10 +388,10 @@ struct CPUState {
     DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS);
 
     /* TODO Move common fields from CPUArchState here. */
-    int cpu_index; /* used by alpha TCG */
-    uint32_t halted; /* used by alpha, cris, ppc TCG */
+    int cpu_index;
+    uint32_t halted;
     uint32_t can_do_io;
-    int32_t exception_index; /* used by m68k TCG */
+    int32_t exception_index;
 
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;
-- 
2.13.5

[Qemu-devel] [PATCH v5 03/22] s390x: get rid of s390-virtio.c

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

Suggested-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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(-)

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 7ee19d3abc..dc704b57d6 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -1,4 +1,3 @@
-obj-y += s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index dd504dd5ae..4c3a3e177b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -2,6 +2,7 @@
  * virtio ccw machine
  *
  * Copyright 2012 IBM Corp.
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
  *
  * 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);
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index da3f49e80e..9316be1cfc 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -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)
 {
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index ca97fd6814..d984cd4115 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -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
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 03/22] s390x: get rid of s390-virtio.c

[David Hildenbrand]

On 13.09.2017 15:23, David Hildenbrand wrote:
> 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.

> 
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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(-)
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 7ee19d3abc..dc704b57d6 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -1,4 +1,3 @@
> -obj-y += s390-virtio.o
>  obj-y += s390-virtio-hcall.o
>  obj-y += sclp.o
>  obj-y += event-facility.o
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index dd504dd5ae..4c3a3e177b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -2,6 +2,7 @@
>   * virtio ccw machine
>   *
>   * Copyright 2012 IBM Corp.
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>   * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
>   *
>   * 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);
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index da3f49e80e..9316be1cfc 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -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)
>  {
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index ca97fd6814..d984cd4115 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -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
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 03/22] s390x: get rid of s390-virtio.c

[Cornelia Huck]

On Wed, 13 Sep 2017 18:34:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.09.2017 15:23, David Hildenbrand wrote:
> > 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.

No worries, fixed up.

> 
> > 
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  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(-)

[Qemu-devel] [PATCH v5 04/22] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[David Hildenbrand]

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

Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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%)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4c3a3e177b..f67b4b5d58 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -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"
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index 23d67d6170..ec7cf8beb3 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -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
 
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
similarity index 77%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..59fcba3a06 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -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 */
-- 
2.13.5

[Qemu-devel] [PATCH v5 05/22] s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h

[David Hildenbrand]

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

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-hcall.h | 1 +
 target/s390x/cpu.h           | 1 -
 target/s390x/kvm.c           | 1 +
 target/s390x/misc_helper.c   | 1 +
 4 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 59fcba3a06..cbc270eef3 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -17,4 +17,5 @@
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
+int s390_virtio_hypercall(CPUS390XState *env);
 #endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 0bd97a5670..7073fa59e9 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -723,6 +723,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 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);
-int s390_virtio_hypercall(CPUS390XState *env);
 
 #endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index d07763ff2c..ed59896423 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -48,6 +48,7 @@
 #include "hw/s390x/ebcdic.h"
 #include "exec/memattrs.h"
 #include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 50cc046ca2..b142db71c6 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -34,6 +34,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ebcdic.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 #endif
 
 /* #define DEBUG_HELPER */
-- 
2.13.5

[Qemu-devel] [PATCH v5 06/22] s390x: move subsystem_reset() to s390-virtio-ccw.h

[David Hildenbrand]

Implemented in s390-virtio-ccw.c, so move it to the right header.
We can also drop the extern. Fix up one include.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/hw/s390x/s390-virtio-ccw.h | 2 ++
 target/s390x/cpu.h                 | 1 -
 target/s390x/diag.c                | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 41a9d2862b..a9a90c2022 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -56,4 +56,6 @@ bool gs_allowed(void);
  */
 bool css_migration_enabled(void);
 
+void subsystem_reset(void);
+
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7073fa59e9..372f7d8885 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -721,7 +721,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
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index e6b5e6de37..82a623948d 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -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)
 {
-- 
2.13.5

[Qemu-devel] [PATCH v5 07/22] target/s390x: move some s390x typedefs to cpu-qom.h

[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 <david@redhat.com>
---
 target/s390x/cpu-qom.h    | 5 ++++-
 target/s390x/cpu.h        | 5 +++--
 target/s390x/cpu_models.h | 8 ++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 4e936e7788..2435cd8390 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -21,7 +21,6 @@
 #define QEMU_S390_CPU_QOM_H
 
 #include "qom/cpu.h"
-#include "cpu_models.h"
 
 #define TYPE_S390_CPU "s390-cpu"
 
@@ -32,6 +31,9 @@
 #define S390_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(S390CPUClass, (obj), TYPE_S390_CPU)
 
+typedef struct S390CPUModel S390CPUModel;
+typedef struct S390CPUDef S390CPUDef;
+
 /**
  * S390CPUClass:
  * @parent_realize: The parent class' realize handler.
@@ -62,5 +64,6 @@ typedef struct S390CPUClass {
 } S390CPUClass;
 
 typedef struct S390CPU S390CPU;
+typedef struct CPUS390XState CPUS390XState;
 
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 372f7d8885..4a0a59dfbc 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -25,6 +25,7 @@
 
 #include "qemu-common.h"
 #include "cpu-qom.h"
+#include "cpu_models.h"
 
 #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.
@@ -174,7 +175,7 @@ typedef struct CPUS390XState {
     /* currently processed sigp order */
     uint8_t sigp_order;
 
-} CPUS390XState;
+};
 
 static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
 {
diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h
index c0bee15d7a..4c6dee1871 100644
--- a/target/s390x/cpu_models.h
+++ b/target/s390x/cpu_models.h
@@ -17,7 +17,7 @@
 #include "qom/cpu.h"
 
 /* static CPU definition */
-typedef struct S390CPUDef {
+struct S390CPUDef {
     const char *name;       /* name exposed to the user */
     const char *desc;       /* description exposed to the user */
     uint8_t gen;            /* hw generation identification */
@@ -37,10 +37,10 @@ typedef struct S390CPUDef {
     S390FeatBitmap full_feat;
     /* used to init full_feat from generated data */
     S390FeatInit full_init;
-} S390CPUDef;
+};
 
 /* CPU model based on a CPU definition */
-typedef struct S390CPUModel {
+struct S390CPUModel {
     const S390CPUDef *def;
     S390FeatBitmap features;
     /* values copied from the "host" model, can change during migration */
@@ -48,7 +48,7 @@ typedef struct S390CPUModel {
     uint32_t cpu_id;        /* CPU id */
     uint8_t cpu_id_format;  /* CPU id format bit */
     uint8_t cpu_ver;        /* CPU version, usually "ff" for kvm */
-} S390CPUModel;
+};
 
 /*
  * CPU ID
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 07/22] target/s390x: move some s390x typedefs to cpu-qom.h

[Thomas Huth]

On 13.09.2017 15:24, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/cpu-qom.h    | 5 ++++-
>  target/s390x/cpu.h        | 5 +++--
>  target/s390x/cpu_models.h | 8 ++++----
>  3 files changed, 11 insertions(+), 7 deletions(-)
[...]
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 372f7d8885..4a0a59dfbc 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -25,6 +25,7 @@
>  
>  #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?

>  #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.
> @@ -174,7 +175,7 @@ typedef struct CPUS390XState {
>      /* currently processed sigp order */
>      uint8_t sigp_order;
>  
> -} CPUS390XState;
> +};
>  
>  static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
>  {

 Thomas

Re: [Qemu-devel] [PATCH v5 07/22] target/s390x: move some s390x typedefs to cpu-qom.h

[David Hildenbrand]

On 13.09.2017 16:27, Thomas Huth wrote:
> On 13.09.2017 15:24, David Hildenbrand wrote:
>> 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 <david@redhat.com>
>> ---
>>  target/s390x/cpu-qom.h    | 5 ++++-
>>  target/s390x/cpu.h        | 5 +++--
>>  target/s390x/cpu_models.h | 8 ++++----
>>  3 files changed, 11 insertions(+), 7 deletions(-)
> [...]
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 372f7d8885..4a0a59dfbc 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -25,6 +25,7 @@
>>  
>>  #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.


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 07/22] target/s390x: move some s390x typedefs to cpu-qom.h

[Thomas Huth]

On 13.09.2017 16:44, David Hildenbrand wrote:
> On 13.09.2017 16:27, Thomas Huth wrote:
>> On 13.09.2017 15:24, David Hildenbrand wrote:
>>> 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 <david@redhat.com>
>>> ---
>>>  target/s390x/cpu-qom.h    | 5 ++++-
>>>  target/s390x/cpu.h        | 5 +++--
>>>  target/s390x/cpu_models.h | 8 ++++----
>>>  3 files changed, 11 insertions(+), 7 deletions(-)
>> [...]
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 372f7d8885..4a0a59dfbc 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -25,6 +25,7 @@
>>>  
>>>  #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.

OK, I just checked and it indeed blows up in a lot of places, so I think
this include here is justified.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v5 08/22] s390x: move sclp_service_call() to sclp.h

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index a72d096081..847ff32f85 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -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
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 4a0a59dfbc..5295bd3c66 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index b142db71c6..8b07535b02 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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 */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 08/22] s390x: move sclp_service_call() to sclp.h

[Thomas Huth]

On 13.09.2017 15:24, David Hildenbrand wrote:
> 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 <thuth@redhat.com>


> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index a72d096081..847ff32f85 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -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
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 4a0a59dfbc..5295bd3c66 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -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
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index b142db71c6..8b07535b02 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -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 */
> 

Re: [Qemu-devel] [PATCH v5 08/22] s390x: move sclp_service_call() to sclp.h

[David Hildenbrand]

On 13.09.2017 16:29, Thomas Huth wrote:
> On 13.09.2017 15:24, David Hildenbrand wrote:
>> 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...

Nope, a leftover. Conny, can you fix this up if I don't have to resend?

Thanks!

> 
> Apart from that:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 08/22] s390x: move sclp_service_call() to sclp.h

[Cornelia Huck]

On Wed, 13 Sep 2017 16:29:23 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 13.09.2017 15:24, David Hildenbrand wrote:
> > 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."

?

> 
> Apart from that:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v5 08/22] s390x: move sclp_service_call() to sclp.h

[David Hildenbrand]

On 13.09.2017 17:21, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 16:29:23 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 13.09.2017 15:24, David Hildenbrand wrote:
>>> 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."
> 
> ?

You know how to use just the right words ;) Thanks!


-- 

Thanks,

David

[Qemu-devel] [PATCH v5 09/22] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[David Hildenbrand]

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

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 361f970db3..14d3160e92 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -59,8 +59,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
 {
     S390CPU *cpu = S390_CPU(cs);
 
-    cs->exception_index = EXCP_PGM;
-    cpu->env.int_pgm_code = PGM_ADDRESSING;
+    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO);
     /* On real machines this value is dropped into LowMem.  Since this
        is userland, simply put this someplace that cpu_loop can find it.  */
     cpu->env.__excp_addr = address;
-- 
2.13.5

[Qemu-devel] [PATCH v5 10/22] target/s390x: use program_interrupt() in per_check_exception()

[David Hildenbrand]

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 8b07535b02..f3624d75eb 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -447,14 +447,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));
+        program_interrupt(env, PGM_PER, ilen);
     }
 }
 
-- 
2.13.5

[Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

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

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f67b4b5d58..417998ec28 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -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);
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

[Igor Mammedov]

On Wed, 13 Sep 2017 15:24:06 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <mjrosato@linux.vnet.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f67b4b5d58..417998ec28 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -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);
>  

Re: [Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

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

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

>
> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f67b4b5d58..417998ec28 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -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);


--
Alex Bennée

Re: [Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 13.09.2017 18:13, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> 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.
> 
> 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.


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> On 13.09.2017 18:13, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> 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.
>>
>> 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

Re: [Qemu-devel] [PATCH v5 11/22] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 15.09.2017 15:17, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 13.09.2017 18:13, Alex Bennée wrote:
>>>
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> 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.
>>>
>>> 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.

One step at a time, right now I only test with single threaded. MTTCG is
the next step. But I have a good feeling about mttcg, at least speaking
about the SIGP implementation (for the "critical" stuff - start, stop,
initialize - I reuse the KVM code which uses even sync work).

> 
> --
> Alex Bennée
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH v5 12/22] target/s390x: set cpu->id for linux user when realizing

[David Hildenbrand]

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

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 74b3e4fd0d..5f9315fb16 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -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)
-- 
2.13.5

[Qemu-devel] [PATCH v5 13/22] target/s390x: use "core-id" for cpu number/address/id handling

[David Hildenbrand]

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

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c         | 72 +++++++++++++---------------------------------
 target/s390x/cpu.h         |  5 ++--
 target/s390x/cpu_models.c  |  2 +-
 target/s390x/excp_helper.c |  2 +-
 target/s390x/helper.c      |  4 +--
 target/s390x/misc_helper.c |  4 +--
 target/s390x/translate.c   |  5 +---
 8 files changed, 30 insertions(+), 66 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 417998ec28..9da9fd3994 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -310,7 +310,7 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     S390CPU *cpu = S390_CPU(dev);
     CPUState *cs = CPU(dev);
 
-    name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+    name = g_strdup_printf("cpu[%i]", cpu->env.core_id);
     object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
                              errp);
     g_free(name);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 5f9315fb16..87ebbe5b28 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -36,6 +36,7 @@
 #include "trace.h"
 #include "qapi/visitor.h"
 #include "exec/exec-all.h"
+#include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/hw.h"
 #include "sysemu/arch_init.h"
@@ -189,28 +190,31 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 #if !defined(CONFIG_USER_ONLY)
-    if (cpu->id >= max_cpus) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", max allowed: %d", cpu->id, max_cpus - 1);
+    if (cpu->env.core_id >= max_cpus) {
+        error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
+                   ", maximum core-id: %d", cpu->env.core_id,
+                   max_cpus - 1);
         goto out;
     }
 #else
     /* implicitly set for linux-user only */
-    cpu->id = scc->next_cpu_id;
+    cpu->env.core_id = scc->next_cpu_id;
 #endif
 
-    if (cpu_exists(cpu->id)) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", it already exists", cpu->id);
+    if (cpu_exists(cpu->env.core_id)) {
+        error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
+                   ", it already exists", cpu->env.core_id);
         goto out;
     }
-    if (cpu->id != scc->next_cpu_id) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", The next available id is %" PRIi64, cpu->id,
+    if (cpu->env.core_id != scc->next_cpu_id) {
+        error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
+                   ", the next available core-id is %" PRIi64, cpu->env.core_id,
                    scc->next_cpu_id);
         goto out;
     }
 
+    /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
+    cs->cpu_index = env->core_id;
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
@@ -220,7 +224,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
-    env->cpu_num = cpu->id;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -241,45 +244,6 @@ out:
     error_propagate(errp, err);
 }
 
-static void s390x_cpu_get_id(Object *obj, Visitor *v, const char *name,
-                             void *opaque, Error **errp)
-{
-    S390CPU *cpu = S390_CPU(obj);
-    int64_t value = cpu->id;
-
-    visit_type_int(v, name, &value, errp);
-}
-
-static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
-                             void *opaque, Error **errp)
-{
-    S390CPU *cpu = S390_CPU(obj);
-    DeviceState *dev = DEVICE(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
-    Error *err = NULL;
-    int64_t value;
-
-    if (dev->realized) {
-        error_setg(errp, "Attempt to set property '%s' on '%s' after "
-                   "it was realized", name, object_get_typename(obj));
-        return;
-    }
-
-    visit_type_int(v, name, &value, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    if (value < min || value > max) {
-        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
-                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
-                   object_get_typename(obj), name, value, min, max);
-        return;
-    }
-    cpu->id = value;
-}
-
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -293,8 +257,6 @@ static void s390_cpu_initfn(Object *obj)
     cs->env_ptr = env;
     cs->halted = 1;
     cs->exception_index = EXCP_HLT;
-    object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
-                        s390x_cpu_set_id, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
     qemu_get_timedate(&tm, 0);
@@ -491,6 +453,11 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
     return g_strdup("s390:64-bit");
 }
 
+static Property s390x_cpu_properties[] = {
+    DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void s390_cpu_class_init(ObjectClass *oc, void *data)
 {
     S390CPUClass *scc = S390_CPU_CLASS(oc);
@@ -500,6 +467,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
+    dc->props = s390x_cpu_properties;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 5295bd3c66..1c8456fa57 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -150,7 +150,7 @@ struct CPUS390XState {
 
     CPU_COMMON
 
-    uint32_t cpu_num;
+    uint32_t core_id; /* PoP "CPU address", same as cpu_index */
     uint64_t cpuid;
 
     uint64_t tod_offset;
@@ -194,7 +194,6 @@ struct S390CPU {
     /*< public >*/
 
     CPUS390XState env;
-    int64_t id;
     S390CPUModel *model;
     /* needed for live migration */
     void *irqstate;
@@ -690,7 +689,7 @@ const char *s390_default_cpu_model_name(void);
 
 /* helper.c */
 #define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
-S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
+S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
    is returned if the signal was handled by the virtual CPU.  */
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 18cbf91d9c..8e20e7637b 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -915,7 +915,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->env.cpuid = s390_cpuid_from_cpu_model(cpu->model);
     if (tcg_enabled()) {
         /* basic mode, write the cpu address into the first 4 bit of the ID */
-        cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.cpu_num);
+        cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.core_id);
     }
 }
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 14d3160e92..470cf8f5bc 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -250,7 +250,7 @@ static void do_ext_interrupt(CPUS390XState *env)
     lowcore->ext_params2 = cpu_to_be64(q->param64);
     lowcore->external_old_psw.mask = cpu_to_be64(get_psw_mask(env));
     lowcore->external_old_psw.addr = cpu_to_be64(env->psw.addr);
-    lowcore->cpu_addr = cpu_to_be16(env->cpu_num | VIRTIO_SUBCODE_64);
+    lowcore->cpu_addr = cpu_to_be16(env->core_id | VIRTIO_SUBCODE_64);
     mask = be64_to_cpu(lowcore->external_new_psw.mask);
     addr = be64_to_cpu(lowcore->external_new_psw.addr);
 
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index ba29504476..dfb24ef5b2 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -104,7 +104,7 @@ S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
     return S390_CPU(CPU(object_new(typename)));
 }
 
-S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp)
+S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp)
 {
     S390CPU *cpu;
     Error *err = NULL;
@@ -114,7 +114,7 @@ S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp)
         goto out;
     }
 
-    object_property_set_int(OBJECT(cpu), id, "id", &err);
+    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
     if (err != NULL) {
         goto out;
     }
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index f3624d75eb..293fc8428a 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -232,7 +232,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
             /* XXX make different for different CPUs? */
             ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16);
             ebcdic_put(sysib.plant, "QEMU", 4);
-            stw_p(&sysib.cpu_addr, env->cpu_num);
+            stw_p(&sysib.cpu_addr, env->core_id);
             cpu_physical_memory_write(a0, &sysib, sizeof(sysib));
         } else if ((sel1 == 2) && (sel2 == 2)) {
             /* Basic Machine CPUs */
@@ -260,7 +260,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
                 /* XXX make different for different CPUs? */
                 ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16);
                 ebcdic_put(sysib.plant, "QEMU", 4);
-                stw_p(&sysib.cpu_addr, env->cpu_num);
+                stw_p(&sysib.cpu_addr, env->core_id);
                 stw_p(&sysib.cpu_id, 0);
                 cpu_physical_memory_write(a0, &sysib, sizeof(sysib));
             } else if ((sel1 == 2) && (sel2 == 2)) {
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 909b12818d..5abd34fb34 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3823,10 +3823,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;
 }
 
-- 
2.13.5

[Qemu-devel] [PATCH v5 14/22] target/s390x: rename next_cpu_id to next_core_id

[David Hildenbrand]

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

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 2435cd8390..2e446fab51 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -54,7 +54,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);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 87ebbe5b28..8c1c644057 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -198,7 +198,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)) {
@@ -206,10 +206,10 @@ 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_cpu_id) {
+    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 %" PRIi64, cpu->env.core_id,
-                   scc->next_cpu_id);
+                   ", the next available core-id is %" PRIu32, cpu->env.core_id,
+                   scc->next_core_id);
         goto out;
     }
 
@@ -219,7 +219,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);
@@ -464,7 +464,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;
-- 
2.13.5

[Qemu-devel] [PATCH v5 15/22] s390x: print CPU definitions in sorted order

[David Hildenbrand]

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.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 56 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 8e20e7637b..c295e641e6 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -270,16 +270,11 @@ 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));
+    CPUListState *s = user_data;
+    const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data);
+    char *name = g_strdup(object_class_get_name((ObjectClass *)data));
     const char *details = "";
 
     if (scc->is_static) {
@@ -290,21 +285,52 @@ static void print_cpu_model_list(ObjectClass *klass, void *opaque)
 
     /* strip off the -s390-cpu */
     g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
-    (*info->print)(info->f, "s390 %-15s %-35s %s\n", name, scc->desc,
-                   details);
+    (*s->cpu_fprintf)(s->file, "s390 %-15s %-35s %s\n", name, scc->desc,
+                      details);
     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);
+
+    /* 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;
+    } else if (name_a[0] == 'h') {
+        return -1;
+    } else if (name_b[0] == 'h') {
+        return 1;
+    }
+
+    /* 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;
+    GSList *list;
 
-    object_class_foreach(print_cpu_model_list, TYPE_S390_CPU, false, &info);
+    list = object_class_get_list(TYPE_S390_CPU, false);
+    list = g_slist_sort(list, s390_cpu_list_compare);
+    g_slist_foreach(list, s390_print_cpu_model_list_entry, &s);
+    g_slist_free(list);
 
     (*print)(f, "\nRecognized feature flags:\n");
     for (feat = 0; feat < S390_FEAT_MAX; feat++) {
-- 
2.13.5

[Qemu-devel] [PATCH v5 16/22] s390x: allow cpu hotplug via device_add

[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

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

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8c1c644057..5724dffb88 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -467,6 +467,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)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 16/22] s390x: allow cpu hotplug via device_add

[Thomas Huth]

On 13.09.2017 15:24, David Hildenbrand wrote:
> 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

Re: [Qemu-devel] [PATCH v5 16/22] s390x: allow cpu hotplug via device_add

[David Hildenbrand]

On 28.09.2017 08:01, Thomas Huth wrote:
> On 13.09.2017 15:24, David Hildenbrand wrote:
>> 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
> 

Interesting, x86 shields hotplug for none completely.

t460s: ~/git/qemu s390x_lap $ sudo qemu-system-x86_64 -M none -smp
1,maxcpus=4 -monitor stdio -Sqemu-system-x86_64: Number of SMP CPUs
requested (4) exceeds max CPUs supported by machine 'none' (1)

Will send a patch.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 16/22] s390x: allow cpu hotplug via device_add

[David Hildenbrand]

On 28.09.2017 08:01, Thomas Huth wrote:
> On 13.09.2017 15:24, David Hildenbrand wrote:
>> 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
> 

Interestingly, this is already fixed by my patch

s390x: raise CPU hotplug irq after really hotplugged

part of the SMP series.

-- 

Thanks,

David

[Qemu-devel] [PATCH v5 17/22] s390x: CPU hot unplug via device_del cannot work for now

[David Hildenbrand]

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 <mjrosato@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 9da9fd3994..3be81d96af 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -337,6 +337,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)
 {
@@ -386,6 +395,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;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 17/22] s390x: CPU hot unplug via device_del cannot work for now

[Cornelia Huck]

On Wed, 13 Sep 2017 15:24:12 +0200
David Hildenbrand <david@redhat.com> wrote:

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

s/emmit/emit/

(fixing while applying)

> 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 <mjrosato@linux.vnet.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

[Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[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 <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json           | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3be81d96af..cc64a81321 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -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;
diff --git a/qapi-schema.json b/qapi-schema.json
index f3af2cb851..79e9f85404 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -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'] }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Igor Mammedov]

On Wed, 13 Sep 2017 15:24:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <david@redhat.com>
looks good to me from cpu hotplug infrastructure pov,

Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json           | 16 ++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3be81d96af..cc64a81321 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -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;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f3af2cb851..79e9f85404 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -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'] }
>  

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Cornelia Huck]

On Wed, 13 Sep 2017 15:24:13 +0200
David Hildenbrand <david@redhat.com> 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)

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json           | 16 ++++++++++++++++
>  2 files changed, 57 insertions(+)

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[David Hildenbrand]

On 13.09.2017 17:49, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:24:13 +0200
> David Hildenbrand <david@redhat.com> 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)

:) Sure!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> 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 <david@redhat.com>
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f3af2cb851..79e9f85404 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -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'] }

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.

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Cornelia Huck]

On Mon, 02 Oct 2017 09:46:41 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> David Hildenbrand <david@redhat.com> writes:
> 
> > 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 <david@redhat.com>  
> [...]
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index f3af2cb851..79e9f85404 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -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'] }  
> 
> 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?

> 
> 
> [*] 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

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> On Mon, 02 Oct 2017 09:46:41 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> David Hildenbrand <david@redhat.com> writes:
>> 
>> > 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 <david@redhat.com>  
>> [...]
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index f3af2cb851..79e9f85404 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -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'] }  
>> 
>> 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?

Whatever makes the generated documentation look better.

>> [*] 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

Pasto, sorry!  Try "make docs/interop/qemu-qmp-ref.txt".  .pdf and .html
also work.

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Cornelia Huck]

On Wed, 04 Oct 2017 14:42:55 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Mon, 02 Oct 2017 09:46:41 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >  
> >> David Hildenbrand <david@redhat.com> writes:
> >>   
> >> > 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 <david@redhat.com>    
> >> [...]  
> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> > index f3af2cb851..79e9f85404 100644
> >> > --- a/qapi-schema.json
> >> > +++ b/qapi-schema.json
> >> > @@ -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'] }    
> >> 
> >> 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?  
> 
> 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.)

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> On Wed, 04 Oct 2017 14:42:55 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Cornelia Huck <cohuck@redhat.com> writes:
>> 
>> > On Mon, 02 Oct 2017 09:46:41 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >  
>> >> David Hildenbrand <david@redhat.com> writes:
>> >>   
>> >> > 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 <david@redhat.com>    
>> >> [...]  
>> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> > index f3af2cb851..79e9f85404 100644
>> >> > --- a/qapi-schema.json
>> >> > +++ b/qapi-schema.json
>> >> > @@ -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'] }    
>> >> 
>> >> 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?  
>> 
>> 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.)

Marc-André, any ideas?

Re: [Qemu-devel] [PATCH v5 18/22] s390x: implement query-hotpluggable-cpus

[Marc-André Lureau]

Hi

----- Original Message -----
> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Wed, 04 Oct 2017 14:42:55 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Cornelia Huck <cohuck@redhat.com> writes:
> >> 
> >> > On Mon, 02 Oct 2017 09:46:41 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >  
> >> >> David Hildenbrand <david@redhat.com> writes:
> >> >>   
> >> >> > 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 <david@redhat.com>
> >> >> [...]
> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> > index f3af2cb851..79e9f85404 100644
> >> >> > --- a/qapi-schema.json
> >> >> > +++ b/qapi-schema.json
> >> >> > @@ -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'] }
> >> >> 
> >> >> 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?
> >> 
> >> 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.)
> 
> Marc-André, any ideas?
> 

It's not supported atm. The ideas to fix this are pretty much a matter of taste.

I'd suggest to keep the code below the Example: section as @example/verbatim by default. And introduce a new section syntax, like Example:: to do "manual" formatting.

Example::

This text would be in regular format.
1. and allow list, *strong* etc.

| -> { then have some code }
| <- ...

And regulat text again.

[Qemu-devel] [PATCH v5 19/22] s390x: get rid of cpu_states and use possible_cpus instead

[David Hildenbrand]

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 <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index cc64a81321..720f145054 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -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);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 19/22] s390x: get rid of cpu_states and use possible_cpus instead

[Igor Mammedov]

On Wed, 13 Sep 2017 15:24:14 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/s390x/s390-virtio-ccw.c | 36 ++++++++++--------------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index cc64a81321..720f145054 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -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);

[Qemu-devel] [PATCH v5 20/22] s390x: get rid of cpu_s390x_create()

[David Hildenbrand]

Now that there is only one user of cpu_s390x_create() left, make cpu
creation look 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

Acked-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 720f145054..0471407187 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -55,6 +55,10 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    const char *typename;
+    gchar **model_pieces;
+    ObjectClass *oc;
+    CPUClass *cc;
     int i;
 
     if (machine->cpu_model == NULL) {
@@ -69,8 +73,25 @@ static void s390_init_cpus(MachineState *machine)
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
+    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_S390_CPU, model_pieces[0]);
+    if (!oc) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    /* after parsing, properties will be applied to all *typename* instances */
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+
     for (i = 0; i < smp_cpus; i++) {
-        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
+        s390x_new_cpu(typename, i, &error_fatal);
     }
 }
 
@@ -380,8 +401,12 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
 static void s390_hot_add_cpu(const int64_t id, Error **errp)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
+    ObjectClass *oc;
+
+    g_assert(machine->possible_cpus->cpus[0].cpu);
+    oc = OBJECT_CLASS(CPU_GET_CLASS(machine->possible_cpus->cpus[0].cpu));
 
-    s390x_new_cpu(machine->cpu_model, id, errp);
+    s390x_new_cpu(object_class_get_name(oc), id, errp);
 }
 
 static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 1c8456fa57..9b549dc491 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -689,7 +689,7 @@ const char *s390_default_cpu_model_name(void);
 
 /* helper.c */
 #define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
-S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp);
+S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, Error **errp);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
    is returned if the signal was handled by the virtual CPU.  */
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index dfb24ef5b2..97adbcc86d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -68,52 +68,11 @@ void s390x_cpu_timer(void *opaque)
 }
 #endif
 
-S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
+S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, Error **errp)
 {
-    static bool features_parsed;
-    char *name, *features;
-    const char *typename;
-    ObjectClass *oc;
-    CPUClass *cc;
-
-    name = g_strdup(cpu_model);
-    features = strchr(name, ',');
-    if (features) {
-        features[0] = 0;
-        features++;
-    }
-
-    oc = cpu_class_by_name(TYPE_S390_CPU, name);
-    if (!oc) {
-        error_setg(errp, "Unknown CPU definition \'%s\'", name);
-        g_free(name);
-        return NULL;
-    }
-    typename = object_class_get_name(oc);
-
-    if (!features_parsed) {
-        features_parsed = true;
-        cc = CPU_CLASS(oc);
-        cc->parse_features(typename, features, errp);
-    }
-    g_free(name);
-
-    if (*errp) {
-        return NULL;
-    }
-    return S390_CPU(CPU(object_new(typename)));
-}
-
-S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp)
-{
-    S390CPU *cpu;
+    S390CPU *cpu = S390_CPU(object_new(typename));
     Error *err = NULL;
 
-    cpu = cpu_s390x_create(cpu_model, &err);
-    if (err != NULL) {
-        goto out;
-    }
-
     object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
     if (err != NULL) {
         goto out;
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index b4d3583b24..bc8f83129a 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -337,7 +337,6 @@ uint64_t get_psw_mask(CPUS390XState *env);
 void s390_cpu_recompute_watchpoints(CPUState *cs);
 void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
-S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
 void do_restart_interrupt(CPUS390XState *env);
 #ifndef CONFIG_USER_ONLY
 LowCore *cpu_map_lowcore(CPUS390XState *env);
-- 
2.13.5

[Qemu-devel] [PATCH v5 21/22] s390x: generate sclp cpu information from possible_cpus

[David Hildenbrand]

This is the first step to allow hot plugging of CPUs in a non-sequential
order. If a cpu is available ("plugged") can directly be decided by
looking at the cpu state pointer.

This makes sure, that really only cpus attached to the machine are
reported.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/sclp.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fd097262c7..30aefbfd15 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -34,16 +34,21 @@ static inline SCLPDevice *get_sclp_device(void)
     return sclp;
 }
 
-static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int count)
+static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
     int i;
 
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
-    for (i = 0; i < count; i++) {
-        entry[i].address = i;
-        entry[i].type = 0;
-        memcpy(entry[i].features, features, sizeof(entry[i].features));
+    for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
+        if (!ms->possible_cpus->cpus[i].cpu) {
+            continue;
+        }
+        entry[*count].address = ms->possible_cpus->cpus[i].arch_id;
+        entry[*count].type = 0;
+        memcpy(entry[*count].features, features, sizeof(features));
+        (*count)++;
     }
 }
 
@@ -53,17 +58,13 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     ReadInfo *read_info = (ReadInfo *) sccb;
     MachineState *machine = MACHINE(qdev_get_machine());
     sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
-    CPUState *cpu;
-    int cpu_count = 0;
+    int cpu_count;
     int rnsize, rnmax;
     int slots = MIN(machine->ram_slots, s390_get_memslot_count());
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
-    CPU_FOREACH(cpu) {
-        cpu_count++;
-    }
-
     /* CPU information */
+    prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
     read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
     read_info->highest_cpu = cpu_to_be16(max_cpus);
@@ -76,8 +77,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
                          read_info->conf_char_ext);
 
-    prepare_cpu_entries(sclp, read_info->entries, cpu_count);
-
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
                                         SCLP_HAS_IOA_RECONFIG);
 
@@ -333,13 +332,9 @@ static void unassign_storage(SCLPDevice *sclp, SCCB *sccb)
 static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 {
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
-    CPUState *cpu;
-    int cpu_count = 0;
-
-    CPU_FOREACH(cpu) {
-        cpu_count++;
-    }
+    int cpu_count;
 
+    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
     cpu_info->nr_configured = cpu_to_be16(cpu_count);
     cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
     cpu_info->nr_standby = cpu_to_be16(0);
@@ -348,7 +343,6 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
         + cpu_info->nr_configured*sizeof(CPUEntry));
 
-    prepare_cpu_entries(sclp, cpu_info->entries, cpu_count);
 
     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
-- 
2.13.5

[Qemu-devel] [PATCH v5 22/22] s390x: allow CPU hotplug in random core-id order

[David Hildenbrand]

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 <david@redhat.com>
---
 target/s390x/cpu.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 5724dffb88..34538c3ab9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -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);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 22/22] s390x: allow CPU hotplug in random core-id order

[Igor Mammedov]

On Wed, 13 Sep 2017 15:24:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/s390x/cpu.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 5724dffb88..34538c3ab9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -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);

Re: [Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[Cornelia Huck]

On Wed, 13 Sep 2017 15:23:55 +0200
David Hildenbrand <david@redhat.com> 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.

Thanks, applied.

Re: [Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> On Wed, 13 Sep 2017 15:23:55 +0200
> David Hildenbrand <david@redhat.com> 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.
>
> Thanks, applied.

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

Re: [Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[Cornelia Huck]

On Mon, 02 Oct 2017 09:47:58 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Wed, 13 Sep 2017 15:23:55 +0200
> > David Hildenbrand <david@redhat.com> 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.  
> >
> > Thanks, applied.  
> 
> Please squash in the one-liner fix I asked for in my (late!) review of
> PATCH 18.

Sadly, the review was a bit too late (already merged)...

Re: [Qemu-devel] [PATCH v5 00/22] s390x cleanups and CPU hotplug via device_add

[David Hildenbrand]

On 13.09.2017 15:23, David Hildenbrand 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.
> 

Just played with libvirt. It seems to automatically use the new
interface. Works great for me (including migration).


-- 

Thanks,

David