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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Tue, 19 Jan 2016 11:55:10 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jan 18, 2016 at 09:51:56AM +0100, Greg Kurz wrote:
> > On Mon, 18 Jan 2016 13:16:44 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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 <gkurz@linux.vnet.ibm.com>    
> > > 
> > > 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.  
> 
> Yes, sorry, I didn't express that well.  My point is that the only
> reason things aren't going horribly wrong is that qemu is only ever
> touching the FP/VSX values for debug, and the get/put into KVM is

I fully agree with that QEMU not touching FP/VSX is a key point for
not breaking anything.

> wrong in such a way that the right values go back again as long as
> qemu doesn't try to change them.
> 

I suppose so but I must confess I did not invest time to understand how
this KVM bug did not break the guest in some way...

> > > 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.  
> 
> Ok.
> 
> Still waiting for a reply for my query on 5/7, then I'm happy to apply
> these.
> 

Yeah sorry for the delay... I had written a reply but I wasn't happy with
my poor English *again* so I spent some more time rewording. I've answered
at last ! :)

Thanks !

--
Greg