Skip to contentSkip to conversation

s390x cleanups and CPU hotplug via device_add

PATCH Superseded David Hildenbrand 6 participants 37 messages Sep 7, 2017 – Oct 2, 2017

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v3 cover s390x cleanups and CPU hotplug via device_add

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.

In contrast to v2, this series now also adds support for hotplugging CPUs in random core-id order (last two patches). 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.

I am pretty sure now that this patch series won't grow even more :)

v2 -> v3:
* "exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly"
- dropped removal of one unrelated include
* "s390x: move subsystem_reset() to s390-virtio-ccw.h"
- previously "s390x: move two function declarations to s390-virtio-ccw.h"
- as Thomas requested, s390_cpu_addr2state() will be later moved to cpu.c
* "s390x: move sclp_service_call() to sclp.h"
- previously "s390x: move sclp_service_call() to interrupt.c"
- only move the declaration and make it compile
* "s390x: allow only 1 CPU with TCG"
- switch to simple ifdef. Let's see what people think about this one :)
* "s390x: print CPU definitions in sorted order"
- fix host model getting reported as static :(
* "s390x: CPU hot unplug via device_del cannot work"
- use the term "currently" instead of "never"
* "s390x: implement query-hotpluggable-cpus"
- as requested by Thomas, moving s390_cpu_addr2state() to cpu.c
* added "s390x: generate sclp cpu information from possible_cpus"
* added "s390x: allow CPU hotplug in random core-id order"

David Hildenbrand (21):
exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly
cpu: drop old comments describing members
s390x: get rid of s390-virtio.c
s390x: rename s390-virtio.h to s390-virtio-hcall.h target/s390x: move typedef of S390CPU to its definition
s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
s390x: move subsystem_reset() to s390-virtio-ccw.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_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         | 245 +++++++++++++++++++++++++++++++++++--
 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             |   4 +-
 target/s390x/cpu.c                 | 101 ++++++---------
 target/s390x/cpu.h                 |  17 +--
 target/s390x/cpu_models.c          |  58 ++++++---
 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 +-
 29 files changed, 400 insertions(+), 269 deletions(-)
 create mode 100644 hw/s390x/s390-virtio-hcall.h
 delete mode 100644 hw/s390x/s390-virtio.h
Signature · 1 line
2.13.5
#1 lore raw

PATCH v3 1/21 exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

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.

---
 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(-)
dump.c +0 -1
index a79773d0f7..2ef6a678e8 100644
@@ -15,7 +15,6 @@
 #include "qemu/cutils.h"
 #include "elf.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"
exec.c +0 -1
index d20c34ca83..a25a4c6018 100644
@@ -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"
target/i386/arch_dump.c +0 -1
index e682904052..35b55fc200 100644
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "elf.h"
 #include "sysemu/memory_mapping.h"
target/i386/arch_memory_mapping.c +0 -1
index 647cff2829..271cb5e41b 100644
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/memory_mapping.h"
 
 /* PAE Paging or IA-32e Paging */
target/i386/svm_helper.c +0 -1
index 59e8b5091c..f479239875 100644
@@ -19,7 +19,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
target/ppc/arch_dump.c +0 -1
index 8e9397aa58..beb7268e57 100644
@@ -15,7 +15,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "sysemu/kvm.h"
 
target/s390x/arch_dump.c +0 -1
index 9b0bf92698..6f61ff95af 100644
@@ -15,7 +15,6 @@
 #include "cpu.h"
 #include "internal.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 
 
Signature · 1 line
2.13.5
#2 lore raw

PATCH v3 2/21 cpu: drop old comments describing members

In reply to David Hildenbrand

These comments are obviously stale.

---
 include/qom/cpu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
include/qom/cpu.h +3 -3
index 995a7beeb5..0dc767a753 100644
@@ -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;
Signature · 1 line
2.13.5
#3 lore raw

PATCH v3 3/21 s390x: get rid of s390-virtio.c

In reply to 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.

---
 hw/s390x/Makefile.objs     |   1 -
 hw/s390x/s390-virtio-ccw.c | 157 ++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     |  37 -----------
 hw/s390x/s390-virtio.h     |  15 -----
 4 files changed, 156 insertions(+), 54 deletions(-)
hw/s390x/Makefile.objs +0 -1
index 7ee19d3abc..dc704b57d6 100644
@@ -1,4 +1,3 @@
-obj-y += s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o
hw/s390x/s390-virtio-ccw.c +156 -1

198 lines not on this page · raw message

hw/s390x/s390-virtio.c +0 -37
index da3f49e80e..9316be1cfc 100644
@@ -48,17 +48,6 @@
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **cpu_states;
-
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
-{
-    if (cpu_addr >= max_cpus) {
-        return NULL;
-    }
-
-    /* Fast lookup via CPU ID */
-    return cpu_states[cpu_addr];
-}
 
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
@@ -86,32 +75,6 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(MachineState *machine)
-{
-    int i;
-    gchar *name;
-
-    if (machine->cpu_model == NULL) {
-        machine->cpu_model = s390_default_cpu_model_name();
-    }
-
-    cpu_states = g_new0(S390CPU *, max_cpus);
-
-    for (i = 0; i < max_cpus; i++) {
-        name = g_strdup_printf("cpu[%i]", i);
-        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
-                                 (Object **) &cpu_states[i],
-                                 object_property_allow_set_link,
-                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
-                                 &error_abort);
-        g_free(name);
-    }
-
-    for (i = 0; i < smp_cpus; i++) {
-        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
-    }
-}
-
 
 void s390_create_virtio_net(BusState *bus, const char *name)
 {
hw/s390x/s390-virtio.h +0 -15
index ca97fd6814..d984cd4115 100644
@@ -12,24 +12,9 @@
 #ifndef HW_S390_VIRTIO_H
 #define HW_S390_VIRTIO_H
 
-#include "hw/nmi.h"
 #include "standard-headers/asm-s390/kvm_virtio.h"
 #include "standard-headers/asm-s390/virtio-ccw.h"
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
-
-void s390_init_cpus(MachineState *machine);
-void s390_init_ipl_dev(const char *kernel_filename,
-                       const char *kernel_cmdline,
-                       const char *initrd_filename,
-                       const char *firmware,
-                       const char *netboot_fw,
-                       bool enforce_bios);
-void s390_create_virtio_net(BusState *bus, const char *name);
-void s390_nmi(NMIState *n, int cpu_index, Error **errp);
-void s390_machine_reset(void);
-void s390_memory_init(ram_addr_t mem_size);
-void gtod_save(QEMUFile *f, void *opaque);
-int gtod_load(QEMUFile *f, void *opaque, int version_id);
 #endif
Signature · 1 line
2.13.5
#4 lore raw

PATCH v3 4/21 s390x: rename s390-virtio.h to s390-virtio-hcall.h

In reply to David Hildenbrand

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

---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (77%)
hw/s390x/s390-virtio-ccw.c +1 -1
index 4c3a3e177b..f67b4b5d58 100644
@@ -16,7 +16,7 @@
 #include "cpu.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
-#include "s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
hw/s390x/s390-virtio-hcall.c +1 -1
index 23d67d6170..ec7cf8beb3 100644
@@ -11,7 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/s390x/s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 
 #define MAX_DIAG_SUBCODES 255
 
hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +4 -4
similarity index 77%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..59fcba3a06 100644
@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <[email protected]>
@@ -9,12 +9,12 @@
  * directory.
  */
 
-#ifndef HW_S390_VIRTIO_H
-#define HW_S390_VIRTIO_H
+#ifndef HW_S390_VIRTIO_HCALL_H
+#define HW_S390_VIRTIO_HCALL_H
 
 #include "standard-headers/asm-s390/kvm_virtio.h"
 #include "standard-headers/asm-s390/virtio-ccw.h"
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
-#endif
+#endif /* HW_S390_VIRTIO_HCALL_H */
Signature · 1 line
2.13.5
#5 lore raw

PATCH v3 5/21 target/s390x: move typedef of S390CPU to its definition

In reply to David Hildenbrand

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

---
 target/s390x/cpu-qom.h | 2 --
 target/s390x/cpu.h     | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)
target/s390x/cpu-qom.h +0 -2
index 4e936e7788..c8fbf8ae03 100644
@@ -61,6 +61,4 @@ typedef struct S390CPUClass {
     void (*initial_cpu_reset)(CPUState *cpu);
 } S390CPUClass;
 
-typedef struct S390CPU S390CPU;
-
 #endif
target/s390x/cpu.h +2 -2
index 0bd97a5670..a7426ef6e2 100644
@@ -187,7 +187,7 @@ static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
  *
  * An S/390 CPU.
  */
-struct S390CPU {
+typedef struct S390CPU {
     /*< private >*/
     CPUState parent_obj;
     /*< public >*/
@@ -198,7 +198,7 @@ struct S390CPU {
     /* needed for live migration */
     void *irqstate;
     uint32_t irqstate_saved_size;
-};
+} S390CPU;
 
 static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
 {
Signature · 1 line
2.13.5
#6 lore raw

PATCH v3 6/21 s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h

In reply to David Hildenbrand

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

---
 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(-)
hw/s390x/s390-virtio-hcall.h +1 -0
index 59fcba3a06..cbc270eef3 100644
@@ -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 */
target/s390x/cpu.h +0 -1
index a7426ef6e2..2343f2f1f7 100644
@@ -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
target/s390x/kvm.c +1 -0
index d07763ff2c..ed59896423 100644
@@ -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
target/s390x/misc_helper.c +1 -0
index 50cc046ca2..b142db71c6 100644
@@ -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 */
Signature · 1 line
2.13.5
#7 lore raw

PATCH v3 7/21 s390x: move subsystem_reset() to s390-virtio-ccw.h

In reply to 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.

---
 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(-)
include/hw/s390x/s390-virtio-ccw.h +2 -0
index 41a9d2862b..a9a90c2022 100644
@@ -56,4 +56,6 @@ bool gs_allowed(void);
  */
 bool css_migration_enabled(void);
 
+void subsystem_reset(void);
+
 #endif
target/s390x/cpu.h +0 -1
index 2343f2f1f7..3aa2e46aac 100644
@@ -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
target/s390x/diag.c +1 -0
index e6b5e6de37..82a623948d 100644
@@ -20,6 +20,7 @@
 #include "hw/watchdog/wdt_diag288.h"
 #include "sysemu/cpus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 static int modified_clear_reset(S390CPU *cpu)
 {
Signature · 1 line
2.13.5
Reviewed-by on v3 7/21
#8 lore raw

Re: [Qemu-devel] [PATCH v3 07/21] s390x: move subsystem_reset() to s390-virtio-ccw.h

Quoted 9 lines · 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.

---
 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(-)
Acked-by on v3 7/21
#9 lore raw

Re: [Qemu-devel] [PATCH v3 07/21] s390x: move subsystem_reset() to s390-virtio-ccw.h

In reply to David Hildenbrand
Quoted 4 lines · 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.

Quoted 42 lines
---
 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(-)
include/hw/s390x/s390-virtio-ccw.h +2 -0
index 41a9d2862b..a9a90c2022 100644
@@ -56,4 +56,6 @@ bool gs_allowed(void);
  */
 bool css_migration_enabled(void);
 
+void subsystem_reset(void);
+
 #endif
target/s390x/cpu.h +0 -1
index 2343f2f1f7..3aa2e46aac 100644
@@ -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
target/s390x/diag.c +1 -0
index e6b5e6de37..82a623948d 100644
@@ -20,6 +20,7 @@
 #include "hw/watchdog/wdt_diag288.h"
 #include "sysemu/cpus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 static int modified_clear_reset(S390CPU *cpu)
 {
#10 lore raw

PATCH v3 8/21 s390x: move sclp_service_call() to sclp.h

In reply to 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 CPUS390XState to fix the two sclp consoles complaining.

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +2 -0
index a72d096081..4b86a8a293 100644
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
target/s390x/cpu.h +0 -1
index 3aa2e46aac..032d1de2e8 100644
@@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
target/s390x/misc_helper.c +1 -0
index b142db71c6..8b07535b02 100644
@@ -35,6 +35,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ebcdic.h"
 #include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 /* #define DEBUG_HELPER */
Signature · 1 line
2.13.5
#11 lore raw

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

Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

#12 lore raw

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

Quoted 31 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

Quoted 7 lines

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

I prefer the kernel style for typedefs myself. But it's strictly a matter of style. Excessive typedeffing makes code harder to understand, it isn't wrong. The part that's wrong is defining things more than once, and that applies to everything, not just typedefs.

Sometimes you get away with defining something more than once. It's still wrong.

You're not supposed to define the same variable more than once, either (although many C compilers let you get away with it, see -fno-common). You define it in *one* place. If you need to declare it, declare it in *one* place, and make sure that place is in scope at the definition, so the compiler can check the two match.

Likewise, you're not supposed to define the same struct tag more than once, and you should declare it in just one place.

Likewise, you're not supposed to define (with typedef) the same type more than once. There is no such thing as a typedef declaration.

qemu/typedefs.h is not a work-around for a typedef-happy style. Its purpose is breaking inclusion cycles. Secondary purpose is reducing the need for non-cyclic includes. If we didn't typedef, we'd still put our struct declarations there.

#13 lore raw

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

Quoted 49 lines · Markus Armbruster
Quoted 31 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

Quoted 7 lines

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

I prefer the kernel style for typedefs myself. But it's strictly a matter of style. Excessive typedeffing makes code harder to understand, it isn't wrong. The part that's wrong is defining things more than once, and that applies to everything, not just typedefs.

Sometimes you get away with defining something more than once. It's still wrong.

Quoted 8 lines

You're not supposed to define the same variable more than once, either (although many C compilers let you get away with it, see -fno-common). You define it in *one* place. If you need to declare it, declare it in *one* place, and make sure that place is in scope at the definition, so the compiler can check the two match.

Likewise, you're not supposed to define the same struct tag more than once, and you should declare it in just one place.

AFAIK it's perfect valid C to do a forward declaration of a struct multiple times by just writing e.g. "struct CPUS390XState;" somewhere in your code. This is also what is done in various Linux kernel headers all over the place.

Quoted 7 lines

Likewise, you're not supposed to define (with typedef) the same type more than once. There is no such thing as a typedef declaration.

qemu/typedefs.h is not a work-around for a typedef-happy style. Its purpose is breaking inclusion cycles. Secondary purpose is reducing the need for non-cyclic includes. If we didn't typedef, we'd still put our struct declarations there.

No, since it's not required for struct forward declarations, only to avoid multiple typedef definitions.

Thomas

#14 lore raw

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

Quoted 56 lines · Thomas Huth
Quoted 55 lines · Markus Armbruster
Quoted 31 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

Quoted 7 lines

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

I prefer the kernel style for typedefs myself. But it's strictly a matter of style. Excessive typedeffing makes code harder to understand, it isn't wrong. The part that's wrong is defining things more than once, and that applies to everything, not just typedefs.

Sometimes you get away with defining something more than once. It's still wrong.

You're not supposed to define the same variable more than once, either (although many C compilers let you get away with it, see -fno-common). You define it in *one* place. If you need to declare it, declare it in *one* place, and make sure that place is in scope at the definition, so the compiler can check the two match.

Quoted 7 lines
Quoted 2 lines

Likewise, you're not supposed to define the same struct tag more than once, and you should declare it in just one place.

AFAIK it's perfect valid C to do a forward declaration of a struct multiple times by just writing e.g. "struct CPUS390XState;" somewhere in your code. This is also what is done in various Linux kernel headers all over the place.

"Define it in one place" is dictated by the language, i.e. violating the rule is wrong.

"Declare it in one place" is merely style. I insist on it for declarations the compiler can meaningfully check against definitions, such as function declarations. It can't for struct tags, and I consider "one place" a matter of taste there.

Quoted 10 lines
Quoted 7 lines

Likewise, you're not supposed to define (with typedef) the same type more than once. There is no such thing as a typedef declaration.

qemu/typedefs.h is not a work-around for a typedef-happy style. Its purpose is breaking inclusion cycles. Secondary purpose is reducing the need for non-cyclic includes. If we didn't typedef, we'd still put our struct declarations there.

No, since it's not required for struct forward declarations, only to avoid multiple typedef definitions.

I definitely would, because I prefer sticking to "declare in one place" style.

#15 lore raw

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

In reply to Thomas Huth
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +0 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, uint32_t code);

#endif

target/s390x/cpu.h +0 -0
index 3aa2e46aac..032d1de2e8 100644
@@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr,

uint8_t ar, void *hostbuf,

/* outside of target/s390x/ */
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

#endif

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

Opinions?

Signature · 3 lines
Thanks,

David
#16 lore raw

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

Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 1 line

#endif

Why not use typedefs.h?

---

include/hw/s390x/sclp.h +0 -1
index 4b86a8a293..3512bf8283 100644
@@ -242,7 +242,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);
-typedef struct CPUS390XState CPUS390XState;
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
include/qemu/typedefs.h +1 -0
index 39bc8351a3..9c97bffa92 100644
@@ -21,6 +21,7 @@ typedef struct Chardev Chardev;
 typedef struct CompatProperty CompatProperty;
 typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
+typedef struct CPUS390XState CPUS390XState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
 typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
target/s390x/cpu.h +2 -2
index 032d1de2e8..f99a82cd5e 100644
@@ -80,7 +80,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 +174,7 @@ typedef struct CPUS390XState {
     /* currently processed sigp order */
     uint8_t sigp_order;
 
-} CPUS390XState;
+};
 
 static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
 {
Quoted 35 lines
target/s390x/cpu.h +0 -1
index 3aa2e46aac..032d1de2e8 100644
@@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr,
 uint8_t ar, void *hostbuf,
 
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 

#endif

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

Opinions?

Signature · 3 lines
Thanks,

David
Signature · 1 line
Eduardo
#17 lore raw

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

include/qemu/typedefs.h

Quoted 85 lines · Eduardo Habkost
Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 1 line

#endif

Why not use typedefs.h?

---

include/hw/s390x/sclp.h +0 -1
index 4b86a8a293..3512bf8283 100644
@@ -242,7 +242,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);
-typedef struct CPUS390XState CPUS390XState;
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
include/qemu/typedefs.h +0 -0
index 39bc8351a3..9c97bffa92 100644

Using include/qemu/typedefs.h here is IMHO really ugly. Do we really want to pollute a common include file with target specific code? My preferences are first to avoid typdefs, but if we really need/want them (do we? There is no comment about this in our coding styles), I think we should rather introduce target-specific typedefs.h headers, too, for everything that is not part of the common code.

Thomas

#18 lore raw

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

Quoted 81 lines · Thomas Huth
Quoted 80 lines · Eduardo Habkost
Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 1 line

#endif

Why not use typedefs.h?

---

include/hw/s390x/sclp.h +0 -1
index 4b86a8a293..3512bf8283 100644
@@ -242,7 +242,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);
-typedef struct CPUS390XState CPUS390XState;
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
Quoted 12 lines

include/qemu/typedefs.h

Quoted 5 lines
include/hw/s390x/sclp.h +0 -0
 #endif
include/qemu/typedefs.h +0 -0
index 39bc8351a3..9c97bffa92 100644

Using include/qeemu/typedefs.h here is IMHO really ugly. Do we really want to pollute a common include file with target specific code? My preferences are first to avoid typdefs, but if we really need/want them (do we? There is no comment about this in our coding styles), I think we should rather introduce target-specific typedefs.h headers, too, for everything that is not part of the common code.

I don't see any advantage in splitting typedefs.h into arch-specific files. We don't split typedefs.h into subsystem-specific or device-specific headers, so I don't see we would need a per-architecture split either. typedefs.h is just a global collection of type identifiers that helps us reduce header dependency hell.

(Anyway, the current problem is now going solved by using S390CPU* instead of CPUS390XState*, so there's no need to touch typedefs.h this time.)

About keeping using typedefs: I don't have an strong opinion for/against them[1], but I would prefer to keep style consistent even if it's not explicitly documented.

[1] The fact that it would make typedefs.h completely unnecessary makes me inclined towards the suggestion to stop using them.

Signature · 1 line
Eduardo
#19 lore raw

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

In reply to Eduardo Habkost
Quoted 63 lines · Eduardo Habkost
Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 3 lines
Quoted 1 line

#endif

Why not use typedefs.h?

See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed.

Quoted 87 lines

---

include/hw/s390x/sclp.h +0 -1
index 4b86a8a293..3512bf8283 100644
@@ -242,7 +242,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);
-typedef struct CPUS390XState CPUS390XState;
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
include/qemu/typedefs.h +1 -0
index 39bc8351a3..9c97bffa92 100644
@@ -21,6 +21,7 @@ typedef struct Chardev Chardev;
 typedef struct CompatProperty CompatProperty;
 typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
+typedef struct CPUS390XState CPUS390XState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
 typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
target/s390x/cpu.h +2 -2
index 032d1de2e8..f99a82cd5e 100644
@@ -80,7 +80,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 +174,7 @@ typedef struct CPUS390XState {
     /* currently processed sigp order */
     uint8_t sigp_order;
 
-} CPUS390XState;
+};
 
 static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
 {
Quoted 38 lines
target/s390x/cpu.h +0 -1
index 3aa2e46aac..032d1de2e8 100644
@@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr,
 uint8_t ar, void *hostbuf,
 
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 

#endif

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

Opinions?

Signature · 3 lines
Thanks,

David
#20 lore raw

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

Quoted 64 lines · Paolo Bonzini
Quoted 63 lines · Eduardo Habkost
Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

#endif

Why not use typedefs.h?

See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed.

I'll go with that approach. I'll drop the dependency from cpu-qom.h to cpu_models.h (by moving typedefs to cpu-qom.h). This makes it compile at hopefully should then be good enough for now.

(this approach implies dropping patch "[PATCH v3 05/21] target/s390x:
move typedef of S390CPU to its definition").

Thanks!

Quoted 2 lines
Quoted 2 lines
Signature · 3 lines
Thanks,

David
#21 lore raw

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

In reply to Paolo Bonzini
Quoted 64 lines · Paolo Bonzini
Quoted 63 lines · Eduardo Habkost
Quoted 62 lines · David Hildenbrand
Quoted 37 lines · Thomas Huth
Quoted 16 lines · David Hildenbrand

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

---
 include/hw/s390x/sclp.h    | 2 ++
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)
include/hw/s390x/sclp.h +0 -0
index a72d096081..4b86a8a293 100644

include/hw/s390x/sclp.h sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); · line 246

Quoted 5 lines
include/hw/s390x/sclp.h +2 -0
@@ -242,5 +242,7 @@ 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);
+typedef struct CPUS390XState CPUS390XState;
+int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);

That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html
https://stackoverflow.com/questions/8367646/redefinition-of-typedef

All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead:

Quoted 4 lines

https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs

Thomas

Yes, this is really nasty. And I wasn't aware of the involved issues.

This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes):

include/hw/s390x/sclp.h +2 -0
index a72d096081..ce80915a02 100644
@@ -242,5 +242,7 @@ 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);
+struct CPUS390XState;
+int sclp_service_call(struct CPUS390XState *env, uint64_t sccb,
 uint32_t code);
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

#endif

Why not use typedefs.h?

See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed.

I don't see an argument against moving typedef CPUS390XState to typedefs.h in Markus' reply. I see one argument for it (reducing the need for non-cyclic includes).

cpu-qom.h includes cpu.h, so I don't know why using S390CPU* would solve any problem. I don't disagree about changing the function to use S390CPU* eventually, but it would still require us make a choice between: a) including the header where the typedef name is declared (cpu.h or cpu-qom.h); or b) moving the typedef name declaration to typedefs.h.

Quoted 89 lines
Quoted 88 lines

---

include/hw/s390x/sclp.h +0 -1
index 4b86a8a293..3512bf8283 100644
@@ -242,7 +242,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);
-typedef struct CPUS390XState CPUS390XState;
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
include/qemu/typedefs.h +1 -0
index 39bc8351a3..9c97bffa92 100644
@@ -21,6 +21,7 @@ typedef struct Chardev Chardev;
 typedef struct CompatProperty CompatProperty;
 typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
+typedef struct CPUS390XState CPUS390XState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
 typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
target/s390x/cpu.h +2 -2
index 032d1de2e8..f99a82cd5e 100644
@@ -80,7 +80,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 +174,7 @@ typedef struct CPUS390XState {
     /* currently processed sigp order */
     uint8_t sigp_order;
 
-} CPUS390XState;
+};
 
 static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
 {
Quoted 38 lines
target/s390x/cpu.h +0 -1
index 3aa2e46aac..032d1de2e8 100644
@@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr,
 uint8_t ar, void *hostbuf,
 
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 

#endif

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

Opinions?

Signature · 3 lines
Thanks,

David
Signature · 1 line
Eduardo
#22 lore raw

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

Quoted 14 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

#endif

Why not use typedefs.h?

See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed.

I don't see an argument against moving typedef CPUS390XState to typedefs.h in Markus' reply. I see one argument for it (reducing the need for non-cyclic includes).

Quoted 6 lines

cpu-qom.h includes cpu.h, so I don't know why using S390CPU* would solve any problem. I don't disagree about changing the function to use S390CPU* eventually, but it would still require us make a choice between: a) including the header where the typedef name is declared (cpu.h or cpu-qom.h); or b) moving the typedef name declaration to typedefs.h.

It includes qom/cpu.h, not cpu.h. That's why using cpu-qom.h for such typedefs works (see v4).

Thanks!

Signature · 3 lines
Thanks,

David
#23 lore raw

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

Quoted 21 lines · David Hildenbrand
Quoted 14 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

#endif

Why not use typedefs.h?

See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed.

I don't see an argument against moving typedef CPUS390XState to typedefs.h in Markus' reply. I see one argument for it (reducing the need for non-cyclic includes).

Quoted 6 lines

cpu-qom.h includes cpu.h, so I don't know why using S390CPU* would solve any problem. I don't disagree about changing the function to use S390CPU* eventually, but it would still require us make a choice between: a) including the header where the typedef name is declared (cpu.h or cpu-qom.h); or b) moving the typedef name declaration to typedefs.h.

Quoted 3 lines

It includes qom/cpu.h, not cpu.h. That's why using cpu-qom.h for such typedefs works (see v4).

Oops, I was looking at an older tree (before commit 347b1a5c). You're right, sorry for the noise.

Signature · 1 line
Eduardo
#24 lore raw

PATCH v3 9/21 target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

In reply to David Hildenbrand

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

---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
target/s390x/excp_helper.c +1 -2
index 361f970db3..14d3160e92 100644
@@ -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;
Signature · 1 line
2.13.5
#25 lore raw

PATCH v3 10/21 target/s390x: use program_interrupt() in per_check_exception()

In reply to David Hildenbrand

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

---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
target/s390x/misc_helper.c +9 -6
index 8b07535b02..f3624d75eb 100644
@@ -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);
     }
 }
 
Signature · 1 line
2.13.5
#26 lore raw

PATCH v3 11/21 s390x: allow only 1 CPU with TCG

In reply to David Hildenbrand

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

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

---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
hw/s390x/s390-virtio-ccw.c +10 -0
index f67b4b5d58..f1198b2745 100644
@@ -23,6 +23,7 @@
 #include "hw/s390x/css.h"
 #include "virtio-ccw.h"
 #include "qemu/config-file.h"
+#include "qemu/error-report.h"
 #include "s390-pci-bus.h"
 #include "hw/s390x/storage-keys.h"
 #include "hw/s390x/storage-attributes.h"
@@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
     return cpu_states[cpu_addr];
 }
 
+/* #define S390_TCG_SMP_SUPPORT */
+
 static void s390_init_cpus(MachineState *machine)
 {
     int i;
@@ -55,6 +58,13 @@ static void s390_init_cpus(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = s390_default_cpu_model_name();
     }
+#ifndef S390_TCG_SMP_SUPPORT
+    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);
+    }
+#endif
 
     cpu_states = g_new0(S390CPU *, max_cpus);
 
Signature · 1 line
2.13.5
#27 lore raw

PATCH v3 12/21 target/s390x: set cpu->id for linux user when realizing

In reply to David Hildenbrand

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

---
 target/s390x/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
target/s390x/cpu.c +4 -7
index 74b3e4fd0d..5f9315fb16 100644
@@ -194,7 +194,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    ", max allowed: %d", cpu->id, max_cpus - 1);
         goto out;
     }
+#else
+    /* implicitly set for linux-user only */
+    cpu->id = scc->next_cpu_id;
 #endif
+
     if (cpu_exists(cpu->id)) {
         error_setg(&err, "Unable to add CPU: %" PRIi64
                    ", it already exists", cpu->id);
@@ -306,13 +310,6 @@ static void s390_cpu_initfn(Object *obj)
         inited = true;
         s390x_translate_init();
     }
-
-#if defined(CONFIG_USER_ONLY)
-    {
-        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
-        cpu->id = scc->next_cpu_id;
-    }
-#endif
 }
 
 static void s390_cpu_finalize(Object *obj)
Signature · 1 line
2.13.5
#28 lore raw

PATCH v3 13/21 target/s390x: use "core-id" for cpu number/address/id handling

In reply to 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".

---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c         | 71 +++++++++++++---------------------------------
 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, 29 insertions(+), 66 deletions(-)
hw/s390x/s390-virtio-ccw.c +1 -1
index f1198b2745..2086eb6ca6 100644
@@ -314,7 +314,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);
target/s390x/cpu.c +19 -52

125 lines not on this page · raw message

target/s390x/cpu.h +2 -3
index 032d1de2e8..ad88cbf368 100644
@@ -149,7 +149,7 @@ typedef 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;
@@ -193,7 +193,6 @@ typedef struct S390CPU {
     /*< public >*/
 
     CPUS390XState env;
-    int64_t id;
     S390CPUModel *model;
     /* needed for live migration */
     void *irqstate;
@@ -689,7 +688,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.  */
target/s390x/cpu_models.c +1 -1
index 18cbf91d9c..8e20e7637b 100644
@@ -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);
     }
 }
 
target/s390x/excp_helper.c +1 -1
index 14d3160e92..470cf8f5bc 100644
@@ -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);
 
target/s390x/helper.c +2 -2
index ba29504476..dfb24ef5b2 100644
@@ -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;
     }
target/s390x/misc_helper.c +2 -2
index f3624d75eb..293fc8428a 100644
@@ -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)) {
target/s390x/translate.c +1 -4
index 909b12818d..5abd34fb34 100644
@@ -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;
 }
 
Signature · 1 line
2.13.5
#29 lore raw

PATCH v3 14/21 target/s390x: rename next_cpu_id to next_core_id

In reply to David Hildenbrand

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

---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)
target/s390x/cpu-qom.h +1 -1
index c8fbf8ae03..21ea15e642 100644
@@ -52,7 +52,7 @@ typedef struct S390CPUClass {
     bool is_migration_safe;
     const char *desc;
 
-    int64_t next_cpu_id;
+    uint32_t next_core_id;
 
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
target/s390x/cpu.c +5 -6
index a2570bbc6b..105ff13034 100644
@@ -197,7 +197,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #else
     /* implicitly set for linux-user only */
-    cpu->env.core_id = scc->next_cpu_id;
+    cpu->env.core_id = scc->next_core_id;
 #endif
 
     if (cpu_exists(cpu->env.core_id)) {
@@ -205,10 +205,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    cpu->env.core_id);
         goto out;
     }
-    if (cpu->env.core_id != scc->next_cpu_id) {
+    if (cpu->env.core_id != scc->next_core_id) {
         error_setg(&err, "Unable to add CPU: %" PRIu32
-                   ", the next available nr is %" PRIi64, cpu->env.core_id,
-                   scc->next_cpu_id);
+                   ", the next available id is %" PRIu32, cpu->env.core_id,
+                   scc->next_core_id);
         goto out;
     }
 
@@ -218,7 +218,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     if (err != NULL) {
         goto out;
     }
-    scc->next_cpu_id++;
+    scc->next_core_id++;
 
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
@@ -463,7 +463,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(scc);
     DeviceClass *dc = DEVICE_CLASS(oc);
 
-    scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
Signature · 1 line
2.13.5
#30 lore raw

PATCH v3 15/21 s390x: print CPU definitions in sorted order

In reply to 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.

---
 target/s390x/cpu_models.c | 56 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 15 deletions(-)
target/s390x/cpu_models.c +41 -15
index 8e20e7637b..c295e641e6 100644
@@ -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++) {
Signature · 1 line
2.13.5
#31 lore raw

PATCH v3 16/21 s390x: allow cpu hotplug via device_add

In reply to 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."
---
 target/s390x/cpu.c | 1 +
 1 file changed, 1 insertion(+)
target/s390x/cpu.c +1 -0
index 105ff13034..be20c2fb0f 100644
@@ -466,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
+    dc->user_creatable = true;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)
Signature · 1 line
2.13.5
#32 lore raw

PATCH v3 17/21 s390x: CPU hot unplug via device_del cannot work for now

In reply to 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

---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
hw/s390x/s390-virtio-ccw.c +10 -0
index 2086eb6ca6..120f82e339 100644
@@ -341,6 +341,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)
 {
@@ -390,6 +399,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;
 }
 
Signature · 1 line
2.13.5
#33 lore raw

PATCH v3 18/21 s390x: implement query-hotpluggable-cpus

In reply to David Hildenbrand

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

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

While rewriting s390_cpu_addr2state() completely to be based on possible_cpus, move it to cpu.c, as it is independent of the virtio-ccw machine.

---
 hw/s390x/s390-virtio-ccw.c | 87 +++++++++++++++++++++++++++++-----------------
 qapi-schema.json           | 16 +++++++++
 target/s390x/cpu.c         | 17 +++++++++
 target/s390x/cpu.h         |  5 +--
 4 files changed, 89 insertions(+), 36 deletions(-)
hw/s390x/s390-virtio-ccw.c +55 -32
index 120f82e339..0e10a4c73a 100644
@@ -36,24 +36,12 @@
 #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];
-}
-
 /* #define S390_TCG_SMP_SUPPORT */
 
 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();
@@ -66,17 +54,8 @@ static void s390_init_cpus(MachineState *machine)
     }
 #endif
 
-    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);
 
     for (i = 0; i < smp_cpus; i++) {
         s390x_new_cpu(machine->cpu_model, i, &error_fatal);
@@ -307,17 +286,14 @@ static void ccw_init(MachineState *machine)
     register_savevm_live(NULL, "todclock", 0, 1, &savevm_gtod, NULL);
 }
 
-static void s390_cpu_plug(HotplugHandler *hotplug_dev,
-                        DeviceState *dev, Error **errp)
+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);
 }
 
 static void s390_machine_reset(void)
@@ -350,6 +326,50 @@ static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
     }
 }
 
+static CPUArchId *s390_find_cpu_slot(MachineState *ms, uint32_t core_id,
+                                     int *idx)
+{
+    if (core_id >= ms->possible_cpus->len) {
+        return NULL;
+    }
+    /* core_id corresponds to the index */
+    if (idx) {
+        *idx = core_id;
+    }
+    return &ms->possible_cpus->cpus[core_id];
+}
+
+static CpuInstanceProperties s390_cpu_index_to_props(MachineState *machine,
+                                                     unsigned cpu_index)
+{
+    CPUArchId *slot = s390_find_cpu_slot(machine, cpu_index, NULL);
+
+    assert(slot);
+    return slot->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)
 {
@@ -397,7 +417,10 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 248;
+    mc->has_hotpluggable_cpus = true;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
+    mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
+    mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
qapi-schema.json +16 -0
index f3af2cb851..79e9f85404 100644
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
 
target/s390x/cpu.c +17 -0
index be20c2fb0f..ad9dcdaaf1 100644
@@ -38,6 +38,7 @@
 #include "exec/exec-all.h"
 #include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
+#include "hw/boards.h"
 #include "hw/hw.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
@@ -445,6 +446,22 @@ void s390_enable_css_support(S390CPU *cpu)
         kvm_s390_enable_css_support(cpu);
     }
 }
+
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    static MachineState *ms;
+
+    if (!ms) {
+        ms = MACHINE(qdev_get_machine());
+        g_assert(ms->possible_cpus);
+    }
+
+    /* 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);
+}
 #endif
 
 static gchar *s390_gdb_arch_name(CPUState *cs)
target/s390x/cpu.h +1 -4
index ad88cbf368..5707268e43 100644
@@ -670,6 +670,7 @@ int s390_cpu_restart(S390CPU *cpu);
 void s390_enable_css_support(S390CPU *cpu);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
@@ -717,8 +718,4 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 #define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len)   \
         s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true)
 
-
-/* outside of target/s390x/ */
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-
 #endif
Signature · 1 line
2.13.5
#34 lore raw

PATCH v3 19/21 s390x: get rid of cpu_s390x_create()

In reply to 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

---
 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(-)
hw/s390x/s390-virtio-ccw.c +27 -2
index 0e10a4c73a..10f6933fbd 100644
@@ -41,6 +41,10 @@
 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) {
@@ -57,8 +61,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);
     }
 }
 
@@ -382,8 +403,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)
target/s390x/cpu.h +1 -1
index 5707268e43..a197869b0e 100644
@@ -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.  */
target/s390x/helper.c +2 -43

54 lines not on this page · raw message

target/s390x/internal.h +0 -1
index b4d3583b24..bc8f83129a 100644
@@ -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);
Signature · 1 line
2.13.5
#35 lore raw

PATCH v3 20/21 s390x: generate sclp cpu information from possible_cpus

In reply to 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.

---
 hw/s390x/sclp.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
hw/s390x/sclp.c +14 -20
index fd097262c7..30aefbfd15 100644
@@ -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);
 }
Signature · 1 line
2.13.5
#36 lore raw

PATCH v3 21/21 s390x: allow CPU hotplug in random core-id order

In reply to 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.

---
 target/s390x/cpu.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
target/s390x/cpu.c +1 -7
index ad9dcdaaf1..5701a41112 100644
@@ -199,6 +199,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #else
     /* implicitly set for linux-user only */
     cpu->env.core_id = scc->next_core_id;
+    scc->next_core_id++;
 #endif
 
     if (cpu_exists(cpu->env.core_id)) {
@@ -206,12 +207,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    cpu->env.core_id);
         goto out;
     }
-    if (cpu->env.core_id != scc->next_core_id) {
-        error_setg(&err, "Unable to add CPU: %" PRIu32
-                   ", the next available 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);
Signature · 1 line
2.13.5