Re: [Qemu-devel] [PATCH v3 03/11] target-arm: implement SCTLR.B, drop bswap_code

[Peter Maydell <peter.maydell@linaro.org>]

On 21 June 2014 13:58, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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 <pbonzini@redhat.com>

> @@ -4191,11 +4191,19 @@ int main(int argc, char **argv, char **envp)
>          for(i = 0; i < 16; i++) {
>              env->regs[i] = regs->uregs[i];
>          }
> +#ifdef TARGET_WORDS_BIGENDIAN
>          /* Enable BE8.  */
>          if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
>              && (info->elf_flags & EF_ARM_BE8)) {
> -            env->bswap_code = 1;
> +            /* nothing for now, CPSR.E not emulated yet */
> +        } else {
> +            if (arm_feature(env, ARM_FEATURE_V7)) {
> +                fprintf(stderr, "BE32 binaries only supported until ARMv6\n");
> +                exit(1);
> +            }
> +            env->cp15.c1_sys |= SCTLR_B;

This will break running BE32 binaries with "-cpu any"
(which sets all the features we know about, including
ARM_FEATURE_V7).

> +static inline bool bswap_code(bool sctlr_b)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    /* Mixed-endian modes are BE8 (SCTLR.B = 0, TARGET_WORDS_BIGENDIAN = 1)
> +     * and "LE8" (SCTLR.B = 1, TARGET_WORDS_BIGENDIAN = 0).

Huh? LE8 is SCTLR.B == 0...

> +     */
> +    return
> +#ifdef TARGET_WORDS_BIGENDIAN
> +        1 ^
> +#endif
> +        sctlr_b;
> +#else
> +    /* We do not implement BE32 mode for system-mode emulation, but
> +     * anyway it would always do little-endian accesses with
> +     * TARGET_WORDS_BIGENDIAN = 0.
> +     */
> +    return 0;
> +#endif
> +}

> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -10786,7 +10786,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
>  {
>      uint32_t insn;
>
> -    insn = arm_ldl_code(env, s->pc, s->bswap_code);
> +    insn = arm_ldl_code(env, s->pc, s->sctlr_b);

This is now slightly odd to read because by definition any
CPU with the A64 instruction set won't have BE32 and A64
insns are always loaded little-endian... Still, it's a
mechanical code transform so I guess it's ok.
We can clean that up later.

> @@ -10899,7 +10899,7 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>
>      dc->aarch64 = 0;
>      dc->thumb = ARM_TBFLAG_THUMB(tb->flags);
> -    dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags);
> +    dc->sctlr_b = ARM_TBFLAG_SCTLR_B(tb->flags);
>      dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1;
>      dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4;
>  #if !defined(CONFIG_USER_ONLY)
> @@ -11142,7 +11142,7 @@ done_generating:
>          qemu_log("----------------\n");
>          qemu_log("IN: %s\n", lookup_symbol(pc_start));
>          log_target_disas(env, pc_start, dc->pc - pc_start,
> -                         dc->thumb | (dc->bswap_code << 1));
> +                         dc->thumb | (dc->sctlr_b << 1));

Don't we need a call to bswap_code() here, since we're
telling the disassembler which endianness to assume?

>          qemu_log("\n");
>      }
>  #endif
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index 31a0104..19f794c 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -16,7 +16,7 @@ typedef struct DisasContext {
>      struct TranslationBlock *tb;
>      int singlestep_enabled;
>      int thumb;
> -    int bswap_code;
> +    int sctlr_b;
>  #if !defined(CONFIG_USER_ONLY)
>      int user;
>  #endif

thanks
-- PMM