Re: [PATCH v4 16/16] target/riscv: add trace-hooks for each case of sw-check exception

[Deepak Gupta]

On Fri, Aug 16, 2024 at 03:52:34PM +1000, Richard Henderson wrote:
> On 8/16/24 11:07, 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/helper.h                         |  3 +++
> >  target/riscv/insn_trans/trans_rvi.c.inc       |  3 +++
> >  target/riscv/insn_trans/trans_rvzicfiss.c.inc |  1 +
> >  target/riscv/op_helper.c                      | 13 +++++++++++++
> >  target/riscv/trace-events                     |  6 ++++++
> >  target/riscv/translate.c                      |  2 ++
> >  6 files changed, 28 insertions(+)
> >
> > diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> > index e946ba61fd..6e90fbd225 100644
> > --- a/target/riscv/helper.h
> > +++ b/target/riscv/helper.h
> > @@ -123,6 +123,9 @@ DEF_HELPER_2(cbo_zero, void, env, tl)
> >  /* helper to raise sw check exception */
> >  DEF_HELPER_2(raise_sw_check_excep, void, env, tl)
> > +/* helper functions to trace riscv cfi violations */
> > +DEF_HELPER_3(zicfilp_label_mismatch, void, env, tl, tl)
> > +DEF_HELPER_3(zicfiss_ra_mismatch, void, env, tl, tl)
> >  /* Special functions */
> >  DEF_HELPER_2(csrr, tl, env, int)
> > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
> > b/target/riscv/insn_trans/trans_rvi.c.inc
> > index 936b430282..7021f8d3da 100644
> > --- a/target/riscv/insn_trans/trans_rvi.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> > @@ -54,6 +54,7 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
> >              /*
> >               * misaligned, according to spec we should raise sw check exception
> >               */
> > +            trace_zicfilp_unaligned_lpad_instr(ctx->base.pc_first);
> >              gen_helper_raise_sw_check_excep(tcg_env,
> >                  tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL));
>
> Ah, no.
>
> This performs the trace at translation time.
> You want the trace at execution time.
>
>    gen_update_pc(ctx, 0);
>    gen_helper_zicfilp_unaligned_lpad(tcg_env);
>    ctx->base.is_jmp = DISAS_NORETURN;
>
>
> void HELPER(zicfilp_unaligned_lpad)(CPURISCVState *env)
> {
>    trace_zicfilp_unaligned_lpad(env->pc);
>    env->sw_check_code = RISCV_EXCP_SW_CHECK_FCFI_TVAL;
>    riscv_raise_exception(RISCV_EXCP_SW_CHECK, 0);
> }

facepalm on me. sorry.

> etc.
>
> Nevermind the previous advice vs patch 5 saying you could inline 
> everything; I had forgotten the desire for tracepoints.

It helps locate finding control flow violations faster and fix such
issues in libc, libraries, and other pieces of software faster.

So desire is basically fixing guest software faster.

> You should probably add these helpers and tracepoints as you add the 
> code.  Anything else is going to be a bit confusing.

Or I can just drop this for now for upstreaming purpose. I'll think about it.
> r~