| Message ID | 20230401124935.20997-5-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix pointer mask related support | expand |
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(); > } >
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(); >> }
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(); >>> }
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(); >>>> }
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~
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~
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(); >>>>> } >
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(); > } >
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(); >> }
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(); >>> }
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(); }