All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 21:41 ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-01 21:41 UTC (permalink / raw)
  To: kvm-ppc, Alexander Graf, Paul Mackerras; +Cc: kvm, linuxppc-dev

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

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 21:41 ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-01 21:41 UTC (permalink / raw)
  To: kvm-ppc, Alexander Graf, Paul Mackerras; +Cc: kvm, linuxppc-dev

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

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 21:41 ` Thomas Huth
@ 2015-09-01 22:24   ` Paul Mackerras
  -1 siblings, 0 replies; 21+ messages in thread
From: Paul Mackerras @ 2015-09-01 22:24 UTC (permalink / raw)
  To: Thomas Huth; +Cc: kvm-ppc, Alexander Graf, kvm, linuxppc-dev

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:24   ` Paul Mackerras
  0 siblings, 0 replies; 21+ messages in thread
From: Paul Mackerras @ 2015-09-01 22:24 UTC (permalink / raw)
  To: Thomas Huth; +Cc: kvm-ppc, Alexander Graf, kvm, linuxppc-dev

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 21:41 ` Thomas Huth
@ 2015-09-01 22:25   ` Benjamin Herrenschmidt
  -1 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:25 UTC (permalink / raw)
  To: Thomas Huth, kvm-ppc, Alexander Graf, Paul Mackerras; +Cc: linuxppc-dev, kvm

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;


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:25   ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:25 UTC (permalink / raw)
  To: Thomas Huth, kvm-ppc, Alexander Graf, Paul Mackerras; +Cc: linuxppc-dev, kvm

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;


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 22:24   ` Paul Mackerras
@ 2015-09-01 22:30     ` Benjamin Herrenschmidt
  -1 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:30 UTC (permalink / raw)
  To: Paul Mackerras, Thomas Huth; +Cc: linuxppc-dev, Alexander Graf, kvm-ppc, kvm

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:30     ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:30 UTC (permalink / raw)
  To: Paul Mackerras, Thomas Huth; +Cc: linuxppc-dev, Alexander Graf, kvm-ppc, kvm

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 22:25   ` Benjamin Herrenschmidt
  (?)
@ 2015-09-01 22:45     ` Paul Mackerras
  -1 siblings, 0 replies; 21+ messages in thread
From: Paul Mackerras @ 2015-09-01 22:45 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Thomas Huth, linuxppc-dev, Alexander Graf, kvm-ppc, kvm

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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:45     ` Paul Mackerras
  0 siblings, 0 replies; 21+ messages in thread
From: Paul Mackerras @ 2015-09-01 22:45 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Thomas Huth, kvm-ppc, Alexander Graf, linuxppc-dev, kvm

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:45     ` Paul Mackerras
  0 siblings, 0 replies; 21+ messages in thread
From: Paul Mackerras @ 2015-09-01 22:45 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Thomas Huth, linuxppc-dev, Alexander Graf, kvm-ppc, kvm

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 22:45     ` Paul Mackerras
@ 2015-09-01 22:55       ` Benjamin Herrenschmidt
  -1 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:55 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Thomas Huth, kvm-ppc, Alexander Graf, linuxppc-dev, kvm

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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-01 22:55       ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2015-09-01 22:55 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Thomas Huth, kvm-ppc, Alexander Graf, linuxppc-dev, kvm

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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 22:24   ` Paul Mackerras
@ 2015-09-02  7:16     ` Thomas Huth
  -1 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  7:16 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: kvm-ppc, Alexander Graf, kvm, linuxppc-dev

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-02  7:16     ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  7:16 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: kvm-ppc, Alexander Graf, kvm, linuxppc-dev

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-01 22:55       ` Benjamin Herrenschmidt
@ 2015-09-02  7:26         ` Thomas Huth
  -1 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  7:26 UTC (permalink / raw)
  To: benh, Paul Mackerras; +Cc: kvm-ppc, Alexander Graf, linuxppc-dev, kvm

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-02  7:26         ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  7:26 UTC (permalink / raw)
  To: benh, Paul Mackerras; +Cc: kvm-ppc, Alexander Graf, linuxppc-dev, kvm

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-02  7:26         ` Thomas Huth
@ 2015-09-02  8:26           ` Alexander Graf
  -1 siblings, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2015-09-02  8:26 UTC (permalink / raw)
  To: Thomas Huth
  Cc: benh, Paul Mackerras, kvm-ppc, Alexander Graf, linuxppc-dev, kvm



> 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-02  8:26           ` Alexander Graf
  0 siblings, 0 replies; 21+ messages in thread
From: Alexander Graf @ 2015-09-02  8:26 UTC (permalink / raw)
  To: Thomas Huth
  Cc: benh, Paul Mackerras, kvm-ppc, Alexander Graf, linuxppc-dev, kvm



> 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
  2015-09-02  8:26           ` Alexander Graf
@ 2015-09-02  8:35             ` Thomas Huth
  -1 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  8:35 UTC (permalink / raw)
  To: Alexander Graf
  Cc: benh, Paul Mackerras, kvm-ppc, Alexander Graf, linuxppc-dev, kvm,
	Andrew Jones

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] KVM: ppc: Fix size of the PSPB register
@ 2015-09-02  8:35             ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2015-09-02  8:35 UTC (permalink / raw)
  To: Alexander Graf
  Cc: benh, Paul Mackerras, kvm-ppc, Alexander Graf, linuxppc-dev, kvm,
	Andrew Jones

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-09-02  8:35 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-01 21:41 [PATCH] KVM: ppc: Fix size of the PSPB register Thomas Huth
2015-09-01 21:41 ` Thomas Huth
2015-09-01 22:24 ` Paul Mackerras
2015-09-01 22:24   ` Paul Mackerras
2015-09-01 22:30   ` Benjamin Herrenschmidt
2015-09-01 22:30     ` Benjamin Herrenschmidt
2015-09-02  7:16   ` Thomas Huth
2015-09-02  7:16     ` Thomas Huth
2015-09-01 22:25 ` Benjamin Herrenschmidt
2015-09-01 22:25   ` Benjamin Herrenschmidt
2015-09-01 22:45   ` Paul Mackerras
2015-09-01 22:45     ` Paul Mackerras
2015-09-01 22:45     ` Paul Mackerras
2015-09-01 22:55     ` Benjamin Herrenschmidt
2015-09-01 22:55       ` Benjamin Herrenschmidt
2015-09-02  7:26       ` Thomas Huth
2015-09-02  7:26         ` Thomas Huth
2015-09-02  8:26         ` Alexander Graf
2015-09-02  8:26           ` Alexander Graf
2015-09-02  8:35           ` Thomas Huth
2015-09-02  8:35             ` Thomas Huth

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.