| Message ID | 52D80FFF.8070407@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
> 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
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
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
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;
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(-) -- 1.8.3.1