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

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

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

I would expect that anybody running BE32 binaries is
probably running them with -cpu any, since it's the
default. So breaking them is a bit harsh...

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

That's not what I would interpret "LE8" to mean... (I don't
actually think we define that term at all, but I would have
taken it to mean 'a BE8-capable system in little-endian mode',
ie CPSR.E==0 SCTLR.B==0).

SCTLR.B==1 CPSR.E==1 is UNPREDICTABLE and so we need not care
at all what it does.

If you want you can add a remark about our not supporting SCTLR.IE
since we don't support any ARMv6/ARMv7 R-class cores, but I don't
think that's really necessary.

> 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.
>      */

Yeah, that's fine.

thanks
-- PMM