Re: [Qemu-arm] [PATCH v1 12/17] target-arm: implement SCTLR.B, drop bswap_code

[Peter Maydell]

On 18 January 2016 at 07:12, Peter Crosthwaite
<address@hidden> wrote:
> From: Paolo Bonzini <address@hidden>
>
> bswap_code is a CPU property of sorts ("is the iside endianness the
> opposite way round to TARGET_WORDS_BIGENDIAN?") but it is not the
> actual CPU state involved here which is SCTLR.B (set for BE32
> binaries, clear for BE8).
>
> Replace bswap_code with SCTLR.B, and pass that to arm_ld*_code.
> The next patches will make data fetches honor both SCTLR.B and
> CPSR.E appropriately.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> [PC changes:
>  * rebased on master (Jan 2016)
>  * Dropped comment about CPSR.E being unimplemented
>  * s/TARGET_USER_ONLY/CONFIG_USER_ONLY
>  * Use bswap_code() for disas_set_info() instead of raw sctlr_b
> ]
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>
>  linux-user/main.c          | 13 ++++++++-----
>  target-arm/arm_ldst.h      |  8 ++++----
>  target-arm/cpu.c           |  2 +-
>  target-arm/cpu.h           | 47 
> ++++++++++++++++++++++++++++++++++++++--------
>  target-arm/helper.c        |  8 ++++----
>  target-arm/translate-a64.c |  6 +++---
>  target-arm/translate.c     | 12 ++++++------
>  target-arm/translate.h     |  2 +-
>  8 files changed, 66 insertions(+), 32 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 2157774..d481458 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -441,7 +441,7 @@ void cpu_loop(CPUX86State *env)
>
>  #define get_user_code_u32(x, gaddr, env)                \
>      ({ abi_long __r = get_user_u32((x), (gaddr));       \
> -        if (!__r && (env)->bswap_code) {                \
> +        if (!__r && bswap_code(arm_sctlr_b(env))) {     \
>              (x) = bswap32(x);                           \
>          }                                               \
>          __r;                                            \
> @@ -449,7 +449,7 @@ void cpu_loop(CPUX86State *env)
>
>  #define get_user_code_u16(x, gaddr, env)                \
>      ({ abi_long __r = get_user_u16((x), (gaddr));       \
> -        if (!__r && (env)->bswap_code) {                \
> +        if (!__r && bswap_code(arm_sctlr_b(env))) {     \
>              (x) = bswap16(x);                           \
>          }                                               \
>          __r;                                            \
> @@ -4489,14 +4489,17 @@ int main(int argc, char **argv, char **envp)
>              env->regs[i] = regs->uregs[i];
>          }
>  #ifdef TARGET_WORDS_BIGENDIAN
> -        env->uncached_cpsr |= CPSR_E;
>          env->cp15.sctlr_el[1] |= SCTLR_E0E;
> -#endif
>          /* Enable BE8.  */
>          if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
>              && (info->elf_flags & EF_ARM_BE8)) {
> -            env->bswap_code = 1;
> +            env->uncached_cpsr |= CPSR_E;

...ah, this is fixing the problem I noted in the earlier patch.

> +        } else {
> +            env->cp15.sctlr_el[1] |= SCTLR_B;
> +            /* We model BE32 as regular BE, so set CPSR_E */
> +            env->uncached_cpsr |= CPSR_E;

...but we still set CPSR_E for BE32, which is wrong.

>          }
> +#endif
>      }
>  #elif defined(TARGET_UNICORE32)
>      {

The rest of this patch is all OK.

thanks
-- PMM