Re: [PATCH v3 18/20] target/riscv: add trace-hooks for each case of sw-check exception

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> Violations to control flow rules setup by zicfilp and zicfiss lead to
> software check exceptions. To debug and fix such sw check issues in guest
> , add trace-hooks for each case.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   target/riscv/insn_trans/trans_rvi.c.inc |  6 ++++--
>   target/riscv/op_helper.c                | 24 ++++++++++++++++++++++++
>   target/riscv/trace-events               |  6 ++++++
>   target/riscv/translate.c                |  2 +-
>   4 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
> b/target/riscv/insn_trans/trans_rvi.c.inc
> index cbd7d5c395..0f5d5def60 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>                */
>               gen_helper_raise_sw_check_excep(tcg_env,
>                   tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> -                tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0));
> +                tcg_constant_tl(MISALIGNED_LPAD),
> +                tcg_constant_tl(ctx->base.pc_next));
>               return true;
>           }
>       }
> @@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>           tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
>           gen_helper_raise_sw_check_excep(tcg_env,
>               tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> -            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
> +            tcg_constant_tl(LABEL_MISMATCH_LPAD),
> +            tcg_constant_tl(a->imm_cfi20));
>           gen_set_label(skip);
>       }
>   
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 3b47fb34ea..07990e6589 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -24,6 +24,7 @@
>   #include "exec/exec-all.h"
>   #include "exec/cpu_ldst.h"
>   #include "exec/helper-proto.h"
> +#include "trace.h"
>   
>   /* Exceptions processing helpers */
>   G_NORETURN void riscv_raise_exception(CPURISCVState *env,
> @@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong 
> address)
>   void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong 
> swcheck_code,
>                                    target_ulong arg1, target_ulong arg2)
>   {
> +    switch (swcheck_code) {
> +    case RISCV_EXCP_SW_CHECK_FCFI_TVAL:
> +        switch (arg1) {
> +        case MISSING_LPAD:
> +            trace_zicfilp_missing_lpad_instr(arg2);
> +            break;
> +        case MISALIGNED_LPAD:
> +            trace_zicfilp_unaligned_lpad_instr(arg2);
> +            break;
> +        case LABEL_MISMATCH_LPAD:
> +            trace_zicfilp_lpad_reg_mismatch(arg2);
> +            break;
> +        }
> +        break;
> +    case RISCV_EXCP_SW_CHECK_BCFI_TVAL:
> +        trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2);
> +        break;
> +    default:
> +        /* any other value of swcheck_code is asserted */
> +        assert(swcheck_code || (swcheck_code == 0));
> +        break;
> +    }

I think this is a mistake.
Better to have 4 different helpers and eliminate MISSING_LPAD, etc entirely.

> @@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, 
> CPUState *cpu)
>           tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l);
>           gen_helper_raise_sw_check_excep(tcg_env,
>               tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> -            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
> +            tcg_constant_tl(MISSING_LPAD), 
> tcg_constant_tl(ctx->base.pc_first));

You certainly don't need to pass pc_first, since if lp_expected is set that is always 
env->pc in the helper.


r~