Re: [Qemu-devel] [PATCH 1/7] target-ppc: kvm: fix floating point registers sync on little-endian hosts

[David Gibson]

On Fri, Jan 15, 2016 at 04:00:12PM +0100, Greg Kurz wrote:
> On VSX capable CPUs, the 32 FP registers are mapped to the high-bits
> of the 32 first VSX registers. So if you have:
> 
> VSR31 = (uint128) 0x0102030405060708090a0b0c0d0e0f00
> 
> then
> 
> FPR31 = (uint64) 0x0102030405060708
> 
> The kernel stores the VSX registers in the fp_state struct following the
> host endian element ordering.
> 
> On big-endian:
> 
> fp_state.fpr[31][0] = 0x0102030405060708
> fp_state.fpr[31][1] = 0x090a0b0c0d0e0f00
> 
> On little-endian:
> 
> fp_state.fpr[31][0] = 0x090a0b0c0d0e0f00
> fp_state.fpr[31][1] = 0x0102030405060708
> 
> The KVM_GET_ONE_REG and KVM_SET_ONE_REG ioctls preserve this ordering, but
> QEMU considers it as big-endian and always copies element [0] to the
> fpr[] array and element [1] to the vsr[] array. This does not work with
> little-endian hosts, and you will get:
> 
> (qemu) p $f31
> 0x90a0b0c0d0e0f00
> 
> instead of:
> 
> (qemu) p $f31
> 0x102030405060708
> 
> This patch fixes the element ordering for little-endian hosts.
> 
> Signed-off-by: Greg Kurz <address@hidden>

If I'm understanding correctly, the only reason this bug didn't affect
things other than the gdbstub is because the get and put routines had
mirrored bugs.  So although qemu ended up with definitely wrong
information in its internal state, it reshuffled it to be right on
setting it back into KVM.

Is that correct?

> ---
>  target-ppc/kvm.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9940a9046220..45249990bda1 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -650,8 +650,13 @@ static int kvm_put_fp(CPUState *cs)
>          for (i = 0; i < 32; i++) {
>              uint64_t vsr[2];
>  
> +#ifdef HOST_WORDS_BIGENDIAN
>              vsr[0] = float64_val(env->fpr[i]);
>              vsr[1] = env->vsr[i];
> +#else
> +            vsr[0] = env->vsr[i];
> +            vsr[1] = float64_val(env->fpr[i]);
> +#endif
>              reg.addr = (uintptr_t) &vsr;
>              reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i);
>  
> @@ -721,10 +726,17 @@ static int kvm_get_fp(CPUState *cs)
>                          vsx ? "VSR" : "FPR", i, strerror(errno));
>                  return ret;
>              } else {
> +#ifdef HOST_WORDS_BIGENDIAN
>                  env->fpr[i] = vsr[0];
>                  if (vsx) {
>                      env->vsr[i] = vsr[1];
>                  }
> +#else
> +                env->fpr[i] = vsr[1];
> +                if (vsx) {
> +                    env->vsr[i] = vsr[0];
> +                }
> +#endif
>              }
>          }
>      }
> 

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