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

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Tue, Jan 19, 2016 at 7:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> 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 <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  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?
>

Reverted

>> +    /* Set new mode endianess */
>
> "endianness"
>

Fixed

>> +    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;
>    }
>

Fixed.

>>      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?
>

Fixed.

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

Resolved.

Regards,
Peter

> thanks
> -- PMM