s390x/cpumodel: wire up cpu type + id for TCG

PATCH Superseded David Hildenbrand 2 participants 5 messages Jun 1, 2017 – Jun 2, 2017

lore

Share & tools

lore

David Hildenbrand

author

2017-06-01 10:14 UTC #0 lore raw

PATCH 1/1 s390x/cpumodel: wire up cpu type + id for TCG

Let's properly expose the CPU type (machine-type number) via "STORE CPU ID" and "STORE SUBSYSTEM INFORMATION".

As TCG emulates basic mode, the CPU identification number has the format "Annnnn", whereby A is the CPU address, and n are parts of the CPU serial number (0 for us for now).

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

---

Tested stidp with a kvm-unit-test that is still being worked on (waiting for Thomas' interception test to integrate). I think we are missing quite some "operand alignment checks" in other handlers, too.

---
 target/s390x/cpu.h         |  1 -
 target/s390x/cpu_models.c  |  2 --
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  2 +-
 target/s390x/misc_helper.c | 26 +++++++++++++++++++++++---
 target/s390x/translate.c   | 11 ++++-------
 6 files changed, 29 insertions(+), 14 deletions(-)

target/s390x/cpu.h +0 -1

index c74b419..02bd8bf 100644
@@ -147,7 +147,6 @@ typedef struct CPUS390XState {
     CPU_COMMON
 
     uint32_t cpu_num;
-    uint32_t machine_type;
 
     uint64_t tod_offset;
     uint64_t tod_basetime;

target/s390x/cpu_models.c +0 -2

index b6220c8..99ec0c8 100644
@@ -710,8 +710,6 @@ static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 
     if (kvm_enabled()) {
         kvm_s390_apply_cpu_model(model, errp);
-    } else if (model) {
-        /* FIXME TCG - use data for stdip/stfl */
     }
 
     if (!*errp) {

target/s390x/helper.h +1 -0

index 0b70770..0c8f745 100644
@@ -121,6 +121,7 @@ DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64)
 DEF_HELPER_1(per_check_exception, void, env)
 DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_2(stidp, void, env, i64)
 
 DEF_HELPER_2(xsch, void, env, i64)
 DEF_HELPER_2(csch, void, env, i64)

target/s390x/insn-data.def +1 -1

index 55a7c52..83e7d01 100644
@@ -902,7 +902,7 @@
 /* STORE CPU ADDRESS */
     C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)
 /* STORE CPU ID */
-    C(0xb202, STIDP,   S,     Z,   la2, 0, new, m1_64, stidp, 0)
+    C(0xb202, STIDP,   S,     Z,   0, a2, 0, 0, stidp, 0)
 /* STORE CPU TIMER */
     C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64, stpt, 0)
 /* STORE FACILITY LIST */

target/s390x/misc_helper.c +23 -3

index 1b9f448..f682511 100644
@@ -383,6 +383,7 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
 uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
                       uint64_t r0, uint64_t r1)
 {
+    S390CPU *cpu = s390_env_get_cpu(env);
     int cc = 0;
     int sel1, sel2;

@@ -402,12 +403,14 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
         if ((sel1 == 1) && (sel2 == 1)) {
             /* Basic Machine Configuration */
             struct sysib_111 sysib;
+            char type[5] = {};
 
             memset(&sysib, 0, sizeof(sysib));
             ebcdic_put(sysib.manuf, "QEMU            ", 16);
-            /* same as machine type number in STORE CPU ID */
-            ebcdic_put(sysib.type, "QEMU", 4);
-            /* same as model number in STORE CPU ID */
+            /* same as machine type number in STORE CPU ID, but in EBCDIC */
+            snprintf(type, ARRAY_SIZE(type), "%X", cpu->model->def->type);
+            ebcdic_put(sysib.type, type, 4);
+            /* model number (not stored in STORE CPU ID for z/Architecure) */
             ebcdic_put(sysib.model, "QEMU            ", 16);
             ebcdic_put(sysib.sequence, "QEMU            ", 16);
             ebcdic_put(sysib.plant, "QEMU", 4);

@@ -736,3 +739,20 @@ uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr)
     env->regs[0] = deposit64(env->regs[0], 0, 8, max_m1);
     return (count_m1 >= max_m1 ? 0 : 3);
 }
+
+#ifndef CONFIG_USER_ONLY
+void HELPER(stidp)(CPUS390XState *env, uint64_t addr)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+    uint64_t cpuid = s390_cpuid_from_cpu_model(cpu->model);
+
+    if (addr & 0x7) {
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
+        return;
+    }
+
+    /* basic mode, write the cpu address into the first 4 bit of the ID */
+    cpuid |= ((uint64_t)env->cpu_num & 0xf) << 54;
+    cpu_stq_data(env, addr, cpuid);
+}
+#endif

target/s390x/translate.c +4 -7

index 4c48c59..1a99093 100644
@@ -3646,18 +3646,15 @@ static ExitStatus op_stctl(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+#ifndef CONFIG_USER_ONLY
 static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 t1 = tcg_temp_new_i64();
-
     check_privileged(s);
-    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
-    tcg_gen_ld32u_i64(t1, cpu_env, offsetof(CPUS390XState, machine_type));
-    tcg_gen_deposit_i64(o->out, o->out, t1, 32, 32);
-    tcg_temp_free_i64(t1);
-
+    potential_page_fault(s);
+    gen_helper_stidp(cpu_env, o->in2);
     return NO_EXIT;
 }
+#endif
 
 static ExitStatus op_spt(DisasContext *s, DisasOps *o)
 {

Signature · 1 line

2.9.3

Aurelien Jarno

2017-06-01 19:20 UTC #1 lore raw

Quoted 114 lines · David Hildenbrand

Let's properly expose the CPU type (machine-type number) via "STORE CPU ID" and "STORE SUBSYSTEM INFORMATION".

As TCG emulates basic mode, the CPU identification number has the format "Annnnn", whereby A is the CPU address, and n are parts of the CPU serial number (0 for us for now).

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

---

---
 target/s390x/cpu.h         |  1 -
 target/s390x/cpu_models.c  |  2 --
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  2 +-
 target/s390x/misc_helper.c | 26 +++++++++++++++++++++++---
 target/s390x/translate.c   | 11 ++++-------
 6 files changed, 29 insertions(+), 14 deletions(-)

target/s390x/cpu.h +0 -1

index c74b419..02bd8bf 100644
@@ -147,7 +147,6 @@ typedef struct CPUS390XState {
     CPU_COMMON
 
     uint32_t cpu_num;
-    uint32_t machine_type;
 
     uint64_t tod_offset;
     uint64_t tod_basetime;

target/s390x/cpu_models.c +0 -2

index b6220c8..99ec0c8 100644
@@ -710,8 +710,6 @@ static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 
     if (kvm_enabled()) {
         kvm_s390_apply_cpu_model(model, errp);
-    } else if (model) {
-        /* FIXME TCG - use data for stdip/stfl */
     }
 
     if (!*errp) {

target/s390x/helper.h +1 -0

index 0b70770..0c8f745 100644
@@ -121,6 +121,7 @@ DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64)
 DEF_HELPER_1(per_check_exception, void, env)
 DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_2(stidp, void, env, i64)
 
 DEF_HELPER_2(xsch, void, env, i64)
 DEF_HELPER_2(csch, void, env, i64)

target/s390x/insn-data.def +1 -1

index 55a7c52..83e7d01 100644
@@ -902,7 +902,7 @@
 /* STORE CPU ADDRESS */
     C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)
 /* STORE CPU ID */
-    C(0xb202, STIDP,   S,     Z,   la2, 0, new, m1_64, stidp, 0)
+    C(0xb202, STIDP,   S,     Z,   0, a2, 0, 0, stidp, 0)
 /* STORE CPU TIMER */
     C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64, stpt, 0)
 /* STORE FACILITY LIST */

target/s390x/misc_helper.c +13 -3

index 1b9f448..f682511 100644
@@ -383,6 +383,7 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
 uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
                       uint64_t r0, uint64_t r1)
 {
+    S390CPU *cpu = s390_env_get_cpu(env);
     int cc = 0;
     int sel1, sel2;

@@ -402,12 +403,14 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
         if ((sel1 == 1) && (sel2 == 1)) {
             /* Basic Machine Configuration */
             struct sysib_111 sysib;
+            char type[5] = {};
 
             memset(&sysib, 0, sizeof(sysib));
             ebcdic_put(sysib.manuf, "QEMU            ", 16);
-            /* same as machine type number in STORE CPU ID */
-            ebcdic_put(sysib.type, "QEMU", 4);
-            /* same as model number in STORE CPU ID */
+            /* same as machine type number in STORE CPU ID, but in EBCDIC */
+            snprintf(type, ARRAY_SIZE(type), "%X", cpu->model->def->type);
+            ebcdic_put(sysib.type, type, 4);
+            /* model number (not stored in STORE CPU ID for z/Architecure) */
             ebcdic_put(sysib.model, "QEMU            ", 16);
             ebcdic_put(sysib.sequence, "QEMU            ", 16);
             ebcdic_put(sysib.plant, "QEMU", 4);

@@ -736,3 +739,20 @@ uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr)
     env->regs[0] = deposit64(env->regs[0], 0, 8, max_m1);
     return (count_m1 >= max_m1 ? 0 : 3);
 }
+
+#ifndef CONFIG_USER_ONLY
+void HELPER(stidp)(CPUS390XState *env, uint64_t addr)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+    uint64_t cpuid = s390_cpuid_from_cpu_model(cpu->model);
+

target/s390x/misc_helper.c uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr) · line 758

Quoted 10 lines

target/s390x/misc_helper.c +10 -0

+    if (addr & 0x7) {
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
+        return;
+    }
+
+    /* basic mode, write the cpu address into the first 4 bit of the ID */
+    cpuid |= ((uint64_t)env->cpu_num & 0xf) << 54;
+    cpu_stq_data(env, addr, cpuid);
+}
+#endif

I don't really see the point of using an helper instead of just updating the existing code. From what I understand the cpuid does not change at runtime, so the s390_cpuid_from_cpu_model function can also be called from translate.c.

Aurelien

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

David Hildenbrand

author

2017-06-02 10:52 UTC #2 lore raw

Quoted 24 lines

Quoted 8 lines
target/s390x/misc_helper.c +7 -0
+
+#ifndef CONFIG_USER_ONLY
+void HELPER(stidp)(CPUS390XState *env, uint64_t addr)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+    uint64_t cpuid = s390_cpuid_from_cpu_model(cpu->model);
+
target/s390x/misc_helper.c uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr) · line 758
Quoted 10 lines
target/s390x/misc_helper.c +10 -0
+    if (addr & 0x7) {
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
+        return;
+    }
+
+    /* basic mode, write the cpu address into the first 4 bit of the ID */
+    cpuid |= ((uint64_t)env->cpu_num & 0xf) << 54;
+    cpu_stq_data(env, addr, cpuid);
+}
+#endif
I don't really see the point of using an helper instead of just updating the existing code. From what I understand the cpuid does not change at runtime, so the s390_cpuid_from_cpu_model function can also be called from translate.c.

Quoted 4 lines

Aurelien
From what I can see, conditional exceptions are more complicated to

implement without helpers (involves generating compares, jumps and so on). As this function is not expected to be executed on hot paths, I think moving it into a helper is the right thing to do.

Signature · 3 lines

Thanks,

David

Aurelien Jarno

2017-06-02 14:04 UTC #3 lore raw

Quoted 24 lines · David Hildenbrand

Quoted 24 lines
Quoted 8 lines
target/s390x/misc_helper.c +7 -0
+
+#ifndef CONFIG_USER_ONLY
+void HELPER(stidp)(CPUS390XState *env, uint64_t addr)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+    uint64_t cpuid = s390_cpuid_from_cpu_model(cpu->model);
+
target/s390x/misc_helper.c uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr) · line 758
Quoted 10 lines
target/s390x/misc_helper.c +10 -0
+    if (addr & 0x7) {
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
+        return;
+    }
+
+    /* basic mode, write the cpu address into the first 4 bit of the ID */
+    cpuid |= ((uint64_t)env->cpu_num & 0xf) << 54;
+    cpu_stq_data(env, addr, cpuid);
+}
+#endif
I don't really see the point of using an helper instead of just updating the existing code. From what I understand the cpuid does not change at runtime, so the s390_cpuid_from_cpu_model function can also be called from translate.c.

Quoted 5 lines

Quoted 2 lines
Aurelien
From what I can see, conditional exceptions are more complicated to implement without helpers (involves generating compares, jumps and so

In that case you don't need to do any compare an jump. It's a standard load/store alignement check, you can just specify the MO_ALIGN flag to the tcg_gen_qemu_st_i64 function.

Quoted 2 lines

on). As this function is not expected to be executed on hot paths, I think moving it into a helper is the right thing to do.

This is not only about performance, but also avoiding code that is spread in many files. Well theoretically increasing the number of entries in the helper hash table has a performance impact, but i don't think it is measurable.

Aurelien

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

David Hildenbrand

author

2017-06-02 14:28 UTC #4 lore raw

Quoted 28 lines · Aurelien Jarno

Quoted 27 lines · David Hildenbrand
Quoted 26 lines
Quoted 8 lines
target/s390x/misc_helper.c +7 -0
+
+#ifndef CONFIG_USER_ONLY
+void HELPER(stidp)(CPUS390XState *env, uint64_t addr)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+    uint64_t cpuid = s390_cpuid_from_cpu_model(cpu->model);
+
target/s390x/misc_helper.c uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr) · line 758
Quoted 10 lines
target/s390x/misc_helper.c +10 -0
+    if (addr & 0x7) {
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
+        return;
+    }
+
+    /* basic mode, write the cpu address into the first 4 bit of the ID */
+    cpuid |= ((uint64_t)env->cpu_num & 0xf) << 54;
+    cpu_stq_data(env, addr, cpuid);
+}
+#endif
I don't really see the point of using an helper instead of just updating the existing code. From what I understand the cpuid does not change at runtime, so the s390_cpuid_from_cpu_model function can also be called from translate.c.
Aurelien

Quoted 7 lines

Quoted 2 lines
From what I can see, conditional exceptions are more complicated to implement without helpers (involves generating compares, jumps and so
In that case you don't need to do any compare an jump. It's a standard load/store alignement check, you can just specify the MO_ALIGN flag to the tcg_gen_qemu_st_i64 function.

Thanks for the hint, will look into that. And also add low-address protection checks.

Signature · 3 lines

Thanks,

David