[PATCH 0/6] KVM: PPC: Book3E: AltiVec support

[PATCH 0/6] KVM: PPC: Book3E: AltiVec support

[Mihai Caraman]

Add KVM Book3E AltiVec support and enable e6500 core.

Mihai Caraman (6):
  KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers
  KVM: PPC: Book3E: Refactor SPE/FP exit handling
  KVM: PPC: Book3E: Increase FPU laziness
  KVM: PPC: Book3E: Add AltiVec support
  KVM: PPC: Book3E: Add ONE_REG AltiVec support
  KVM: PPC: Book3E: Enable e6500 core

 arch/powerpc/kvm/booke.c              |  211 +++++++++++++++++++++++++++------
 arch/powerpc/kvm/booke.h              |    5 +-
 arch/powerpc/kvm/bookehv_interrupts.S |    8 +-
 arch/powerpc/kvm/e500.c               |   10 +-
 arch/powerpc/kvm/e500_emulate.c       |    8 +-
 arch/powerpc/kvm/e500mc.c             |   12 ++-
 6 files changed, 201 insertions(+), 53 deletions(-)

-- 
1.7.3.4

[PATCH 1/6] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Mihai Caraman]

Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
which share the same interrupts numbers.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c              |   16 ++++++++--------
 arch/powerpc/kvm/booke.h              |    4 ++--
 arch/powerpc/kvm/bookehv_interrupts.S |    8 ++++----
 arch/powerpc/kvm/e500.c               |   10 ++++++----
 arch/powerpc/kvm/e500_emulate.c       |    8 ++++----
 5 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index d2fef74..fb47e85 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -362,8 +362,8 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 	case BOOKE_IRQPRIO_ITLB_MISS:
 	case BOOKE_IRQPRIO_SYSCALL:
 	case BOOKE_IRQPRIO_FP_UNAVAIL:
-	case BOOKE_IRQPRIO_SPE_UNAVAIL:
-	case BOOKE_IRQPRIO_SPE_FP_DATA:
+	case BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL:
+	case BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST:
 	case BOOKE_IRQPRIO_SPE_FP_ROUND:
 	case BOOKE_IRQPRIO_AP_UNAVAIL:
 		allowed = 1;
@@ -944,18 +944,18 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		break;
 
 #ifdef CONFIG_SPE
-	case BOOKE_INTERRUPT_SPE_UNAVAIL: {
+	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_UNAVAIL);
+						   BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
 		r = RESUME_GUEST;
 		break;
 	}
 
-	case BOOKE_INTERRUPT_SPE_FP_DATA:
-		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_DATA);
+	case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
+		kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
 		r = RESUME_GUEST;
 		break;
 
@@ -964,7 +964,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		r = RESUME_GUEST;
 		break;
 #else
-	case BOOKE_INTERRUPT_SPE_UNAVAIL:
+	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.
@@ -977,7 +977,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	 * 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_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);
diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
index 5fd1ba6..9e92006 100644
--- a/arch/powerpc/kvm/booke.h
+++ b/arch/powerpc/kvm/booke.h
@@ -32,8 +32,8 @@
 #define BOOKE_IRQPRIO_ALIGNMENT 2
 #define BOOKE_IRQPRIO_PROGRAM 3
 #define BOOKE_IRQPRIO_FP_UNAVAIL 4
-#define BOOKE_IRQPRIO_SPE_UNAVAIL 5
-#define BOOKE_IRQPRIO_SPE_FP_DATA 6
+#define BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL 5
+#define BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST 6
 #define BOOKE_IRQPRIO_SPE_FP_ROUND 7
 #define BOOKE_IRQPRIO_SYSCALL 8
 #define BOOKE_IRQPRIO_AP_UNAVAIL 9
diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
index e8ed7d6..8d35dc0 100644
--- a/arch/powerpc/kvm/bookehv_interrupts.S
+++ b/arch/powerpc/kvm/bookehv_interrupts.S
@@ -295,9 +295,9 @@ kvm_handler BOOKE_INTERRUPT_DTLB_MISS, EX_PARAMS_TLB, \
 	SPRN_SRR0, SPRN_SRR1, (NEED_EMU | NEED_DEAR | NEED_ESR)
 kvm_handler BOOKE_INTERRUPT_ITLB_MISS, EX_PARAMS_TLB, \
 	SPRN_SRR0, SPRN_SRR1, 0
-kvm_handler BOOKE_INTERRUPT_SPE_UNAVAIL, EX_PARAMS(GEN), \
+kvm_handler BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL, EX_PARAMS(GEN), \
 	SPRN_SRR0, SPRN_SRR1, 0
-kvm_handler BOOKE_INTERRUPT_SPE_FP_DATA, EX_PARAMS(GEN), \
+kvm_handler BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST, EX_PARAMS(GEN), \
 	SPRN_SRR0, SPRN_SRR1, 0
 kvm_handler BOOKE_INTERRUPT_SPE_FP_ROUND, EX_PARAMS(GEN), \
 	SPRN_SRR0, SPRN_SRR1, 0
@@ -398,8 +398,8 @@ kvm_lvl_handler BOOKE_INTERRUPT_WATCHDOG, \
 kvm_handler BOOKE_INTERRUPT_DTLB_MISS, \
 	SPRN_SRR0, SPRN_SRR1, (NEED_EMU | NEED_DEAR | NEED_ESR)
 kvm_handler BOOKE_INTERRUPT_ITLB_MISS, SPRN_SRR0, SPRN_SRR1, 0
-kvm_handler BOOKE_INTERRUPT_SPE_UNAVAIL, SPRN_SRR0, SPRN_SRR1, 0
-kvm_handler BOOKE_INTERRUPT_SPE_FP_DATA, SPRN_SRR0, SPRN_SRR1, 0
+kvm_handler BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL, SPRN_SRR0, SPRN_SRR1, 0
+kvm_handler BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST, SPRN_SRR0, SPRN_SRR1, 0
 kvm_handler BOOKE_INTERRUPT_SPE_FP_ROUND, SPRN_SRR0, SPRN_SRR1, 0
 kvm_handler BOOKE_INTERRUPT_PERFORMANCE_MONITOR, SPRN_SRR0, SPRN_SRR1, 0
 kvm_handler BOOKE_INTERRUPT_DOORBELL, SPRN_SRR0, SPRN_SRR1, 0
diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
index ce6b73c..0fb2f4d 100644
--- a/arch/powerpc/kvm/e500.c
+++ b/arch/powerpc/kvm/e500.c
@@ -380,8 +380,10 @@ void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 	sregs->u.e.impl.fsl.hid0 = vcpu_e500->hid0;
 	sregs->u.e.impl.fsl.mcar = vcpu_e500->mcar;
 
-	sregs->u.e.ivor_high[0] = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_UNAVAIL];
-	sregs->u.e.ivor_high[1] = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA];
+	sregs->u.e.ivor_high[0] =
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL];
+	sregs->u.e.ivor_high[1] =
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST];
 	sregs->u.e.ivor_high[2] = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_ROUND];
 	sregs->u.e.ivor_high[3] =
 		vcpu->arch.ivor[BOOKE_IRQPRIO_PERFORMANCE_MONITOR];
@@ -409,9 +411,9 @@ int kvmppc_core_set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 		return 0;
 
 	if (sregs->u.e.features & KVM_SREGS_E_SPE) {
-		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_UNAVAIL] =
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL] =
 			sregs->u.e.ivor_high[0];
-		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA] =
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST] =
 			sregs->u.e.ivor_high[1];
 		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_ROUND] =
 			sregs->u.e.ivor_high[2];
diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
index b10a012..b2e159b 100644
--- a/arch/powerpc/kvm/e500_emulate.c
+++ b/arch/powerpc/kvm/e500_emulate.c
@@ -211,10 +211,10 @@ int kvmppc_core_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 
 	/* extra exceptions */
 	case SPRN_IVOR32:
-		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_UNAVAIL] = spr_val;
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL] = spr_val;
 		break;
 	case SPRN_IVOR33:
-		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA] = spr_val;
+		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST] = spr_val;
 		break;
 	case SPRN_IVOR34:
 		vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_ROUND] = spr_val;
@@ -329,10 +329,10 @@ int kvmppc_core_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
 
 	/* extra exceptions */
 	case SPRN_IVOR32:
-		*spr_val = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_UNAVAIL];
+		*spr_val = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL];
 		break;
 	case SPRN_IVOR33:
-		*spr_val = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA];
+		*spr_val = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST];
 		break;
 	case SPRN_IVOR34:
 		*spr_val = vcpu->arch.ivor[BOOKE_IRQPRIO_SPE_FP_ROUND];
-- 
1.7.3.4

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

[Mihai Caraman]

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
+			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);
-- 
1.7.3.4

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

[Alexander Graf]

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

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

[Caraman Mihai Claudiu-B02008]

> > -#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

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

[Alexander Graf]

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
> 

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

[Scott Wood]

  On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
> 
> 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 :).

kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined.   
More seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't  
interpret it as SPE unless CONFIG_SPE is defined.  And we can't rely on  
the "if (kvmppc_supports_spe())" here because a later patch changes it  
to "if (kvmppc_supports_altivec() || kvmppc_supports_spe())".  So I  
think we still need the ifdef CONFIG_SPE here.

As for the HV ifndef, we should try not to confuse HV/PR with  
e500mc/e500v2, even if we happen to only run HV on e500mc and PR on  
e500v2.  We would not want to call kvmppc_vcpu_enable_spe() here on a  
hypothetical HV target with SPE.  And we *would* want to call  
kvmppc_vcpu_enable_fp() here on a hypothetical PR target with normal  
FP.  It's one thing to leave out the latter, since it would involve  
writing actual code that we have no way to test at this point, but  
quite another to leave out the proper conditions for when we want to  
run code that we do have.

-Scott

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

[Alexander Graf]

On 03.07.2013, at 20:28, Scott Wood wrote:

> On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
>> 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 :).
> 
> kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined.  More seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't interpret it as SPE unless CONFIG_SPE is defined.  And we can't rely on the "if (kvmppc_supports_spe())" here because a later patch changes it to "if (kvmppc_supports_altivec() || kvmppc_supports_spe())".  So I think we still need the ifdef CONFIG_SPE here.
> 
> As for the HV ifndef, we should try not to confuse HV/PR with e500mc/e500v2, even if we happen to only run HV on e500mc and PR on e500v2.  We would not want to call kvmppc_vcpu_enable_spe() here on a hypothetical HV target with SPE.  And we *would* want to call kvmppc_vcpu_enable_fp() here on a hypothetical PR target with normal FP.  It's one thing to leave out the latter, since it would involve writing actual code that we have no way to test at this point, but quite another to leave out the proper conditions for when we want to run code that we do have.

So we should make this an #ifdef CONFIG_SPE rather than #ifndef CONFIG_KVM_BOOKE_HV?


Alex

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

[Scott Wood]

On 07/03/2013 01:42:12 PM, Alexander Graf wrote:
> 
> On 03.07.2013, at 20:28, Scott Wood wrote:
> 
> > On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
> >> 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 :).
> >
> > kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined.   
> More seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't  
> interpret it as SPE unless CONFIG_SPE is defined.  And we can't rely  
> on the "if (kvmppc_supports_spe())" here because a later patch  
> changes it to "if (kvmppc_supports_altivec() ||  
> kvmppc_supports_spe())".  So I think we still need the ifdef  
> CONFIG_SPE here.
> >
> > As for the HV ifndef, we should try not to confuse HV/PR with  
> e500mc/e500v2, even if we happen to only run HV on e500mc and PR on  
> e500v2.  We would not want to call kvmppc_vcpu_enable_spe() here on a  
> hypothetical HV target with SPE.  And we *would* want to call  
> kvmppc_vcpu_enable_fp() here on a hypothetical PR target with normal  
> FP.  It's one thing to leave out the latter, since it would involve  
> writing actual code that we have no way to test at this point, but  
> quite another to leave out the proper conditions for when we want to  
> run code that we do have.
> 
> So we should make this an #ifdef CONFIG_SPE rather than #ifndef  
> CONFIG_KVM_BOOKE_HV?

I think it should be "#if !defined(CONFIG_KVM_BOOKE_HV) &&  
defined(CONFIG_SPE)" for the reasons I described in my second paragraph.

-Scott

[PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Mihai Caraman]

Increase FPU laziness by calling kvmppc_load_guest_fp() just before
returning to guest instead of each sched in. Without this improvement
an interrupt may also claim floting point corrupting guest state.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c  |    1 +
 arch/powerpc/kvm/e500mc.c |    2 --
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 113961f..3cae2e3 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
 		} else {
 			kvmppc_lazy_ee_enable();
+			kvmppc_load_guest_fp(vcpu);
 		}
 	}
 
diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 19c8379..09da1ac 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		kvmppc_e500_tlbil_all(vcpu_e500);
 		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
 	}
-
-	kvmppc_load_guest_fp(vcpu);
 }
 
 void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
-- 
1.7.3.4

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 14:42, Mihai Caraman wrote:

> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> returning to guest instead of each sched in. Without this improvement
> an interrupt may also claim floting point corrupting guest state.

Not sure I follow. Could you please describe exactly what's happening?


Alex

> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
> arch/powerpc/kvm/booke.c  |    1 +
> arch/powerpc/kvm/e500mc.c |    2 --
> 2 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 113961f..3cae2e3 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
> 		} else {
> 			kvmppc_lazy_ee_enable();
> +			kvmppc_load_guest_fp(vcpu);
> 		}
> 	}
> 
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index 19c8379..09da1ac 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> 		kvmppc_e500_tlbil_all(vcpu_e500);
> 		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
> 	}
> -
> -	kvmppc_load_guest_fp(vcpu);
> }
> 
> void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> -- 
> 1.7.3.4
> 
> 
> --
> 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

RE: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Caraman Mihai Claudiu-B02008]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, July 03, 2013 4:45 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness
> 
> 
> On 03.07.2013, at 14:42, Mihai Caraman wrote:
> 
> > Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> > returning to guest instead of each sched in. Without this improvement
> > an interrupt may also claim floting point corrupting guest state.
> 
> Not sure I follow. Could you please describe exactly what's happening?

This was already discussed on the list, I will forward you the thread.

-Mike

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 15:55, Caraman Mihai Claudiu-B02008 wrote:

>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, July 03, 2013 4:45 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org
>> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness
>> 
>> 
>> On 03.07.2013, at 14:42, Mihai Caraman wrote:
>> 
>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>>> returning to guest instead of each sched in. Without this improvement
>>> an interrupt may also claim floting point corrupting guest state.
>> 
>> Not sure I follow. Could you please describe exactly what's happening?
> 
> This was already discussed on the list, I will forward you the thread.

The only thing I've seen in that thread was some pathetic theoretical case where an interrupt handler would enable fp and clobber state carelessly. That's not something I'm worried about.

I really don't see where this patch improves anything tbh. It certainly makes the code flow more awkward.


Alex

RE: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Caraman Mihai Claudiu-B02008]

> >>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> >>> returning to guest instead of each sched in. Without this improvement
> >>> an interrupt may also claim floting point corrupting guest state.
> >>
> >> Not sure I follow. Could you please describe exactly what's happening?
> >
> > This was already discussed on the list, I will forward you the thread.
> 
> The only thing I've seen in that thread was some pathetic theoretical
> case where an interrupt handler would enable fp and clobber state
> carelessly. That's not something I'm worried about.

Neither me though I don't find it pathetic. Please refer it to Scott.

> 
> I really don't see where this patch improves anything tbh. It certainly
> makes the code flow more awkward.

I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel
is struggling to achieve is to reduce the number of store/restore operations.
Without this improvement we restore the unit each time we are sched it. If an
other process take the ownership of the unit (on SMP it's even worse but don't
bother with this) the kernel store the unit state to qemu task. This can happen
multiple times during handle_exit().

Do you see it now? 

-Mike

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 17:41, Caraman Mihai Claudiu-B02008 wrote:

>>>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>>>>> returning to guest instead of each sched in. Without this improvement
>>>>> an interrupt may also claim floting point corrupting guest state.
>>>> 
>>>> Not sure I follow. Could you please describe exactly what's happening?
>>> 
>>> This was already discussed on the list, I will forward you the thread.
>> 
>> The only thing I've seen in that thread was some pathetic theoretical
>> case where an interrupt handler would enable fp and clobber state
>> carelessly. That's not something I'm worried about.
> 
> Neither me though I don't find it pathetic. Please refer it to Scott.

If from Linux's point of view we look like a user space program with active floating point registers, we don't have to worry about this case. Kernel code that would clobber that fp state would clobber random user space's fp state too.

> 
>> 
>> I really don't see where this patch improves anything tbh. It certainly
>> makes the code flow more awkward.
> 
> I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel
> is struggling to achieve is to reduce the number of store/restore operations.
> Without this improvement we restore the unit each time we are sched it. If an
> other process take the ownership of the unit (on SMP it's even worse but don't
> bother with this) the kernel store the unit state to qemu task. This can happen
> multiple times during handle_exit().
> 
> Do you see it now? 

Yup. Looks good. The code flow is very hard to follow though - there are a lot of implicit assumptions that don't get documented anywhere. For example the fact that we rely on giveup_fpu() to remove MSR_FP from our thread.


Alex

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Scott Wood]

On 07/03/2013 11:59:45 AM, Alexander Graf wrote:
> 
> On 03.07.2013, at 17:41, Caraman Mihai Claudiu-B02008 wrote:
> 
> >>>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just  
> before
> >>>>> returning to guest instead of each sched in. Without this  
> improvement
> >>>>> an interrupt may also claim floting point corrupting guest  
> state.
> >>>>
> >>>> Not sure I follow. Could you please describe exactly what's  
> happening?
> >>>
> >>> This was already discussed on the list, I will forward you the  
> thread.
> >>
> >> The only thing I've seen in that thread was some pathetic  
> theoretical
> >> case where an interrupt handler would enable fp and clobber state
> >> carelessly. That's not something I'm worried about.
> >
> > Neither me though I don't find it pathetic. Please refer it to  
> Scott.
> 
> If from Linux's point of view we look like a user space program with  
> active floating point registers, we don't have to worry about this  
> case. Kernel code that would clobber that fp state would clobber  
> random user space's fp state too.

This patch makes it closer to how it works with a user space program.   
Or rather, it reduces the time window when we don't (and can't) act  
like a normal userspace program -- and ensures that we have interrupts  
disabled during that window.  An interrupt can't randomly clobber FP  
state; it has to call enable_kernel_fp() just like KVM does.   
enable_kernel_fp() clears the userspace MSR_FP to ensure that the state  
it saves gets restored before userspace uses it again, but that won't  
have any effect on guest execution (especially in HV-mode).  Thus  
kvmppc_load_guest_fp() needs to be atomic with guest entry.   
Conceptually it's like taking an automatic FP unavailable trap when we  
enter the guest, since we can't be lazy in HV-mode.

> >> I really don't see where this patch improves anything tbh. It  
> certainly
> >> makes the code flow more awkward.
> >
> > I was pointing you to this: The idea of FPU/AltiVec laziness that  
> the kernel
> > is struggling to achieve is to reduce the number of store/restore  
> operations.
> > Without this improvement we restore the unit each time we are sched  
> it. If an
> > other process take the ownership of the unit (on SMP it's even  
> worse but don't
> > bother with this) the kernel store the unit state to qemu task.  
> This can happen
> > multiple times during handle_exit().
> >
> > Do you see it now?
> 
> Yup. Looks good. The code flow is very hard to follow though - there  
> are a lot of implicit assumptions that don't get documented anywhere.  
> For example the fact that we rely on giveup_fpu() to remove MSR_FP  
> from our thread.

That's not new to this patch...

-Scott

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 19:17, Scott Wood wrote:

> On 07/03/2013 11:59:45 AM, Alexander Graf wrote:
>> On 03.07.2013, at 17:41, Caraman Mihai Claudiu-B02008 wrote:
>> >>>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> >>>>> returning to guest instead of each sched in. Without this improvement
>> >>>>> an interrupt may also claim floting point corrupting guest state.
>> >>>>
>> >>>> Not sure I follow. Could you please describe exactly what's happening?
>> >>>
>> >>> This was already discussed on the list, I will forward you the thread.
>> >>
>> >> The only thing I've seen in that thread was some pathetic theoretical
>> >> case where an interrupt handler would enable fp and clobber state
>> >> carelessly. That's not something I'm worried about.
>> >
>> > Neither me though I don't find it pathetic. Please refer it to Scott.
>> If from Linux's point of view we look like a user space program with active floating point registers, we don't have to worry about this case. Kernel code that would clobber that fp state would clobber random user space's fp state too.
> 
> This patch makes it closer to how it works with a user space program.  Or rather, it reduces the time window when we don't (and can't) act like a normal userspace program -- and ensures that we have interrupts disabled during that window.  An interrupt can't randomly clobber FP state; it has to call enable_kernel_fp() just like KVM does.  enable_kernel_fp() clears the userspace MSR_FP to ensure that the state it saves gets restored before userspace uses it again, but that won't have any effect on guest execution (especially in HV-mode).  Thus kvmppc_load_guest_fp() needs to be atomic with guest entry.  Conceptually it's like taking an automatic FP unavailable trap when we enter the guest, since we can't be lazy in HV-mode.

Yep. Once I understood that point things became clear to me :).

> 
>> >> I really don't see where this patch improves anything tbh. It certainly
>> >> makes the code flow more awkward.
>> >
>> > I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel
>> > is struggling to achieve is to reduce the number of store/restore operations.
>> > Without this improvement we restore the unit each time we are sched it. If an
>> > other process take the ownership of the unit (on SMP it's even worse but don't
>> > bother with this) the kernel store the unit state to qemu task. This can happen
>> > multiple times during handle_exit().
>> >
>> > Do you see it now?
>> Yup. Looks good. The code flow is very hard to follow though - there are a lot of implicit assumptions that don't get documented anywhere. For example the fact that we rely on giveup_fpu() to remove MSR_FP from our thread.
> 
> That's not new to this patch...

Would be nice to fix nevertheless. I'm probably not going to be the last person forgetting how this works.


Alex

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Scott Wood]

On 07/03/2013 10:11:50 AM, Alexander Graf wrote:
> 
> On 03.07.2013, at 15:55, Caraman Mihai Claudiu-B02008 wrote:
> 
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Wednesday, July 03, 2013 4:45 PM
> >> To: Caraman Mihai Claudiu-B02008
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> >> dev@lists.ozlabs.org
> >> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness
> >>
> >>
> >> On 03.07.2013, at 14:42, Mihai Caraman wrote:
> >>
> >>> Increase FPU laziness by calling kvmppc_load_guest_fp() just  
> before
> >>> returning to guest instead of each sched in. Without this  
> improvement
> >>> an interrupt may also claim floting point corrupting guest state.
> >>
> >> Not sure I follow. Could you please describe exactly what's  
> happening?
> >
> > This was already discussed on the list, I will forward you the  
> thread.
> 
> The only thing I've seen in that thread was some pathetic theoretical  
> case where an interrupt handler would enable fp and clobber state  
> carelessly. That's not something I'm worried about.

On x86 floating point registers can be used for memcpy(), which can be  
used in interrupt handlers.  Just because it doesn't happen on PPC  
today doesn't make it a "pathetic theoretical case" that we should  
ignore and leave a landmine buried in the KVM code.  Even power7 is  
using something similar for copyuser (which isn't called from interrupt  
context, but it's not a huge leap from that to doing it in memcpy).

It also doesn't seem *that* farfetched that some driver for unusual  
hardware could decide it needs FP in its interrupt handler, and call  
the function that is specifically meant to ensure that.  It's frowned  
upon, but that doesn't mean nobody will ever do it.

-Scott

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 19:07, Scott Wood wrote:

> On 07/03/2013 10:11:50 AM, Alexander Graf wrote:
>> On 03.07.2013, at 15:55, Caraman Mihai Claudiu-B02008 wrote:
>> >> -----Original Message-----
>> >> From: Alexander Graf [mailto:agraf@suse.de]
>> >> Sent: Wednesday, July 03, 2013 4:45 PM
>> >> To: Caraman Mihai Claudiu-B02008
>> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> >> dev@lists.ozlabs.org
>> >> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness
>> >>
>> >>
>> >> On 03.07.2013, at 14:42, Mihai Caraman wrote:
>> >>
>> >>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> >>> returning to guest instead of each sched in. Without this improvement
>> >>> an interrupt may also claim floting point corrupting guest state.
>> >>
>> >> Not sure I follow. Could you please describe exactly what's happening?
>> >
>> > This was already discussed on the list, I will forward you the thread.
>> The only thing I've seen in that thread was some pathetic theoretical case where an interrupt handler would enable fp and clobber state carelessly. That's not something I'm worried about.
> 
> On x86 floating point registers can be used for memcpy(), which can be used in interrupt handlers.  Just because it doesn't happen on PPC today doesn't make it a "pathetic theoretical case" that we should ignore and leave a landmine buried in the KVM code.  Even power7 is using something similar for copyuser (which isn't called from interrupt context, but it's not a huge leap from that to doing it in memcpy).
> 
> It also doesn't seem *that* farfetched that some driver for unusual hardware could decide it needs FP in its interrupt handler, and call the function that is specifically meant to ensure that.  It's frowned upon, but that doesn't mean nobody will ever do it.

Oh, sure. But in that case I would strongly hope that the driver first saves off the current FPU state to the thread struct before it goes off and uses them for itself. Otherwise it would break user space, no?


Alex

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Scott Wood]

On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> returning to guest instead of each sched in. Without this improvement
> an interrupt may also claim floting point corrupting guest state.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c  |    1 +
>  arch/powerpc/kvm/e500mc.c |    2 --
>  2 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 113961f..3cae2e3 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run,  
> struct kvm_vcpu *vcpu,
>  			r = (s << 2) | RESUME_HOST | (r &  
> RESUME_FLAG_NV);
>  		} else {
>  			kvmppc_lazy_ee_enable();
> +			kvmppc_load_guest_fp(vcpu);
>  		}

This should go before the kvmppc_lazy_ee_enable().

-Scott

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 19:18, Scott Wood wrote:

> On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> returning to guest instead of each sched in. Without this improvement
>> an interrupt may also claim floting point corrupting guest state.
>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>> ---
>> arch/powerpc/kvm/booke.c  |    1 +
>> arch/powerpc/kvm/e500mc.c |    2 --
>> 2 files changed, 1 insertions(+), 2 deletions(-)
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 113961f..3cae2e3 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>> 		} else {
>> 			kvmppc_lazy_ee_enable();
>> +			kvmppc_load_guest_fp(vcpu);
>> 		}
> 
> This should go before the kvmppc_lazy_ee_enable().

Why? What difference does that make? We're running with interrupts disabled here, right?


Alex

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Scott Wood]

On 07/03/2013 12:23:16 PM, Alexander Graf wrote:
> 
> On 03.07.2013, at 19:18, Scott Wood wrote:
> 
> > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
> >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> >> returning to guest instead of each sched in. Without this  
> improvement
> >> an interrupt may also claim floting point corrupting guest state.
> >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >> ---
> >> arch/powerpc/kvm/booke.c  |    1 +
> >> arch/powerpc/kvm/e500mc.c |    2 --
> >> 2 files changed, 1 insertions(+), 2 deletions(-)
> >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> >> index 113961f..3cae2e3 100644
> >> --- a/arch/powerpc/kvm/booke.c
> >> +++ b/arch/powerpc/kvm/booke.c
> >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run,  
> struct kvm_vcpu *vcpu,
> >> 			r = (s << 2) | RESUME_HOST | (r &  
> RESUME_FLAG_NV);
> >> 		} else {
> >> 			kvmppc_lazy_ee_enable();
> >> +			kvmppc_load_guest_fp(vcpu);
> >> 		}
> >
> > This should go before the kvmppc_lazy_ee_enable().
> 
> Why? What difference does that make? We're running with interrupts  
> disabled here, right?

Yes, and we want to minimize the code we run where we have interrupts  
disabled but the lazy ee state says they're enabled.  So  
kvmppc_lazy_ee_enable() should be the last thing we do before entering  
asm code.

See http://patchwork.ozlabs.org/patch/249565/

-Scott

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 19:44, Scott Wood wrote:

> On 07/03/2013 12:23:16 PM, Alexander Graf wrote:
>> On 03.07.2013, at 19:18, Scott Wood wrote:
>> > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
>> >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> >> returning to guest instead of each sched in. Without this improvement
>> >> an interrupt may also claim floting point corrupting guest state.
>> >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>> >> ---
>> >> arch/powerpc/kvm/booke.c  |    1 +
>> >> arch/powerpc/kvm/e500mc.c |    2 --
>> >> 2 files changed, 1 insertions(+), 2 deletions(-)
>> >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> >> index 113961f..3cae2e3 100644
>> >> --- a/arch/powerpc/kvm/booke.c
>> >> +++ b/arch/powerpc/kvm/booke.c
>> >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> >> 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>> >> 		} else {
>> >> 			kvmppc_lazy_ee_enable();
>> >> +			kvmppc_load_guest_fp(vcpu);
>> >> 		}
>> >
>> > This should go before the kvmppc_lazy_ee_enable().
>> Why? What difference does that make? We're running with interrupts disabled here, right?
> 
> Yes, and we want to minimize the code we run where we have interrupts disabled but the lazy ee state says they're enabled.  So kvmppc_lazy_ee_enable() should be the last thing we do before entering asm code.
> 
> See http://patchwork.ozlabs.org/patch/249565/

Ah, cool. So we should add a comment saying that this should be the last thing before entering asm code then :). That way we make sure nobody else repeats the same mistake.


Alex

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Scott Wood]

On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> returning to guest instead of each sched in. Without this improvement
> an interrupt may also claim floting point corrupting guest state.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c  |    1 +
>  arch/powerpc/kvm/e500mc.c |    2 --
>  2 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 113961f..3cae2e3 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run,  
> struct kvm_vcpu *vcpu,
>  			r = (s << 2) | RESUME_HOST | (r &  
> RESUME_FLAG_NV);
>  		} else {
>  			kvmppc_lazy_ee_enable();
> +			kvmppc_load_guest_fp(vcpu);
>  		}
>  	}
> 
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index 19c8379..09da1ac 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu,  
> int cpu)
>  		kvmppc_e500_tlbil_all(vcpu_e500);
>  		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
>  	}
> -
> -	kvmppc_load_guest_fp(vcpu);
>  }
> 
>  void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)

Can we now remove vcpu->fpu_active, and the comment that says "Kernel  
usage of FP (via
enable_kernel_fp()) in this thread must not occur while  
vcpu->fpu_active is set."?

-Scott

Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 20:37, Scott Wood wrote:

> On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> returning to guest instead of each sched in. Without this improvement
>> an interrupt may also claim floting point corrupting guest state.
>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>> ---
>> arch/powerpc/kvm/booke.c  |    1 +
>> arch/powerpc/kvm/e500mc.c |    2 --
>> 2 files changed, 1 insertions(+), 2 deletions(-)
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 113961f..3cae2e3 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>> 		} else {
>> 			kvmppc_lazy_ee_enable();
>> +			kvmppc_load_guest_fp(vcpu);
>> 		}
>> 	}
>> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
>> index 19c8379..09da1ac 100644
>> --- a/arch/powerpc/kvm/e500mc.c
>> +++ b/arch/powerpc/kvm/e500mc.c
>> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>> 		kvmppc_e500_tlbil_all(vcpu_e500);
>> 		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
>> 	}
>> -
>> -	kvmppc_load_guest_fp(vcpu);
>> }
>> void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> 
> Can we now remove vcpu->fpu_active, and the comment that says "Kernel usage of FP (via
> enable_kernel_fp()) in this thread must not occur while vcpu->fpu_active is set."?

I think so, yes.


Alex

RE: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Caraman Mihai Claudiu-B02008]

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
> owner@vger.kernel.org] On Behalf Of Alexander Graf
> Sent: Wednesday, July 03, 2013 9:40 PM
> To: Wood Scott-B07421
> Cc: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org;
> kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness
> 
> 
> On 03.07.2013, at 20:37, Scott Wood wrote:
> 
> > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote:
> >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
> >> returning to guest instead of each sched in. Without this improvement
> >> an interrupt may also claim floting point corrupting guest state.
> >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >> ---
> >> arch/powerpc/kvm/booke.c  |    1 +
> >> arch/powerpc/kvm/e500mc.c |    2 --
> >> 2 files changed, 1 insertions(+), 2 deletions(-)
> >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> >> index 113961f..3cae2e3 100644
> >> --- a/arch/powerpc/kvm/booke.c
> >> +++ b/arch/powerpc/kvm/booke.c
> >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run,
> struct kvm_vcpu *vcpu,
> >> 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
> >> 		} else {
> >> 			kvmppc_lazy_ee_enable();
> >> +			kvmppc_load_guest_fp(vcpu);
> >> 		}
> >> 	}
> >> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> >> index 19c8379..09da1ac 100644
> >> --- a/arch/powerpc/kvm/e500mc.c
> >> +++ b/arch/powerpc/kvm/e500mc.c
> >> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu,
> int cpu)
> >> 		kvmppc_e500_tlbil_all(vcpu_e500);
> >> 		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
> >> 	}
> >> -
> >> -	kvmppc_load_guest_fp(vcpu);
> >> }
> >> void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> >
> > Can we now remove vcpu->fpu_active, and the comment that says "Kernel
> usage of FP (via
> > enable_kernel_fp()) in this thread must not occur while vcpu-
> >fpu_active is set."?
> 
> I think so, yes.

Yes, as I already did this for AltiVec.

-Mike

[PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Mihai Caraman]

Add KVM Book3E AltiVec support. KVM Book3E FPU support gracefully reuse host
infrastructure so follow the same approach for AltiVec.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |   72 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 3cae2e3..c3c3af6 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -98,6 +98,19 @@ static inline bool kvmppc_supports_spe(void)
 	return false;
 }
 
+/*
+ * Always returns true is AltiVec unit is present, see
+ * kvmppc_core_check_processor_compat().
+ */
+static inline bool kvmppc_supports_altivec(void)
+{
+#ifdef CONFIG_ALTIVEC
+		if (cpu_has_feature(CPU_FTR_ALTIVEC))
+			return true;
+#endif
+	return false;
+}
+
 #ifdef CONFIG_SPE
 void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
 {
@@ -151,6 +164,21 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 }
 
 /*
+ * Simulate AltiVec unavailable fault to load guest state
+ * from thread to AltiVec unit.
+ * It requires to be called with preemption disabled.
+ */
+static inline void kvmppc_load_guest_altivec(struct kvm_vcpu *vcpu)
+{
+	if (kvmppc_supports_altivec()) {
+		if (!(current->thread.regs->msr & MSR_VEC)) {
+			load_up_altivec(NULL);
+			current->thread.regs->msr |= MSR_VEC;
+		}
+	}
+}
+
+/*
  * Helper function for "full" MSR writes.  No need to call this if only
  * EE/CE/ME/DE/RI are changing.
  */
@@ -678,6 +706,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	u64 fpr[32];
 #endif
 
+#ifdef CONFIG_ALTIVEC
+	vector128 vr[32];
+	vector128 vscr;
+	int used_vr = 0;
+#endif
+
 	if (!vcpu->arch.sane) {
 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		return -EINVAL;
@@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	kvmppc_load_guest_fp(vcpu);
 #endif
 
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
+		/* Save userspace VEC state in stack */
+		enable_kernel_altivec();
+		memcpy(vr, current->thread.vr, sizeof(current->thread.vr));
+		vscr = current->thread.vscr;
+		used_vr = current->thread.used_vr;
+
+		/* Restore guest VEC state to thread */
+		memcpy(current->thread.vr, vcpu->arch.vr, sizeof(vcpu->arch.vr));
+		current->thread.vscr = vcpu->arch.vscr;
+
+		kvmppc_load_guest_altivec(vcpu);
+	}
+#endif
+
 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
 
 	/* No need for kvm_guest_exit. It's done in handle_exit.
@@ -736,6 +786,23 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	current->thread.fpexc_mode = fpexc_mode;
 #endif
 
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
+		/* Save AltiVec state to thread */
+		if (current->thread.regs->msr & MSR_VEC)
+			giveup_altivec(current);
+
+		/* Save guest state */
+		memcpy(vcpu->arch.vr, current->thread.vr, sizeof(vcpu->arch.vr));
+		vcpu->arch.vscr = current->thread.vscr;
+
+		/* Restore userspace state */
+		memcpy(current->thread.vr, vr, sizeof(current->thread.vr));
+		current->thread.vscr = vscr;
+		current->thread.used_vr = used_vr;
+	}
+#endif
+
 out:
 	vcpu->mode = OUTSIDE_GUEST_MODE;
 	return ret;
@@ -961,7 +1028,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		break;
 
 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
-		if (kvmppc_supports_spe()) {
+		if (kvmppc_supports_altivec() || kvmppc_supports_spe()) {
 			bool enabled = false;
 
 #ifndef CONFIG_KVM_BOOKE_HV
@@ -987,7 +1054,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	}
 
 	case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
-		if (kvmppc_supports_spe()) {
+		if (kvmppc_supports_altivec() || kvmppc_supports_spe()) {
 			kvmppc_booke_queue_irqprio(vcpu,
 				BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
 			r = RESUME_GUEST;
@@ -1205,6 +1272,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		} else {
 			kvmppc_lazy_ee_enable();
 			kvmppc_load_guest_fp(vcpu);
+			kvmppc_load_guest_altivec(vcpu);
 		}
 	}
 
-- 
1.7.3.4

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Alexander Graf]

On 03.07.2013, at 14:42, Mihai Caraman wrote:

> Add KVM Book3E AltiVec support. KVM Book3E FPU support gracefully reuse host
> infrastructure so follow the same approach for AltiVec.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
> arch/powerpc/kvm/booke.c |   72 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 3cae2e3..c3c3af6 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -98,6 +98,19 @@ static inline bool kvmppc_supports_spe(void)
> 	return false;
> }
> 
> +/*
> + * Always returns true is AltiVec unit is present, see
> + * kvmppc_core_check_processor_compat().
> + */
> +static inline bool kvmppc_supports_altivec(void)
> +{
> +#ifdef CONFIG_ALTIVEC
> +		if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +			return true;
> +#endif
> +	return false;
> +}
> +
> #ifdef CONFIG_SPE
> void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu)
> {
> @@ -151,6 +164,21 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
> }
> 
> /*
> + * Simulate AltiVec unavailable fault to load guest state
> + * from thread to AltiVec unit.
> + * It requires to be called with preemption disabled.
> + */
> +static inline void kvmppc_load_guest_altivec(struct kvm_vcpu *vcpu)
> +{
> +	if (kvmppc_supports_altivec()) {
> +		if (!(current->thread.regs->msr & MSR_VEC)) {
> +			load_up_altivec(NULL);
> +			current->thread.regs->msr |= MSR_VEC;

Does this ensure that the kernel saves / restores all altivec state on task switch? Does it load it again when it schedules us in? Would definitely be worth a comment.

> +		}
> +	}
> +}
> +
> +/*
>  * Helper function for "full" MSR writes.  No need to call this if only
>  * EE/CE/ME/DE/RI are changing.
>  */
> @@ -678,6 +706,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> 	u64 fpr[32];
> #endif
> 
> +#ifdef CONFIG_ALTIVEC
> +	vector128 vr[32];
> +	vector128 vscr;
> +	int used_vr = 0;

bool

> +#endif
> +
> 	if (!vcpu->arch.sane) {
> 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> 		return -EINVAL;
> @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> 	kvmppc_load_guest_fp(vcpu);
> #endif
> 
> +#ifdef CONFIG_ALTIVEC

/* Switch from user space Altivec to guest Altivec state */

> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {

Why not use your kvmppc_supports_altivec() helper here?

> +		/* Save userspace VEC state in stack */
> +		enable_kernel_altivec();

Can't you hide this in the load function? We only want to enable kernel altivec for a short time while we shuffle the registers around.

> +		memcpy(vr, current->thread.vr, sizeof(current->thread.vr));

vr = current->thread.vr;

> +		vscr = current->thread.vscr;
> +		used_vr = current->thread.used_vr;
> +
> +		/* Restore guest VEC state to thread */
> +		memcpy(current->thread.vr, vcpu->arch.vr, sizeof(vcpu->arch.vr));

current->thread.vr = vcpu->arch.vr;

> +		current->thread.vscr = vcpu->arch.vscr;
> +
> +		kvmppc_load_guest_altivec(vcpu);
> +	}

Do we need to do this even when the guest doesn't use Altivec? Can't we just load it on demand then once we fault? This code path really should only be a prefetch enable when MSR_VEC is already set in guest context.

> +#endif
> +
> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> 
> 	/* No need for kvm_guest_exit. It's done in handle_exit.
> @@ -736,6 +786,23 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> 	current->thread.fpexc_mode = fpexc_mode;
> #endif
> 
> +#ifdef CONFIG_ALTIVEC

/* Switch from guest Altivec to user space Altivec state */

> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> +		/* Save AltiVec state to thread */
> +		if (current->thread.regs->msr & MSR_VEC)
> +			giveup_altivec(current);
> +
> +		/* Save guest state */
> +		memcpy(vcpu->arch.vr, current->thread.vr, sizeof(vcpu->arch.vr));
> +		vcpu->arch.vscr = current->thread.vscr;
> +
> +		/* Restore userspace state */
> +		memcpy(current->thread.vr, vr, sizeof(current->thread.vr));
> +		current->thread.vscr = vscr;
> +		current->thread.used_vr = used_vr;
> +	}

Same comments here. If the guest never touched Altivec state, don't bother restoring it, as it's still good.


Alex

> +#endif
> +
> out:
> 	vcpu->mode = OUTSIDE_GUEST_MODE;
> 	return ret;
> @@ -961,7 +1028,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 		break;
> 
> 	case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
> -		if (kvmppc_supports_spe()) {
> +		if (kvmppc_supports_altivec() || kvmppc_supports_spe()) {
> 			bool enabled = false;
> 
> #ifndef CONFIG_KVM_BOOKE_HV
> @@ -987,7 +1054,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 	}
> 
> 	case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
> -		if (kvmppc_supports_spe()) {
> +		if (kvmppc_supports_altivec() || kvmppc_supports_spe()) {
> 			kvmppc_booke_queue_irqprio(vcpu,
> 				BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
> 			r = RESUME_GUEST;
> @@ -1205,6 +1272,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 		} else {
> 			kvmppc_lazy_ee_enable();
> 			kvmppc_load_guest_fp(vcpu);
> +			kvmppc_load_guest_altivec(vcpu);
> 		}
> 	}
> 
> -- 
> 1.7.3.4
> 
> 
> --
> 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

RE: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Caraman Mihai Claudiu-B02008]

> > + * Simulate AltiVec unavailable fault to load guest state
> > + * from thread to AltiVec unit.
> > + * It requires to be called with preemption disabled.
> > + */
> > +static inline void kvmppc_load_guest_altivec(struct kvm_vcpu *vcpu)
> > +{
> > +	if (kvmppc_supports_altivec()) {
> > +		if (!(current->thread.regs->msr & MSR_VEC)) {
> > +			load_up_altivec(NULL);
> > +			current->thread.regs->msr |= MSR_VEC;
> 
> Does this ensure that the kernel saves / restores all altivec state on
> task switch? Does it load it again when it schedules us in? Would
> definitely be worth a comment.

These units are _LAZY_ !!! Only SMP kernel save the state when it schedules out.

> 
> > +		}
> > +	}
> > +}
> > +
> > +/*
> >  * Helper function for "full" MSR writes.  No need to call this if only
> >  * EE/CE/ME/DE/RI are changing.
> >  */
> > @@ -678,6 +706,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
> struct kvm_vcpu *vcpu)
> > 	u64 fpr[32];
> > #endif
> >
> > +#ifdef CONFIG_ALTIVEC
> > +	vector128 vr[32];
> > +	vector128 vscr;
> > +	int used_vr = 0;
> 
> bool

Why don't you ask first to change the type of used_vr member in
/include/asm/processor.h?

> 
> > +#endif
> > +
> > 	if (!vcpu->arch.sane) {
> > 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> > 		return -EINVAL;
> > @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
> struct kvm_vcpu *vcpu)
> > 	kvmppc_load_guest_fp(vcpu);
> > #endif
> >
> > +#ifdef CONFIG_ALTIVEC
> 
> /* Switch from user space Altivec to guest Altivec state */
> 
> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> 
> Why not use your kvmppc_supports_altivec() helper here?

Give it a try ... because Linus guarded this members with CONFIG_ALTIVEC :)

> 
> > +		/* Save userspace VEC state in stack */
> > +		enable_kernel_altivec();
> 
> Can't you hide this in the load function? We only want to enable kernel
> altivec for a short time while we shuffle the registers around.
> 
> > +		memcpy(vr, current->thread.vr, sizeof(current->thread.vr));
> 
> vr = current->thread.vr;

Are you kidding, be more careful with arrays :) 

> 
> > +		vscr = current->thread.vscr;
> > +		used_vr = current->thread.used_vr;
> > +
> > +		/* Restore guest VEC state to thread */
> > +		memcpy(current->thread.vr, vcpu->arch.vr, sizeof(vcpu-
> >arch.vr));
> 
> current->thread.vr = vcpu->arch.vr;
> 
> > +		current->thread.vscr = vcpu->arch.vscr;
> > +
> > +		kvmppc_load_guest_altivec(vcpu);
> > +	}
> 
> Do we need to do this even when the guest doesn't use Altivec? Can't we
> just load it on demand then once we fault? This code path really should
> only be a prefetch enable when MSR_VEC is already set in guest context.

No we can't, read 6/6. 

> 
> > +#endif
> > +
> > 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> >
> > 	/* No need for kvm_guest_exit. It's done in handle_exit.
> > @@ -736,6 +786,23 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
> struct kvm_vcpu *vcpu)
> > 	current->thread.fpexc_mode = fpexc_mode;
> > #endif
> >
> > +#ifdef CONFIG_ALTIVEC
> 
> /* Switch from guest Altivec to user space Altivec state */
> 
> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> > +		/* Save AltiVec state to thread */
> > +		if (current->thread.regs->msr & MSR_VEC)
> > +			giveup_altivec(current);
> > +
> > +		/* Save guest state */
> > +		memcpy(vcpu->arch.vr, current->thread.vr, sizeof(vcpu-
> >arch.vr));
> > +		vcpu->arch.vscr = current->thread.vscr;
> > +
> > +		/* Restore userspace state */
> > +		memcpy(current->thread.vr, vr, sizeof(current->thread.vr));
> > +		current->thread.vscr = vscr;
> > +		current->thread.used_vr = used_vr;
> > +	}
> 
> Same comments here. If the guest never touched Altivec state, don't
> bother restoring it, as it's still good.

LOL, the mighty guest kernel does whatever he wants with AltiVec and
doesn't inform us.

-Mike

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Alexander Graf]

On 03.07.2013, at 18:09, Caraman Mihai Claudiu-B02008 wrote:

>>> + * Simulate AltiVec unavailable fault to load guest state
>>> + * from thread to AltiVec unit.
>>> + * It requires to be called with preemption disabled.
>>> + */
>>> +static inline void kvmppc_load_guest_altivec(struct kvm_vcpu *vcpu)
>>> +{
>>> +	if (kvmppc_supports_altivec()) {
>>> +		if (!(current->thread.regs->msr & MSR_VEC)) {
>>> +			load_up_altivec(NULL);
>>> +			current->thread.regs->msr |= MSR_VEC;
>> 
>> Does this ensure that the kernel saves / restores all altivec state on
>> task switch? Does it load it again when it schedules us in? Would
>> definitely be worth a comment.
> 
> These units are _LAZY_ !!! Only SMP kernel save the state when it schedules out.

Then how do you ensure that altivec state is still consistent after another altivec user got scheduled? Have I missed a vcpu_load hook anywhere?

> 
>> 
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +/*
>>> * Helper function for "full" MSR writes.  No need to call this if only
>>> * EE/CE/ME/DE/RI are changing.
>>> */
>>> @@ -678,6 +706,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
>> struct kvm_vcpu *vcpu)
>>> 	u64 fpr[32];
>>> #endif
>>> 
>>> +#ifdef CONFIG_ALTIVEC
>>> +	vector128 vr[32];
>>> +	vector128 vscr;
>>> +	int used_vr = 0;
>> 
>> bool
> 
> Why don't you ask first to change the type of used_vr member in
> /include/asm/processor.h?

Ah, it's a copy from thread. Fair enough.

> 
>> 
>>> +#endif
>>> +
>>> 	if (!vcpu->arch.sane) {
>>> 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>> 		return -EINVAL;
>>> @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
>> struct kvm_vcpu *vcpu)
>>> 	kvmppc_load_guest_fp(vcpu);
>>> #endif
>>> 
>>> +#ifdef CONFIG_ALTIVEC
>> 
>> /* Switch from user space Altivec to guest Altivec state */
>> 
>>> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
>> 
>> Why not use your kvmppc_supports_altivec() helper here?
> 
> Give it a try ... because Linus guarded this members with CONFIG_ALTIVEC :)

Huh? You already are in an #ifdef CONFIG_ALTIVEC here. I think it's a good idea to be consistent in helper usage. And the name you gave to the helper (kvmppc_supports_altivec) is actually quite nice and tells us exactly what we're asking for.

> 
>> 
>>> +		/* Save userspace VEC state in stack */
>>> +		enable_kernel_altivec();
>> 
>> Can't you hide this in the load function? We only want to enable kernel
>> altivec for a short time while we shuffle the registers around.
>> 
>>> +		memcpy(vr, current->thread.vr, sizeof(current->thread.vr));
>> 
>> vr = current->thread.vr;
> 
> Are you kidding, be more careful with arrays :) 

Bleks :).

> 
>> 
>>> +		vscr = current->thread.vscr;
>>> +		used_vr = current->thread.used_vr;
>>> +
>>> +		/* Restore guest VEC state to thread */
>>> +		memcpy(current->thread.vr, vcpu->arch.vr, sizeof(vcpu-
>>> arch.vr));
>> 
>> current->thread.vr = vcpu->arch.vr;
>> 
>>> +		current->thread.vscr = vcpu->arch.vscr;
>>> +
>>> +		kvmppc_load_guest_altivec(vcpu);
>>> +	}
>> 
>> Do we need to do this even when the guest doesn't use Altivec? Can't we
>> just load it on demand then once we fault? This code path really should
>> only be a prefetch enable when MSR_VEC is already set in guest context.
> 
> No we can't, read 6/6. 

So we have to make sure we're completely unlazy when it comes to a KVM guest. Are we?


Alex

> 
>> 
>>> +#endif
>>> +
>>> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>>> 
>>> 	/* No need for kvm_guest_exit. It's done in handle_exit.
>>> @@ -736,6 +786,23 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
>> struct kvm_vcpu *vcpu)
>>> 	current->thread.fpexc_mode = fpexc_mode;
>>> #endif
>>> 
>>> +#ifdef CONFIG_ALTIVEC
>> 
>> /* Switch from guest Altivec to user space Altivec state */
>> 
>>> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
>>> +		/* Save AltiVec state to thread */
>>> +		if (current->thread.regs->msr & MSR_VEC)
>>> +			giveup_altivec(current);
>>> +
>>> +		/* Save guest state */
>>> +		memcpy(vcpu->arch.vr, current->thread.vr, sizeof(vcpu-
>>> arch.vr));
>>> +		vcpu->arch.vscr = current->thread.vscr;
>>> +
>>> +		/* Restore userspace state */
>>> +		memcpy(current->thread.vr, vr, sizeof(current->thread.vr));
>>> +		current->thread.vscr = vscr;
>>> +		current->thread.used_vr = used_vr;
>>> +	}
>> 
>> Same comments here. If the guest never touched Altivec state, don't
>> bother restoring it, as it's still good.
> 
> LOL, the mighty guest kernel does whatever he wants with AltiVec and
> doesn't inform us.
> 
> -Mike
> 

RE: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Caraman Mihai Claudiu-B02008]

> >>> +
> >>> 	if (!vcpu->arch.sane) {
> >>> 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> >>> 		return -EINVAL;
> >>> @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
> >> struct kvm_vcpu *vcpu)
> >>> 	kvmppc_load_guest_fp(vcpu);
> >>> #endif
> >>>
> >>> +#ifdef CONFIG_ALTIVEC
> >>
> >> /* Switch from user space Altivec to guest Altivec state */
> >>
> >>> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> >>
> >> Why not use your kvmppc_supports_altivec() helper here?
> >
> > Give it a try ... because Linus guarded this members with
> CONFIG_ALTIVEC :)
> 
> Huh? You already are in an #ifdef CONFIG_ALTIVEC here. I think it's a
> good idea to be consistent in helper usage. And the name you gave to the
> helper (kvmppc_supports_altivec) is actually quite nice and tells us
> exactly what we're asking for.

I thought you asking to replace #ifdef CONFIG_ALTIVEC.

> >> Do we need to do this even when the guest doesn't use Altivec? Can't
> we
> >> just load it on demand then once we fault? This code path really
> should
> >> only be a prefetch enable when MSR_VEC is already set in guest
> context.
> >
> > No we can't, read 6/6.
> 
> So we have to make sure we're completely unlazy when it comes to a KVM
> guest. Are we?

Yes, because MSR[SPV] is under its control.

-Mike

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Alexander Graf]

On 03.07.2013, at 18:49, Caraman Mihai Claudiu-B02008 wrote:

>>>>> +
>>>>> 	if (!vcpu->arch.sane) {
>>>>> 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>>> 		return -EINVAL;
>>>>> @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
>>>> struct kvm_vcpu *vcpu)
>>>>> 	kvmppc_load_guest_fp(vcpu);
>>>>> #endif
>>>>> 
>>>>> +#ifdef CONFIG_ALTIVEC
>>>> 
>>>> /* Switch from user space Altivec to guest Altivec state */
>>>> 
>>>>> +	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
>>>> 
>>>> Why not use your kvmppc_supports_altivec() helper here?
>>> 
>>> Give it a try ... because Linus guarded this members with
>> CONFIG_ALTIVEC :)
>> 
>> Huh? You already are in an #ifdef CONFIG_ALTIVEC here. I think it's a
>> good idea to be consistent in helper usage. And the name you gave to the
>> helper (kvmppc_supports_altivec) is actually quite nice and tells us
>> exactly what we're asking for.
> 
> I thought you asking to replace #ifdef CONFIG_ALTIVEC.
> 
>>>> Do we need to do this even when the guest doesn't use Altivec? Can't
>> we
>>>> just load it on demand then once we fault? This code path really
>> should
>>>> only be a prefetch enable when MSR_VEC is already set in guest
>> context.
>>> 
>>> No we can't, read 6/6.
>> 
>> So we have to make sure we're completely unlazy when it comes to a KVM
>> guest. Are we?
> 
> Yes, because MSR[SPV] is under its control.

Oh, sure, KVM wants it unlazy. That part is obvious. But does the kernel always give us unlazyness? The way I read the code, process.c goes lazy when !CONFIG_SMP.

So the big question is why we're manually enforcing FPU giveup, but not Altivec giveup? One of the 2 probably is wrong :).


Alex

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Scott Wood]

On 07/03/2013 12:07:30 PM, Alexander Graf wrote:
> 
> On 03.07.2013, at 18:49, Caraman Mihai Claudiu-B02008 wrote:
> 
> >>>> Do we need to do this even when the guest doesn't use Altivec?  
> Can't
> >> we
> >>>> just load it on demand then once we fault? This code path really
> >> should
> >>>> only be a prefetch enable when MSR_VEC is already set in guest
> >> context.
> >>>
> >>> No we can't, read 6/6.
> >>
> >> So we have to make sure we're completely unlazy when it comes to a  
> KVM
> >> guest. Are we?
> >
> > Yes, because MSR[SPV] is under its control.
> 
> Oh, sure, KVM wants it unlazy. That part is obvious. But does the  
> kernel always give us unlazyness? The way I read the code, process.c  
> goes lazy when !CONFIG_SMP.
> 
> So the big question is why we're manually enforcing FPU giveup, but  
> not Altivec giveup? One of the 2 probably is wrong :).

Why do you think we're not enforcing it for Altivec?  Is there some  
specific piece of code you're referring to that is different in this  
regard?

-Scott

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Alexander Graf]

On 03.07.2013, at 20:36, Scott Wood wrote:

> On 07/03/2013 12:07:30 PM, Alexander Graf wrote:
>> On 03.07.2013, at 18:49, Caraman Mihai Claudiu-B02008 wrote:
>> >>>> Do we need to do this even when the guest doesn't use Altivec? Can't
>> >> we
>> >>>> just load it on demand then once we fault? This code path really
>> >> should
>> >>>> only be a prefetch enable when MSR_VEC is already set in guest
>> >> context.
>> >>>
>> >>> No we can't, read 6/6.
>> >>
>> >> So we have to make sure we're completely unlazy when it comes to a KVM
>> >> guest. Are we?
>> >
>> > Yes, because MSR[SPV] is under its control.
>> Oh, sure, KVM wants it unlazy. That part is obvious. But does the kernel always give us unlazyness? The way I read the code, process.c goes lazy when !CONFIG_SMP.
>> So the big question is why we're manually enforcing FPU giveup, but not Altivec giveup? One of the 2 probably is wrong :).
> 
> Why do you think we're not enforcing it for Altivec?  Is there some specific piece of code you're referring to that is different in this regard?

Well, apparently because I misread the code :). All is well.


Alex

Re: [PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Scott Wood]

On 07/03/2013 07:42:37 AM, Mihai Caraman wrote:
> Add KVM Book3E AltiVec support. KVM Book3E FPU support gracefully  
> reuse host
> infrastructure so follow the same approach for AltiVec.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c |   72  
> ++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 3cae2e3..c3c3af6 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -98,6 +98,19 @@ static inline bool kvmppc_supports_spe(void)
>  	return false;
>  }
> 
> +/*
> + * Always returns true is AltiVec unit is present, see
> + * kvmppc_core_check_processor_compat().
> + */
> +static inline bool kvmppc_supports_altivec(void)
> +{
> +#ifdef CONFIG_ALTIVEC
> +		if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +			return true;
> +#endif
> +	return false;
> +}

Whitespace.

-Scott

[PATCH 5/6] KVM: PPC: Book3E: Add ONE_REG AltiVec support

[Mihai Caraman]

Add ONE_REG support for AltiVec on Book3E.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index c3c3af6..6ac1f68 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1585,6 +1585,22 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	case KVM_REG_PPC_DEBUG_INST:
 		val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV);
 		break;
+#ifdef CONFIG_ALTIVEC
+	case KVM_REG_PPC_VR0 ... KVM_REG_PPC_VR31:
+		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
+			r = -ENXIO;
+			break;
+		}
+		val.vval = vcpu->arch.vr[reg->id - KVM_REG_PPC_VR0];
+		break;
+	case KVM_REG_PPC_VSCR:
+		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
+			r = -ENXIO;
+			break;
+		}
+		val = get_reg_val(reg->id, vcpu->arch.vscr.u[3]);
+		break;
+#endif /* CONFIG_ALTIVEC */
 	default:
 		r = kvmppc_get_one_reg(vcpu, reg->id, &val);
 		break;
@@ -1658,6 +1674,22 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 		kvmppc_set_tcr(vcpu, tcr);
 		break;
 	}
+#ifdef CONFIG_ALTIVEC
+	case KVM_REG_PPC_VR0 ... KVM_REG_PPC_VR31:
+		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
+			r = -ENXIO;
+			break;
+		}
+		vcpu->arch.vr[reg->id - KVM_REG_PPC_VR0] = val.vval;
+		break;
+	case KVM_REG_PPC_VSCR:
+		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
+			r = -ENXIO;
+			break;
+		}
+		vcpu->arch.vscr.u[3] = set_reg_val(reg->id, val);
+		break;
+#endif /* CONFIG_ALTIVEC */
 	default:
 		r = kvmppc_set_one_reg(vcpu, reg->id, &val);
 		break;
-- 
1.7.3.4

[PATCH 6/6] KVM: PPC: Book3E: Enable e6500 core

[Mihai Caraman]

Now that AltiVec support is in place enable e6500 core.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/e500mc.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 09da1ac..bec897c 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -175,6 +175,16 @@ int kvmppc_core_check_processor_compat(void)
 		r = 0;
 	else if (strcmp(cur_cpu_spec->cpu_name, "e5500") == 0)
 		r = 0;
+#ifdef CONFIG_ALTIVEC
+	/*
+	 * Since guests have the priviledge to enable AltiVec, we need AltiVec
+	 * support in the host to save/restore their context.
+	 * Don't use CPU_FTR_ALTIVEC to identify cores with AltiVec unit
+	 * because it's cleared in the absence of CONFIG_ALTIVEC!
+	 */
+	else if (strcmp(cur_cpu_spec->cpu_name, "e6500") == 0)
+		r = 0;
+#endif
 	else
 		r = -ENOTSUPP;
 
-- 
1.7.3.4