On Tue, Aug 20, 2024 at 11:55 AM Deepak Gupta <de...@rivosinc.com> wrote:
>
> 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) {
This is a wrong check because we never store MMU_IDX_SS_WRITE in `ctx->mem_idx`
But I can place an indicator in `DisasContext` and set it up in
`trans_xxx` for shadow stack instr.
> + /* 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~
