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

[Alexander Graf <agraf@suse.de>]

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