Hi Paul,
On Thu, Jun 07, 2018 at 06:06:21PM +1000, Paul Mackerras wrote:
> The treclaim. emulation needs to record failure status in the TEXASR
> register if the transaction had not previously failed. However, the
> current code first does kvmppc_save_tm_pr() (which does a treclaim.
> itself) and then checks the failure summary bit in TEXASR after that.
> Since treclaim. itself causes transaction failure, the FS bit is
> always set, so we were never updating TEXASR with the failure cause
> supplied by the guest as the RA parameter to the treclaim. instruction.
> This caused the tm-unavailable test in tools/testing/selftests/powerpc/tm
> to fail.
>
> To fix this, we need to read TEXASR before calling kvmppc_save_tm_pr(),
> and base the final value of TEXASR on that value.
>
> Fixes: 03c81682a90b ("KVM: PPC: Book3S PR: Add emulation for treclaim.")
> Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx>
> ---
> This patch is against my kvm-ppc-next branch.
> arch/powerpc/kvm/book3s_emulate.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
> index fdbc695038dc..05cac5ea79c5 100644
> --- a/arch/powerpc/kvm/book3s_emulate.c
> +++ b/arch/powerpc/kvm/book3s_emulate.c
> @@ -138,6 +138,7 @@ static void kvmppc_emulate_treclaim(struct kvm_vcpu *vcpu, int ra_val)
> {
> unsigned long guest_msr = kvmppc_get_msr(vcpu);
> int fc_val = ra_val ? ra_val : 1;
> + uint64_t texasr;
>
> /* CR0 = 0 | MSR[TS] | 0 */
> vcpu->arch.cr = (vcpu->arch.cr & ~(CR0_MASK << CR0_SHIFT)) |
> @@ -145,25 +146,26 @@ static void kvmppc_emulate_treclaim(struct kvm_vcpu *vcpu, int ra_val)
> << CR0_SHIFT);
>
> preempt_disable();
> + tm_enable();
> + texasr = mfspr(SPRN_TEXASR);
> kvmppc_save_tm_pr(vcpu);
> kvmppc_copyfrom_vcpu_tm(vcpu);
>
> - tm_enable();
> - vcpu->arch.texasr = mfspr(SPRN_TEXASR);
> /* failure recording depends on Failure Summary bit */
> - if (!(vcpu->arch.texasr & TEXASR_FS)) {
> - vcpu->arch.texasr &= ~TEXASR_FC;
> - vcpu->arch.texasr |= ((u64)fc_val << TEXASR_FC_LG);
> + if (!(texasr & TEXASR_FS)) {
> + texasr &= ~TEXASR_FC;
> + texasr |= ((u64)fc_val << TEXASR_FC_LG) | TEXASR_FS;
>
> - vcpu->arch.texasr &= ~(TEXASR_PR | TEXASR_HV);
> + texasr &= ~(TEXASR_PR | TEXASR_HV);
> if (kvmppc_get_msr(vcpu) & MSR_PR)
> - vcpu->arch.texasr |= TEXASR_PR;
> + texasr |= TEXASR_PR;
>
> if (kvmppc_get_msr(vcpu) & MSR_HV)
> - vcpu->arch.texasr |= TEXASR_HV;
> + texasr |= TEXASR_HV;
>
> + vcpu->arch.texasr = texasr;
> vcpu->arch.tfiar = kvmppc_get_pc(vcpu);
> - mtspr(SPRN_TEXASR, vcpu->arch.texasr);
> + mtspr(SPRN_TEXASR, texasr);
> mtspr(SPRN_TFIAR, vcpu->arch.tfiar);
> }
> tm_disable();
> --
> 2.17.1
>
Thanks for fix this.
Reviewed-by: Simon Guo <wei.guo.simon@xxxxxxxxx>
I think the similar issue is in tabort. emulation and can be fixed by
following patch.
One problem here is that MSR will be still in suspend state after tabort.
and we cannot mtspr the recording result (TEXASR/TFIAR) in vcpu to TM
registers at that time (or TM Bad Thing Exception will be fired),
which need to be considered more.
If it is OK, please feel free to fold following change, or please let me
know to send another patch mail.
-------
>From 9ad747289d417537c87cbca3d371797f2b6f5dcd Mon Sep 17 00:00:00 2001
From: Simon Guo <wei.guo.simon@xxxxxxxxx>
Date: Fri, 8 Jun 2018 01:40:03 -0400
Subject: [PATCH] KVM: PPC: Book3S PR: Fix failure status setting in tabort.
emulation
tabort. will perform transaction failure recording and the recording
depends on TEXASR FS bit. Currently the TEXASR FS bit is retrieved
after tabort., when the TEXASR FS bit is already been updated by
tabort. itself.
This patch corrects this behavior by retrieving TEXASR val before
tabort.
tabort. will not immediately leads to transaction failure handling
in suspend state. So this patch also remove the mtspr on TEXASR/TFIAR
registers to avoid TM bad thing exception.
Signed-off-by: Simon Guo <wei.guo.simon@xxxxxxxxx>
---
arch/powerpc/kvm/book3s_emulate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index fdbc695038dc..24de660e3826 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -210,9 +210,11 @@ void kvmppc_emulate_tabort(struct kvm_vcpu *vcpu, int ra_val)
* present.
*/
unsigned long guest_msr = kvmppc_get_msr(vcpu);
+ uint64_t org_texasr;
preempt_disable();
tm_enable();
+ org_texasr = mfspr(SPRN_TEXASR);
tm_abort(ra_val);
/* CR0 = 0 | MSR[TS] | 0 */
@@ -225,7 +227,7 @@ void kvmppc_emulate_tabort(struct kvm_vcpu *vcpu, int ra_val)
* and tabort will be treated as nops in non-transactional
* state.
*/
- if (!(vcpu->arch.texasr & TEXASR_FS) &&
+ if (!(org_texasr & TEXASR_FS) &&
MSR_TM_ACTIVE(guest_msr)) {
vcpu->arch.texasr &= ~(TEXASR_PR | TEXASR_HV);
if (guest_msr & MSR_PR)
@@ -235,8 +237,6 @@ void kvmppc_emulate_tabort(struct kvm_vcpu *vcpu, int ra_val)
vcpu->arch.texasr |= TEXASR_HV;
vcpu->arch.tfiar = kvmppc_get_pc(vcpu);
- mtspr(SPRN_TEXASR, vcpu->arch.texasr);
- mtspr(SPRN_TFIAR, vcpu->arch.tfiar);
}
tm_disable();
preempt_enable();
--
2.14.1
-----
Thanks,
- Simon
