diff mbox series

[v4,27/27] target/s390x: Enable TARGET_TB_PCREL

Message ID 20230220184052.163465-28-richard.henderson@linaro.org
State New
Headers show
Series target/s390x: pc-relative translation blocks | expand

Commit Message

Richard Henderson Feb. 20, 2023, 6:40 p.m. UTC
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/cpu-param.h     |  4 ++
 target/s390x/cpu.c           | 12 +++++
 target/s390x/tcg/translate.c | 86 +++++++++++++++++++++++-------------
 3 files changed, 71 insertions(+), 31 deletions(-)

Comments

Thomas Huth Feb. 21, 2023, 2:35 p.m. UTC | #1
On 20/02/2023 19.40, Richard Henderson wrote:
> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/cpu-param.h     |  4 ++
>   target/s390x/cpu.c           | 12 +++++
>   target/s390x/tcg/translate.c | 86 +++++++++++++++++++++++-------------
>   3 files changed, 71 insertions(+), 31 deletions(-)
> 
> diff --git a/target/s390x/cpu-param.h b/target/s390x/cpu-param.h
> index bf951a002e..52bb95de57 100644
> --- a/target/s390x/cpu-param.h
> +++ b/target/s390x/cpu-param.h
> @@ -14,4 +14,8 @@
>   #define TARGET_VIRT_ADDR_SPACE_BITS 64
>   #define NB_MMU_MODES 4
>   
> +#ifndef CONFIG_USER_ONLY
> +# define TARGET_TB_PCREL 1
> +#endif
> +
>   #endif
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index b10a8541ff..933ff06395 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -37,6 +37,7 @@
>   #ifndef CONFIG_USER_ONLY
>   #include "sysemu/reset.h"
>   #endif
> +#include "exec/exec-all.h"
>   
>   #define CR0_RESET       0xE0UL
>   #define CR14_RESET      0xC2000000UL;
> @@ -83,6 +84,16 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
>       return r;
>   }
>   
> +static void s390_cpu_synchronize_from_tb(CPUState *cs,
> +                                         const TranslationBlock *tb)
> +{
> +    /* The program counter is always up to date with TARGET_TB_PCREL. */
> +    if (!TARGET_TB_PCREL) {
> +        S390CPU *cpu = S390_CPU(cs);
> +        cpu->env.psw.addr = tb_pc(tb);
> +    }
> +}

FYI, this causes a build failure with --disable-tcg:

  https://gitlab.com/thuth/qemu/-/jobs/3806828645#L1885

I'll add this to fix it, no need to respin:

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -84,6 +84,7 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
      return r;
  }
  
+#ifdef CONFIG_TCG
  static void s390_cpu_synchronize_from_tb(CPUState *cs,
                                           const TranslationBlock *tb)
  {
@@ -93,6 +94,7 @@ static void s390_cpu_synchronize_from_tb(CPUState *cs,
          cpu->env.psw.addr = tb_pc(tb);
      }
  }
+#endif
  
  static void s390_cpu_set_pc(CPUState *cs, vaddr value)
  {

  Thomas
Richard Henderson Feb. 21, 2023, 3:53 p.m. UTC | #2
On 2/21/23 04:35, Thomas Huth wrote:
> 
> FYI, this causes a build failure with --disable-tcg:
> 
> https://gitlab.com/thuth/qemu/-/jobs/3806828645#L1885
> 
> I'll add this to fix it, no need to respin:

Thanks.  I should have thought to test that.


r~
Thomas Huth Feb. 27, 2023, 11:41 a.m. UTC | #3
On 21/02/2023 15.35, Thomas Huth wrote:
> On 20/02/2023 19.40, Richard Henderson wrote:
>> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/s390x/cpu-param.h     |  4 ++
>>   target/s390x/cpu.c           | 12 +++++
>>   target/s390x/tcg/translate.c | 86 +++++++++++++++++++++++-------------
>>   3 files changed, 71 insertions(+), 31 deletions(-)
>>
>> diff --git a/target/s390x/cpu-param.h b/target/s390x/cpu-param.h
>> index bf951a002e..52bb95de57 100644
>> --- a/target/s390x/cpu-param.h
>> +++ b/target/s390x/cpu-param.h
>> @@ -14,4 +14,8 @@
>>   #define TARGET_VIRT_ADDR_SPACE_BITS 64
>>   #define NB_MMU_MODES 4
>> +#ifndef CONFIG_USER_ONLY
>> +# define TARGET_TB_PCREL 1
>> +#endif
>> +
>>   #endif
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index b10a8541ff..933ff06395 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -37,6 +37,7 @@
>>   #ifndef CONFIG_USER_ONLY
>>   #include "sysemu/reset.h"
>>   #endif
>> +#include "exec/exec-all.h"
>>   #define CR0_RESET       0xE0UL
>>   #define CR14_RESET      0xC2000000UL;
>> @@ -83,6 +84,16 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
>>       return r;
>>   }
>> +static void s390_cpu_synchronize_from_tb(CPUState *cs,
>> +                                         const TranslationBlock *tb)
>> +{
>> +    /* The program counter is always up to date with TARGET_TB_PCREL. */
>> +    if (!TARGET_TB_PCREL) {
>> +        S390CPU *cpu = S390_CPU(cs);
>> +        cpu->env.psw.addr = tb_pc(tb);
>> +    }
>> +}
> 
> FYI, this causes a build failure with --disable-tcg:
> 
>   https://gitlab.com/thuth/qemu/-/jobs/3806828645#L1885
> 
> I'll add this to fix it, no need to respin:

FYI, I've unqueued this series from my current pull request again since it's 
causing trouble with the avocado-system-centos and avocado-system-opensuse 
CI jobs ... I'll try to pin-point the failing patch later, but since the 
rest of my current queue is quite long already, I really want to get that 
big queue merged first.

  Thomas
Thomas Huth March 2, 2023, 1:03 p.m. UTC | #4
On 27/02/2023 12.41, Thomas Huth wrote:
> On 21/02/2023 15.35, Thomas Huth wrote:
>> On 20/02/2023 19.40, Richard Henderson wrote:
>>> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   target/s390x/cpu-param.h     |  4 ++
>>>   target/s390x/cpu.c           | 12 +++++
>>>   target/s390x/tcg/translate.c | 86 +++++++++++++++++++++++-------------
>>>   3 files changed, 71 insertions(+), 31 deletions(-)
...
> FYI, I've unqueued this series from my current pull request again since it's 
> causing trouble with the avocado-system-centos and avocado-system-opensuse 
> CI jobs ... I'll try to pin-point the failing patch later, but since the 
> rest of my current queue is quite long already, I really want to get that 
> big queue merged first.

  Hi Richard,

I've bisected the issue with the failing CI jobs now, and it is indeed this 
last patch here ("Enable TARGET_TB_PCREL") that is causing the failure.

Everything is still green with "Pass original r2 register to BCR":

  https://gitlab.com/thuth/qemu/-/pipelines/794022206

But as soon as I queue "Enable TARGET_TB_PCREL", the avocado jobs start failing:

  https://gitlab.com/thuth/qemu/-/pipelines/793154874

Could you please have a look at the last patch to see what's going wrong here?

... and since soft freeze is approaching: Do you want me to queue the 
remaining patches except the last one, or does the series only make sense as 
a whole?

  Thomas
Richard Henderson March 2, 2023, 6:05 p.m. UTC | #5
On 3/2/23 03:03, Thomas Huth wrote:
> Could you please have a look at the last patch to see what's going wrong here?

Will do.

> ... and since soft freeze is approaching: Do you want me to queue the remaining patches 
> except the last one, or does the series only make sense as a whole?

Let's defer it all to next cycle.


r~
diff mbox series

Patch

diff --git a/target/s390x/cpu-param.h b/target/s390x/cpu-param.h
index bf951a002e..52bb95de57 100644
--- a/target/s390x/cpu-param.h
+++ b/target/s390x/cpu-param.h
@@ -14,4 +14,8 @@ 
 #define TARGET_VIRT_ADDR_SPACE_BITS 64
 #define NB_MMU_MODES 4
 
+#ifndef CONFIG_USER_ONLY
+# define TARGET_TB_PCREL 1
+#endif
+
 #endif
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index b10a8541ff..933ff06395 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -37,6 +37,7 @@ 
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/reset.h"
 #endif
+#include "exec/exec-all.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -83,6 +84,16 @@  uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
     return r;
 }
 
+static void s390_cpu_synchronize_from_tb(CPUState *cs,
+                                         const TranslationBlock *tb)
+{
+    /* The program counter is always up to date with TARGET_TB_PCREL. */
+    if (!TARGET_TB_PCREL) {
+        S390CPU *cpu = S390_CPU(cs);
+        cpu->env.psw.addr = tb_pc(tb);
+    }
+}
+
 static void s390_cpu_set_pc(CPUState *cs, vaddr value)
 {
     S390CPU *cpu = S390_CPU(cs);
@@ -274,6 +285,7 @@  static void s390_cpu_reset_full(DeviceState *dev)
 
 static const struct TCGCPUOps s390_tcg_ops = {
     .initialize = s390x_translate_init,
+    .synchronize_from_tb = s390_cpu_synchronize_from_tb,
     .restore_state_to_opc = s390x_restore_state_to_opc,
 
 #ifdef CONFIG_USER_ONLY
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index e4db5f1c02..a016d23d53 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -139,6 +139,7 @@  struct DisasContext {
     DisasContextBase base;
     const DisasInsn *insn;
     TCGOp *insn_start;
+    target_ulong pc_save;
     DisasFields fields;
     uint64_t ex_value;
     uint32_t ilen;
@@ -163,29 +164,6 @@  static uint64_t inline_branch_hit[CC_OP_MAX];
 static uint64_t inline_branch_miss[CC_OP_MAX];
 #endif
 
-static void gen_psw_addr_disp(DisasContext *s, TCGv_i64 dest, int64_t disp)
-{
-    tcg_gen_movi_i64(dest, s->base.pc_next + disp);
-}
-
-static void pc_to_link_info(TCGv_i64 out, DisasContext *s)
-{
-    TCGv_i64 tmp;
-
-    if (s->base.tb->flags & FLAG_MASK_64) {
-        gen_psw_addr_disp(s, out, s->ilen);
-        return;
-    }
-
-    tmp = tcg_temp_new_i64();
-    gen_psw_addr_disp(s, tmp, s->ilen);
-    if (s->base.tb->flags & FLAG_MASK_32) {
-        tcg_gen_ori_i64(tmp, tmp, 0x80000000);
-    }
-    tcg_gen_deposit_i64(out, out, tmp, 0, 32);
-    tcg_temp_free_i64(tmp);
-}
-
 static TCGv_i64 psw_addr;
 static TCGv_i64 psw_mask;
 static TCGv_i64 gbea;
@@ -348,9 +326,39 @@  static void return_low128(TCGv_i64 dest)
     tcg_gen_ld_i64(dest, cpu_env, offsetof(CPUS390XState, retxl));
 }
 
+static void gen_psw_addr_disp(DisasContext *s, TCGv_i64 dest, int64_t disp)
+{
+    assert(s->pc_save != -1);
+    if (TARGET_TB_PCREL) {
+        disp += s->base.pc_next - s->pc_save;
+        tcg_gen_addi_i64(dest, psw_addr, disp);
+    } else {
+        tcg_gen_movi_i64(dest, s->base.pc_next + disp);
+    }
+}
+
+static void pc_to_link_info(TCGv_i64 out, DisasContext *s)
+{
+    TCGv_i64 tmp;
+
+    if (s->base.tb->flags & FLAG_MASK_64) {
+        gen_psw_addr_disp(s, out, s->ilen);
+        return;
+    }
+
+    tmp = tcg_temp_new_i64();
+    gen_psw_addr_disp(s, tmp, s->ilen);
+    if (s->base.tb->flags & FLAG_MASK_32) {
+        tcg_gen_ori_i64(tmp, tmp, 0x80000000);
+    }
+    tcg_gen_deposit_i64(out, out, tmp, 0, 32);
+    tcg_temp_free_i64(tmp);
+}
+
 static void update_psw_addr_disp(DisasContext *s, int64_t disp)
 {
     gen_psw_addr_disp(s, psw_addr, disp);
+    s->pc_save = s->base.pc_next + disp;
 }
 
 static inline bool per_enabled(DisasContext *s)
@@ -1188,6 +1196,7 @@  static DisasJumpType help_goto_indirect(DisasContext *s, TCGv_i64 dest)
 {
     per_breaking_event(s);
     tcg_gen_mov_i64(psw_addr, dest);
+    s->pc_save = -1;
     per_branch_dest(s, psw_addr);
     return DISAS_PC_UPDATED;
 }
@@ -1197,6 +1206,7 @@  static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
 {
     DisasJumpType ret;
     int64_t disp = (int64_t)imm * 2;
+    TCGv_i64 cdest_save = NULL;
     TCGLabel *lab;
 
     /* Take care of the special cases first.  */
@@ -1229,12 +1239,12 @@  static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
     update_cc_op(s);
 
     /*
-     * Store taken branch destination before the brcond.  This
-     * avoids having to allocate a new local temp to hold it.
-     * We'll overwrite this in the not taken case anyway.
+     * Save taken branch destination across the brcond if required.
      */
     if (!is_imm) {
-        tcg_gen_mov_i64(psw_addr, cdest);
+        cdest_save = tcg_temp_local_new_i64();
+        tcg_gen_mov_i64(cdest_save, cdest);
+        cdest = cdest_save;
     }
 
     lab = gen_new_label();
@@ -1250,6 +1260,11 @@  static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
     per_breaking_event(s);
     if (is_imm) {
         gen_psw_addr_disp(s, psw_addr, disp);
+    } else {
+        tcg_gen_mov_i64(psw_addr, cdest);
+    }
+    if (cdest_save) {
+        tcg_temp_free_i64(cdest_save);
     }
     per_branch_dest(s, psw_addr);
 
@@ -1263,15 +1278,15 @@  static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
     gen_set_label(lab);
 
     /* Branch not taken.  */
+    gen_psw_addr_disp(s, psw_addr, s->ilen);
     if (use_goto_tb(s, s->base.pc_next + s->ilen)) {
         tcg_gen_goto_tb(1);
-        gen_psw_addr_disp(s, psw_addr, s->ilen);
         tcg_gen_exit_tb(s->base.tb, 1);
     } else {
-        gen_psw_addr_disp(s, psw_addr, s->ilen);
         tcg_gen_lookup_and_goto_ptr();
     }
 
+    s->pc_save = -1;
     ret = DISAS_NORETURN;
 
  egress:
@@ -6523,6 +6538,7 @@  static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 
     /* Note cpu_get_tb_cpu_state asserts PC is masked for the mode. */
 
+    dc->pc_save = dc->base.pc_first;
     dc->cc_op = CC_OP_DYNAMIC;
     dc->ex_value = dc->base.tb->cs_base;
     dc->exit_to_mainloop = per_enabled(dc) || dc->ex_value;
@@ -6535,9 +6551,13 @@  static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
 static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
+    target_ulong pc_arg = dc->base.pc_next;
 
+    if (TARGET_TB_PCREL) {
+        pc_arg &= ~TARGET_PAGE_MASK;
+    }
     /* Delay the set of ilen until we've read the insn. */
-    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0);
+    tcg_gen_insn_start(pc_arg, dc->cc_op, 0);
     dc->insn_start = tcg_last_op();
 }
 
@@ -6631,7 +6651,11 @@  void s390x_restore_state_to_opc(CPUState *cs,
     CPUS390XState *env = &cpu->env;
     int cc_op = data[1];
 
-    env->psw.addr = data[0];
+    if (TARGET_TB_PCREL) {
+        env->psw.addr = (env->psw.addr & TARGET_PAGE_MASK) | data[0];
+    } else {
+        env->psw.addr = data[0];
+    }
 
     /* Update the CC opcode if it is not already up-to-date.  */
     if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) {