[18/26] KVM: PPC: Book3S PR: make mtspr/mfspr emulation behavior based on active TM SPRs

Message ID 1515665499-31710-19-git-send-email-wei.guo.simon@gmail.com
State Changes Requested
Headers show
Series
  • [01/26] KVM: PPC: Book3S PR: Move kvmppc_save_tm/kvmppc_restore_tm to separate file
Related show

Commit Message

Simon Guo Jan. 11, 2018, 10:11 a.m.
From: Simon Guo <wei.guo.simon@gmail.com>

The mfspr/mtspr on TM SPRs(TEXASR/TFIAR/TFHAR) are non-privileged
instructions and can be executed at PR KVM guest without trapping
into host in problem state. We only emulate mtspr/mfspr
texasr/tfiar/tfhar at guest PR=0 state.

When we are emulating mtspr tm sprs at guest PR=0 state, the emulation
result need to be visible to guest PR=1 state. That is, the actual TM
SPR val should be loaded into actual registers.

We already flush TM SPRs into vcpu when switching out of CPU, and load
TM SPRs when switching back.

This patch corrects mfspr()/mtspr() emulation for TM SPRs to make the
actual source/dest based on actual TM SPRs.

Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 arch/powerpc/kvm/book3s_emulate.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

Comments

Paul Mackerras Jan. 23, 2018, 8:17 a.m. | #1
On Thu, Jan 11, 2018 at 06:11:31PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
> 
> The mfspr/mtspr on TM SPRs(TEXASR/TFIAR/TFHAR) are non-privileged
> instructions and can be executed at PR KVM guest without trapping
> into host in problem state. We only emulate mtspr/mfspr
> texasr/tfiar/tfhar at guest PR=0 state.
> 
> When we are emulating mtspr tm sprs at guest PR=0 state, the emulation
> result need to be visible to guest PR=1 state. That is, the actual TM
> SPR val should be loaded into actual registers.
> 
> We already flush TM SPRs into vcpu when switching out of CPU, and load
> TM SPRs when switching back.
> 
> This patch corrects mfspr()/mtspr() emulation for TM SPRs to make the
> actual source/dest based on actual TM SPRs.
> 
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> ---
>  arch/powerpc/kvm/book3s_emulate.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
> index e096d01..c2836330 100644
> --- a/arch/powerpc/kvm/book3s_emulate.c
> +++ b/arch/powerpc/kvm/book3s_emulate.c
> @@ -521,13 +521,26 @@ int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
>  		break;
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	case SPRN_TFHAR:
> -		vcpu->arch.tfhar = spr_val;
> -		break;
>  	case SPRN_TEXASR:
> -		vcpu->arch.texasr = spr_val;
> -		break;
>  	case SPRN_TFIAR:
> -		vcpu->arch.tfiar = spr_val;
> +		if (MSR_TM_ACTIVE(kvmppc_get_msr(vcpu))) {
> +			/* it is illegal to mtspr() TM regs in
> +			 * other than non-transactional state.
> +			 */
> +			kvmppc_core_queue_program(vcpu, SRR1_PROGTM);
> +			emulated = EMULATE_AGAIN;
> +			break;
> +		}

We also need to check that the guest has TM enabled in the guest MSR,
and give them a facility unavailable interrupt if not.

> +
> +		tm_enable();
> +		if (sprn == SPRN_TFHAR)
> +			mtspr(SPRN_TFHAR, spr_val);
> +		else if (sprn == SPRN_TEXASR)
> +			mtspr(SPRN_TEXASR, spr_val);
> +		else
> +			mtspr(SPRN_TFIAR, spr_val);
> +		tm_disable();

I haven't seen any checks that we are on a CPU that has TM.  What
happens if a guest does a mtmsrd with TM=1 and then a mtspr to TEXASR
when running on a POWER7 (assuming the host kernel was compiled with
CONFIG_PPC_TRANSACTIONAL_MEM=y)?

Ideally, if the host CPU does not have TM functionality, these mtsprs
would be treated as no-ops and attempts to set the TM or TS fields in
the guest MSR would be ignored.

> +
>  		break;
>  #endif
>  #endif
> @@ -674,13 +687,19 @@ int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val
>  		break;
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	case SPRN_TFHAR:
> -		*spr_val = vcpu->arch.tfhar;
> +		tm_enable();
> +		*spr_val = mfspr(SPRN_TFHAR);
> +		tm_disable();
>  		break;
>  	case SPRN_TEXASR:
> -		*spr_val = vcpu->arch.texasr;
> +		tm_enable();
> +		*spr_val = mfspr(SPRN_TEXASR);
> +		tm_disable();
>  		break;
>  	case SPRN_TFIAR:
> -		*spr_val = vcpu->arch.tfiar;
> +		tm_enable();
> +		*spr_val = mfspr(SPRN_TFIAR);
> +		tm_disable();
>  		break;

These need to check MSR_TM in the guest MSR, and become no-ops on
machines without TM capability.

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Guo Jan. 30, 2018, 3:02 a.m. | #2
Hi Paul,
On Tue, Jan 23, 2018 at 07:17:45PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:31PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> > 
> > The mfspr/mtspr on TM SPRs(TEXASR/TFIAR/TFHAR) are non-privileged
> > instructions and can be executed at PR KVM guest without trapping
> > into host in problem state. We only emulate mtspr/mfspr
> > texasr/tfiar/tfhar at guest PR=0 state.
> > 
> > When we are emulating mtspr tm sprs at guest PR=0 state, the emulation
> > result need to be visible to guest PR=1 state. That is, the actual TM
> > SPR val should be loaded into actual registers.
> > 
> > We already flush TM SPRs into vcpu when switching out of CPU, and load
> > TM SPRs when switching back.
> > 
> > This patch corrects mfspr()/mtspr() emulation for TM SPRs to make the
> > actual source/dest based on actual TM SPRs.
> > 
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> > ---
> >  arch/powerpc/kvm/book3s_emulate.c | 35 +++++++++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
> > index e096d01..c2836330 100644
> > --- a/arch/powerpc/kvm/book3s_emulate.c
> > +++ b/arch/powerpc/kvm/book3s_emulate.c
> > @@ -521,13 +521,26 @@ int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
> >  		break;
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  	case SPRN_TFHAR:
> > -		vcpu->arch.tfhar = spr_val;
> > -		break;
> >  	case SPRN_TEXASR:
> > -		vcpu->arch.texasr = spr_val;
> > -		break;
> >  	case SPRN_TFIAR:
> > -		vcpu->arch.tfiar = spr_val;
> > +		if (MSR_TM_ACTIVE(kvmppc_get_msr(vcpu))) {
> > +			/* it is illegal to mtspr() TM regs in
> > +			 * other than non-transactional state.
> > +			 */
> > +			kvmppc_core_queue_program(vcpu, SRR1_PROGTM);
> > +			emulated = EMULATE_AGAIN;
> > +			break;
> > +		}
> 
> We also need to check that the guest has TM enabled in the guest MSR,
> and give them a facility unavailable interrupt if not.
> 
> > +
> > +		tm_enable();
> > +		if (sprn == SPRN_TFHAR)
> > +			mtspr(SPRN_TFHAR, spr_val);
> > +		else if (sprn == SPRN_TEXASR)
> > +			mtspr(SPRN_TEXASR, spr_val);
> > +		else
> > +			mtspr(SPRN_TFIAR, spr_val);
> > +		tm_disable();
> 
> I haven't seen any checks that we are on a CPU that has TM.  What
> happens if a guest does a mtmsrd with TM=1 and then a mtspr to TEXASR
> when running on a POWER7 (assuming the host kernel was compiled with
> CONFIG_PPC_TRANSACTIONAL_MEM=y)?
> 
> Ideally, if the host CPU does not have TM functionality, these mtsprs
> would be treated as no-ops and attempts to set the TM or TS fields in
> the guest MSR would be ignored.
> 
> > +
> >  		break;
> >  #endif
> >  #endif
> > @@ -674,13 +687,19 @@ int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val
> >  		break;
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  	case SPRN_TFHAR:
> > -		*spr_val = vcpu->arch.tfhar;
> > +		tm_enable();
> > +		*spr_val = mfspr(SPRN_TFHAR);
> > +		tm_disable();
> >  		break;
> >  	case SPRN_TEXASR:
> > -		*spr_val = vcpu->arch.texasr;
> > +		tm_enable();
> > +		*spr_val = mfspr(SPRN_TEXASR);
> > +		tm_disable();
> >  		break;
> >  	case SPRN_TFIAR:
> > -		*spr_val = vcpu->arch.tfiar;
> > +		tm_enable();
> > +		*spr_val = mfspr(SPRN_TFIAR);
> > +		tm_disable();
> >  		break;
> 
> These need to check MSR_TM in the guest MSR, and become no-ops on
> machines without TM capability.

Thanks for the above catches. I will rework later.

BR,
- Simon
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index e096d01..c2836330 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -521,13 +521,26 @@  int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 		break;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	case SPRN_TFHAR:
-		vcpu->arch.tfhar = spr_val;
-		break;
 	case SPRN_TEXASR:
-		vcpu->arch.texasr = spr_val;
-		break;
 	case SPRN_TFIAR:
-		vcpu->arch.tfiar = spr_val;
+		if (MSR_TM_ACTIVE(kvmppc_get_msr(vcpu))) {
+			/* it is illegal to mtspr() TM regs in
+			 * other than non-transactional state.
+			 */
+			kvmppc_core_queue_program(vcpu, SRR1_PROGTM);
+			emulated = EMULATE_AGAIN;
+			break;
+		}
+
+		tm_enable();
+		if (sprn == SPRN_TFHAR)
+			mtspr(SPRN_TFHAR, spr_val);
+		else if (sprn == SPRN_TEXASR)
+			mtspr(SPRN_TEXASR, spr_val);
+		else
+			mtspr(SPRN_TFIAR, spr_val);
+		tm_disable();
+
 		break;
 #endif
 #endif
@@ -674,13 +687,19 @@  int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val
 		break;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	case SPRN_TFHAR:
-		*spr_val = vcpu->arch.tfhar;
+		tm_enable();
+		*spr_val = mfspr(SPRN_TFHAR);
+		tm_disable();
 		break;
 	case SPRN_TEXASR:
-		*spr_val = vcpu->arch.texasr;
+		tm_enable();
+		*spr_val = mfspr(SPRN_TEXASR);
+		tm_disable();
 		break;
 	case SPRN_TFIAR:
-		*spr_val = vcpu->arch.tfiar;
+		tm_enable();
+		*spr_val = mfspr(SPRN_TFIAR);
+		tm_disable();
 		break;
 #endif
 #endif