[5/7] target-ppc: gdbstub: fix altivec registers for little-endian guests

Altivec registers are 128-bit wide. They are stored in memory as two
64-bit values that must be byteswapped when the guest is little-endian.
Let's reuse the ppc_maybe_bswap_register() helper for this.

We also need to fix the ordering of the 64-bit elements according to
the target endianness, for both system and user mode.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> Altivec registers are 128-bit wide. They are stored in memory as two
> 64-bit values that must be byteswapped when the guest is little-endian.
> Let's reuse the ppc_maybe_bswap_register() helper for this.
> 
> We also need to fix the ordering of the 64-bit elements according to
> the target endianness, for both system and user mode.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

What bothers me about this is that avr_need_swap() now depends on both
host and guest endianness.  However the VSCR and VRSAVE swap - like
the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
depends only on guest endianness.

Why does altivec depend on the host endianness?

> ---
>  target-ppc/translate_init.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 18e9e561561f..80d53e4dcf5a 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
>  static bool avr_need_swap(CPUPPCState *env)
>  {
>  #ifdef HOST_WORDS_BIGENDIAN
> -    return false;
> +    return msr_le;
>  #else
> -    return true;
> +    return !msr_le;
>  #endif
>  }
>  
> @@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>              stq_p(mem_buf, env->avr[n].u64[1]);
>              stq_p(mem_buf+8, env->avr[n].u64[0]);
>          }
> +        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
>          return 16;
>      }
>      if (n == 32) {
>          stl_p(mem_buf, env->vscr);
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
>      if (n == 33) {
>          stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
>      return 0;
> @@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  {
>      if (n < 32) {
> +        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
>          if (!avr_need_swap(env)) {
>              env->avr[n].u64[0] = ldq_p(mem_buf);
>              env->avr[n].u64[1] = ldq_p(mem_buf+8);
> @@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>          return 16;
>      }
>      if (n == 32) {
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          env->vscr = ldl_p(mem_buf);
>          return 4;
>      }
>      if (n == 33) {
> +        ppc_maybe_bswap_register(env, mem_buf, 4);
>          env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
>          return 4;
>      }
>

On Mon, 18 Jan 2016 13:25:19 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> > Altivec registers are 128-bit wide. They are stored in memory as two
> > 64-bit values that must be byteswapped when the guest is little-endian.
> > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > 
> > We also need to fix the ordering of the 64-bit elements according to
> > the target endianness, for both system and user mode.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> 
> What bothers me about this is that avr_need_swap() now depends on both
> host and guest endianness.  However the VSCR and VRSAVE swap - like
> the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> depends only on guest endianness.
> 
> Why does altivec depend on the host endianness?
> 

This has always been the case:

commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Sat Jan 24 15:08:09 2009 +0000

    target-ppc: Add Altivec register read/write using XML

[...]

+static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
+{
+    if (n < 32) {
+#ifdef WORDS_BIGENDIAN
+        stq_p(mem_buf, env->avr[n].u64[0]);
+        stq_p(mem_buf+8, env->avr[n].u64[1]);
+#else
+        stq_p(mem_buf, env->avr[n].u64[1]);
+        stq_p(mem_buf+8, env->avr[n].u64[0]);
+#endif
+        return 16;
+    }

My understanding is that gdb expects registers to be presented with
the target endianness but QEMU have them in host endianness.

The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
according to the target effective endianness because stq_p() always
consider both ppc64 and ppc64le to be big endian.

Here, we have a 128-bit register that we break into two 64-bit values
in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
But we also have to reorder these quad words if the host endianness
differs from the target's one. This is the purpose of avr_need_swap().

Cheers.

--
Greg

> > ---
> >  target-ppc/translate_init.c |   12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 18e9e561561f..80d53e4dcf5a 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -8754,9 +8754,9 @@ static void dump_ppc_insns (CPUPPCState *env)
> >  static bool avr_need_swap(CPUPPCState *env)
> >  {
> >  #ifdef HOST_WORDS_BIGENDIAN
> > -    return false;
> > +    return msr_le;
> >  #else
> > -    return true;
> > +    return !msr_le;
> >  #endif
> >  }
> >  
> > @@ -8800,14 +8800,18 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >              stq_p(mem_buf, env->avr[n].u64[1]);
> >              stq_p(mem_buf+8, env->avr[n].u64[0]);
> >          }
> > +        ppc_maybe_bswap_register(env, mem_buf, 8);
> > +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> >          return 16;
> >      }
> >      if (n == 32) {
> >          stl_p(mem_buf, env->vscr);
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          return 4;
> >      }
> >      if (n == 33) {
> >          stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          return 4;
> >      }
> >      return 0;
> > @@ -8816,6 +8820,8 @@ static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >  static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >  {
> >      if (n < 32) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 8);
> > +        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> >          if (!avr_need_swap(env)) {
> >              env->avr[n].u64[0] = ldq_p(mem_buf);
> >              env->avr[n].u64[1] = ldq_p(mem_buf+8);
> > @@ -8826,10 +8832,12 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >          return 16;
> >      }
> >      if (n == 32) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          env->vscr = ldl_p(mem_buf);
> >          return 4;
> >      }
> >      if (n == 33) {
> > +        ppc_maybe_bswap_register(env, mem_buf, 4);
> >          env->spr[SPR_VRSAVE] = (target_ulong)ldl_p(mem_buf);
> >          return 4;
> >      }
> >   
>

On Tue, Jan 19, 2016 at 10:59:20AM +0100, Greg Kurz wrote:
> On Mon, 18 Jan 2016 13:25:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:
> > > Altivec registers are 128-bit wide. They are stored in memory as two
> > > 64-bit values that must be byteswapped when the guest is little-endian.
> > > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > > 
> > > We also need to fix the ordering of the 64-bit elements according to
> > > the target endianness, for both system and user mode.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>  
> > 
> > What bothers me about this is that avr_need_swap() now depends on both
> > host and guest endianness.  However the VSCR and VRSAVE swap - like
> > the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> > depends only on guest endianness.
> > 
> > Why does altivec depend on the host endianness?
> > 
> 
> This has always been the case:
> 
> commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
> Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Sat Jan 24 15:08:09 2009 +0000
> 
>     target-ppc: Add Altivec register read/write using XML
> 
> [...]
> 
> +static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
> +{
> +    if (n < 32) {
> +#ifdef WORDS_BIGENDIAN
> +        stq_p(mem_buf, env->avr[n].u64[0]);
> +        stq_p(mem_buf+8, env->avr[n].u64[1]);
> +#else
> +        stq_p(mem_buf, env->avr[n].u64[1]);
> +        stq_p(mem_buf+8, env->avr[n].u64[0]);
> +#endif
> +        return 16;
> +    }
> 
> My understanding is that gdb expects registers to be presented with
> the target endianness but QEMU have them in host endianness.
> 
> The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
> according to the target effective endianness because stq_p() always
> consider both ppc64 and ppc64le to be big endian.
> 
> Here, we have a 128-bit register that we break into two 64-bit values
> in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
> But we also have to reorder these quad words if the host endianness
> differs from the target's one. This is the purpose of avr_need_swap().

Ok, understood.  I've merged the series to ppc-for-2.6

On Wed, 20 Jan 2016 13:13:57 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jan 19, 2016 at 10:59:20AM +0100, Greg Kurz wrote:
> > On Mon, 18 Jan 2016 13:25:19 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jan 15, 2016 at 04:00:38PM +0100, Greg Kurz wrote:  
> > > > Altivec registers are 128-bit wide. They are stored in memory as two
> > > > 64-bit values that must be byteswapped when the guest is little-endian.
> > > > Let's reuse the ppc_maybe_bswap_register() helper for this.
> > > > 
> > > > We also need to fix the ordering of the 64-bit elements according to
> > > > the target endianness, for both system and user mode.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>    
> > > 
> > > What bothers me about this is that avr_need_swap() now depends on both
> > > host and guest endianness.  However the VSCR and VRSAVE swap - like
> > > the swaps for GPRs and FPRs - uses ppc_maybe_bswap_register() which
> > > depends only on guest endianness.
> > > 
> > > Why does altivec depend on the host endianness?
> > >   
> > 
> > This has always been the case:
> > 
> > commit b4f8d821e5211bbb51a278ba0fc4a4db2d581221
> > Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> > Date:   Sat Jan 24 15:08:09 2009 +0000
> > 
> >     target-ppc: Add Altivec register read/write using XML
> > 
> > [...]
> > 
> > +static int gdb_get_avr_reg(CPUState *env, uint8_t *mem_buf, int n)
> > +{
> > +    if (n < 32) {
> > +#ifdef WORDS_BIGENDIAN
> > +        stq_p(mem_buf, env->avr[n].u64[0]);
> > +        stq_p(mem_buf+8, env->avr[n].u64[1]);
> > +#else
> > +        stq_p(mem_buf, env->avr[n].u64[1]);
> > +        stq_p(mem_buf+8, env->avr[n].u64[0]);
> > +#endif
> > +        return 16;
> > +    }
> > 
> > My understanding is that gdb expects registers to be presented with
> > the target endianness but QEMU have them in host endianness.
> > 
> > The ppc_maybe_bswap_register() helper is needed to fix 64-bit values
> > according to the target effective endianness because stq_p() always
> > consider both ppc64 and ppc64le to be big endian.
> > 
> > Here, we have a 128-bit register that we break into two 64-bit values
> > in memory. Each quad word has to be fixed by ppc_maybe_bswap_register().
> > But we also have to reorder these quad words if the host endianness
> > differs from the target's one. This is the purpose of avr_need_swap().  
> 
> Ok, understood.  I've merged the series to ppc-for-2.6
> 

Cool ! Thanks.

--
Greg