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

[Deepak Gupta <debug@rivosinc.com>]

On Mon, Aug 19, 2024 at 05:01:25PM -0700, Deepak Gupta wrote:
>zicfiss protects shadow stack using new page table encodings PTE.W=0,
>PTE.R=0 and PTE.X=0. This encoding is reserved if zicfiss is not
>implemented or if shadow stack are not enabled.
>Loads on shadow stack memory are allowed while stores to shadow stack
>memory leads to access faults. Shadow stack accesses to RO memory
>leads to store page fault.
>
>To implement special nature of shadow stack memory where only selected
>stores (shadow stack stores from sspush) have to be allowed while rest
>of regular stores disallowed, new MMU TLB index is created for shadow
>stack.
>
>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>---
>@@ -1406,6 +1431,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>     qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                   __func__, address, access_type, mmu_idx);
>
>+    /* 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?

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;
+     }
+
      switch (access_type) {
      case MMU_INST_FETCH:
          if (pmp_violation) {
@@ -1433,7 +1438,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
  
      /* If shadow stack instruction initiated this access, treat it as store */
      if (mmu_idx & MMU_IDX_SS_WRITE) {
-        access_type = MMU_DATA_STORE;
+        tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] |= 1;
      }
  
      pmu_tlb_fill_incr_ctr(cpu, access_type);
@@ -1529,6 +1534,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
      if (ret == TRANSLATE_SUCCESS) {
          tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
                       prot, mmu_idx, tlb_size);
+        tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1;
          return true;
      } else if (probe) {


"""

>     pmu_tlb_fill_incr_ctr(cpu, access_type);
>     if (two_stage_lookup) {
>         /* Two stage lookup */
>diff --git a/target/riscv/internals.h b/target/riscv/internals.h
>index 0ac17bc5ad..ddbdee885b 100644
>--- a/target/riscv/internals.h
>+++ b/target/riscv/internals.h
>@@ -30,12 +30,15 @@
>  *  - U+2STAGE          0b100
>  *  - S+2STAGE          0b101
>  *  - S+SUM+2STAGE      0b110
>+ *  - Shadow stack+U   0b1000
>+ *  - Shadow stack+S   0b1001
>  */
> #define MMUIdx_U            0
> #define MMUIdx_S            1
> #define MMUIdx_S_SUM        2
> #define MMUIdx_M            3
> #define MMU_2STAGE_BIT      (1 << 2)
>+#define MMU_IDX_SS_WRITE    (1 << 3)
>
> static inline int mmuidx_priv(int mmu_idx)
> {
>-- 
>2.44.0
>