Re: [PATCH v5 11/15] target/riscv: mmu changes for zicfiss shadow stack protection

[Deepak Gupta <debug@rivosinc.com>]

On Tue, Aug 20, 2024 at 07:20:48PM +1000, Richard Henderson wrote:
>On 8/20/24 17:35, Deepak Gupta wrote:
>>>+    /* If shadow stack instruction initiated this access, treat it as store */
>>>+    if (mmu_idx & MMU_IDX_SS_WRITE) {
>>>+        access_type = MMU_DATA_STORE;
>>>+    }
>>>+
>>
>>I think I forgot to address this. Do you still want me to fix this up like you
>>had suggested?
>
>Yes, this still needs fixing.
>
>
>>IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know
>>what it means and how its used. Based on git grep and some readup, 
>>are you expecting something
>>along the below lines?
>>
>>
>>"""
>>
>>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>index fee31b8037..dfd2efa941 100644
>>--- a/target/riscv/cpu.h
>>+++ b/target/riscv/cpu.h
>>@@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState;
>>   * RISC-V-specific extra insn start words:
>>   * 1: Original instruction opcode
>>   */
>>-#define TARGET_INSN_START_EXTRA_WORDS 1
>>+#define TARGET_INSN_START_EXTRA_WORDS 2
>>
>>  #define RV(x) ((target_ulong)1 << (x - 'A'))
>>
>>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>index f74a1216b1..b266177e46 100644
>>--- a/target/riscv/cpu_helper.c
>>+++ b/target/riscv/cpu_helper.c
>>@@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState 
>>*env, target_ulong address,
>>  {
>>      CPUState *cs = env_cpu(env);
>>
>>+     if (!pmp_violation &&
>>+         tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) {
>>+         tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1;
>>+         access_type = MMU_DATA_STORE;
>>+     }
>
>The first thing to understand is that the unwind data is stored by the 
>compiler and recovered by the unwinder.
>
>The unwind data is exposed to the target via one of two methods:
>
>(1) TCGCPUOps.restore_state_to_opc, i.e. riscv_restore_state_to_opc.
>    The data[] argument contains the extra words.
>
>    With this method, the extra words are restored to env and are
>    available in a later call to riscv_cpu_do_interrupt.
>    Compare env->bins from the first extra word, which is used exactly so.
>
>    This is probably the easiest and best option.
>    You'd promote LOAD* to STORE_AMO* while dispatching the interrupt.
>
>(2) cpu_unwind_state_data()
>
>    With this method, you have immediate access to the extra words,
>    and don't need to store them anywhere else.
>
>    This is supposed to be used when we are *not* going to raise
>    an exception, merely look something up and continue execution.
>    Otherwise, we'd be performing the unwind operation twice,
>    and it's not cheap.
>
>So, tcg_ctx->gen_insn_data[] is not something you'd ever touch,
>and this is the wrong spot to do anything.

Thanks for more information and guiding me through this.

Not going to say that I still understand everything. But I looked
at one arm example. Before I do something more with it. I wanted to run
it by you.

Something on the below lines? I've one question as well for you in comment.

""""
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index fee31b8037..b4e04fe849 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -46,8 +46,14 @@ typedef struct CPUArchState CPURISCVState;
  /*
   * RISC-V-specific extra insn start words:
   * 1: Original instruction opcode
+ * 2: more information about instruction
   */
-#define TARGET_INSN_START_EXTRA_WORDS 1
+#define TARGET_INSN_START_EXTRA_WORDS 2
+
+/*
+ * b0: Whether a shadow stack operation/instruction or not.
+ */
+#define RISCV_INSN_START_WORD2_SS_OP 1
  
  #define RV(x) ((target_ulong)1 << (x - 'A'))
  
@@ -226,6 +232,7 @@ struct CPUArchState {
      bool      elp;
      /* shadow stack register for zicfiss extension */
      target_ulong ssp;
+    bool      ss_op;
      /* sw check code for sw check exception */
      target_ulong sw_check_code;
  #ifdef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f74a1216b1..c28b13d42c 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1777,6 +1777,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
      target_ulong mtval2 = 0;
  
      if (!async) {
+        /* shadow stack op, promote load page fault to store page fault */
+        if (env->ss_op && cause == RISCV_EXCP_LOAD_PAGE_FAULT) {
+            cause = RISCV_EXCP_STORE_PAGE_FAULT;
+        }
          /* set tval to badaddr for traps with address information */
          switch (cause) {
          case RISCV_EXCP_SEMIHOST:
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 4da26cb926..c0f21fe3db 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -129,6 +129,7 @@ static void riscv_restore_state_to_opc(CPUState *cs,
          env->pc = pc;
      }
      env->bins = data[1];
+    env->ss_op = data[2] & RISCV_INSN_START_WORD2_SS_OP;
  }
  
  static const TCGCPUOps riscv_tcg_ops = {
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 580aa23c5b..6f952db823 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1271,7 +1271,7 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
          pc_next &= ~TARGET_PAGE_MASK;
      }
  
-    tcg_gen_insn_start(pc_next, 0);
+    tcg_gen_insn_start(pc_next, 0, 0);
      ctx->insn_start_updated = false;
  }
  
@@ -1301,6 +1301,14 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
          ctx->base.is_jmp = DISAS_NORETURN;
      }
  
+    /* shadow stack index means shadow stack instruction is translated */
+    if (ctx->mem_idx & MMU_IDX_SS_WRITE) {
+        /* Is this needed to set true? */
+        ctx->insn_start_updated = true;
+        tcg_set_insn_start_param(ctx->base.insn_start, 2,
+                                 RISCV_INSN_START_WORD2_SS_OP);
+    }
+
      /* Only the first insn within a TB is allowed to cross a page boundary. */
      if (ctx->base.is_jmp == DISAS_NEXT) {
          if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) {

"""
>
>
>r~