| Message ID | 20230216030854.1212208-15-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_LSE2 | expand |
On Thu, 16 Feb 2023 at 03:11, Richard Henderson <richard.henderson@linaro.org> wrote: > > Fixes a bug in that with SCTLR.A set, we should raise any > alignment fault before raising any MTE check fault. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/internals.h | 3 ++- > target/arm/mte_helper.c | 18 ++++++++++++++++++ > target/arm/translate-a64.c | 2 ++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index e1e018da46..fa264e368c 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -1222,7 +1222,8 @@ FIELD(MTEDESC, MIDX, 0, 4) > FIELD(MTEDESC, TBI, 4, 2) > FIELD(MTEDESC, TCMA, 6, 2) > FIELD(MTEDESC, WRITE, 8, 1) > -FIELD(MTEDESC, SIZEM1, 9, SIMD_DATA_BITS - 9) /* size - 1 */ > +FIELD(MTEDESC, ALIGN, 9, 3) > +FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - 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/mte_helper.c b/target/arm/mte_helper.c > index 98bcf59c22..e50bb4ea13 100644 > --- a/target/arm/mte_helper.c > +++ b/target/arm/mte_helper.c > @@ -784,6 +784,24 @@ uint64_t mte_check(CPUARMState *env, uint32_t desc, uint64_t ptr, uintptr_t ra) > > uint64_t HELPER(mte_check)(CPUARMState *env, uint32_t desc, uint64_t ptr) > { > + /* > + * In the Arm ARM pseudocode, the alignment check happens at the top > + * of Mem[], while the MTE check happens later in AArch64.MemSingle[]. > + * Thus the alignment check has priority. > + * When the mte check is disabled, tcg performs the alignment check > + * during the code generated for the memory access. > + */ Also described in the text: the I_ZFGJP priority table lists MTE faults at priority 33, basically lower than anything else except an external abort. Looking at the code, is this really the only case here where we were mis-prioritizing tag check faults? Have we already checked things like "no page table entry" and all the other cases that can cause data aborts at this point? thanks -- PMM
On 2/23/23 06:28, Peter Maydell wrote: > Also described in the text: the I_ZFGJP priority table lists > MTE faults at priority 33, basically lower than anything else > except an external abort. > > Looking at the code, is this really the only case here where > we were mis-prioritizing tag check faults? Have we already > checked things like "no page table entry" and all the other > cases that can cause data aborts at this point? Well, yes, we do the page table lookup in allocation_tag_mem(), while fetching the tag memory against which the check is performed. If there's another mis-prioritization, I don't know about it. r~
On Thu, 16 Feb 2023 at 03:11, Richard Henderson <richard.henderson@linaro.org> wrote: > > Fixes a bug in that with SCTLR.A set, we should raise any > alignment fault before raising any MTE check fault. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/target/arm/internals.h b/target/arm/internals.h index e1e018da46..fa264e368c 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1222,7 +1222,8 @@ FIELD(MTEDESC, MIDX, 0, 4) FIELD(MTEDESC, TBI, 4, 2) FIELD(MTEDESC, TCMA, 6, 2) FIELD(MTEDESC, WRITE, 8, 1) -FIELD(MTEDESC, SIZEM1, 9, SIMD_DATA_BITS - 9) /* size - 1 */ +FIELD(MTEDESC, ALIGN, 9, 3) +FIELD(MTEDESC, SIZEM1, 12, SIMD_DATA_BITS - 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/mte_helper.c b/target/arm/mte_helper.c index 98bcf59c22..e50bb4ea13 100644 --- a/target/arm/mte_helper.c +++ b/target/arm/mte_helper.c @@ -784,6 +784,24 @@ uint64_t mte_check(CPUARMState *env, uint32_t desc, uint64_t ptr, uintptr_t ra) uint64_t HELPER(mte_check)(CPUARMState *env, uint32_t desc, uint64_t ptr) { + /* + * In the Arm ARM pseudocode, the alignment check happens at the top + * of Mem[], while the MTE check happens later in AArch64.MemSingle[]. + * Thus the alignment check has priority. + * When the mte check is disabled, tcg performs the alignment check + * during the code generated for the memory access. + */ + unsigned align = FIELD_EX32(desc, MTEDESC, ALIGN); + if (unlikely(align)) { + align = (1u << align) - 1; + if (unlikely(ptr & align)) { + int idx = FIELD_EX32(desc, MTEDESC, MIDX); + bool w = FIELD_EX32(desc, MTEDESC, WRITE); + MMUAccessType type = w ? MMU_DATA_STORE : MMU_DATA_LOAD; + arm_cpu_do_unaligned_access(env_cpu(env), ptr, type, idx, GETPC()); + } + } + return mte_check(env, desc, ptr, GETPC()); } diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 1117a1cc41..caeb91efa5 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -267,6 +267,7 @@ static TCGv_i64 gen_mte_check1_mmuidx(DisasContext *s, TCGv_i64 addr, 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, ALIGN, get_alignment_bits(memop)); desc = FIELD_DP32(desc, MTEDESC, SIZEM1, memop_size(memop) - 1); ret = new_tmp_a64(s); @@ -298,6 +299,7 @@ TCGv_i64 gen_mte_checkN(DisasContext *s, TCGv_i64 addr, bool is_write, 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, ALIGN, get_alignment_bits(single_mop)); desc = FIELD_DP32(desc, MTEDESC, SIZEM1, total_size - 1); ret = new_tmp_a64(s);
Fixes a bug in that with SCTLR.A set, we should raise any alignment fault before raising any MTE check fault. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/internals.h | 3 ++- target/arm/mte_helper.c | 18 ++++++++++++++++++ target/arm/translate-a64.c | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-)