Re: [PATCH v6 12/16] target/riscv: AMO operations always raise store/AMO fault

[Deepak Gupta <debug@rivosinc.com>]

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~