Re: [RESEND PATCH v5 4/6] target/riscv: Add support for PC-relative translation

Sun, 02 Apr 2023 06:54:48 -0700 


On 2023/4/2 21:17, LIU Zhiwei wrote:


On 2023/4/2 16:17, liweiwei wrote:


On 2023/4/2 08:34, LIU Zhiwei wrote:


On 2023/4/1 20:49, Weiwei Li wrote:

Add a base save_pc For

pc_save for

PC-relative translation(CF_PCREL).
Diable the directly sync pc from tb by riscv_cpu_synchronize_from_tb.
Sync pc before it's used or updated from tb related pc:
    real_pc = (old)env->pc + target_pc(from tb) - ctx->save_pc

pc_save in the code.

OK. I'll fix this.

Use gen_get_target_pc to compute target address of auipc and successor
address of jalr.

Signed-off-by: Weiwei Li <liwei...@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqi...@iscas.ac.cn>
Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
---
  target/riscv/cpu.c                      | 29 +++++++++-----
  target/riscv/insn_trans/trans_rvi.c.inc | 14 +++++--

  target/riscv/translate.c                | 53 
+++++++++++++++++++++----

  3 files changed, 75 insertions(+), 21 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..646fa31a59 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -658,16 +658,18 @@ static vaddr riscv_cpu_get_pc(CPUState *cs)
  static void riscv_cpu_synchronize_from_tb(CPUState *cs,

                                            const TranslationBlock 
*tb)

  {
-    RISCVCPU *cpu = RISCV_CPU(cs);
-    CPURISCVState *env = &cpu->env;
-    RISCVMXL xl = FIELD_EX32(tb->flags, TB_FLAGS, XL);
+    if (!(tb_cflags(tb) & CF_PCREL)) {
+        RISCVCPU *cpu = RISCV_CPU(cs);
+        CPURISCVState *env = &cpu->env;
+        RISCVMXL xl = FIELD_EX32(tb->flags, TB_FLAGS, XL);
  -    tcg_debug_assert(!(cs->tcg_cflags & CF_PCREL));
+        tcg_debug_assert(!(cs->tcg_cflags & CF_PCREL));
  -    if (xl == MXL_RV32) {
-        env->pc = (int32_t) tb->pc;
-    } else {
-        env->pc = tb->pc;
+        if (xl == MXL_RV32) {
+            env->pc = (int32_t) tb->pc;
+        } else {
+            env->pc = tb->pc;
+        }
      }
  }

  @@ -693,11 +695,18 @@ static void 
riscv_restore_state_to_opc(CPUState *cs,

      RISCVCPU *cpu = RISCV_CPU(cs);
      CPURISCVState *env = &cpu->env;
      RISCVMXL xl = FIELD_EX32(tb->flags, TB_FLAGS, XL);
+    target_ulong pc;
+
+    if (tb_cflags(tb) & CF_PCREL) {
+        pc = (env->pc & TARGET_PAGE_MASK) | data[0];
+    } else {
+        pc = data[0];
+    }
        if (xl == MXL_RV32) {
-        env->pc = (int32_t)data[0];
+        env->pc = (int32_t)pc;
      } else {
-        env->pc = data[0];
+        env->pc = pc;
      }
      env->bins = data[1];
  }

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
b/target/riscv/insn_trans/trans_rvi.c.inc

index 48c73cfcfe..52ef260eff 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -38,7 +38,9 @@ static bool trans_lui(DisasContext *ctx, arg_lui *a)
    static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
  {
-    gen_set_gpri(ctx, a->rd, a->imm + ctx->base.pc_next);
+    TCGv target_pc = dest_gpr(ctx, a->rd);
+    gen_get_target_pc(target_pc, ctx, a->imm + ctx->base.pc_next);
+    gen_set_gpr(ctx, a->rd, target_pc);
      return true;
  }

  @@ -52,6 +54,7 @@ static bool trans_jalr(DisasContext *ctx, 
arg_jalr *a)

  {
      TCGLabel *misaligned = NULL;
      TCGv target_pc = tcg_temp_new();
+    TCGv succ_pc = dest_gpr(ctx, a->rd);

        tcg_gen_addi_tl(target_pc, get_gpr(ctx, a->rs1, EXT_NONE), 
a->imm);

      tcg_gen_andi_tl(target_pc, target_pc, (target_ulong)-2);

@@ -68,7 +71,9 @@ static bool trans_jalr(DisasContext *ctx, 
arg_jalr *a)

          tcg_gen_brcondi_tl(TCG_COND_NE, t0, 0x0, misaligned);
      }
  -    gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
+    gen_get_target_pc(succ_pc, ctx, ctx->pc_succ_insn);
+    gen_set_gpr(ctx, a->rd, succ_pc);
+
      tcg_gen_mov_tl(cpu_pc, target_pc);
      lookup_and_goto_ptr(ctx);

  @@ -159,6 +164,7 @@ static bool gen_branch(DisasContext *ctx, 
arg_b *a, TCGCond cond)

      TCGv src1 = get_gpr(ctx, a->rs1, EXT_SIGN);
      TCGv src2 = get_gpr(ctx, a->rs2, EXT_SIGN);
      target_ulong next_pc;
+    target_ulong orig_pc_save = ctx->pc_save;
        if (get_xl(ctx) == MXL_RV128) {
          TCGv src1h = get_gprh(ctx, a->rs1);

@@ -175,6 +181,7 @@ static bool gen_branch(DisasContext *ctx, arg_b 
*a, TCGCond cond)

        gen_set_label(l); /* branch taken */
  +    ctx->pc_save = orig_pc_save;
      next_pc = ctx->base.pc_next + a->imm;
      if (!has_ext(ctx, RVC) && (next_pc & 0x3)) {
          /* misaligned */

@@ -182,8 +189,9 @@ static bool gen_branch(DisasContext *ctx, arg_b 
*a, TCGCond cond)

          gen_get_target_pc(target_pc, ctx, next_pc);
          gen_exception_inst_addr_mis(ctx, target_pc);
      } else {
-        gen_goto_tb(ctx, 0, ctx->base.pc_next + a->imm);
+        gen_goto_tb(ctx, 0, next_pc);
      }
+    ctx->pc_save = -1;
      ctx->base.is_jmp = DISAS_NORETURN;
        return true;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 7b5223efc2..2dd594ddae 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -59,6 +59,7 @@ typedef struct DisasContext {
      DisasContextBase base;

      /* pc_succ_insn points to the instruction following 
base.pc_next */

      target_ulong pc_succ_insn;
+    target_ulong pc_save;
      target_ulong priv_ver;
      RISCVMXL misa_mxl_max;
      RISCVMXL xl;
@@ -225,15 +226,24 @@ static void decode_save_opc(DisasContext *ctx)
  static void gen_get_target_pc(TCGv target, DisasContext *ctx,
                                target_ulong dest)
  {
-    if (get_xl(ctx) == MXL_RV32) {
-        dest = (int32_t)dest;
+    assert(ctx->pc_save != -1);
+    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
+        tcg_gen_addi_tl(target, cpu_pc, dest - ctx->pc_save);
+        if (get_xl(ctx) == MXL_RV32) {
+            tcg_gen_ext32s_tl(target, target);
+        }
+    } else {
+        if (get_xl(ctx) == MXL_RV32) {
+            dest = (int32_t)dest;
+        }
+        tcg_gen_movi_tl(target, dest);
      }
-    tcg_gen_movi_tl(target, dest);
  }
    static void gen_set_pc_imm(DisasContext *ctx, target_ulong dest)
  {
      gen_get_target_pc(cpu_pc, ctx, dest);
+    ctx->pc_save = dest;


Why set pc_save here?  IMHO, pc_save is a constant.


pc_save is a value which is strictly related to the value of env->pc.
real_pc = (old)env->pc + target_pc(from tb) - ctx->pc_save



In this formula, the meaning of target_pc(from tb) doesn't match with 
gen_get_target_pc in the code. Its meaning in the code matches the 
real_pc in the formula. I think we should rename the gen_get_target_pc 
to gen_get_real_pc.

gen_get_target_pc()  is usually used to generate the target pc of branch 
instruction, or successor instruction. So it's called target_pc in last 
patch. Maybe we can change it to gen_get_real_target_pc in this patch 
when PC-relative translation is introduced.



We should also move the comment in patch 5 to this patch. That will 
help us understand what we are doing here.

OK.



Absolute field in DisasContext used in translation should be replaced 
with a relative representation. For example, ctx->base.pc_next should 
replace with (cpu_pc + ctx->base.pc_next - ctx->base.pc_first).



Yeah. pc_next have been changed to a relative pc address after 
PC-relative translation is introduced.



However, we can remain the original logic, and transform it to the real 
pc by the above formula before we use to affect the architecture state.




So the formula can be described as,

real_pc =  env->pc + abs_dest_pc - abs_first_pc



So it also needs update when cpu_pc is updated.



When cpu_pc updates (usually a jmp or branch instruction), we end the 
block at the same time.  Does a field in DisasContext, i.e., the 
ctx->pc_save still have some meaning after a block has been translated?



cpu_pc may need be updated twice in original logic when instruction mis 
exception is triggered before gen_get_target_pc() is introduced in the 
new version of patch 3:



the first time  is to get the wrong branch target address to fill  into 
badaddr,  the second time is to restore the current pc in 
generate_exception() to get the right epc.


I'm not sure whether there is other corner case currently.

Regards,

Weiwei Li



Zhiwei


Regards,

Weiwei Li



Zhiwei


  }
    static void generate_exception(DisasContext *ctx, int excp)

@@ -287,8 +297,21 @@ static void gen_goto_tb(DisasContext *ctx, int 
n, target_ulong dest)

        * direct block chain benefits will be small.
        */

      if (translator_use_goto_tb(&ctx->base, dest) && 
!ctx->itrigger) {

-        tcg_gen_goto_tb(n);
-        gen_set_pc_imm(ctx, dest);
+        /*
+         * For pcrel, the pc must always be up-to-date on entry to
+         * the linked TB, so that it can use simple additions for all

+         * further adjustments.  For !pcrel, the linked TB is 
compiled

+         * to know its full virtual address, so we can delay the
+         * update to pc to the unlinked path.  A long chain of links
+         * can thus avoid many updates to the PC.
+         */
+        if (tb_cflags(ctx->base.tb) & CF_PCREL) {
+            gen_set_pc_imm(ctx, dest);
+            tcg_gen_goto_tb(n);
+        } else {
+            tcg_gen_goto_tb(n);
+            gen_set_pc_imm(ctx, dest);
+        }
          tcg_gen_exit_tb(ctx->base.tb, n);
      } else {
          gen_set_pc_imm(ctx, dest);

@@ -555,8 +578,16 @@ static void gen_jal(DisasContext *ctx, int rd, 
target_ulong imm)

          }
      }
  -    gen_set_gpri(ctx, rd, ctx->pc_succ_insn);

-    gen_goto_tb(ctx, 0, ctx->base.pc_next + imm); /* must use this 
for safety */

+    assert(ctx->pc_save != -1);
+    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
+        TCGv succ_pc = dest_gpr(ctx, rd);

+        tcg_gen_addi_tl(succ_pc, cpu_pc, ctx->pc_succ_insn - 
ctx->pc_save);

+        gen_set_gpr(ctx, rd, succ_pc);
+    } else {
+        gen_set_gpri(ctx, rd, ctx->pc_succ_insn);
+    }
+
+    gen_goto_tb(ctx, 0, next_pc); /* must use this for safety */
      ctx->base.is_jmp = DISAS_NORETURN;
  }

  @@ -1150,6 +1181,7 @@ static void 
riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)

      RISCVCPU *cpu = RISCV_CPU(cs);
      uint32_t tb_flags = ctx->base.tb->flags;
  +    ctx->pc_save = ctx->base.pc_first;
      ctx->pc_succ_insn = ctx->base.pc_first;
      ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MEM_IDX);
      ctx->mstatus_fs = tb_flags & TB_FLAGS_MSTATUS_FS;

@@ -1195,8 +1227,13 @@ static void 
riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
  static void riscv_tr_insn_start(DisasContextBase *dcbase, 
CPUState *cpu)

  {
      DisasContext *ctx = container_of(dcbase, DisasContext, base);
+    target_ulong pc_next = ctx->base.pc_next;
+
+    if (tb_cflags(dcbase->tb) & CF_PCREL) {
+        pc_next &= ~TARGET_PAGE_MASK;
+    }
  -    tcg_gen_insn_start(ctx->base.pc_next, 0);
+    tcg_gen_insn_start(pc_next, 0);
      ctx->insn_start = tcg_last_op();
  }

























					Reply via email to