Re: [PATCH v7 40/42] 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>,
	Stephen Long <steplong@quicinc.com>
Subject: Re: [PATCH v7 40/42] target/arm: Cache the Tagged bit for a page in MemTxAttrs
Date: Fri, 19 Jun 2020 15:29:00 +0100	[thread overview]
Message-ID: <CAFEAcA8zBPh8T3J_=LAg2CNWS4nZC+iGsR053yXtd78gSoma1A@mail.gmail.com> (raw)
In-Reply-To: <20200603011317.473934-41-richard.henderson@linaro.org>

On Wed, 3 Jun 2020 at 02:14, 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>
> ---
> v6: Test HCR_EL2.{DC,DCT}; test Stage2 attributes.
> ---
>  target/arm/helper.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 50717afa4a..74925010ea 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10875,6 +10875,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 support shareability levels. */
>      if (aarch64) {
> @@ -11099,17 +11100,32 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          txattrs->target_tlb_bit0 = true;
>      }
>
> -    if (cacheattrs != NULL) {
> +    if (mmu_idx == ARMMMUIdx_Stage2) {
> +        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 MTE is enabled, remember Tagged Memory in IOTLB. */
> +    if (aarch64 && cpu_isar_feature(aa64_mte, cpu)) {
>          if (mmu_idx == ARMMMUIdx_Stage2) {
> -            cacheattrs->attrs = convert_stage2_attrs(env,
> -                                                     extract32(attrs, 0, 4));
> +            /*
> +             * Require Normal, I+O Shareable, WB, NT, RA, WA (0xff).
> +             * If not, squash stage1 tagged normal setting.
> +             */
> +            if (memattr != 0xff) {
> +                txattrs->target_tlb_bit1 = false;
> +            }

Can we implement the effect of stage1+stage2 attrs on whether
the memory location is tagged in the same way it's described in
the Arm ARM, ie by combining the stage1+stage2 attrs and then
looking at the combined result to decide ? In particular, this
code works for the classic attribute-combining logic (where stage2
gets to override only in the more-like-device direction), but
when we get round to implementing ARMv8.4-S2FWB then HCR_EL2.FWB
will allow the stage2 page tables to entirely override stage1
so that we might have memory that is tagged even if stage 1's
attributes said it wasn't.

Currently we do this attribute-combining only for cacheattrs
(the combine_cacheattrs() call) but we could make that deal with
more attributes if we need them for this purpose.

thanks
-- PMM