Message ID | 20190620060040.26945-1-mikey@neuling.org |
---|---|
State | Accepted |
Headers | show |
Series | KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation | expand |
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
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
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
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; }