[Qemu-devel] [PATCH 0/7] target-ppc: gdbstub: endiannes fixes and VSX support

[Qemu-devel] [PATCH 0/7] target-ppc: gdbstub: endiannes fixes and VSX support

[Greg Kurz]

Hi,

This series was posted before last Xmas. Since I am not sure if Alex is
available to handle this, and this affects ppc64le (hence most people
running sPAPR guests), I repost with David Gibson added to the Cc: list.

This series is a sequel to Anton's tentative at bringing VSX support in
our gdbstub:

http://patchwork.ozlabs.org/patch/453758/

Indeed, FP, SPE and Altivec registers need to be copied to memory with
the appropriate ordering, like we already do for core registers. This
series reuses the maybe_bswap_register() helper to do the job, since
it already handles the user mode case where the target endianness is
known at build time and don't need byteswap. This is covered by
patches 2 to 6.

For these to work, two bugs that break FP/VR/VSX synchronization between
QEMU and KVM had to be addressed.

First one is a bug in KVM, that completely breaks the KVM_GET_ONE_REG
and KVM_SET_ONE_REG ioctls for Altivec registers. The fix is already
in Paul Mackerras' tree at:

http://git.kernel.org/cgit/linux/kernel/git/paulus/powerpc.git/commit/?h=kvm-ppc-next&id=b4d7f161feb3015d6306e1d35b565c888ff70c9d

Second one is a bug in QEMU that breaks synchronisation with KVM for FP,
Altivec and VSX registers on little-endian hosts. I pushed the fix to
patch 1 since it is needed for the gdbstub fixes to actually work, but it
could even be handled separately.

And finally, patch 7 is Anton's + the byteswapping for little-endian
guests.

Cheers.

---

Anton Blanchard (1):
      target-ppc: gdbstub: Add VSX support

Greg Kurz (6):
      target-ppc: kvm: fix floating point registers sync on little-endian hosts
      target-ppc: rename and export maybe_bswap_register()
      target-ppc: gdbstub: fix float registers for little-endian guests
      target-ppc: gdbstub: introduce avr_need_swap()
      target-ppc: gdbstub: fix altivec registers for little-endian guests
      target-ppc: gdbstub: fix spe registers for little-endian guests


 configure                   |    6 ++-
 gdb-xml/power-vsx.xml       |   44 +++++++++++++++++++++++
 target-ppc/cpu.h            |    1 +
 target-ppc/gdbstub.c        |   10 +++--
 target-ppc/kvm.c            |   12 ++++++
 target-ppc/translate_init.c |   84 +++++++++++++++++++++++++++++++++++--------
 6 files changed, 134 insertions(+), 23 deletions(-)
 create mode 100644 gdb-xml/power-vsx.xml

[Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[Greg Kurz]

On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
of the 32 first VSX registers. So if you have:

VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00

then

FPR31 = (uint64) 0x0102030405060708

The kernel stores the VSX registers in the fp_state struct following the
host endian element ordering.

On big-endian:

fp_state.fpr[31][0] = 0x0102030405060708
fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00

On little-endian:

fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
fp_state.fpr[31][1] = 0x0102030405060708

The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
QEMU considers it as big-endian and always copies element [0] to the
fpr[] array and element [1] to the vsr[] array. This does not work with
little-endian hosts, and you will get:

(qemu) p $f31
0x90a0b0c0d0e0f00

instead of:

(qemu) p $f31
0x102030405060708

This patch fixes the element ordering for little-endian hosts.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/kvm.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9940a9046220..45249990bda1 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -650,8 +650,13 @@ static int kvm_put_fp(CPUState *cs)
         for (i = 0; i < 32; i++) {
             uint64_t vsr[2];
 
+#ifdef HOST_WORDS_BIGENDIAN
             vsr[0] = float64_val(env->fpr[i]);
             vsr[1] = env->vsr[i];
+#else
+            vsr[0] = env->vsr[i];
+            vsr[1] = float64_val(env->fpr[i]);
+#endif
             reg.addr = (uintptr_t) &vsr;
             reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i);
 
@@ -721,10 +726,17 @@ static int kvm_get_fp(CPUState *cs)
                         vsx ? "VSR" : "FPR", i, strerror(errno));
                 return ret;
             } else {
+#ifdef HOST_WORDS_BIGENDIAN
                 env->fpr[i] = vsr[0];
                 if (vsx) {
                     env->vsr[i] = vsr[1];
                 }
+#else
+                env->fpr[i] = vsr[1];
+                if (vsx) {
+                    env->vsr[i] = vsr[0];
+                }
+#endif
             }
         }
     }

[Qemu-devel] [PATCH 2/7] target-ppc: rename and export maybe_bswap_register()

[Greg Kurz]

This helper will be used to support FP, Altivec and VSX registers when
the guest is little-endian.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/cpu.h     |    1 +
 target-ppc/gdbstub.c |   10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 9706000f8bf1..1e2516e071b7 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -2355,4 +2355,5 @@ int ppc_get_vcpu_dt_id(PowerPCCPU *cpu);
  */
 PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
 
+void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
 #endif /* !defined (__CPU_PPC_H__) */
diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
index 14675f45653d..b20bb0c80ef3 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -88,7 +88,7 @@ static int ppc_gdb_register_len(int n)
    the proper ordering for the binary, and cannot be changed.
    For system mode, TARGET_WORDS_BIGENDIAN is always set, and we must check
    the current mode of the chip to see if we're running in little-endian.  */
-static void maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
+void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
 {
 #ifndef CONFIG_USER_ONLY
     if (!msr_le) {
@@ -158,7 +158,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         }
     }
-    maybe_bswap_register(env, mem_buf, r);
+    ppc_maybe_bswap_register(env, mem_buf, r);
     return r;
 }
 
@@ -214,7 +214,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         }
     }
-    maybe_bswap_register(env, mem_buf, r);
+    ppc_maybe_bswap_register(env, mem_buf, r);
     return r;
 }
 
@@ -227,7 +227,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     if (!r) {
         return r;
     }
-    maybe_bswap_register(env, mem_buf, r);
+    ppc_maybe_bswap_register(env, mem_buf, r);
     if (n < 32) {
         /* gprs */
         env->gpr[n] = ldtul_p(mem_buf);
@@ -277,7 +277,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
     if (!r) {
         return r;
     }
-    maybe_bswap_register(env, mem_buf, r);
+    ppc_maybe_bswap_register(env, mem_buf, r);
     if (n < 32) {
         /* gprs */
         env->gpr[n] = ldq_p(mem_buf);

[Qemu-devel] [PATCH 3/7] target-ppc: gdbstub: fix float registers for little-endian guests

[Greg Kurz]

Let's reuse the ppc_maybe_bswap_register() helper, like we already do
with the general registers.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e88dc7fc7aa3..d31d7f6e9bd8 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8755,10 +8755,12 @@ static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
         stfq_p(mem_buf, env->fpr[n]);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
         return 8;
     }
     if (n == 32) {
         stl_p(mem_buf, env->fpscr);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     return 0;
@@ -8767,10 +8769,12 @@ static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
         env->fpr[n] = ldfq_p(mem_buf);
         return 8;
     }
     if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         helper_store_fpscr(env, ldl_p(mem_buf), 0xffffffff);
         return 4;
     }

[Qemu-devel] [PATCH 4/7] target-ppc: gdbstub: introduce avr_need_swap()

[Greg Kurz]

This helper will be used to support Altivec registers in little-endian guests.
This patch does not change functionnality.

Note: I had to put the helper some lines away from the gdb_*_avr_reg()
routines to get a more readable patch.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |   37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d31d7f6e9bd8..18e9e561561f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8751,6 +8751,15 @@ static void dump_ppc_insns (CPUPPCState *env)
 }
 #endif
 
+static bool avr_need_swap(CPUPPCState *env)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+    return false;
+#else
+    return true;
+#endif
+}
+
 static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
@@ -8784,13 +8793,13 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
-#ifdef HOST_WORDS_BIGENDIAN
-        stq_p(mem_buf, env->avr[n].u64[0]);
-        stq_p(mem_buf+8, env->avr[n].u64[1]);
-#else
-        stq_p(mem_buf, env->avr[n].u64[1]);
-        stq_p(mem_buf+8, env->avr[n].u64[0]);
-#endif
+        if (!avr_need_swap(env)) {
+            stq_p(mem_buf, env->avr[n].u64[0]);
+            stq_p(mem_buf+8, env->avr[n].u64[1]);
+        } else {
+            stq_p(mem_buf, env->avr[n].u64[1]);
+            stq_p(mem_buf+8, env->avr[n].u64[0]);
+        }
         return 16;
     }
     if (n == 32) {
@@ -8807,13 +8816,13 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
-#ifdef HOST_WORDS_BIGENDIAN
-        env->avr[n].u64[0] = ldq_p(mem_buf);
-        env->avr[n].u64[1] = ldq_p(mem_buf+8);
-#else
-        env->avr[n].u64[1] = ldq_p(mem_buf);
-        env->avr[n].u64[0] = ldq_p(mem_buf+8);
-#endif
+        if (!avr_need_swap(env)) {
+            env->avr[n].u64[0] = ldq_p(mem_buf);
+            env->avr[n].u64[1] = ldq_p(mem_buf+8);
+        } else {
+            env->avr[n].u64[1] = ldq_p(mem_buf);
+            env->avr[n].u64[0] = ldq_p(mem_buf+8);
+        }
         return 16;
     }
     if (n == 32) {

[Qemu-devel] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[Greg Kurz]

Altivec registers are 128-bit wide. They are stored in memory as two
64-bit values that must be byteswapped when the guest is little-endian.
Let's reuse the ppc_maybe_bswap_register() helper for this.

We also need to fix the ordering of the 64-bit elements according to
the target endianness, for both system and user mode.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 18e9e561561f..80d53e4dcf5a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
 static bool avr_need_swap(CPUPPCState *env)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-    return false;
+    return msr_le;
 #else
-    return true;
+    return !msr_le;
 #endif
 }
 
@@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
             stq_p(mem_buf, env->avr[n].u64[1]);
             stq_p(mem_buf+8, env->avr[n].u64[0]);
         }
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
         return 16;
     }
     if (n == 32) {
         stl_p(mem_buf, env->vscr);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     if (n == 33) {
         stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     return 0;
@@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
         if (!avr_need_swap(env)) {
             env->avr[n].u64[0] = ldq_p(mem_buf);
             env->avr[n].u64[1] = ldq_p(mem_buf+8);
@@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
         return 16;
     }
     if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         env->vscr = ldl_p(mem_buf);
         return 4;
     }
     if (n == 33) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
         return 4;
     }

[Qemu-devel] [PATCH 6/7] target-ppc: gdbstub: fix spe registers for little-endian guests

[Greg Kurz]

Let's reuse the ppc_maybe_bswap_register() helper, like we already do
with the general registers.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 80d53e4dcf5a..5ea168c19eae 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8849,6 +8849,7 @@ static int gdb_get_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     if (n < 32) {
 #if defined(TARGET_PPC64)
         stl_p(mem_buf, env->gpr[n] >> 32);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
 #else
         stl_p(mem_buf, env->gprh[n]);
 #endif
@@ -8856,10 +8857,12 @@ static int gdb_get_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     }
     if (n == 32) {
         stq_p(mem_buf, env->spe_acc);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
         return 8;
     }
     if (n == 33) {
         stl_p(mem_buf, env->spe_fscr);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     return 0;
@@ -8870,7 +8873,11 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     if (n < 32) {
 #if defined(TARGET_PPC64)
         target_ulong lo = (uint32_t)env->gpr[n];
-        target_ulong hi = (target_ulong)ldl_p(mem_buf) << 32;
+        target_ulong hi;
+
+        ppc_maybe_bswap_register(env, mem_buf, 4);
+
+        hi = (target_ulong)ldl_p(mem_buf) << 32;
         env->gpr[n] = lo | hi;
 #else
         env->gprh[n] = ldl_p(mem_buf);
@@ -8878,10 +8885,12 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
         return 4;
     }
     if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
         env->spe_acc = ldq_p(mem_buf);
         return 8;
     }
     if (n == 33) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         env->spe_fscr = ldl_p(mem_buf);
         return 4;
     }

[Qemu-devel] [PATCH 7/7] target-ppc: gdbstub: Add VSX support

[Greg Kurz]

From: Anton Blanchard <anton@samba.org>

Add the XML and functions to get and set VSX registers.

Signed-off-by: Anton Blanchard <anton@samba.org>
(fixed little-endian guests)
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 configure                   |    6 +++---
 gdb-xml/power-vsx.xml       |   44 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/translate_init.c |   24 +++++++++++++++++++++++
 3 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 gdb-xml/power-vsx.xml

diff --git a/configure b/configure
index 44ac9abc7e08..d96d646ba924 100755
--- a/configure
+++ b/configure
@@ -5632,20 +5632,20 @@ case "$target_name" in
   ppc64)
     TARGET_BASE_ARCH=ppc
     TARGET_ABI_DIR=ppc
-    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml"
+    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
   ;;
   ppc64le)
     TARGET_ARCH=ppc64
     TARGET_BASE_ARCH=ppc
     TARGET_ABI_DIR=ppc
-    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml"
+    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
   ;;
   ppc64abi32)
     TARGET_ARCH=ppc64
     TARGET_BASE_ARCH=ppc
     TARGET_ABI_DIR=ppc
     echo "TARGET_ABI32=y" >> $config_target_mak
-    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml"
+    gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
   ;;
   sh4|sh4eb)
     TARGET_ARCH=sh4
diff --git a/gdb-xml/power-vsx.xml b/gdb-xml/power-vsx.xml
new file mode 100644
index 000000000000..fd290e970b41
--- /dev/null
+++ b/gdb-xml/power-vsx.xml
@@ -0,0 +1,44 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2008-2015 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!-- POWER7 VSX registers that do not overlap existing FP and VMX
+     registers.  -->
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.power.vsx">
+  <reg name="vs0h" bitsize="64" type="uint64"/>
+  <reg name="vs1h" bitsize="64" type="uint64"/>
+  <reg name="vs2h" bitsize="64" type="uint64"/>
+  <reg name="vs3h" bitsize="64" type="uint64"/>
+  <reg name="vs4h" bitsize="64" type="uint64"/>
+  <reg name="vs5h" bitsize="64" type="uint64"/>
+  <reg name="vs6h" bitsize="64" type="uint64"/>
+  <reg name="vs7h" bitsize="64" type="uint64"/>
+  <reg name="vs8h" bitsize="64" type="uint64"/>
+  <reg name="vs9h" bitsize="64" type="uint64"/>
+  <reg name="vs10h" bitsize="64" type="uint64"/>
+  <reg name="vs11h" bitsize="64" type="uint64"/>
+  <reg name="vs12h" bitsize="64" type="uint64"/>
+  <reg name="vs13h" bitsize="64" type="uint64"/>
+  <reg name="vs14h" bitsize="64" type="uint64"/>
+  <reg name="vs15h" bitsize="64" type="uint64"/>
+  <reg name="vs16h" bitsize="64" type="uint64"/>
+  <reg name="vs17h" bitsize="64" type="uint64"/>
+  <reg name="vs18h" bitsize="64" type="uint64"/>
+  <reg name="vs19h" bitsize="64" type="uint64"/>
+  <reg name="vs20h" bitsize="64" type="uint64"/>
+  <reg name="vs21h" bitsize="64" type="uint64"/>
+  <reg name="vs22h" bitsize="64" type="uint64"/>
+  <reg name="vs23h" bitsize="64" type="uint64"/>
+  <reg name="vs24h" bitsize="64" type="uint64"/>
+  <reg name="vs25h" bitsize="64" type="uint64"/>
+  <reg name="vs26h" bitsize="64" type="uint64"/>
+  <reg name="vs27h" bitsize="64" type="uint64"/>
+  <reg name="vs28h" bitsize="64" type="uint64"/>
+  <reg name="vs29h" bitsize="64" type="uint64"/>
+  <reg name="vs30h" bitsize="64" type="uint64"/>
+  <reg name="vs31h" bitsize="64" type="uint64"/>
+</feature>
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 5ea168c19eae..8069f3c27956 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8897,6 +8897,26 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     return 0;
 }
 
+static int gdb_get_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+        stq_p(mem_buf, env->vsr[n]);
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        return 8;
+    }
+    return 0;
+}
+
+static int gdb_set_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        env->vsr[n] = ldq_p(mem_buf);
+        return 8;
+    }
+    return 0;
+}
+
 static int ppc_fixup_cpu(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
@@ -9002,6 +9022,10 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
         gdb_register_coprocessor(cs, gdb_get_spe_reg, gdb_set_spe_reg,
                                  34, "power-spe.xml", 0);
     }
+    if (pcc->insns_flags2 & PPC2_VSX) {
+        gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
+                                 32, "power-vsx.xml", 0);
+    }
 
     qemu_init_vcpu(cs);
 

Re: [Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[David Gibson]

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

On Fri, Jan 15, 2016 at 04:00:12PM +0100, Greg Kurz wrote:
> On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
> of the 32 first VSX registers. So if you have:
> 
> VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00
> 
> then
> 
> FPR31 = (uint64) 0x0102030405060708
> 
> The kernel stores the VSX registers in the fp_state struct following the
> host endian element ordering.
> 
> On big-endian:
> 
> fp_state.fpr[31][0] = 0x0102030405060708
> fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00
> 
> On little-endian:
> 
> fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
> fp_state.fpr[31][1] = 0x0102030405060708
> 
> The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
> QEMU considers it as big-endian and always copies element [0] to the
> fpr[] array and element [1] to the vsr[] array. This does not work with
> little-endian hosts, and you will get:
> 
> (qemu) p $f31
> 0x90a0b0c0d0e0f00
> 
> instead of:
> 
> (qemu) p $f31
> 0x102030405060708
> 
> This patch fixes the element ordering for little-endian hosts.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

If I'm understanding correctly, the only reason this bug didn't affect
things other than the gdbstub is because the get and put routines had
mirrored bugs.  So although qemu ended up with definitely wrong
information in its internal state, it reshuffled it to be right on
setting it back into KVM.

Is that correct?

> ---
>  target-ppc/kvm.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9940a9046220..45249990bda1 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -650,8 +650,13 @@ static int kvm_put_fp(CPUState *cs)
>          for (i = 0; i < 32; i++) {
>              uint64_t vsr[2];
>  
> +#ifdef HOST_WORDS_BIGENDIAN
>              vsr[0] = float64_val(env->fpr[i]);
>              vsr[1] = env->vsr[i];
> +#else
> +            vsr[0] = env->vsr[i];
> +            vsr[1] = float64_val(env->fpr[i]);
> +#endif
>              reg.addr = (uintptr_t) &vsr;
>              reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i);
>  
> @@ -721,10 +726,17 @@ static int kvm_get_fp(CPUState *cs)
>                          vsx ? "VSR" : "FPR", i, strerror(errno));
>                  return ret;
>              } else {
> +#ifdef HOST_WORDS_BIGENDIAN
>                  env->fpr[i] = vsr[0];
>                  if (vsx) {
>                      env->vsr[i] = vsr[1];
>                  }
> +#else
> +                env->fpr[i] = vsr[1];
> +                if (vsx) {
> +                    env->vsr[i] = vsr[0];
> +                }
> +#endif
>              }
>          }
>      }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[David Gibson]

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

On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> Altivec registers are 128-bit wide. They are stored in memory as two
> 64-bit values that must be byteswapped when the guest is little-endian.
> Let's reuse the ppc_maybe_bswap_register() helper for this.
> 
> We also need to fix the ordering of the 64-bit elements according to
> the target endianness, for both system and user mode.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

What bothers me about this is that avr_need_swap() now depends on both
host and guest endianness.  However the VSCR and VRSAVE swap - like
the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
depends only on guest endianness.

Why does altivec depend on the host endianness?

> ---
>  target-ppc/translate_init.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 18e9e561561f..80d53e4dcf5a 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
>  static bool avr_need_swap(CPUPPCState *env)
>  {
>  #ifdef HOST_WORDS_BIGENDIAN
> -    return false;
> +    return msr_le;
>  #else
> -    return true;
> +    return !msr_le;
>  #endif
>  }
>  
> @@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>              stq_p(mem_buf, env->avr[n].u64[1]);
>              stq_p(mem_buf+8, env->avr[n].u64[0]);
>          }
> +        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
>          return 16;
>      }
>      if (n == 32) {
>          stl_p(mem_buf, env->vscr);
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
>      if (n == 33) {
>          stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
>      return 0;
> @@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  {
>      if (n < 32) {
> +        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
>          if (!avr_need_swap(env)) {
>              env->avr[n].u64[0] = ldq_p(mem_buf);
>              env->avr[n].u64[1] = ldq_p(mem_buf+8);
> @@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>          return 16;
>      }
>      if (n == 32) {
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          env->vscr = ldl_p(mem_buf);
>          return 4;
>      }
>      if (n == 33) {
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
>          return 4;
>      }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[Greg Kurz]

On Mon, 18 Jan 2016 13:16:44 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jan 15, 2016 at 04:00:12PM +0100, Greg Kurz wrote:
> > On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
> > of the 32 first VSX registers. So if you have:
> > 
> > VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00
> > 
> > then
> > 
> > FPR31 = (uint64) 0x0102030405060708
> > 
> > The kernel stores the VSX registers in the fp_state struct following the
> > host endian element ordering.
> > 
> > On big-endian:
> > 
> > fp_state.fpr[31][0] = 0x0102030405060708
> > fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00
> > 
> > On little-endian:
> > 
> > fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
> > fp_state.fpr[31][1] = 0x0102030405060708
> > 
> > The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
> > QEMU considers it as big-endian and always copies element [0] to the
> > fpr[] array and element [1] to the vsr[] array. This does not work with
> > little-endian hosts, and you will get:
> > 
> > (qemu) p $f31
> > 0x90a0b0c0d0e0f00
> > 
> > instead of:
> > 
> > (qemu) p $f31
> > 0x102030405060708
> > 
> > This patch fixes the element ordering for little-endian hosts.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> 
> If I'm understanding correctly, the only reason this bug didn't affect
> things other than the gdbstub is because the get and put routines had

Well it is not only gdbstub actually... as showed in the changelog, it also
affects the QEMU monitor which outputs wrong values since it calls kvm_get_fpu()
as well.

> mirrored bugs.  So although qemu ended up with definitely wrong
> information in its internal state, it reshuffled it to be right on
> setting it back into KVM.
> 
> Is that correct?
> 

My guess is that the bug only affects gdbstub and ppc_cpu_dump_state(), because
these are the only cases where QEMU parses the state of FP registers... this
is indeed confirmed by the KVM bug you are referring to, that had no visible
effect for more than a year BTW.

> > ---
> >  target-ppc/kvm.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 9940a9046220..45249990bda1 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -650,8 +650,13 @@ static int kvm_put_fp(CPUState *cs)
> >          for (i = 0; i < 32; i++) {
> >              uint64_t vsr[2];
> >  
> > +#ifdef HOST_WORDS_BIGENDIAN
> >              vsr[0] = float64_val(env->fpr[i]);
> >              vsr[1] = env->vsr[i];
> > +#else
> > +            vsr[0] = env->vsr[i];
> > +            vsr[1] = float64_val(env->fpr[i]);
> > +#endif
> >              reg.addr = (uintptr_t) &vsr;
> >              reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i);
> >  
> > @@ -721,10 +726,17 @@ static int kvm_get_fp(CPUState *cs)
> >                          vsx ? "VSR" : "FPR", i, strerror(errno));
> >                  return ret;
> >              } else {
> > +#ifdef HOST_WORDS_BIGENDIAN
> >                  env->fpr[i] = vsr[0];
> >                  if (vsx) {
> >                      env->vsr[i] = vsr[1];
> >                  }
> > +#else
> > +                env->fpr[i] = vsr[1];
> > +                if (vsx) {
> > +                    env->vsr[i] = vsr[0];
> > +                }
> > +#endif
> >              }
> >          }
> >      }
> >   
> 

Re: [Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[David Gibson]

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

On Mon, Jan 18, 2016 at 09:51:56AM +0100, Greg Kurz wrote:
> On Mon, 18 Jan 2016 13:16:44 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jan 15, 2016 at 04:00:12PM +0100, Greg Kurz wrote:
> > > On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
> > > of the 32 first VSX registers. So if you have:
> > > 
> > > VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00
> > > 
> > > then
> > > 
> > > FPR31 = (uint64) 0x0102030405060708
> > > 
> > > The kernel stores the VSX registers in the fp_state struct following the
> > > host endian element ordering.
> > > 
> > > On big-endian:
> > > 
> > > fp_state.fpr[31][0] = 0x0102030405060708
> > > fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00
> > > 
> > > On little-endian:
> > > 
> > > fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
> > > fp_state.fpr[31][1] = 0x0102030405060708
> > > 
> > > The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
> > > QEMU considers it as big-endian and always copies element [0] to the
> > > fpr[] array and element [1] to the vsr[] array. This does not work with
> > > little-endian hosts, and you will get:
> > > 
> > > (qemu) p $f31
> > > 0x90a0b0c0d0e0f00
> > > 
> > > instead of:
> > > 
> > > (qemu) p $f31
> > > 0x102030405060708
> > > 
> > > This patch fixes the element ordering for little-endian hosts.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> > 
> > If I'm understanding correctly, the only reason this bug didn't affect
> > things other than the gdbstub is because the get and put routines had
> 
> Well it is not only gdbstub actually... as showed in the changelog, it also
> affects the QEMU monitor which outputs wrong values since it calls kvm_get_fpu()
> as well.

Yes, sorry, I didn't express that well.  My point is that the only
reason things aren't going horribly wrong is that qemu is only ever
touching the FP/VSX values for debug, and the get/put into KVM is
wrong in such a way that the right values go back again as long as
qemu doesn't try to change them.

> > mirrored bugs.  So although qemu ended up with definitely wrong
> > information in its internal state, it reshuffled it to be right on
> > setting it back into KVM.
> > 
> > Is that correct?
> > 
> 
> My guess is that the bug only affects gdbstub and ppc_cpu_dump_state(), because
> these are the only cases where QEMU parses the state of FP registers... this
> is indeed confirmed by the KVM bug you are referring to, that had no visible
> effect for more than a year BTW.

Ok.

Still waiting for a reply for my query on 5/7, then I'm happy to apply
these.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[Greg Kurz]

On Mon, 18 Jan 2016 13:25:19 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> > Altivec registers are 128-bit wide. They are stored in memory as two
> > 64-bit values that must be byteswapped when the guest is little-endian.
> > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > 
> > We also need to fix the ordering of the 64-bit elements according to
> > the target endianness, for both system and user mode.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> 
> What bothers me about this is that avr_need_swap() now depends on both
> host and guest endianness.  However the VSCR and VRSAVE swap - like
> the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> depends only on guest endianness.
> 
> Why does altivec depend on the host endianness?
> 

This has always been the case:

commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Sat Jan 24 15:08:09 2009 +0000

    target-ppc: Add Altivec register read/write using XML

[...]

+static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+#ifdef WORDS_BIGENDIAN
+        stq_p(mem_buf, env->avr[n].u64[0]);
+        stq_p(mem_buf+8, env->avr[n].u64[1]);
+#else
+        stq_p(mem_buf, env->avr[n].u64[1]);
+        stq_p(mem_buf+8, env->avr[n].u64[0]);
+#endif
+        return 16;
+    }

My understanding is that gdb expects registers to be presented with
the target endianness but QEMU have them in host endianness.

The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
according to the target effective endianness because stq_p() always
consider both ppc64 and ppc64le to be big endian.

Here, we have a 128-bit register that we break into two 64-bit values
in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
But we also have to reorder these quad words if the host endianness
differs from the target's one. This is the purpose of avr_need_swap().

Cheers.

--
Greg

> > ---
> >  target-ppc/translate_init.c |   12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 18e9e561561f..80d53e4dcf5a 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
> >  static bool avr_need_swap(CPUPPCState *env)
> >  {
> >  #ifdef HOST_WORDS_BIGENDIAN
> > -    return false;
> > +    return msr_le;
> >  #else
> > -    return true;
> > +    return !msr_le;
> >  #endif
> >  }
> >  
> > @@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >              stq_p(mem_buf, env->avr[n].u64[1]);
> >              stq_p(mem_buf+8, env->avr[n].u64[0]);
> >          }
> > +        ppc_maybe_bswap_register(env, mem_buf, 8);
> > +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> >          return 16;
> >      }
> >      if (n == 32) {
> >          stl_p(mem_buf, env->vscr);
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          return 4;
> >      }
> >      if (n == 33) {
> >          stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          return 4;
> >      }
> >      return 0;
> > @@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >  static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >  {
> >      if (n < 32) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 8);
> > +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> >          if (!avr_need_swap(env)) {
> >              env->avr[n].u64[0] = ldq_p(mem_buf);
> >              env->avr[n].u64[1] = ldq_p(mem_buf+8);
> > @@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >          return 16;
> >      }
> >      if (n == 32) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          env->vscr = ldl_p(mem_buf);
> >          return 4;
> >      }
> >      if (n == 33) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
> >          return 4;
> >      }
> >   
> 

Re: [Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[Greg Kurz]

On Tue, 19 Jan 2016 11:55:10 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jan 18, 2016 at 09:51:56AM +0100, Greg Kurz wrote:
> > On Mon, 18 Jan 2016 13:16:44 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jan 15, 2016 at 04:00:12PM +0100, Greg Kurz wrote:  
> > > > On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
> > > > of the 32 first VSX registers. So if you have:
> > > > 
> > > > VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00
> > > > 
> > > > then
> > > > 
> > > > FPR31 = (uint64) 0x0102030405060708
> > > > 
> > > > The kernel stores the VSX registers in the fp_state struct following the
> > > > host endian element ordering.
> > > > 
> > > > On big-endian:
> > > > 
> > > > fp_state.fpr[31][0] = 0x0102030405060708
> > > > fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00
> > > > 
> > > > On little-endian:
> > > > 
> > > > fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
> > > > fp_state.fpr[31][1] = 0x0102030405060708
> > > > 
> > > > The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
> > > > QEMU considers it as big-endian and always copies element [0] to the
> > > > fpr[] array and element [1] to the vsr[] array. This does not work with
> > > > little-endian hosts, and you will get:
> > > > 
> > > > (qemu) p $f31
> > > > 0x90a0b0c0d0e0f00
> > > > 
> > > > instead of:
> > > > 
> > > > (qemu) p $f31
> > > > 0x102030405060708
> > > > 
> > > > This patch fixes the element ordering for little-endian hosts.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>    
> > > 
> > > If I'm understanding correctly, the only reason this bug didn't affect
> > > things other than the gdbstub is because the get and put routines had  
> > 
> > Well it is not only gdbstub actually... as showed in the changelog, it also
> > affects the QEMU monitor which outputs wrong values since it calls kvm_get_fpu()
> > as well.  
> 
> Yes, sorry, I didn't express that well.  My point is that the only
> reason things aren't going horribly wrong is that qemu is only ever
> touching the FP/VSX values for debug, and the get/put into KVM is

I fully agree with that QEMU not touching FP/VSX is a key point for
not breaking anything.

> wrong in such a way that the right values go back again as long as
> qemu doesn't try to change them.
> 

I suppose so but I must confess I did not invest time to understand how
this KVM bug did not break the guest in some way...

> > > mirrored bugs.  So although qemu ended up with definitely wrong
> > > information in its internal state, it reshuffled it to be right on
> > > setting it back into KVM.
> > > 
> > > Is that correct?
> > >   
> > 
> > My guess is that the bug only affects gdbstub and ppc_cpu_dump_state(), because
> > these are the only cases where QEMU parses the state of FP registers... this
> > is indeed confirmed by the KVM bug you are referring to, that had no visible
> > effect for more than a year BTW.  
> 
> Ok.
> 
> Still waiting for a reply for my query on 5/7, then I'm happy to apply
> these.
> 

Yeah sorry for the delay... I had written a reply but I wasn't happy with
my poor English *again* so I spent some more time rewording. I've answered
at last ! :)

Thanks !

--
Greg

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[David Gibson]

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

On Tue, Jan 19, 2016 at 10:59:20AM +0100, Greg Kurz wrote:
> On Mon, 18 Jan 2016 13:25:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> > > Altivec registers are 128-bit wide. They are stored in memory as two
> > > 64-bit values that must be byteswapped when the guest is little-endian.
> > > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > > 
> > > We also need to fix the ordering of the 64-bit elements according to
> > > the target endianness, for both system and user mode.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> > 
> > What bothers me about this is that avr_need_swap() now depends on both
> > host and guest endianness.  However the VSCR and VRSAVE swap - like
> > the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> > depends only on guest endianness.
> > 
> > Why does altivec depend on the host endianness?
> > 
> 
> This has always been the case:
> 
> commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
> Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Sat Jan 24 15:08:09 2009 +0000
> 
>     target-ppc: Add Altivec register read/write using XML
> 
> [...]
> 
> +static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
> +{
> +    if (n < 32) {
> +#ifdef WORDS_BIGENDIAN
> +        stq_p(mem_buf, env->avr[n].u64[0]);
> +        stq_p(mem_buf+8, env->avr[n].u64[1]);
> +#else
> +        stq_p(mem_buf, env->avr[n].u64[1]);
> +        stq_p(mem_buf+8, env->avr[n].u64[0]);
> +#endif
> +        return 16;
> +    }
> 
> My understanding is that gdb expects registers to be presented with
> the target endianness but QEMU have them in host endianness.
> 
> The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
> according to the target effective endianness because stq_p() always
> consider both ppc64 and ppc64le to be big endian.
> 
> Here, we have a 128-bit register that we break into two 64-bit values
> in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
> But we also have to reorder these quad words if the host endianness
> differs from the target's one. This is the purpose of avr_need_swap().

Ok, understood.  I've merged the series to ppc-for-2.6

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[Greg Kurz]

On Wed, 20 Jan 2016 13:13:57 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jan 19, 2016 at 10:59:20AM +0100, Greg Kurz wrote:
> > On Mon, 18 Jan 2016 13:25:19 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:  
> > > > Altivec registers are 128-bit wide. They are stored in memory as two
> > > > 64-bit values that must be byteswapped when the guest is little-endian.
> > > > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > > > 
> > > > We also need to fix the ordering of the 64-bit elements according to
> > > > the target endianness, for both system and user mode.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>    
> > > 
> > > What bothers me about this is that avr_need_swap() now depends on both
> > > host and guest endianness.  However the VSCR and VRSAVE swap - like
> > > the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> > > depends only on guest endianness.
> > > 
> > > Why does altivec depend on the host endianness?
> > >   
> > 
> > This has always been the case:
> > 
> > commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
> > Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> > Date:   Sat Jan 24 15:08:09 2009 +0000
> > 
> >     target-ppc: Add Altivec register read/write using XML
> > 
> > [...]
> > 
> > +static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
> > +{
> > +    if (n < 32) {
> > +#ifdef WORDS_BIGENDIAN
> > +        stq_p(mem_buf, env->avr[n].u64[0]);
> > +        stq_p(mem_buf+8, env->avr[n].u64[1]);
> > +#else
> > +        stq_p(mem_buf, env->avr[n].u64[1]);
> > +        stq_p(mem_buf+8, env->avr[n].u64[0]);
> > +#endif
> > +        return 16;
> > +    }
> > 
> > My understanding is that gdb expects registers to be presented with
> > the target endianness but QEMU have them in host endianness.
> > 
> > The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
> > according to the target effective endianness because stq_p() always
> > consider both ppc64 and ppc64le to be big endian.
> > 
> > Here, we have a 128-bit register that we break into two 64-bit values
> > in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
> > But we also have to reorder these quad words if the host endianness
> > differs from the target's one. This is the purpose of avr_need_swap().  
> 
> Ok, understood.  I've merged the series to ppc-for-2.6
> 

Cool ! Thanks.

--
Greg

[Qemu-devel] [PATCH 5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

[Greg Kurz]

Altivec registers are 128-bit wide. They are stored in memory as two
64-bit values that must be byteswapped when the guest is little-endian.
Let's reuse the ppc_maybe_bswap_register() helper for this.

We also need to fix the ordering of the 64-bit elements according to
the target endianness, for both system and user mode.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 18e9e561561f..80d53e4dcf5a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
 static bool avr_need_swap(CPUPPCState *env)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-    return false;
+    return msr_le;
 #else
-    return true;
+    return !msr_le;
 #endif
 }
 
@@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
             stq_p(mem_buf, env->avr[n].u64[1]);
             stq_p(mem_buf+8, env->avr[n].u64[0]);
         }
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
         return 16;
     }
     if (n == 32) {
         stl_p(mem_buf, env->vscr);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     if (n == 33) {
         stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
     }
     return 0;
@@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 8);
+        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
         if (!avr_need_swap(env)) {
             env->avr[n].u64[0] = ldq_p(mem_buf);
             env->avr[n].u64[1] = ldq_p(mem_buf+8);
@@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
         return 16;
     }
     if (n == 32) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         env->vscr = ldl_p(mem_buf);
         return 4;
     }
     if (n == 33) {
+        ppc_maybe_bswap_register(env, mem_buf, 4);
         env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
         return 4;
     }