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));
             return true;
@@ -66,6 +67,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
         TCGv tmp = tcg_temp_new();
         tcg_gen_extract_tl(tmp, get_gpr(ctx, xT2, EXT_NONE), 12, 20);
         tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->label, skip);
+        gen_helper_zicfilp_label_mismatch(tcg_env, tcg_constant_tl(a->label),
+            tmp);
         gen_helper_raise_sw_check_excep(tcg_env,
             tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL));
         gen_set_label(skip);
diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
index 67f5c7804a..f1cf7ca438 100644
--- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc
+++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
@@ -45,6 +45,7 @@ static bool gen_sspopchk(DisasContext *ctx, int rs1_reg)
                        mxl_memop(ctx) | MO_ALIGN);
     TCGv rs1 = get_gpr(ctx, rs1_reg, EXT_NONE);
     tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
+    gen_helper_zicfiss_ra_mismatch(tcg_env, data, rs1);
     gen_helper_raise_sw_check_excep(tcg_env,
         tcg_constant_tl(RISCV_EXCP_SW_CHECK_BCFI_TVAL));
     gen_set_label(skip);
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 9ec19c4afa..b681f0f1aa 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,
@@ -265,6 +266,18 @@ void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code)
     riscv_raise_exception(env, RISCV_EXCP_SW_CHECK, GETPC());
 }
 
+void helper_zicfilp_label_mismatch(CPURISCVState *env, target_ulong lpad_label,
+                                   target_ulong t2_label)
+{
+    trace_zicfilp_lpad_reg_mismatch(lpad_label, t2_label);
+}
+
+void helper_zicfiss_ra_mismatch(CPURISCVState *env, target_ulong ssra,
+                                target_ulong rs1)
+{
+    trace_zicfiss_sspopchk_reg_mismatch(ssra, rs1);
+}
+
 #ifndef CONFIG_USER_ONLY
 
 target_ulong helper_sret(CPURISCVState *env)
diff --git a/target/riscv/trace-events b/target/riscv/trace-events
index 49ec4d3b7d..9d5b61a2da 100644
--- a/target/riscv/trace-events
+++ b/target/riscv/trace-events
@@ -9,3 +9,9 @@ pmpaddr_csr_write(uint64_t mhartid, uint32_t addr_index, uint64_t val) "hart %"
 
 mseccfg_csr_read(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": read mseccfg, val: 0x%" PRIx64
 mseccfg_csr_write(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": write mseccfg, val: 0x%" PRIx64
+
+# zicfiss/lp
+zicfiss_sspopchk_reg_mismatch(uint64_t ssra, uint64_t rs1) "shadow_stack_ra: 0x%" PRIx64 ", rs1: 0x%" PRIx64
+zicfilp_missing_lpad_instr(uint64_t pc_first) "pc_first: 0x%" PRIx64
+zicfilp_unaligned_lpad_instr(uint64_t pc_next) "pc_next: 0x%" PRIx64
+zicfilp_lpad_reg_mismatch(uint64_t lpad_label, uint64_t t2_label) "lpad_label: 0x%" PRIx64 ", t2_label: 0x%" PRIx64
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 6fa98e88d9..fbef430848 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -35,6 +35,7 @@
 #undef  HELPER_H
 
 #include "tcg/tcg-cpu.h"
+#include "trace.h"
 
 /* global register indices */
 static TCGv cpu_gpr[32], cpu_gprh[32], cpu_pc, cpu_vl, cpu_vstart;
@@ -1348,6 +1349,7 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
          */
         tcg_set_insn_param(tcg_ctx->cfi_lp_check, 1,
                            tcgv_i32_arg(tcg_constant_i32(1)));
+        trace_zicfilp_missing_lpad_instr(ctx->base.pc_first);
     }
 }
 
-- 
2.44.0

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);
}

etc.

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

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


r~

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~