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

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:27:22PM +1000, Richard Henderson wrote:
> 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.

Honestly for raising sw check exception, helper is not needed.

I added a helper for raising sw check exception, because needed to add trace
capability on violations. Helps locate issues in cfi compiled binaries faster.

If you prefer 4 different helpers for 4 different trace needs, I will do that.

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

noted.

> r~