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.

Signed-off-by: Thomas Falcon<tlfalcon@linux.vnet.ibm.com>
---
  target-ppc/gdbstub.c | 50 ++++++++++++++++++++++++++++++++------------------
  1 file changed, 32 insertions(+), 18 deletions(-)

-- 1.8.3.1

On 14 January 2014 22:06, 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 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.

>
> Signed-off-by: Thomas Falcon<tlfalcon@linux.vnet.ibm.com>
> ---
>  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

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

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

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
>

On 14 January 2014 22:40, Alexander Graf <agraf@suse.de> wrote:
> 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 in mem_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

On 14.01.2014, at 23:55, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 14 January 2014 22:40, Alexander Graf <agraf@suse.de> wrote:
>> 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 in mem_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...)

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

On 14 January 2014 23:01, Alexander Graf <agraf@suse.de> wrote:
>> 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 <agraf@suse.de> wrote on 14.01.2014 23:40:20:
> On 14.01.2014, at 23:06, 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 only if the MSR:LE value is set and if the host machine
> > is big endian.
> >
> > Signed-off-by: Thomas Falcon<tlfalcon@linux.vnet.ibm.com>
>
> 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

On 15.01.2014, at 18:29, Ulrich Weigand <Ulrich.Weigand@de.ibm.com> wrote:

> Alexander Graf <agraf@suse.de> wrote on 14.01.2014 23:40:20:
>> On 14.01.2014, at 23:06, 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 only if the MSR:LE value is set and if the host machine
>>> is big endian.
>>> 
>>> Signed-off-by: Thomas Falcon<tlfalcon@linux.vnet.ibm.com>
>> 
>> 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