Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anton Blanchard <anton@samba.org>]

Hi,

> > The distinction is important in QEMU.  ppc64 is still
> > TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat
> > integers as big endian.  There's just this extra concept that CPU
> > loads/stores are sometimes byte swapped.  That affects virtio but
> > not a lot else.
> 
> You've redefined endian here; please don't do that.  Endian is the
> order in memory which a CPU does loads and stores.  From any
> reasonable definition, PPC is bi-endian.
> 
> It's actually a weird thing for the qemu core to know at all: almost
> everything which cares is in target-specific code.  The exceptions are
> gdb stubs and virtio, both of which are "native endian" (and that
> weird code in exec.c: what is notdirty_mem_write?).
> 
> Your argument that we shouldn't fix stl_* might be justifiable (ie.
> just hack virtio and gdb as one-offs), but it's neither clear nor
> "least surprise".

Here is the hack I have to get gdbstub going with a little endian
PowerPC kernel. Basically:

LE guest -> BE QEMU -> BE gdb (pointing at the LE vmlinux)

In this setup, gdb expects registers to be sent in little endian mode.

It's a pretty big mistake for the gdb remote protocol to be using
native endian to transfer registers especially when there is no other
protocol negotation to work out what endian that is.

Anton
--

Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -317,6 +317,8 @@ static GDBState *gdbserver_state;
 
 bool gdb_has_xml;
 
+bool gdbstub_cross_endian;
+
 #ifdef CONFIG_USER_ONLY
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
Index: b/include/exec/gdbstub.h
===================================================================
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -42,8 +42,13 @@ static inline int cpu_index(CPUState *cp
 /* The GDB remote protocol transfers values in target byte order.
This means
  * we can use the raw memory access routines to access the value
buffer.
  * Conveniently, these also handle the case where the buffer is
mis-aligned.
+ *
+ * We do need to byte swap if the CPU isn't running in the QEMU
compiled
+ * target endian mode.
  */
 
+extern bool gdbstub_cross_endian;
+
 static inline int gdb_get_reg8(uint8_t *mem_buf, uint8_t val)
 {
     stb_p(mem_buf, val);
@@ -52,28 +57,49 @@ static inline int gdb_get_reg8(uint8_t *
 
 static inline int gdb_get_reg16(uint8_t *mem_buf, uint16_t val)
 {
-    stw_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stw_p(mem_buf, bswap16(val));
+    else
+        stw_p(mem_buf, val);
     return 2;
 }
 
 static inline int gdb_get_reg32(uint8_t *mem_buf, uint32_t val)
 {
-    stl_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stq_p(mem_buf, bswap32(val));
+    else
+        stl_p(mem_buf, val);
     return 4;
 }
 
 static inline int gdb_get_reg64(uint8_t *mem_buf, uint64_t val)
 {
-    stq_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stq_p(mem_buf, bswap64(val));
+    else
+        stq_p(mem_buf, val);
     return 8;
 }
 
 #if TARGET_LONG_BITS == 64
 #define gdb_get_regl(buf, val) gdb_get_reg64(buf, val)
-#define ldtul_p(addr) ldq_p(addr)
+static inline uint64_t ldtul_p(const void *ptr)
+{
+	uint64_t tmp = ldq_p(ptr);
+	if (gdbstub_cross_endian)
+		tmp = bswap64(tmp);
+	return tmp;
+}
 #else
 #define gdb_get_regl(buf, val) gdb_get_reg32(buf, val)
-#define ldtul_p(addr) ldl_p(addr)
+static inline uint32_t ldtul_p(const void *ptr)
+{
+	uint32_t tmp = ldl_p(ptr);
+	if (gdbstub_cross_endian)
+		tmp = bswap32(tmp);
+	return tmp;
+}
 #endif
 
 #endif