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

[Greg Kurz]

On Mon, 18 Jan 2016 13:25:19 +1100
David Gibson <address@hidden> 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 <address@hidden>  
> 
> 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 <address@hidden>
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;
> >      }
> >   
>