[PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation

[PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation

[Michael Neuling]

When emulating tsr, treclaim and trechkpt, we incorrectly set CR0. The
code currently sets:
    CR0 <- 00 || MSR[TS]
but according to the ISA it should be:
    CR0 <-  0 || MSR[TS] || 0

This fixes the bit shift to put the bits in the correct location.

Tested-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kvm/book3s_hv_tm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
index 888e2609e3..31cd0f327c 100644
--- a/arch/powerpc/kvm/book3s_hv_tm.c
+++ b/arch/powerpc/kvm/book3s_hv_tm.c
@@ -131,7 +131,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
 		}
 		/* Set CR0 to indicate previous transactional state */
 		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
-			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
+			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
 		/* L=1 => tresume, L=0 => tsuspend */
 		if (instr & (1 << 21)) {
 			if (MSR_TM_SUSPENDED(msr))
@@ -175,7 +175,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
 
 		/* Set CR0 to indicate previous transactional state */
 		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
-			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
+			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
 		vcpu->arch.shregs.msr &= ~MSR_TS_MASK;
 		return RESUME_GUEST;
 
@@ -205,7 +205,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
 
 		/* Set CR0 to indicate previous transactional state */
 		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
-			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
+			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
 		vcpu->arch.shregs.msr = msr | MSR_TS_S;
 		return RESUME_GUEST;
 	}
-- 
2.21.0

Re: [PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation

[Michael Ellerman]

Michael Neuling <mikey@neuling.org> writes:
> When emulating tsr, treclaim and trechkpt, we incorrectly set CR0. The
> code currently sets:
>     CR0 <- 00 || MSR[TS]
> but according to the ISA it should be:
>     CR0 <-  0 || MSR[TS] || 0

Seems bad, what's the worst case impact?

Do we have a test case for this?

> This fixes the bit shift to put the bits in the correct location.

Fixes: ?

cheers

> diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
> index 888e2609e3..31cd0f327c 100644
> --- a/arch/powerpc/kvm/book3s_hv_tm.c
> +++ b/arch/powerpc/kvm/book3s_hv_tm.c
> @@ -131,7 +131,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
>  		}
>  		/* Set CR0 to indicate previous transactional state */
>  		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> -			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> +			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
>  		/* L=1 => tresume, L=0 => tsuspend */
>  		if (instr & (1 << 21)) {
>  			if (MSR_TM_SUSPENDED(msr))
> @@ -175,7 +175,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
>  
>  		/* Set CR0 to indicate previous transactional state */
>  		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> -			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> +			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
>  		vcpu->arch.shregs.msr &= ~MSR_TS_MASK;
>  		return RESUME_GUEST;
>  
> @@ -205,7 +205,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
>  
>  		/* Set CR0 to indicate previous transactional state */
>  		vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> -			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> +			(((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
>  		vcpu->arch.shregs.msr = msr | MSR_TS_S;
>  		return RESUME_GUEST;
>  	}
> -- 
> 2.21.0

Re: [PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation

[Michael Neuling]

On Mon, 2019-06-24 at 21:48 +1000, Michael Ellerman wrote:
> Michael Neuling <mikey@neuling.org> writes:
> > When emulating tsr, treclaim and trechkpt, we incorrectly set CR0. The
> > code currently sets:
> >     CR0 <- 00 || MSR[TS]
> > but according to the ISA it should be:
> >     CR0 <-  0 || MSR[TS] || 0
> 
> Seems bad, what's the worst case impact?

It's a data integrity issue as CR0 is corrupted.

> Do we have a test case for this?

Suraj has a KVM unit test for it.

> > This fixes the bit shift to put the bits in the correct location.
> 
> Fixes: ?

It's been around since we first wrote the code so:

Fixes: 4bb3c7a0208fc13c ("KVM: PPC: Book3S HV: Work around transactional memory bugs in POWER9")

Mikey

Re: [PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation

[Michael Ellerman]

On Thu, 2019-06-20 at 06:00:40 UTC, Michael Neuling wrote:
> When emulating tsr, treclaim and trechkpt, we incorrectly set CR0. The
> code currently sets:
>     CR0 <- 00 || MSR[TS]
> but according to the ISA it should be:
>     CR0 <-  0 || MSR[TS] || 0
> 
> This fixes the bit shift to put the bits in the correct location.
> 
> Tested-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/3fefd1cd95df04da67c83c1cb93b663f04b3324f

cheers