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

[Thomas Falcon <tlfalcon@linux.vnet.ibm.com>]

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