| Message ID | 20240821215014.3859190-13-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On 8/22/24 07:50, Deepak Gupta wrote: > @@ -1779,13 +1780,25 @@ void riscv_cpu_do_interrupt(CPUState *cs) > env->pc += 4; > return; > case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: > + if (always_storeamo) { > + cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT; > + } > + goto load_store_fault; > case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: > case RISCV_EXCP_LOAD_ADDR_MIS: > case RISCV_EXCP_STORE_AMO_ADDR_MIS: > case RISCV_EXCP_LOAD_ACCESS_FAULT: > + if (always_storeamo) { > + cause = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; > + } > + goto load_store_fault; > case RISCV_EXCP_STORE_AMO_ACCESS_FAULT: > case RISCV_EXCP_LOAD_PAGE_FAULT: > case RISCV_EXCP_STORE_PAGE_FAULT: > + if (always_storeamo) { > + cause = RISCV_EXCP_STORE_PAGE_FAULT; > + } > + load_store_fault: These case labels need to be re-sorted; you're mising load/store when you're intending to check for load alone. I expect LOAD_ADDR_MIS needs adjustment as well? > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index d44103a273..8961dda244 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -121,6 +121,7 @@ typedef struct DisasContext { > bool fcfi_lp_expected; > /* zicfiss extension, if shadow stack was enabled during TB gen */ > bool bcfi_enabled; > + target_ulong excp_uw2; > } DisasContext; > > static inline bool has_ext(DisasContext *ctx, uint32_t ext) > @@ -144,6 +145,9 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) > #define get_address_xl(ctx) ((ctx)->address_xl) > #endif > > +#define SET_INSTR_ALWAYS_STORE_AMO(ctx) \ > + (ctx->excp_uw2 |= RISCV_UW2_ALWAYS_STORE_AMO) > + > /* The word size for this machine mode. */ > static inline int __attribute__((unused)) get_xlen(DisasContext *ctx) > { > @@ -214,6 +218,12 @@ static void decode_save_opc(DisasContext *ctx) > assert(!ctx->insn_start_updated); > ctx->insn_start_updated = true; > tcg_set_insn_start_param(ctx->base.insn_start, 1, ctx->opcode); > + > + if (ctx->excp_uw2) { > + tcg_set_insn_start_param(ctx->base.insn_start, 2, > + ctx->excp_uw2); > + ctx->excp_uw2 = 0; > + } I really don't think having data on the side like this... > } > > static void gen_pc_plus_diff(TCGv target, DisasContext *ctx, > @@ -1096,6 +1106,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a, > mop |= MO_ALIGN; > } > > + SET_INSTR_ALWAYS_STORE_AMO(ctx); > decode_save_opc(ctx); ... or the requirement for ordering of two function calls is a good interface. I did say perhaps add another helper, but what I expected was decode_save_opc_set_amo_store(ctx); where decode_save_opc and decode_save_opc_set_amo_store call into a common helper. But perhaps in the end maybe just decode_save_opc(ctx, uw2) is better. I expect gen_cmpxchg also needs updating, though I don't have Zacas to hand. r~
On Thu, Aug 22, 2024 at 10:43:05AM +1000, Richard Henderson wrote: >On 8/22/24 07:50, Deepak Gupta wrote: >>@@ -1779,13 +1780,25 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> env->pc += 4; >> return; >> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: >>+ if (always_storeamo) { >>+ cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT; >>+ } >>+ goto load_store_fault; >> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: >> case RISCV_EXCP_LOAD_ADDR_MIS: >> case RISCV_EXCP_STORE_AMO_ADDR_MIS: >> case RISCV_EXCP_LOAD_ACCESS_FAULT: >>+ if (always_storeamo) { >>+ cause = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; >>+ } >>+ goto load_store_fault; >> case RISCV_EXCP_STORE_AMO_ACCESS_FAULT: >> case RISCV_EXCP_LOAD_PAGE_FAULT: >> case RISCV_EXCP_STORE_PAGE_FAULT: >>+ if (always_storeamo) { >>+ cause = RISCV_EXCP_STORE_PAGE_FAULT; >>+ } >>+ load_store_fault: > >These case labels need to be re-sorted; Yeah it looks ugly but I didn't know what's expected. I'll sort cases. >you're mising load/store when you're intending to check for load alone. I didn't get this. >I expect LOAD_ADDR_MIS needs adjustment as well? Hmm atleast for shadow stack, spec says never raise misaligned and raise access fault. Not sure what's the behavior for Atomic memory operations. > >>diff --git a/target/riscv/translate.c b/target/riscv/translate.c >>index d44103a273..8961dda244 100644 >>--- a/target/riscv/translate.c >>+++ b/target/riscv/translate.c >>@@ -121,6 +121,7 @@ typedef struct DisasContext { >> bool fcfi_lp_expected; >> /* zicfiss extension, if shadow stack was enabled during TB gen */ >> bool bcfi_enabled; >>+ target_ulong excp_uw2; >> } DisasContext; >> static inline bool has_ext(DisasContext *ctx, uint32_t ext) >>@@ -144,6 +145,9 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) >> #define get_address_xl(ctx) ((ctx)->address_xl) >> #endif >>+#define SET_INSTR_ALWAYS_STORE_AMO(ctx) \ >>+ (ctx->excp_uw2 |= RISCV_UW2_ALWAYS_STORE_AMO) >>+ >> /* The word size for this machine mode. */ >> static inline int __attribute__((unused)) get_xlen(DisasContext *ctx) >> { >>@@ -214,6 +218,12 @@ static void decode_save_opc(DisasContext *ctx) >> assert(!ctx->insn_start_updated); >> ctx->insn_start_updated = true; >> tcg_set_insn_start_param(ctx->base.insn_start, 1, ctx->opcode); >>+ >>+ if (ctx->excp_uw2) { >>+ tcg_set_insn_start_param(ctx->base.insn_start, 2, >>+ ctx->excp_uw2); >>+ ctx->excp_uw2 = 0; >>+ } > >I really don't think having data on the side like this... Ok. > >> } >> static void gen_pc_plus_diff(TCGv target, DisasContext *ctx, >>@@ -1096,6 +1106,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a, >> mop |= MO_ALIGN; >> } >>+ SET_INSTR_ALWAYS_STORE_AMO(ctx); >> decode_save_opc(ctx); > >... or the requirement for ordering of two function calls is a good interface. > >I did say perhaps add another helper, but what I expected was > > decode_save_opc_set_amo_store(ctx); > >where decode_save_opc and decode_save_opc_set_amo_store call into a common helper. >But perhaps in the end maybe just decode_save_opc(ctx, uw2) is better. > >I expect gen_cmpxchg also needs updating, though I don't have Zacas to hand. I prefer decode_save_opc(ctx, uw2) but then $git grep decode_save_opc | wc -l 38 I can update all these locations but it'll be handful. > > >r~
On 8/22/24 10:58, Deepak Gupta wrote: > On Thu, Aug 22, 2024 at 10:43:05AM +1000, Richard Henderson wrote: >> On 8/22/24 07:50, Deepak Gupta wrote: >>> @@ -1779,13 +1780,25 @@ void riscv_cpu_do_interrupt(CPUState *cs) >>> env->pc += 4; >>> return; >>> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: >>> + if (always_storeamo) { >>> + cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT; >>> + } >>> + goto load_store_fault; >>> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: >>> case RISCV_EXCP_LOAD_ADDR_MIS: >>> case RISCV_EXCP_STORE_AMO_ADDR_MIS: >>> case RISCV_EXCP_LOAD_ACCESS_FAULT: >>> + if (always_storeamo) { >>> + cause = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; >>> + } >>> + goto load_store_fault; >>> case RISCV_EXCP_STORE_AMO_ACCESS_FAULT: >>> case RISCV_EXCP_LOAD_PAGE_FAULT: >>> case RISCV_EXCP_STORE_PAGE_FAULT: >>> + if (always_storeamo) { >>> + cause = RISCV_EXCP_STORE_PAGE_FAULT; >>> + } >>> + load_store_fault: >> >> These case labels need to be re-sorted; > > Yeah it looks ugly but I didn't know what's expected. I'll sort cases. > >> you're mising load/store when you're intending to check for load alone. > > I didn't get this. Fall through of various case groups into the storeamo checks. Only the first RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT case is correct. >> But perhaps in the end maybe just decode_save_opc(ctx, uw2) is better. >> >> I expect gen_cmpxchg also needs updating, though I don't have Zacas to hand. > > I prefer decode_save_opc(ctx, uw2) but then > > $git grep decode_save_opc | wc -l 38 > > I can update all these locations but it'll be handful. That's fine. Let's add the operand and update to pass 0 from existing sites as a separate patch. r~
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index dcc3bc9d93..3143141863 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -46,8 +46,13 @@ 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 instruction always raise a store AMO or not. + */ +#define RISCV_UW2_ALWAYS_STORE_AMO 1 #define RV(x) ((target_ulong)1 << (x - 'A')) @@ -226,6 +231,8 @@ struct CPUArchState { bool elp; /* shadow stack register for zicfiss extension */ target_ulong ssp; + /* env place holder for extra word 2 during unwind */ + target_ulong excp_uw2; /* 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 36dd67befc..a0fc10ddb5 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1753,6 +1753,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) RISCVCPU *cpu = RISCV_CPU(cs); CPURISCVState *env = &cpu->env; bool write_gva = false; + bool always_storeamo = (env->excp_uw2 & RISCV_UW2_ALWAYS_STORE_AMO); uint64_t s; /* @@ -1779,13 +1780,25 @@ void riscv_cpu_do_interrupt(CPUState *cs) env->pc += 4; return; case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: + if (always_storeamo) { + cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT; + } + goto load_store_fault; case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: case RISCV_EXCP_LOAD_ADDR_MIS: case RISCV_EXCP_STORE_AMO_ADDR_MIS: case RISCV_EXCP_LOAD_ACCESS_FAULT: + if (always_storeamo) { + cause = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; + } + goto load_store_fault; case RISCV_EXCP_STORE_AMO_ACCESS_FAULT: case RISCV_EXCP_LOAD_PAGE_FAULT: case RISCV_EXCP_STORE_PAGE_FAULT: + if (always_storeamo) { + cause = RISCV_EXCP_STORE_PAGE_FAULT; + } + load_store_fault: write_gva = env->two_stage_lookup; tval = env->badaddr; if (env->two_stage_indirect_lookup) { diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 4da26cb926..83771303a8 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->excp_uw2 = data[2]; } static const TCGCPUOps riscv_tcg_ops = { diff --git a/target/riscv/translate.c b/target/riscv/translate.c index d44103a273..8961dda244 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -121,6 +121,7 @@ typedef struct DisasContext { bool fcfi_lp_expected; /* zicfiss extension, if shadow stack was enabled during TB gen */ bool bcfi_enabled; + target_ulong excp_uw2; } DisasContext; static inline bool has_ext(DisasContext *ctx, uint32_t ext) @@ -144,6 +145,9 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) #define get_address_xl(ctx) ((ctx)->address_xl) #endif +#define SET_INSTR_ALWAYS_STORE_AMO(ctx) \ + (ctx->excp_uw2 |= RISCV_UW2_ALWAYS_STORE_AMO) + /* The word size for this machine mode. */ static inline int __attribute__((unused)) get_xlen(DisasContext *ctx) { @@ -214,6 +218,12 @@ static void decode_save_opc(DisasContext *ctx) assert(!ctx->insn_start_updated); ctx->insn_start_updated = true; tcg_set_insn_start_param(ctx->base.insn_start, 1, ctx->opcode); + + if (ctx->excp_uw2) { + tcg_set_insn_start_param(ctx->base.insn_start, 2, + ctx->excp_uw2); + ctx->excp_uw2 = 0; + } } static void gen_pc_plus_diff(TCGv target, DisasContext *ctx, @@ -1096,6 +1106,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a, mop |= MO_ALIGN; } + SET_INSTR_ALWAYS_STORE_AMO(ctx); decode_save_opc(ctx); src1 = get_address(ctx, a->rs1, 0); func(dest, src1, src2, ctx->mem_idx, mop); @@ -1250,6 +1261,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->zero = tcg_constant_tl(0); ctx->virt_inst_excp = false; ctx->decoders = cpu->decoders; + ctx->excp_uw2 = 0; } static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu) @@ -1265,7 +1277,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; }
This patch adds one more word for tcg compile which can be obtained during unwind time to determine fault type for original operation (example AMO). Depending on that, fault can be promoted to store/AMO fault. Signed-off-by: Deepak Gupta <debug@rivosinc.com> Suggested-by: Richard Henderson <richard.henderson@linaro.org> --- target/riscv/cpu.h | 9 ++++++++- target/riscv/cpu_helper.c | 13 +++++++++++++ target/riscv/tcg/tcg-cpu.c | 1 + target/riscv/translate.c | 14 +++++++++++++- 4 files changed, 35 insertions(+), 2 deletions(-)