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

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

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~