| Message ID | 20240207025210.8837-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: assorted mte fixes | expand |
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
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~
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);