Re: [PATCH v2 19/21] target/arm: Move device detection earlier in get_phys_addr_lpae

[Helge Deller <deller@gmx.de>]

On 10/5/24 22:05, Richard Henderson wrote:
> Determine cache attributes, and thence Device vs Normal memory,

"thence" ?

Other than that I have no arm knowledge to review the patch below....

Helge

> earlier in the function.  We have an existing regime_is_stage2
> if block into which this can be slotted.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/ptw.c | 49 ++++++++++++++++++++++++------------------------
>   1 file changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 238b2c92a9..0a1a820362 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2029,8 +2029,20 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>               xn = extract64(attrs, 53, 2);
>               result->f.prot = get_S2prot(env, ap, xn, ptw->in_s1_is_el0);
>           }
> +
> +        result->cacheattrs.is_s2_format = true;
> +        result->cacheattrs.attrs = extract32(attrs, 2, 4);
> +        /*
> +         * Security state does not really affect HCR_EL2.FWB;
> +         * we only need to filter FWB for aa32 or other FEAT.
> +         */
> +        device = S2_attrs_are_device(arm_hcr_el2_eff(env),
> +                                     result->cacheattrs.attrs);
>       } else {
>           int nse, ns = extract32(attrs, 5, 1);
> +        uint8_t attrindx;
> +        uint64_t mair;
> +
>           switch (out_space) {
>           case ARMSS_Root:
>               /*
> @@ -2102,6 +2114,19 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>            */
>           result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap, xn, pxn,
>                                       result->f.attrs.space, out_space);
> +
> +        /* Index into MAIR registers for cache attributes */
> +        attrindx = extract32(attrs, 2, 3);
> +        mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> +        assert(attrindx <= 7);
> +        result->cacheattrs.is_s2_format = false;
> +        result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
> +
> +        /* When in aarch64 mode, and BTI is enabled, remember GP in the TLB. */
> +        if (aarch64 && cpu_isar_feature(aa64_bti, cpu)) {
> +            result->f.extra.arm.guarded = extract64(attrs, 50, 1); /* GP */
> +        }
> +        device = S1_attrs_are_device(result->cacheattrs.attrs);
>       }
>
>       if (!(result->f.prot & (1 << access_type))) {
> @@ -2131,30 +2156,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>       result->f.attrs.space = out_space;
>       result->f.attrs.secure = arm_space_is_secure(out_space);
>
> -    if (regime_is_stage2(mmu_idx)) {
> -        result->cacheattrs.is_s2_format = true;
> -        result->cacheattrs.attrs = extract32(attrs, 2, 4);
> -        /*
> -         * Security state does not really affect HCR_EL2.FWB;
> -         * we only need to filter FWB for aa32 or other FEAT.
> -         */
> -        device = S2_attrs_are_device(arm_hcr_el2_eff(env),
> -                                     result->cacheattrs.attrs);
> -    } else {
> -        /* Index into MAIR registers for cache attributes */
> -        uint8_t attrindx = extract32(attrs, 2, 3);
> -        uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> -        assert(attrindx <= 7);
> -        result->cacheattrs.is_s2_format = false;
> -        result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
> -
> -        /* When in aarch64 mode, and BTI is enabled, remember GP in the TLB. */
> -        if (aarch64 && cpu_isar_feature(aa64_bti, cpu)) {
> -            result->f.extra.arm.guarded = extract64(attrs, 50, 1); /* GP */
> -        }
> -        device = S1_attrs_are_device(result->cacheattrs.attrs);
> -    }
> -
>       /*
>        * Enable alignment checks on Device memory.
>        *