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

[Alexander Graf <agraf@suse.de>]

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