s390x cleanups and CPU hotplug via device_add
v4 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.
Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places - 152e9f65d66f ("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.
v3 -> v4:
* Dropped "target/s390x: move typedef of S390CPU to its definition"
* Added "target/s390x: move some s390x typedefs to cpu-qom.h"
- this moves some typedefs and makes cpu-qom.h independent from cpu_models.h. This is necessary to cleanly use cpu-qom.h in sclp.c later.
* "s390x: move sclp_service_call() to sclp.h"
- Use cpu-qom.h now as a s390x replacement for typedefs.h
* "target/s390x: use "core-id" for cpu number/address/id handling"
- Indicate also in error messages the property name "core-id". So just the error messages were touched.
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
s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
s390x: move subsystem_reset() to s390-virtio-ccw.h target/s390x: move some s390x typedefs to cpu-qom.h
s390x: move sclp_service_call() to sclp.h target/s390x: use trigger_pgm_exception() in
s390_cpu_handle_mmu_fault()
target/s390x: use program_interrupt() in per_check_exception()
s390x: allow only 1 CPU with TCG
target/s390x: set cpu->id for linux user when realizing target/s390x: use "core-id" for cpu number/address/id handling target/s390x: rename next_cpu_id to next_core_id
s390x: print CPU definitions in sorted order
s390x: allow cpu hotplug via device_add
s390x: CPU hot unplug via device_del cannot work for now
s390x: implement query-hotpluggable-cpus
s390x: get rid of cpu_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 | 7 +- target/s390x/cpu.c | 102 ++++++--------- target/s390x/cpu.h | 18 +-- target/s390x/cpu_models.c | 58 ++++++--- target/s390x/cpu_models.h | 8 +- target/s390x/diag.c | 1 + target/s390x/excp_helper.c | 5 +- target/s390x/helper.c | 47 +------ target/s390x/internal.h | 1 - target/s390x/kvm.c | 1 + target/s390x/misc_helper.c | 21 ++-- target/s390x/translate.c | 5 +- 30 files changed, 410 insertions(+), 272 deletions(-) create mode 100644 hw/s390x/s390-virtio-hcall.h delete mode 100644 hw/s390x/s390-virtio.h
Signature · 1 line
2.13.5
PATCH v4 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v4 2/21 cpu: drop old comments describing members
In reply to David HildenbrandThese comments are obviously stale.
--- include/qom/cpu.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
include/qom/cpu.h +3 -3
@@ -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
PATCH v4 3/21 s390x: get rid of s390-virtio.c
In reply to David HildenbrandIt 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
@@ -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
@@ -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
@@ -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
PATCH v4 4/21 s390x: rename s390-virtio.h to s390-virtio-hcall.h
In reply to David HildenbrandThe 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
@@ -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
@@ -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
@@ -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
PATCH v4 5/21 s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
In reply to David HildenbrandImplemented 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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v4 6/21 s390x: move subsystem_reset() to s390-virtio-ccw.h
In reply to David HildenbrandImplemented 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
@@ -56,4 +56,6 @@ bool gs_allowed(void);*/ bool css_migration_enabled (void ); +void subsystem_reset (void ); +#endif
target/s390x/cpu.h +0 -1
@@ -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
@@ -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
PATCH v4 7/21 target/s390x: move some s390x typedefs to cpu-qom.h
In reply to David HildenbrandThis allows us to drop inclusion of cpu_models.h in cpu-qom.h, and prepares for using cpu-qom.h as a s390 specific version of typedefs.h
--- target/s390x/cpu-qom.h | 5 ++++- target/s390x/cpu.h | 5 +++-- target/s390x/cpu_models.h | 8 ++++---- 3 files changed, 11 insertions(+), 7 deletions(-)
target/s390x/cpu-qom.h +4 -1
@@ -21,7 +21,6 @@#define QEMU_S390_CPU_QOM_H #include "qom/cpu.h" -#include "cpu_models.h" #define TYPE_S390_CPU "s390-cpu"
@@ -32,6 +31,9 @@#define S390_CPU_GET_CLASS (obj) \ OBJECT_GET_CLASS(S390CPUClass, (obj), TYPE_S390_CPU) +typedef struct S390CPUModel S390CPUModel ; +typedef struct S390CPUDef S390CPUDef ; +/** * S390CPUClass: * @parent_realize: The parent class' realize handler.
@@ -62,5 +64,6 @@ typedef struct S390CPUClass { } S390CPUClass;typedef struct S390CPU S390CPU ; +typedef struct CPUS390XState CPUS390XState ;#endif
target/s390x/cpu.h +3 -2
@@ -25,6 +25,7 @@#include "qemu-common.h" #include "cpu-qom.h" +#include "cpu_models.h" #define TARGET_LONG_BITS 64
@@ -80,7 +81,7 @@ typedef struct MchkQueue {uint16_t type; } MchkQueue; -typedef struct CPUS390XState { +struct CPUS390XState {uint64_t regs[16 ];/* GP registers */ /* * The floating point registers are part of the vector registers.
@@ -174,7 +175,7 @@ typedef struct CPUS390XState {/* currently processed sigp order */ uint8_t sigp_order; -} CPUS390XState; +};static inline CPU_DoubleU *get_freg (CPUS390XState *cs,int nr) {
target/s390x/cpu_models.h +4 -4
@@ -17,7 +17,7 @@#include "qom/cpu.h" /* static CPU definition */ -typedef struct S390CPUDef { +struct S390CPUDef {const char *name;/* name exposed to the user */ const char *desc;/* description exposed to the user */ uint8_t gen;/* hw generation identification */
@@ -37,10 +37,10 @@ typedef struct S390CPUDef {S390FeatBitmap full_feat;/* used to init full_feat from generated data */ S390FeatInit full_init; -} S390CPUDef; +};/* CPU model based on a CPU definition */ -typedef struct S390CPUModel { +struct S390CPUModel {const S390CPUDef *def;S390FeatBitmap features;/* values copied from the "host" model, can change during migration */
@@ -48,7 +48,7 @@ typedef struct S390CPUModel {uint32_t cpu_id;/* CPU id */ uint8_t cpu_id_format;/* CPU id format bit */ uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */ -} S390CPUModel; +};/* * CPU ID
Signature · 1 line
2.13.5
PATCH v4 8/21 s390x: move sclp_service_call() to sclp.h
In reply to David HildenbrandImplemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of struct CPUS390XState to fix the two sclp consoles complaining.
--- include/hw/s390x/sclp.h | 2 ++ 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
@@ -16,6 +16,7 @@#include "hw/sysbus.h" #include "hw/qdev.h" +#include "target/s390x/cpu-qom.h" #define SCLP_CMD_CODE_MASK 0xffff00ff
@@ -242,5 +243,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void);sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev (void );void sclp_service_interrupt (uint32_t sccb);void raise_irq_cpu_hotplug (void ); +int sclp_service_call (CPUS390XState *env,uint64_t sccb,uint32_t code);#endif
target/s390x/cpu.h +0 -1
@@ -722,6 +722,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,/* outside of target/s390x/ */ S390CPU *s390_cpu_addr2state (uint16_t cpu_addr); -int sclp_service_call (CPUS390XState *env,uint64_t sccb,uint32_t code);#endif
target/s390x/misc_helper.c +1 -0
@@ -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
PATCH v4 9/21 target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()
In reply to David HildenbrandThis 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
@@ -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
PATCH v4 10/21 target/s390x: use program_interrupt() in per_check_exception()
In reply to David HildenbrandClean 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
@@ -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
PATCH v4 11/21 s390x: allow only 1 CPU with TCG
In reply to David HildenbrandSpecifying 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
@@ -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
Re: [Qemu-devel] [PATCH v4 11/21] s390x: allow only 1 CPU with TCG
Quoted 26 lines · On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <[email protected]> wrote:
Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.
Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.
Tested-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)hw/s390x/s390-virtio-ccw.c +1
-0@@ -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"
hw/s390x/s390-virtio-ccw.c S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) · line 51
Quoted 4 lines
hw/s390x/s390-virtio-ccw.c +1
-0@@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)return cpu_states[cpu_addr]; } +/* #define S390_TCG_SMP_SUPPORT */
I'd drop define and ifdef for something that doesn't exists
Quoted 17 lines
hw/s390x/s390-virtio-ccw.c +7
-0static 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);
Re: [Qemu-devel] [PATCH v4 11/21] s390x: allow only 1 CPU with TCG
Quoted 32 lines · Igor Mammedov
Quoted 29 lines · On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <[email protected]> wrote:
Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.
Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.
Tested-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)hw/s390x/s390-virtio-ccw.c +1
-0@@ -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]; }
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) · line 51
Quoted 1 line
hw/s390x/s390-virtio-ccw.c +1
-0+/* #define S390_TCG_SMP_SUPPORT */ I'd drop define and ifdef for something that doesn't exists
Conny requested it as we might see some work on that area (supporting smp) soon. So as long as there are no other opinions, I'll stick to the current version.
Thanks!
Signature · 3 lines
Thanks, David
Re: [Qemu-devel] [PATCH v4 11/21] s390x: allow only 1 CPU with TCG
Quoted 33 lines · On Tue, 12 Sep 2017 17:42:35 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Igor Mammedov
Quoted 29 lines · On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <[email protected]> wrote:
Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.
Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.
Tested-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)hw/s390x/s390-virtio-ccw.c +1
-0@@ -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]; }
Quoted 9 lines
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) · line 51
Quoted 1 line
hw/s390x/s390-virtio-ccw.c +1
-0+/* #define S390_TCG_SMP_SUPPORT */ I'd drop define and ifdef for something that doesn't exists
Conny requested it as we might see some work on that area (supporting smp) soon. So as long as there are no other opinions, I'll stick to the current version.
Thanks!
I've just removed a bunch of TODO cpu models in PPC with corresponding macro, my guess they were also introduced with similar intent but ended up as dead code.
So it's better to add new code when it actually is needed, instead of just in case.
Re: [Qemu-devel] [PATCH v4 11/21] s390x: allow only 1 CPU with TCG
Quoted 43 lines · Igor Mammedov
Quoted 40 lines · On Tue, 12 Sep 2017 17:42:35 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Igor Mammedov
Quoted 29 lines · On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <[email protected]> wrote:
Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.
Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.
Tested-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)hw/s390x/s390-virtio-ccw.c +1
-0@@ -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]; }Quoted 2 lines
hw/s390x/s390-virtio-ccw.c S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) · line 51
Quoted 1 line
hw/s390x/s390-virtio-ccw.c +1
-0+/* #define S390_TCG_SMP_SUPPORT */ I'd drop define and ifdef for something that doesn't exists
Conny requested it as we might see some work on that area (supporting smp) soon. So as long as there are no other opinions, I'll stick to the current version.
Quoted 9 lines
Quoted 2 lines
Thanks!
I've just removed a bunch of TODO cpu models in PPC with corresponding macro, my guess they were also introduced with similar intent but ended up as dead code.
So it's better to add new code when it actually is needed, instead of just in case.
v1 of this patch had no ifdef at all. So I'll let Conny decide whether to keep it like this or whether to drop the ifdef again.
Signature · 3 lines
Thanks, David
Re: [Qemu-devel] [PATCH v4 11/21] s390x: allow only 1 CPU with TCG
Quoted 50 lines · On Wed, 13 Sep 2017 14:10:53 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 49 lines · Igor Mammedov
Quoted 33 lines · On Tue, 12 Sep 2017 17:42:35 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Igor Mammedov
Quoted 29 lines · On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <[email protected]> wrote:
Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the guest tries to bring these CPUs up but fails), because we don't support multiple CPUs on s390x under TCG.
Let's bail out if more than 1 is specified, so we don't raise people's hope. Make it a define, so we can easily bump it up later.
Tested-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)hw/s390x/s390-virtio-ccw.c +1
-0@@ -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]; }Quoted 9 lines
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) · line 51
Quoted 1 line
hw/s390x/s390-virtio-ccw.c +1
-0+/* #define S390_TCG_SMP_SUPPORT */ I'd drop define and ifdef for something that doesn't exists
Conny requested it as we might see some work on that area (supporting smp) soon. So as long as there are no other opinions, I'll stick to the current version.
Thanks!
I've just removed a bunch of TODO cpu models in PPC with corresponding macro, my guess they were also introduced with similar intent but ended up as dead code.
Quoted 5 lines
Quoted 3 lines
So it's better to add new code when it actually is needed, instead of just in case.
v1 of this patch had no ifdef at all. So I'll let Conny decide whether to keep it like this or whether to drop the ifdef again.
You could add a comment? OTOH, this one hits immediately, so whoever implements smp for tcg will remember to change this :)
So no real objection to leaving out the ifdef.
PATCH v4 12/21 target/s390x: set cpu->id for linux user when realizing
In reply to David Hildenbrandscc->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
@@ -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
Re: [Qemu-devel] [PATCH v4 12/21] target/s390x: set cpu->id for linux user when realizing
Quoted 5 lines · On Mon, 11 Sep 2017 17:21:41 +0200 David Hildenbrand <[email protected]> wrote:
scc->next_cpu_idis updated when realizing. Setting it just before that point looks cleaner.Reviewed-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>
Quoted 34 lines
--- target/s390x/cpu.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)target/s390x/cpu.c +4
-7@@ -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)
PATCH v4 13/21 target/s390x: use "core-id" for cpu number/address/id handling
In reply to David HildenbrandSome 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 | 72 +++++++++++++--------------------------------- target/s390x/cpu.h | 5 ++-- target/s390x/cpu_models.c | 2 +- target/s390x/excp_helper.c | 2 +- target/s390x/helper.c | 4 +-- target/s390x/misc_helper.c | 4 +-- target/s390x/translate.c | 5 +--- 8 files changed, 30 insertions(+), 66 deletions(-)
hw/s390x/s390-virtio-ccw.c +1 -1
@@ -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 +20 -52
126 lines not on this page · raw message
target/s390x/cpu.h +2 -3
@@ -150,7 +150,7 @@ struct CPUS390XState {CPU_COMMON - uint32_t cpu_num; + uint32_t core_id; /* PoP "CPU address", same as cpu_index */uint64_t cpuid;uint64_t tod_offset;
@@ -194,7 +194,6 @@ struct S390CPU {/*< public >*/ CPUS390XState env; -int64_t id;S390CPUModel *model;/* needed for live migration */ void *irqstate;
@@ -690,7 +689,7 @@ const char *s390_default_cpu_model_name(void);/* helper.c */ #define cpu_init (cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model) -S390CPU *s390x_new_cpu (const char *cpu_model,int64_t id,Error **errp); +S390CPU *s390x_new_cpu (const char *cpu_model,uint32_t core_id,Error **errp);/* you can call this signal handler from your SIGBUS and SIGSEGV signal handlers to inform the virtual CPU of exceptions. non zero is returned if the signal was handled by the virtual CPU. */
target/s390x/cpu_models.c +1 -1
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Re: [Qemu-devel] [PATCH v4 13/21] target/s390x: use "core-id" for cpu number/address/id handling
Quoted 8 lines · On Mon, 11 Sep 2017 17:21:42 +0200 David Hildenbrand <[email protected]> wrote:
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_addlater on (x86 and ppc).
is there possibility that later The core (something that contains threads) would appear/exist in real hw?
(my concern here is that we would have to use some other name for it as renaming of already shipped public property name would be not an option)
Quoted 18 lines
We can get rid of
cpu->idnow. Keepcpu_indexandenv->core_idin sync.cpu_indexwas already implicitly used by e.g.cpu_exists(), so keeping both in sync seems to be the right thing to do.
cpu_indexwill now no longer automatically get set viacpu_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_addwill later need the reserved "id" property. Hotplugging a CPU on s390x will then be: "device_addhost-s390-cpu,id=cpu2,core-id=2".Reviewed-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>---
[...]
Quoted 27 lines
target/s390x/misc_helper.c +2
-2@@ -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 +0
-0
target/s390x/translate.c static ExitStatus op_ssm(DisasContext *s, DisasOps *o) · line 3828
Quoted 10 lines
target/s390x/translate.c +1
-4@@ -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 ; }
I see core_id is used in several instructions,
does it really have any influence on code executed by *-user target?
Re: [Qemu-devel] [PATCH v4 13/21] target/s390x: use "core-id" for cpu number/address/id handling
Quoted 7 lines · Igor Mammedov
Quoted 4 lines · On Mon, 11 Sep 2017 17:21:42 +0200 David Hildenbrand <[email protected]> wrote:
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.
Quoted 8 lines
Quoted 4 lines
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_addlater on (x86 and ppc).is there possibility that later The core (something that contains threads) would appear/exist in real hw?
(my concern here is that we would have to use some other name for it as renaming of already shipped public property name would be not an option)
There is the possibility (s390x has SMP support starting with z13), but it is really, really unlikely for KVM. There would be the chance of implementing this somewhen in the future for TCG (although I also doubt this will happen in the near future).
If so, I think there will only be one solution: a new machine type.
Faking threads now would be just plain wrong. So I think this should just be fine for now.
[...]
Quoted 31 lines
Quoted 31 lines
target/s390x/misc_helper.c +2
-2@@ -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 +0
-1@@ -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
Quoted 9 lines
target/s390x/translate.c static ExitStatus op_ssm(DisasContext *s, DisasOps *o) · line 3828
Quoted 6 lines
target/s390x/translate.c +1
-3-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 ; }I see
core_idis used in several instructions,
does it really have any influence on code executed by *-user target?
Nope, these should all be privileged instructions and therefore not relevant for -user targets.
Signature · 3 lines
Thanks, David
PATCH v4 14/21 target/s390x: rename next_cpu_id to next_core_id
In reply to David HildenbrandAdapt 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
@@ -54,7 +54,7 @@ typedef struct S390CPUClass {bool is_migration_safe;const char *desc; -int64_t next_cpu_id; +uint32_t next_core_id;DeviceRealize parent_realize;void (*parent_reset)(CPUState *cpu);
target/s390x/cpu.c +5 -6
@@ -198,7 +198,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) }#else /* implicitly set for linux-user only */ - cpu->env.core_id = scc->next_cpu_id; + cpu->env.core_id = scc->next_core_id;#endif if (cpu_exists (cpu->env.core_id)) {
@@ -206,10 +206,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) ", it already exists", cpu->env.core_id); goto out; } -if (cpu->env.core_id != scc->next_cpu_id) { +if (cpu->env.core_id != scc->next_core_id) {error_setg (&err,"Unable to add CPU with core-id: %" PRIu32 -", the next available core-id is %" PRIi64, cpu->env.core_id, - scc->next_cpu_id); +", the next available core-id is %" PRIu32, cpu->env.core_id, + scc->next_core_id); goto out; }
@@ -219,7 +219,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)if (err !=NULL ) { goto out; } - scc->next_cpu_id++; + scc->next_core_id++;#if !defined(CONFIG_USER_ONLY )qemu_register_reset (s390_cpu_machine_reset_cb, cpu);
@@ -464,7 +464,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)CPUClass *cc =CPU_CLASS (scc);DeviceClass *dc =DEVICE_CLASS (oc); - scc->next_cpu_id =0 ; scc->parent_realize = dc->realize; dc->realize = s390_cpu_realizefn; dc->props = s390x_cpu_properties;
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH v4 14/21] target/s390x: rename next_cpu_id to next_core_id
Quoted 15 lines · On Mon, 11 Sep 2017 17:21:43 +0200 David Hildenbrand <[email protected]> wrote:
Adapt to the new term "
core_id". While at it, fix the type and drop the initialization to 0 (which is superfluous).Reviewed-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu-qom.h | 2 +- target/s390x/cpu.c | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-)target/s390x/cpu-qom.h +0
-0
target/s390x/cpu-qom.h typedef struct S390CPUClass { · line 57
Quoted 5 lines
target/s390x/cpu-qom.h +1
-1@@ -54,7 +54,7 @@ typedef struct S390CPUClass {bool is_migration_safe;const char *desc; -int64_t next_cpu_id; +uint32_t next_core_id;
is it possible to hotplug cpus in out of order in real hw?
Quoted 47 lines
target/s390x/cpu-qom.h +0
-0DeviceRealize parent_realize;void (*parent_reset)(CPUState *cpu);target/s390x/cpu.c +5
-6@@ -198,7 +198,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) }#else /* implicitly set for linux-user only */ - cpu->env.core_id = scc->next_cpu_id; + cpu->env.core_id = scc->next_core_id;#endif if (cpu_exists (cpu->env.core_id)) {@@ -206,10 +206,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) ", it already exists", cpu->env.core_id); goto out; } -if (cpu->env.core_id != scc->next_cpu_id) { +if (cpu->env.core_id != scc->next_core_id) {error_setg (&err,"Unable to add CPU with core-id: %" PRIu32 -", the next available core-id is %" PRIi64, cpu->env.core_id, - scc->next_cpu_id); +", the next available core-id is %" PRIu32, cpu->env.core_id, + scc->next_core_id); goto out; }@@ -219,7 +219,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)if (err !=NULL ) { goto out; } - scc->next_cpu_id++; + scc->next_core_id++;#if !defined(CONFIG_USER_ONLY )qemu_register_reset (s390_cpu_machine_reset_cb, cpu);@@ -464,7 +464,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)CPUClass *cc =CPU_CLASS (scc);DeviceClass *dc =DEVICE_CLASS (oc); - scc->next_cpu_id =0 ; scc->parent_realize = dc->realize; dc->realize = s390_cpu_realizefn; dc->props = s390x_cpu_properties;
Re: [Qemu-devel] [PATCH v4 14/21] target/s390x: rename next_cpu_id to next_core_id
Quoted 21 lines · Igor Mammedov
Quoted 18 lines · On Mon, 11 Sep 2017 17:21:43 +0200 David Hildenbrand <[email protected]> wrote:
Adapt to the new term "
core_id". While at it, fix the type and drop the initialization to 0 (which is superfluous).Reviewed-by Matthew Rosato <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu-qom.h | 2 +- target/s390x/cpu.c | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-)target/s390x/cpu-qom.h +0
-0@@ -54,7 +54,7 @@ typedef struct S390CPUClass {bool is_migration_safe;const char *desc;
Quoted 3 lines
target/s390x/cpu-qom.h typedef struct S390CPUClass { · line 57
Quoted 2 lines
target/s390x/cpu-qom.h +1
-1-int64_t next_cpu_id; +uint32_t next_core_id;is it possible to hotplug cpus in out of order in real hw?
Yes, at least under z/VM. See last two patches in this series.
Signature · 3 lines
Thanks, David
PATCH v4 15/21 s390x: print CPU definitions in sorted order
In reply to David HildenbrandOther 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
@@ -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
Re: [Qemu-devel] [PATCH v4 15/21] s390x: print CPU definitions in sorted order
Quoted 6 lines · 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.
Signed-off-by David Hildenbrand <[email protected]>
looks much nicer now, with the most important ones (new ones) still on screen.
Quoted 89 lines
--- target/s390x/cpu_models.c | 56 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 15 deletions(-)target/s390x/cpu_models.c +41
-15@@ -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++) {
PATCH v4 16/21 s390x: allow cpu hotplug via device_add
In reply to David HildenbrandE.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
@@ -467,6 +467,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) scc->parent_realize = dc->realize; dc->realize = s390_cpu_realizefn; dc->props = s390x_cpu_properties; + dc->user_creatable = true; scc->parent_reset = cc->reset;#if !defined(CONFIG_USER_ONLY )
Signature · 1 line
2.13.5
PATCH v4 17/21 s390x: CPU hot unplug via device_del cannot work for now
In reply to David Hildenbranddevice_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 cpu1device_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
@@ -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
PATCH v4 18/21 s390x: implement query-hotpluggable-cpus
In reply to David HildenbrandCPU 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
@@ -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
@@ -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
@@ -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"
@@ -446,6 +447,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
@@ -671,6 +671,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
@@ -718,8 +719,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
Re: [Qemu-devel] [PATCH v4 18/21] s390x: implement query-hotpluggable-cpus
Quoted 8 lines · On Mon, 11 Sep 2017 17:21:47 +0200 David Hildenbrand <[email protected]> wrote:
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 variablecpu_states.While rewriting
s390_cpu_addr2state()completely to be based onpossible_cpus, move it tocpu.c, as it is independent of the virtio-ccw machine.
I'd split patch on
1) introduce possible cpus
2) rewrite s390_cpu_addr2state() using #2
Quoted 87 lines
Signed-off-by David Hildenbrand <[email protected]>--- 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 +10
-32@@ -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)
hw/s390x/s390-virtio-ccw.c static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, · line 340
Quoted 10 lines
hw/s390x/s390-virtio-ccw.c +10
-0+{ +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]; +}
it looks like cpu_index == core_id == idx in possible_cpus, is this helper really necessary?
(we have it in x86 because of possible not 1:1 mapping)
I'd drop it and just access array directly
Quoted 86 lines
hw/s390x/s390-virtio-ccw.c +35
-0+ +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@@ -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 +1
-0@@ -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"
target/s390x/cpu.c void s390_enable_css_support(S390CPU *cpu) · line 452
Quoted 6 lines
target/s390x/cpu.c +3
-0@@ -446,6 +447,22 @@ void s390_enable_css_support(S390CPU *cpu)kvm_s390_enable_css_support (cpu); } } + +S390CPU *s390_cpu_addr2state (uint16_t cpu_addr) +{
target/cpu.c and cpu itself preferably shouldn't pull in
or depend on machine, so I'd keep s390_cpu_addr2state() where it's now or somewhere in board related files
Quoted 37 lines
target/s390x/cpu.c +13
-0+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@@ -671,6 +671,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 @@ -718,8 +719,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
Re: [Qemu-devel] [PATCH v4 18/21] s390x: implement query-hotpluggable-cpus
Quoted 8 lines · Igor Mammedov
Quoted 5 lines · On Mon, 11 Sep 2017 17:21:47 +0200 David Hildenbrand <[email protected]> wrote:
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 variablecpu_states.
Quoted 6 lines
Quoted 3 lines
While rewriting
s390_cpu_addr2state()completely to be based onpossible_cpus, move it tocpu.c, as it is independent of the virtio-ccw machine.I'd split patch on
1) introduce possible cpus
2) rewrites390_cpu_addr2state()using #2
Than I have to keep the global variable + setting it for one patch. Might not be worth the trouble. Will have a look.
[...]
Quoted 17 lines
hw/s390x/s390-virtio-ccw.c static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, · line 340
Quoted 13 lines
hw/s390x/s390-virtio-ccw.c +12
-0+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]; +}it looks like
cpu_index==core_id== idx inpossible_cpus, is this helper really necessary?
(we have it in x86 because of possible not 1:1 mapping)
Quoted 1 line
I'd drop it and just access array directly
Just kept this because the other architectures also have this. I can of course drop it. The nice thing about this helper is the comment :)
[...]
Quoted 7 lines
target/s390x/cpu.c void s390_enable_css_support(S390CPU *cpu) · line 452
Quoted 4 lines
}
target/s390x/cpu.c +3
-0+ +S390CPU *s390_cpu_addr2state (uint16_t cpu_addr) +{
target/cpu.cand cpu itself preferably shouldn't pull in
or depend on machine, so I'd keeps390_cpu_addr2state()where it's now or somewhere in board related files
Thomas requested this. I actually don't care., but it looks like a generic "get_cpu_by_arch_id" function. But I don't really want to go that additional path now. And also I don't want to move this back and forth.
Thomas, what's your opinion?
Thanks!
Signature · 3 lines
Thanks, David
Re: [Qemu-devel] [PATCH v4 18/21] s390x: implement query-hotpluggable-cpus
Quoted 51 lines · David Hildenbrand
Quoted 8 lines · Igor Mammedov
Quoted 5 lines · On Mon, 11 Sep 2017 17:21:47 +0200 David Hildenbrand <[email protected]> wrote:
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 variablecpu_states.Quoted 6 lines
Quoted 3 lines
While rewriting
s390_cpu_addr2state()completely to be based onpossible_cpus, move it tocpu.c, as it is independent of the virtio-ccw machine.I'd split patch on
1) introduce possible cpus
2) rewrites390_cpu_addr2state()using #2Than I have to keep the global variable + setting it for one patch. Might not be worth the trouble. Will have a look.
[...]
Quoted 17 lines
hw/s390x/s390-virtio-ccw.c static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, · line 340
Quoted 13 lines
hw/s390x/s390-virtio-ccw.c +12
-0+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]; +}it looks like
cpu_index==core_id== idx inpossible_cpus, is this helper really necessary?
(we have it in x86 because of possible not 1:1 mapping)Quoted 1 line
I'd drop it and just access array directly
Just kept this because the other architectures also have this. I can of course drop it. The nice thing about this helper is the comment :)
[...]
Quoted 7 lines
target/s390x/cpu.c void s390_enable_css_support(S390CPU *cpu) · line 452
Quoted 4 lines
}
target/s390x/cpu.c +3
-0+ +S390CPU *s390_cpu_addr2state (uint16_t cpu_addr) +{
target/cpu.cand cpu itself preferably shouldn't pull in
or depend on machine, so I'd keeps390_cpu_addr2state()where it's now or somewhere in board related files
Quoted 5 lines
Thomas requested this. I actually don't care., but it looks like a generic "
get_cpu_by_arch_id" function. But I don't really want to go that additional path now. And also I don't want to move this back and forth.Thomas, what's your opinion?
Hmm, since the new version uses MachineState now, it's maybe really better to leave it in the hw/s390x/ directory, I guess. Sorry for the inconvenience ...
Thomas
PATCH v4 19/21 s390x: get rid of cpu_s390x_create()
In reply to David HildenbrandNow 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
@@ -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
@@ -690,7 +690,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
@@ -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
Re: [Qemu-devel] [PATCH v4 19/21] s390x: get rid of cpu_s390x_create()
Quoted 8 lines · On Mon, 11 Sep 2017 17:21:48 +0200 David Hildenbrand <[email protected]> wrote:
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 ins390_init_cpus()
- Parse features only once without having to remember if already parsed
- Pass only the typename tos390x_new_cpu()
- Use the typename of an existing CPU for hotplug via cpu-addSigned-off-by David Hildenbrand <[email protected]>
Quoted 147 lines
--- 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@@ -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@@ -690,7 +690,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@@ -68,52 +68,11 @@ void s390x_cpu_timer(void *opaque) }#endif -S390CPU *cpu_s390x_create (const char *cpu_model,Error **errp) +S390CPU *s390x_new_cpu (const char *typename,uint32_t core_id,Error **errp) { -static bool features_parsed; -char *name, *features; -const char *typename; -ObjectClass *oc; -CPUClass *cc; - - name =g_strdup (cpu_model); - features =strchr (name,',' ); -if (features) { - features[0 ] =0 ; - features++; - } - - oc =cpu_class_by_name (TYPE_S390_CPU , name); -if (!oc) { -error_setg (errp,"Unknown CPU definition \'%s\'" , name); -g_free (name); -return NULL ; - } - typename =object_class_get_name (oc); - -if (!features_parsed) { - features_parsed = true; - cc =CPU_CLASS (oc); - cc->parse_features (typename, features, errp); - } -g_free (name); - -if (*errp) { -return NULL ; - } -return S390_CPU (CPU (object_new (typename))); -} - -S390CPU *s390x_new_cpu (const char *cpu_model,uint32_t core_id,Error **errp) -{ -S390CPU *cpu; +S390CPU *cpu =S390_CPU (object_new (typename));Error *err =NULL ; - cpu =cpu_s390x_create (cpu_model, &err); -if (err !=NULL ) { - goto out; - } -object_property_set_int (OBJECT (cpu), core_id,"core-id" , &err);if (err !=NULL ) { goto out;target/s390x/internal.h +0
-1@@ -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);
PATCH v4 20/21 s390x: generate sclp cpu information from possible_cpus
In reply to David HildenbrandThis 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
@@ -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
PATCH v4 21/21 s390x: allow CPU hotplug in random core-id order
In reply to David HildenbrandSCLP 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
@@ -200,6 +200,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)) {
@@ -207,12 +208,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) ", it already exists", cpu->env.core_id); goto out; } -if (cpu->env.core_id != scc->next_core_id) { -error_setg (&err,"Unable to add CPU with core-id: %" PRIu32 -", the next available core-id is %" PRIu32, cpu->env.core_id, - scc->next_core_id); - goto out; - }/* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */ cs->cpu_index = env->core_id;
@@ -220,7 +215,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
Quoted 4 lines · On Mon, 11 Sep 2017 17:21:29 +0200 David Hildenbrand <[email protected]> wrote:
The first patches are a bunch of cleanups. I decided to go the extra mile and implement CPU hotplug via "
device_add", as well as "query-hotpluggable-cpus".
Quoted 8 lines
On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.
Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places -
152e9f65d66f("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.
do you plan to convert hw/s390x/s390-virtio.c machine to possible_cpus as well? it's not necessary to enable hotplug on it but sharing code and initializing cpus the same way would be plus.
[...]
Quoted 16 lines · Igor Mammedov
Quoted 4 lines · On Mon, 11 Sep 2017 17:21:29 +0200 David Hildenbrand <[email protected]> wrote:
The first patches are a bunch of cleanups. I decided to go the extra mile and implement CPU hotplug via "
device_add", as well as "query-hotpluggable-cpus".Quoted 8 lines
On s390x, only complete cores can be plugged. CPU hot unplug is currently not supported by the architecture.
Support for hotplugging CPUs in random core-id order (last two patches) is added. We once hat a KVM bug preventing this, but the stable patch should now be included in relevant places -
152e9f65d66f("KVM: s390: fix wrong lookup of VCPUs by array index"). Current tooling will plug them in sequential order anyway, so it should not hurt.
Quoted 3 lines
do you plan to convert
hw/s390x/s390-virtio.cmachine topossible_cpusas well? it's not necessary to enable hotplug on it but sharing code and initializing cpus the same way would be plus.
See patch #3. s390-virtio.c is an old leftover, the machine doesn't exist anymore. So this should be clean.
Signature · 3 lines
Thanks, David