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

[Deepak Gupta]

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~