gdbstub: allow byte swapping for reading/writing registers
PATCH 1/1 gdbstub: allow byte swapping for reading/writing registers
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 only if the MSR:LE value is set and if the host machine is big endian.
--- target-ppc/gdbstub.c | 50 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 18 deletions(-)
target-ppc/gdbstub.c +32 -18
@@ -21,6 +21,19 @@#include "qemu-common.h" #include "exec/gdbstub.h" +/* The following macros 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 +*/ + +#define end_swap64 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap64(x) : x +#define end_swap32 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap32(x) : x +#if TARGET_LONG_BITS ==64 +#define end_swapl (x) end_swap64(x) +#else +#define end_swapl (x) end_swap32(x) +#endif +/* 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
@@ -35,20 +48,20 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)if (n <32 ) {/* gprs */ -return gdb_get_regl (mem_buf, env->gpr[n]); +return gdb_get_regl (mem_buf,end_swapl (env->gpr[n])); }else if (n <64 ) {/* fprs */ if (gdb_has_xml) {return 0 ; } -stfq_p (mem_buf, env->fpr[n-32 ]); +stfq_p (mem_buf,end_swapl (env->fpr[n-32 ]));return 8 ; }else {switch (n) {case 64 : -return gdb_get_regl (mem_buf, env->nip); +return gdb_get_regl (mem_buf,end_swapl (env->nip));case 65 : -return gdb_get_regl (mem_buf, env->msr); +return gdb_get_regl (mem_buf,end_swapl (env->msr));case 66 : {uint32_t cr =0 ;
@@ -56,20 +69,20 @@ 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); +return gdb_get_reg32 (mem_buf,end_swap32 (cr)); }case 67 : -return gdb_get_regl (mem_buf, env->lr); +return gdb_get_regl (mem_buf,end_swapl (env->lr));case 68 : -return gdb_get_regl (mem_buf, env->ctr); +return gdb_get_regl (mem_buf,end_swapl (env->ctr));case 69 : -return gdb_get_regl (mem_buf, env->xer); +return gdb_get_regl (mem_buf,end_swapl (env->xer));case 70 : {if (gdb_has_xml) {return 0 ; } -return gdb_get_reg32 (mem_buf, env->fpscr); +return gdb_get_reg32 (mem_buf,end_swap32 (env->fpscr)); } } }
@@ -83,47 +96,48 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)if (n <32 ) {/* gprs */ - env->gpr[n] =ldtul_p (mem_buf); + env->gpr[n] =end_swapl (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); + env->fpr[n-32 ] =end_swapl (ldfq_p (mem_buf));return 8 ; }else {switch (n) {case 64 : - env->nip =ldtul_p (mem_buf); + env->nip =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 65 : -ppc_store_msr (env,ldtul_p (mem_buf)); +ppc_store_msr (env,end_swapl (ldtul_p (mem_buf)));return sizeof (target_ulong);case 66 : {uint32_t cr =ldl_p (mem_buf);int i;for (i =0 ; i <8 ; i++) { - env->crf[i] = (cr >> (32 - ((i +1 ) *4 ))) &0xF ; + env->crf[i] =end_swap32 ((cr >> (32 - + ((i +1 ) *4 ))) &0xF ); }return 4 ; }case 67 : - env->lr =ldtul_p (mem_buf); + env->lr =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 68 : - env->ctr =ldtul_p (mem_buf); + env->ctr =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 69 : - env->xer =ldtul_p (mem_buf); + env->xer =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 70 :/* fpscr */ if (gdb_has_xml) {return 0 ; } -store_fpscr (env,ldtul_p (mem_buf),0xffffffff ); +store_fpscr (env,end_swapl (ldtul_p (mem_buf)),0xffffffff );return sizeof (target_ulong); } }
-- 1.8.3.1
Quoted 5 lines · 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 only if the MSR:LE value is set and if the host machine is big endian.
Since this patch only affects ppc targets it would be helpful if the subject line had an appropriate prefix indicating that.
Quoted 20 lines
Signed-off-by Thomas Falcon<[email protected]>--- target-ppc/gdbstub.c | 50 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 18 deletions(-)target-ppc/gdbstub.c +3
-0@@ -21,6 +21,19 @@#include "qemu-common.h" #include "exec/gdbstub.h" +/* The following macros 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
+ */
target-ppc/gdbstub.c line 30
Quoted 3 lines
target-ppc/gdbstub.c +3
-0+ +#define end_swap64 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap64(x) : x +#define end_swap32 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap32(x) : x
Surely we need to swap if the host is little endian and the target is bigendian, as well as if the host is bigendian and the target little endian?
Also, it seems a bit dubious to switch the endianness of words based on a runtime flag in the guest CPU -- I'm pretty sure a connected gdb won't be able to cope with that. On the other hand, gdb's pretty bad at dealing with the kind of thing real CPUs can do with switching endianness or word size at run time, so maybe this is just better than the alternatives...
thanks
-- PMM
Quoted 7 lines · 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 only if the MSR:LE value is set and if the host machine is big endian.
Signed-off-by Thomas Falcon<[email protected]>
Uli, I thought ppc64le gdb wasn't finalized yet? What does the gdbstub layout look like? Are all fields the same as ppc64(be) but simply byte swapped - including FPR ones?
Alex
Quoted 29 lines
---
target-ppc/gdbstub.c| 50 ++++++++++++++++++++++++++++++++------------------1 file changed, 32 insertions(+), 18 deletions(-)target-ppc/gdbstub.c +13
-0@@ -21,6 +21,19 @@#include "qemu-common.h" #include "exec/gdbstub.h" +/* The following macros 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 +*/ + +#define end_swap64 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap64(x) : x +#define end_swap32 (x) (msr_le && HOST_WORDS_BIGENDIAN) ? bswap32(x) : x +#if TARGET_LONG_BITS ==64 +#define end_swapl (x) end_swap64(x) +#else +#define end_swapl (x) end_swap32(x) +#endif +/* 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
target-ppc/gdbstub.c int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) · line 51
Quoted 5 lines
target-ppc/gdbstub.c +1
-1@@ -35,20 +48,20 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)if (n <32 ) {/* gprs */ -return gdb_get_regl (mem_buf, env->gpr[n]); +return gdb_get_regl (mem_buf,end_swapl (env->gpr[n]));
This is quite invasive (and prone to get wrong). If we really just have to swap every single register by its size (which we yet have to confirm with Uli) why don't we just wrap this function by another one that takes the return value of ppc_cpu_gdb_read_register (the integer size) and swaps it in-place in mem_buf? At least we're 100% consistent that way.
Unless of course we only need to swap half of the registers, then it makes more sense the way you do it now.
Alex
Quoted 105 lines
target-ppc/gdbstub.c +18
-18}else if (n <64 ) {/* fprs */ if (gdb_has_xml) {return 0 ; } -stfq_p (mem_buf, env->fpr[n-32 ]); +stfq_p (mem_buf,end_swapl (env->fpr[n-32 ]));return 8 ; }else {switch (n) {case 64 : -return gdb_get_regl (mem_buf, env->nip); +return gdb_get_regl (mem_buf,end_swapl (env->nip));case 65 : -return gdb_get_regl (mem_buf, env->msr); +return gdb_get_regl (mem_buf,end_swapl (env->msr));case 66 : {uint32_t cr =0 ;@@ -56,20 +69,20 @@ 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); +return gdb_get_reg32 (mem_buf,end_swap32 (cr)); }case 67 : -return gdb_get_regl (mem_buf, env->lr); +return gdb_get_regl (mem_buf,end_swapl (env->lr));case 68 : -return gdb_get_regl (mem_buf, env->ctr); +return gdb_get_regl (mem_buf,end_swapl (env->ctr));case 69 : -return gdb_get_regl (mem_buf, env->xer); +return gdb_get_regl (mem_buf,end_swapl (env->xer));case 70 : {if (gdb_has_xml) {return 0 ; } -return gdb_get_reg32 (mem_buf, env->fpscr); +return gdb_get_reg32 (mem_buf,end_swap32 (env->fpscr)); } } }@@ -83,47 +96,48 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)if (n <32 ) {/* gprs */ - env->gpr[n] =ldtul_p (mem_buf); + env->gpr[n] =end_swapl (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); + env->fpr[n-32 ] =end_swapl (ldfq_p (mem_buf));return 8 ; }else {switch (n) {case 64 : - env->nip =ldtul_p (mem_buf); + env->nip =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 65 : -ppc_store_msr (env,ldtul_p (mem_buf)); +ppc_store_msr (env,end_swapl (ldtul_p (mem_buf)));return sizeof (target_ulong);case 66 : {uint32_t cr =ldl_p (mem_buf);int i;for (i =0 ; i <8 ; i++) { - env->crf[i] = (cr >> (32 - ((i +1 ) *4 ))) &0xF ; + env->crf[i] =end_swap32 ((cr >> (32 - + ((i +1 ) *4 ))) &0xF ); }return 4 ; }case 67 : - env->lr =ldtul_p (mem_buf); + env->lr =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 68 : - env->ctr =ldtul_p (mem_buf); + env->ctr =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 69 : - env->xer =ldtul_p (mem_buf); + env->xer =end_swapl (ldtul_p (mem_buf));return sizeof (target_ulong);case 70 :/* fpscr */ if (gdb_has_xml) {return 0 ; } -store_fpscr (env,ldtul_p (mem_buf),0xffffffff ); +store_fpscr (env,end_swapl (ldtul_p (mem_buf)),0xffffffff );return sizeof (target_ulong); } } -- 1.8.3.1
Quoted 9 lines · Alexander Graf
Uli, I thought ppc64le gdb wasn't finalized yet? What does the gdbstub layout look like? Are all fields the same as ppc64(be) but simply byte swapped - including FPR ones?
This is quite invasive (and prone to get wrong). If we really just have to swap every single register by its size (which we yet have to confirm with Uli) why don't we just wrap this function by another one that takes the return value of
ppc_cpu_gdb_read_register(the integer size) and swaps it in-place inmem_buf? At least we're 100% consistent that way.
Note that we already support "fields in the buffer are in target byte order" (ie matching TARGET_WORDS_BIGENDIAN) with gdb_get_reg*,
"fields are always LE" (use st*_le_p()) and "fields are always BE" (use st*_be_p()).
Is the underlying issue here that we might have a CPU which is in littleendian mode but in a QEMU executable compiled with TARGET_WORDS_BIGENDIAN ? (If so I can't help feeling that the gdb stub is only the tip of the iceberg for things that might need attention...)
thanks
-- PMM
Quoted 16 lines · Peter Maydell
Quoted 9 lines · Alexander Graf
Uli, I thought ppc64le gdb wasn't finalized yet? What does the gdbstub layout look like? Are all fields the same as ppc64(be) but simply byte swapped - including FPR ones?
This is quite invasive (and prone to get wrong). If we really just have to swap every single register by its size (which we yet have to confirm with Uli) why don't we just wrap this function by another one that takes the return value of
ppc_cpu_gdb_read_register(the integer size) and swaps it in-place inmem_buf? At least we're 100% consistent that way.Note that we already support "fields in the buffer are in target byte order" (ie matching
TARGET_WORDS_BIGENDIAN) with gdb_get_reg*,
"fields are always LE" (use st*_le_p()) and "fields are always BE" (use st*_be_p()).
Quoted 5 lines
Is the underlying issue here that we might have a CPU which is in littleendian mode but in a QEMU executable compiled with
TARGET_WORDS_BIGENDIAN? (If so I can't help feeling that the gdb stub is only the tip of the iceberg for things that might need attention...)
Yes, which is going to be the same problem you have for AArch64 :). LE vs BE is really just a register flip. The qemu binary is the same for both when you run system emulation mode.
Alex
Quoted 9 lines · Alexander Graf
Quoted 5 lines
Is the underlying issue here that we might have a CPU which is in littleendian mode but in a QEMU executable compiled with
TARGET_WORDS_BIGENDIAN? (If so I can't help feeling that the gdb stub is only the tip of the iceberg for things that might need attention...)Yes, which is going to be the same problem you have for AArch64 :). LE vs BE is really just a register flip. The qemu binary is the same for both when you run system emulation mode.
Right, then I think we really need to look at the issue in a more holistic fashion. You're essentially trying to make TARGET_WORDS_BIGENDIAN irrelevant, which is a goal I thoroughly approve of, but it's pretty thoroughly baked into various parts of the codebase and I don't think it's going to be easy to eradicate.
thanks
-- PMM
Alexander Graf <[email protected]> wrote on 14.01.2014 23:40:20:
Quoted 1 line
On 14.01.2014, at 23:06, Thomas Falcon <[email protected]>
Quoted 7 lines · wrote:
Quoted 7 lines
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 only if the MSR:LE value is set and if the host machine is big endian.
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Falcon<[email protected]>Uli, I thought ppc64le gdb wasn't finalized yet? What does the gdbstub layout look like? Are all fields the same as ppc64(be) but simply byte swapped - including FPR ones?
Hi Alex, yes, the layout of the gdb protocol packets should be the same as on BE, except that all registers are byte-swapped. This includes FPRs and VPRs.
Bye,
Ulrich
Quoted 12 lines · Ulrich Weigand
Alexander Graf <[email protected]> wrote on 14.01.2014 23:40:20:
Quoted 1 line
On 14.01.2014, at 23:06, Thomas Falcon <[email protected]>
Quoted 9 lines · wrote:
Quoted 8 lines
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 only if the MSR:LE value is set and if the host machine is big endian.
Signed-off-by Thomas Falcon<[email protected]>
Quoted 7 lines
Quoted 3 lines
Uli, I thought ppc64le gdb wasn't finalized yet? What does the gdbstub layout look like? Are all fields the same as ppc64(be) but simply byte swapped - including FPR ones?
Hi Alex, yes, the layout of the gdb protocol packets should be the same as on BE, except that all registers are byte-swapped. This includes FPRs and VPRs.
Phew - I don't think we have byte swapping primitives above 64bit :).
But good, then let's abstract this whole swappiness one level higher as indicated in my previous email. That way we're guaranteed to not miss any fields by accident.
Alex