[PATCH 0/6 v2] KVM: PPC: Book3e: AltiVec support

[PATCH 0/6 v2] KVM: PPC: Book3e: AltiVec support

[Mihai Caraman]

Add KVM Book3E AltiVec support and enable e6500 core.

Integrates Paul's FP/VMX/VSX changes that landed in kvm-ppc-queue in January
and take into account feedback.

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/include/asm/kvm_asm.h    |   8 --
 arch/powerpc/include/uapi/asm/kvm.h   |   5 +
 arch/powerpc/kvm/booke.c              | 238 ++++++++++++++++++++++++++++------
 arch/powerpc/kvm/booke.h              |  38 +-----
 arch/powerpc/kvm/booke_interrupts.S   |   9 +-
 arch/powerpc/kvm/bookehv_interrupts.S |   4 +-
 arch/powerpc/kvm/e500.c               |  10 +-
 arch/powerpc/kvm/e500_emulate.c       |  10 +-
 arch/powerpc/kvm/e500mc.c             |  12 +-
 9 files changed, 232 insertions(+), 102 deletions(-)

-- 
1.7.11.7

[PATCH 1/6 v2] 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 interrupt numbers.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
v2:
 - remove outdated definitions

 arch/powerpc/include/asm/kvm_asm.h    |  8 --------
 arch/powerpc/kvm/booke.c              | 17 +++++++++--------
 arch/powerpc/kvm/booke.h              |  4 ++--
 arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
 arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
 arch/powerpc/kvm/e500.c               | 10 ++++++----
 arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
 7 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_asm.h b/arch/powerpc/include/asm/kvm_asm.h
index 9601741..c94fd33 100644
--- a/arch/powerpc/include/asm/kvm_asm.h
+++ b/arch/powerpc/include/asm/kvm_asm.h
@@ -56,14 +56,6 @@
 /* E500 */
 #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
 #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
-/*
- * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same defines
- */
-#define BOOKE_INTERRUPT_SPE_UNAVAIL BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
-#define BOOKE_INTERRUPT_SPE_FP_DATA BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
-#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
-#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
-				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
 #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
 #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
 #define BOOKE_INTERRUPT_DOORBELL 36
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ab62109..3c86d9b 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -388,8 +388,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;
@@ -977,18 +977,19 @@ 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;
 
@@ -997,7 +998,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.
@@ -1010,7 +1011,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 b632cd3..f182b32 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/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 2c6deb5ef..a275dc5 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -137,8 +137,9 @@ KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
 KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
-KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
-KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
+KVM_HANDLER BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
+KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST SPRN_SPRG_RSCRATCH0 \
+	SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 _GLOBAL(kvmppc_handlers_end)
 
@@ -525,8 +526,8 @@ KVM_HANDLER_ADDR BOOKE_INTERRUPT_WATCHDOG
 KVM_HANDLER_ADDR BOOKE_INTERRUPT_DTLB_MISS
 KVM_HANDLER_ADDR BOOKE_INTERRUPT_ITLB_MISS
 KVM_HANDLER_ADDR BOOKE_INTERRUPT_DEBUG
-KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_UNAVAIL
-KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_DATA
+KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
+KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
 KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_ROUND
 KVM_HANDLER_END /*Always keep this in end*/
 
diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
index a1712b8..ff73143 100644
--- a/arch/powerpc/kvm/bookehv_interrupts.S
+++ b/arch/powerpc/kvm/bookehv_interrupts.S
@@ -300,9 +300,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
diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
index 2e02ed8..3c1a30d 100644
--- a/arch/powerpc/kvm/e500.c
+++ b/arch/powerpc/kvm/e500.c
@@ -383,8 +383,10 @@ static int kvmppc_core_get_sregs_e500(struct kvm_vcpu *vcpu,
 	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];
@@ -414,9 +416,9 @@ static int kvmppc_core_set_sregs_e500(struct kvm_vcpu *vcpu,
 		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 98a22e5..6a6833f 100644
--- a/arch/powerpc/kvm/e500_emulate.c
+++ b/arch/powerpc/kvm/e500_emulate.c
@@ -256,10 +256,11 @@ int kvmppc_core_emulate_mtspr_e500(struct kvm_vcpu *vcpu, int sprn, ulong spr_va
 
 	/* 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;
@@ -378,10 +379,11 @@ int kvmppc_core_emulate_mfspr_e500(struct kvm_vcpu *vcpu, int sprn, ulong *spr_v
 
 	/* 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.11.7

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

[Alexander Graf]

On 30.06.14 17:34, Mihai Caraman wrote:
> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> which share the same interrupt numbers.
>
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
> v2:
>   - remove outdated definitions
>
>   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
>   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
>   arch/powerpc/kvm/booke.h              |  4 ++--
>   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
>   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
>   arch/powerpc/kvm/e500.c               | 10 ++++++----
>   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
>   7 files changed, 30 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_asm.h b/arch/powerpc/include/asm/kvm_asm.h
> index 9601741..c94fd33 100644
> --- a/arch/powerpc/include/asm/kvm_asm.h
> +++ b/arch/powerpc/include/asm/kvm_asm.h
> @@ -56,14 +56,6 @@
>   /* E500 */
>   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
>   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> -/*
> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same defines
> - */
> -#define BOOKE_INTERRUPT_SPE_UNAVAIL BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> -#define BOOKE_INTERRUPT_SPE_FP_DATA BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST

I think I'd prefer to keep them separate.

>   #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
>   #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
>   #define BOOKE_INTERRUPT_DOORBELL 36
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index ab62109..3c86d9b 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -388,8 +388,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:

#ifdef CONFIG_KVM_E500V2
   case ...SPE:
#else
   case ..ALTIVEC:
#endif

>   	case BOOKE_IRQPRIO_SPE_FP_ROUND:
>   	case BOOKE_IRQPRIO_AP_UNAVAIL:
>   		allowed = 1;
> @@ -977,18 +977,19 @@ 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;
>   
> @@ -997,7 +998,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.
> @@ -1010,7 +1011,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 b632cd3..f182b32 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

#ifdef CONFIG_KVM_E500V2
#define ...SPE...
#else
#define ...ALTIVEC...
#endif

That way we can be 100% sure that no SPE cruft leaks into anything.


>   #define BOOKE_IRQPRIO_SPE_FP_ROUND 7
>   #define BOOKE_IRQPRIO_SYSCALL 8
>   #define BOOKE_IRQPRIO_AP_UNAVAIL 9
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> index 2c6deb5ef..a275dc5 100644
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -137,8 +137,9 @@ KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
>   KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
>   KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
>   KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> -KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> -KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> +KVM_HANDLER BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> +KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST SPRN_SPRG_RSCRATCH0 \
> +	SPRN_SRR0

Same thing here - just only trap SPE when CONFIG_KVM_E500V2 is available 
and trap altivec otherwise (to make sure we always have a handler).


Alex

>   KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0
>   _GLOBAL(kvmppc_handlers_end)
>   
> @@ -525,8 +526,8 @@ KVM_HANDLER_ADDR BOOKE_INTERRUPT_WATCHDOG
>   KVM_HANDLER_ADDR BOOKE_INTERRUPT_DTLB_MISS
>   KVM_HANDLER_ADDR BOOKE_INTERRUPT_ITLB_MISS
>   KVM_HANDLER_ADDR BOOKE_INTERRUPT_DEBUG
> -KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_UNAVAIL
> -KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_DATA
> +KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> +KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>   KVM_HANDLER_ADDR BOOKE_INTERRUPT_SPE_FP_ROUND
>   KVM_HANDLER_END /*Always keep this in end*/
>   
> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
> index a1712b8..ff73143 100644
> --- a/arch/powerpc/kvm/bookehv_interrupts.S
> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
> @@ -300,9 +300,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
> diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
> index 2e02ed8..3c1a30d 100644
> --- a/arch/powerpc/kvm/e500.c
> +++ b/arch/powerpc/kvm/e500.c
> @@ -383,8 +383,10 @@ static int kvmppc_core_get_sregs_e500(struct kvm_vcpu *vcpu,
>   	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];
> @@ -414,9 +416,9 @@ static int kvmppc_core_set_sregs_e500(struct kvm_vcpu *vcpu,
>   		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 98a22e5..6a6833f 100644
> --- a/arch/powerpc/kvm/e500_emulate.c
> +++ b/arch/powerpc/kvm/e500_emulate.c
> @@ -256,10 +256,11 @@ int kvmppc_core_emulate_mtspr_e500(struct kvm_vcpu *vcpu, int sprn, ulong spr_va
>   
>   	/* 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;
> @@ -378,10 +379,11 @@ int kvmppc_core_emulate_mfspr_e500(struct kvm_vcpu *vcpu, int sprn, ulong *spr_v
>   
>   	/* 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];

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 3:21 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> SPE/FP/AltiVec int numbers
>=20
>=20
> On 30.06.14 17:34, Mihai Caraman wrote:
> > Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> > which share the same interrupt numbers.
> >
> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > ---
> > v2:
> >   - remove outdated definitions
> >
> >   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> >   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> >   arch/powerpc/kvm/booke.h              |  4 ++--
> >   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> >   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> >   arch/powerpc/kvm/e500.c               | 10 ++++++----
> >   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> >   7 files changed, 30 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_asm.h
> b/arch/powerpc/include/asm/kvm_asm.h
> > index 9601741..c94fd33 100644
> > --- a/arch/powerpc/include/asm/kvm_asm.h
> > +++ b/arch/powerpc/include/asm/kvm_asm.h
> > @@ -56,14 +56,6 @@
> >   /* E500 */
> >   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> >   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> > -/*
> > - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
> defines
> > - */
> > -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > -#define BOOKE_INTERRUPT_SPE_FP_DATA
> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> > -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>=20
> I think I'd prefer to keep them separate.

What is the reason from changing your mind from ver 1? Do you want to have
different defines with same values (we specifically mapped them to the
hardware interrupt numbers). We already upstreamed the necessary changes
in the kernel. Scott, please share your opinion here.

>=20
> >   #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> >   #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
> >   #define BOOKE_INTERRUPT_DOORBELL 36
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index ab62109..3c86d9b 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -388,8 +388,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:
>=20
> #ifdef CONFIG_KVM_E500V2
>    case ...SPE:
> #else
>    case ..ALTIVEC:
> #endif

-Mike

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

[Alexander Graf]

On 03.07.14 17:25, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 03, 2014 3:21 PM
>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
>> SPE/FP/AltiVec int numbers
>>
>>
>> On 30.06.14 17:34, Mihai Caraman wrote:
>>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
>>> which share the same interrupt numbers.
>>>
>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>> ---
>>> v2:
>>>    - remove outdated definitions
>>>
>>>    arch/powerpc/include/asm/kvm_asm.h    |  8 --------
>>>    arch/powerpc/kvm/booke.c              | 17 +++++++++--------
>>>    arch/powerpc/kvm/booke.h              |  4 ++--
>>>    arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
>>>    arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
>>>    arch/powerpc/kvm/e500.c               | 10 ++++++----
>>>    arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
>>>    7 files changed, 30 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
>> b/arch/powerpc/include/asm/kvm_asm.h
>>> index 9601741..c94fd33 100644
>>> --- a/arch/powerpc/include/asm/kvm_asm.h
>>> +++ b/arch/powerpc/include/asm/kvm_asm.h
>>> @@ -56,14 +56,6 @@
>>>    /* E500 */
>>>    #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
>>>    #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
>>> -/*
>>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
>> defines
>>> - */
>>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
>> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
>>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>> I think I'd prefer to keep them separate.
> What is the reason from changing your mind from ver 1? Do you want to have

Uh, mind to point me to an email where I said I like the approach? :)

> different defines with same values (we specifically mapped them to the
> hardware interrupt numbers). We already upstreamed the necessary changes

Yes, I think that'd end up the most readable flow of things.

> in the kernel. Scott, please share your opinion here.

I'm not going to be religious about it, but names like 
"BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST" are

   1) too long
   2) too ambiguous

It just means the code gets harder to read. Any way we can take to 
simplify the code flow is a win IMHO. And if I don't even remotely have 
to consider SPE when reading an Altivec path, I think that's a good 
thing :).


Alex

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 6:31 PM
> To: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; kvm-
> ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> SPE/FP/AltiVec int numbers
>=20
>=20
> On 03.07.14 17:25, mihai.caraman@freescale.com wrote:
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, July 03, 2014 3:21 PM
> >> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> >> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> >> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> >> SPE/FP/AltiVec int numbers
> >>
> >>
> >> On 30.06.14 17:34, Mihai Caraman wrote:
> >>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for
> SPE/FP/AltiVec
> >>> which share the same interrupt numbers.
> >>>
> >>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >>> ---
> >>> v2:
> >>>    - remove outdated definitions
> >>>
> >>>    arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> >>>    arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> >>>    arch/powerpc/kvm/booke.h              |  4 ++--
> >>>    arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> >>>    arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> >>>    arch/powerpc/kvm/e500.c               | 10 ++++++----
> >>>    arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> >>>    7 files changed, 30 insertions(+), 32 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
> >> b/arch/powerpc/include/asm/kvm_asm.h
> >>> index 9601741..c94fd33 100644
> >>> --- a/arch/powerpc/include/asm/kvm_asm.h
> >>> +++ b/arch/powerpc/include/asm/kvm_asm.h
> >>> @@ -56,14 +56,6 @@
> >>>    /* E500 */
> >>>    #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> >>>    #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> >>> -/*
> >>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use
> same
> >> defines
> >>> - */
> >>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> >> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> >>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
> >> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> >>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> >> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> >>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> >>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> >> I think I'd prefer to keep them separate.
> > What is the reason from changing your mind from ver 1? Do you want to
> have
>=20
> Uh, mind to point me to an email where I said I like the approach? :)

You tacitly approved it in this thread ... I did not say you like it :)

https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-July/108501.html

-Mike

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

[Scott Wood]

On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf@suse.de]
> > Sent: Thursday, July 03, 2014 3:21 PM
> > To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> > Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> > SPE/FP/AltiVec int numbers
> > 
> > 
> > On 30.06.14 17:34, Mihai Caraman wrote:
> > > Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> > > which share the same interrupt numbers.
> > >
> > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > > ---
> > > v2:
> > >   - remove outdated definitions
> > >
> > >   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> > >   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> > >   arch/powerpc/kvm/booke.h              |  4 ++--
> > >   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> > >   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> > >   arch/powerpc/kvm/e500.c               | 10 ++++++----
> > >   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> > >   7 files changed, 30 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/kvm_asm.h
> > b/arch/powerpc/include/asm/kvm_asm.h
> > > index 9601741..c94fd33 100644
> > > --- a/arch/powerpc/include/asm/kvm_asm.h
> > > +++ b/arch/powerpc/include/asm/kvm_asm.h
> > > @@ -56,14 +56,6 @@
> > >   /* E500 */
> > >   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> > >   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> > > -/*
> > > - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
> > defines
> > > - */
> > > -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > -#define BOOKE_INTERRUPT_SPE_FP_DATA
> > BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > > -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> > > -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > 
> > I think I'd prefer to keep them separate.
> 
> What is the reason from changing your mind from ver 1? Do you want to have
> different defines with same values (we specifically mapped them to the
> hardware interrupt numbers). We already upstreamed the necessary changes
> in the kernel. Scott, please share your opinion here.

I don't like hiding the fact that they're the same number, which could
lead to wrong code in the absence of ifdefs that strictly mutually
exclude SPE and Altivec code -- there was an instance of this with
MSR_VEC versus MSR_SPE in a previous patchset.
 
> > >   #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> > >   #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
> > >   #define BOOKE_INTERRUPT_DOORBELL 36
> > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > index ab62109..3c86d9b 100644
> > > --- a/arch/powerpc/kvm/booke.c
> > > +++ b/arch/powerpc/kvm/booke.c
> > > @@ -388,8 +388,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:
> > 
> > #ifdef CONFIG_KVM_E500V2
> >    case ...SPE:
> > #else
> >    case ..ALTIVEC:
> > #endif

I don't think that's an improvement.

-Scott

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

[Scott Wood]

On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
> On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > -----Original Message-----
> > > From: Alexander Graf [mailto:agraf@suse.de]
> > > Sent: Thursday, July 03, 2014 3:21 PM
> > > To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> > > Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> > > SPE/FP/AltiVec int numbers
> > > 
> > > 
> > > On 30.06.14 17:34, Mihai Caraman wrote:
> > > > Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> > > > which share the same interrupt numbers.
> > > >
> > > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > > > ---
> > > > v2:
> > > >   - remove outdated definitions
> > > >
> > > >   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> > > >   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> > > >   arch/powerpc/kvm/booke.h              |  4 ++--
> > > >   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> > > >   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> > > >   arch/powerpc/kvm/e500.c               | 10 ++++++----
> > > >   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> > > >   7 files changed, 30 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/include/asm/kvm_asm.h
> > > b/arch/powerpc/include/asm/kvm_asm.h
> > > > index 9601741..c94fd33 100644
> > > > --- a/arch/powerpc/include/asm/kvm_asm.h
> > > > +++ b/arch/powerpc/include/asm/kvm_asm.h
> > > > @@ -56,14 +56,6 @@
> > > >   /* E500 */
> > > >   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> > > >   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> > > > -/*
> > > > - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
> > > defines
> > > > - */
> > > > -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> > > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > > -#define BOOKE_INTERRUPT_SPE_FP_DATA
> > > BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > > > -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> > > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > > -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> > > > -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > > 
> > > I think I'd prefer to keep them separate.
> > 
> > What is the reason from changing your mind from ver 1? Do you want to have
> > different defines with same values (we specifically mapped them to the
> > hardware interrupt numbers). We already upstreamed the necessary changes
> > in the kernel. Scott, please share your opinion here.
> 
> I don't like hiding the fact that they're the same number, which could
> lead to wrong code in the absence of ifdefs that strictly mutually
> exclude SPE and Altivec code -- there was an instance of this with
> MSR_VEC versus MSR_SPE in a previous patchset. 

That said, if you want to enforce that mutual exclusion in a way that is
clear, I won't object too loudly -- but the code does look pretty
similar between the two (as well as between the two IVORs).

-Scott

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

[Alexander Graf]

On 04.07.14 00:31, Scott Wood wrote:
> On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
>> On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Thursday, July 03, 2014 3:21 PM
>>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>>>> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
>>>> SPE/FP/AltiVec int numbers
>>>>
>>>>
>>>> On 30.06.14 17:34, Mihai Caraman wrote:
>>>>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
>>>>> which share the same interrupt numbers.
>>>>>
>>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>>>> ---
>>>>> v2:
>>>>>    - remove outdated definitions
>>>>>
>>>>>    arch/powerpc/include/asm/kvm_asm.h    |  8 --------
>>>>>    arch/powerpc/kvm/booke.c              | 17 +++++++++--------
>>>>>    arch/powerpc/kvm/booke.h              |  4 ++--
>>>>>    arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
>>>>>    arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
>>>>>    arch/powerpc/kvm/e500.c               | 10 ++++++----
>>>>>    arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
>>>>>    7 files changed, 30 insertions(+), 32 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
>>>> b/arch/powerpc/include/asm/kvm_asm.h
>>>>> index 9601741..c94fd33 100644
>>>>> --- a/arch/powerpc/include/asm/kvm_asm.h
>>>>> +++ b/arch/powerpc/include/asm/kvm_asm.h
>>>>> @@ -56,14 +56,6 @@
>>>>>    /* E500 */
>>>>>    #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
>>>>>    #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
>>>>> -/*
>>>>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
>>>> defines
>>>>> - */
>>>>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
>>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
>>>> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>>>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
>>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
>>>>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>>> I think I'd prefer to keep them separate.
>>> What is the reason from changing your mind from ver 1? Do you want to have
>>> different defines with same values (we specifically mapped them to the
>>> hardware interrupt numbers). We already upstreamed the necessary changes
>>> in the kernel. Scott, please share your opinion here.
>> I don't like hiding the fact that they're the same number, which could
>> lead to wrong code in the absence of ifdefs that strictly mutually
>> exclude SPE and Altivec code -- there was an instance of this with
>> MSR_VEC versus MSR_SPE in a previous patchset.
> That said, if you want to enforce that mutual exclusion in a way that is
> clear, I won't object too loudly -- but the code does look pretty
> similar between the two (as well as between the two IVORs).

Yes, I want to make sure we have 2 separate code paths for SPE and 
Altivec. No code sharing at all unless it's very generically possible.

Also, which code does look pretty similar? The fact that we deflect 
interrupts back into the guest? That's mostly boilerplate.


Alex

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

[Scott Wood]

On Fri, 2014-07-04 at 00:35 +0200, Alexander Graf wrote:
> On 04.07.14 00:31, Scott Wood wrote:
> > On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
> >> On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Thursday, July 03, 2014 3:21 PM
> >>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> >>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> >>>> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> >>>> SPE/FP/AltiVec int numbers
> >>>>
> >>>>
> >>>> On 30.06.14 17:34, Mihai Caraman wrote:
> >>>>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> >>>>> which share the same interrupt numbers.
> >>>>>
> >>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >>>>> ---
> >>>>> v2:
> >>>>>    - remove outdated definitions
> >>>>>
> >>>>>    arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> >>>>>    arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> >>>>>    arch/powerpc/kvm/booke.h              |  4 ++--
> >>>>>    arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> >>>>>    arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> >>>>>    arch/powerpc/kvm/e500.c               | 10 ++++++----
> >>>>>    arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> >>>>>    7 files changed, 30 insertions(+), 32 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
> >>>> b/arch/powerpc/include/asm/kvm_asm.h
> >>>>> index 9601741..c94fd33 100644
> >>>>> --- a/arch/powerpc/include/asm/kvm_asm.h
> >>>>> +++ b/arch/powerpc/include/asm/kvm_asm.h
> >>>>> @@ -56,14 +56,6 @@
> >>>>>    /* E500 */
> >>>>>    #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> >>>>>    #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> >>>>> -/*
> >>>>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
> >>>> defines
> >>>>> - */
> >>>>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> >>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> >>>>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
> >>>> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> >>>>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> >>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> >>>>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> >>>>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> >>>> I think I'd prefer to keep them separate.
> >>> What is the reason from changing your mind from ver 1? Do you want to have
> >>> different defines with same values (we specifically mapped them to the
> >>> hardware interrupt numbers). We already upstreamed the necessary changes
> >>> in the kernel. Scott, please share your opinion here.
> >> I don't like hiding the fact that they're the same number, which could
> >> lead to wrong code in the absence of ifdefs that strictly mutually
> >> exclude SPE and Altivec code -- there was an instance of this with
> >> MSR_VEC versus MSR_SPE in a previous patchset.
> > That said, if you want to enforce that mutual exclusion in a way that is
> > clear, I won't object too loudly -- but the code does look pretty
> > similar between the two (as well as between the two IVORs).
> 
> Yes, I want to make sure we have 2 separate code paths for SPE and 
> Altivec. No code sharing at all unless it's very generically possible.
> 
> Also, which code does look pretty similar? The fact that we deflect 
> interrupts back into the guest? That's mostly boilerplate.

There's also the injection of a program check (or exiting to userspace)
when CONFIG_SPE/ALTIVEC is missing.  Not a big deal, but maybe it could
be factored into a helper function.  I like minimizing boilerplate.

-Scott

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

[Alexander Graf]

On 04.07.14 01:00, Scott Wood wrote:
> On Fri, 2014-07-04 at 00:35 +0200, Alexander Graf wrote:
>> On 04.07.14 00:31, Scott Wood wrote:
>>> On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
>>>> On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Thursday, July 03, 2014 3:21 PM
>>>>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>>>>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>>>>>> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
>>>>>> SPE/FP/AltiVec int numbers
>>>>>>
>>>>>>
>>>>>> On 30.06.14 17:34, Mihai Caraman wrote:
>>>>>>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
>>>>>>> which share the same interrupt numbers.
>>>>>>>
>>>>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>>     - remove outdated definitions
>>>>>>>
>>>>>>>     arch/powerpc/include/asm/kvm_asm.h    |  8 --------
>>>>>>>     arch/powerpc/kvm/booke.c              | 17 +++++++++--------
>>>>>>>     arch/powerpc/kvm/booke.h              |  4 ++--
>>>>>>>     arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
>>>>>>>     arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
>>>>>>>     arch/powerpc/kvm/e500.c               | 10 ++++++----
>>>>>>>     arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
>>>>>>>     7 files changed, 30 insertions(+), 32 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
>>>>>> b/arch/powerpc/include/asm/kvm_asm.h
>>>>>>> index 9601741..c94fd33 100644
>>>>>>> --- a/arch/powerpc/include/asm/kvm_asm.h
>>>>>>> +++ b/arch/powerpc/include/asm/kvm_asm.h
>>>>>>> @@ -56,14 +56,6 @@
>>>>>>>     /* E500 */
>>>>>>>     #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
>>>>>>>     #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
>>>>>>> -/*
>>>>>>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
>>>>>> defines
>>>>>>> - */
>>>>>>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
>>>>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>>>>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
>>>>>> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>>>>>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
>>>>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>>>>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
>>>>>>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>>>>> I think I'd prefer to keep them separate.
>>>>> What is the reason from changing your mind from ver 1? Do you want to have
>>>>> different defines with same values (we specifically mapped them to the
>>>>> hardware interrupt numbers). We already upstreamed the necessary changes
>>>>> in the kernel. Scott, please share your opinion here.
>>>> I don't like hiding the fact that they're the same number, which could
>>>> lead to wrong code in the absence of ifdefs that strictly mutually
>>>> exclude SPE and Altivec code -- there was an instance of this with
>>>> MSR_VEC versus MSR_SPE in a previous patchset.
>>> That said, if you want to enforce that mutual exclusion in a way that is
>>> clear, I won't object too loudly -- but the code does look pretty
>>> similar between the two (as well as between the two IVORs).
>> Yes, I want to make sure we have 2 separate code paths for SPE and
>> Altivec. No code sharing at all unless it's very generically possible.
>>
>> Also, which code does look pretty similar? The fact that we deflect
>> interrupts back into the guest? That's mostly boilerplate.
> There's also the injection of a program check (or exiting to userspace)
> when CONFIG_SPE/ALTIVEC is missing.  Not a big deal, but maybe it could
> be factored into a helper function.  I like minimizing boilerplate.

Yes, me too - but I also like to be explicit. If there's enough code to 
share, factoring those into helpers certainly works well for me.


Alex

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

[Alexander Graf]

On 04.07.14 00:15, Scott Wood wrote:
> On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, July 03, 2014 3:21 PM
>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>>> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
>>> SPE/FP/AltiVec int numbers
>>>
>>>
>>> On 30.06.14 17:34, Mihai Caraman wrote:
>>>> Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
>>>> which share the same interrupt numbers.
>>>>
>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>>> ---
>>>> v2:
>>>>    - remove outdated definitions
>>>>
>>>>    arch/powerpc/include/asm/kvm_asm.h    |  8 --------
>>>>    arch/powerpc/kvm/booke.c              | 17 +++++++++--------
>>>>    arch/powerpc/kvm/booke.h              |  4 ++--
>>>>    arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
>>>>    arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
>>>>    arch/powerpc/kvm/e500.c               | 10 ++++++----
>>>>    arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
>>>>    7 files changed, 30 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/kvm_asm.h
>>> b/arch/powerpc/include/asm/kvm_asm.h
>>>> index 9601741..c94fd33 100644
>>>> --- a/arch/powerpc/include/asm/kvm_asm.h
>>>> +++ b/arch/powerpc/include/asm/kvm_asm.h
>>>> @@ -56,14 +56,6 @@
>>>>    /* E500 */
>>>>    #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
>>>>    #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
>>>> -/*
>>>> - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
>>> defines
>>>> - */
>>>> -#define BOOKE_INTERRUPT_SPE_UNAVAIL
>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>> -#define BOOKE_INTERRUPT_SPE_FP_DATA
>>> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>>> -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
>>> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
>>>> -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
>>>> -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>>> I think I'd prefer to keep them separate.
>> What is the reason from changing your mind from ver 1? Do you want to have
>> different defines with same values (we specifically mapped them to the
>> hardware interrupt numbers). We already upstreamed the necessary changes
>> in the kernel. Scott, please share your opinion here.
> I don't like hiding the fact that they're the same number, which could
> lead to wrong code in the absence of ifdefs that strictly mutually
> exclude SPE and Altivec code -- there was an instance of this with
> MSR_VEC versus MSR_SPE in a previous patchset.

The nice thing here is that we use almost all of these numbers in 
switch() statements which give us automated duplicate checking - so we 
don't accidentally go into the wrong code path without knowing it.


Alex

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 3:21 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> SPE/FP/AltiVec int numbers
>=20
>=20
> On 30.06.14 17:34, Mihai Caraman wrote:
> > Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
> > which share the same interrupt numbers.
> >
> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > ---
> > v2:
> >   - remove outdated definitions
> >
> >   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> >   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> >   arch/powerpc/kvm/booke.h              |  4 ++--
> >   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> >   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> >   arch/powerpc/kvm/e500.c               | 10 ++++++----
> >   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> >   7 files changed, 30 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_asm.h
> b/arch/powerpc/include/asm/kvm_asm.h
> > index 9601741..c94fd33 100644
> > --- a/arch/powerpc/include/asm/kvm_asm.h
> > +++ b/arch/powerpc/include/asm/kvm_asm.h
> > @@ -56,14 +56,6 @@
> >   /* E500 */
> >   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> >   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> > -/*
> > - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
> defines
> > - */
> > -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > -#define BOOKE_INTERRUPT_SPE_FP_DATA
> BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> > -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
>=20
> I think I'd prefer to keep them separate.
>=20
> >   #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> >   #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
> >   #define BOOKE_INTERRUPT_DOORBELL 36
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index ab62109..3c86d9b 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -388,8 +388,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:
>=20
> #ifdef CONFIG_KVM_E500V2
>    case ...SPE:
> #else
>    case ..ALTIVEC:
> #endif
>=20
> >   	case BOOKE_IRQPRIO_SPE_FP_ROUND:
> >   	case BOOKE_IRQPRIO_AP_UNAVAIL:
> >   		allowed =3D 1;
> > @@ -977,18 +977,19 @@ 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 =3D 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 =3D RESUME_GUEST;
> >   		break;
> >
> > @@ -997,7 +998,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu,
> >   		r =3D 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.
> > @@ -1010,7 +1011,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 b632cd3..f182b32 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
>=20
> #ifdef CONFIG_KVM_E500V2
> #define ...SPE...
> #else
> #define ...ALTIVEC...
> #endif
>=20
> That way we can be 100% sure that no SPE cruft leaks into anything.
>=20
>=20
> >   #define BOOKE_IRQPRIO_SPE_FP_ROUND 7
> >   #define BOOKE_IRQPRIO_SYSCALL 8
> >   #define BOOKE_IRQPRIO_AP_UNAVAIL 9
> > diff --git a/arch/powerpc/kvm/booke_interrupts.S
> b/arch/powerpc/kvm/booke_interrupts.S
> > index 2c6deb5ef..a275dc5 100644
> > --- a/arch/powerpc/kvm/booke_interrupts.S
> > +++ b/arch/powerpc/kvm/booke_interrupts.S
> > @@ -137,8 +137,9 @@ KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG
> SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> >   KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> >   KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> >   KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT
> SPRN_CSRR0
> > -KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > -KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > +KVM_HANDLER BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL SPRN_SPRG_RSCRATCH0
> SPRN_SRR0
> > +KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> SPRN_SPRG_RSCRATCH0 \
> > +	SPRN_SRR0
>=20
> Same thing here - just only trap SPE when CONFIG_KVM_E500V2 is available
> and trap altivec otherwise (to make sure we always have a handler).

This will not even build with current kernel. 32-bit FSL kernel defines SPE
handlers for e500v2/e500mc/e5500 (see head_fsl_booke.S which is guarded by
CONFIG_FSL_BOOKE) even when CONFIG_SPE is not defined. This is simple to
verify by removing KVM's HV 32-bit BOOKE_INTERRUPT_SPE_xxx handlers from
bookehv_interrupts.S.

The kernel equivalent of your CONFIG_KVM_E500V2 suggestion looks like this:

#ifndef CONFIG_PPC_E500MC
/* e500v2 */
#define BOOKE_INTERRUPT_SPE_UNAVAIL 32
#define BOOKE_INTERRUPT_SPE_FP_DATA 33
#define BOOKE_INTERRUPT_SPE_FP_ROUND 34
#elif
/* e500mc, e5500, e6500 */
#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
#define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
#endif

but instead, the current kernel expects something like this:

#ifdef CONFIG_FSL_BOOKE
/* 32-bit targets: e500v2, e500mc, e5500 */
#define BOOKE_INTERRUPT_SPE_UNAVAIL 32
#define BOOKE_INTERRUPT_SPE_FP_DATA 33
#define BOOKE_INTERRUPT_SPE_FP_ROUND 34
#elif CONFIG_PPC_BOOK3E_64
/* 64-bit targets: e5500, e6500 */
#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
#define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
#endif

We can guard kernel SPE handlers with !CONFIG_PPC_E500MC to have
something like:

#ifdef CONFIG_FSL_BOOKE
#ifndef CONFIG_PPC_E500MC
/* e500v2 */
#define BOOKE_INTERRUPT_SPE_UNAVAIL 32
#define BOOKE_INTERRUPT_SPE_FP_DATA 33
#define BOOKE_INTERRUPT_SPE_FP_ROUND 34
#endif
#elif CONFIG_PPC_BOOK3E_64
/* e5500, e6500 */
#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
#define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
#endif

My suggestion is to go ahead with KVM AltiVec patches without waiting for
this kernel cleanup. I will do the KVM synchronization later when you will
have these kernel changes in your tree.

Scott, do you agree to guard SPE kernel handlers in head_fsl_booke.S with
!CONFIG_PPC_E500MC?

-Mike
=20
>=20
> Alex

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

[mihai.caraman]

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
> owner@vger.kernel.org] On Behalf Of mihai.caraman@freescale.com
> Sent: Monday, July 21, 2014 4:23 PM
> To: Alexander Graf; Wood Scott-B07421
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: RE: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> SPE/FP/AltiVec int numbers
>=20
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf@suse.de]
> > Sent: Thursday, July 03, 2014 3:21 PM
> > To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> > Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> > SPE/FP/AltiVec int numbers
> >
> >
> > On 30.06.14 17:34, Mihai Caraman wrote:
> > > Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for
> SPE/FP/AltiVec
> > > which share the same interrupt numbers.
> > >
> > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > > ---
> > > v2:
> > >   - remove outdated definitions
> > >
> > >   arch/powerpc/include/asm/kvm_asm.h    |  8 --------
> > >   arch/powerpc/kvm/booke.c              | 17 +++++++++--------
> > >   arch/powerpc/kvm/booke.h              |  4 ++--
> > >   arch/powerpc/kvm/booke_interrupts.S   |  9 +++++----
> > >   arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
> > >   arch/powerpc/kvm/e500.c               | 10 ++++++----
> > >   arch/powerpc/kvm/e500_emulate.c       | 10 ++++++----
> > >   7 files changed, 30 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/kvm_asm.h
> > b/arch/powerpc/include/asm/kvm_asm.h
> > > index 9601741..c94fd33 100644
> > > --- a/arch/powerpc/include/asm/kvm_asm.h
> > > +++ b/arch/powerpc/include/asm/kvm_asm.h
> > > @@ -56,14 +56,6 @@
> > >   /* E500 */
> > >   #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
> > >   #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
> > > -/*
> > > - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use
> same
> > defines
> > > - */
> > > -#define BOOKE_INTERRUPT_SPE_UNAVAIL
> > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > -#define BOOKE_INTERRUPT_SPE_FP_DATA
> > BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > > -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
> > BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
> > > -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
> > > -				BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> >
> > I think I'd prefer to keep them separate.
> >
> > >   #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> > >   #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
> > >   #define BOOKE_INTERRUPT_DOORBELL 36
> > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > index ab62109..3c86d9b 100644
> > > --- a/arch/powerpc/kvm/booke.c
> > > +++ b/arch/powerpc/kvm/booke.c
> > > @@ -388,8 +388,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:
> >
> > #ifdef CONFIG_KVM_E500V2
> >    case ...SPE:
> > #else
> >    case ..ALTIVEC:
> > #endif
> >
> > >   	case BOOKE_IRQPRIO_SPE_FP_ROUND:
> > >   	case BOOKE_IRQPRIO_AP_UNAVAIL:
> > >   		allowed =3D 1;
> > > @@ -977,18 +977,19 @@ 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 =3D 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 =3D RESUME_GUEST;
> > >   		break;
> > >
> > > @@ -997,7 +998,7 @@ int kvmppc_handle_exit(struct kvm_run *run,
> struct
> > kvm_vcpu *vcpu,
> > >   		r =3D 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.
> > > @@ -1010,7 +1011,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 b632cd3..f182b32 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
> >
> > #ifdef CONFIG_KVM_E500V2
> > #define ...SPE...
> > #else
> > #define ...ALTIVEC...
> > #endif
> >
> > That way we can be 100% sure that no SPE cruft leaks into anything.
> >
> >
> > >   #define BOOKE_IRQPRIO_SPE_FP_ROUND 7
> > >   #define BOOKE_IRQPRIO_SYSCALL 8
> > >   #define BOOKE_IRQPRIO_AP_UNAVAIL 9
> > > diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > b/arch/powerpc/kvm/booke_interrupts.S
> > > index 2c6deb5ef..a275dc5 100644
> > > --- a/arch/powerpc/kvm/booke_interrupts.S
> > > +++ b/arch/powerpc/kvm/booke_interrupts.S
> > > @@ -137,8 +137,9 @@ KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG
> > SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> > >   KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > >   KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > >   KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT
> > SPRN_CSRR0
> > > -KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0
> SPRN_SRR0
> > > -KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0
> SPRN_SRR0
> > > +KVM_HANDLER BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL SPRN_SPRG_RSCRATCH0
> > SPRN_SRR0
> > > +KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
> > SPRN_SPRG_RSCRATCH0 \
> > > +	SPRN_SRR0
> >
> >
> > Same thing here - just only trap SPE when CONFIG_KVM_E500V2 is
> available
> > and trap altivec otherwise (to make sure we always have a handler).
>=20
>
> This will not even build with current kernel. 32-bit FSL kernel defines
> SPE
> handlers for e500v2/e500mc/e5500 (see head_fsl_booke.S which is guarded
> by
> CONFIG_FSL_BOOKE) even when CONFIG_SPE is not defined. This is simple to
> verify by removing KVM's HV 32-bit BOOKE_INTERRUPT_SPE_xxx handlers from
> bookehv_interrupts.S.
>=20
> The kernel equivalent of your CONFIG_KVM_E500V2 suggestion looks like
> this:
>=20
> #ifndef CONFIG_PPC_E500MC
> /* e500v2 */
> #define BOOKE_INTERRUPT_SPE_UNAVAIL 32
> #define BOOKE_INTERRUPT_SPE_FP_DATA 33
> #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> #elif
> /* e500mc, e5500, e6500 */
> #define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
> #define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
> #endif
>=20
> but instead, the current kernel expects something like this:
>=20
> #ifdef CONFIG_FSL_BOOKE
> /* 32-bit targets: e500v2, e500mc, e5500 */
> #define BOOKE_INTERRUPT_SPE_UNAVAIL 32
> #define BOOKE_INTERRUPT_SPE_FP_DATA 33
> #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> #elif CONFIG_PPC_BOOK3E_64
> /* 64-bit targets: e5500, e6500 */
> #define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
> #define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
> #endif
>=20
> We can guard kernel SPE handlers with !CONFIG_PPC_E500MC to have
> something like:
>=20
> #ifdef CONFIG_FSL_BOOKE
> #ifndef CONFIG_PPC_E500MC
> /* e500v2 */
> #define BOOKE_INTERRUPT_SPE_UNAVAIL 32
> #define BOOKE_INTERRUPT_SPE_FP_DATA 33
> #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
> #endif
> #elif CONFIG_PPC_BOOK3E_64
> /* e5500, e6500 */
> #define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL 32
> #define BOOKE_INTERRUPT_ALTIVEC_ASSIST BOOKE_33
> #endif
>=20
> My suggestion is to go ahead with KVM AltiVec patches without waiting for
> this kernel cleanup. I will do the KVM synchronization later when you
> will
> have these kernel changes in your tree.
>=20
> Scott, do you agree to guard SPE kernel handlers in head_fsl_booke.S with
> !CONFIG_PPC_E500MC?
>=20
> -Mike

Scott, Alex's request to define SPE handlers only for e500v2 implies change=
s
in 32-bit FSL kernel to have exclusive configurations for e200/e500v2 and
e500mc/e5500. We would probably need something like this, what's your take =
on it?

diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/hea=
d_fsl_booke.S
index b497188..9d41015 100644
--- a/arch/powerpc/kernel/head_fsl_booke.S
+++ b/arch/powerpc/kernel/head_fsl_booke.S
@@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
        mfspr   r10, SPRN_SPRG_RSCRATCH0
        b       InstructionStorage
=20
+/* Define SPE handlers only for e500v2 and e200 */
+#ifndef CONFIG_PPC_E500MC
 #ifdef CONFIG_SPE
        /* SPE Unavailable */
        START_EXCEPTION(SPEUnavailable)
@@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
        EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
                  unknown_exception, EXC_XFER_EE)
 #endif /* CONFIG_SPE */
+#endif
=20
+#ifndef CONFIG_PPC_E500MC
        /* SPE Floating Point Data */
 #ifdef CONFIG_SPE
        EXCEPTION(0x2030, SPE_FP_DATA_ALTIVEC_ASSIST, SPEFloatingPointData,
@@ -641,6 +645,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
        EXCEPTION(0x2050, SPE_FP_ROUND, SPEFloatingPointRound, \
                  unknown_exception, EXC_XFER_EE)
 #endif /* CONFIG_SPE */
+#endif
=20
        /* Performance Monitor */
        EXCEPTION(0x2060, PERFORMANCE_MONITOR, PerformanceMonitor, \
@@ -947,6 +952,7 @@ get_phys_addr:
  * Global functions
  */
=20
+#ifdef CONFIG_E200
 /* Adjust or setup IVORs for e200 */
 _GLOBAL(__setup_e200_ivors)
        li      r3,DebugDebug@l
@@ -959,7 +965,9 @@ _GLOBAL(__setup_e200_ivors)
        mtspr   SPRN_IVOR34,r3
        sync
        blr
+#endif
=20
+#ifndef CONFIG_PPC_E500MC
 /* Adjust or setup IVORs for e500v1/v2 */
 _GLOBAL(__setup_e500_ivors)
        li      r3,DebugCrit@l
@@ -974,6 +982,7 @@ _GLOBAL(__setup_e500_ivors)
        mtspr   SPRN_IVOR35,r3
        sync
        blr
+#endif
=20
 /* Adjust or setup IVORs for e500mc */
 _GLOBAL(__setup_e500mc_ivors)
diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kerne=
l/cpu_setup_fsl_booke.S
index cc2d896..32afb50 100644
--- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
+++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
@@ -109,12 +109,16 @@ _GLOBAL(__setup_cpu_e6500)
        blr
=20
 #ifdef CONFIG_PPC32
+#ifdef CONFIG_E200
 _GLOBAL(__setup_cpu_e200)
        /* enable dedicated debug exception handling resources (Debug APU) =
*/
        mfspr   r3,SPRN_HID0
        ori     r3,r3,HID0_DAPUEN@l
        mtspr   SPRN_HID0,r3
        b       __setup_e200_ivors
+#endif /* CONFIG_E200 */
+#ifdef CONFIG_E500
+#ifndef CONFIG_PPC_E500MC
 _GLOBAL(__setup_cpu_e500v1)
 _GLOBAL(__setup_cpu_e500v2)
        mflr    r4
@@ -129,6 +133,8 @@ _GLOBAL(__setup_cpu_e500v2)
 #endif
        mtlr    r4
        blr
+#endif /* !CONFIG_PPC_E500MC */
+
 _GLOBAL(__setup_cpu_e500mc)
 _GLOBAL(__setup_cpu_e5500)
        mflr    r5
@@ -223,3 +229,4 @@ _GLOBAL(__setup_cpu_e5500)
        mtlr    r5
        blr
 #endif
+#endif /* CONFIG_E500 */
diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.=
c
index c1faade..3ab65c2 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] =3D {
 #endif /* CONFIG_PPC32 */
 #ifdef CONFIG_E500
 #ifdef CONFIG_PPC32
+#ifndef CONFIG_PPC_E500MC
        {       /* e500 */
                .pvr_mask               =3D 0xffff0000,
                .pvr_value              =3D 0x80200000,
@@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] =3D {
                .machine_check          =3D machine_check_e500,
                .platform               =3D "ppc8548",
        },
+#endif /* !CONFIG_PPC_E500MC */
        {       /* e500mc */
                .pvr_mask               =3D 0xffff0000,
                .pvr_value              =3D 0x80230000,

-Mike

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

[Scott Wood]

On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
> Scott, Alex's request to define SPE handlers only for e500v2 implies changes
> in 32-bit FSL kernel to have exclusive configurations for e200/e500v2 and
> e500mc/e5500. We would probably need something like this, what's your take on it?

That is already a compile-time decision.
        
> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> index b497188..9d41015 100644
> --- a/arch/powerpc/kernel/head_fsl_booke.S
> +++ b/arch/powerpc/kernel/head_fsl_booke.S
> @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
>         mfspr   r10, SPRN_SPRG_RSCRATCH0
>         b       InstructionStorage
>  
> +/* Define SPE handlers only for e500v2 and e200 */
> +#ifndef CONFIG_PPC_E500MC
>  #ifdef CONFIG_SPE
>         /* SPE Unavailable */
>         START_EXCEPTION(SPEUnavailable)
> @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
>         EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
>                   unknown_exception, EXC_XFER_EE)
>  #endif /* CONFIG_SPE */
> +#endif

This is redundant:

        config SPE
                bool "SPE Support"
                depends on E200 || (E500 && !PPC_E500MC)
                default y 

> diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> index c1faade..3ab65c2 100644
> --- a/arch/powerpc/kernel/cputable.c
> +++ b/arch/powerpc/kernel/cputable.c
> @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
>  #endif /* CONFIG_PPC32 */
>  #ifdef CONFIG_E500
>  #ifdef CONFIG_PPC32
> +#ifndef CONFIG_PPC_E500MC
>         {       /* e500 */
>                 .pvr_mask               = 0xffff0000,
>                 .pvr_value              = 0x80200000,
> @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
>                 .machine_check          = machine_check_e500,
>                 .platform               = "ppc8548",
>         },
> +#endif /* !CONFIG_PPC_E500MC */
>         {       /* e500mc */
>                 .pvr_mask               = 0xffff0000,
>                 .pvr_value              = 0x80230000,
> 

This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
e500mc gets included in !PPC_E500MC?

-Scott

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

[mihai.caraman]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0K
PiBTZW50OiBTYXR1cmRheSwgSnVseSAyNiwgMjAxNCAzOjExIEFNDQo+IFRvOiBDYXJhbWFuIE1p
aGFpIENsYXVkaXUtQjAyMDA4DQo+IENjOiBBbGV4YW5kZXIgR3JhZjsga3ZtLXBwY0B2Z2VyLmtl
cm5lbC5vcmc7IGt2bUB2Z2VyLmtlcm5lbC5vcmc7DQo+IGxpbnV4cHBjLWRldkBsaXN0cy5vemxh
YnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMS82IHYyXSBLVk06IFBQQzogQm9vazNFOiBV
c2UgY29tbW9uIGRlZmluZXMgZm9yDQo+IFNQRS9GUC9BbHRpVmVjIGludCBudW1iZXJzDQo+IA0K
PiBPbiBUaHUsIDIwMTQtMDctMjQgYXQgMDQ6MTYgLTA1MDAsIENhcmFtYW4gTWloYWkgQ2xhdWRp
dS1CMDIwMDggd3JvdGU6DQo+ID4gU2NvdHQsIEFsZXgncyByZXF1ZXN0IHRvIGRlZmluZSBTUEUg
aGFuZGxlcnMgb25seSBmb3IgZTUwMHYyIGltcGxpZXMNCj4gY2hhbmdlcw0KPiA+IGluIDMyLWJp
dCBGU0wga2VybmVsIHRvIGhhdmUgZXhjbHVzaXZlIGNvbmZpZ3VyYXRpb25zIGZvciBlMjAwL2U1
MDB2Mg0KPiBhbmQNCj4gPiBlNTAwbWMvZTU1MDAuIFdlIHdvdWxkIHByb2JhYmx5IG5lZWQgc29t
ZXRoaW5nIGxpa2UgdGhpcywgd2hhdCdzIHlvdXINCj4gdGFrZSBvbiBpdD8NCj4gDQo+IFRoYXQg
aXMgYWxyZWFkeSBhIGNvbXBpbGUtdGltZSBkZWNpc2lvbi4NCg0KWWVzLCBidXQgaXMgbm90IGZ1
bGx5IGltcGxlbWVudGVkLg0KDQo+IA0KPiA+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMva2Vy
bmVsL2hlYWRfZnNsX2Jvb2tlLlMNCj4gYi9hcmNoL3Bvd2VycGMva2VybmVsL2hlYWRfZnNsX2Jv
b2tlLlMNCj4gPiBpbmRleCBiNDk3MTg4Li45ZDQxMDE1IDEwMDY0NA0KPiA+IC0tLSBhL2FyY2gv
cG93ZXJwYy9rZXJuZWwvaGVhZF9mc2xfYm9va2UuUw0KPiA+ICsrKyBiL2FyY2gvcG93ZXJwYy9r
ZXJuZWwvaGVhZF9mc2xfYm9va2UuUw0KPiA+IEBAIC02MTMsNiArNjEzLDggQEAgRU5EX0ZUUl9T
RUNUSU9OX0lGU0VUKENQVV9GVFJfRU1CX0hWKQ0KPiA+ICAgICAgICAgbWZzcHIgICByMTAsIFNQ
Uk5fU1BSR19SU0NSQVRDSDANCj4gPiAgICAgICAgIGIgICAgICAgSW5zdHJ1Y3Rpb25TdG9yYWdl
DQo+ID4NCj4gPiArLyogRGVmaW5lIFNQRSBoYW5kbGVycyBvbmx5IGZvciBlNTAwdjIgYW5kIGUy
MDAgKi8NCj4gPiArI2lmbmRlZiBDT05GSUdfUFBDX0U1MDBNQw0KPiA+ICAjaWZkZWYgQ09ORklH
X1NQRQ0KPiA+ICAgICAgICAgLyogU1BFIFVuYXZhaWxhYmxlICovDQo+ID4gICAgICAgICBTVEFS
VF9FWENFUFRJT04oU1BFVW5hdmFpbGFibGUpDQo+ID4gQEAgLTYyNiw3ICs2MjgsOSBAQCBFTkRf
RlRSX1NFQ1RJT05fSUZTRVQoQ1BVX0ZUUl9FTUJfSFYpDQo+ID4gICAgICAgICBFWENFUFRJT04o
MHgyMDIwLCBTUEVfQUxUSVZFQ19VTkFWQUlMLCBTUEVVbmF2YWlsYWJsZSwgXA0KPiA+ICAgICAg
ICAgICAgICAgICAgIHVua25vd25fZXhjZXB0aW9uLCBFWENfWEZFUl9FRSkNCj4gPiAgI2VuZGlm
IC8qIENPTkZJR19TUEUgKi8NCj4gPiArI2VuZGlmDQo+IA0KPiBUaGlzIGlzIHJlZHVuZGFudDoN
Cj4gDQo+ICAgICAgICAgY29uZmlnIFNQRQ0KPiAgICAgICAgICAgICAgICAgYm9vbCAiU1BFIFN1
cHBvcnQiDQo+ICAgICAgICAgICAgICAgICBkZXBlbmRzIG9uIEUyMDAgfHwgKEU1MDAgJiYgIVBQ
Q19FNTAwTUMpDQo+ICAgICAgICAgICAgICAgICBkZWZhdWx0IHkNCg0KSSBzZWUgd2hhdCB5b3Ug
bWVhbiwgYnV0IGl0J3Mgbm90IHJlZHVuZGFudC4gQWxleCB3YXMgbG9va2luZyB0byByZW1vdmUg
U1BFDQpoYW5kbGVycyBmb3IgZTUwMG1jKyBhbmQgdGhlIHByb3Bvc2FsIGhhbmRsZWQgIVNQRSBj
YXNlLiBJbiB0aGUgbmV3DQpsaWdodCBJIGZpbmQgdGhpcyB2YXJpYW50IG1vcmUgcmVhZGFibGU6
DQoNCisvKiBEZWZpbmUgU1BFIGhhbmRsZXJzIGZvciBlMjAwIGFuZCBlNTAwdjIgKi8NCiAjaWZk
ZWYgQ09ORklHX1NQRQ0KICAgICAgICAvKiBTUEUgVW5hdmFpbGFibGUgKi8NCiAgICAgICAgU1RB
UlRfRVhDRVBUSU9OKFNQRVVuYXZhaWxhYmxlKQ0KQEAgLTYyMiwxMSArNjIzLDEzIEBAIEVORF9G
VFJfU0VDVElPTl9JRlNFVChDUFVfRlRSX0VNQl9IVikNCiAgICAgICAgYiAgICAgICBmYXN0X2V4
Y2VwdGlvbl9yZXR1cm4NCiAxOiAgICAgYWRkaSAgICByMyxyMSxTVEFDS19GUkFNRV9PVkVSSEVB
RA0KICAgICAgICBFWENfWEZFUl9FRV9MSVRFKDB4MjAxMCwgS2VybmVsU1BFKQ0KLSNlbHNlDQor
I2VsaWYgZGVmaW5lZChDT05GSUdfRTIwMCkgfHwgXA0KKyAgICAgIChkZWZpbmVkKENPTkZJR19F
NTAwKSAmJiAhZGVmaW5lZChDT05GSUdfUFBDX0U1MDBNQykpDQogICAgICAgIEVYQ0VQVElPTigw
eDIwMjAsIFNQRV9BTFRJVkVDX1VOQVZBSUwsIFNQRVVuYXZhaWxhYmxlLCBcDQogICAgICAgICAg
ICAgICAgICB1bmtub3duX2V4Y2VwdGlvbiwgRVhDX1hGRVJfRUUpDQogI2VuZGlmIC8qIENPTkZJ
R19TUEUgKi8NCg0KPiANCj4gPiBkaWZmIC0tZ2l0IGEvYXJjaC9wb3dlcnBjL2tlcm5lbC9jcHV0
YWJsZS5jDQo+IGIvYXJjaC9wb3dlcnBjL2tlcm5lbC9jcHV0YWJsZS5jDQo+ID4gaW5kZXggYzFm
YWFkZS4uM2FiNjVjMiAxMDA2NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMva2VybmVsL2NwdXRh
YmxlLmMNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMva2VybmVsL2NwdXRhYmxlLmMNCj4gPiBAQCAt
MjAzMCw2ICsyMDMwLDcgQEAgc3RhdGljIHN0cnVjdCBjcHVfc3BlYyBfX2luaXRkYXRhIGNwdV9z
cGVjc1tdID0gew0KPiA+ICAjZW5kaWYgLyogQ09ORklHX1BQQzMyICovDQo+ID4gICNpZmRlZiBD
T05GSUdfRTUwMA0KPiA+ICAjaWZkZWYgQ09ORklHX1BQQzMyDQo+ID4gKyNpZm5kZWYgQ09ORklH
X1BQQ19FNTAwTUMNCj4gPiAgICAgICAgIHsgICAgICAgLyogZTUwMCAqLw0KPiA+ICAgICAgICAg
ICAgICAgICAucHZyX21hc2sgICAgICAgICAgICAgICA9IDB4ZmZmZjAwMDAsDQo+ID4gICAgICAg
ICAgICAgICAgIC5wdnJfdmFsdWUgICAgICAgICAgICAgID0gMHg4MDIwMDAwMCwNCj4gPiBAQCAt
MjA2OSw2ICsyMDcwLDcgQEAgc3RhdGljIHN0cnVjdCBjcHVfc3BlYyBfX2luaXRkYXRhIGNwdV9z
cGVjc1tdID0gew0KPiA+ICAgICAgICAgICAgICAgICAubWFjaGluZV9jaGVjayAgICAgICAgICA9
IG1hY2hpbmVfY2hlY2tfZTUwMCwNCj4gPiAgICAgICAgICAgICAgICAgLnBsYXRmb3JtICAgICAg
ICAgICAgICAgPSAicHBjODU0OCIsDQo+ID4gICAgICAgICB9LA0KPiA+ICsjZW5kaWYgLyogIUNP
TkZJR19QUENfRTUwME1DICovDQo+ID4gICAgICAgICB7ICAgICAgIC8qIGU1MDBtYyAqLw0KPiA+
ICAgICAgICAgICAgICAgICAucHZyX21hc2sgICAgICAgICAgICAgICA9IDB4ZmZmZjAwMDAsDQo+
ID4gICAgICAgICAgICAgICAgIC5wdnJfdmFsdWUgICAgICAgICAgICAgID0gMHg4MDIzMDAwMCwN
Cj4gPg0KPiANCj4gVGhpcyBsb29rcyBhIGJpdCBzdHJhbmdlIC0tIGU1MDB2MiBnZXRzIGV4Y2x1
ZGVkIGlmIFBQQ19FNTAwTUMsIGJ1dA0KPiBlNTAwbWMgZ2V0cyBpbmNsdWRlZCBpbiAhUFBDX0U1
MDBNQz8NCg0KUmlnaHQsIG15IG1haW4gcHVycG9zZSB3YXMgdG8gZ2V0IHJpZCBvZiBfX3NldHVw
X2U1MDBfaXZvcnMgb24gUFBDX0U1MDBNQw0Kd2hpY2ggcmVmZXJzIFNQRVVuYXZhaWxhYmxlLiBJ
IHdpbGwgYWRkIGFuICNlbHNlIHRvIGV4Y2x1ZGUgZTUwMG1jLg0KDQpUaGUgImdlbmVyaWMgRTUw
MCBQUEMiIGRlZmF1bHQgY3B1IGFkdmVydGlzZXMgUFBDX0ZFQVRVUkVfSEFTX1NQRV9DT01QLg0K
RG8gd2Ugd2FudCB0byBleGNsdWRlZCBpdCBpZiBQUENfRTUwME1DPyBJcyB0aGlzIGNwdSB1c2Vm
dWwgd2l0aG91dCBjcHVfc2V0dXAoKQ0KYW5kIGlmIHNvIHdoeSBkb24ndCB3ZSBhbHNvIGhhdmUg
YSBkZWZhdWx0IGZvciA2NC1iaXQ/DQoNCi1NaWtlDQo=

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

[Scott Wood]

On Mon, 2014-07-28 at 03:54 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Saturday, July 26, 2014 3:11 AM
> > To: Caraman Mihai Claudiu-B02008
> > Cc: Alexander Graf; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> > SPE/FP/AltiVec int numbers
> > 
> > On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > Scott, Alex's request to define SPE handlers only for e500v2 implies
> > changes
> > > in 32-bit FSL kernel to have exclusive configurations for e200/e500v2
> > and
> > > e500mc/e5500. We would probably need something like this, what's your
> > take on it?
> > 
> > That is already a compile-time decision.
> 
> Yes, but is not fully implemented.

We might be missing some kconfig language to prohibit e500v2 boards from
being enabled in an e500mc kernel, but if you try to do so it won't work
(except for obsolete hacks like running QEMU's mpc8544ds platform with
"-cpu e500mc").

> > > diff --git a/arch/powerpc/kernel/head_fsl_booke.S
> > b/arch/powerpc/kernel/head_fsl_booke.S
> > > index b497188..9d41015 100644
> > > --- a/arch/powerpc/kernel/head_fsl_booke.S
> > > +++ b/arch/powerpc/kernel/head_fsl_booke.S
> > > @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> > >         mfspr   r10, SPRN_SPRG_RSCRATCH0
> > >         b       InstructionStorage
> > >
> > > +/* Define SPE handlers only for e500v2 and e200 */
> > > +#ifndef CONFIG_PPC_E500MC
> > >  #ifdef CONFIG_SPE
> > >         /* SPE Unavailable */
> > >         START_EXCEPTION(SPEUnavailable)
> > > @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> > >         EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
> > >                   unknown_exception, EXC_XFER_EE)
> > >  #endif /* CONFIG_SPE */
> > > +#endif
> > 
> > This is redundant:
> > 
> >         config SPE
> >                 bool "SPE Support"
> >                 depends on E200 || (E500 && !PPC_E500MC)
> >                 default y
> 
> I see what you mean, but it's not redundant.

OK, I didn't realize there was an #else that wasn't included in the
context.  It would have been nice if the comment at the end were
"!CONFIG_SPE" rather than "CONFIG_SPE".

> Alex was looking to remove SPE
> handlers for e500mc+ and the proposal handled !SPE case. In the new
> light I find this variant more readable:
> 
> +/* Define SPE handlers for e200 and e500v2 */
>  #ifdef CONFIG_SPE
>         /* SPE Unavailable */
>         START_EXCEPTION(SPEUnavailable)
> @@ -622,11 +623,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
>         b       fast_exception_return
>  1:     addi    r3,r1,STACK_FRAME_OVERHEAD
>         EXC_XFER_EE_LITE(0x2010, KernelSPE)
> -#else
> +#elif defined(CONFIG_E200) || \
> +      (defined(CONFIG_E500) && !defined(CONFIG_PPC_E500MC))
>         EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
>                   unknown_exception, EXC_XFER_EE)
>  #endif /* CONFIG_SPE */

Yes, or better define a CONFIG_SPE_POSSIBLE so that the list only has to
exist in one place, and the intent is clearer.
 
> > > diff --git a/arch/powerpc/kernel/cputable.c
> > b/arch/powerpc/kernel/cputable.c
> > > index c1faade..3ab65c2 100644
> > > --- a/arch/powerpc/kernel/cputable.c
> > > +++ b/arch/powerpc/kernel/cputable.c
> > > @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
> > >  #endif /* CONFIG_PPC32 */
> > >  #ifdef CONFIG_E500
> > >  #ifdef CONFIG_PPC32
> > > +#ifndef CONFIG_PPC_E500MC
> > >         {       /* e500 */
> > >                 .pvr_mask               = 0xffff0000,
> > >                 .pvr_value              = 0x80200000,
> > > @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
> > >                 .machine_check          = machine_check_e500,
> > >                 .platform               = "ppc8548",
> > >         },
> > > +#endif /* !CONFIG_PPC_E500MC */
> > >         {       /* e500mc */
> > >                 .pvr_mask               = 0xffff0000,
> > >                 .pvr_value              = 0x80230000,
> > >
> > 
> > This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
> > e500mc gets included in !PPC_E500MC?
> 
> Right, my main purpose was to get rid of __setup_e500_ivors on PPC_E500MC
> which refers SPEUnavailable. I will add an #else to exclude e500mc.
> 
> The "generic E500 PPC" default cpu advertises PPC_FEATURE_HAS_SPE_COMP.
> Do we want to excluded it if PPC_E500MC? Is this cpu useful without cpu_setup()
> and if so why don't we also have a default for 64-bit?

I don't think that default will do anything useful.

-Scott

[PATCH 2/6 v2] 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 on the same flow. Add kvmppc_supports_spe() to detect
suport for the unit at runtime since it can be configured in the kernel but not
featured on hardware and vice versa.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
v2:
 - enable SPE only if !HV && SPE

 arch/powerpc/kvm/booke.c | 93 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 3c86d9b..80cd8df 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -91,6 +91,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)
 {
@@ -976,49 +985,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;
+
+#if !defined(CONFIG_KVM_BOOKE_HV) && defined(CONFIG_SPE)
+			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.
+			 */
+			pr_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.
+			 */
+			pr_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,
-- 
1.7.11.7

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

[Alexander Graf]

On 30.06.14 17:34, Mihai Caraman wrote:
> SPE/FP/AltiVec interrupts share the same numbers. Refactor SPE/FP exit handling
> to accommodate AltiVec later on the same flow. Add kvmppc_supports_spe() to detect
> suport for the unit at runtime since it can be configured in the kernel but not
> featured on hardware and vice versa.
>
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>

Why not keep them 100% separate?


Alex

[PATCH 3/6 v2] 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>
---
v2:
 - remove fpu_active
 - add descriptive comments

 arch/powerpc/kvm/booke.c  | 43 ++++++++++++++++++++++++++++++++++++-------
 arch/powerpc/kvm/booke.h  | 34 ----------------------------------
 arch/powerpc/kvm/e500mc.c |  2 --
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 80cd8df..4cc9b26 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -134,6 +134,40 @@ static void kvmppc_vcpu_sync_spe(struct kvm_vcpu *vcpu)
 }
 #endif
 
+/*
+ * Load up guest vcpu FP state if it's needed.
+ * It also set the MSR_FP in thread so that host know
+ * we're holding FPU, and then host can help to save
+ * guest vcpu FP state if other threads require to use FPU.
+ * This simulates an FP unavailable fault.
+ *
+ * It requires to be called with preemption disabled.
+ */
+static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu)
+{
+#ifdef CONFIG_PPC_FPU
+	if (!(current->thread.regs->msr & MSR_FP)) {
+		enable_kernel_fp();
+		load_fp_state(&vcpu->arch.fp);
+		current->thread.fp_save_area = &vcpu->arch.fp;
+		current->thread.regs->msr |= MSR_FP;
+	}
+#endif
+}
+
+/*
+ * Save guest vcpu FP state into thread.
+ * It requires to be called with preemption disabled.
+ */
+static inline void kvmppc_save_guest_fp(struct kvm_vcpu *vcpu)
+{
+#ifdef CONFIG_PPC_FPU
+	if (current->thread.regs->msr & MSR_FP)
+		giveup_fpu(current);
+	current->thread.fp_save_area = NULL;
+#endif
+}
+
 static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 {
 #if defined(CONFIG_PPC_FPU) && !defined(CONFIG_KVM_BOOKE_HV)
@@ -710,12 +744,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 
 	/*
 	 * Since we can't trap on MSR_FP in GS-mode, we consider the guest
-	 * as always using the FPU.  Kernel usage of FP (via
-	 * enable_kernel_fp()) in this thread must not occur while
-	 * vcpu->fpu_active is set.
+	 * as always using the FPU.
 	 */
-	vcpu->fpu_active = 1;
-
 	kvmppc_load_guest_fp(vcpu);
 #endif
 
@@ -739,8 +769,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 
 #ifdef CONFIG_PPC_FPU
 	kvmppc_save_guest_fp(vcpu);
-
-	vcpu->fpu_active = 0;
 #endif
 
 out:
@@ -1220,6 +1248,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		else {
 			/* interrupts now hard-disabled */
 			kvmppc_fix_ee_before_entry();
+			kvmppc_load_guest_fp(vcpu);
 		}
 	}
 
diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
index f182b32..faad8af 100644
--- a/arch/powerpc/kvm/booke.h
+++ b/arch/powerpc/kvm/booke.h
@@ -123,40 +123,6 @@ extern int kvmppc_core_emulate_mtspr_e500(struct kvm_vcpu *vcpu, int sprn,
 extern int kvmppc_core_emulate_mfspr_e500(struct kvm_vcpu *vcpu, int sprn,
 					  ulong *spr_val);
 
-/*
- * Load up guest vcpu FP state if it's needed.
- * It also set the MSR_FP in thread so that host know
- * we're holding FPU, and then host can help to save
- * guest vcpu FP state if other threads require to use FPU.
- * This simulates an FP unavailable fault.
- *
- * It requires to be called with preemption disabled.
- */
-static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu)
-{
-#ifdef CONFIG_PPC_FPU
-	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
-		enable_kernel_fp();
-		load_fp_state(&vcpu->arch.fp);
-		current->thread.fp_save_area = &vcpu->arch.fp;
-		current->thread.regs->msr |= MSR_FP;
-	}
-#endif
-}
-
-/*
- * Save guest vcpu FP state into thread.
- * It requires to be called with preemption disabled.
- */
-static inline void kvmppc_save_guest_fp(struct kvm_vcpu *vcpu)
-{
-#ifdef CONFIG_PPC_FPU
-	if (vcpu->fpu_active && (current->thread.regs->msr & MSR_FP))
-		giveup_fpu(current);
-	current->thread.fp_save_area = NULL;
-#endif
-}
-
 static inline void kvmppc_clear_dbsr(void)
 {
 	mtspr(SPRN_DBSR, mfspr(SPRN_DBSR));
diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 690499d..c60b653 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -145,8 +145,6 @@ static void kvmppc_core_vcpu_load_e500mc(struct kvm_vcpu *vcpu, int cpu)
 		kvmppc_e500_tlbil_all(vcpu_e500);
 		__get_cpu_var(last_vcpu_of_lpid)[vcpu->kvm->arch.lpid] = vcpu;
 	}
-
-	kvmppc_load_guest_fp(vcpu);
 }
 
 static void kvmppc_core_vcpu_put_e500mc(struct kvm_vcpu *vcpu)
-- 
1.7.11.7

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

[Alexander Graf]

On 30.06.14 17:34, 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.

How do you handle context switching with this patch applied? During most 
of the guest's lifetime we never exit kvmppc_vcpu_run(), so when the 
guest gets switched out all FPU state gets lost?


Alex

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 3:29 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 3/6 v2] KVM: PPC: Book3E: Increase FPU laziness
>=20
>=20
> On 30.06.14 17:34, 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.
>
> How do you handle context switching with this patch applied? During most
> of the guest's lifetime we never exit kvmppc_vcpu_run(), so when the
> guest gets switched out all FPU state gets lost?

No, we had this discussion in ver 1. The FP/VMX/VSX is implemented lazy in
the kernel i.e. the unit state is not saved/restored until another thread
that once claimed the unit is sched in.

Since FP/VMX/VSX can be activated by the guest independent of the host, the
vcpu thread is always using the unit (even if it did not claimed it once).

Now, this patch optimize the sched in flow. Instead of checking on each vcp=
u
sched in if the kernel unloaded unit's guest state for another competing ho=
st
process we do this when we enter the guest.

-Mike

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

[Alexander Graf]

On 03.07.14 17:46, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 03, 2014 3:29 PM
>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [PATCH 3/6 v2] KVM: PPC: Book3E: Increase FPU laziness
>>
>>
>> On 30.06.14 17:34, 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.
>> How do you handle context switching with this patch applied? During most
>> of the guest's lifetime we never exit kvmppc_vcpu_run(), so when the
>> guest gets switched out all FPU state gets lost?
> No, we had this discussion in ver 1. The FP/VMX/VSX is implemented lazy in
> the kernel i.e. the unit state is not saved/restored until another thread
> that once claimed the unit is sched in.
>
> Since FP/VMX/VSX can be activated by the guest independent of the host, the
> vcpu thread is always using the unit (even if it did not claimed it once).
>
> Now, this patch optimize the sched in flow. Instead of checking on each vcpu
> sched in if the kernel unloaded unit's guest state for another competing host
> process we do this when we enter the guest.

But we only do it when we enter the guest from QEMU, not when we enter 
the guest after a context switch on cond_resched(), no?


Alex

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

[Alexander Graf]

On 04.07.14 09:46, Alexander Graf wrote:
>
> On 03.07.14 17:46, mihai.caraman@freescale.com wrote:
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, July 03, 2014 3:29 PM
>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>>> Subject: Re: [PATCH 3/6 v2] KVM: PPC: Book3E: Increase FPU laziness
>>>
>>>
>>> On 30.06.14 17:34, 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.
>>> How do you handle context switching with this patch applied? During 
>>> most
>>> of the guest's lifetime we never exit kvmppc_vcpu_run(), so when the
>>> guest gets switched out all FPU state gets lost?
>> No, we had this discussion in ver 1. The FP/VMX/VSX is implemented 
>> lazy in
>> the kernel i.e. the unit state is not saved/restored until another 
>> thread
>> that once claimed the unit is sched in.
>>
>> Since FP/VMX/VSX can be activated by the guest independent of the 
>> host, the
>> vcpu thread is always using the unit (even if it did not claimed it 
>> once).
>>
>> Now, this patch optimize the sched in flow. Instead of checking on 
>> each vcpu
>> sched in if the kernel unloaded unit's guest state for another 
>> competing host
>> process we do this when we enter the guest.
>
> But we only do it when we enter the guest from QEMU, not when we enter 
> the guest after a context switch on cond_resched(), no?

Ah, I missed the call to the load function in handle_exit(). Ok, I think 
that approach should work.


Alex

[PATCH 4/6 v2] 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>
---
v2:
 - integrate Paul's FP/VMX/VSX changes

 arch/powerpc/kvm/booke.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 4cc9b26..4ba75f6 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -100,6 +100,19 @@ static inline bool kvmppc_supports_spe(void)
 	return false;
 }
 
+/*
+ * Always returns true if 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)
 {
@@ -178,6 +191,40 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 
+/*
+ * 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)
+{
+#ifdef CONFIG_ALTIVEC
+	if (kvmppc_supports_altivec()) {
+		if (!(current->thread.regs->msr & MSR_VEC)) {
+			enable_kernel_altivec();
+			load_vr_state(&vcpu->arch.vr);
+			current->thread.vr_save_area = &vcpu->arch.vr;
+			current->thread.regs->msr |= MSR_VEC;
+		}
+	}
+#endif
+}
+
+/*
+ * Save guest vcpu AltiVec state into thread.
+ * It requires to be called with preemption disabled.
+ */
+static inline void kvmppc_save_guest_altivec(struct kvm_vcpu *vcpu)
+{
+#ifdef CONFIG_ALTIVEC
+	if (kvmppc_supports_altivec()) {
+		if (current->thread.regs->msr & MSR_VEC)
+			giveup_altivec(current);
+		current->thread.vr_save_area = NULL;
+	}
+#endif
+}
+
 static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
 {
 	/* Synchronize guest's desire to get debug interrupts into shadow MSR */
@@ -749,6 +796,17 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	kvmppc_load_guest_fp(vcpu);
 #endif
 
+#ifdef CONFIG_ALTIVEC
+	/* Save userspace AltiVec state in stack */
+	if (kvmppc_supports_altivec())
+		enable_kernel_altivec();
+	/*
+	 * Since we can't trap on MSR_VEC in GS-mode, we consider the guest
+	 * as always using the AltiVec.
+	 */
+	kvmppc_load_guest_altivec(vcpu);
+#endif
+
 	/* Switch to guest debug context */
 	debug = vcpu->arch.shadow_dbg_reg;
 	switch_booke_debug_regs(&debug);
@@ -771,6 +829,10 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	kvmppc_save_guest_fp(vcpu);
 #endif
 
+#ifdef CONFIG_ALTIVEC
+	kvmppc_save_guest_altivec(vcpu);
+#endif
+
 out:
 	vcpu->mode = OUTSIDE_GUEST_MODE;
 	return ret;
@@ -1014,7 +1076,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_spe() || kvmppc_supports_altivec()) {
 			bool enabled = false;
 
 #if !defined(CONFIG_KVM_BOOKE_HV) && defined(CONFIG_SPE)
@@ -1040,7 +1102,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_spe() || kvmppc_supports_altivec()) {
 			kvmppc_booke_queue_irqprio(vcpu,
 				BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST);
 			r = RESUME_GUEST;
@@ -1249,6 +1311,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			/* interrupts now hard-disabled */
 			kvmppc_fix_ee_before_entry();
 			kvmppc_load_guest_fp(vcpu);
+			kvmppc_load_guest_altivec(vcpu);
 		}
 	}
 
-- 
1.7.11.7

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

[Alexander Graf]

On 30.06.14 17:34, 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>

Same comment here - I fail to see how we refetch Altivec state after a 
context switch.


Alex

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 3:32 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 4/6 v2] KVM: PPC: Book3E: Add AltiVec support
>=20
>=20
> On 30.06.14 17:34, 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>
>=20
> Same comment here - I fail to see how we refetch Altivec state after a
> context switch.

See previous comment. I also run my usual Altivec stress test consisting in
a guest and host process running affine to a physical core an competing for
the same unit's resources using different data sets.

-Mike

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

[Scott Wood]

On Mon, 2014-06-30 at 18:34 +0300, 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>
> ---
> v2:
>  - integrate Paul's FP/VMX/VSX changes
> 
>  arch/powerpc/kvm/booke.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)

I had to apply the whole patchset to get proper context for reviewing
this, and found some nits:

>         case BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST:
>                 if (kvmppc_supports_spe() || kvmppc_supports_altivec()) {
>                         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.
>                          */

Besides the comment not being updated for Altivec, it's not true on HV,
where the guest can enable MSR[VEC] all by itself.  For HV, the reason
we shouldn't be able to get here is that we disable KVM on e6500 if
CONFIG_ALTIVEC is not enabled, and no other HV core supports either SPE
or Altivec.

>                         pr_crit("%s: unexpected SPE interrupt %u at %08lx\n",
>                                 __func__, exit_nr, vcpu->arch.pc);

Error string will say SPE regardless of what sort of chip you're on.
Given that this is explicitly on the "no support for Altivec or SPE"
path, "SPE/Altivec" phrasing seems appropriate.  Of course we have
bigger problems than that if we ever reach this code. :-)

-Scott

[PATCH 5/6 v2] 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>
---
v2:
 - add comment describing VCSR register representation in KVM vs kernel

 arch/powerpc/include/uapi/asm/kvm.h |  5 +++++
 arch/powerpc/kvm/booke.c            | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index 2bc4a94..3adbce4 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -476,6 +476,11 @@ struct kvm_get_htab_header {
 
 /* FP and vector status/control registers */
 #define KVM_REG_PPC_FPSCR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x80)
+/*
+ * VSCR register is documented as a 32-bit register in the ISA, but it can
+ * only be accesses via a vector register. Expose VSCR as a 32-bit register
+ * even though the kernel represents it as a 128-bit vector.
+ */
 #define KVM_REG_PPC_VSCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x81)
 
 /* Virtual processor areas */
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 4ba75f6..fe15a94 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1634,6 +1634,23 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	case KVM_REG_PPC_VRSAVE:
 		val = get_reg_val(reg->id, vcpu->arch.vrsave);
 		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.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.vr.vscr.u[3]);
+		break;
+#endif /* CONFIG_ALTIVEC */
+
 	default:
 		r = vcpu->kvm->arch.kvm_ops->get_one_reg(vcpu, reg->id, &val);
 		break;
@@ -1717,6 +1734,23 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	case KVM_REG_PPC_VRSAVE:
 		vcpu->arch.vrsave = set_reg_val(reg->id, val);
 		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.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.vr.vscr.u[3] = set_reg_val(reg->id, val);
+		break;
+#endif /* CONFIG_ALTIVEC */
+
 	default:
 		r = vcpu->kvm->arch.kvm_ops->set_one_reg(vcpu, reg->id, &val);
 		break;
-- 
1.7.11.7

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

[Alexander Graf]

On 30.06.14 17:34, Mihai Caraman wrote:
> Add ONE_REG support for AltiVec on Book3E.
>
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>

Any chance we can handle these in generic code?


Alex

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

[mihai.caraman]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 03, 2014 3:34 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 5/6 v2] KVM: PPC: Book3E: Add ONE_REG AltiVec support
>=20
>=20
> On 30.06.14 17:34, Mihai Caraman wrote:
> > Add ONE_REG support for AltiVec on Book3E.
> >
> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>=20
> Any chance we can handle these in generic code?

I expected this request :) Can we let this for a second phase to have
e6500 enabled first?

Can you share with us a Book3S setup so I can validate the requested
changes? I already fell anxious touching strange hardware specific
Book3S code without running it.

-Mike

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

[Alexander Graf]

On 03.07.14 18:11, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 03, 2014 3:34 PM
>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [PATCH 5/6 v2] KVM: PPC: Book3E: Add ONE_REG AltiVec support
>>
>>
>> On 30.06.14 17:34, Mihai Caraman wrote:
>>> Add ONE_REG support for AltiVec on Book3E.
>>>
>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>> Any chance we can handle these in generic code?
> I expected this request :) Can we let this for a second phase to have
> e6500 enabled first?

I don't see the value of duplicating code in e500 specific code only to 
remove and combine it in common code in a follow-up patch after that.

> Can you share with us a Book3S setup so I can validate the requested
> changes? I already fell anxious touching strange hardware specific
> Book3S code without running it.

Until a few weeks ago I had an externally reachable G5 machine that we 
could've used for this. Unfortunately I had to replace the box with 
another one that's not quite as stable. I'll try and see if I can fix or 
replace it soon.


Alex

[PATCH 6/6 v2] 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>
---
v2:
 - no changes

 arch/powerpc/kvm/e500mc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index c60b653..0bc9684 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -177,6 +177,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.11.7