[Qemu-devel] big endian arm.

[Qemu-devel] big endian arm.

[KONRAD Frederic]

Hi Peters,

I got some strange results since this commit:

commit 9776f636455b6f0d9c14dce112242ed653f954b4
Author: Peter Crosthwaite <crosthwaitepeter@gmail.com>
Date:   Fri Mar 4 11:30:21 2016 +0000

     arm: boot: Support big-endian elfs

     Support ARM big-endian ELF files in system-mode emulation. 
When loading
     an elf, determine the endianness mode expected by the elf, 
and set the
     relevant CPU state accordingly.

     With this, big-endian modes are now fully supported via 
system-mode LE,
     so there is no need to restrict the elf loading to the TARGET
     endianness so the ifdeffery on TARGET_WORDS_BIGENDIAN goes away.

     Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
     Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
     [PMM: fix typo in comments]
     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

It seems that when I try to load a big endian image on a
Cortex-R5 it gets confused:
  * the instructions are fine it executes some code.
  * GDB address / insns are wrong endianness.
  * in monitor x command is good endianness.
  * the data order seems to be wrong endianness:
    Here is my Hello World: lleHlroW

We used to add a armeb-softmmu/qemu-system-armeb target for the
big endian cpu but it doesn't work anymore as it's double
reversed by the elf endianness (data_swab <= 2).

And I'm surprised we don't set the CPSR_E and SCTLR_EE bits
accordingly?

Thanks,
Fred

Re: [Qemu-devel] big endian arm.

[Peter Maydell]

On 11 August 2017 at 10:59, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> Hi Peters,
>
> I got some strange results since this commit:
>
> commit 9776f636455b6f0d9c14dce112242ed653f954b4
> Author: Peter Crosthwaite <crosthwaitepeter@gmail.com>
> Date:   Fri Mar 4 11:30:21 2016 +0000
>
>     arm: boot: Support big-endian elfs
>
>     Support ARM big-endian ELF files in system-mode emulation. When loading
>     an elf, determine the endianness mode expected by the elf, and set the
>     relevant CPU state accordingly.
>
>     With this, big-endian modes are now fully supported via system-mode LE,
>     so there is no need to restrict the elf loading to the TARGET
>     endianness so the ifdeffery on TARGET_WORDS_BIGENDIAN goes away.
>
>     Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>     Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>     [PMM: fix typo in comments]
>     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> It seems that when I try to load a big endian image on a
> Cortex-R5 it gets confused:
>  * the instructions are fine it executes some code.
>  * GDB address / insns are wrong endianness.
>  * in monitor x command is good endianness.
>  * the data order seems to be wrong endianness:
>    Here is my Hello World: lleHlroW

R profile bigendian is weird (ie not like A profile) because
it has a special fudge: SCTLR.IE is an Instruction Endianness
bit which lets you specify big-endian instruction order (IMPDEF
how it's set, but for the Cortex-R5 it's an external config
signal so the SoC can hardwire it as it likes). For A profile
this bit is always 0 and instructions are LE always. We
don't implement letting the board model set SCTLR.IE (or
the code needed to honour it being non-zero).

If your code requires SCTLR.IE to be 1 then you'll need
to implement that handling (if the instructions are being
correctly executed then that suggests it doesn't, but
you should check).

> We used to add a armeb-softmmu/qemu-system-armeb target for the
> big endian cpu but it doesn't work anymore as it's double
> reversed by the elf endianness (data_swab <= 2).

This was never the right way to do system BE :-)  We should
fix up whatever the R profile issues you see are...

> And I'm surprised we don't set the CPSR_E and SCTLR_EE bits
> accordingly?

Setting SCTLR_EE is the job of the board model: a BE board
model sets the 'cfgend' property on the CPU, which configures
the reset SCTLR.EE (or SCTLR.B for v5/v6 cores). (You may
also be able to set -cpu=something,cfgend=true on the command
line but that's kind of a hack.) SCTLR.EE is then supposed
to set the CPSR.E on exception entry including reset.

There is I think a bug here though: we set CPSR.E on
exception entry in arm_cpu_do_interrupt_aarch32():

    /* Set new mode endianness */
    env->uncached_cpsr &= ~CPSR_E;
    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
        env->uncached_cpsr |= CPSR_E;
    }

but we forgot to do anything in arm_cpu_reset() to get
the CPSR.E bit right on initial reset.

That said, if you specify a BE elf file then we do
set the SCTLR.EE and CPSR.E bits on reset in do_cpu_reset()
(a change added in the commit you quote), which is probably
why we haven't noticed the arm_cpu_reset() bug.

Dunno about the gdbstub stuff.

thanks
-- PMM

Re: [Qemu-devel] big endian arm.

[KONRAD Frederic]

On 08/11/2017 12:18 PM, Peter Maydell wrote:
> On 11 August 2017 at 10:59, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> Hi Peters,
>>
>> I got some strange results since this commit:
>>
>> commit 9776f636455b6f0d9c14dce112242ed653f954b4
>> Author: Peter Crosthwaite <crosthwaitepeter@gmail.com>
>> Date:   Fri Mar 4 11:30:21 2016 +0000
>>
>>      arm: boot: Support big-endian elfs
>>
>>      Support ARM big-endian ELF files in system-mode emulation. When loading
>>      an elf, determine the endianness mode expected by the elf, and set the
>>      relevant CPU state accordingly.
>>
>>      With this, big-endian modes are now fully supported via system-mode LE,
>>      so there is no need to restrict the elf loading to the TARGET
>>      endianness so the ifdeffery on TARGET_WORDS_BIGENDIAN goes away.
>>
>>      Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>      Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>      [PMM: fix typo in comments]
>>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> It seems that when I try to load a big endian image on a
>> Cortex-R5 it gets confused:
>>   * the instructions are fine it executes some code.
>>   * GDB address / insns are wrong endianness.
>>   * in monitor x command is good endianness.
>>   * the data order seems to be wrong endianness:
>>     Here is my Hello World: lleHlroW
> 
> R profile bigendian is weird (ie not like A profile) because
> it has a special fudge: SCTLR.IE is an Instruction Endianness
> bit which lets you specify big-endian instruction order (IMPDEF
> how it's set, but for the Cortex-R5 it's an external config
> signal so the SoC can hardwire it as it likes). For A profile
> this bit is always 0 and instructions are LE always. We
> don't implement letting the board model set SCTLR.IE (or
> the code needed to honour it being non-zero).
> 
> If your code requires SCTLR.IE to be 1 then you'll need
> to implement that handling (if the instructions are being
> correctly executed then that suggests it doesn't, but
> you should check).

Yes it seems the instruction are OK as they are swapped during
the load.
> 
>> We used to add a armeb-softmmu/qemu-system-armeb target for the
>> big endian cpu but it doesn't work anymore as it's double
>> reversed by the elf endianness (data_swab <= 2).
> 
> This was never the right way to do system BE :-)  We should
> fix up whatever the R profile issues you see are...
> 
>> And I'm surprised we don't set the CPSR_E and SCTLR_EE bits
>> accordingly?
> 
> Setting SCTLR_EE is the job of the board model: a BE board
> model sets the 'cfgend' property on the CPU, which configures
> the reset SCTLR.EE (or SCTLR.B for v5/v6 cores). (You may
> also be able to set -cpu=something,cfgend=true on the command
> line but that's kind of a hack.) SCTLR.EE is then supposed
> to set the CPSR.E on exception entry including reset.
> 
> There is I think a bug here though: we set CPSR.E on
> exception entry in arm_cpu_do_interrupt_aarch32():
> 
>      /* Set new mode endianness */
>      env->uncached_cpsr &= ~CPSR_E;
>      if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
>          env->uncached_cpsr |= CPSR_E;
>      }
> 
> but we forgot to do anything in arm_cpu_reset() to get
> the CPSR.E bit right on initial reset.
> 
> That said, if you specify a BE elf file then we do
> set the SCTLR.EE and CPSR.E bits on reset in do_cpu_reset()
> (a change added in the commit you quote), which is probably
> why we haven't noticed the arm_cpu_reset() bug.

Yes that was what I saw. But seems the bits are not reset anymore
in do_cpu_reset() we probably lost that change.

Fred

> 
> Dunno about the gdbstub stuff.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] big endian arm.

[Peter Maydell]

On 11 August 2017 at 12:03, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> On 08/11/2017 12:18 PM, Peter Maydell wrote:
>> That said, if you specify a BE elf file then we do
>> set the SCTLR.EE and CPSR.E bits on reset in do_cpu_reset()
>> (a change added in the commit you quote), which is probably
>> why we haven't noticed the arm_cpu_reset() bug.
>
>
> Yes that was what I saw. But seems the bits are not reset anymore
> in do_cpu_reset() we probably lost that change.

hw/arm/boot.c do_cpu_reset() still has that code as of
current master...

thanks
-- PMM

Re: [Qemu-devel] big endian arm.

[KONRAD Frederic]

On 08/11/2017 01:05 PM, Peter Maydell wrote:
> On 11 August 2017 at 12:03, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> On 08/11/2017 12:18 PM, Peter Maydell wrote:
>>> That said, if you specify a BE elf file then we do
>>> set the SCTLR.EE and CPSR.E bits on reset in do_cpu_reset()
>>> (a change added in the commit you quote), which is probably
>>> why we haven't noticed the arm_cpu_reset() bug.
>>
>>
>> Yes that was what I saw. But seems the bits are not reset anymore
>> in do_cpu_reset() we probably lost that change.
> 
> hw/arm/boot.c do_cpu_reset() still has that code as of
> current master...
> 

oops sorry you're right was on the wrong branch..

Thanks!
Fred

> thanks
> -- PMM
> 

Re: [Qemu-devel] big endian arm.

[Philippe Mathieu-Daudé]

On 08/11/2017 07:18 AM, Peter Maydell wrote:
> On 11 August 2017 at 10:59, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
[...]>> It seems that when I try to load a big endian image on a
>> Cortex-R5 it gets confused:
>>   * the instructions are fine it executes some code.
>>   * GDB address / insns are wrong endianness.
>>   * in monitor x command is good endianness.
>>   * the data order seems to be wrong endianness:
>>     Here is my Hello World: lleHlroW
> 
[...]
> 
> Dunno about the gdbstub stuff.

Frederic you might try/improve those patches:

[PATCH v3 4/7] ARM big-endian semihosting support.
[PATCH v3 5/7] ARM big-endian system-mode gdbstub support.

see:
http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg04386.html
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg00843.html

Regards,

Phil.

Re: [Qemu-devel] big endian arm.

[Philippe Mathieu-Daudé]

Hi Frederic,

I'm slowly working on Hercules MCU peripherals and use a R4F core.

> It seems that when I try to load a big endian image on a
> Cortex-R5 it gets confused:
>   * the instructions are fine it executes some code.

So far no problem here

>   * GDB address / insns are wrong endianness.

Maybe I didn't hit this problem since I use the same setup than with 
openocd/real cpu with the following .gdbinit:

     set architecture armv5te
     set arm fpu vfp
     set endian big
     set remote hardware-breakpoint-limit 6
     set breakpoint auto-hw on
     set displaced-stepping on

>   * in monitor x command is good endianness.
>   * the data order seems to be wrong endianness:
>     Here is my Hello World: lleHlroW

Indeed I hit a similar problem and found that 
memory::access_with_adjusted_size() has some issues, I could never 
finish the unit-tests but this is the first entry on my 2.11 TODO.

I'll send as RFC.

Regards,

Phil.

Re: [Qemu-devel] big endian arm.

[KONRAD Frederic]

On 08/11/2017 01:29 PM, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> I'm slowly working on Hercules MCU peripherals and use a R4F core.
> 
>> It seems that when I try to load a big endian image on a
>> Cortex-R5 it gets confused:
>>   * the instructions are fine it executes some code.
> 
> So far no problem here
> 
>>   * GDB address / insns are wrong endianness.
> 
> Maybe I didn't hit this problem since I use the same setup than 
> with openocd/real cpu with the following .gdbinit:
> 
>      set architecture armv5te
>      set arm fpu vfp
>      set endian big
>      set remote hardware-breakpoint-limit 6
>      set breakpoint auto-hw on
>      set displaced-stepping on
> 
>>   * in monitor x command is good endianness.
>>   * the data order seems to be wrong endianness:
>>     Here is my Hello World: lleHlroW
> 
> Indeed I hit a similar problem and found that 
> memory::access_with_adjusted_size() has some issues, I could 
> never finish the unit-tests but this is the first entry on my 
> 2.11 TODO.
> 
> I'll send as RFC.

Hi Philippe,

It would be nice!

Thanks!
Fred

> 
> Regards,
> 
> Phil.