Re: [PATCH v6 04/20] target/arm: Split arm_cpu_data_is_big_endian

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> Set TBFLAG_ANY.BE_DATA in rebuild_hflags_common_32 and
> rebuild_hflags_a64 instead of rebuild_hflags_common, where we do
> not need to re-test is_a64() nor re-compute the various inputs.
>
> Signed-off-by: Richard Henderson <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> ---
>  target/arm/cpu.h    | 49 +++++++++++++++++++++++++++------------------
>  target/arm/helper.c | 16 +++++++++++----
>  2 files changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ad79a6153b..4d961474ce 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3108,33 +3108,44 @@ static inline uint64_t arm_sctlr(CPUARMState *env, 
> int el)
>      }
>  }
>
> +static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env,
> +                                                  bool sctlr_b)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    /*
> +     * In system mode, BE32 is modelled in line with the
> +     * architecture (as word-invariant big-endianness), where loads
> +     * and stores are done little endian but from addresses which
> +     * are adjusted by XORing with the appropriate constant. So the
> +     * endianness to use for the raw data access is not affected by
> +     * SCTLR.B.
> +     * In user mode, however, we model BE32 as byte-invariant
> +     * big-endianness (because user-only code cannot tell the
> +     * difference), and so we need to use a data access endianness
> +     * that depends on SCTLR.B.
> +     */
> +    if (sctlr_b) {
> +        return true;
> +    }
> +#endif
> +    /* In 32bit endianness is determined by looking at CPSR's E bit */
> +    return env->uncached_cpsr & CPSR_E;
> +}
> +
> +static inline bool arm_cpu_data_is_big_endian_a64(int el, uint64_t sctlr)
> +{
> +    return sctlr & (el ? SCTLR_EE : SCTLR_E0E);
> +}
>
>  /* Return true if the processor is in big-endian mode. */
>  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  {
> -    /* In 32bit endianness is determined by looking at CPSR's E bit */
>      if (!is_a64(env)) {
> -        return
> -#ifdef CONFIG_USER_ONLY
> -            /* In system mode, BE32 is modelled in line with the
> -             * architecture (as word-invariant big-endianness), where loads
> -             * and stores are done little endian but from addresses which
> -             * are adjusted by XORing with the appropriate constant. So the
> -             * endianness to use for the raw data access is not affected by
> -             * SCTLR.B.
> -             * In user mode, however, we model BE32 as byte-invariant
> -             * big-endianness (because user-only code cannot tell the
> -             * difference), and so we need to use a data access endianness
> -             * that depends on SCTLR.B.
> -             */
> -            arm_sctlr_b(env) ||
> -#endif
> -                ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
> +        return arm_cpu_data_is_big_endian_a32(env, arm_sctlr_b(env));
>      } else {
>          int cur_el = arm_current_el(env);
>          uint64_t sctlr = arm_sctlr(env, cur_el);
> -
> -        return (sctlr & (cur_el ? SCTLR_EE : SCTLR_E0E)) != 0;
> +        return arm_cpu_data_is_big_endian_a64(cur_el, sctlr);
>      }
>  }
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f05d042474..4c65476d93 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11061,9 +11061,6 @@ static uint32_t rebuild_hflags_common(CPUARMState 
> *env, int fp_el,
>      flags = FIELD_DP32(flags, TBFLAG_ANY, MMUIDX,
>                         arm_to_core_mmu_idx(mmu_idx));
>
> -    if (arm_cpu_data_is_big_endian(env)) {
> -        flags = FIELD_DP32(flags, TBFLAG_ANY, BE_DATA, 1);
> -    }
>      if (arm_singlestep_active(env)) {
>          flags = FIELD_DP32(flags, TBFLAG_ANY, SS_ACTIVE, 1);
>      }
> @@ -11073,7 +11070,14 @@ static uint32_t rebuild_hflags_common(CPUARMState 
> *env, int fp_el,
>  static uint32_t rebuild_hflags_common_32(CPUARMState *env, int fp_el,
>                                           ARMMMUIdx mmu_idx, uint32_t flags)
>  {
> -    flags = FIELD_DP32(flags, TBFLAG_A32, SCTLR_B, arm_sctlr_b(env));
> +    bool sctlr_b = arm_sctlr_b(env);
> +
> +    if (sctlr_b) {
> +        flags = FIELD_DP32(flags, TBFLAG_A32, SCTLR_B, 1);
> +    }
> +    if (arm_cpu_data_is_big_endian_a32(env, sctlr_b)) {
> +        flags = FIELD_DP32(flags, TBFLAG_ANY, BE_DATA, 1);
> +    }
>      flags = FIELD_DP32(flags, TBFLAG_A32, NS, !access_secure_reg(env));
>
>      return rebuild_hflags_common(env, fp_el, mmu_idx, flags);
> @@ -11122,6 +11126,10 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, 
> int el, int fp_el,
>
>      sctlr = arm_sctlr(env, el);
>
> +    if (arm_cpu_data_is_big_endian_a64(el, sctlr)) {
> +        flags = FIELD_DP32(flags, TBFLAG_ANY, BE_DATA, 1);
> +    }
> +
>      if (cpu_isar_feature(aa64_pauth, env_archcpu(env))) {
>          /*
>           * In order to save space in flags, we record only whether


--
Alex Bennée