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

[David Gibson]

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?

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature