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;
+    }
+
     env->sw_check_code = swcheck_code;
     riscv_raise_exception(env, RISCV_EXCP_SW_CHECK, GETPC());
 }
diff --git a/target/riscv/trace-events b/target/riscv/trace-events
index 49ec4d3b7d..0e8807f0d4 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(int lpad_label) "lpad_label: 0x%x"
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 4772191bd8..9ef1f220e0 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -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));
         gen_set_label(l);
         /*
          * Despite the use of gen_exception_illegal(), the rest of
-- 
2.44.0

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~

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~