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

[Richard Henderson]

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~