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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 26/06/2014 16:01, Peter Maydell ha scritto:
> 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).

Yes, this was on purpose.

>> +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...

I think LE8 is an R core with SCTLR.IE=1 SCTLR.EE=1 but CPSR.E=0, i.e. 
little endian data and big-endian code.  I put it in quotes because I 
get this with SCTLR.B=1 CPSR.E=1.  The difference is user visible due 
to CPSR.E.

I can modify the comment to:

    /* BE8 (SCTLR.B = 0, TARGET_WORDS_BIGENDIAN = 1) is mixed endian.
     * The invalid combination SCTLR.B=1/CPSR.E=1/TARGET_WORDS_BIGENDIAN=0
     * would also end up as a mixed-endian mode with BE code, LE data.
     */

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

Yes, I misread the code in disas.c.

Paolo