| Message ID | 1441143678-14295-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 01, 2015 at 11:41:18PM +0200, Thomas Huth wrote: > The size of the Problem State Priority Boost Register is only > 32 bits, so let's change the type of the corresponding variable > accordingly to avoid future trouble. Since we're already using lwz/stw in the assembly code in book3s_hv_rmhandlers.S, this is actually a bug fix, isn't it? How did you find it? Did you observe a failure of some kind, or did you just find it by code inspection? Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: > The size of the Problem State Priority Boost Register is only > 32 bits, so let's change the type of the corresponding variable > accordingly to avoid future trouble. It's not future trouble, it's broken today for LE and this should fix it BUT .... The asm accesses it using lwz/stw and C accesses it as a ulong. On LE that will mean that userspace will see the value << 32 Now "fixing" it might break migration if that field is already stored/loaded in its "broken" form. We may have to keep the "broken" behaviour and document that qemu sees a value shifted by 32. Cheers, Ben. > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > arch/powerpc/include/asm/kvm_host.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > index d91f65b..c825f3a 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -473,7 +473,7 @@ struct kvm_vcpu_arch { > ulong ciabr; > ulong cfar; > ulong ppr; > - ulong pspb; > + u32 pspb; > ulong fscr; > ulong shadow_fscr; > ulong ebbhr; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-09-02 at 08:24 +1000, Paul Mackerras wrote: > On Tue, Sep 01, 2015 at 11:41:18PM +0200, Thomas Huth wrote: > > The size of the Problem State Priority Boost Register is only > > 32 bits, so let's change the type of the corresponding variable > > accordingly to avoid future trouble. > > Since we're already using lwz/stw in the assembly code in > book3s_hv_rmhandlers.S, this is actually a bug fix, isn't it? > How did you find it? Did you observe a failure of some kind, or did > you just find it by code inspection? Won't the fix break migration ? Unless qemu doens't migrate it ... > Paul. > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 02, 2015 at 08:25:05AM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: > > The size of the Problem State Priority Boost Register is only > > 32 bits, so let's change the type of the corresponding variable > > accordingly to avoid future trouble. > > It's not future trouble, it's broken today for LE and this should fix > it BUT .... No, it's broken today for BE hosts, which will always see 0 for the PSPB register value. LE hosts are fine. > The asm accesses it using lwz/stw and C accesses it as a ulong. On LE > that will mean that userspace will see the value << 32 No, that will happen on BE, and since KVM_REG_PPC_PSPB says it's a 32-bit register, we'll just pass 0 back to userspace when it reads it. > Now "fixing" it might break migration if that field is already > stored/loaded in its "broken" form. We may have to keep the "broken" > behaviour and document that qemu sees a value shifted by 32. It will be being set to 0 on BE hosts across migration today (fortunately 0 is a benign value for PSPB). If we fix this on both the source and destination host, then the value will get migrated across correctly. I think Thomas's patch is fine, it just needs a stronger patch description saying that it fixes an actual bug. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-09-02 at 08:45 +1000, Paul Mackerras wrote: > On Wed, Sep 02, 2015 at 08:25:05AM +1000, Benjamin Herrenschmidt > wrote: > > On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: > > > The size of the Problem State Priority Boost Register is only > > > 32 bits, so let's change the type of the corresponding variable > > > accordingly to avoid future trouble. > > > > It's not future trouble, it's broken today for LE and this should > > fix > > it BUT .... > > No, it's broken today for BE hosts, which will always see 0 for the > PSPB register value. LE hosts are fine. 0 or PSPB << 32 ? > > The asm accesses it using lwz/stw and C accesses it as a ulong. On > > LE > > that will mean that userspace will see the value << 32 > > No, that will happen on BE, and since KVM_REG_PPC_PSPB says it's a > 32-bit register, we'll just pass 0 back to userspace when it reads > it. Ah ok, I missed that bit about KVM_REG_PPC_PSPB > > Now "fixing" it might break migration if that field is already > > stored/loaded in its "broken" form. We may have to keep the > > "broken" > > behaviour and document that qemu sees a value shifted by 32. > > It will be being set to 0 on BE hosts across migration today > (fortunately 0 is a benign value for PSPB). If we fix this on both > the source and destination host, then the value will get migrated > across correctly. Ok, I missed the part where KVM_REG_PPC_PSPB passed it down as a 32 -bit. That means Thomas patch should work indeed. > I think Thomas's patch is fine, it just needs a stronger patch > description saying that it fixes an actual bug. Right. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/09/15 00:24, Paul Mackerras wrote: > On Tue, Sep 01, 2015 at 11:41:18PM +0200, Thomas Huth wrote: >> The size of the Problem State Priority Boost Register is only >> 32 bits, so let's change the type of the corresponding variable >> accordingly to avoid future trouble. > > Since we're already using lwz/stw in the assembly code in > book3s_hv_rmhandlers.S, this is actually a bug fix, isn't it? > How did you find it? Did you observe a failure of some kind, or did > you just find it by code inspection? Code inspection. I was looking for similar problems like the issue with the XER register that we've hit recently (https://patchwork.ozlabs.org/patch/476872/) while I was trying to debug a similar problem. Thomas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/09/15 00:55, Benjamin Herrenschmidt wrote: > On Wed, 2015-09-02 at 08:45 +1000, Paul Mackerras wrote: >> On Wed, Sep 02, 2015 at 08:25:05AM +1000, Benjamin Herrenschmidt >> wrote: >>> On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: >>>> The size of the Problem State Priority Boost Register is only >>>> 32 bits, so let's change the type of the corresponding variable >>>> accordingly to avoid future trouble. >>> >>> It's not future trouble, it's broken today for LE and this should >>> fix >>> it BUT .... >> >> No, it's broken today for BE hosts, which will always see 0 for the >> PSPB register value. LE hosts are fine. Right ... I just meant that nobody really experienced trouble with this today yet, but the bug is already present now already of course. >>> The asm accesses it using lwz/stw and C accesses it as a ulong. On >>> LE >>> that will mean that userspace will see the value << 32 >> >> No, that will happen on BE, and since KVM_REG_PPC_PSPB says it's a >> 32-bit register, we'll just pass 0 back to userspace when it reads >> it. > > Ah ok, I missed that bit about KVM_REG_PPC_PSPB > >>> Now "fixing" it might break migration if that field is already >>> stored/loaded in its "broken" form. We may have to keep the >>> "broken" >>> behaviour and document that qemu sees a value shifted by 32. >> >> It will be being set to 0 on BE hosts across migration today >> (fortunately 0 is a benign value for PSPB). If we fix this on both >> the source and destination host, then the value will get migrated >> across correctly. > > Ok, I missed the part where KVM_REG_PPC_PSPB passed it down as a 32 > -bit. That means Thomas patch should work indeed. ... and if I get the QEMU source code right, the register is currently not migrated at all - or at least I was not able to find the spot in the source code that migrates this register. >> I think Thomas's patch is fine, it just needs a stronger patch >> description saying that it fixes an actual bug. Ok, I'll resend with a better patch description. Thomas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Am 02.09.2015 um 09:26 schrieb Thomas Huth <thuth@redhat.com>: > >> On 02/09/15 00:55, Benjamin Herrenschmidt wrote: >>> On Wed, 2015-09-02 at 08:45 +1000, Paul Mackerras wrote: >>> On Wed, Sep 02, 2015 at 08:25:05AM +1000, Benjamin Herrenschmidt >>> wrote: >>>> On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: >>>>> The size of the Problem State Priority Boost Register is only >>>>> 32 bits, so let's change the type of the corresponding variable >>>>> accordingly to avoid future trouble. >>>> >>>> It's not future trouble, it's broken today for LE and this should >>>> fix >>>> it BUT .... >>> >>> No, it's broken today for BE hosts, which will always see 0 for the >>> PSPB register value. LE hosts are fine. > > Right ... I just meant that nobody really experienced trouble with this > today yet, but the bug is already present now already of course. Sounds like a great candidate for kvm-unit-tests then, no? ;) Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/09/15 10:26, Alexander Graf wrote: > >> Am 02.09.2015 um 09:26 schrieb Thomas Huth <thuth@redhat.com>: >> >>> On 02/09/15 00:55, Benjamin Herrenschmidt wrote: >>>> On Wed, 2015-09-02 at 08:45 +1000, Paul Mackerras wrote: >>>> On Wed, Sep 02, 2015 at 08:25:05AM +1000, Benjamin Herrenschmidt >>>> wrote: >>>>> On Tue, 2015-09-01 at 23:41 +0200, Thomas Huth wrote: >>>>>> The size of the Problem State Priority Boost Register is only >>>>>> 32 bits, so let's change the type of the corresponding variable >>>>>> accordingly to avoid future trouble. >>>>> >>>>> It's not future trouble, it's broken today for LE and this should >>>>> fix >>>>> it BUT .... >>>> >>>> No, it's broken today for BE hosts, which will always see 0 for the >>>> PSPB register value. LE hosts are fine. >> >> Right ... I just meant that nobody really experienced trouble with this >> today yet, but the bug is already present now already of course. > > Sounds like a great candidate for kvm-unit-tests then, no? ;) I'm certainly looking forward to seeing powerpc support in there :-) Thomas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index d91f65b..c825f3a 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -473,7 +473,7 @@ struct kvm_vcpu_arch { ulong ciabr; ulong cfar; ulong ppr; - ulong pspb; + u32 pspb; ulong fscr; ulong shadow_fscr; ulong ebbhr;
The size of the Problem State Priority Boost Register is only 32 bits, so let's change the type of the corresponding variable accordingly to avoid future trouble. Signed-off-by: Thomas Huth <thuth@redhat.com> --- arch/powerpc/include/asm/kvm_host.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)