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

[Richard Henderson]

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~