Patchwork [2/6] KVM: PPC: Book3E: Refactor SPE/FP exit handling

login
register
mail settings
Submitter Mihai Caraman
Date July 3, 2013, 12:42 p.m.
Message ID <1372855359-13452-3-git-send-email-mihai.caraman@freescale.com>
Download mbox | patch
Permalink /patch/256618/
State Not Applicable
Headers show

Comments

Mihai Caraman - July 3, 2013, 12:42 p.m.
SPE/FP/AltiVec interrupts share the same numbers. Refactor SPE/FP exit handling
to accommodate AltiVec later. Detect the targeted unit at run time since it can
be configured in the kernel but not featured on hardware.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |  102 +++++++++++++++++++++++++++++++---------------
 arch/powerpc/kvm/booke.h |    1 +
 2 files changed, 70 insertions(+), 33 deletions(-)
Alexander Graf - July 3, 2013, 1:30 p.m.
On 03.07.2013, at 14:42, Mihai Caraman wrote:

> SPE/FP/AltiVec interrupts share the same numbers. Refactor SPE/FP exit handling
> to accommodate AltiVec later. Detect the targeted unit at run time since it can
> be configured in the kernel but not featured on hardware.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
> arch/powerpc/kvm/booke.c |  102 +++++++++++++++++++++++++++++++---------------
> arch/powerpc/kvm/booke.h |    1 +
> 2 files changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fb47e85..113961f 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -89,6 +89,15 @@ void kvmppc_dump_vcpu(struct kvm_vcpu *vcpu)
> 	}
> }
> 
> +static inline bool kvmppc_supports_spe(void)
> +{
> +#ifdef CONFIG_SPE
> +		if (cpu_has_feature(CPU_FTR_SPE))
> +			return true;
> +#endif
> +	return false;
> +}
> +
> #ifdef CONFIG_SPE
> void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
> {
> @@ -99,7 +108,7 @@ void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
> 	preempt_enable();
> }
> 
> -static void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
> +void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
> {
> 	preempt_disable();
> 	enable_kernel_spe();
> @@ -118,6 +127,14 @@ static void kvmppc_vcpu_sync_spe(struct kvm_vcpu *vcpu)
> 	}
> }
> #else
> +void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
> +{
> +}
> +
> +void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
> +{
> +}
> +
> static void kvmppc_vcpu_sync_spe(struct kvm_vcpu *vcpu)
> {
> }
> @@ -943,48 +960,67 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 		r = RESUME_GUEST;
> 		break;
> 
> -#ifdef CONFIG_SPE
> 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
> -		if (vcpu->arch.shared->msr & MSR_SPE)
> -			kvmppc_vcpu_enable_spe(vcpu);
> -		else
> -			kvmppc_booke_queue_irqprio(vcpu,
> -						   BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
> +		if (kvmppc_supports_spe()) {
> +			bool enabled = false;
> +
> +#ifndef CONFIG_KVM_BOOKE_HV
> +			if (vcpu->arch.shared->msr & MSR_SPE) {
> +				kvmppc_vcpu_enable_spe(vcpu);
> +				enabled = true;
> +			}
> +#endif

Why the #ifdef? On HV capable systems kvmppc_supports_spe() will just always return false. And I don't really understand why HV would be special in the first place here. Is it because we're accessing shared->msr?

> +			if (!enabled)
> +				kvmppc_booke_queue_irqprio(vcpu,
> +					BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
> +		} else {
> +			/*
> +			 * Guest wants SPE, but host kernel doesn't support it.

host kernel or hardware


Alex
Caraman Mihai Claudiu-B02008 - July 3, 2013, 1:53 p.m.
> > -#ifdef CONFIG_SPE
> > 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
> > -		if (vcpu->arch.shared->msr & MSR_SPE)
> > -			kvmppc_vcpu_enable_spe(vcpu);
> > -		else
> > -			kvmppc_booke_queue_irqprio(vcpu,
> > -
> BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
> > +		if (kvmppc_supports_spe()) {
> > +			bool enabled = false;
> > +
> > +#ifndef CONFIG_KVM_BOOKE_HV
> > +			if (vcpu->arch.shared->msr & MSR_SPE) {
> > +				kvmppc_vcpu_enable_spe(vcpu);
> > +				enabled = true;
> > +			}
> > +#endif
> 
> Why the #ifdef? On HV capable systems kvmppc_supports_spe() will just
> always return false. 

AltiVec and SPE unavailable exceptions follows the same path. While 
kvmppc_supports_spe() will always return false kvmppc_supports_altivec()
may not.

> And I don't really understand why HV would be special in the first place
> here. Is it because we're accessing shared->msr?

You are right on HV case MSP[SPV] should be always zero when an unavailabe
exception take place. The distrinction was made because on non HV the guest
doesn't have direct access to MSR[SPE]. The name of the bit (not the position)
was changed on HV cores.

> 
> > +			if (!enabled)
> > +				kvmppc_booke_queue_irqprio(vcpu,
> > +					BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
> > +		} else {
> > +			/*
> > +			 * Guest wants SPE, but host kernel doesn't support it.
> 
> host kernel or hardware

Ok.

-Mike
Alexander Graf - July 3, 2013, 3:13 p.m.
On 03.07.2013, at 15:53, Caraman Mihai Claudiu-B02008 wrote:

>>> -#ifdef CONFIG_SPE
>>> 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
>>> -		if (vcpu->arch.shared->msr & MSR_SPE)
>>> -			kvmppc_vcpu_enable_spe(vcpu);
>>> -		else
>>> -			kvmppc_booke_queue_irqprio(vcpu,
>>> -
>> BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
>>> +		if (kvmppc_supports_spe()) {
>>> +			bool enabled = false;
>>> +
>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>> +			if (vcpu->arch.shared->msr & MSR_SPE) {
>>> +				kvmppc_vcpu_enable_spe(vcpu);
>>> +				enabled = true;
>>> +			}
>>> +#endif
>> 
>> Why the #ifdef? On HV capable systems kvmppc_supports_spe() will just
>> always return false. 
> 
> AltiVec and SPE unavailable exceptions follows the same path. While 
> kvmppc_supports_spe() will always return false kvmppc_supports_altivec()
> may not.

There is no chip that supports SPE and HV at the same time. So we'll never hit this anyway, since kvmppc_supports_spe() always returns false on HV capable systems.

Just add a comment saying so and remove the ifdef :).


Alex

> 
>> And I don't really understand why HV would be special in the first place
>> here. Is it because we're accessing shared->msr?
> 
> You are right on HV case MSP[SPV] should be always zero when an unavailabe
> exception take place. The distrinction was made because on non HV the guest
> doesn't have direct access to MSR[SPE]. The name of the bit (not the position)
> was changed on HV cores.
> 
>> 
>>> +			if (!enabled)
>>> +				kvmppc_booke_queue_irqprio(vcpu,
>>> +					BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
>>> +		} else {
>>> +			/*
>>> +			 * Guest wants SPE, but host kernel doesn't support it.
>> 
>> host kernel or hardware
> 
> Ok.
> 
> -Mike
>

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index fb47e85..113961f 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -89,6 +89,15 @@  void kvmppc_dump_vcpu(struct kvm_vcpu *vcpu)
 	}
 }
 
+static inline bool kvmppc_supports_spe(void)
+{
+#ifdef CONFIG_SPE
+		if (cpu_has_feature(CPU_FTR_SPE))
+			return true;
+#endif
+	return false;
+}
+
 #ifdef CONFIG_SPE
 void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
 {
@@ -99,7 +108,7 @@  void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
 	preempt_enable();
 }
 
-static void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
+void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
 {
 	preempt_disable();
 	enable_kernel_spe();
@@ -118,6 +127,14 @@  static void kvmppc_vcpu_sync_spe(struct kvm_vcpu *vcpu)
 	}
 }
 #else
+void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
+{
+}
+
+void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu)
+{
+}
+
 static void kvmppc_vcpu_sync_spe(struct kvm_vcpu *vcpu)
 {
 }
@@ -943,48 +960,67 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		r = RESUME_GUEST;
 		break;
 
-#ifdef CONFIG_SPE
 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
-		if (vcpu->arch.shared->msr & MSR_SPE)
-			kvmppc_vcpu_enable_spe(vcpu);
-		else
-			kvmppc_booke_queue_irqprio(vcpu,
-						   BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
+		if (kvmppc_supports_spe()) {
+			bool enabled = false;
+
+#ifndef CONFIG_KVM_BOOKE_HV
+			if (vcpu->arch.shared->msr & MSR_SPE) {
+				kvmppc_vcpu_enable_spe(vcpu);
+				enabled = true;
+			}
+#endif
+			if (!enabled)
+				kvmppc_booke_queue_irqprio(vcpu,
+					BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
+		} else {
+			/*
+			 * Guest wants SPE, but host kernel doesn't support it.
+			 * Send an "unimplemented operation" program check to
+			 * the guest.
+			 */
+			kvmppc_core_queue_program(vcpu, ESR_PUO | ESR_SPV);
+		}
+
 		r = RESUME_GUEST;
 		break;
 	}
 
 	case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
-		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
-		r = RESUME_GUEST;
-		break;
-
-	case BOOKE_INTERRUPT_SPE_FP_ROUND:
-		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_ROUND);
-		r = RESUME_GUEST;
-		break;
-#else
-	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL:
-		/*
-		 * Guest wants SPE, but host kernel doesn't support it.  Send
-		 * an "unimplemented operation" program check to the guest.
-		 */
-		kvmppc_core_queue_program(vcpu, ESR_PUO | ESR_SPV);
-		r = RESUME_GUEST;
+		if (kvmppc_supports_spe()) {
+			kvmppc_booke_queue_irqprio(vcpu,
+				BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
+			r = RESUME_GUEST;
+		} else {
+			/*
+			 * These really should never happen without CONFIG_SPE,
+			 * as we should never enable the real MSR[SPE] in the
+			 * guest.
+			 */
+			printk(KERN_CRIT "%s: unexpected SPE interrupt %u at \
+				%08lx\n", __func__, exit_nr, vcpu->arch.pc);
+			run->hw.hardware_exit_reason = exit_nr;
+			r = RESUME_HOST;
+		}
 		break;
 
-	/*
-	 * These really should never happen without CONFIG_SPE,
-	 * as we should never enable the real MSR[SPE] in the guest.
-	 */
-	case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
 	case BOOKE_INTERRUPT_SPE_FP_ROUND:
-		printk(KERN_CRIT "%s: unexpected SPE interrupt %u at %08lx\n",
-		       __func__, exit_nr, vcpu->arch.pc);
-		run->hw.hardware_exit_reason = exit_nr;
-		r = RESUME_HOST;
+		if (kvmppc_supports_spe()) {
+			kvmppc_booke_queue_irqprio(vcpu,
+				BOOKE_IRQPRIO_SPE_FP_ROUND);
+			r = RESUME_GUEST;
+		} else {
+			/*
+			 * These really should never happen without CONFIG_SPE,
+			 * as we should never enable the real MSR[SPE] in the
+			 * guest.
+			 */
+			printk(KERN_CRIT "%s: unexpected SPE interrupt %u at %08lx\n",
+			       __func__, exit_nr, vcpu->arch.pc);
+			run->hw.hardware_exit_reason = exit_nr;
+			r = RESUME_HOST;
+		}
 		break;
-#endif
 
 	case BOOKE_INTERRUPT_DATA_STORAGE:
 		kvmppc_core_queue_data_storage(vcpu, vcpu->arch.fault_dear,
diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
index 9e92006..e92ce5b 100644
--- a/arch/powerpc/kvm/booke.h
+++ b/arch/powerpc/kvm/booke.h
@@ -85,6 +85,7 @@  void kvmppc_load_guest_spe(struct kvm_vcpu *vcpu);
 void kvmppc_save_guest_spe(struct kvm_vcpu *vcpu);
 
 /* high-level function, manages flags, host state */
+void kvmppc_vcpu_enable_spe(struct kvm_vcpu *vcpu);
 void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu);
 
 void kvmppc_booke_vcpu_load(struct kvm_vcpu *vcpu, int cpu);