diff mbox series

ppc: Fix size of ppc64 xer register (fwd)

Message ID alpine.LSU.2.21.1802231729040.10187@wotan.suse.de
State New
Headers show
Series ppc: Fix size of ppc64 xer register (fwd) | expand

Commit Message

Michael Matz Feb. 23, 2018, 5:29 p.m. UTC
The normal gdb definition of the XER registers is only 32 bit,
and that's what the current version of power64-core.xml also
says (seems copied from gdb's).  But qemu's idea of the XER register
is target_ulong (in CPUPPCState, ppc_gdb_register_len and
ppc_cpu_gdb_read_register)

That mismatch leads to the following message when attaching
with gdb:

  Truncated register 32 in remote 'g' packet

(and following on that qemu stops responding).  The simple fix is
to say the truth in the .xml file.  But the better fix is to
actually make it 32bit on the wire, as old gdbs don't support
XML files for describing registers.  Also the XER state in qemu
doesn't seem to use the high 32 bits, so sending it off to gdb
doesn't seem worthwhile.

Signed-off-by: Michael Matz <matz@suse.de>
---
 target/ppc/gdbstub.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

no-reply@patchew.org Feb. 24, 2018, 7:57 a.m. UTC | #1
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: alpine.LSU.2.21.1802231729040.10187@wotan.suse.de
Subject: [Qemu-devel] [PATCH] ppc: Fix size of ppc64 xer register (fwd)

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
41b2585f4d ppc: Fix size of ppc64 xer register (fwd)

=== OUTPUT BEGIN ===
Checking PATCH 1/1: ppc: Fix size of ppc64 xer register (fwd)...
ERROR: spaces required around that '+' (ctx:VxV)
#39: FILE: target/ppc/gdbstub.c:43:
+    case 69+32: /* xer */
            ^

total: 1 errors, 0 warnings, 59 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
David Gibson March 24, 2018, 5:30 a.m. UTC | #2
On Fri, Feb 23, 2018 at 05:29:56PM +0000, Michael Matz wrote:
> The normal gdb definition of the XER registers is only 32 bit,
> and that's what the current version of power64-core.xml also
> says (seems copied from gdb's).  But qemu's idea of the XER register
> is target_ulong (in CPUPPCState, ppc_gdb_register_len and
> ppc_cpu_gdb_read_register)
> 
> That mismatch leads to the following message when attaching
> with gdb:
> 
>   Truncated register 32 in remote 'g' packet
> 
> (and following on that qemu stops responding).  The simple fix is
> to say the truth in the .xml file.  But the better fix is to
> actually make it 32bit on the wire, as old gdbs don't support
> XML files for describing registers.  Also the XER state in qemu
> doesn't seem to use the high 32 bits, so sending it off to gdb
> doesn't seem worthwhile.
> 
> Signed-off-by: Michael Matz <matz@suse.de>

Sorry I've taken so long to look at this.  I've now applied it to my
ppc-for-2.13 branch (since it's not a regression, I don't think it's
justified to include it during the 2.12 hard freeze).

In future, please CC me directly on mails (as ppc maintainer) and also
CC qemu-ppc@nongnu.org.  I might never have spotted this if Alex Graf
hadn't forwarded a pointer to me.

> ---
>  target/ppc/gdbstub.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 7a33813..b6f6693 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -37,10 +37,10 @@ static int ppc_gdb_register_len_apple(int n)
>      case 65+32: /* msr */
>      case 67+32: /* lr */
>      case 68+32: /* ctr */
> -    case 69+32: /* xer */
>      case 70+32: /* fpscr */
>          return 8;
>      case 66+32: /* cr */
> +    case 69+32: /* xer */
>          return 4;
>      default:
>          return 0;
> @@ -61,6 +61,8 @@ static int ppc_gdb_register_len(int n)
>          return 8;
>      case 66:
>          /* cr */
> +    case 69:
> +        /* xer */
>          return 4;
>      case 64:
>          /* nip */
> @@ -70,8 +72,6 @@ static int ppc_gdb_register_len(int n)
>          /* lr */
>      case 68:
>          /* ctr */
> -    case 69:
> -        /* xer */
>          return sizeof(target_ulong);
>      case 70:
>          /* fpscr */
> @@ -152,7 +152,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>              gdb_get_regl(mem_buf, env->ctr);
>              break;
>          case 69:
> -            gdb_get_regl(mem_buf, env->xer);
> +            gdb_get_reg32(mem_buf, env->xer);
>              break;
>          case 70:
>              gdb_get_reg32(mem_buf, env->fpscr);
> @@ -208,7 +208,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>              gdb_get_reg64(mem_buf, env->ctr);
>              break;
>          case 69 + 32:
> -            gdb_get_reg64(mem_buf, env->xer);
> +            gdb_get_reg32(mem_buf, env->xer);
>              break;
>          case 70 + 32:
>              gdb_get_reg64(mem_buf, env->fpscr);
> @@ -259,7 +259,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>              env->ctr = ldtul_p(mem_buf);
>              break;
>          case 69:
> -            env->xer = ldtul_p(mem_buf);
> +            env->xer = ldl_p(mem_buf);
>              break;
>          case 70:
>              /* fpscr */
> @@ -309,7 +309,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>              env->ctr = ldq_p(mem_buf);
>              break;
>          case 69 + 32:
> -            env->xer = ldq_p(mem_buf);
> +            env->xer = ldl_p(mem_buf);
>              break;
>          case 70 + 32:
>              /* fpscr */
diff mbox series

Patch

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 7a33813..b6f6693 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -37,10 +37,10 @@  static int ppc_gdb_register_len_apple(int n)
     case 65+32: /* msr */
     case 67+32: /* lr */
     case 68+32: /* ctr */
-    case 69+32: /* xer */
     case 70+32: /* fpscr */
         return 8;
     case 66+32: /* cr */
+    case 69+32: /* xer */
         return 4;
     default:
         return 0;
@@ -61,6 +61,8 @@  static int ppc_gdb_register_len(int n)
         return 8;
     case 66:
         /* cr */
+    case 69:
+        /* xer */
         return 4;
     case 64:
         /* nip */
@@ -70,8 +72,6 @@  static int ppc_gdb_register_len(int n)
         /* lr */
     case 68:
         /* ctr */
-    case 69:
-        /* xer */
         return sizeof(target_ulong);
     case 70:
         /* fpscr */
@@ -152,7 +152,7 @@  int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
             gdb_get_regl(mem_buf, env->ctr);
             break;
         case 69:
-            gdb_get_regl(mem_buf, env->xer);
+            gdb_get_reg32(mem_buf, env->xer);
             break;
         case 70:
             gdb_get_reg32(mem_buf, env->fpscr);
@@ -208,7 +208,7 @@  int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
             gdb_get_reg64(mem_buf, env->ctr);
             break;
         case 69 + 32:
-            gdb_get_reg64(mem_buf, env->xer);
+            gdb_get_reg32(mem_buf, env->xer);
             break;
         case 70 + 32:
             gdb_get_reg64(mem_buf, env->fpscr);
@@ -259,7 +259,7 @@  int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             env->ctr = ldtul_p(mem_buf);
             break;
         case 69:
-            env->xer = ldtul_p(mem_buf);
+            env->xer = ldl_p(mem_buf);
             break;
         case 70:
             /* fpscr */
@@ -309,7 +309,7 @@  int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
             env->ctr = ldq_p(mem_buf);
             break;
         case 69 + 32:
-            env->xer = ldq_p(mem_buf);
+            env->xer = ldl_p(mem_buf);
             break;
         case 70 + 32:
             /* fpscr */