diff mbox series

[v3,19/52] KVM: PPC: Book3S HV P9: Reduce mtmsrd instructions required to save host SPRs

Message ID 20211004160049.1338837-20-npiggin@gmail.com
State New
Headers show
Series KVM: PPC: Book3S HV P9: entry/exit optimisations | expand

Commit Message

Nicholas Piggin Oct. 4, 2021, 4 p.m. UTC
This reduces the number of mtmsrd required to enable facility bits when
saving/restoring registers, by having the KVM code set all bits up front
rather than using individual facility functions that set their particular
MSR bits.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/switch_to.h  |  2 +
 arch/powerpc/kernel/process.c         | 28 +++++++++++++
 arch/powerpc/kvm/book3s_hv.c          | 59 ++++++++++++++++++---------
 arch/powerpc/kvm/book3s_hv_p9_entry.c |  1 +
 4 files changed, 71 insertions(+), 19 deletions(-)

Comments

Fabiano Rosas Oct. 16, 2021, 1:45 p.m. UTC | #1
Nicholas Piggin <npiggin@gmail.com> writes:

> This reduces the number of mtmsrd required to enable facility bits when
> saving/restoring registers, by having the KVM code set all bits up front
> rather than using individual facility functions that set their particular
> MSR bits.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

Aside: at msr_check_and_set what's with MSR_VSX always being implicitly
set whenever MSR_FP is set? I get that it depends on MSR_FP, but if FP
always implies VSX, then you could stop setting MSR_VSX in this patch.

> ---
>  arch/powerpc/include/asm/switch_to.h  |  2 +
>  arch/powerpc/kernel/process.c         | 28 +++++++++++++
>  arch/powerpc/kvm/book3s_hv.c          | 59 ++++++++++++++++++---------
>  arch/powerpc/kvm/book3s_hv_p9_entry.c |  1 +
>  4 files changed, 71 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> index 9d1fbd8be1c7..e8013cd6b646 100644
> --- a/arch/powerpc/include/asm/switch_to.h
> +++ b/arch/powerpc/include/asm/switch_to.h
> @@ -112,6 +112,8 @@ static inline void clear_task_ebb(struct task_struct *t)
>  #endif
>  }
>
> +void kvmppc_save_user_regs(void);
> +
>  extern int set_thread_tidr(struct task_struct *t);
>
>  #endif /* _ASM_POWERPC_SWITCH_TO_H */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 50436b52c213..3fca321b820d 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1156,6 +1156,34 @@ static inline void save_sprs(struct thread_struct *t)
>  #endif
>  }
>
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +void kvmppc_save_user_regs(void)
> +{
> +	unsigned long usermsr;
> +
> +	if (!current->thread.regs)
> +		return;
> +
> +	usermsr = current->thread.regs->msr;
> +
> +	if (usermsr & MSR_FP)
> +		save_fpu(current);
> +
> +	if (usermsr & MSR_VEC)
> +		save_altivec(current);
> +
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	if (usermsr & MSR_TM) {
> +		current->thread.tm_tfhar = mfspr(SPRN_TFHAR);
> +		current->thread.tm_tfiar = mfspr(SPRN_TFIAR);
> +		current->thread.tm_texasr = mfspr(SPRN_TEXASR);
> +		current->thread.regs->msr &= ~MSR_TM;
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_save_user_regs);
> +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
> +
>  static inline void restore_sprs(struct thread_struct *old_thread,
>  				struct thread_struct *new_thread)
>  {
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fca89ed2244f..16365c0e9872 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4140,6 +4140,7 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>  	struct p9_host_os_sprs host_os_sprs;
>  	s64 dec;
>  	u64 tb, next_timer;
> +	unsigned long msr;
>  	int trap;
>
>  	WARN_ON_ONCE(vcpu->arch.ceded);
> @@ -4151,8 +4152,23 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>  	if (next_timer < time_limit)
>  		time_limit = next_timer;
>
> +	vcpu->arch.ceded = 0;
> +
>  	save_p9_host_os_sprs(&host_os_sprs);
>
> +	/* MSR bits may have been cleared by context switch */
> +	msr = 0;
> +	if (IS_ENABLED(CONFIG_PPC_FPU))
> +		msr |= MSR_FP;
> +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +		msr |= MSR_VEC;
> +	if (cpu_has_feature(CPU_FTR_VSX))
> +		msr |= MSR_VSX;
> +	if (cpu_has_feature(CPU_FTR_TM) ||
> +	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
> +		msr |= MSR_TM;
> +	msr = msr_check_and_set(msr);
> +
>  	kvmppc_subcore_enter_guest();
>
>  	vc->entry_exit_map = 1;
> @@ -4161,12 +4177,13 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>  	vcpu_vpa_increment_dispatch(vcpu);
>
>  	if (cpu_has_feature(CPU_FTR_TM) ||
> -	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
> +	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) {
>  		kvmppc_restore_tm_hv(vcpu, vcpu->arch.shregs.msr, true);
> +		msr = mfmsr(); /* TM restore can update msr */
> +	}
>
>  	switch_pmu_to_guest(vcpu, &host_os_sprs);
>
> -	msr_check_and_set(MSR_FP | MSR_VEC | MSR_VSX);
>  	load_fp_state(&vcpu->arch.fp);
>  #ifdef CONFIG_ALTIVEC
>  	load_vr_state(&vcpu->arch.vr);
> @@ -4275,7 +4292,6 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>
>  	restore_p9_host_os_sprs(vcpu, &host_os_sprs);
>
> -	msr_check_and_set(MSR_FP | MSR_VEC | MSR_VSX);
>  	store_fp_state(&vcpu->arch.fp);
>  #ifdef CONFIG_ALTIVEC
>  	store_vr_state(&vcpu->arch.vr);
> @@ -4825,19 +4841,24 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>  	unsigned long user_tar = 0;
>  	unsigned int user_vrsave;
>  	struct kvm *kvm;
> +	unsigned long msr;
>
>  	if (!vcpu->arch.sane) {
>  		run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		return -EINVAL;
>  	}
>
> +	/* No need to go into the guest when all we'll do is come back out */
> +	if (signal_pending(current)) {
> +		run->exit_reason = KVM_EXIT_INTR;
> +		return -EINTR;
> +	}
> +
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	/*
>  	 * Don't allow entry with a suspended transaction, because
>  	 * the guest entry/exit code will lose it.
> -	 * If the guest has TM enabled, save away their TM-related SPRs
> -	 * (they will get restored by the TM unavailable interrupt).
>  	 */
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	if (cpu_has_feature(CPU_FTR_TM) && current->thread.regs &&
>  	    (current->thread.regs->msr & MSR_TM)) {
>  		if (MSR_TM_ACTIVE(current->thread.regs->msr)) {
> @@ -4845,12 +4866,6 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>  			run->fail_entry.hardware_entry_failure_reason = 0;
>  			return -EINVAL;
>  		}
> -		/* Enable TM so we can read the TM SPRs */
> -		mtmsr(mfmsr() | MSR_TM);
> -		current->thread.tm_tfhar = mfspr(SPRN_TFHAR);
> -		current->thread.tm_tfiar = mfspr(SPRN_TFIAR);
> -		current->thread.tm_texasr = mfspr(SPRN_TEXASR);
> -		current->thread.regs->msr &= ~MSR_TM;
>  	}
>  #endif
>
> @@ -4865,18 +4880,24 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>
>  	kvmppc_core_prepare_to_enter(vcpu);
>
> -	/* No need to go into the guest when all we'll do is come back out */
> -	if (signal_pending(current)) {
> -		run->exit_reason = KVM_EXIT_INTR;
> -		return -EINTR;
> -	}
> -
>  	kvm = vcpu->kvm;
>  	atomic_inc(&kvm->arch.vcpus_running);
>  	/* Order vcpus_running vs. mmu_ready, see kvmppc_alloc_reset_hpt */
>  	smp_mb();
>
> -	flush_all_to_thread(current);
> +	msr = 0;
> +	if (IS_ENABLED(CONFIG_PPC_FPU))
> +		msr |= MSR_FP;
> +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +		msr |= MSR_VEC;
> +	if (cpu_has_feature(CPU_FTR_VSX))
> +		msr |= MSR_VSX;
> +	if (cpu_has_feature(CPU_FTR_TM) ||
> +	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
> +		msr |= MSR_TM;
> +	msr = msr_check_and_set(msr);
> +
> +	kvmppc_save_user_regs();
>
>  	/* Save userspace EBB and other register values */
>  	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
> index a7f63082b4e3..fb9cb34445ea 100644
> --- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
> +++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
> @@ -224,6 +224,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>  		vc->tb_offset_applied = vc->tb_offset;
>  	}
>
> +	/* Could avoid mfmsr by passing around, but probably no big deal */
>  	msr = mfmsr();
>
>  	host_hfscr = mfspr(SPRN_HFSCR);
Nicholas Piggin Oct. 20, 2021, 5:35 a.m. UTC | #2
Excerpts from Fabiano Rosas's message of October 16, 2021 11:45 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> This reduces the number of mtmsrd required to enable facility bits when
>> saving/restoring registers, by having the KVM code set all bits up front
>> rather than using individual facility functions that set their particular
>> MSR bits.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
> 
> Aside: at msr_check_and_set what's with MSR_VSX always being implicitly
> set whenever MSR_FP is set? I get that it depends on MSR_FP, but if FP
> always implies VSX, then you could stop setting MSR_VSX in this patch.

Good question, this seems to come from quite old code and is carried
forward. I did not immediately see why, might have been to avoid
another mtmsrd operation if we later want to set VSX.

But the rule seems to be to set MSR_VSX if both FP and VEC are set, so
this seems a bit odd. __msr_check_and_clear similarly clears VSX if we 
clear FP, but not if we clear VEC.

I might be good to remove that logic or turn it into warnings and make 
sure the callers do the right thing. Not sure.

Thanks,
Nick
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index 9d1fbd8be1c7..e8013cd6b646 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -112,6 +112,8 @@  static inline void clear_task_ebb(struct task_struct *t)
 #endif
 }
 
+void kvmppc_save_user_regs(void);
+
 extern int set_thread_tidr(struct task_struct *t);
 
 #endif /* _ASM_POWERPC_SWITCH_TO_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 50436b52c213..3fca321b820d 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1156,6 +1156,34 @@  static inline void save_sprs(struct thread_struct *t)
 #endif
 }
 
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+void kvmppc_save_user_regs(void)
+{
+	unsigned long usermsr;
+
+	if (!current->thread.regs)
+		return;
+
+	usermsr = current->thread.regs->msr;
+
+	if (usermsr & MSR_FP)
+		save_fpu(current);
+
+	if (usermsr & MSR_VEC)
+		save_altivec(current);
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	if (usermsr & MSR_TM) {
+		current->thread.tm_tfhar = mfspr(SPRN_TFHAR);
+		current->thread.tm_tfiar = mfspr(SPRN_TFIAR);
+		current->thread.tm_texasr = mfspr(SPRN_TEXASR);
+		current->thread.regs->msr &= ~MSR_TM;
+	}
+#endif
+}
+EXPORT_SYMBOL_GPL(kvmppc_save_user_regs);
+#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
+
 static inline void restore_sprs(struct thread_struct *old_thread,
 				struct thread_struct *new_thread)
 {
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index fca89ed2244f..16365c0e9872 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4140,6 +4140,7 @@  static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
 	struct p9_host_os_sprs host_os_sprs;
 	s64 dec;
 	u64 tb, next_timer;
+	unsigned long msr;
 	int trap;
 
 	WARN_ON_ONCE(vcpu->arch.ceded);
@@ -4151,8 +4152,23 @@  static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
 	if (next_timer < time_limit)
 		time_limit = next_timer;
 
+	vcpu->arch.ceded = 0;
+
 	save_p9_host_os_sprs(&host_os_sprs);
 
+	/* MSR bits may have been cleared by context switch */
+	msr = 0;
+	if (IS_ENABLED(CONFIG_PPC_FPU))
+		msr |= MSR_FP;
+	if (cpu_has_feature(CPU_FTR_ALTIVEC))
+		msr |= MSR_VEC;
+	if (cpu_has_feature(CPU_FTR_VSX))
+		msr |= MSR_VSX;
+	if (cpu_has_feature(CPU_FTR_TM) ||
+	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
+		msr |= MSR_TM;
+	msr = msr_check_and_set(msr);
+
 	kvmppc_subcore_enter_guest();
 
 	vc->entry_exit_map = 1;
@@ -4161,12 +4177,13 @@  static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
 	vcpu_vpa_increment_dispatch(vcpu);
 
 	if (cpu_has_feature(CPU_FTR_TM) ||
-	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
+	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) {
 		kvmppc_restore_tm_hv(vcpu, vcpu->arch.shregs.msr, true);
+		msr = mfmsr(); /* TM restore can update msr */
+	}
 
 	switch_pmu_to_guest(vcpu, &host_os_sprs);
 
-	msr_check_and_set(MSR_FP | MSR_VEC | MSR_VSX);
 	load_fp_state(&vcpu->arch.fp);
 #ifdef CONFIG_ALTIVEC
 	load_vr_state(&vcpu->arch.vr);
@@ -4275,7 +4292,6 @@  static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
 
 	restore_p9_host_os_sprs(vcpu, &host_os_sprs);
 
-	msr_check_and_set(MSR_FP | MSR_VEC | MSR_VSX);
 	store_fp_state(&vcpu->arch.fp);
 #ifdef CONFIG_ALTIVEC
 	store_vr_state(&vcpu->arch.vr);
@@ -4825,19 +4841,24 @@  static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
 	unsigned long user_tar = 0;
 	unsigned int user_vrsave;
 	struct kvm *kvm;
+	unsigned long msr;
 
 	if (!vcpu->arch.sane) {
 		run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		return -EINVAL;
 	}
 
+	/* No need to go into the guest when all we'll do is come back out */
+	if (signal_pending(current)) {
+		run->exit_reason = KVM_EXIT_INTR;
+		return -EINTR;
+	}
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	/*
 	 * Don't allow entry with a suspended transaction, because
 	 * the guest entry/exit code will lose it.
-	 * If the guest has TM enabled, save away their TM-related SPRs
-	 * (they will get restored by the TM unavailable interrupt).
 	 */
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	if (cpu_has_feature(CPU_FTR_TM) && current->thread.regs &&
 	    (current->thread.regs->msr & MSR_TM)) {
 		if (MSR_TM_ACTIVE(current->thread.regs->msr)) {
@@ -4845,12 +4866,6 @@  static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
 			run->fail_entry.hardware_entry_failure_reason = 0;
 			return -EINVAL;
 		}
-		/* Enable TM so we can read the TM SPRs */
-		mtmsr(mfmsr() | MSR_TM);
-		current->thread.tm_tfhar = mfspr(SPRN_TFHAR);
-		current->thread.tm_tfiar = mfspr(SPRN_TFIAR);
-		current->thread.tm_texasr = mfspr(SPRN_TEXASR);
-		current->thread.regs->msr &= ~MSR_TM;
 	}
 #endif
 
@@ -4865,18 +4880,24 @@  static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
 
 	kvmppc_core_prepare_to_enter(vcpu);
 
-	/* No need to go into the guest when all we'll do is come back out */
-	if (signal_pending(current)) {
-		run->exit_reason = KVM_EXIT_INTR;
-		return -EINTR;
-	}
-
 	kvm = vcpu->kvm;
 	atomic_inc(&kvm->arch.vcpus_running);
 	/* Order vcpus_running vs. mmu_ready, see kvmppc_alloc_reset_hpt */
 	smp_mb();
 
-	flush_all_to_thread(current);
+	msr = 0;
+	if (IS_ENABLED(CONFIG_PPC_FPU))
+		msr |= MSR_FP;
+	if (cpu_has_feature(CPU_FTR_ALTIVEC))
+		msr |= MSR_VEC;
+	if (cpu_has_feature(CPU_FTR_VSX))
+		msr |= MSR_VSX;
+	if (cpu_has_feature(CPU_FTR_TM) ||
+	    cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
+		msr |= MSR_TM;
+	msr = msr_check_and_set(msr);
+
+	kvmppc_save_user_regs();
 
 	/* Save userspace EBB and other register values */
 	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index a7f63082b4e3..fb9cb34445ea 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -224,6 +224,7 @@  int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
 		vc->tb_offset_applied = vc->tb_offset;
 	}
 
+	/* Could avoid mfmsr by passing around, but probably no big deal */
 	msr = mfmsr();
 
 	host_hfscr = mfspr(SPRN_HFSCR);