Re: [PATCH v2 3/3] target/riscv: Relax fld alignment requirement

[Richard Henderson <richard.henderson@linaro.org>]

On 8/2/24 13:16, LIU Zhiwei wrote:
> According to the risc-v specification:
> "FLD and FSD are only guaranteed to execute atomically if the effective
> address is naturally aligned and XLEN≥64."
> 
> We currently implement fld as MO_ATOM_IFALIGN when XLEN < 64, which does
> not violate the rules. But it will hide some problems. So relax it to
> MO_ATOM_NONE.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   target/riscv/insn_trans/trans_rvd.c.inc | 26 ++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
> index dbe508c7e0..458d7db745 100644
> --- a/target/riscv/insn_trans/trans_rvd.c.inc
> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> @@ -48,12 +48,20 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>       REQUIRE_EXT(ctx, RVD);
>   
>       /*
> -     * Zama16b applies to loads and stores of no more than MXLEN bits defined
> -     * in the F, D, and Q extensions. Otherwise, it falls through to default
> -     * MO_ATOM_IFALIGN.
> +     * FLD and FSD are only guaranteed to execute atomically if the effective
> +     * address is naturally aligned and XLEN≥64.
>        */
> -    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
> -        memop |= MO_ATOM_WITHIN16;
> +    if (ctx->misa_mxl_max >= MXL_RV64) {
> +        /*
> +         * Zama16b applies to loads and stores of no more than MXLEN bits
> +         * defined in the F, D, and Q extensions. Otherwise, it falls through
> +         * to default MO_ATOM_IFALIGN.
> +         */
> +        if (ctx->cfg_ptr->ext_zama16b) {
> +            memop |= MO_ATOM_WITHIN16;
> +        }
> +    } else {
> +        memop |= MO_ATOM_NONE;
>       }

Does this really have byte atomicity, not atomic on two aligned 32-bit loads (which would 
be MO_ATOM_IFALIGN_PAIR).

It's probably clearer to fill out the if-tree completely,
rather than explain about defaults.

     if (get_mxl(ctx) == MXL_RV32) {
         memop |= MO_ATOM_NONE;
     } else if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     } else {
         memop |= MO_ATOM_IFALIGN;
     }


r~