| Message ID | 20201221204426.88514-1-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Fix MTE0_ACTIVE | expand |
On Mon, 21 Dec 2020 at 20:44, Richard Henderson <richard.henderson@linaro.org> wrote: > > In 50244cc76abc we updated mte_check_fail to match the ARM > pseudocode, using the correct EL to select the TCF field. > But we failed to update MTE0_ACTIVE the same way, which led > to g_assert_not_reached(). > > Cc: qemu-stable@nongnu.org > Buglink: https://bugs.launchpad.net/bugs/1907137 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 7b8bcd6903..4597081d5d 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -12932,7 +12932,7 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el, > if (FIELD_EX32(flags, TBFLAG_A64, UNPRIV) > && tbid > && !(env->pstate & PSTATE_TCO) > - && (sctlr & SCTLR_TCF0) > + && (sctlr & SCTLR_TCF) > && allocation_tag_access_enabled(env, 0, sctlr)) { > flags = FIELD_DP32(flags, TBFLAG_A64, MTE0_ACTIVE, 1); > } I don't understand this change, could you explain a bit more? In commit 50244cc76abcac we change to looking at the TCF field corresponding to the actual current EL instead of the EL for the memory-access. But if we're doing that then why should we be looking at exclusively SCTLR_TCF0 in this for-unpriv-access code rather than doing the same thing we do for normal accesses and checking (sctlr & (el == 0 ? SCTLR_TCF0 : SCTLR_TCF)) ? thanks -- PMM
On 1/7/21 7:54 AM, Peter Maydell wrote: >> - && (sctlr & SCTLR_TCF0) >> + && (sctlr & SCTLR_TCF) >> && allocation_tag_access_enabled(env, 0, sctlr)) { >> flags = FIELD_DP32(flags, TBFLAG_A64, MTE0_ACTIVE, 1); >> } > > > I don't understand this change, could you explain a bit more? > In commit 50244cc76abcac we change to looking at the TCF > field corresponding to the actual current EL instead of the > EL for the memory-access. Correct. > But if we're doing that then why > should we be looking at exclusively SCTLR_TCF0 in this > for-unpriv-access code rather than doing the same thing we do > for normal accesses and checking > (sctlr & (el == 0 ? SCTLR_TCF0 : SCTLR_TCF)) Because this is for the UNPRIV instructions which are UNDEF at el == 0. r~
On Thu, 7 Jan 2021 at 19:10, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 1/7/21 7:54 AM, Peter Maydell wrote: > >> - && (sctlr & SCTLR_TCF0) > >> + && (sctlr & SCTLR_TCF) > >> && allocation_tag_access_enabled(env, 0, sctlr)) { > >> flags = FIELD_DP32(flags, TBFLAG_A64, MTE0_ACTIVE, 1); > >> } > > > > > > I don't understand this change, could you explain a bit more? > > In commit 50244cc76abcac we change to looking at the TCF > > field corresponding to the actual current EL instead of the > > EL for the memory-access. > > Correct. > > > But if we're doing that then why > > should we be looking at exclusively SCTLR_TCF0 in this > > for-unpriv-access code rather than doing the same thing we do > > for normal accesses and checking > > (sctlr & (el == 0 ? SCTLR_TCF0 : SCTLR_TCF)) > > Because this is for the UNPRIV instructions which are UNDEF at el == 0. Ah, right. (It didn't help that I'd read the diff backwards: the new code looks at SCTLR_TCF, not SCTLR_TCF0.) Further, the SCTLR_*.ATA/ATA0 checks *are* based on the privilege of the access, which is why calling allocation_tag_access_enabled(env, 0, sctlr) is still correct. Applied to target-arm.next, thanks. -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 7b8bcd6903..4597081d5d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12932,7 +12932,7 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el, if (FIELD_EX32(flags, TBFLAG_A64, UNPRIV) && tbid && !(env->pstate & PSTATE_TCO) - && (sctlr & SCTLR_TCF0) + && (sctlr & SCTLR_TCF) && allocation_tag_access_enabled(env, 0, sctlr)) { flags = FIELD_DP32(flags, TBFLAG_A64, MTE0_ACTIVE, 1); }
In 50244cc76abc we updated mte_check_fail to match the ARM pseudocode, using the correct EL to select the TCF field. But we failed to update MTE0_ACTIVE the same way, which led to g_assert_not_reached(). Cc: qemu-stable@nongnu.org Buglink: https://bugs.launchpad.net/bugs/1907137 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)