[Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers

[Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers

[Thomas Falcon]

This patch allows registers to be properly read from and written to
when using the gdbstub to debug a ppc guest running in little
endian mode.  It accomplishes this goal by byte swapping the values of
any registers if the MSR:LE value is set.

Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
Differences from v2:

Fixed formatting issues
Added logic to ensure only FP registers have a guaranteed size of 8 bytes

---
  target-ppc/cpu-qom.h        |  2 ++
  target-ppc/gdbstub.c        | 45 
+++++++++++++++++++++++++++++++++++++++++++++
  target-ppc/translate_init.c |  4 ++--
  3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 72b2232..992963f 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -109,7 +109,9 @@ void ppc_cpu_dump_statistics(CPUState *cpu, FILE *f,
                               fprintf_function cpu_fprintf, int flags);
  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
+int ppc_cpu_gdb_read_register_wrap(CPUState *cpu, uint8_t *buf, int reg);
  int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+int ppc_cpu_gdb_write_register_wrap(CPUState *cpu, uint8_t *buf, int reg);
  int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
                                     CPUState *cpu, void *opaque);
  int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
index 1c91090..cc4eac5 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -21,6 +21,51 @@
  #include "qemu-common.h"
  #include "exec/gdbstub.h"

+/* The following functions are used to ensure the correct
+ * transfer of registers between a little endian ppc target
+ * and a big endian host by checking the LE bit in the Machine State 
Register
+ */
+
+int ppc_cpu_gdb_read_register_wrap(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    int len = ppc_cpu_gdb_read_register(cs, mem_buf, n), i;
+    if (msr_le) {
+        uint8_t tmp;
+        for (i = 0; i < len/2 ; i++) {
+            tmp = *(mem_buf + i);
+            *(mem_buf + i) = *(mem_buf + len - 1 - i);
+            *(mem_buf + len - 1 - i) = tmp;
+        }
+    }
+    return len;
+}
+
+int ppc_cpu_gdb_write_register_wrap(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    if (msr_le) {
+        int len = 0, i = 0;
+        if (n > 31 && n < 64) {
+            len = 8;
+        } else if (n == 66) {
+            len = 4;
+        } else {
+            len = sizeof(target_ulong);
+        }
+        uint8_t tmp;
+        for (i = 0; i < len/2; i++) {
+            tmp = *(mem_buf + i);
+            *(mem_buf+i) = *(mem_buf + len - 1 - i);
+            *(mem_buf + len - 1 - i) = tmp;
+        }
+    }
+    return ppc_cpu_gdb_write_register(cs, mem_buf, n);
+}
+
  /* Old gdb always expects FP registers.  Newer (xml-aware) gdb only
   * expects whatever the target description contains.  Due to a
   * historical mishap the FP registers appear in between core integer
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c030a20..41ea4b7 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8655,8 +8655,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, 
void *data)
      cc->dump_state = ppc_cpu_dump_state;
      cc->dump_statistics = ppc_cpu_dump_statistics;
      cc->set_pc = ppc_cpu_set_pc;
-    cc->gdb_read_register = ppc_cpu_gdb_read_register;
-    cc->gdb_write_register = ppc_cpu_gdb_write_register;
+    cc->gdb_read_register = ppc_cpu_gdb_read_register_wrap;
+    cc->gdb_write_register = ppc_cpu_gdb_write_register_wrap;
  #ifndef CONFIG_USER_ONLY
      cc->get_phys_page_debug = ppc_cpu_get_phys_page_debug;
      cc->vmsd = &vmstate_ppc_cpu;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers

[Alexander Graf]

On 17.01.2014, at 22:02, Thomas Falcon <tlfalcon@linux.vnet.ibm.com> wrote:

> This patch allows registers to be properly read from and written to
> when using the gdbstub to debug a ppc guest running in little
> endian mode.  It accomplishes this goal by byte swapping the values of
> any registers if the MSR:LE value is set.
> 
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> ---
> Differences from v2:
> 
> Fixed formatting issues
> Added logic to ensure only FP registers have a guaranteed size of 8 bytes


I don't really like how the write case has to know about the size of a register (maybe we could factor this out into a single function for all reads and writes?), but this is good enough for now :). However, I can't apply the patch as your email client seems to have broken the patch formatting.


Alex

Re: [Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers

[Thomas Falcon]

On 01/20/2014 08:33 AM, Alexander Graf wrote:
> On 17.01.2014, at 22:02, Thomas Falcon <tlfalcon@linux.vnet.ibm.com> wrote:
>
>> This patch allows registers to be properly read from and written to
>> when using the gdbstub to debug a ppc guest running in little
>> endian mode.  It accomplishes this goal by byte swapping the values of
>> any registers if the MSR:LE value is set.
>>
>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>> ---
>> Differences from v2:
>>
>> Fixed formatting issues
>> Added logic to ensure only FP registers have a guaranteed size of 8 bytes
>
> I don't really like how the write case has to know about the size of a register (maybe we could factor this out into a single function for all reads and writes?), but this is good enough for now :). However, I can't apply the patch as your email client seems to have broken the patch formatting.
>
>
> Alex
>
>
I'm not sure of a way to swap the value without knowing its size.  In both read and write, the size needs to be known and is hardcoded in some  cases.  The write case cannot know the size without a conditional since we need to swap in mem_buf before we call ppc_cpu_gdb_write_register.  Maybe we could get around this by hanging ppc_cpu_gdb_write_register so that it returns a pointer to the register being overwritten, and then we could swap that instead of mem_buf?  But even then I guess we would still need to check the size of the register before we called bswap32/64.

Anyway, sorry about the formatting issues again.  Should I just resubmit the patch as is?


Thanks,

Tom

Re: [Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers

[Alexander Graf]

On 20.01.2014, at 18:49, Thomas Falcon <tlfalcon@linux.vnet.ibm.com> wrote:

> On 01/20/2014 08:33 AM, Alexander Graf wrote:
>> On 17.01.2014, at 22:02, Thomas Falcon <tlfalcon@linux.vnet.ibm.com> wrote:
>> 
>>> This patch allows registers to be properly read from and written to
>>> when using the gdbstub to debug a ppc guest running in little
>>> endian mode.  It accomplishes this goal by byte swapping the values of
>>> any registers if the MSR:LE value is set.
>>> 
>>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>>> ---
>>> Differences from v2:
>>> 
>>> Fixed formatting issues
>>> Added logic to ensure only FP registers have a guaranteed size of 8 bytes
>> 
>> I don't really like how the write case has to know about the size of a register (maybe we could factor this out into a single function for all reads and writes?), but this is good enough for now :). However, I can't apply the patch as your email client seems to have broken the patch formatting.
>> 
>> 
>> Alex
>> 
>> 
> I'm not sure of a way to swap the value without knowing its size.  In both read and write, the size needs to be known and is hardcoded in some  cases.  The write case cannot know the size without a conditional since we need to swap in mem_buf before we call ppc_cpu_gdb_write_register.  Maybe we could get around this by hanging ppc_cpu_gdb_write_register so that it returns a pointer to the register being overwritten, and then we could swap that instead of mem_buf?  But even then I guess we would still need to check the size of the register before we called bswap32/64.

Well, the easiest way would be to factor out the size from the content reads and writes. Something like this:

diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
index 1c91090..17243cb 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -28,27 +28,67 @@
  * FP regs zero size when talking to a newer gdb.
  */

+static int ppc_cpu_gdb_register_len(int n)
+{
+    switch (n) {
+    case 0 ... 31:
+        /* gprs */
+        return sizeof(target_ulong);
+    case 32 ... 63:
+        /* fprs */
+        if (gdb_has_xml) {
+            return 0;
+        }
+        return 8;
+    case 66:
+        /* cr */
+        return 4;
+    case 64:
+        /* nip */
+    case 65:
+        /* msr */
+    case 67:
+        /* lr */
+    case 68:
+        /* ctr */
+    case 69:
+        /* xer */
+        return sizeof(target_ulong);
+    case 70:
+        /* fpscr */
+        if (gdb_has_xml) {
+            return 0;
+        }
+        return sizeof(target_ulong);
+    default:
+        return 0;
+    }
+}
+
 int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    int r = ppc_cpu_gdb_register_len(n);
+
+    if (!r) {
+        return r;
+    }

     if (n < 32) {
         /* gprs */
-        return gdb_get_regl(mem_buf, env->gpr[n]);
+        gdb_get_regl(mem_buf, env->gpr[n]);
     } else if (n < 64) {
         /* fprs */
-        if (gdb_has_xml) {
-            return 0;
-        }
         stfq_p(mem_buf, env->fpr[n-32]);
-        return 8;
     } else {
         switch (n) {
         case 64:
-            return gdb_get_regl(mem_buf, env->nip);
+            gdb_get_regl(mem_buf, env->nip);
+            break;
         case 65:
-            return gdb_get_regl(mem_buf, env->msr);
+            gdb_get_regl(mem_buf, env->msr);
+            break;
         case 66:
             {
                 uint32_t cr = 0;
@@ -56,50 +96,51 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
                 for (i = 0; i < 8; i++) {
                     cr |= env->crf[i] << (32 - ((i + 1) * 4));
                 }
-                return gdb_get_reg32(mem_buf, cr);
+                gdb_get_reg32(mem_buf, cr);
+                break;
             }
         case 67:
-            return gdb_get_regl(mem_buf, env->lr);
+            gdb_get_regl(mem_buf, env->lr);
+            break;
         case 68:
-            return gdb_get_regl(mem_buf, env->ctr);
+            gdb_get_regl(mem_buf, env->ctr);
+            break;
         case 69:
-            return gdb_get_regl(mem_buf, env->xer);
+            gdb_get_regl(mem_buf, env->xer);
+            break;
         case 70:
-            {
-                if (gdb_has_xml) {
-                    return 0;
-                }
-                return gdb_get_reg32(mem_buf, env->fpscr);
-            }
+            gdb_get_reg32(mem_buf, env->fpscr);
+            break;
         }
     }
-    return 0;
+
+    return r;
 }

 int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    int r = ppc_cpu_gdb_register_len(n);
+
+    if (!r) {
+        return r;
+    }

     if (n < 32) {
         /* gprs */
         env->gpr[n] = ldtul_p(mem_buf);
-        return sizeof(target_ulong);
     } else if (n < 64) {
         /* fprs */
-        if (gdb_has_xml) {
-            return 0;
-        }
         env->fpr[n-32] = ldfq_p(mem_buf);
-        return 8;
     } else {
         switch (n) {
         case 64:
             env->nip = ldtul_p(mem_buf);
-            return sizeof(target_ulong);
+            break;
         case 65:
             ppc_store_msr(env, ldtul_p(mem_buf));
-            return sizeof(target_ulong);
+            break;
         case 66:
             {
                 uint32_t cr = ldl_p(mem_buf);
@@ -107,25 +148,23 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
                 for (i = 0; i < 8; i++) {
                     env->crf[i] = (cr >> (32 - ((i + 1) * 4))) & 0xF;
                 }
-                return 4;
+                break;
             }
         case 67:
             env->lr = ldtul_p(mem_buf);
-            return sizeof(target_ulong);
+            break;
         case 68:
             env->ctr = ldtul_p(mem_buf);
-            return sizeof(target_ulong);
+            break;
         case 69:
             env->xer = ldtul_p(mem_buf);
-            return sizeof(target_ulong);
+            break;
         case 70:
             /* fpscr */
-            if (gdb_has_xml) {
-                return 0;
-            }
             store_fpscr(env, ldtul_p(mem_buf), 0xffffffff);
-            return sizeof(target_ulong);
+            break;
         }
     }
-    return 0;
+
+    return r;
 }

> 
> Anyway, sorry about the formatting issues again.  Should I just resubmit the patch as is?

Whichever way you prefer. The way it is now I can't apply it :).


Alex

[Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for reading/writing registers

[Thomas Falcon]

This patch allows registers to be properly read from and written to
when using the gdbstub to debug a ppc guest running in little
endian mode.  It accomplishes this goal by byte swapping the values of
any registers if the MSR:LE value is set.

Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
Differences from v2:

Fixed formatting issues
Added logic to ensure only FP registers have a guaranteed size of 8 bytes
---
 target-ppc/cpu-qom.h        |  2 ++
 target-ppc/gdbstub.c        | 46 +++++++++++++++++++++++++++++++++++++++++++++
 target-ppc/translate_init.c |  4 ++--
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 72b2232..992963f 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -109,7 +109,9 @@ void ppc_cpu_dump_statistics(CPUState *cpu, FILE *f,
                              fprintf_function cpu_fprintf, int flags);
 hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
+int ppc_cpu_gdb_read_register_wrap(CPUState *cpu, uint8_t *buf, int reg);
 int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+int ppc_cpu_gdb_write_register_wrap(CPUState *cpu, uint8_t *buf, int reg);
 int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
                                    CPUState *cpu, void *opaque);
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
index 1c91090..18db8a2 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -21,6 +21,52 @@
 #include "qemu-common.h"
 #include "exec/gdbstub.h"
 
+/* The following functions are used to ensure the correct
+ * transfer of registers between a little endian ppc target
+ * and a big endian host by checking the LE bit in the Machine State Register
+ */
+
+int ppc_cpu_gdb_read_register_wrap(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    int len = ppc_cpu_gdb_read_register(cs, mem_buf, n), i;
+    if (msr_le) {
+        uint8_t tmp;
+        for (i = 0; i < len/2 ; i++) {
+            tmp = *(mem_buf + i);
+            *(mem_buf + i) = *(mem_buf + len - 1 - i);
+            *(mem_buf + len - 1 - i) = tmp;
+        }
+    }
+    return len;
+}
+
+int ppc_cpu_gdb_write_register_wrap(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+   
+     if (msr_le) {
+        int len = 0, i = 0;
+        if (n > 31 && n < 64) {
+            len = 8;
+        } else if (n == 66) {
+            len = 4;
+        } else {
+            len = sizeof(target_ulong);
+        }
+        uint8_t tmp;
+        for (i = 0; i < len/2; i++) {
+            tmp = *(mem_buf + i);
+            *(mem_buf+i) = *(mem_buf + len - 1 - i);
+            *(mem_buf + len - 1 - i) = tmp;
+        }
+    }
+    return ppc_cpu_gdb_write_register(cs, mem_buf, n);
+}
+
 /* Old gdb always expects FP registers.  Newer (xml-aware) gdb only
  * expects whatever the target description contains.  Due to a
  * historical mishap the FP registers appear in between core integer
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c030a20..41ea4b7 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8655,8 +8655,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->dump_state = ppc_cpu_dump_state;
     cc->dump_statistics = ppc_cpu_dump_statistics;
     cc->set_pc = ppc_cpu_set_pc;
-    cc->gdb_read_register = ppc_cpu_gdb_read_register;
-    cc->gdb_write_register = ppc_cpu_gdb_write_register;
+    cc->gdb_read_register = ppc_cpu_gdb_read_register_wrap;
+    cc->gdb_write_register = ppc_cpu_gdb_write_register_wrap;
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_debug = ppc_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_ppc_cpu;
-- 
1.8.3.1