Jimi Xenidis <ji...@pobox.com> wrote: > > On Dec 7, 2012, at 7:38 AM, Jimi Xenidis <ji...@pobox.com> wrote: > > > > > On Dec 6, 2012, at 11:54 PM, Michael Neuling <mi...@neuling.org> wrote: > > > >>> commit 279c0615917b959a652e81f4ad0d886e2d426d85 > >>> Author: Jimi Xenidis <ji...@pobox.com> > >>> Date: Wed Dec 5 13:43:22 2012 -0500 > >>> > >>> powerpc/book3e: IBM Blue Gene/Q Quad Processing eXtention (QPX) > >>> > >>> This enables kernel support for the QPX extention and is intended for > >>> processors that support it, usually an IBM Blue Gene processor. > >>> Turning it on does not effect other processors but it does add code > >>> and will quadruple the per thread save and restore area for the FPU > >>> (hense the name). If you have enabled VSX it will only double the > >>> space. > >>> > >>> Signed-off-by: Jimi Xenidis <ji...@pobox.com> > >> > > <<snip>> > >> > >> > >> > >> +BEGIN_FTR_SECTION \ > >> + SAVE_32VSRS(n,c,base); \ > >> +END_FTR_SECTION_IFSET(CPU_FTR_VSX); > >> \ > >> +BEGIN_FTR_SECTION \ > >> + SAVE_32QRS(n,c,base); \ > >> +END_FTR_SECTION_IFSET(CPU_FTR_QPX); > >> > >> I don't think we want to do this. We are going to end up with 64 > >> NOPS here somewhere. > > > > Excellent point, NOPs are cheap on most processors but not A2 and a lot of > > embedded, I can wrap some branches with the FTR instead. > > Do you have a concern on the code size? > > Thought about it a bit and came up with this solution for > arch/powerpc/kernel/fpu.S. > This should address the following issues > - MSR_VSX vs MSR_VEC > - Big chunks of NOPs in the code path > - Less FTR space fixups at boot time. > - IMNHSO easier to read especially when disassembled
Indeed, I think it looks better. I was going to mention that it was already pretty complex to read, so a rewrite like this was probably needed. So thanks!! That being said, there is a pretty complex testing matrix of CONFIG_VSX/VMX/FPU/QPX/SMP/64/32 CPU_FTR/VSX/FPU/QPX/VMX so I'd need to look/test more carefully to make sure all of these are covered. Also, transactional memory (see http://lists.ozlabs.org/pipermail/linuxppc-dev/2012-November/102216.html) will change this code. You should rebase on top of that if you really want it considered for upstream. Mikey > > I did consider using the LR and BLR, but the !CONFIG_SMP case only adds one > more special block and uses a different register set. > Also if this is agreeable I would like us to consider removing the > *_32FPVSRS* macros entirely and put the FTR tests in the actual code. > This would allow us to use #ifdefs and reduce the amount of code that > actually gets compiled. > > Thoughts? > > diff --git a/arch/powerpc/kernel/fpu.S b/arch/powerpc/kernel/fpu.S > index e0ada05..5964067 100644 > --- a/arch/powerpc/kernel/fpu.S > +++ b/arch/powerpc/kernel/fpu.S > @@ -25,30 +25,81 @@ > #include <asm/asm-offsets.h> > #include <asm/ptrace.h> > > -#ifdef CONFIG_VSX > -#define __REST_32FPVSRS(n,c,base) \ > -BEGIN_FTR_SECTION \ > - b 2f; \ > -END_FTR_SECTION_IFSET(CPU_FTR_VSX); \ > - REST_32FPRS(n,base); \ > - b 3f; \ > -2: REST_32VSRS(n,c,base); \ > -3: > - > -#define __SAVE_32FPVSRS(n,c,base) \ > -BEGIN_FTR_SECTION \ > - b 2f; \ > -END_FTR_SECTION_IFSET(CPU_FTR_VSX); \ > - SAVE_32FPRS(n,base); \ > - b 3f; \ > -2: SAVE_32VSRS(n,c,base); \ > -3: > -#else > -#define __REST_32FPVSRS(n,b,base) REST_32FPRS(n, base) > -#define __SAVE_32FPVSRS(n,b,base) SAVE_32FPRS(n, base) > -#endif > -#define REST_32FPVSRS(n,c,base) __REST_32FPVSRS(n,__REG_##c,__REG_##base) > -#define SAVE_32FPVSRS(n,c,base) __SAVE_32FPVSRS(n,__REG_##c,__REG_##base) > + > +/* > + * Restore subroutines, R4 is scratch and R5 is base > + */ > +vsx_restore: > + REST_32VSRS(0, __REG_R4, __REG_R5) > + b after_restore > +qpx_restore: > + REST_32QRS(0, __REG_R4, __REG_R5) > + b after_restore > +fpu_restore: > + REST_32FPRS(0, __REG_R5) > + b after_restore > + > +#define REST_32FPVSRS(n, c, base) \ > +BEGIN_FTR_SECTION \ > + b vsx_restore; \ > +END_FTR_SECTION_IFSET(CPU_FTR_VSX) \ > +BEGIN_FTR_SECTION \ > + b qpx_restore; \ > +END_FTR_SECTION_IFSET(CPU_FTR_QPX) \ > + b fpu_restore; \ > +after_restore: > + > +/* > + * Save subroutines, R4 is scratch and R3 is base > + */ > +vsx_save: > + SAVE_32VSRS(0, __REG_R4, __REG_R3) > + b after_save > +qpx_save: > + SAVE_32QRS(0, __REG_R4, __REG_R3) > + b after_save > +fpu_save: > + SAVE_32FPRS(0, __REG_R3) > + b after_save > + > +#define SAVE_32FPVSRS(n, c, base) \ > +BEGIN_FTR_SECTION \ > + b vsx_save; \ > +END_FTR_SECTION_IFSET(CPU_FTR_VSX) \ > +BEGIN_FTR_SECTION \ > + b qpx_save; \ > +END_FTR_SECTION_IFSET(CPU_FTR_QPX) \ > + b fpu_save; \ > +after_save: > + > +#ifndef CONFIG_SMP > +/* > + * we need an extra save set for the !CONFIG_SMP case, see below > + * Scratch it R5 and base is R4 > + */ > +vsx_save_nosmp: > + SAVE_32VSRS(0,R5,R4) > + b after_save_nosmp > + > +qpx_save_nosmp: > + SAVE_32QRS(0,R5,R4) > + b after_save_nosmp > + > +fpu_save_nosmp: > + SAVE_32FPRS(0,R4) > + b after_save_nosmp > + > +#define SAVE_32FPVSRS_NOSMP(n,c,base) \ > +BEGIN_FTR_SECTION \ > + b vsx_save_nosmp; \ > +END_FTR_SECTION_IFSET(CPU_FTR_VSX) \ > +BEGIN_FTR_SECTION \ > + b qpx_save_nosmp; \ > +END_FTR_SECTION_IFSET(CPU_FTR_QPX) \ > + b fpu_save_nosmp; \ > +after_save_nosmp: > + > +#endif /* !CONFIG_SMP */ > > /* > * This task wants to use the FPU now. > @@ -65,6 +116,11 @@ BEGIN_FTR_SECTION > oris r5,r5,MSR_VSX@h > END_FTR_SECTION_IFSET(CPU_FTR_VSX) > #endif > +#ifdef CONFIG_PPC_QPX > +BEGIN_FTR_SECTION > + oris r5,r5,MSR_VEC@h > +END_FTR_SECTION_IFSET(CPU_FTR_QPX) > +#endif > SYNC > MTMSRD(r5) /* enable use of fpu now */ > isync > @@ -81,7 +137,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX) > beq 1f > toreal(r4) > addi r4,r4,THREAD /* want last_task_used_math->thread */ > - SAVE_32FPVSRS(0, R5, R4) > + SAVE_32FPVSRS_NOSMP(0, R5, R4) > mffs fr0 > stfd fr0,THREAD_FPSCR(r4) > PPC_LL r5,PT_REGS(r4) > @@ -94,7 +150,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX) > #endif /* CONFIG_SMP */ > /* enable use of FP after return */ > #ifdef CONFIG_PPC32 > - mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) > */ > + mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) */ > lwz r4,THREAD_FPEXC_MODE(r5) > ori r9,r9,MSR_FP /* enable FP for current */ > or r9,r9,r4 > @@ -132,6 +188,11 @@ BEGIN_FTR_SECTION > oris r5,r5,MSR_VSX@h > END_FTR_SECTION_IFSET(CPU_FTR_VSX) > #endif > +#ifdef CONFIG_PPC_QPX > +BEGIN_FTR_SECTION > + oris r5,r5,MSR_VEC@h > +END_FTR_SECTION_IFSET(CPU_FTR_QPX) > +#endif > SYNC_601 > ISYNC_601 > MTMSRD(r5) /* enable use of fpu now */ > @@ -148,10 +209,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX) > beq 1f > PPC_LL r4,_MSR-STACK_FRAME_OVERHEAD(r5) > li r3,MSR_FP|MSR_FE0|MSR_FE1 > -#ifdef CONFIG_VSX > +#if defined(CONFIG_VSX) || defined(CONFIG_PPC_QPX) > BEGIN_FTR_SECTION > oris r3,r3,MSR_VSX@h > -END_FTR_SECTION_IFSET(CPU_FTR_VSX) > +END_FTR_SECTION_IFSET(CPU_FTR_VSX | CPU_FTR_QPX) > #endif > andc r4,r4,r3 /* disable FP for previous task */ > PPC_STL r4,_MSR-STACK_FRAME_OVERHEAD(r5) > > -jx > > > > > >> > >> I'd like to see this patch broken into different parts. > > > > I'm not sure how _this_ patch: > > > > <https://github.com/jimix/linux-bgq/commit/279c0615917b959a652e81f4ad0d886e2d426d85> > > could be broken up, please advise. > > > >> > >> Also, have you boot tested this change on a VSX enabled box? > > > > I can try, I may bug you for help. > > Is there a commonly test (or apps) I should run? > > > > -jx > > > > > >> > >> Mikey > > > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
