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

[Deepak Gupta]

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~