[RESEND PATCH v5 0/6] target/riscv: Fix pointer mask related support

[RESEND PATCH v5 0/6] target/riscv: Fix pointer mask related support

[Weiwei Li]

This patchset tries to fix some problem in current implementation for pointer mask, and add support for pointer mask of instruction fetch.

The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-pm-fix-v5

v2:
* drop some error patchs
* Add patch 2 and 3 to fix the new problems
* Add patch 4 and 5 to use PC-relative translation for pointer mask for instruction fetch

v3:
* use target_pc temp instead of cpu_pc to store into badaddr in patch 3
* use dest_gpr instead of tcg_temp_new() for succ_pc in patch 4
* enable CF_PCREL for system mode in seperate patch 5

v4：
* Fix wrong pc_save value for conditional jump in patch 4
* Fix tcg_cflags overwrite problem to make CF_PCREL really work in new patch 5
* Fix tb mis-matched problem in new patch 6

v5:
* use gen_get_target_pc to compute target address of auipc and successor address of jalr in patch 4.
* separate tcg related fix patches(5, 6) from this patchset

Weiwei Li (6):
  target/riscv: Fix pointer mask transformation for vector address
  target/riscv: Update cur_pmmask/base when xl changes
  target/riscv: Fix target address to update badaddr
  target/riscv: Add support for PC-relative translation
  target/riscv: Enable PC-relative translation in system mode
  target/riscv: Add pointer mask support for instruction fetch

 target/riscv/cpu.c                      | 31 +++++++----
 target/riscv/cpu.h                      |  1 +
 target/riscv/cpu_helper.c               | 20 ++++++-
 target/riscv/csr.c                      | 11 ++--
 target/riscv/insn_trans/trans_rvi.c.inc | 37 +++++++++----
 target/riscv/translate.c                | 72 ++++++++++++++++++-------
 target/riscv/vector_helper.c            |  2 +-
 7 files changed, 131 insertions(+), 43 deletions(-)

-- 
2.25.1

[RESEND PATCH v5 1/6] target/riscv: Fix pointer mask transformation for vector address

[Weiwei Li]

actual_address = (requested_address & ~mpmmask) | mpmbase.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/vector_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 2423affe37..a58d82af8c 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -172,7 +172,7 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
 
 static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
 {
-    return (addr & env->cur_pmmask) | env->cur_pmbase;
+    return (addr & ~env->cur_pmmask) | env->cur_pmbase;
 }
 
 /*
-- 
2.25.1

[RESEND PATCH v5 2/6] target/riscv: Update cur_pmmask/base when xl changes

[Weiwei Li]

write_mstatus() can only change current xl when in debug mode.
And we need update cur_pmmask/base in this case.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/csr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..43b9ad4500 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1277,8 +1277,15 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
         mstatus = set_field(mstatus, MSTATUS64_SXL, xl);
     }
     env->mstatus = mstatus;
-    env->xl = cpu_recompute_xl(env);
 
+    /*
+     * Except in debug mode, UXL/SXL can only be modified by higher
+     * privilege mode. So xl will not be changed in normal mode.
+     */
+    if (env->debugger) {
+        env->xl = cpu_recompute_xl(env);
+        riscv_cpu_update_mask(env);
+    }
     return RISCV_EXCP_NONE;
 }
 
-- 
2.25.1

[RESEND PATCH v5 3/6] target/riscv: Fix target address to update badaddr

[Weiwei Li]

Compute the target address  before storing it into badaddr
when mis-aligned exception is triggered.
Use a target_pc temp to store the target address to avoid
the confusing operation that udpate target address into
cpu_pc before misalign check, then update it into badaddr
and restore cpu_pc to current pc if exception is triggered.

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/insn_trans/trans_rvi.c.inc | 23 ++++++++++++++++-------
 target/riscv/translate.c                | 21 ++++++++++-----------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 4ad54e8a49..48c73cfcfe 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -51,25 +51,30 @@ static bool trans_jal(DisasContext *ctx, arg_jal *a)
 static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
 {
     TCGLabel *misaligned = NULL;
+    TCGv target_pc = tcg_temp_new();
 
-    tcg_gen_addi_tl(cpu_pc, get_gpr(ctx, a->rs1, EXT_NONE), a->imm);
-    tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
+    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);
+
+    if (get_xl(ctx) == MXL_RV32) {
+        tcg_gen_ext32s_tl(target_pc, target_pc);
+    }
 
-    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);
     }
 
     gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
+    tcg_gen_mov_tl(cpu_pc, target_pc);
     lookup_and_goto_ptr(ctx);
 
     if (misaligned) {
         gen_set_label(misaligned);
-        gen_exception_inst_addr_mis(ctx);
+        gen_exception_inst_addr_mis(ctx, target_pc);
     }
     ctx->base.is_jmp = DISAS_NORETURN;
 
@@ -153,6 +158,7 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond)
     TCGLabel *l = gen_new_label();
     TCGv src1 = get_gpr(ctx, a->rs1, EXT_SIGN);
     TCGv src2 = get_gpr(ctx, a->rs2, EXT_SIGN);
+    target_ulong next_pc;
 
     if (get_xl(ctx) == MXL_RV128) {
         TCGv src1h = get_gprh(ctx, a->rs1);
@@ -169,9 +175,12 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond)
 
     gen_set_label(l); /* branch taken */
 
-    if (!has_ext(ctx, RVC) && ((ctx->base.pc_next + a->imm) & 0x3)) {
+    next_pc = ctx->base.pc_next + a->imm;
+    if (!has_ext(ctx, RVC) && (next_pc & 0x3)) {
         /* misaligned */
-        gen_exception_inst_addr_mis(ctx);
+        TCGv target_pc = tcg_temp_new();
+        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);
     }
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0ee8ee147d..7b5223efc2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -222,21 +222,18 @@ static void decode_save_opc(DisasContext *ctx)
     ctx->insn_start = NULL;
 }
 
-static void gen_set_pc_imm(DisasContext *ctx, target_ulong dest)
+static void gen_get_target_pc(TCGv target, DisasContext *ctx,
+                              target_ulong dest)
 {
     if (get_xl(ctx) == MXL_RV32) {
         dest = (int32_t)dest;
     }
-    tcg_gen_movi_tl(cpu_pc, dest);
+    tcg_gen_movi_tl(target, dest);
 }
 
-static void gen_set_pc(DisasContext *ctx, TCGv dest)
+static void gen_set_pc_imm(DisasContext *ctx, target_ulong dest)
 {
-    if (get_xl(ctx) == MXL_RV32) {
-        tcg_gen_ext32s_tl(cpu_pc, dest);
-    } else {
-        tcg_gen_mov_tl(cpu_pc, dest);
-    }
+    gen_get_target_pc(cpu_pc, ctx, dest);
 }
 
 static void generate_exception(DisasContext *ctx, int excp)
@@ -257,9 +254,9 @@ static void gen_exception_illegal(DisasContext *ctx)
     }
 }
 
-static void gen_exception_inst_addr_mis(DisasContext *ctx)
+static void gen_exception_inst_addr_mis(DisasContext *ctx, TCGv target)
 {
-    tcg_gen_st_tl(cpu_pc, cpu_env, offsetof(CPURISCVState, badaddr));
+    tcg_gen_st_tl(target, cpu_env, offsetof(CPURISCVState, badaddr));
     generate_exception(ctx, RISCV_EXCP_INST_ADDR_MIS);
 }
 
@@ -551,7 +548,9 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm)
     next_pc = ctx->base.pc_next + imm;
     if (!has_ext(ctx, RVC)) {
         if ((next_pc & 0x3) != 0) {
-            gen_exception_inst_addr_mis(ctx);
+            TCGv target_pc = tcg_temp_new();
+            gen_get_target_pc(target_pc, ctx, next_pc);
+            gen_exception_inst_addr_mis(ctx, target_pc);
             return;
         }
     }
-- 
2.25.1

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

[Weiwei Li]

Add a base save_pc 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
Use gen_get_target_pc to compute target address of auipc and successor
address of jalr.

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 +++++--
 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;
 }
 
 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();
 }
 
-- 
2.25.1

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

[LIU Zhiwei]

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.
> Use gen_get_target_pc to compute target address of auipc and successor
> address of jalr.
>
> 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 +++++--
>   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.

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();
>   }
>   

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

[liweiwei]

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 <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 +++++--
>>   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

So it also needs update when cpu_pc is updated.

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();
>>   }

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

[LIU Zhiwei]

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 <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 +++++--
>>>   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.

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

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).

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?

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();
>>>   }

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

[liweiwei]

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 <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 +++++--
>>>>   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();
>>>>   }

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

[liweiwei]

On 2023/4/2 21:53, liweiwei wrote:
>
> 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 <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 +++++--
>>>>>   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.

Another corner case is wfi instruction, we should udpate env->pc to the 
next instruction before trigger EXCP_HLT. However, this instruction will 
not exit tb.

Regards,

Weiwei Li

>
> 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();
>>>>>   }
>

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

[Richard Henderson]

On 4/2/23 06:17, LIU Zhiwei wrote:
>>> 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.

Neither name is ideal, because it is also used for things that are not "pc".
See e.g. target/arm/, where this is called gen_pc_plus_diff.

This makes slightly more sense for uses like auipc and jalr.


r~

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

[liweiwei]

On 2023/4/3 02:00, Richard Henderson wrote:
> On 4/2/23 06:17, LIU Zhiwei wrote:
>>>> 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.
>
> Neither name is ideal, because it is also used for things that are not 
> "pc".
> See e.g. target/arm/, where this is called gen_pc_plus_diff.
>
OK. Acceptable to me.

Regards,

Weiwei Li

> This makes slightly more sense for uses like auipc and jalr.
>
>
> r~

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

[LIU Zhiwei]

On 2023/4/1 20:49, Weiwei Li wrote:
> Add a base save_pc 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
> Use gen_get_target_pc to compute target address of auipc and successor
> address of jalr.
>
> 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 +++++--
>   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);

If pointer masking enabled, should we adjust the target_pc before it is 
written into the cpu_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);

Same like here.

Zhiwei

> +    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);
> @@ -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();
>   }
>   

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

[liweiwei]

On 2023/4/4 09:58, LIU Zhiwei wrote:
>
> On 2023/4/1 20:49, Weiwei Li wrote:
>> Add a base save_pc 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
>> Use gen_get_target_pc to compute target address of auipc and successor
>> address of jalr.
>>
>> 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 +++++--
>>   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);
>
> If pointer masking enabled, should we adjust the target_pc before it 
> is written into the cpu_pc?

Pointer mask works on effective address. So I think it will not affect 
pc register value, but the fetch address.

And I remember one of its application is memory tag. If pc register is 
already affected by pointer mask,

the memory tag will not work.

Regards,

Weiwei Li

>
>>       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);
>
> Same like here.
>
> Zhiwei
>
>> +    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);
>> @@ -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();
>>   }

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

[LIU Zhiwei]

On 2023/4/4 10:13, liweiwei wrote:
>
> On 2023/4/4 09:58, LIU Zhiwei wrote:
>>
>> On 2023/4/1 20:49, Weiwei Li wrote:
>>> Add a base save_pc 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
>>> Use gen_get_target_pc to compute target address of auipc and successor
>>> address of jalr.
>>>
>>> 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 +++++--
>>>   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);
>>
>> If pointer masking enabled, should we adjust the target_pc before it 
>> is written into the cpu_pc?
>
> Pointer mask works on effective address. So I think it will not affect 
> pc register value, but the fetch address.
>
> And I remember one of its application is memory tag. If pc register is 
> already affected by pointer mask,
>
> the memory tag will not work.

Make sense. Thanks.

Zhiwei

>
> Regards,
>
> Weiwei Li
>
>>
>>>       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);
>>
>> Same like here.
>>
>> Zhiwei
>>
>>> +    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);
>>> @@ -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();
>>>   }

[RESEND PATCH v5 5/6] target/riscv: Enable PC-relative translation in system mode

[Weiwei Li]

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: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 646fa31a59..3b562d5d9f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1193,6 +1193,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 
 
 #ifndef CONFIG_USER_ONLY
+    cs->tcg_cflags |= CF_PCREL;
+
     if (cpu->cfg.ext_sstc) {
         riscv_timer_init(cpu);
     }
-- 
2.25.1

[RESEND PATCH v5 6/6] target/riscv: Add pointer mask support for instruction fetch

[Weiwei Li]

Transform the fetch address in cpu_get_tb_cpu_state() when pointer
mask for instruction is enabled.

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.h        |  1 +
 target/riscv/cpu_helper.c | 20 +++++++++++++++++++-
 target/riscv/csr.c        |  2 --
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..57bd9c3279 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -368,6 +368,7 @@ struct CPUArchState {
 #endif
     target_ulong cur_pmmask;
     target_ulong cur_pmbase;
+    bool cur_pminsn;
 
     /* Fields from here on are preserved across CPU reset. */
     QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..b683a770fe 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -40,6 +40,19 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 #endif
 }
 
+static target_ulong adjust_pc_address(CPURISCVState *env, target_ulong pc)
+{
+    target_ulong adjust_pc = pc;
+
+    if (env->cur_pminsn) {
+        adjust_pc = (adjust_pc & ~env->cur_pmmask) | env->cur_pmbase;
+    } else if (env->xl == MXL_RV32) {
+        adjust_pc &= UINT32_MAX;
+    }
+
+    return adjust_pc;
+}
+
 void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
                           target_ulong *cs_base, uint32_t *pflags)
 {
@@ -48,7 +61,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
 
     uint32_t flags = 0;
 
-    *pc = env->xl == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;
+    *pc = adjust_pc_address(env, env->pc);
     *cs_base = 0;
 
     if (cpu->cfg.ext_zve32f) {
@@ -124,6 +137,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
 void riscv_cpu_update_mask(CPURISCVState *env)
 {
     target_ulong mask = -1, base = 0;
+    bool insn = false;
     /*
      * TODO: Current RVJ spec does not specify
      * how the extension interacts with XLEN.
@@ -135,18 +149,21 @@ void riscv_cpu_update_mask(CPURISCVState *env)
             if (env->mmte & M_PM_ENABLE) {
                 mask = env->mpmmask;
                 base = env->mpmbase;
+                insn = env->mmte & MMTE_M_PM_INSN;
             }
             break;
         case PRV_S:
             if (env->mmte & S_PM_ENABLE) {
                 mask = env->spmmask;
                 base = env->spmbase;
+                insn = env->mmte & MMTE_S_PM_INSN;
             }
             break;
         case PRV_U:
             if (env->mmte & U_PM_ENABLE) {
                 mask = env->upmmask;
                 base = env->upmbase;
+                insn = env->mmte & MMTE_U_PM_INSN;
             }
             break;
         default:
@@ -161,6 +178,7 @@ void riscv_cpu_update_mask(CPURISCVState *env)
         env->cur_pmmask = mask;
         env->cur_pmbase = base;
     }
+    env->cur_pminsn = insn;
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 43b9ad4500..0902b64129 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3518,8 +3518,6 @@ static RISCVException write_mmte(CPURISCVState *env, int csrno,
     /* for machine mode pm.current is hardwired to 1 */
     wpri_val |= MMTE_M_PM_CURRENT;
 
-    /* hardwiring pm.instruction bit to 0, since it's not supported yet */
-    wpri_val &= ~(MMTE_M_PM_INSN | MMTE_S_PM_INSN | MMTE_U_PM_INSN);
     env->mmte = wpri_val | PM_EXT_DIRTY;
     riscv_cpu_update_mask(env);
 
-- 
2.25.1