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: 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
 
> 
> Alex
--
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