[Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[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 <david@redhat.com>
---

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(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index c74b419..02bd8bf 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index b6220c8..99ec0c8 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -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) {
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 0b70770..0c8f745 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -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)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 55a7c52..83e7d01 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -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 */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 1b9f448..f682511 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4c48c59..1a99093 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -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)
 {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[Aurelien Jarno]

On 2017-06-01 12:14, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
> 
> 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(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index c74b419..02bd8bf 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -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;
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index b6220c8..99ec0c8 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -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) {
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 0b70770..0c8f745 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -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)
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index 55a7c52..83e7d01 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -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 */
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 1b9f448..f682511 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -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

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

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[David Hildenbrand]

>> +
>> +#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
> 
> 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
> 

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

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[Aurelien Jarno]

On 2017-06-02 12:52, David Hildenbrand wrote:
> 
> >> +
> >> +#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
> > 
> > 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
> > 
> 
> 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.


> 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	

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v1] s390x/cpumodel: wire up cpu type + id for TCG

[David Hildenbrand]

On 02.06.2017 16:04, Aurelien Jarno wrote:
> On 2017-06-02 12:52, David Hildenbrand wrote:
>>
>>>> +
>>>> +#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
>>>
>>> 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
>>>
>>
>> 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.

-- 

Thanks,

David