Patchwork [RFC,2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling

login
register
mail settings
Submitter Mihai Caraman
Date June 3, 2013, 8:54 p.m.
Message ID <1370292868-2697-3-git-send-email-mihai.caraman@freescale.com>
Download mbox | patch
Permalink /patch/248422/
State New
Headers show

Comments

Mihai Caraman - June 3, 2013, 8:54 p.m.
SPE_FP interrupts are shared with ALTIVEC. Refactor SPE_FP exit handling
to detect KVM support for the featured unit at run-time, in order to
accommodate ALTIVEC later.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |   80 ++++++++++++++++++++++++++++++++++------------
 1 files changed, 59 insertions(+), 21 deletions(-)
Scott Wood - June 4, 2013, 10:14 p.m.
On 06/03/2013 03:54:24 PM, Mihai Caraman wrote:
> SPE_FP interrupts are shared with ALTIVEC. Refactor SPE_FP exit  
> handling
> to detect KVM support for the featured unit at run-time, in order to
> accommodate ALTIVEC later.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c |   80  
> ++++++++++++++++++++++++++++++++++------------
>  1 files changed, 59 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 1020119..d082bbc 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -822,6 +822,15 @@ static void kvmppc_restart_interrupt(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;
> +}

Whitespace

>  /**
>   * kvmppc_handle_exit
>   *
> @@ -931,42 +940,71 @@ int kvmppc_handle_exit(struct kvm_run *run,  
> struct kvm_vcpu *vcpu,
>  		r = RESUME_GUEST;
>  		break;
> 
> -#ifdef CONFIG_SPE
>  	case BOOKE_INTERRUPT_SPE_UNAVAIL: {
> -		if (vcpu->arch.shared->msr & MSR_SPE)
> -			kvmppc_vcpu_enable_spe(vcpu);
> -		else
> -			kvmppc_booke_queue_irqprio(vcpu,
> -						    
> BOOKE_IRQPRIO_SPE_UNAVAIL);
> +		/*
> +		 * The interrupt is shared, KVM support for the  
> featured unit
> +		 * is detected at run-time.
> +		 */

This is a decent comment for the changelog, but for the code itself it  
seems fairly obvious if you look at the definition of  
kvmppc_supports_spe().

> +		bool handled = false;
> +
> +		if (kvmppc_supports_spe()) {
> +#ifdef CONFIG_SPE
> +			if (cpu_has_feature(CPU_FTR_SPE))

Didn't you already check this using kvmppc_supports_spe()?

>  	case BOOKE_INTERRUPT_SPE_FP_ROUND:
> +#ifdef CONFIG_SPE
>  		kvmppc_booke_queue_irqprio(vcpu,  
> BOOKE_IRQPRIO_SPE_FP_ROUND);
>  		r = RESUME_GUEST;
>  		break;

Why not use kvmppc_supports_spe() here, for consistency?

-Scott
--
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
Caraman Mihai Claudiu-B02008 - June 5, 2013, 7:29 a.m.
> > +		/*
> > +		 * The interrupt is shared, KVM support for the
> > featured unit
> > +		 * is detected at run-time.
> > +		 */
> 
> This is a decent comment for the changelog, but for the code itself it
> seems fairly obvious if you look at the definition of
> kvmppc_supports_spe().

I will move it to change log.

> 
> > +		bool handled = false;
> > +
> > +		if (kvmppc_supports_spe()) {
> > +#ifdef CONFIG_SPE
> > +			if (cpu_has_feature(CPU_FTR_SPE))
> 
> Didn't you already check this using kvmppc_supports_spe()?

It makes sense with the next patch. It handles the improbable case of having
CONFIG_ALTIVEC and CONFIG_SPE defined:

	if (kvmppc_supports_altivec() || kvmppc_supports_spe()).

> 
> >  	case BOOKE_INTERRUPT_SPE_FP_ROUND:
> > +#ifdef CONFIG_SPE
> >  		kvmppc_booke_queue_irqprio(vcpu,
> > BOOKE_IRQPRIO_SPE_FP_ROUND);
> >  		r = RESUME_GUEST;
> >  		break;
> 
> Why not use kvmppc_supports_spe() here, for consistency?

I added cpu_has_feature(CPU_FTR_SPE) for the case specified above, but here
SPE_FP_ROUND is not shared with ALTIVEC. CONFIG_SPE is used in other places
in KVM without this check, shouldn't be all or nothing?

-Mike




--
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/booke.c b/arch/powerpc/kvm/booke.c
index 1020119..d082bbc 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -822,6 +822,15 @@  static void kvmppc_restart_interrupt(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;
+}
+
 /**
  * kvmppc_handle_exit
  *
@@ -931,42 +940,71 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		r = RESUME_GUEST;
 		break;
 
-#ifdef CONFIG_SPE
 	case BOOKE_INTERRUPT_SPE_UNAVAIL: {
-		if (vcpu->arch.shared->msr & MSR_SPE)
-			kvmppc_vcpu_enable_spe(vcpu);
-		else
-			kvmppc_booke_queue_irqprio(vcpu,
-						   BOOKE_IRQPRIO_SPE_UNAVAIL);
+		/*
+		 * The interrupt is shared, KVM support for the featured unit
+		 * is detected at run-time.
+		 */
+		bool handled = false;
+
+		if (kvmppc_supports_spe()) {
+#ifdef CONFIG_SPE
+			if (cpu_has_feature(CPU_FTR_SPE))
+				if (vcpu->arch.shared->msr & MSR_SPE) {
+					kvmppc_vcpu_enable_spe(vcpu);
+					handled = true;
+				}
+#endif
+			if (!handled)
+				kvmppc_booke_queue_irqprio(vcpu,
+					BOOKE_IRQPRIO_SPE_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:
-		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_DATA);
-		r = RESUME_GUEST;
+		/*
+		 * The interrupt is shared, KVM support for the featured unit
+		 * is detected at run-time.
+		 */
+		if (kvmppc_supports_spe()) {
+			kvmppc_booke_queue_irqprio(vcpu,
+				BOOKE_IRQPRIO_SPE_FP_DATA);
+			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;
 
 	case BOOKE_INTERRUPT_SPE_FP_ROUND:
+#ifdef CONFIG_SPE
 		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_ROUND);
 		r = RESUME_GUEST;
 		break;
 #else
-	case BOOKE_INTERRUPT_SPE_UNAVAIL:
 		/*
-		 * Guest wants SPE, but host kernel doesn't support it.  Send
-		 * an "unimplemented operation" program check to the guest.
+		 * These really should never happen without CONFIG_SPE,
+		 * as we should never enable the real MSR[SPE] in the
+		 * guest.
 		 */
-		kvmppc_core_queue_program(vcpu, ESR_PUO | ESR_SPV);
-		r = RESUME_GUEST;
-		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:
-	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;