Re: [PATCH] aarch64 sim big-endian support

[Jim Wilson <jim.wilson@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On Mon, Jun 13, 2016 at 5:38 AM, Nick Clifton <nickc@redhat.com> wrote:
> I think that I agree with this comment, although I could not find
> the raw opcode reading functions to which he was referring, (unless
> he meant sim_core_read_buffer), so would you mind trying out this
> variation of your patch to see if it works instead ?

I finally got back to this.  I don't see any raw read function other
than sim_core_read_buffer either.  A raw read is not quite what I
want, as I need a little-endian to host translation, but I can call
endian_le2h_4 to do the swap after the raw read.  The interface is a
little awkward, as sim_core_read_buffer stores into a buffer instead
of returning a pointer, so I need to store the instruction, and then
read it back out again, swap it, and store it back again.

An alternative solution might be to make a copy of sim-n-core.h, call
it sim-n-core-le.h, and then change all of the T2H_M/H2T_M calls into
LE2H_M/H2LE_M calls, along with a few other minor changes to complete
the conversion.  We can then call sim_core_read_le_aligned_N instead
of sim_core_read_aligned_N for the instruction loads.  Note that
big-endian aarch64 is not the only target with this problem.
big-endian ARMv7-A works the same way, and if we had an IA-64
simulator, it would work the same way too.  So there are other
potential users of these functions.  This is maybe a little overkill
though for now, as we don't need the unaligned and misaligned read
functions for aarch64/armv7-a/ia-64 instruction loads, and we don't
need the write functions either.  We only need the aligned read
functions.

I tried testing this for all four combinations of big/little endian
host/target with a hello world program, and discovered that the
big-endian host support is broken.  The problem is with the
GRegisterValue untion.  You have
typedef union GRegisterValue
{
  int8_t   s8;
...
  int64_t s64;
} GRegister;
On a little-endian host, the s8 member will match the low-byte of the
s64 member, which is what we want.  However, on a big-endian host, the
s8 member will match the high-byte of the u64 member, and the
simulator fails.  I can fix this by using an anonymous struct for the
big-endian case
typedef union GRegisterValue
{
  struct { int64_t :56; int8_t s8; };
...l
  sint64_t s64;
} GRegister;
There are other ways to fix this, but this just seemed to me like the
quickest and smallest patch that would make it work.  There may also
be other issues here, as I only tested an integer hello world program.

Fixing the problem this way means that we require either an ISO C 2011
compiler, or a compiler that supports GCC extensions to ISO C 1990 or
1999.  Otherwise, you may get an error for the anonymous structs.  Or
alternatively, it requires using a C++ compiler, as C++ added
anonymous structs long before C did.  I'm not sure how much of a
problem this will be.  If this is a serious problem, it could be fixed
by giving names to the structs, adding the structs to the little
endian side also with the field order switched, and then fixing all
users to use the new names for the fields.  That will be a bigger
patch.

With both changes, a hello world program works on all four
combinations of big/little host/target.

if you aren't happy with the cpustate.h change, it would be nice to
get an approval for just the simulator.c change, as that is the part I
care more about.  We can worry about how to fix the big-endian host
cpustate.h support later.

Jim

[-- Attachment #2: gdb-sim-aarch64.patch --]
[-- Type: text/x-patch, Size: 1895 bytes --]

2016-06-29  Jim Wilson  <jim.wilson@linaro.org>

	sim/aarch64/
	* cpustate.h: Include config.h.
	(union GRegisterValue): Add WORDS_BIGENDIAN check.  For big endian code
	use anonymous structs to align members.
	* simulator.c (aarch64_step): Use sim_core_read_buffer and
	endian_le2h_4 to read instruction from pc.

diff --git a/sim/aarch64/cpustate.h b/sim/aarch64/cpustate.h
index 07446a2..2754f7c 100644
--- a/sim/aarch64/cpustate.h
+++ b/sim/aarch64/cpustate.h
@@ -22,6 +22,7 @@
 #ifndef _CPU_STATE_H
 #define _CPU_STATE_H
 
+#include "config.h"
 #include <sys/types.h>
 #include <stdint.h>
 #include <inttypes.h>
@@ -133,6 +134,7 @@ typedef enum VReg
    an explicit extend.  */
 typedef union GRegisterValue
 {
+#if !WORDS_BIGENDIAN
   int8_t   s8;
   int16_t  s16;
   int32_t  s32;
@@ -141,6 +143,16 @@ typedef union GRegisterValue
   uint16_t u16;
   uint32_t u32;
   uint64_t u64;
+#else
+  struct { int64_t :56; int8_t s8; };
+  struct { int64_t :48; int16_t s16; };
+  struct { int64_t :32; int32_t s32; };
+  int64_t s64;
+  struct { uint64_t :56; uint8_t u8; };
+  struct { uint64_t :48; uint16_t u16; };
+  struct { uint64_t :32; uint32_t u32; };
+  uint64_t u64;
+#endif
 } GRegister;
 
 /* Float registers provide for storage of a single, double or quad
diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 88cb03d..8eb582a 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -14083,7 +14083,11 @@ aarch64_step (sim_cpu *cpu)
     return FALSE;
 
   aarch64_set_next_PC (cpu, pc + 4);
-  aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc);
+
+  /* Code is always little-endian.  */
+  sim_core_read_buffer (CPU_STATE (cpu), cpu, read_map,
+			&aarch64_get_instr (cpu), pc, 4);
+  aarch64_get_instr (cpu) = endian_le2h_4 (aarch64_get_instr (cpu));
 
   TRACE_INSN (cpu, " pc = %" PRIx64 " instr = %08x", pc,
 	      aarch64_get_instr (cpu));