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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2877 bytes --]

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
wrong in such a way that the right values go back again as long as
qemu doesn't try to change them.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]