don't hardcode EL1 in extended_addresses_enabled

Commit Message

Stefano Stabellini Oct. 18, 2017, 10:41 p.m. UTC

extended_addresses_enabled calls arm_el_is_aa64, hardcoding exception
level 1. Instead, retrieve the current el calling arm_current_el.

Signed-off-by: Stefano Stabellini <[e-mail]>

Comments

Peter Maydell Oct. 19, 2017, 3:39 p.m. UTC | #1

On 18 October 2017 at 23:41, Stefano Stabellini <[e-mail]> wrote:
> extended_addresses_enabled calls arm_el_is_aa64, hardcoding exception
> level 1. Instead, retrieve the current el calling arm_current_el.
>
> Signed-off-by: Stefano Stabellini <[e-mail]>
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1f6efef..63507d9 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -187,7 +187,7 @@ static inline unsigned int arm_pamax(ARMCPU *cpu)
>  static inline bool extended_addresses_enabled(CPUARMState *env)
>  {
>      TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> -    return arm_el_is_aa64(env, 1) ||
> +    return arm_el_is_aa64(env, arm_current_el(env)) ||
>             (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
>  }

Hmm. The current code was definitely written under a "aarch64
only supports EL0 and EL1" assumption, but I'm not sure
this change is entirely correct.

We use this function in 3 places:

 * identifying whether to flush TLBs on CONTEXTIDR writes
    -- using current EL seems like the right thing (but I'm not
       sure -- perhaps we should pass in the S/NS from which
       version of the 32-bit banked register is being updated
       rather than using arm_is_secure(env) ???)
 * choosing a PAR format for ATS operations
    -- using the current EL is closer to correct that what we
       have now. There are cases for a 32-bit CPU in Hyp mode that
       this code mishandles, but we don't support that yet.
 * choosing an FSR value when taking a breakpoint or watchpoint trap
    -- here what we want the function to mean is "what is the
       FSR format for the EL we're about to take this debug
       exception to", which isn't necessarily the answer for the
       current EL (consider 32 bit EL0 under a 64-bit EL1)

So this change will fix the PAR format for ATS operations
made at AArch64 EL2 when EL1 is AArch32, but it will break
the FSR format for breakpoints/watchpoints hit at AArch32 EL0
and taken to AArch64 EL1, I think.

We probably need to separate out these uses to not all try
to use the same function, and clarify what they're checking.

thanks
-- PMM

Patch

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 1f6efef..63507d9 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -187,7 +187,7 @@  static inline unsigned int arm_pamax(ARMCPU *cpu)
 static inline bool extended_addresses_enabled(CPUARMState *env)
 {
     TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
-    return arm_el_is_aa64(env, 1) ||
+    return arm_el_is_aa64(env, arm_current_el(env)) ||
            (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
 }