qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: LIU Zhiwei
Subject: Re: [PATCH v6 4/6] target/riscv: Add support for PC-relative translation
Date: Tue, 4 Apr 2023 17:23:56 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 

On 2023/4/4 16:48, liweiwei wrote:


On 2023/4/4 15:07, LIU Zhiwei wrote:


On 2023/4/4 11:46, liweiwei wrote:


On 2023/4/4 11:12, LIU Zhiwei wrote:


On 2023/4/4 10:06, Weiwei Li wrote:

Add a base pc_save for PC-relative translation(CF_PCREL).
Diable the directly sync pc from tb by riscv_cpu_synchronize_from_tb.
We can get pc-relative address from following formula:
   real_pc = (old)env->pc + diff, where diff = target_pc - ctx->pc_save. Use gen_get_target_pc to compute target address of auipc and successor 
address of jalr and jal.

The existence of CF_PCREL can improve performance with the guest
kernel's address space randomization.  Each guest process maps libc.so 
(et al) at a different virtual address, and this allows those
translations to be shared.

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


Miss the process for trans_ebreak.
I want to construct the PCREL feature on the processing of ctx pc related fields, which is the reason why we need do specially process. For example, 

 static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
 {
-    gen_set_gpri(ctx, a->rd, a->imm + ctx->base.pc_next);
+    if (tb_cflags(ctx->cflags) & CF_PCREL) {
+        target_ulong pc_rel = ctx->base.pc_next - ctx->base.pc_first + a->imm; 
+        gen_set_gpr_pcrel(ctx, a->rd, cpu_pc, pc_rel);
+    } else {
+        gen_set_gpri(ctx, a->rd, a->imm + ctx->base.pc_next);
+    }
     return true;
 }
+static void gen_set_gpr_pcrel(DisasContext *ctx, int reg_num, TCGv t, target_ulong rel) 
+{
+    TCGv dest = dest_gpr(ctx, reg_num);
+    tcg_gen_addi_tl(dest, t, rel);
+    gen_set_gpr(ctx, reg_num, dest);
+}
+
But if it is too difficult to reuse the current implementation, your implementation is also acceptable to me.
Sorry, I don't get your idea. gen_pc_plus_diff() can do all the above job.
Yes, I think so. I just suspect whether it is easy to read and verify the correctness. And the maintenance for the future.
1) Maybe we should split the PCREL to a split patch set, as it is a new feature. The point masking can still use this thread.
Point mask for instruction relies on PCREL. That's why I introduce PCREL in this patchset. 

Maybe we can divide this patch if needed.

If we won't use another way to rewrite the PCREL, we don't have to split it.
2) For the new patch set for PCREL, process where we need to modify one by one. One clue for recognize where to modify is the ctx pc related fields, such as pc_next/pc_first/succ_insn_pc.
One thing may worth to try is that don't change the code in insn_trans/trans_X.  Just rename the origin API we need to modify to a new name with _abs suffix. And and a correspond set of API for PCREL with _pcrel suffix. 


 I don't find a good way to  remain trans_* unchanged to support PCREL.

For example, in DisasContext, we define

void (*gen_set_gpri)(DisasContext *ctx, int reg_num, target_long imm);

In disas_init_fn,

if (tb_cflags(tb) & CF_PCREL) {
    gen_set_gpri = gen_set_gpri_pcrel;
} else {
    gen_set_gpri = gen_set_gpri_abs;
} Thus we can write the code in trans_insn without think about the PCREL.
That's what I want. And PCREL also have  been avoided in following code of trans_*.
However, we should't update  pc_next/pc_succ_insn related address into register directly by reusing existed API like gen_set_gpri.
It's a general API to set gpr to any imm. However PC-relative only works for pc-related imm.
Maybe we can introduce a new API like gen_set_gpr_pci() to set pc related imm.
Yes, I think so, except _pci is not a good suffix.  But I don't insist on this way. 

Zhiwei
However it seems unnecessary, because it's no difference from current logic by using gen_pc_plus_diff() from the developer hand. 

Regards,

Weiwei Li



Thanks,
Zhiwei



Regards,

Weiwei Li



Zhiwei


  target/riscv/translate.c | 48 ++++++++++++++++++++-----
  3 files changed, 70 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 cc72864d32..7cbbdac5aa 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_pc_plus_diff(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_pc_plus_diff(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_pc_plus_diff(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 d434fedb37..4623749602 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_pc_plus_diff(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_pc_plus_diff(cpu_pc, ctx, dest);
+    ctx->pc_save = dest;
  }
    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);
@@ -543,6 +566,7 @@ static void gen_set_fpr_d(DisasContext *ctx, int reg_num, TCGv_i64 t) 
  static void gen_jal(DisasContext *ctx, int rd, target_ulong imm)
  {
      target_ulong next_pc;
+    TCGv succ_pc = dest_gpr(ctx, rd);
        /* check misaligned: */
      next_pc = ctx->base.pc_next + imm;
@@ -555,8 +579,10 @@ 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 */ 
+    gen_pc_plus_diff(succ_pc, ctx, ctx->pc_succ_insn);
+    gen_set_gpr(ctx, rd, succ_pc);
+
+    gen_goto_tb(ctx, 0, next_pc); /* must use this for safety */
      ctx->base.is_jmp = DISAS_NORETURN;
  }
  @@ -1150,6 +1176,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 +1222,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
[Prev in Thread] Current Thread [Next in Thread]