[PATCH v2 00/10] target/s390x: pc-relative translation

[PATCH v2 00/10] target/s390x: pc-relative translation

[Richard Henderson]

v1: 20220906101747.344559-1-richard.henderson@linaro.org

A lot has changed in the 20 months since, including generic
cleanups and splitting out the PER fixes.


r~


Richard Henderson (10):
  target/s390x: Change help_goto_direct to work on displacements
  target/s390x: Introduce gen_psw_addr_disp
  target/s390x: Remove pc argument to pc_to_link_into
  target/s390x: Use gen_psw_addr_disp in pc_to_link_info
  target/s390x: Use gen_psw_addr_disp in save_link_info
  target/s390x: Use deposit in save_link_info
  target/s390x: Use gen_psw_addr_disp in op_sam
  target/s390x: Use ilen instead in branches
  target/s390x: Assert masking of psw.addr in cpu_get_tb_cpu_state
  target/s390x: Enable CF_PCREL

 target/s390x/cpu.c           |  23 +++++
 target/s390x/tcg/translate.c | 190 +++++++++++++++++++++--------------
 2 files changed, 138 insertions(+), 75 deletions(-)

-- 
2.34.1

[PATCH v2 01/10] target/s390x: Change help_goto_direct to work on displacements

[Richard Henderson]

In preparation for CF_PCREL, reduce reliance on absolute values.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index c81e035dea..f25ae02a4e 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1073,13 +1073,15 @@ struct DisasInsn {
 /* ====================================================================== */
 /* Miscellaneous helpers, used by several operations.  */
 
-static DisasJumpType help_goto_direct(DisasContext *s, uint64_t dest)
+static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp)
 {
+    uint64_t dest = s->base.pc_next + disp;
+
     update_cc_op(s);
     per_breaking_event(s);
     per_branch(s, tcg_constant_i64(dest));
 
-    if (dest == s->pc_tmp) {
+    if (disp == s->ilen) {
         return DISAS_NEXT;
     }
     if (use_goto_tb(s, dest)) {
@@ -1105,7 +1107,8 @@ static DisasJumpType help_goto_indirect(DisasContext *s, TCGv_i64 dest)
 static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
                                  bool is_imm, int imm, TCGv_i64 cdest)
 {
-    uint64_t dest = s->base.pc_next + (int64_t)imm * 2;
+    int64_t disp = (int64_t)imm * 2;
+    uint64_t dest = s->base.pc_next + disp;
     TCGLabel *lab;
 
     /* Take care of the special cases first.  */
@@ -1120,7 +1123,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
         if (c->cond == TCG_COND_ALWAYS
             || (dest == s->pc_tmp &&
                 !(s->base.tb->flags & FLAG_MASK_PER_BRANCH))) {
-            return help_goto_direct(s, dest);
+            return help_goto_direct(s, disp);
         }
     } else {
         if (!cdest) {
-- 
2.34.1

[PATCH v2 02/10] target/s390x: Introduce gen_psw_addr_disp

[Richard Henderson]

In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index f25ae02a4e..bd4ad33802 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -167,6 +167,11 @@ 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, uint64_t pc)
 {
     if (s->base.tb->flags & FLAG_MASK_32) {
@@ -337,8 +342,7 @@ static void store_freg32_i64(int reg, TCGv_i64 v)
 
 static void update_psw_addr(DisasContext *s)
 {
-    /* psw.addr */
-    tcg_gen_movi_i64(psw_addr, s->base.pc_next);
+    gen_psw_addr_disp(s, psw_addr, 0);
 }
 
 static void per_branch(DisasContext *s, TCGv_i64 dest)
@@ -352,7 +356,7 @@ static void per_branch(DisasContext *s, TCGv_i64 dest)
 
 static void per_breaking_event(DisasContext *s)
 {
-    tcg_gen_movi_i64(gbea, s->base.pc_next);
+    gen_psw_addr_disp(s, gbea, 0);
 }
 
 static void update_cc_op(DisasContext *s)
@@ -1086,11 +1090,11 @@ static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp)
     }
     if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(0);
-        tcg_gen_movi_i64(psw_addr, dest);
+        gen_psw_addr_disp(s, psw_addr, disp);
         tcg_gen_exit_tb(s->base.tb, 0);
         return DISAS_NORETURN;
     } else {
-        tcg_gen_movi_i64(psw_addr, dest);
+        gen_psw_addr_disp(s, psw_addr, disp);
         return DISAS_PC_CC_UPDATED;
     }
 }
@@ -1121,7 +1125,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
          * still need a conditional call to helper_per_branch.
          */
         if (c->cond == TCG_COND_ALWAYS
-            || (dest == s->pc_tmp &&
+            || (disp == s->ilen &&
                 !(s->base.tb->flags & FLAG_MASK_PER_BRANCH))) {
             return help_goto_direct(s, disp);
         }
@@ -1154,7 +1158,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
     /* Branch taken.  */
     per_breaking_event(s);
     if (is_imm) {
-        tcg_gen_movi_i64(psw_addr, dest);
+        gen_psw_addr_disp(s, psw_addr, disp);
     } else {
         tcg_gen_mov_i64(psw_addr, cdest);
     }
@@ -1170,7 +1174,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
     gen_set_label(lab);
 
     /* Branch not taken.  */
-    tcg_gen_movi_i64(psw_addr, s->pc_tmp);
+    gen_psw_addr_disp(s, psw_addr, s->ilen);
     if (use_goto_tb(s, s->pc_tmp)) {
         tcg_gen_goto_tb(1);
         tcg_gen_exit_tb(s->base.tb, 1);
@@ -5758,7 +5762,8 @@ static TCGv gen_ri2(DisasContext *s)
 
     disas_jdest(s, i2, is_imm, imm, ri2);
     if (is_imm) {
-        ri2 = tcg_constant_i64(s->base.pc_next + (int64_t)imm * 2);
+        ri2 = tcg_temp_new_i64();
+        gen_psw_addr_disp(s, ri2, (int64_t)imm * 2);
     }
 
     return ri2;
@@ -6367,7 +6372,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
             s->base.is_jmp = DISAS_PC_CC_UPDATED;
             /* fall through */
         case DISAS_NEXT:
-            tcg_gen_movi_i64(psw_addr, s->pc_tmp);
+            gen_psw_addr_disp(s, psw_addr, s->ilen);
             break;
         default:
             break;
-- 
2.34.1

[PATCH v2 03/10] target/s390x: Remove pc argument to pc_to_link_into

[Richard Henderson]

All callers pass s->pc_tmp.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index bd4ad33802..14162769a9 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -172,8 +172,10 @@ 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, uint64_t pc)
+static void pc_to_link_info(TCGv_i64 out, DisasContext *s)
 {
+    uint64_t pc = s->pc_tmp;
+
     if (s->base.tb->flags & FLAG_MASK_32) {
         if (s->base.tb->flags & FLAG_MASK_64) {
             tcg_gen_movi_i64(out, pc);
@@ -1404,7 +1406,7 @@ static DisasJumpType op_ni(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 {
-    pc_to_link_info(o->out, s, s->pc_tmp);
+    pc_to_link_info(o->out, s);
     if (o->in2) {
         return help_goto_indirect(s, o->in2);
     } else {
@@ -1417,7 +1419,7 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     TCGv_i64 t;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
-        pc_to_link_info(o->out, s, s->pc_tmp);
+        pc_to_link_info(o->out, s);
         return;
     }
     gen_op_calc_cc(s);
@@ -1474,7 +1476,7 @@ static DisasJumpType op_basi(DisasContext *s, DisasOps *o)
     bool is_imm;
     int imm;
 
-    pc_to_link_info(o->out, s, s->pc_tmp);
+    pc_to_link_info(o->out, s);
 
     disas_jdest(s, i2, is_imm, imm, o->in2);
     disas_jcc(s, &c, 0xf);
-- 
2.34.1

[PATCH v2 04/10] target/s390x: Use gen_psw_addr_disp in pc_to_link_info

[Richard Henderson]

This is slightly more complicated than a straight displacement
for 31 and 24-bit modes.  Dont bother with a cant-happen assert.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 14162769a9..2d611da8af 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -174,17 +174,19 @@ static void gen_psw_addr_disp(DisasContext *s, TCGv_i64 dest, int64_t disp)
 
 static void pc_to_link_info(TCGv_i64 out, DisasContext *s)
 {
-    uint64_t pc = s->pc_tmp;
+    TCGv_i64 tmp;
 
-    if (s->base.tb->flags & FLAG_MASK_32) {
-        if (s->base.tb->flags & FLAG_MASK_64) {
-            tcg_gen_movi_i64(out, pc);
-            return;
-        }
-        pc |= 0x80000000;
+    if (s->base.tb->flags & FLAG_MASK_64) {
+        gen_psw_addr_disp(s, out, s->ilen);
+        return;
     }
-    assert(!(s->base.tb->flags & FLAG_MASK_64));
-    tcg_gen_deposit_i64(out, out, tcg_constant_i64(pc), 0, 32);
+
+    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);
 }
 
 static TCGv_i64 psw_addr;
-- 
2.34.1

[PATCH v2 05/10] target/s390x: Use gen_psw_addr_disp in save_link_info

[Richard Henderson]

Trivial but non-mechanical conversion away from pc_tmp.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 2d611da8af..2654c85a8e 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1425,9 +1425,11 @@ static void save_link_info(DisasContext *s, DisasOps *o)
         return;
     }
     gen_op_calc_cc(s);
-    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-    tcg_gen_ori_i64(o->out, o->out, ((s->ilen / 2) << 30) | s->pc_tmp);
     t = tcg_temp_new_i64();
+    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
+    gen_psw_addr_disp(s, t, s->ilen);
+    tcg_gen_or_i64(o->out, o->out, t);
+    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
     tcg_gen_shri_i64(t, psw_mask, 16);
     tcg_gen_andi_i64(t, t, 0x0f000000);
     tcg_gen_or_i64(o->out, o->out, t);
-- 
2.34.1

[PATCH v2 06/10] target/s390x: Use deposit in save_link_info

[Richard Henderson]

Replace manual masking and oring with deposits.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 2654c85a8e..0f0688424f 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1418,24 +1418,32 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 t;
+    TCGv_i64 t1, t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
         pc_to_link_info(o->out, s);
         return;
     }
+
     gen_op_calc_cc(s);
-    t = tcg_temp_new_i64();
-    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-    gen_psw_addr_disp(s, t, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t, psw_mask, 16);
-    tcg_gen_andi_i64(t, t, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_extu_i32_i64(t, cc_op);
-    tcg_gen_shli_i64(t, t, 28);
-    tcg_gen_or_i64(o->out, o->out, t);
+    t1 = tcg_temp_new_i64();
+    t2 = tcg_temp_new_i64();
+
+    /* Shift program mask into place, garbage outside of [27:24]. */
+    tcg_gen_shri_i64(t1, psw_mask, 16);
+    /* Deposit pc to replace garbage bits below program mask. */
+    gen_psw_addr_disp(s, t2, s->ilen);
+    tcg_gen_deposit_i64(t1, t1, t2, 0, 24);
+    /*
+     * Deposit cc to replace garbage bits above program mask.
+     * Note that cc is in [0-3], thus [63:30] are set to zero.
+     */
+    tcg_gen_extu_i32_i64(t2, cc_op);
+    tcg_gen_deposit_i64(t1, t1, t2, 28, 64 - 28);
+    /* Install ilen. */
+    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
+
+    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.34.1

[PATCH v2 06/10 1/4] target/s390x: Rename local variable in save_link_info

[Philippe Mathieu-Daudé]

From: Richard Henderson <richard.henderson@linaro.org>

To simplify the following commits, rename 't' as 't2'.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 1/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index e1b1dd43e1..faa6d37c8e 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1417,24 +1417,25 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 t;
+    TCGv_i64 t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
         pc_to_link_info(o->out, s);
         return;
     }
+
     gen_op_calc_cc(s);
-    t = tcg_temp_new_i64();
+    t2 = tcg_temp_new_i64();
     tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-    gen_psw_addr_disp(s, t, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t);
+    gen_psw_addr_disp(s, t2, s->ilen);
+    tcg_gen_or_i64(o->out, o->out, t2);
     tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t, psw_mask, 16);
-    tcg_gen_andi_i64(t, t, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_extu_i32_i64(t, cc_op);
-    tcg_gen_shli_i64(t, t, 28);
-    tcg_gen_or_i64(o->out, o->out, t);
+    tcg_gen_shri_i64(t2, psw_mask, 16);
+    tcg_gen_andi_i64(t2, t2, 0x0f000000);
+    tcg_gen_or_i64(o->out, o->out, t2);
+    tcg_gen_extu_i32_i64(t2, cc_op);
+    tcg_gen_shli_i64(t2, t2, 28);
+    tcg_gen_or_i64(o->out, o->out, t2);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.45.2

[PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

[Philippe Mathieu-Daudé]

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 2/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index faa6d37c8e..53ec817e29 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
+    TCGv_i64 t1;
     TCGv_i64 t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
@@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     }
 
     gen_op_calc_cc(s);
+    t1 = tcg_temp_new_i64();
     t2 = tcg_temp_new_i64();
+
     tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
+
+    /* Shift program mask into place, garbage outside of [27:24]. */
+    tcg_gen_shri_i64(t1, psw_mask, 16);
+    /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t2);
+    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
     tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t2, psw_mask, 16);
-    tcg_gen_andi_i64(t2, t2, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t2);
     tcg_gen_extu_i32_i64(t2, cc_op);
     tcg_gen_shli_i64(t2, t2, 28);
     tcg_gen_or_i64(o->out, o->out, t2);
-- 
2.45.2

Re: [PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

[Richard Henderson]

On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
> [PMD: Split patch, part 2/4]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index faa6d37c8e..53ec817e29 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
>   
>   static void save_link_info(DisasContext *s, DisasOps *o)
>   {
> +    TCGv_i64 t1;
>       TCGv_i64 t2;
>   
>       if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
> @@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, DisasOps *o)
>       }
>   
>       gen_op_calc_cc(s);
> +    t1 = tcg_temp_new_i64();
>       t2 = tcg_temp_new_i64();
> +
>       tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
> +
> +    /* Shift program mask into place, garbage outside of [27:24]. */
> +    tcg_gen_shri_i64(t1, psw_mask, 16);
> +    /* Deposit pc to replace garbage bits below program mask. */
>       gen_psw_addr_disp(s, t2, s->ilen);
> -    tcg_gen_or_i64(o->out, o->out, t2);
> +    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);

This is incorrect, as you've lost the high 32-bits of out.


r~


>       tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
> -    tcg_gen_shri_i64(t2, psw_mask, 16);
> -    tcg_gen_andi_i64(t2, t2, 0x0f000000);
> -    tcg_gen_or_i64(o->out, o->out, t2);
>       tcg_gen_extu_i32_i64(t2, cc_op);
>       tcg_gen_shli_i64(t2, t2, 28);
>       tcg_gen_or_i64(o->out, o->out, t2);

Re: [PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

[Philippe Mathieu-Daudé]

On 10/9/24 01:50, Richard Henderson wrote:
> On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
>> [PMD: Split patch, part 2/4]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/s390x/tcg/translate.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
>> index faa6d37c8e..53ec817e29 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, 
>> DisasOps *o)
>>   static void save_link_info(DisasContext *s, DisasOps *o)
>>   {
>> +    TCGv_i64 t1;
>>       TCGv_i64 t2;
>>       if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
>> @@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, 
>> DisasOps *o)
>>       }
>>       gen_op_calc_cc(s);
>> +    t1 = tcg_temp_new_i64();
>>       t2 = tcg_temp_new_i64();
>> +
>>       tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
>> +
>> +    /* Shift program mask into place, garbage outside of [27:24]. */
>> +    tcg_gen_shri_i64(t1, psw_mask, 16);
>> +    /* Deposit pc to replace garbage bits below program mask. */
>>       gen_psw_addr_disp(s, t2, s->ilen);
>> -    tcg_gen_or_i64(o->out, o->out, t2);
>> +    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
> 
> This is incorrect, as you've lost the high 32-bits of out.

Ah, I felt something was not right but couldn't figure it out,
thanks for pointing at it.

The original patch is not trivial to review...

> 
> r~
> 
> 
>>       tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
>> -    tcg_gen_shri_i64(t2, psw_mask, 16);
>> -    tcg_gen_andi_i64(t2, t2, 0x0f000000);
>> -    tcg_gen_or_i64(o->out, o->out, t2);
>>       tcg_gen_extu_i32_i64(t2, cc_op);
>>       tcg_gen_shli_i64(t2, t2, 28);
>>       tcg_gen_or_i64(o->out, o->out, t2);
> 

[PATCH v2 06/10 3/4] target/s390x: Use deposit to set ilen in save_link_info

[Philippe Mathieu-Daudé]

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 3/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 53ec817e29..bfb7662329 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1429,17 +1429,19 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     t1 = tcg_temp_new_i64();
     t2 = tcg_temp_new_i64();
 
-    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-
     /* Shift program mask into place, garbage outside of [27:24]. */
     tcg_gen_shri_i64(t1, psw_mask, 16);
     /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
     tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
-    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
     tcg_gen_extu_i32_i64(t2, cc_op);
     tcg_gen_shli_i64(t2, t2, 28);
     tcg_gen_or_i64(o->out, o->out, t2);
+
+    /* Install ilen. */
+    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
+
+    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.45.2

Re: [PATCH v2 06/10 3/4] target/s390x: Use deposit to set ilen in save_link_info

[Richard Henderson]

On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
> [PMD: Split patch, part 3/4]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 53ec817e29..bfb7662329 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1429,17 +1429,19 @@ static void save_link_info(DisasContext *s, DisasOps *o)
>       t1 = tcg_temp_new_i64();
>       t2 = tcg_temp_new_i64();
>   
> -    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
> -
>       /* Shift program mask into place, garbage outside of [27:24]. */
>       tcg_gen_shri_i64(t1, psw_mask, 16);
>       /* Deposit pc to replace garbage bits below program mask. */
>       gen_psw_addr_disp(s, t2, s->ilen);
>       tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
> -    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
>       tcg_gen_extu_i32_i64(t2, cc_op);
>       tcg_gen_shli_i64(t2, t2, 28);
>       tcg_gen_or_i64(o->out, o->out, t2);
> +
> +    /* Install ilen. */
> +    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);

This is incorrect, as bits [63:28] of t1 are still garbage.

> +
> +    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);

You've fixed the missing high bits of out, though.

r~

>   }
>   
>   static DisasJumpType op_bal(DisasContext *s, DisasOps *o)

[PATCH v2 06/10 4/4] target/s390x: Use deposit to set cc_op in save_link_info

[Philippe Mathieu-Daudé]

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 4/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index bfb7662329..b9fdfdc85a 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1433,11 +1433,13 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     tcg_gen_shri_i64(t1, psw_mask, 16);
     /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
-    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
+    tcg_gen_deposit_i64(t1, t1, t2, 0, 24);
+    /*
+     * Deposit cc to replace garbage bits above program mask.
+     * Note that cc is in [0-3], thus [63:30] are set to zero.
+     */
     tcg_gen_extu_i32_i64(t2, cc_op);
-    tcg_gen_shli_i64(t2, t2, 28);
-    tcg_gen_or_i64(o->out, o->out, t2);
-
+    tcg_gen_deposit_i64(t1, t1, t2, 28, 64 - 28);
     /* Install ilen. */
     tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
 
-- 
2.45.2

[PATCH v2 07/10] target/s390x: Use gen_psw_addr_disp in op_sam

[Richard Henderson]

Complicated because we may now require a runtime jump.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 39 +++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 0f0688424f..bce9a0aeb0 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -3805,7 +3805,7 @@ static DisasJumpType op_sacf(DisasContext *s, DisasOps *o)
 static DisasJumpType op_sam(DisasContext *s, DisasOps *o)
 {
     int sam = s->insn->data;
-    TCGv_i64 tsam;
+    TCGLabel *fault = NULL;
     uint64_t mask;
 
     switch (sam) {
@@ -3820,20 +3820,35 @@ static DisasJumpType op_sam(DisasContext *s, DisasOps *o)
         break;
     }
 
-    /* Bizarre but true, we check the address of the current insn for the
-       specification exception, not the next to be executed.  Thus the PoO
-       documents that Bad Things Happen two bytes before the end.  */
-    if (s->base.pc_next & ~mask) {
-        gen_program_exception(s, PGM_SPECIFICATION);
-        return DISAS_NORETURN;
-    }
-    s->pc_tmp &= mask;
+    /*
+     * Bizarre but true, we check the address of the current insn for the
+     * specification exception, not the next to be executed.  Thus the PoO
+     * documents that Bad Things Happen two bytes before the end.
+     */
+    if (mask != -1) {
+        TCGv_i64 t = tcg_temp_new_i64();
+        fault = gen_new_label();
 
-    tsam = tcg_constant_i64(sam);
-    tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
+        gen_psw_addr_disp(s, t, 0);
+        tcg_gen_andi_i64(t, t, ~mask);
+        tcg_gen_brcondi_i64(TCG_COND_NE, t, 0, fault);
+    }
+
+    update_cc_op(s);
+
+    tcg_gen_deposit_i64(psw_mask, psw_mask, tcg_constant_i64(sam), 31, 2);
+
+    gen_psw_addr_disp(s, psw_addr, s->ilen);
+    tcg_gen_andi_i64(psw_addr, psw_addr, mask);
 
     /* Always exit the TB, since we (may have) changed execution mode.  */
-    return DISAS_TOO_MANY;
+    tcg_gen_lookup_and_goto_ptr();
+
+    if (mask != -1) {
+        gen_set_label(fault);
+        gen_program_exception(s, PGM_SPECIFICATION);
+    }
+    return DISAS_NORETURN;
 }
 
 static DisasJumpType op_sar(DisasContext *s, DisasOps *o)
-- 
2.34.1

[PATCH v2 08/10] target/s390x: Use ilen instead in branches

[Richard Henderson]

Remove the remaining uses of pc_tmp, and remove the variable.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index bce9a0aeb0..3014cbea4f 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -141,12 +141,6 @@ struct DisasContext {
     const DisasInsn *insn;
     DisasFields fields;
     uint64_t ex_value;
-    /*
-     * During translate_one(), pc_tmp is used to determine the instruction
-     * to be executed after base.pc_next - e.g. next sequential instruction
-     * or a branch target.
-     */
-    uint64_t pc_tmp;
     uint32_t ilen;
     enum cc_op cc_op;
     bool exit_to_mainloop;
@@ -344,11 +338,6 @@ static void store_freg32_i64(int reg, TCGv_i64 v)
     tcg_gen_st32_i64(v, tcg_env, freg32_offset(reg));
 }
 
-static void update_psw_addr(DisasContext *s)
-{
-    gen_psw_addr_disp(s, psw_addr, 0);
-}
-
 static void per_branch(DisasContext *s, TCGv_i64 dest)
 {
 #ifndef CONFIG_USER_ONLY
@@ -420,7 +409,7 @@ static void gen_program_exception(DisasContext *s, int code)
                    offsetof(CPUS390XState, int_pgm_ilen));
 
     /* update the psw */
-    update_psw_addr(s);
+    gen_psw_addr_disp(s, psw_addr, 0);
 
     /* Save off cc.  */
     update_cc_op(s);
@@ -1179,7 +1168,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
 
     /* Branch not taken.  */
     gen_psw_addr_disp(s, psw_addr, s->ilen);
-    if (use_goto_tb(s, s->pc_tmp)) {
+    if (use_goto_tb(s, s->base.pc_next + s->ilen)) {
         tcg_gen_goto_tb(1);
         tcg_gen_exit_tb(s->base.tb, 1);
         return DISAS_NORETURN;
@@ -2361,7 +2350,7 @@ static DisasJumpType op_ex(DisasContext *s, DisasOps *o)
         return DISAS_NORETURN;
     }
 
-    update_psw_addr(s);
+    gen_psw_addr_disp(s, psw_addr, 0);
     update_cc_op(s);
 
     if (r1 == 0) {
@@ -3085,7 +3074,7 @@ static DisasJumpType op_lpd(DisasContext *s, DisasOps *o)
 
     /* In a parallel context, stop the world and single step.  */
     if (tb_cflags(s->base.tb) & CF_PARALLEL) {
-        update_psw_addr(s);
+        gen_psw_addr_disp(s, psw_addr, 0);
         update_cc_op(s);
         gen_exception(EXCP_ATOMIC);
         return DISAS_NORETURN;
@@ -4379,7 +4368,7 @@ static DisasJumpType op_stura(DisasContext *s, DisasOps *o)
 
     if (s->base.tb->flags & FLAG_MASK_PER_STORE_REAL) {
         update_cc_op(s);
-        update_psw_addr(s);
+        gen_psw_addr_disp(s, psw_addr, 0);
         gen_helper_per_store_real(tcg_env, tcg_constant_i32(s->ilen));
         return DISAS_NORETURN;
     }
@@ -4611,7 +4600,7 @@ static DisasJumpType op_svc(DisasContext *s, DisasOps *o)
 {
     TCGv_i32 t;
 
-    update_psw_addr(s);
+    gen_psw_addr_disp(s, psw_addr, 0);
     update_cc_op(s);
 
     t = tcg_constant_i32(get_field(s, i1) & 0xff);
@@ -6193,7 +6182,6 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
             g_assert_not_reached();
         }
     }
-    s->pc_tmp = s->base.pc_next + ilen;
     s->ilen = ilen;
 
     /* We can't actually determine the insn format until we've looked up
@@ -6413,7 +6401,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 
 out:
     /* Advance to the next instruction.  */
-    s->base.pc_next = s->pc_tmp;
+    s->base.pc_next += s->ilen;
     return ret;
 }
 
@@ -6475,7 +6463,7 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_NORETURN:
         break;
     case DISAS_TOO_MANY:
-        update_psw_addr(dc);
+        gen_psw_addr_disp(dc, psw_addr, 0);
         /* FALLTHRU */
     case DISAS_PC_UPDATED:
         /* Next TB starts off with CC_OP_DYNAMIC, so make sure the
-- 
2.34.1

[PATCH v2 09/10] target/s390x: Assert masking of psw.addr in cpu_get_tb_cpu_state

[Richard Henderson]

When changing modes via SAM, we raise a specification exception if the
new PC is out of range.  The masking in s390x_tr_init_disas_context
was too late to be correct, but may be removed.  Add a debugging
assert in cpu_get_tb_cpu_state.

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/cpu.c           | 6 ++++++
 target/s390x/tcg/translate.c | 6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 2bbeaca36e..c786767bd1 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -358,6 +358,12 @@ void cpu_get_tb_cpu_state(CPUS390XState *env, vaddr *pc,
         flags |= FLAG_MASK_VECTOR;
     }
     *pflags = flags;
+
+    if (!(flags & FLAG_MASK_32)) {
+        tcg_debug_assert(*pc <= 0x00ffffff);
+    } else if (!(flags & FLAG_MASK_64)) {
+        tcg_debug_assert(*pc <= 0x7fffffff);
+    }
 }
 
 static const TCGCPUOps s390_tcg_ops = {
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 3014cbea4f..0ee14484d0 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6409,11 +6409,7 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
 
-    /* 31-bit mode */
-    if (!(dc->base.tb->flags & FLAG_MASK_64)) {
-        dc->base.pc_first &= 0x7fffffff;
-        dc->base.pc_next = dc->base.pc_first;
-    }
+    /* Note cpu_get_tb_cpu_state asserts PC is masked for the mode. */
 
     dc->cc_op = CC_OP_DYNAMIC;
     dc->ex_value = dc->base.tb->cs_base;
-- 
2.34.1

[PATCH v2 10/10] target/s390x: Enable CF_PCREL

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/cpu.c           | 17 +++++++++
 target/s390x/tcg/translate.c | 71 +++++++++++++++++++++++-------------
 2 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c786767bd1..9f03190c35 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -39,6 +39,7 @@
 #include "sysemu/reset.h"
 #endif
 #include "hw/s390x/cpu-topology.h"
+#include "exec/translation-block.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -111,6 +112,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 CF_PCREL. */
+    if (!(tb_cflags(tb) & CF_PCREL)) {
+        CPUS390XState *env = cpu_env(cs);
+        env->psw.addr = tb->pc;
+    }
+}
+
 static void s390_cpu_set_pc(CPUState *cs, vaddr value)
 {
     S390CPU *cpu = S390_CPU(cs);
@@ -246,6 +257,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
     Error *err = NULL;
 
+#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
+    /* Use pc-relative instructions in system-mode */
+    cs->tcg_cflags |= CF_PCREL;
+#endif
+
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
     s390_realize_cpu_model(cs, &err);
     if (err) {
@@ -368,6 +384,7 @@ void cpu_get_tb_cpu_state(CPUS390XState *env, vaddr *pc,
 
 static const 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 0ee14484d0..6961ad7c67 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -139,6 +139,7 @@ struct DisasFields {
 struct DisasContext {
     DisasContextBase base;
     const DisasInsn *insn;
+    target_ulong pc_save;
     DisasFields fields;
     uint64_t ex_value;
     uint32_t ilen;
@@ -161,28 +162,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);
-}
-
 static TCGv_i64 psw_addr;
 static TCGv_i64 psw_mask;
 static TCGv_i64 gbea;
@@ -338,6 +317,34 @@ static void store_freg32_i64(int reg, TCGv_i64 v)
     tcg_gen_st32_i64(v, tcg_env, freg32_offset(reg));
 }
 
+static void gen_psw_addr_disp(DisasContext *s, TCGv_i64 dest, int64_t disp)
+{
+    assert(s->pc_save != -1);
+    if (tb_cflags(s->base.tb) & CF_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);
+}
+
 static void per_branch(DisasContext *s, TCGv_i64 dest)
 {
 #ifndef CONFIG_USER_ONLY
@@ -1081,13 +1088,13 @@ static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp)
     if (disp == s->ilen) {
         return DISAS_NEXT;
     }
+    gen_psw_addr_disp(s, psw_addr, disp);
     if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(0);
-        gen_psw_addr_disp(s, psw_addr, disp);
         tcg_gen_exit_tb(s->base.tb, 0);
         return DISAS_NORETURN;
     } else {
-        gen_psw_addr_disp(s, psw_addr, disp);
+        s->pc_save = dest;
         return DISAS_PC_CC_UPDATED;
     }
 }
@@ -1097,6 +1104,7 @@ static DisasJumpType help_goto_indirect(DisasContext *s, TCGv_i64 dest)
     update_cc_op(s);
     per_breaking_event(s);
     tcg_gen_mov_i64(psw_addr, dest);
+    s->pc_save = -1;
     per_branch(s, psw_addr);
     return DISAS_PC_CC_UPDATED;
 }
@@ -1173,6 +1181,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
         tcg_gen_exit_tb(s->base.tb, 1);
         return DISAS_NORETURN;
     }
+    s->pc_save = s->base.pc_next + s->ilen;
     return DISAS_PC_CC_UPDATED;
 }
 
@@ -2351,6 +2360,7 @@ static DisasJumpType op_ex(DisasContext *s, DisasOps *o)
     }
 
     gen_psw_addr_disp(s, psw_addr, 0);
+    s->pc_save = s->base.pc_next;
     update_cc_op(s);
 
     if (r1 == 0) {
@@ -6411,6 +6421,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 = dc->ex_value;
@@ -6423,9 +6434,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 (tb_cflags(dc->base.tb) & CF_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);
 }
 
 static target_ulong get_next_pc(CPUS390XState *env, DisasContext *s,
@@ -6517,7 +6532,11 @@ void s390x_restore_state_to_opc(CPUState *cs,
     CPUS390XState *env = cpu_env(cs);
     int cc_op = data[1];
 
-    env->psw.addr = data[0];
+    if (tb_cflags(tb) & CF_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)) {
-- 
2.34.1

Re: [PATCH v2 10/10] target/s390x: Enable CF_PCREL

[Pierrick Bouvier]

On 6/5/24 14:57, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/cpu.c           | 17 +++++++++
>   target/s390x/tcg/translate.c | 71 +++++++++++++++++++++++-------------
>   2 files changed, 62 insertions(+), 26 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c786767bd1..9f03190c35 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -39,6 +39,7 @@
>   #include "sysemu/reset.h"
>   #endif
>   #include "hw/s390x/cpu-topology.h"
> +#include "exec/translation-block.h"
>   
>   #define CR0_RESET       0xE0UL
>   #define CR14_RESET      0xC2000000UL;
> @@ -111,6 +112,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 CF_PCREL. */
> +    if (!(tb_cflags(tb) & CF_PCREL)) {
> +        CPUS390XState *env = cpu_env(cs);
> +        env->psw.addr = tb->pc;
> +    }
> +}
> +
>   static void s390_cpu_set_pc(CPUState *cs, vaddr value)
>   {
>       S390CPU *cpu = S390_CPU(cs);
> @@ -246,6 +257,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>       S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>       Error *err = NULL;
>   
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> +    /* Use pc-relative instructions in system-mode */
> +    cs->tcg_cflags |= CF_PCREL;
> +#endif
> +
>       /* the model has to be realized before qemu_init_vcpu() due to kvm */
>       s390_realize_cpu_model(cs, &err);
>       if (err) {
> @@ -368,6 +384,7 @@ void cpu_get_tb_cpu_state(CPUS390XState *env, vaddr *pc,
>   
>   static const 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 0ee14484d0..6961ad7c67 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -139,6 +139,7 @@ struct DisasFields {
>   struct DisasContext {
>       DisasContextBase base;
>       const DisasInsn *insn;
> +    target_ulong pc_save;
>       DisasFields fields;
>       uint64_t ex_value;
>       uint32_t ilen;
> @@ -161,28 +162,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);
> -}
> -
>   static TCGv_i64 psw_addr;
>   static TCGv_i64 psw_mask;
>   static TCGv_i64 gbea;
> @@ -338,6 +317,34 @@ static void store_freg32_i64(int reg, TCGv_i64 v)
>       tcg_gen_st32_i64(v, tcg_env, freg32_offset(reg));
>   }
>   
> +static void gen_psw_addr_disp(DisasContext *s, TCGv_i64 dest, int64_t disp)
> +{
> +    assert(s->pc_save != -1);
> +    if (tb_cflags(s->base.tb) & CF_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);
> +}
> +
>   static void per_branch(DisasContext *s, TCGv_i64 dest)
>   {
>   #ifndef CONFIG_USER_ONLY
> @@ -1081,13 +1088,13 @@ static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp)
>       if (disp == s->ilen) {
>           return DISAS_NEXT;
>       }
> +    gen_psw_addr_disp(s, psw_addr, disp);
>       if (use_goto_tb(s, dest)) {
>           tcg_gen_goto_tb(0);
> -        gen_psw_addr_disp(s, psw_addr, disp);
>           tcg_gen_exit_tb(s->base.tb, 0);
>           return DISAS_NORETURN;
>       } else {
> -        gen_psw_addr_disp(s, psw_addr, disp);
> +        s->pc_save = dest;
>           return DISAS_PC_CC_UPDATED;
>       }
>   }
> @@ -1097,6 +1104,7 @@ static DisasJumpType help_goto_indirect(DisasContext *s, TCGv_i64 dest)
>       update_cc_op(s);
>       per_breaking_event(s);
>       tcg_gen_mov_i64(psw_addr, dest);
> +    s->pc_save = -1;
>       per_branch(s, psw_addr);
>       return DISAS_PC_CC_UPDATED;
>   }
> @@ -1173,6 +1181,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
>           tcg_gen_exit_tb(s->base.tb, 1);
>           return DISAS_NORETURN;
>       }
> +    s->pc_save = s->base.pc_next + s->ilen;
>       return DISAS_PC_CC_UPDATED;
>   }
>   
> @@ -2351,6 +2360,7 @@ static DisasJumpType op_ex(DisasContext *s, DisasOps *o)
>       }
>   
>       gen_psw_addr_disp(s, psw_addr, 0);
> +    s->pc_save = s->base.pc_next;
>       update_cc_op(s);
>   
>       if (r1 == 0) {
> @@ -6411,6 +6421,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 = dc->ex_value;
> @@ -6423,9 +6434,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 (tb_cflags(dc->base.tb) & CF_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);
>   }
>   
>   static target_ulong get_next_pc(CPUS390XState *env, DisasContext *s,
> @@ -6517,7 +6532,11 @@ void s390x_restore_state_to_opc(CPUState *cs,
>       CPUS390XState *env = cpu_env(cs);
>       int cc_op = data[1];
>   
> -    env->psw.addr = data[0];
> +    if (tb_cflags(tb) & CF_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)) {

I'm not an expert on s390x, but based on implementation of CF_PCREL for 
other arch, it seems correct.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v2 00/10] target/s390x: pc-relative translation

[Richard Henderson]

Ping.  It rebases onto master just fine.

r~

On 6/5/24 14:57, Richard Henderson wrote:
> v1: 20220906101747.344559-1-richard.henderson@linaro.org
> 
> A lot has changed in the 20 months since, including generic
> cleanups and splitting out the PER fixes.
> 
> 
> r~
> 
> 
> Richard Henderson (10):
>    target/s390x: Change help_goto_direct to work on displacements
>    target/s390x: Introduce gen_psw_addr_disp
>    target/s390x: Remove pc argument to pc_to_link_into
>    target/s390x: Use gen_psw_addr_disp in pc_to_link_info
>    target/s390x: Use gen_psw_addr_disp in save_link_info
>    target/s390x: Use deposit in save_link_info
>    target/s390x: Use gen_psw_addr_disp in op_sam
>    target/s390x: Use ilen instead in branches
>    target/s390x: Assert masking of psw.addr in cpu_get_tb_cpu_state
>    target/s390x: Enable CF_PCREL
> 
>   target/s390x/cpu.c           |  23 +++++
>   target/s390x/tcg/translate.c | 190 +++++++++++++++++++++--------------
>   2 files changed, 138 insertions(+), 75 deletions(-)
> 

Re: [PATCH v2 00/10] target/s390x: pc-relative translation

[Richard Henderson]

Ping.

On 7/4/24 15:26, Richard Henderson wrote:
> Ping.  It rebases onto master just fine.
> 
> r~
> 
> On 6/5/24 14:57, Richard Henderson wrote:
>> v1: 20220906101747.344559-1-richard.henderson@linaro.org
>>
>> A lot has changed in the 20 months since, including generic
>> cleanups and splitting out the PER fixes.
>>
>>
>> r~
>>
>>
>> Richard Henderson (10):
>>    target/s390x: Change help_goto_direct to work on displacements
>>    target/s390x: Introduce gen_psw_addr_disp
>>    target/s390x: Remove pc argument to pc_to_link_into
>>    target/s390x: Use gen_psw_addr_disp in pc_to_link_info
>>    target/s390x: Use gen_psw_addr_disp in save_link_info
>>    target/s390x: Use deposit in save_link_info
>>    target/s390x: Use gen_psw_addr_disp in op_sam
>>    target/s390x: Use ilen instead in branches
>>    target/s390x: Assert masking of psw.addr in cpu_get_tb_cpu_state
>>    target/s390x: Enable CF_PCREL
>>
>>   target/s390x/cpu.c           |  23 +++++
>>   target/s390x/tcg/translate.c | 190 +++++++++++++++++++++--------------
>>   2 files changed, 138 insertions(+), 75 deletions(-)
>>