1. Patchew
  2. QEMU
  3. [PATCH v3 0/6] target/arm: assorted mte fixes
  4. View patch

[PATCH v3 3/6] target/arm: Adjust and validate mtedesc sizem1

Richard Henderson posted 6 patches 9 months, 3 weeks ago
Maintainers: Laurent Vivier <laurent@vivier.eu>, Peter Maydell <peter.maydell@linaro.org>
[PATCH v3 3/6] target/arm: Adjust and validate mtedesc sizem1
Posted by Richard Henderson 9 months, 3 weeks ago 
When we added SVE_MTEDESC_SHIFT, we effectively limited the
maximum size of MTEDESC.  Adjust SIZEM1 to consume the remaining
bits (32 - 10 - 5 - 12 == 5).  Assert that the data to be stored
fits within the field (expecting 8 * 4 - 1 == 31, exact fit).

Cc: qemu-stable@nongnu.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h         | 2 +-
 target/arm/tcg/translate-sve.c | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index fc337fe40e..50bff44549 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1278,7 +1278,7 @@ FIELD(MTEDESC, TBI,   4, 2)
 FIELD(MTEDESC, TCMA,  6, 2)
 FIELD(MTEDESC, WRITE, 8, 1)
 FIELD(MTEDESC, ALIGN, 9, 3)
-FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - 12)  /* size - 1 */
+FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - SVE_MTEDESC_SHIFT - 12)  /* size - 1 */
 
 bool mte_probe(CPUARMState *env, uint32_t desc, uint64_t ptr);
 uint64_t mte_check(CPUARMState *env, uint32_t desc, uint64_t ptr, uintptr_t ra);
diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c
index 7108938251..a88e523cba 100644
--- a/target/arm/tcg/translate-sve.c
+++ b/target/arm/tcg/translate-sve.c
@@ -4443,17 +4443,18 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr,
 {
     unsigned vsz = vec_full_reg_size(s);
     TCGv_ptr t_pg;
+    uint32_t sizem1;
     int desc = 0;
 
     assert(mte_n >= 1 && mte_n <= 4);
+    sizem1 = (mte_n << dtype_msz(dtype)) - 1;
+    assert(sizem1 <= R_MTEDESC_SIZEM1_MASK >> R_MTEDESC_SIZEM1_SHIFT);
     if (s->mte_active[0]) {
-        int msz = dtype_msz(dtype);
-
         desc = FIELD_DP32(desc, MTEDESC, MIDX, get_mem_index(s));
         desc = FIELD_DP32(desc, MTEDESC, TBI, s->tbid);
         desc = FIELD_DP32(desc, MTEDESC, TCMA, s->tcma);
         desc = FIELD_DP32(desc, MTEDESC, WRITE, is_write);
-        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, (mte_n << msz) - 1);
+        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, sizem1);
         desc <<= SVE_MTEDESC_SHIFT;
     } else {
         addr = clean_data_tbi(s, addr);
-- 
2.34.1
Re: [PATCH v3 3/6] target/arm: Adjust and validate mtedesc sizem1
Posted by Michael Tokarev 9 months, 1 week ago 
07.02.2024 05:52, Richard Henderson :
> When we added SVE_MTEDESC_SHIFT, we effectively limited the
> maximum size of MTEDESC.  Adjust SIZEM1 to consume the remaining
> bits (32 - 10 - 5 - 12 == 5).  Assert that the data to be stored
> fits within the field (expecting 8 * 4 - 1 == 31, exact fit).
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/internals.h         | 2 +-
>   target/arm/tcg/translate-sve.c | 7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index fc337fe40e..50bff44549 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1278,7 +1278,7 @@ FIELD(MTEDESC, TBI,   4, 2)
>   FIELD(MTEDESC, TCMA,  6, 2)
>   FIELD(MTEDESC, WRITE, 8, 1)
>   FIELD(MTEDESC, ALIGN, 9, 3)
> -FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - 12)  /* size - 1 */
> +FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - SVE_MTEDESC_SHIFT - 12)  /* size - 1 */
>   
>   bool mte_probe(CPUARMState *env, uint32_t desc, uint64_t ptr);
>   uint64_t mte_check(CPUARMState *env, uint32_t desc, uint64_t ptr, uintptr_t ra);
> diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c
> index 7108938251..a88e523cba 100644
> --- a/target/arm/tcg/translate-sve.c
> +++ b/target/arm/tcg/translate-sve.c
> @@ -4443,17 +4443,18 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr,
>   {
>       unsigned vsz = vec_full_reg_size(s);
>       TCGv_ptr t_pg;
> +    uint32_t sizem1;
>       int desc = 0;
>   
>       assert(mte_n >= 1 && mte_n <= 4);
> +    sizem1 = (mte_n << dtype_msz(dtype)) - 1;
> +    assert(sizem1 <= R_MTEDESC_SIZEM1_MASK >> R_MTEDESC_SIZEM1_SHIFT);
>       if (s->mte_active[0]) {
> -        int msz = dtype_msz(dtype);
> -
>           desc = FIELD_DP32(desc, MTEDESC, MIDX, get_mem_index(s));
>           desc = FIELD_DP32(desc, MTEDESC, TBI, s->tbid);
>           desc = FIELD_DP32(desc, MTEDESC, TCMA, s->tcma);
>           desc = FIELD_DP32(desc, MTEDESC, WRITE, is_write);
> -        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, (mte_n << msz) - 1);
> +        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, sizem1);
>           desc <<= SVE_MTEDESC_SHIFT;
>       } else {
>           addr = clean_data_tbi(s, addr);

There's no question about stable-8.2 here, this change needed there.
But I've a question about stable-7.2 - does it make sense to pick this
one up for 7.2?  It quickly goes out of control, because this one
is on top of

  523da6b963455ce0a0e8d572d98d9cd91f952785 target/arm: Check alignment in helper_mte_check
  (this one might be good for 7.2 by its own)
  which needs:
   3b97520c86e704b0533627c26b98173b71834bad target/arm: Pass single_memop to gen_mte_checkN
   which needs:
    6f47e7c18972802c428a5e03eb52a8f0a7bebe5c target/arm: Load/store integer pair with one tcg operation
    which needs:
     needs 128bit ops
     659aed5feda4472d8aed4ccc69e125bba2af8b89 target/arm: Drop tcg_temp_free from translator-a64.c
     ...

So I think it's not a good idea to go down this hole..

Probably ditto for the other two:
   target/arm: Split out make_svemte_desc
   target/arm: Handle mte in do_ldrq, do_ldro

Makes sense?  Or it's better to do a proper backport?

Thanks,

/mjt
Re: [PATCH v3 3/6] target/arm: Adjust and validate mtedesc sizem1
Posted by Richard Henderson 9 months, 1 week ago 
On 2/16/24 05:12, Michael Tokarev wrote:
> 07.02.2024 05:52, Richard Henderson :
>> When we added SVE_MTEDESC_SHIFT, we effectively limited the
>> maximum size of MTEDESC.  Adjust SIZEM1 to consume the remaining
>> bits (32 - 10 - 5 - 12 == 5).  Assert that the data to be stored
>> fits within the field (expecting 8 * 4 - 1 == 31, exact fit).
>>
>> Cc: qemu-stable@nongnu.org
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/internals.h         | 2 +-
>>   target/arm/tcg/translate-sve.c | 7 ++++---
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index fc337fe40e..50bff44549 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1278,7 +1278,7 @@ FIELD(MTEDESC, TBI,   4, 2)
>>   FIELD(MTEDESC, TCMA,  6, 2)
>>   FIELD(MTEDESC, WRITE, 8, 1)
>>   FIELD(MTEDESC, ALIGN, 9, 3)
>> -FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - 12)  /* size - 1 */
>> +FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - SVE_MTEDESC_SHIFT - 12)  /* size - 1 */
>>   bool mte_probe(CPUARMState *env, uint32_t desc, uint64_t ptr);
>>   uint64_t mte_check(CPUARMState *env, uint32_t desc, uint64_t ptr, uintptr_t ra);
>> diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c
>> index 7108938251..a88e523cba 100644
>> --- a/target/arm/tcg/translate-sve.c
>> +++ b/target/arm/tcg/translate-sve.c
>> @@ -4443,17 +4443,18 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 
>> addr,
>>   {
>>       unsigned vsz = vec_full_reg_size(s);
>>       TCGv_ptr t_pg;
>> +    uint32_t sizem1;
>>       int desc = 0;
>>       assert(mte_n >= 1 && mte_n <= 4);
>> +    sizem1 = (mte_n << dtype_msz(dtype)) - 1;
>> +    assert(sizem1 <= R_MTEDESC_SIZEM1_MASK >> R_MTEDESC_SIZEM1_SHIFT);
>>       if (s->mte_active[0]) {
>> -        int msz = dtype_msz(dtype);
>> -
>>           desc = FIELD_DP32(desc, MTEDESC, MIDX, get_mem_index(s));
>>           desc = FIELD_DP32(desc, MTEDESC, TBI, s->tbid);
>>           desc = FIELD_DP32(desc, MTEDESC, TCMA, s->tcma);
>>           desc = FIELD_DP32(desc, MTEDESC, WRITE, is_write);
>> -        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, (mte_n << msz) - 1);
>> +        desc = FIELD_DP32(desc, MTEDESC, SIZEM1, sizem1);
>>           desc <<= SVE_MTEDESC_SHIFT;
>>       } else {
>>           addr = clean_data_tbi(s, addr);
> 
> There's no question about stable-8.2 here, this change needed there.
> But I've a question about stable-7.2 - does it make sense to pick this
> one up for 7.2?  It quickly goes out of control, because this one
> is on top of
> 
>   523da6b963455ce0a0e8d572d98d9cd91f952785 target/arm: Check alignment in helper_mte_check
>   (this one might be good for 7.2 by its own)
>   which needs:
>    3b97520c86e704b0533627c26b98173b71834bad target/arm: Pass single_memop to gen_mte_checkN
>    which needs:
>     6f47e7c18972802c428a5e03eb52a8f0a7bebe5c target/arm: Load/store integer pair with one 
> tcg operation
>     which needs:
>      needs 128bit ops
>      659aed5feda4472d8aed4ccc69e125bba2af8b89 target/arm: Drop tcg_temp_free from 
> translator-a64.c
>      ...
> 
> So I think it's not a good idea to go down this hole..
> 
> Probably ditto for the other two:
>    target/arm: Split out make_svemte_desc
>    target/arm: Handle mte in do_ldrq, do_ldro
> 
> Makes sense?  Or it's better to do a proper backport?

I don't think it makes sense to go back too far.

r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.