[Qemu-devel] [PATCH 0/2] ppc64le-linux-user fixes

[Qemu-devel] [PATCH 0/2] ppc64le-linux-user fixes

[Richard Henderson]

Two fixes needed to run and debug hello world.


r~


Richard Henderson (2):
  target-ppc: Change default cpu for ppc64le-linux-user
  target-ppc: Fix gdbstub for ppc64le-linux-user

 linux-user/main.c           | 10 +++++++---
 target-ppc/gdbstub.c        | 24 +++++++++++++-----------
 target-ppc/translate_init.c |  4 ++++
 3 files changed, 24 insertions(+), 14 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] target-ppc: Change default cpu for ppc64le-linux-user

[Richard Henderson]

The default, 970fx, doesn't support MSR_LE.  So even though we set LE in
ppc_cpu_reset, it gets cleared again in hreg_store_msr.  Error out if a
user-selected cpu model doesn't support LE.

Cc: Aldy Hernandez <aldyh@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
The warning one gets from the unimplemented insns from tcg
is perhaps unfortunate, but it's better than silently dropping
the MSR_LE bit and interpreting the first few insns with the
wrong endianness and generating SIGILL.

One could, perhaps, simply return false from need_byteswap.
But then the value of MSR would still be wrong, and I can imagine
that would affect gdb's interpretation of the current mode.


r~
---
 linux-user/main.c           | 10 +++++++---
 target-ppc/translate_init.c |  4 ++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 900a17f..058b08c 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3901,11 +3901,15 @@ int main(int argc, char **argv, char **envp)
 #elif defined TARGET_OPENRISC
         cpu_model = "or1200";
 #elif defined(TARGET_PPC)
-#ifdef TARGET_PPC64
+# ifdef TARGET_PPC64
+#  ifdef TARGET_WORDS_BIGENDIAN
         cpu_model = "970fx";
-#else
+#  else
+        cpu_model = "POWER7";
+#  endif
+# else
         cpu_model = "750";
-#endif
+# endif
 #else
         cpu_model = "any";
 #endif
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 85581c9..72128d8 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9538,6 +9538,10 @@ static void ppc_cpu_reset(CPUState *s)
 #endif
 #if !defined(TARGET_WORDS_BIGENDIAN)
     msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
+    if (!((env->msr_mask >> MSR_LE) & 1)) {
+        fprintf(stderr, "Selected CPU does not support little-endian.\n");
+        exit(1);
+    }
 #endif
 #endif
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Change default cpu for ppc64le-linux-user

[Alexander Graf]

> Am 28.06.2014 um 18:45 schrieb Richard Henderson <rth@twiddle.net>:
> 
> The default, 970fx, doesn't support MSR_LE.  So even though we set LE in
> ppc_cpu_reset, it gets cleared again in hreg_store_msr.  Error out if a
> user-selected cpu model doesn't support LE.
> 
> Cc: Aldy Hernandez <aldyh@redhat.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
> The warning one gets from the unimplemented insns from tcg
> is perhaps unfortunate, but it's better than silently dropping
> the MSR_LE bit and interpreting the first few insns with the
> wrong endianness and generating SIGILL.
> 
> One could, perhaps, simply return false from need_byteswap.
> But then the value of MSR would still be wrong, and I can imagine
> that would affect gdb's interpretation of the current mode.
> 
> 
> r~
> ---
> linux-user/main.c           | 10 +++++++---
> target-ppc/translate_init.c |  4 ++++
> 2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 900a17f..058b08c 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3901,11 +3901,15 @@ int main(int argc, char **argv, char **envp)
> #elif defined TARGET_OPENRISC
>         cpu_model = "or1200";
> #elif defined(TARGET_PPC)
> -#ifdef TARGET_PPC64
> +# ifdef TARGET_PPC64
> +#  ifdef TARGET_WORDS_BIGENDIAN
>         cpu_model = "970fx";
> -#else
> +#  else
> +        cpu_model = "POWER7";

How about we switch to p7 for BE top?

Alex

> +#  endif
> +# else
>         cpu_model = "750";
> -#endif
> +# endif
> #else
>         cpu_model = "any";
> #endif
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 85581c9..72128d8 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9538,6 +9538,10 @@ static void ppc_cpu_reset(CPUState *s)
> #endif
> #if !defined(TARGET_WORDS_BIGENDIAN)
>     msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
> +    if (!((env->msr_mask >> MSR_LE) & 1)) {
> +        fprintf(stderr, "Selected CPU does not support little-endian.\n");
> +        exit(1);
> +    }
> #endif
> #endif
> 
> -- 
> 1.9.3
> 

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Change default cpu for ppc64le-linux-user

[Richard Henderson]

On 06/28/2014 09:50 AM, Alexander Graf wrote:
> How about we switch to p7 for BE top?

Not ideal until we implement all of p7's insns in TCG:

Warning: Disabling some instructions which are not emulated by TCG (0x0, 0x4)


r~

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Change default cpu for ppc64le-linux-user

[Tom Musta]

On 6/28/2014 1:42 PM, Richard Henderson wrote:
> On 06/28/2014 09:50 AM, Alexander Graf wrote:
>> How about we switch to p7 for BE top?
> 
> Not ideal until we implement all of p7's insns in TCG:
> 
> Warning: Disabling some instructions which are not emulated by TCG (0x0, 0x4)
> 
> 
> r~
> 

That was dealt with in this patch: http://lists.nongnu.org/archive/html/qemu-ppc/2014-06/msg00479.html and has now made its way into HEAD.

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Change default cpu for ppc64le-linux-user

[Alexander Graf]

On 30.06.14 14:08, Tom Musta wrote:
> On 6/28/2014 1:42 PM, Richard Henderson wrote:
>> On 06/28/2014 09:50 AM, Alexander Graf wrote:
>>> How about we switch to p7 for BE top?
>> Not ideal until we implement all of p7's insns in TCG:
>>
>> Warning: Disabling some instructions which are not emulated by TCG (0x0, 0x4)
>>
>>
>> r~
>>
> That was dealt with in this patch: http://lists.nongnu.org/archive/html/qemu-ppc/2014-06/msg00479.html and has now made its way into HEAD.
>

Yup, I'll change the default for BE to POWER7 too while applying.


Alex

[Qemu-devel] [PATCH 2/2] target-ppc: Fix gdbstub for ppc64le-linux-user

[Richard Henderson]

The bswap that's needed for system mode isn't required for
user mode, and in fact breaks debugging.

Cc: Aldy Hernandez <aldyh@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-ppc/gdbstub.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
index 381a3c7..7a01e62 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -58,16 +58,24 @@ static int ppc_gdb_register_len(int n)
     }
 }
 
-
-static void ppc_gdb_swap_register(uint8_t *mem_buf, int n, int len)
+/* We need to present the registers to gdb in the "current" memory ordering.
+   For user-only mode we get this for free; TARGET_WORDS_BIGENDIAN is set to
+   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)
 {
-    if (len == 4) {
+#ifndef CONFIG_USER_ONLY
+    if (!msr_le) {
+        /* do nothing */
+    } else if (len == 4) {
         bswap32s((uint32_t *)mem_buf);
     } else if (len == 8) {
         bswap64s((uint64_t *)mem_buf);
     } else {
         g_assert_not_reached();
     }
+#endif
 }
 
 /* Old gdb always expects FP registers.  Newer (xml-aware) gdb only
@@ -125,10 +133,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         }
     }
-    if (msr_le) {
-        /* If cpu is in LE mode, convert memory contents to LE. */
-        ppc_gdb_swap_register(mem_buf, n, r);
-    }
+    maybe_bswap_register(env, mem_buf, r);
     return r;
 }
 
@@ -141,10 +146,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     if (!r) {
         return r;
     }
-    if (msr_le) {
-        /* If cpu is in LE mode, convert memory contents to LE. */
-        ppc_gdb_swap_register(mem_buf, n, r);
-    }
+    maybe_bswap_register(env, mem_buf, r);
     if (n < 32) {
         /* gprs */
         env->gpr[n] = ldtul_p(mem_buf);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2] target-ppc: Fix gdbstub for ppc64le-linux-user

[Alexander Graf]

On 28.06.14 18:45, Richard Henderson wrote:
> The bswap that's needed for system mode isn't required for
> user mode, and in fact breaks debugging.
>
> Cc: Aldy Hernandez <aldyh@redhat.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

This breaks the Apple gdbstub backend we recently got in target-ppc. 
I'll fix it up while applying.


Alex

Re: [Qemu-devel] [PATCH 0/2] ppc64le-linux-user fixes

[Alexander Graf]

On 28.06.14 18:45, Richard Henderson wrote:
> Two fixes needed to run and debug hello world.

Thanks, fixed up both patches and applied the to ppc-next (2.1 branch).


Alex