Thanks Richard. I have incorporated your feedback in v5.
-Rajnesh
On Wed, Dec 4, 2024 at 7:30 PM Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 12/4/24 06:56, Rajnesh Kanwal wrote:
> > diff --git a/target/riscv/insn_trans/trans_privileged.c.inc
> > b/target/riscv/insn_trans/trans_privileged.c.inc
> > index
> > 0bdfa9a0ed3313223ce9032fb24484c3887cddf9..a5c2410cfa0779b1a928e7b89bd2ee5bb24216e4
> > 100644
> > --- a/target/riscv/insn_trans/trans_privileged.c.inc
> > +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> > @@ -78,9 +78,10 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
> > {
> > #ifndef CONFIG_USER_ONLY
> > if (has_ext(ctx, RVS)) {
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> This is incorrect.
> You need to use gen_pc_plus_diff(src, ctx, 0).
>
> Alternately, for here in sret and mret, instead of adding an extra parameter,
> use
> gen_update_pc(ctx, 0) to update env->pc
>
>
>
> > @@ -95,9 +96,10 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
> > static bool trans_mret(DisasContext *ctx, arg_mret *a)
> > {
> > #ifndef CONFIG_USER_ONLY
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> Likewise.
>
>
> > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc
> > b/target/riscv/insn_trans/trans_rvi.c.inc
> > index
> > 96c218a9d7875c6419287ac3aa9746251be3f442..fc182e7b18a289e13ad212f10a3233aca25fae41
> > 100644
> > --- a/target/riscv/insn_trans/trans_rvi.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> > @@ -93,6 +93,50 @@ static bool trans_jal(DisasContext *ctx, arg_jal *a)
> > return true;
> > }
> >
> > +#ifndef CONFIG_USER_ONLY
> > +/*
> > + * Indirect calls
> > + * - jalr x1, rs where rs != x5;
> > + * - jalr x5, rs where rs != x1;
> > + * - c.jalr rs1 where rs1 != x5;
> > + *
> > + * Indirect jumps
> > + * - jalr x0, rs where rs != x1 and rs != x5;
> > + * - c.jr rs1 where rs1 != x1 and rs1 != x5.
> > + *
> > + * Returns
> > + * - jalr rd, rs where (rs == x1 or rs == x5) and rd != x1 and rd != x5;
> > + * - c.jr rs1 where rs1 == x1 or rs1 == x5.
> > + *
> > + * Co-routine swap
> > + * - jalr x1, x5;
> > + * - jalr x5, x1;
> > + * - c.jalr x5.
> > + *
> > + * Other indirect jumps
> > + * - jalr rd, rs where rs != x1, rs != x5, rd != x0, rd != x1 and rd != x5.
> > + */
> > +static void helper_ctr_jalr(DisasContext *ctx, arg_jalr *a)
>
> Generally "helper_*" are out-of-line functions, whereas this is generating
> inline code.
> Better as "gen_ctr_jalr".
>
> > +{
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> gen_pc_plus_diff
>
> > @@ -219,6 +269,9 @@ 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 orig_pc_save = ctx->pc_save;
> > +#ifndef CONFIG_USER_ONLY
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
> > +#endif
>
> gen_pc_plus_diff, though perhaps delay until used.
>
> >
> > if (get_xl(ctx) == MXL_RV128) {
> > TCGv src1h = get_gprh(ctx, a->rs1);
> > @@ -231,6 +284,15 @@ static bool gen_branch(DisasContext *ctx, arg_b *a,
> > TCGCond cond)
> > } else {
> > tcg_gen_brcond_tl(cond, src1, src2, l);
> > }
> > +
> > +#ifndef CONFIG_USER_ONLY
> > + if (ctx->cfg_ptr->ext_smctr || ctx->cfg_ptr->ext_ssctr) {
> > + TCGv type = tcg_constant_tl(CTRDATA_TYPE_NONTAKEN_BRANCH);
> > + TCGv dest = tcg_constant_tl(ctx->base.pc_next + ctx->cur_insn_len);
>
> gen_pc_plus_diff
>
> > + gen_helper_ctr_add_entry(tcg_env, src, dest, type);
> > + }
> > +#endif
> > +
> > gen_goto_tb(ctx, 1, ctx->cur_insn_len);
> > ctx->pc_save = orig_pc_save;
> >
> > @@ -243,6 +305,14 @@ static bool gen_branch(DisasContext *ctx, arg_b *a,
> > TCGCond cond)
> > gen_pc_plus_diff(target_pc, ctx, a->imm);
> > gen_exception_inst_addr_mis(ctx, target_pc);
> > } else {
> > +#ifndef CONFIG_USER_ONLY
> > + if (ctx->cfg_ptr->ext_smctr || ctx->cfg_ptr->ext_ssctr) {
> > + TCGv type = tcg_constant_tl(CTRDATA_TYPE_TAKEN_BRANCH);
> > + TCGv dest = tcg_constant_tl(ctx->base.pc_next + a->imm);
>
> gen_pc_plus_diff.
>
> > diff --git a/target/riscv/insn_trans/trans_rvzce.c.inc
> > b/target/riscv/insn_trans/trans_rvzce.c.inc
> > index
> > cd234ad960724c936b92afb6fd1f3c7c2a37cb80..07b51d9f4d847c4411165b422a843fea65c86d45
> > 100644
> > --- a/target/riscv/insn_trans/trans_rvzce.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzce.c.inc
> > @@ -204,6 +204,13 @@ static bool gen_pop(DisasContext *ctx, arg_cmpp *a,
> > bool ret, bool ret_val)
> > if (ret) {
> > TCGv ret_addr = get_gpr(ctx, xRA, EXT_SIGN);
> > tcg_gen_mov_tl(cpu_pc, ret_addr);
> > +#ifndef CONFIG_USER_ONLY
> > + if (ctx->cfg_ptr->ext_smctr || ctx->cfg_ptr->ext_ssctr) {
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> gen_pc_plus_diff, and it will need to be done *before* the assignment to
> cpu_pc.
>
> > @@ -309,6 +316,21 @@ static bool trans_cm_jalt(DisasContext *ctx,
> > arg_cm_jalt *a)
> > gen_set_gpr(ctx, xRA, succ_pc);
> > }
> >
> > +#ifndef CONFIG_USER_ONLY
> > + if (ctx->cfg_ptr->ext_smctr || ctx->cfg_ptr->ext_ssctr) {
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> Here, we have updated cpu_pc to current (see the start of the function), so
> you can just
> use that instead of src.
>
> > +void helper_ctr_add_entry(CPURISCVState *env, target_ulong src,
> > + target_ulong dest, target_ulong type)
> > +{
> > + riscv_ctr_add_entry(env, src, dest, (enum CTRType)type,
> > + env->priv, env->virt_enabled);
>
> Indent to line up after (.
>
> > +static void helper_ctr_jal(DisasContext *ctx, int rd, target_ulong imm)
>
> gen_ctr_jal.
>
> > +{
> > + TCGv dest = tcg_constant_tl(ctx->base.pc_next + imm);
> > + TCGv src = tcg_constant_tl(ctx->base.pc_next);
>
> gen_pc_plus_diff.
>
>
> r~
