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

[Greg Kurz]

On Mon, 18 Jan 2016 13:16:44 +1100
David Gibson <address@hidden> wrote:

> 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

Well it is not only gdbstub actually... as showed in the changelog, it also
affects the QEMU monitor which outputs wrong values since it calls kvm_get_fpu()
as well.

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

My guess is that the bug only affects gdbstub and ppc_cpu_dump_state(), because
these are the only cases where QEMU parses the state of FP registers... this
is indeed confirmed by the KVM bug you are referring to, that had no visible
effect for more than a year BTW.

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