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

[Alexander Graf]

On 14.01.2014, at 23:06, Thomas Falcon <address@hidden> 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 only if the MSR:LE value is set and if the host machine
> is big endian.
> 
> Signed-off-by: Thomas Falcon<address@hidden>

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

> ---
> target-ppc/gdbstub.c | 50 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
> index 1c91090..eba501a 100644
> --- a/target-ppc/gdbstub.c
> +++ b/target-ppc/gdbstub.c
> @@ -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]));

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

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