| Message ID | 20160115150012.17358.95155.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
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 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 > } > } > } >
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. > 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 > > } > > } > > } > > >
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.
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
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 } } }
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> --- target-ppc/kvm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)