Re: [PATCH v4 4/7] target/riscv: Add support to record CTR entries.

[Richard Henderson]

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~