Re: [PATCH v2 4/5] target/riscv: Add support for PC-relative translation

[liweiwei]

On 2023/3/30 00:27, Richard Henderson wrote:
On 3/28/23 20:23, Weiwei Li wrote:
  static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
  {
-    gen_set_gpri(ctx, a->rd, a->imm + ctx->base.pc_next);
+    assert(ctx->pc_save != -1);
+    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
+        TCGv target_pc = tcg_temp_new();

dest_gpr(s, a->rd)
OK. I'll fix this.

@@ -51,26 +59,43 @@ static bool trans_jal(DisasContext *ctx, arg_jal *a)
  static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
  {
      TCGLabel *misaligned = NULL;
+    TCGv succ_pc = tcg_temp_new();

succ_pc can by null for !CF_PCREL...
I think this is OK since it's only used for CF_PCREL.

+    TCGv target_pc = tcg_temp_new();
+
+    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
+        tcg_gen_addi_tl(succ_pc, cpu_pc, ctx->pc_succ_insn - 
ctx->pc_save);
+    }

... or initialized like

       } else {
           succ_pc = tcg_constant_tl(ctx->pc_succ_insn);
       }

-    gen_set_pc(ctx, cpu_pc);
      if (!has_ext(ctx, RVC)) {
          TCGv t0 = tcg_temp_new();
            misaligned = gen_new_label();
-        tcg_gen_andi_tl(t0, cpu_pc, 0x2);
+        tcg_gen_andi_tl(t0, target_pc, 0x2);
          tcg_gen_brcondi_tl(TCG_COND_NE, t0, 0x0, misaligned);
      }
...
      if (misaligned) {
          gen_set_label(misaligned);
-        gen_exception_inst_addr_mis(ctx);
+        gen_exception_inst_addr_mis(ctx, target_pc);
      }

This is what I expected from patch 3: cpu_pc is unchanged, with the 
new (incorrect) address passed to inst_addr_mis for assigning to 
badaddr.  Bug being fixed here, thus should really be a separate patch.

It's OK to update cpu_pc before gen_exception_inst_addr_mis() since it 
will restore the current pc by gen_set_pc_imm() after update cpu_pc into 
badaddr.

However, after PC-relative translation is enabled, we cannot use 
gen_set_pc to directly update cpu_pc in above case, since gen_set_pc() 
will break the pc_save, and make gen_set_pc_imm() in 
gen_exception_inst_addr_mis() failed. So we introduce a temp target_pc  
instead of cpu_pc to compute the destination pc and use it to do 
misaligned check.


@@ -172,7 +197,7 @@ static bool gen_branch(DisasContext *ctx, arg_b 
*a, TCGCond cond)
      if (!has_ext(ctx, RVC) && ((ctx->base.pc_next + a->imm) & 0x3)) {
          /* misaligned */
          gen_set_pc_imm(ctx, ctx->base.pc_next + a->imm);
-        gen_exception_inst_addr_mis(ctx);
+        gen_exception_inst_addr_mis(ctx, cpu_pc);

But this one's different and (probably) incorrect.

@@ -552,13 +567,21 @@ static void gen_jal(DisasContext *ctx, int rd, 
target_ulong imm)
      if (!has_ext(ctx, RVC)) {
          if ((next_pc & 0x3) != 0) {
              gen_set_pc_imm(ctx, next_pc);
-            gen_exception_inst_addr_mis(ctx);
+            gen_exception_inst_addr_mis(ctx, cpu_pc);

Likewise.

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

dest_gpr.

OK. I'll fix this.

Regards,

Weiwei Li




r~