[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 at suse.de]
> > > Sent: Thursday, July 03, 2014 3:21 PM
> > > To: Caraman Mihai Claudiu-B02008; kvm-ppc at vger.kernel.org
> > > Cc: kvm at vger.kernel.org; linuxppc-dev at 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 at 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