Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32

[Peter Maydell]

On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
> tough they don't have the TTBCR register.
> See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
> AArch32 architecture profile Version:A.c section C1.2.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> ---
>  target/arm/debug_helper.c | 3 +++
>  target/arm/internals.h    | 4 ++++
>  target/arm/tlb_helper.c   | 3 +++
>  3 files changed, 10 insertions(+)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index c21739242c..73665f988b 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
>
>      if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
>          using_lpae = true;
> +    } else if (arm_feature(env, ARM_FEATURE_PMSA)
> +            && arm_feature(env, ARM_FEATURE_V8)) {

Indentation looks wrong here. Generally the second line of a
multiline if (...) condition starts in the column after the '(',
so it lines up with the first part of the condition.

> +        using_lpae = true;
>      } else {
>          if (arm_feature(env, ARM_FEATURE_LPAE) &&
>              (env->cp15.tcr_el[target_el] & TTBCR_EAE)) {

For instance this is an example in the existing code.

We are inconsistent about whether we put operators like '&&' at
the end of the first line or beginning of the second line, so
pick whichever you like best, I guess.

> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 307a596505..e3699421b0 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -253,6 +253,10 @@ unsigned int arm_pamax(ARMCPU *cpu);
>  static inline bool extended_addresses_enabled(CPUARMState *env)
>  {
>      uint64_t tcr = env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> +    if (arm_feature(env, ARM_FEATURE_PMSA)
> +     && arm_feature(env, ARM_FEATURE_V8)) {

Indentation is a bit odd here too.

> +        return true;
> +    }
>      return arm_el_is_aa64(env, 1) ||
>             (arm_feature(env, ARM_FEATURE_LPAE) && (tcr & TTBCR_EAE));
>  }
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index ad225b1cb2..a2047b0bc6 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -18,6 +18,9 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx 
> mmu_idx)
>      int el = regime_el(env, mmu_idx);
>      if (el == 2 || arm_el_is_aa64(env, el)) {
>          return true;
> +    } else if (arm_feature(env, ARM_FEATURE_PMSA)
> +            && arm_feature(env, ARM_FEATURE_V8)) {
> +        return true;
>      }

Use an "if ()..." not an "else if" here to match the style
of the other check in this function.

>      if (arm_feature(env, ARM_FEATURE_LPAE)
>          && (regime_tcr(env, mmu_idx) & TTBCR_EAE)) {

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM