[PATCH v2 0/3] target/riscv: Remove redundant insn length check for zama16b

[PATCH v2 0/3] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

In this patch set, we remove the redundant insn length check for zama16b as the
specification clarified that zama16b applies to compressed encodings[1].

Richard points out we should obey the MXLEN requirement for F/D/Q loads or stores,
so we add this constraint for trans_fld/fsd.

I notice that we have a too strict aligment implementation for fld/fsd when xlen < 64.
It will hide some problems. So relex it from MO_ATOM_IFALIGN to MO_ATOM_NONE.

[1]: https://github.com/riscv/riscv-isa-manual/pull/1557

v2<-v1:
  1. Add mxlen check for fld when applies zama16b.
  2. Relax fld/fsd alignment for MO_ATOM_IFALIGN to MO_ATOM_NONE.

LIU Zhiwei (3):
  target/riscv: Remove redundant insn length check for zama16b
  target/riscv: Add MXLEN check for F/D/Q applies to zama16b
  target/riscv: Relax fld alignment requirement

 target/riscv/insn_trans/trans_rvd.c.inc | 25 +++++++++++++++++++++----
 target/riscv/insn_trans/trans_rvf.c.inc |  4 ++--
 target/riscv/insn_trans/trans_rvi.c.inc |  4 ++--
 3 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

Compressed encodings also applies to zama16b.
https://github.com/riscv/riscv-isa-manual/pull/1557

Suggested-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/insn_trans/trans_rvd.c.inc | 9 +++++++--
 target/riscv/insn_trans/trans_rvf.c.inc | 4 ++--
 target/riscv/insn_trans/trans_rvi.c.inc | 4 ++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 1f5fac65a2..2be037930a 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -47,7 +47,12 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    /*
+     * 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->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
         memop |= MO_ATOM_WITHIN16;
     }
 
@@ -67,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
 
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index f771aa1939..0222a728df 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -48,7 +48,7 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
 
@@ -70,7 +70,7 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
 
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 98e3806d5e..fab5c06719 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -268,7 +268,7 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
 {
     bool out;
 
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
     decode_save_opc(ctx);
@@ -369,7 +369,7 @@ static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
 
 static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
 {
-    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
+    if (ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
     decode_save_opc(ctx);
-- 
2.25.1

Re: [PATCH v2 1/3] target/riscv: Remove redundant insn length check for zama16b

[Richard Henderson]

On 8/2/24 13:16, LIU Zhiwei wrote:
> @@ -47,7 +47,12 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>       REQUIRE_FPU;
>       REQUIRE_EXT(ctx, RVD);
>   
> -    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
> +    /*
> +     * 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->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {

I think you meant to add the mxlen check in the next patch,
because you modify this line again.


r~

Re: [PATCH v2 1/3] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

On 2024/8/2 13:38, Richard Henderson wrote:
> On 8/2/24 13:16, LIU Zhiwei wrote:
>> @@ -47,7 +47,12 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>>       REQUIRE_FPU;
>>       REQUIRE_EXT(ctx, RVD);
>>   -    if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) {
>> +    /*
>> +     * 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->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
>
> I think you meant to add the mxlen check in the next patch,
> because you modify this line again.
>
Oh, I didn't notice it.  I once split the whole patch into two patches. 
But obviously I did only a half.

Thanks,
Zhiwei

>
> r~

[PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[LIU Zhiwei]

Zama16b loads and stores of no more than MXLEN bits defined in the F, D, and Q
extensions.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/insn_trans/trans_rvd.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 2be037930a..dbe508c7e0 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -52,7 +52,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
      * in the F, D, and Q extensions. Otherwise, it falls through to default
      * MO_ATOM_IFALIGN.
      */
-    if ((ctx->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
+    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
 
@@ -72,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    if (ctx->cfg_ptr->ext_zama16b) {
+    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
         memop |= MO_ATOM_WITHIN16;
     }
 
-- 
2.25.1

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[Richard Henderson]

On 8/2/24 13:16, LIU Zhiwei wrote:
> Zama16b loads and stores of no more than MXLEN bits defined in the F, D, and Q
> extensions.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   target/riscv/insn_trans/trans_rvd.c.inc | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
> index 2be037930a..dbe508c7e0 100644
> --- a/target/riscv/insn_trans/trans_rvd.c.inc
> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> @@ -52,7 +52,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>        * in the F, D, and Q extensions. Otherwise, it falls through to default
>        * MO_ATOM_IFALIGN.
>        */
> -    if ((ctx->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>           memop |= MO_ATOM_WITHIN16;
>       }
>   
> @@ -72,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
>       REQUIRE_FPU;
>       REQUIRE_EXT(ctx, RVD);
>   
> -    if (ctx->cfg_ptr->ext_zama16b) {
> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>           memop |= MO_ATOM_WITHIN16;
>       }

I guess this is ok, because MXL cannot currently be changed. But since that is a possible 
future enhancement, perhaps add a get_mxl(ctx) accessor anyway, for documentation purposes.


r~

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[LIU Zhiwei]

On 2024/8/2 13:47, Richard Henderson wrote:
> On 8/2/24 13:16, LIU Zhiwei wrote:
>> Zama16b loads and stores of no more than MXLEN bits defined in the F, 
>> D, and Q
>> extensions.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   target/riscv/insn_trans/trans_rvd.c.inc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
>> b/target/riscv/insn_trans/trans_rvd.c.inc
>> index 2be037930a..dbe508c7e0 100644
>> --- a/target/riscv/insn_trans/trans_rvd.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
>> @@ -52,7 +52,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>>        * in the F, D, and Q extensions. Otherwise, it falls through 
>> to default
>>        * MO_ATOM_IFALIGN.
>>        */
>> -    if ((ctx->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>           memop |= MO_ATOM_WITHIN16;
>>       }
>>   @@ -72,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
>>       REQUIRE_FPU;
>>       REQUIRE_EXT(ctx, RVD);
>>   -    if (ctx->cfg_ptr->ext_zama16b) {
>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>           memop |= MO_ATOM_WITHIN16;
>>       }
>
> I guess this is ok, because MXL cannot currently be changed. But since 
> that is a possible future enhancement, perhaps add a get_mxl(ctx) 
> accessor anyway, for documentation purposes.

OK, I will use it. By the way, the MXL is const now in recently updated 
RISC-V specification.

Thanks，
Zhiwei

>
>
> r~

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[Richard Henderson]

On 8/2/24 16:21, LIU Zhiwei wrote:
> By the way, the MXL is const now in recently updated RISC-V specification.

Oh yes?  Then perhaps we should rename misa_mxl_max to misa_mxl.


r~

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[LIU Zhiwei]

On 2024/8/2 14:45, Richard Henderson wrote:
> On 8/2/24 16:21, LIU Zhiwei wrote:
>> By the way, the MXL is const now in recently updated RISC-V 
>> specification.
>
> Oh yes? 
Yes.  In 1.13 privileged specification about MISA CSR:

"The MXL field is read-only. If misa is nonzero, the MXL field indicates 
the effective XLEN in M-mode, a
constant termed MXLEN."

> Then perhaps we should rename misa_mxl_max to misa_mxl.

I will mark this on my todo list.

Thanks,
Zhiwei

>
>
> r~

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[LIU Zhiwei]

On 2024/8/2 13:47, Richard Henderson wrote:
> On 8/2/24 13:16, LIU Zhiwei wrote:
>> Zama16b loads and stores of no more than MXLEN bits defined in the F, 
>> D, and Q
>> extensions.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   target/riscv/insn_trans/trans_rvd.c.inc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
>> b/target/riscv/insn_trans/trans_rvd.c.inc
>> index 2be037930a..dbe508c7e0 100644
>> --- a/target/riscv/insn_trans/trans_rvd.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
>> @@ -52,7 +52,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>>        * in the F, D, and Q extensions. Otherwise, it falls through 
>> to default
>>        * MO_ATOM_IFALIGN.
>>        */
>> -    if ((ctx->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>           memop |= MO_ATOM_WITHIN16;
>>       }
>>   @@ -72,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
>>       REQUIRE_FPU;
>>       REQUIRE_EXT(ctx, RVD);
>>   -    if (ctx->cfg_ptr->ext_zama16b) {
>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>           memop |= MO_ATOM_WITHIN16;
>>       }
>
> I guess this is ok, because MXL cannot currently be changed. But since 
> that is a possible future enhancement, perhaps add a get_mxl(ctx) 
> accessor anyway, for documentation purposes.
Can we use the existing get_xl_max(ctx) instead of adding a new 
get_mxl(ctx).

Thanks,
Zhiwei

>
>
> r~

Re: [PATCH v2 2/3] target/riscv: Add MXLEN check for F/D/Q applies to zama16b

[Richard Henderson]

On 8/2/24 16:42, LIU Zhiwei wrote:
> 
> On 2024/8/2 13:47, Richard Henderson wrote:
>> On 8/2/24 13:16, LIU Zhiwei wrote:
>>> Zama16b loads and stores of no more than MXLEN bits defined in the F, D, and Q
>>> extensions.
>>>
>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>>> ---
>>>   target/riscv/insn_trans/trans_rvd.c.inc | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/ 
>>> trans_rvd.c.inc
>>> index 2be037930a..dbe508c7e0 100644
>>> --- a/target/riscv/insn_trans/trans_rvd.c.inc
>>> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
>>> @@ -52,7 +52,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
>>>        * in the F, D, and Q extensions. Otherwise, it falls through to default
>>>        * MO_ATOM_IFALIGN.
>>>        */
>>> -    if ((ctx->xl >= MXL_RV64) && (ctx->cfg_ptr->ext_zama16b)) {
>>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>>           memop |= MO_ATOM_WITHIN16;
>>>       }
>>>   @@ -72,7 +72,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
>>>       REQUIRE_FPU;
>>>       REQUIRE_EXT(ctx, RVD);
>>>   -    if (ctx->cfg_ptr->ext_zama16b) {
>>> +    if ((ctx->misa_mxl_max >= MXL_RV64) && ctx->cfg_ptr->ext_zama16b) {
>>>           memop |= MO_ATOM_WITHIN16;
>>>       }
>>
>> I guess this is ok, because MXL cannot currently be changed. But since that is a 
>> possible future enhancement, perhaps add a get_mxl(ctx) accessor anyway, for 
>> documentation purposes.
> Can we use the existing get_xl_max(ctx) instead of adding a new get_mxl(ctx).

Yes, that looks fine.
That could be renamed get_mxl.  :-)


r~

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

[LIU Zhiwei]

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

[Richard Henderson]

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

[LIU Zhiwei]

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

[Richard Henderson]

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~