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);
         }
-- 
2.25.1

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