Re: [PATCH v5 19/22] target/arm: Cache the Tagged bit for a page in MemTxAttrs

From: Peter Maydell <peter.maydell@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-arm <qemu-arm@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v5 19/22] target/arm: Cache the Tagged bit for a page in MemTxAttrs
Date: Thu, 5 Dec 2019 18:32:31 +0000	[thread overview]
Message-ID: <CAFEAcA-2uT9Pj=HCP+ZrCy2GygNy+BhME0o5P8OQReEdKJgscg@mail.gmail.com> (raw)
In-Reply-To: <20191011134744.2477-20-richard.henderson@linaro.org>

On Fri, 11 Oct 2019 at 14:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This "bit" is a particular value of the page's MemAttr.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e988398fce..17981d7c48 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9609,6 +9609,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      uint64_t descaddrmask;
>      bool aarch64 = arm_el_is_aa64(env, el);
>      bool guarded = false;
> +    uint8_t memattr;
>
>      /* TODO:
>       * This code does not handle the different format TCR for VTCR_EL2.
> @@ -9836,17 +9837,21 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          txattrs->target_tlb_bit0 = true;
>      }
>
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        memattr = convert_stage2_attrs(env, extract32(attrs, 0, 4));
> +    } else {
> +        /* Index into MAIR registers for cache attributes */
> +        uint64_t mair = env->cp15.mair_el[el];
> +        memattr = extract64(mair, extract32(attrs, 0, 3) * 8, 8);
> +    }
> +
> +    /* When in aarch64 mode, and MTE is enabled, remember Tagged in IOTLB.  */
> +    if (aarch64 && memattr == 0xf0 && cpu_isar_feature(aa64_mte, cpu)) {
> +        txattrs->target_tlb_bit1 = true;
> +    }

A comment somewhere saying that 0xf0 is the "Tagged Normal Memory"
attribute would probably be helpful.

> +
>      if (cacheattrs != NULL) {
> -        if (mmu_idx == ARMMMUIdx_S2NS) {
> -            cacheattrs->attrs = convert_stage2_attrs(env,
> -                                                     extract32(attrs, 0, 4));
> -        } else {
> -            /* Index into MAIR registers for cache attributes */
> -            uint8_t attrindx = extract32(attrs, 0, 3);
> -            uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> -            assert(attrindx <= 7);
> -            cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
> -        }
> +        cacheattrs->attrs = memattr;
>          cacheattrs->shareability = extract32(attrs, 6, 2);
>      }

Don't we also need to care about the stage 2 attributes
somewhere ? If the combo of stage 1 + stage 2 doesn't
say it's Normal Inner&OuterShareable WB NT RA WA then
it shouldn't be Tagged even if the stage 1 attributes ask
for that.

There's also the special case for "stage 1 translation
disabled" -- depends on HCR_EL2.DC and HCR_EL2.DCT.

thanks
-- PMM