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~