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@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. > >>> 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. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
