Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

Michael Ellerman wrote:

Christophe Leroy [off-list ref] writes:

quoted

Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :

quoted

Christophe Leroy wrote:

quoted

Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.

Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/Kconfig                 |  1 +
 arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
 arch/powerpc/include/asm/livepatch.h |  4 +---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -210,6 +210,7 @@ config PPC

     select HAVE_DEBUG_KMEMLEAK
     select HAVE_DEBUG_STACKOVERFLOW
     select HAVE_DYNAMIC_FTRACE
+    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
     select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
     select HAVE_EBPF_JIT
     select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN && POWER7_CPU)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h

@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)

 struct dyn_arch_ftrace {
     struct module *mod;
 };
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+    struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+    return &fregs->regs;
+}

I think this is wrong. We need to differentiate between ftrace_caller() and ftrace_regs_caller() here, and only return pt_regs if coming in through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).

Not sure I follow you.

This is based on 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS support")

It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also with ftrace_caller().

Sure you only have the params, but that's the same on s390, so what did I miss ?

It looks like s390 is special since it apparently saves all registers even for ftrace_caller: https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/ (local)

As I understand it, the reason ftrace_get_regs() was introduced was to be able to only return the pt_regs, if _all_ registers were saved into it, which we don't do when coming in through ftrace_caller(). See the x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for arguments to be passed in to ftrace_regs by default"), which returns pt_regs conditionally.

I already have this series in next, I can pull it out, but I'd rather
not.

Yeah, I'm sorry about the late review on this one.

I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.

I think changes to this particular patch can be added as an incremental patch. If anything, pt_regs won't have all valid registers, but no one should depend on it without also setting FL_SAVE_REGS anyway.

I was concerned about patch 8 though, where we are missing saving r1 into pt_regs. That gets used in patch 11, and will be used during unwinding when the function_graph tracer is active. But, this should still just result in us being unable to unwind the stack, so I think that can also be an incremental patch.


Thanks,
Naveen