1. Patchew
  2. QEMU
  3. [PATCH v2 0/3] target/riscv: Remove redundant insn length check for zama16b
  4. View patch

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

LIU Zhiwei posted 3 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v2 3/3] target/riscv: Relax fld alignment requirement
Posted by LIU Zhiwei 3 months, 3 weeks ago 
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;
     }
 
     decode_save_opc(ctx);
@@ -72,8 +80,12 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
-        memop |= MO_ATOM_WITHIN16;
+    if (ctx->misa_mxl_max >= MXL_RV64) {
+        if (ctx->cfg_ptr->ext_zama16b) {
+            memop |= MO_ATOM_WITHIN16;
+        }
+    } else {
+        memop |= MO_ATOM_NONE;
     }
 
     decode_save_opc(ctx);
-- 
2.25.1
Re: [PATCH v2 3/3] target/riscv: Relax fld alignment requirement
Posted by Richard Henderson 3 months, 3 weeks ago 
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~
Re: [PATCH v2 3/3] target/riscv: Relax fld alignment requirement
Posted by LIU Zhiwei 3 months, 3 weeks ago 
On 2024/8/2 13:52, Richard Henderson wrote:
> 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).
The specification doesn't rule it. I think we can choose either way. The 
byte atomicity may expose more problems on alignment.
>
> 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;
>     }

OK.

Thanks,
Zhiwei

>
>
> r~
Re: [PATCH v2 3/3] target/riscv: Relax fld alignment requirement
Posted by Richard Henderson 3 months, 3 weeks ago 
On 8/2/24 16:27, LIU Zhiwei wrote:
>> Does this really have byte atomicity, not atomic on two aligned 32-bit loads (which 
>> would be MO_ATOM_IFALIGN_PAIR).
> The specification doesn't rule it. I think we can choose either way. The byte atomicity 
> may expose more problems on alignment.

Ok.

r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.