On Wed, Jan 27, 2016 at 09:19:27PM +1100, Michael Ellerman wrote:
> Hi Torsten,
>
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1206,7 +1206,12 @@ _GLOBAL(enter_prom)
> > #ifdef CONFIG_DYNAMIC_FTRACE
> > _GLOBAL(mcount)
> > _GLOBAL(_mcount)
> > - blr
> > + std r0,LRSAVE(r1) /* gcc6 does this _after_ this call _only_ */
> > + mflr r0
> > + mtctr r0
> > + ld r0,LRSAVE(r1)
> > + mtlr r0
> > + bctr
>
> Can we use r11 instead? eg:
>
> _GLOBAL(_mcount)
> mflr r11
> mtctr r11
> mtlr r0
> bctr
>
> Otherwise I worry the std/ld is going to cause a load-hit-store. And it's just
> plain more instructions too.
>
> I don't quite grok the gcc code enough to tell if that's always safe, GCC does
> use r11 sometimes, but I don't think it ever expects it to survive across
> _mcount()?
I used r11 in that area once, and it crashed, but I don't recall the deatils.
We'll see. The performance shouldn't be critical, as the code is only used
during boot-up. With DYNAMIC_FTRACE, The calls will be replaced by
0x600000^W PPC_INST_NOP :)
> >
> > bl prepare_ftrace_return
> > nop
>
> AFAICS these end up being the only instructions shared between the two
> versions. Which I don't think is worth the semantic burden of all the #ifdefs.
> So please just write it as two separate functions, one for
> CC_USING_MPROFILE_KERNEL and one for not.
>
> > index 44d4d8e..080c525 100644
> > --- a/arch/powerpc/kernel/ftrace.c
> > +++ b/arch/powerpc/kernel/ftrace.c
> > @@ -306,11 +306,19 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned
> > long addr)
> > * The load offset is different depending on the ABI. For simplicity
> > * just mask it out when doing the compare.
> > */
> > +#ifndef CC_USING_MPROFILE_KERNEL
> > if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
> > - pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
> > + pr_err("Unexpected call sequence at %p: %x %x\n",
> > + ip, op[0], op[1]);
> > return -EINVAL;
> > }
> > -
> > +#else
> > + /* look for patched "NOP" on ppc64 with -mprofile-kernel */
> > + if (op[0] != 0x60000000) {
>
> That is "PPC_INST_NOP".
>
> > + pr_err("Unexpected call at %p: %x\n", ip, op[0]);
> > + return -EINVAL;
> > + }
> > +#endif
>
> Can you please break that out into a static inline, with separate versions for
> the two cases.
>
> We should aim for no #ifdefs inside functions.
Points taken.
Does this set _work_ for you now? That'd be great to hear.
Stay tuned for v7...
Torsten
