Re: [Qemu-devel] [PATCH v1 04/17] target-arm: implement SCTLR.EE

[Peter Maydell]

On 18 January 2016 at 07:12, Peter Crosthwaite
<address@hidden> wrote:
> From: Peter Crosthwaite <address@hidden>
>
> Implement SCTLR.EE bit which controls data endianess for exceptions
> and page table translations. SCTLR.EE is mirrored to the CPSR.E bit
> on exception entry.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>
>  target-arm/helper.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 59d5a41..afac1b2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5889,7 +5889,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      /* Clear IT bits.  */
>      env->condexec_bits = 0;
>      /* Switch to the new mode, and to the correct instruction set.  */
> -    env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
> +    env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_M)) | new_mode;

Why change this line?

> +    /* Set new mode endianess */

"endianness"

> +    env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_E)) |
> +        (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE ? CPSR_E : 0);

This is a bit confusing. I think just splitting it into
multiple statements would help:
   env->uncached_cpsr &= ~CPSR_E;
   if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
       env->uncached_cpsr |= CPSR_E;
   }

>      env->daif |= mask;
>      /* this is a lie, as the was no c1_sys on V4T/V5, but who cares
>       * and we should just guard the thumb mode on V4 */
> @@ -5958,6 +5961,12 @@ static inline bool 
> regime_translation_disabled(CPUARMState *env,
>      return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
>  }
>
> +static inline bool regime_translation_big_endian(CPUARMState *env,
> +                                                 ARMMMUIdx mmu_idx)
> +{
> +    return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
> +}
> +
>  /* Return the TCR controlling this translation regime */
>  static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
>  {
> @@ -6263,7 +6272,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
> ARMMMUIdx mmu_idx,
>   */
>  static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>                              ARMMMUIdx mmu_idx, uint32_t *fsr,
> -                            ARMMMUFaultInfo *fi)
> +                            ARMMMUFaultInfo *fi, bool be)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -6274,12 +6283,16 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr 
> addr, bool is_secure,
>      if (fi->s1ptw) {
>          return 0;
>      }
> -    return address_space_ldl(cs->as, addr, attrs, NULL);
> +    if (be) {
> +        return address_space_ldl_be(cs->as, addr, attrs, NULL);
> +    } else {
> +        return address_space_ldl_le(cs->as, addr, attrs, NULL);
> +    }
>  }

Why not just call regime_translation_big_endian() inside arm_ldl_ptw()
and arm_ldq_ptw(), rather than having every call site making the call
and passing in the result?

PS: this patch will conflict with the multi-ases series but only
fairly trivially.

thanks
-- PMM