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

[Qemu-devel] [PATCH v2] 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>
---
Have created wrapper functions that swap mem_buf in-place.
mem_buf is swapped regardless of the the host's endianness if msr_le is true.
---
  target-ppc/cpu-qom.h        |  2 ++
  target-ppc/gdbstub.c        | 48 +++++++++++++++++++++++++++++++++++++++++++++
  target-ppc/translate_init.c |  4 ++--
  3 files changed, 52 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..964fd85 100644
--- a/target-ppc/gdbstub.c
+++ b/target-ppc/gdbstub.c
@@ -21,6 +21,54 @@
  #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 < 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 v2] target-ppc: gdbstub allow byte swapping for reading/writing registers

[Alexander Graf]

On 16.01.2014, at 17:59, 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>
> ---
> Have created wrapper functions that swap mem_buf in-place.
> mem_buf is swapped regardless of the the host's endianness if msr_le is true.
> ---
> target-ppc/cpu-qom.h        |  2 ++
> target-ppc/gdbstub.c        | 48 +++++++++++++++++++++++++++++++++++++++++++++
> target-ppc/translate_init.c |  4 ++--
> 3 files changed, 52 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..964fd85 100644
> --- a/target-ppc/gdbstub.c
> +++ b/target-ppc/gdbstub.c
> @@ -21,6 +21,54 @@
> #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 < 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);

Please run checkpatch.pl :).

Also the return value is already then length. No need to duplicate that logic.


Alex

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

[Peter Maydell]

On 16 January 2014 16:59, 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>
> ---
> Have created wrapper functions that swap mem_buf in-place.
> mem_buf is swapped regardless of the the host's endianness if msr_le is
> true.
> ---

> +    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;
> +        }

Please don't roll your own byte swapping.

Also see my remarks on the previous patch series suggesting
that we should look at this in a more holistic way than
just randomly fixing small bits of things. A good place
to start would be "what should the semantics of stl_p()
be for a QEMU where the CPU is currently operating with
a reversed endianness to the TARGET_WORDS_BIGENDIAN
setting?".

thanks
-- PMM

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

[Alexander Graf]

> Am 16.01.2014 um 18:41 schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
>> On 16 January 2014 16:59, 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>
>> ---
>> Have created wrapper functions that swap mem_buf in-place.
>> mem_buf is swapped regardless of the the host's endianness if msr_le is
>> true.
>> ---
> 
>> +    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;
>> +        }
> 
> Please don't roll your own byte swapping.
> 
> Also see my remarks on the previous patch series suggesting
> that we should look at this in a more holistic way than
> just randomly fixing small bits of things. A good place
> to start would be "what should the semantics of stl_p()
> be for a QEMU where the CPU is currently operating with
> a reversed endianness to the TARGET_WORDS_BIGENDIAN
> setting?".

That'd open a giant can of worms that I'd rather not open. RTAS for example is still BE regardless of guest endianness.


Alex

> 
> thanks
> -- PMM

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

[Peter Maydell]

On 16 January 2014 17:51, Alexander Graf <agraf@suse.de> wrote:
>> Am 16.01.2014 um 18:41 schrieb Peter Maydell <peter.maydell@linaro.org>:
>> Also see my remarks on the previous patch series suggesting
>> that we should look at this in a more holistic way than
>> just randomly fixing small bits of things. A good place
>> to start would be "what should the semantics of stl_p()
>> be for a QEMU where the CPU is currently operating with
>> a reversed endianness to the TARGET_WORDS_BIGENDIAN
>> setting?".
>
> That'd open a giant can of worms that I'd rather not open.

If you don't want to open the can of worms then your
correct path is to use QEMU's existing mechanisms for
BE/LE support -- build a separate executable for the
other endianness, which will then have TARGET_WORDS_BIGENDIAN
set appropriately, and the gdb register read functions
will work correctly.

Attempting to handle either-endian guests in the same
executable is exactly opening the can of worms, and if
we're doing it we should do it with some kind of plan
of attack for killing the worms.

> RTAS for example is still BE regardless of guest endianness.

"Always BE" accesses are easy -- they should be done
with the versions of the ld/st functions which have
_be_ in them. If there are places using the plain
stl_p() functions and assuming they're bigendian, they
should be fixed.

thanks
-- PMM

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

[Thomas Falcon]

On 01/16/2014 11:10 AM, Alexander Graf wrote:
> On 16.01.2014, at 17:59, 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>
>> ---
>> Have created wrapper functions that swap mem_buf in-place.
>> mem_buf is swapped regardless of the the host's endianness if msr_le is true.
>> ---
>> target-ppc/cpu-qom.h        |  2 ++
>> target-ppc/gdbstub.c        | 48 +++++++++++++++++++++++++++++++++++++++++++++
>> target-ppc/translate_init.c |  4 ++--
>> 3 files changed, 52 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..964fd85 100644
>> --- a/target-ppc/gdbstub.c
>> +++ b/target-ppc/gdbstub.c
>> @@ -21,6 +21,54 @@
>> #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 < 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);
> Please run checkpatch.pl :).
>
> Also the return value is already then length. No need to duplicate that logic.
>
>
> Alex
>
Sorry about the formatting issues.  It slipped my mind.  But what do you mean about the return value?

Would you rather this:

ppc_cpu_gdb_write_register(cs, mem_buf, n);
return len;

Thanks for your suggestions,

Tom