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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-
> owner@xxxxxxxxxxxxxxx] On Behalf Of mihai.caraman@xxxxxxxxxxxxx
> Sent: Monday, July 21, 2014 4:23 PM
> To: Alexander Graf; Wood Scott-B07421
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linuxppc-
> dev@xxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> SPE/FP/AltiVec int numbers
> 
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf@xxxxxxx]
> > Sent: Thursday, July 03, 2014 3:21 PM
> > To: Caraman Mihai Claudiu-B02008; kvm-ppc@xxxxxxxxxxxxxxx
> > Cc: kvm@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxx>
> > > ---
> > > 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).
> 
>
> 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

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?

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
 
+#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
 
        /* Performance Monitor */
        EXCEPTION(0x2060, PERFORMANCE_MONITOR, PerformanceMonitor, \
@@ -947,6 +952,7 @@ get_phys_addr:
  * Global functions
  */
 
+#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
 
+#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
 
 /* Adjust or setup IVORs for e500mc */
 _GLOBAL(__setup_e500mc_ivors)
diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/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
 
 #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[] = {
 #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,

-Mike
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux