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 01:00, Scott Wood wrote:
> On Fri, 2014-07-04 at 00:35 +0200, Alexander Graf wrote:
>> On 04.07.14 00:31, Scott Wood wrote:
>>> 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@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.
>>> 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).
>> Yes, I want to make sure we have 2 separate code paths for SPE and
>> Altivec. No code sharing at all unless it's very generically possible.
>>
>> Also, which code does look pretty similar? The fact that we deflect
>> interrupts back into the guest? That's mostly boilerplate.
> There's also the injection of a program check (or exiting to userspace)
> when CONFIG_SPE/ALTIVEC is missing.  Not a big deal, but maybe it could
> be factored into a helper function.  I like minimizing boilerplate.

Yes, me too - but I also like to be explicit. If there's enough code to 
share, factoring those into helpers certainly works well for me.


Alex