On Fri Jun 21, 2024 at 4:54 AM AEST, Naveen N Rao wrote: > On 32-bit powerpc, gcc generates a three instruction sequence for > function profiling: > mflr r0 > stw r0, 4(r1) > bl _mcount > > On kernel boot, the call to _mcount() is nop-ed out, to be patched back > in when ftrace is actually enabled. The 'stw' instruction therefore is > not necessary unless ftrace is enabled. Nop it out during ftrace init. > > When ftrace is enabled, we want the 'stw' so that stack unwinding works > properly. Perform the same within the ftrace handler, similar to 64-bit > powerpc. > > For 64-bit powerpc, early versions of gcc used to emit a three > instruction sequence for function profiling (with -mprofile-kernel) with > a 'std' instruction to mimic the 'stw' above. Address that scenario also > by nop-ing out the 'std' instruction during ftrace init.
Cool! Could 32-bit use the 2-insn sequence as well if it had -mprofile-kernel, out of curiosity? > > Signed-off-by: Naveen N Rao <[email protected]> > --- > arch/powerpc/kernel/trace/ftrace.c | 6 ++++-- > arch/powerpc/kernel/trace/ftrace_entry.S | 4 ++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/trace/ftrace.c > b/arch/powerpc/kernel/trace/ftrace.c > index d8d6b4fd9a14..463bd7531dc8 100644 > --- a/arch/powerpc/kernel/trace/ftrace.c > +++ b/arch/powerpc/kernel/trace/ftrace.c > @@ -241,13 +241,15 @@ int ftrace_init_nop(struct module *mod, struct > dyn_ftrace *rec) > /* Expected sequence: 'mflr r0', 'stw r0,4(r1)', 'bl _mcount' */ > ret = ftrace_validate_inst(ip - 8, ppc_inst(PPC_RAW_MFLR(_R0))); > if (!ret) > - ret = ftrace_validate_inst(ip - 4, > ppc_inst(PPC_RAW_STW(_R0, _R1, 4))); > + ret = ftrace_modify_code(ip - 4, > ppc_inst(PPC_RAW_STW(_R0, _R1, 4)), > + ppc_inst(PPC_RAW_NOP())); > } else if (IS_ENABLED(CONFIG_MPROFILE_KERNEL)) { > /* Expected sequence: 'mflr r0', ['std r0,16(r1)'], 'bl > _mcount' */ > ret = ftrace_read_inst(ip - 4, &old); > if (!ret && !ppc_inst_equal(old, ppc_inst(PPC_RAW_MFLR(_R0)))) { > ret = ftrace_validate_inst(ip - 8, > ppc_inst(PPC_RAW_MFLR(_R0))); > - ret |= ftrace_validate_inst(ip - 4, > ppc_inst(PPC_RAW_STD(_R0, _R1, 16))); > + ret |= ftrace_modify_code(ip - 4, > ppc_inst(PPC_RAW_STD(_R0, _R1, 16)), > + ppc_inst(PPC_RAW_NOP())); So this is the old style path... Should you check the mflr validate result first? Also do you know what GCC version, roughly? Maybe we could have a comment here and eventually deprecate it. You could split this change into its own patch. > } > } else { > return -EINVAL; > diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S > b/arch/powerpc/kernel/trace/ftrace_entry.S > index 76dbe9fd2c0f..244a1c7bb1e8 100644 > --- a/arch/powerpc/kernel/trace/ftrace_entry.S > +++ b/arch/powerpc/kernel/trace/ftrace_entry.S > @@ -33,6 +33,8 @@ > * and then arrange for the ftrace function to be called. > */ > .macro ftrace_regs_entry allregs > + /* Save the original return address in A's stack frame */ > + PPC_STL r0, LRSAVE(r1) > /* Create a minimal stack frame for representing B */ > PPC_STLU r1, -STACK_FRAME_MIN_SIZE(r1) > > @@ -44,8 +46,6 @@ > SAVE_GPRS(3, 10, r1) > > #ifdef CONFIG_PPC64 > - /* Save the original return address in A's stack frame */ > - std r0, LRSAVE+SWITCH_FRAME_SIZE+STACK_FRAME_MIN_SIZE(r1) > /* Ok to continue? */ > lbz r3, PACA_FTRACE_ENABLED(r13) > cmpdi r3, 0 That seems right to me. Reviewed-by: Nicholas Piggin <[email protected]>
