[PATCH 0/4] arm64/ftrace: move to DYNAMIC_FTRACE_WITH_ARGS

[PATCH 0/4] arm64/ftrace: move to DYNAMIC_FTRACE_WITH_ARGS

[Mark Rutland]

This series replaces arm64's support for FTRACE_WITH_REGS with support
for FTRACE_WITH_ARGS. This removes some overhead and complexity, and
removes some latent issues with inconsistent presentation of struct
pt_regs (which can only be reliably saved/restored at exception
boundaries).

The existing FTRACE_WITH_REGS support was added for two major reasons:

(1) To make it possible to use the ftrace graph tracer with pointer
    authentication, where it's necessary to snapshot/manipulate the LR
    before it is signed by the instrumented function.

(2) To make it possible to implement LIVEPATCH in future, where we need
    to hook function entry before an instrumented function manipulates
    the stack or argument registers. Practically speaking, we need to
    preserve the argument/return registers, PC, LR, and SP.

Neither of these requires the full set of pt_regs, and only requires us
to save/restore a subset of registers used for passing
arguments/return-values and context/return information (which is the
minimum set we always need to save/restore today).

As there is no longer a need to save different sets of registers for
different features, we no longer need distinct `ftrace_caller` and
`ftrace_regs_caller` trampolines. This allows the trampoline assembly to
be simpler, and simplifies code which previously had to handle the two
trampolines.

I've tested this with the ftrace selftests, where there are no
unexpected failures.

I plan to build atop this with subsequent patches to add per-callsite
ftrace_ops, and I'm sending these patches on their own as I think they
make sense regardless.

Thanks,
Mark.

Mark Rutland (4):
  ftrace: pass fregs to arch_ftrace_set_direct_caller()
  ftrace: rename ftrace_instruction_pointer_set() ->
    ftrace_regs_set_instruction_pointer()
  ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses
  ftrace: arm64: move from REGS to ARGS

 arch/arm64/Kconfig                |  18 +++--
 arch/arm64/Makefile               |   2 +-
 arch/arm64/include/asm/ftrace.h   |  70 ++++++++++++++++--
 arch/arm64/kernel/asm-offsets.c   |  13 ++++
 arch/arm64/kernel/entry-ftrace.S  | 117 ++++++++++++------------------
 arch/arm64/kernel/ftrace.c        |  37 +---------
 arch/arm64/kernel/module.c        |   3 -
 arch/powerpc/include/asm/ftrace.h |  22 +++++-
 arch/s390/include/asm/ftrace.h    |  25 ++++++-
 arch/x86/include/asm/ftrace.h     |  42 +++++++----
 include/linux/ftrace.h            |  40 ++++++++--
 kernel/livepatch/patch.c          |   2 +-
 kernel/trace/Kconfig              |   6 +-
 kernel/trace/ftrace.c             |   3 +-
 14 files changed, 247 insertions(+), 153 deletions(-)

-- 
2.30.2

[PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()

[Mark Rutland]

In subsequent patches we'll arrange for architectures to have an
ftrace_regs which is entirely distinct from pt_regs. In preparation for
this, we need to minimize the use of pt_regs to where strictly
necessary in the core ftrace code.

This patch changes the prototype of arch_ftrace_set_direct_caller() to
take ftrace_regs rather than pt_regs, and moves the extraction of the
pt_regs into arch_ftrace_set_direct_caller().

For x86, we need to move arch_ftrace_set_direct_caller() after the
definition of struct ftrace_regs. As arch_ftrace_set_direct_caller() is
always called with a non-NULL pt_regs by call_direct_funcs() whose ops
have FTRACE_OPS_FL_SAVE_REGS, it's not necessary to add an additional
NULL check.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Florent Revest <revest@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 arch/s390/include/asm/ftrace.h |  3 ++-
 arch/x86/include/asm/ftrace.h  | 26 +++++++++++++-------------
 include/linux/ftrace.h         |  4 +++-
 kernel/trace/ftrace.c          |  3 +--
 4 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
index 6f80ec9c04be..53c4d1257549 100644
--- a/arch/s390/include/asm/ftrace.h
+++ b/arch/s390/include/asm/ftrace.h
@@ -67,8 +67,9 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
  * place the direct caller in the ORIG_GPR2 part of pt_regs. This
  * tells the ftrace_caller that there's a direct caller.
  */
-static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
+static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs, unsigned long addr)
 {
+	struct pt_regs *regs = &fregs->regs;
 	regs->orig_gpr2 = addr;
 }
 
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 908d99b127d3..788e7c1f6463 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -34,19 +34,6 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 	return addr;
 }
 
-/*
- * When a ftrace registered caller is tracing a function that is
- * also set by a register_ftrace_direct() call, it needs to be
- * differentiated in the ftrace_caller trampoline. To do this, we
- * place the direct caller in the ORIG_AX part of pt_regs. This
- * tells the ftrace_caller that there's a direct caller.
- */
-static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
-{
-	/* Emulate a call */
-	regs->orig_ax = addr;
-}
-
 #ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
 struct ftrace_regs {
 	struct pt_regs		regs;
@@ -72,6 +59,19 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
 #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 #endif
 
+/*
+ * When a ftrace registered caller is tracing a function that is
+ * also set by a register_ftrace_direct() call, it needs to be
+ * differentiated in the ftrace_caller trampoline. To do this, we
+ * place the direct caller in the ORIG_AX part of pt_regs. This
+ * tells the ftrace_caller that there's a direct caller.
+ */
+static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs, unsigned long addr)
+{
+	/* Emulate a call */
+	fregs->regs.orig_ax = addr;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 struct dyn_arch_ftrace {
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 62557d4bffc2..5ca6c6680460 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -429,6 +429,7 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
 }
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
+#ifdef CONFIG_FUNCTION_TRACER
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 /*
  * This must be implemented by the architecture.
@@ -443,9 +444,10 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
  * the return from the trampoline jump to the direct caller
  * instead of going back to the function it just traced.
  */
-static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
+static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs,
 						 unsigned long addr) { }
 #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+#endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_STACK_TRACER
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index fbf2543111c0..234c5414deee 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2487,14 +2487,13 @@ ftrace_add_rec_direct(unsigned long ip, unsigned long addr,
 static void call_direct_funcs(unsigned long ip, unsigned long pip,
 			      struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
-	struct pt_regs *regs = ftrace_get_regs(fregs);
 	unsigned long addr;
 
 	addr = ftrace_find_rec_direct(ip);
 	if (!addr)
 		return;
 
-	arch_ftrace_set_direct_caller(regs, addr);
+	arch_ftrace_set_direct_caller(fregs, addr);
 }
 
 struct ftrace_ops direct_ops = {
-- 
2.30.2

[PATCH 2/4] ftrace: rename ftrace_instruction_pointer_set() -> ftrace_regs_set_instruction_pointer()

[Mark Rutland]

In subsequent patches we'll add a sew of ftrace_regs_{get,set}_*()
helpers. In preparation, this patch renames
ftrace_instruction_pointer_set() to
ftrace_regs_set_instruction_pointer().

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Florent Revest <revest@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 arch/powerpc/include/asm/ftrace.h | 5 +++--
 arch/s390/include/asm/ftrace.h    | 5 +++--
 arch/x86/include/asm/ftrace.h     | 2 +-
 include/linux/ftrace.h            | 9 ++++-----
 kernel/livepatch/patch.c          | 2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 3cee7115441b..c3eb48f67566 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -37,8 +37,9 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
 	return fregs->regs.msr ? &fregs->regs : NULL;
 }
 
-static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
-							   unsigned long ip)
+static __always_inline void
+ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
+				    unsigned long ip)
 {
 	regs_set_return_ip(&fregs->regs, ip);
 }
diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
index 53c4d1257549..b8957882404f 100644
--- a/arch/s390/include/asm/ftrace.h
+++ b/arch/s390/include/asm/ftrace.h
@@ -54,8 +54,9 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
 	return NULL;
 }
 
-static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
-							   unsigned long ip)
+static __always_inline void
+ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
+				    unsigned long ip)
 {
 	fregs->regs.psw.addr = ip;
 }
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 788e7c1f6463..b73e858bd96f 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -48,7 +48,7 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
 	return &fregs->regs;
 }
 
-#define ftrace_instruction_pointer_set(fregs, _ip)	\
+#define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
 	do { (fregs)->regs.ip = (_ip); } while (0)
 
 struct ftrace_ops;
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 5ca6c6680460..e9905f741916 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -110,12 +110,11 @@ struct ftrace_regs {
 #define arch_ftrace_get_regs(fregs) (&(fregs)->regs)
 
 /*
- * ftrace_instruction_pointer_set() is to be defined by the architecture
- * if to allow setting of the instruction pointer from the ftrace_regs
- * when HAVE_DYNAMIC_FTRACE_WITH_ARGS is set and it supports
- * live kernel patching.
+ * ftrace_regs_set_instruction_pointer() is to be defined by the architecture
+ * if to allow setting of the instruction pointer from the ftrace_regs when
+ * HAVE_DYNAMIC_FTRACE_WITH_ARGS is set and it supports live kernel patching.
  */
-#define ftrace_instruction_pointer_set(fregs, ip) do { } while (0)
+#define ftrace_regs_set_instruction_pointer(fregs, ip) do { } while (0)
 #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs)
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 4c4f5a776d80..4152c71507e2 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -118,7 +118,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	if (func->nop)
 		goto unlock;
 
-	ftrace_instruction_pointer_set(fregs, (unsigned long)func->new_func);
+	ftrace_regs_set_instruction_pointer(fregs, (unsigned long)func->new_func);
 
 unlock:
 	ftrace_test_recursion_unlock(bit);
-- 
2.30.2

[PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Mark Rutland]

In subsequent patches we'll arrange for architectures to have an
ftrace_regs which is entirely distinct from pt_regs. In preparation for
this, we need to minimize the use of pt_regs to where strictly necessary
in the core ftrace code.

This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
these can always be used on any ftrace_regs, and when
CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
available. A new ftrace_regs_has_args(fregs) helper is added which code
can use to check when these are usable.

Co-developed-by: Florent Revest <revest@chromium.org>
Signed-off-by: Florent Revest <revest@chromium.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
 arch/s390/include/asm/ftrace.h    | 17 +++++++++++++++++
 arch/x86/include/asm/ftrace.h     | 14 ++++++++++++++
 include/linux/ftrace.h            | 27 +++++++++++++++++++++++++++
 kernel/trace/Kconfig              |  6 +++---
 5 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index c3eb48f67566..faecb20d78bf 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -44,6 +44,23 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
 	regs_set_return_ip(&fregs->regs, ip);
 }
 
+static __always_inline unsigned long
+ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
+{
+	return instruction_pointer(&fregs->regs)
+}
+
+#define ftrace_regs_get_argument(fregs, n) \
+	regs_get_kernel_argument(&(fregs)->regs, n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+	kernel_stack_pointer(&(fregs)->regs)
+#define ftrace_regs_return_value(fregs) \
+	regs_return_value(&(fregs)->regs)
+#define ftrace_regs_set_return_value(fregs, ret) \
+	regs_set_return_value(&(fregs)->regs, ret)
+#define ftrace_override_function_with_return(fregs) \
+	override_function_with_return(&(fregs)->regs)
+
 struct ftrace_ops;
 
 #define ftrace_graph_func ftrace_graph_func
diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
index b8957882404f..5fdc806458aa 100644
--- a/arch/s390/include/asm/ftrace.h
+++ b/arch/s390/include/asm/ftrace.h
@@ -54,6 +54,12 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
 	return NULL;
 }
 
+static __always_inline unsigned long
+ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
+{
+	return fregs->regs.psw.addr;
+}
+
 static __always_inline void
 ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
 				    unsigned long ip)
@@ -61,6 +67,17 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
 	fregs->regs.psw.addr = ip;
 }
 
+#define ftrace_regs_get_argument(fregs, n) \
+	regs_get_kernel_argument(&(fregs)->regs, n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+	kernel_stack_pointer(&(fregs)->regs)
+#define ftrace_regs_return_value(fregs) \
+	regs_return_value(&(fregs)->regs)
+#define ftrace_regs_set_return_value(fregs, ret) \
+	regs_set_return_value(&(fregs)->regs, ret)
+#define ftrace_override_function_with_return(fregs) \
+	override_function_with_return(&(fregs)->regs)
+
 /*
  * When an ftrace registered caller is tracing a function that is
  * also set by a register_ftrace_direct() call, it needs to be
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index b73e858bd96f..b3737b42e8a1 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -51,6 +51,20 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
 #define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
 	do { (fregs)->regs.ip = (_ip); } while (0)
 
+#define ftrace_regs_get_instruction_pointer(fregs) \
+	((fregs)->regs.ip)
+
+#define ftrace_regs_get_argument(fregs, n) \
+	regs_get_kernel_argument(&(fregs)->regs, n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+	kernel_stack_pointer(&(fregs)->regs)
+#define ftrace_regs_return_value(fregs) \
+	regs_return_value(&(fregs)->regs)
+#define ftrace_regs_set_return_value(fregs, ret) \
+	regs_set_return_value(&(fregs)->regs, ret)
+#define ftrace_override_function_with_return(fregs) \
+	override_function_with_return(&(fregs)->regs)
+
 struct ftrace_ops;
 #define ftrace_graph_func ftrace_graph_func
 void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e9905f741916..3b13e3c21438 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -125,6 +125,33 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
 	return arch_ftrace_get_regs(fregs);
 }
 
+/*
+ * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
+ * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.
+ */
+static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
+{
+	if (IS_ENABLED(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS))
+		return true;
+
+	return ftrace_get_regs(fregs) != NULL;
+}
+
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+#define ftrace_regs_get_instruction_pointer(fregs) \
+	instruction_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_get_argument(fregs, n) \
+	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
+#define ftrace_regs_get_stack_pointer(fregs) \
+	kernel_stack_pointer(ftrace_get_regs(fregs))
+#define ftrace_regs_return_value(fregs) \
+	regs_return_value(ftrace_get_regs(fregs))
+#define ftrace_regs_set_return_value(fregs, ret) \
+	regs_set_return_value(ftrace_get_regs(fregs), ret)
+#define ftrace_override_function_with_return(fregs) \
+	override_function_with_return(ftrace_get_regs(fregs))
+#endif
+
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct ftrace_regs *fregs);
 
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index e9e95c790b8e..2c6611c13f99 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -46,10 +46,10 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	bool
 	help
 	 If this is set, then arguments and stack can be found from
-	 the pt_regs passed into the function callback regs parameter
+	 the ftrace_regs passed into the function callback regs parameter
 	 by default, even without setting the REGS flag in the ftrace_ops.
-	 This allows for use of regs_get_kernel_argument() and
-	 kernel_stack_pointer().
+	 This allows for use of ftrace_regs_get_argument() and
+	 ftrace_regs_get_stack_pointer().
 
 config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
 	bool
-- 
2.30.2

[PATCH 4/4] ftrace: arm64: move from REGS to ARGS

[Mark Rutland]

This commit replaces arm64's support for FTRACE_WITH_REGS with support
for FTRACE_WITH_ARGS. This removes some overhead and complexity, and
removes some latent issues with inconsistent presentation of struct
pt_regs (which can only be reliably saved/restored at exception
boundaries).

FTRACE_WITH_REGS has been supported on arm64 since commit:

  3b23e4991fb66f6d ("arm64: implement ftrace with regs")

As noted in the commit message, the major reasons for implementing
FTRACE_WITH_REGS were:

(1) To make it possible to use the ftrace graph tracer with pointer
    authentication, where it's necessary to snapshot/manipulate the LR
    before it is signed by the instrumented function.

(2) To make it possible to implement LIVEPATCH in future, where we need
    to hook function entry before an instrumented function manipulates
    the stack or argument registers. Practically speaking, we need to
    preserve the argument/return registers, PC, LR, and SP.

Neither of these need a struct pt_regs, and only require the set of
registers which are live at function call/return boundaries. Our calling
convention is defined by "Procedure Call Standard for the Arm® 64-bit
Architecture (AArch64)" (AKA "AAPCS64"), which can currently be found
at:

  https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst

Per AAPCS64, all function call argument and return values are held in
the following GPRs:

* X0 - X7 : parameter / result registers
* X8      : indirect result location register
* SP      : stack pointer (AKA SP)

Additionally, ad function call boundaries, the following GPRs hold
context/return information:

* X29 : frame pointer (AKA FP)
* X30 : link register (AKA LR)

... and for ftrace we need to capture the instrumented address:

 * PC  : program counter

No other GPRs are relevant, as none of the other arguments hold
parameters or return values:

* X9  - X17 : temporaries, may be clobbered
* X18       : shadow call stack pointer (or temorary)
* X19 - X28 : callee saved

This patch implements FTRACE_WITH_ARGS for arm64, only saving/restoring
the minimal set of registers necessary. This is always sufficient to
manipulate control flow (e.g. for live-patching) or to manipulate
function arguments and return values.

As there is no longer a need to save different sets of registers for
different features, we no longer need distinct `ftrace_caller` and
`ftrace_regs_caller` trampolines. This allows the trampoline assembly to
be simpler, and simplifies code which previously had to handle the two
trampolines.

I've tested this with the ftrace selftests, where there are no
unexpected failures.

Co-developed-by: Florent Revest <revest@chromium.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Florent Revest <revest@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/Kconfig               |  18 ++---
 arch/arm64/Makefile              |   2 +-
 arch/arm64/include/asm/ftrace.h  |  70 ++++++++++++++++--
 arch/arm64/kernel/asm-offsets.c  |  13 ++++
 arch/arm64/kernel/entry-ftrace.S | 117 ++++++++++++-------------------
 arch/arm64/kernel/ftrace.c       |  37 ++--------
 arch/arm64/kernel/module.c       |   3 -
 7 files changed, 138 insertions(+), 122 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 505c8a1ccbe0..b6b3305ba701 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -181,8 +181,10 @@ config ARM64
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS \
+		if $(cc-option,-fpatchable-function-entry=2)
 	select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY \
-		if DYNAMIC_FTRACE_WITH_REGS
+		if DYNAMIC_FTRACE_WITH_ARGS
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_FAST_GUP
 	select HAVE_FTRACE_MCOUNT_RECORD
@@ -233,16 +235,16 @@ config ARM64
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
-config CLANG_SUPPORTS_DYNAMIC_FTRACE_WITH_REGS
+config CLANG_SUPPORTS_DYNAMIC_FTRACE_WITH_ARGS
 	def_bool CC_IS_CLANG
 	# https://github.com/ClangBuiltLinux/linux/issues/1507
 	depends on AS_IS_GNU || (AS_IS_LLVM && (LD_IS_LLD || LD_VERSION >= 23600))
-	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS
 
-config GCC_SUPPORTS_DYNAMIC_FTRACE_WITH_REGS
+config GCC_SUPPORTS_DYNAMIC_FTRACE_WITH_ARGS
 	def_bool CC_IS_GCC
 	depends on $(cc-option,-fpatchable-function-entry=2)
-	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS
 
 config 64BIT
 	def_bool y
@@ -1816,7 +1818,7 @@ config ARM64_PTR_AUTH_KERNEL
 	# which is only understood by binutils starting with version 2.33.1.
 	depends on LD_IS_LLD || LD_VERSION >= 23301 || (CC_IS_GCC && GCC_VERSION < 90100)
 	depends on !CC_IS_CLANG || AS_HAS_CFI_NEGATE_RA_STATE
-	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
+	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_ARGS)
 	help
 	  If the compiler supports the -mbranch-protection or
 	  -msign-return-address flag (e.g. GCC 7 or later), then this option
@@ -1826,7 +1828,7 @@ config ARM64_PTR_AUTH_KERNEL
 	  disabled with minimal loss of protection.
 
 	  This feature works with FUNCTION_GRAPH_TRACER option only if
-	  DYNAMIC_FTRACE_WITH_REGS is enabled.
+	  DYNAMIC_FTRACE_WITH_ARGS is enabled.
 
 config CC_HAS_BRANCH_PROT_PAC_RET
 	# GCC 9 or later, clang 8 or later
@@ -1924,7 +1926,7 @@ config ARM64_BTI_KERNEL
 	depends on !CC_IS_GCC
 	# https://github.com/llvm/llvm-project/commit/a88c722e687e6780dcd6a58718350dc76fcc4cc9
 	depends on !CC_IS_CLANG || CLANG_VERSION >= 120000
-	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
+	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_ARGS)
 	help
 	  Build the kernel with Branch Target Identification annotations
 	  and enable enforcement of this for kernel code. When this option
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 5e56d26a2239..b1202fa84bab 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -128,7 +128,7 @@ endif
 
 CHECKFLAGS	+= -D__aarch64__
 
-ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_ARGS),y)
   KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
   CC_FLAGS_FTRACE := -fpatchable-function-entry=2
 endif
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 329dbbd4d50b..2691064d4e04 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -23,7 +23,7 @@
  */
 #define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
 #define ARCH_SUPPORTS_FTRACE_OPS 1
 #else
 #define MCOUNT_ADDR		((unsigned long)_mcount)
@@ -33,8 +33,7 @@
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
 #define FTRACE_PLT_IDX		0
-#define FTRACE_REGS_PLT_IDX	1
-#define NR_FTRACE_PLTS		2
+#define NR_FTRACE_PLTS		1
 
 /*
  * Currently, gcc tends to save the link register after the local variables
@@ -69,7 +68,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 	 * Adjust addr to point at the BL in the callsite.
 	 * See ftrace_init_nop() for the callsite sequence.
 	 */
-	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
 		return addr + AARCH64_INSN_SIZE;
 	/*
 	 * addr is the address of the mcount call instruction.
@@ -78,10 +77,69 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 	return addr;
 }
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
 struct dyn_ftrace;
 struct ftrace_ops;
-struct ftrace_regs;
+
+#define arch_ftrace_get_regs(regs) NULL
+
+struct ftrace_regs {
+	/* x0 - x8 */
+	unsigned long regs[9];
+	unsigned long __unused;
+
+	unsigned long fp;
+	unsigned long lr;
+
+	unsigned long sp;
+	unsigned long pc;
+};
+
+static __always_inline unsigned long
+ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
+{
+	return fregs->pc;
+}
+
+static __always_inline void
+ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
+				    unsigned long pc)
+{
+	fregs->pc = pc;
+}
+
+static __always_inline unsigned long
+ftrace_regs_get_stack_pointer(const struct ftrace_regs *fregs)
+{
+	return fregs->sp;
+}
+
+static __always_inline unsigned long
+ftrace_regs_get_argument(struct ftrace_regs *fregs, unsigned int n)
+{
+	if (n < 8)
+		return fregs->regs[n];
+	return 0;
+}
+
+static __always_inline unsigned long
+ftrace_regs_get_return_value(const struct ftrace_regs *fregs)
+{
+	return fregs->regs[0];
+}
+
+static __always_inline void
+ftrace_regs_set_return_value(struct ftrace_regs *fregs,
+			     unsigned long ret)
+{
+	fregs->regs[0] = ret;
+}
+
+static __always_inline void
+ftrace_override_function_with_return(struct ftrace_regs *fregs)
+{
+	fregs->pc = fregs->lr;
+}
 
 int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
 #define ftrace_init_nop ftrace_init_nop
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 1197e7679882..2234624536d9 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -82,6 +82,19 @@ int main(void)
   DEFINE(S_STACKFRAME,		offsetof(struct pt_regs, stackframe));
   DEFINE(PT_REGS_SIZE,		sizeof(struct pt_regs));
   BLANK();
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+  DEFINE(FREGS_X0,		offsetof(struct ftrace_regs, regs[0]));
+  DEFINE(FREGS_X2,		offsetof(struct ftrace_regs, regs[2]));
+  DEFINE(FREGS_X4,		offsetof(struct ftrace_regs, regs[4]));
+  DEFINE(FREGS_X6,		offsetof(struct ftrace_regs, regs[6]));
+  DEFINE(FREGS_X8,		offsetof(struct ftrace_regs, regs[8]));
+  DEFINE(FREGS_FP,		offsetof(struct ftrace_regs, fp));
+  DEFINE(FREGS_LR,		offsetof(struct ftrace_regs, lr));
+  DEFINE(FREGS_SP,		offsetof(struct ftrace_regs, sp));
+  DEFINE(FREGS_PC,		offsetof(struct ftrace_regs, pc));
+  DEFINE(FREGS_SIZE,		sizeof(struct ftrace_regs));
+  BLANK();
+#endif
 #ifdef CONFIG_COMPAT
   DEFINE(COMPAT_SIGFRAME_REGS_OFFSET,		offsetof(struct compat_sigframe, uc.uc_mcontext.arm_r0));
   DEFINE(COMPAT_RT_SIGFRAME_REGS_OFFSET,	offsetof(struct compat_rt_sigframe, sig.uc.uc_mcontext.arm_r0));
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 795344ab4ec4..4d3050549aa6 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -13,83 +13,58 @@
 #include <asm/ftrace.h>
 #include <asm/insn.h>
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
 /*
  * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
  * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
  * ftrace_make_call() have patched those NOPs to:
  *
  * 	MOV	X9, LR
- * 	BL	<entry>
- *
- * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
+ * 	BL	ftrace_caller
  *
  * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are
  * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to
  * clobber.
  *
- * We save the callsite's context into a pt_regs before invoking any ftrace
- * callbacks. So that we can get a sensible backtrace, we create a stack record
- * for the callsite and the ftrace entry assembly. This is not sufficient for
- * reliable stacktrace: until we create the callsite stack record, its caller
- * is missing from the LR and existing chain of frame records.
+ * We save the callsite's context into a struct ftrace_regs before invoking any
+ * ftrace callbacks. So that we can get a sensible backtrace, we create frame
+ * records for the callsite and the ftrace entry assembly. This is not
+ * sufficient for reliable stacktrace: until we create the callsite stack
+ * record, its caller is missing from the LR and existing chain of frame
+ * records.
  */
-	.macro  ftrace_regs_entry, allregs=0
-	/* Make room for pt_regs, plus a callee frame */
-	sub	sp, sp, #(PT_REGS_SIZE + 16)
-
-	/* Save function arguments (and x9 for simplicity) */
-	stp	x0, x1, [sp, #S_X0]
-	stp	x2, x3, [sp, #S_X2]
-	stp	x4, x5, [sp, #S_X4]
-	stp	x6, x7, [sp, #S_X6]
-	stp	x8, x9, [sp, #S_X8]
-
-	/* Optionally save the callee-saved registers, always save the FP */
-	.if \allregs == 1
-	stp	x10, x11, [sp, #S_X10]
-	stp	x12, x13, [sp, #S_X12]
-	stp	x14, x15, [sp, #S_X14]
-	stp	x16, x17, [sp, #S_X16]
-	stp	x18, x19, [sp, #S_X18]
-	stp	x20, x21, [sp, #S_X20]
-	stp	x22, x23, [sp, #S_X22]
-	stp	x24, x25, [sp, #S_X24]
-	stp	x26, x27, [sp, #S_X26]
-	stp	x28, x29, [sp, #S_X28]
-	.else
-	str	x29, [sp, #S_FP]
-	.endif
-
-	/* Save the callsite's SP and LR */
-	add	x10, sp, #(PT_REGS_SIZE + 16)
-	stp	x9, x10, [sp, #S_LR]
+SYM_CODE_START(ftrace_caller)
+	bti	c
 
-	/* Save the PC after the ftrace callsite */
-	str	x30, [sp, #S_PC]
+	/* Save original SP */
+	mov	x10, sp
 
-	/* Create a frame record for the callsite above pt_regs */
-	stp	x29, x9, [sp, #PT_REGS_SIZE]
-	add	x29, sp, #PT_REGS_SIZE
+	/* Make room for ftrace regs, plus two frame records */
+	sub	sp, sp, #(FREGS_SIZE + 32)
 
-	/* Create our frame record within pt_regs. */
-	stp	x29, x30, [sp, #S_STACKFRAME]
-	add	x29, sp, #S_STACKFRAME
-	.endm
+	/* Save function arguments */
+	stp	x0, x1, [sp, #FREGS_X0]
+	stp	x2, x3, [sp, #FREGS_X2]
+	stp	x4, x5, [sp, #FREGS_X4]
+	stp	x6, x7, [sp, #FREGS_X6]
+	str	x8,     [sp, #FREGS_X8]
 
-SYM_CODE_START(ftrace_regs_caller)
-	bti	c
-	ftrace_regs_entry	1
-	b	ftrace_common
-SYM_CODE_END(ftrace_regs_caller)
+	/* Save the callsite's FP, LR, SP */
+	str	x29, [sp, #FREGS_FP]
+	str	x9,  [sp, #FREGS_LR]
+	str	x10, [sp, #FREGS_SP]
 
-SYM_CODE_START(ftrace_caller)
-	bti	c
-	ftrace_regs_entry	0
-	b	ftrace_common
-SYM_CODE_END(ftrace_caller)
+	/* Save the PC after the ftrace callsite */
+	str	x30, [sp, #FREGS_PC]
+
+	/* Create a frame record for the callsite above the ftrace regs */
+	stp	x29, x9, [sp, #FREGS_SIZE + 16]
+	add	x29, sp, #FREGS_SIZE + 16
+
+	/* Create our frame record above the ftrace regs */
+	stp	x29, x30, [sp, #FREGS_SIZE]
+	add	x29, sp, #FREGS_SIZE
 
-SYM_CODE_START(ftrace_common)
 	sub	x0, x30, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
 	mov	x1, x9				// parent_ip (callsite's LR)
 	ldr_l	x2, function_trace_op		// op
@@ -104,24 +79,24 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
  * to restore x0-x8, x29, and x30.
  */
 	/* Restore function arguments */
-	ldp	x0, x1, [sp]
-	ldp	x2, x3, [sp, #S_X2]
-	ldp	x4, x5, [sp, #S_X4]
-	ldp	x6, x7, [sp, #S_X6]
-	ldr	x8, [sp, #S_X8]
+	ldp	x0, x1, [sp, #FREGS_X0]
+	ldp	x2, x3, [sp, #FREGS_X2]
+	ldp	x4, x5, [sp, #FREGS_X4]
+	ldp	x6, x7, [sp, #FREGS_X6]
+	ldr	x8,     [sp, #FREGS_X8]
 
 	/* Restore the callsite's FP, LR, PC */
-	ldr	x29, [sp, #S_FP]
-	ldr	x30, [sp, #S_LR]
-	ldr	x9, [sp, #S_PC]
+	ldr	x29, [sp, #FREGS_FP]
+	ldr	x30, [sp, #FREGS_LR]
+	ldr	x9,  [sp, #FREGS_PC]
 
 	/* Restore the callsite's SP */
-	add	sp, sp, #PT_REGS_SIZE + 16
+	add	sp, sp, #FREGS_SIZE + 32
 
 	ret	x9
-SYM_CODE_END(ftrace_common)
+SYM_CODE_END(ftrace_caller)
 
-#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_ARGS */
 
 /*
  * Gcc with -pg will put the following code in the beginning of each function:
@@ -293,7 +268,7 @@ SYM_FUNC_START(ftrace_graph_caller)
 	mcount_exit
 SYM_FUNC_END(ftrace_graph_caller)
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_ARGS */
 
 SYM_TYPED_FUNC_START(ftrace_stub)
 	ret
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 8745175f4a75..ed6b8a70c0d1 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -70,9 +70,6 @@ static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
 
 	if (addr == FTRACE_ADDR)
 		return &plt[FTRACE_PLT_IDX];
-	if (addr == FTRACE_REGS_ADDR &&
-	    IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
-		return &plt[FTRACE_REGS_PLT_IDX];
 #endif
 	return NULL;
 }
@@ -154,25 +151,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	return ftrace_modify_code(pc, old, new, true);
 }
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
-int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
-			unsigned long addr)
-{
-	unsigned long pc = rec->ip;
-	u32 old, new;
-
-	if (!ftrace_find_callable_addr(rec, NULL, &old_addr))
-		return -EINVAL;
-	if (!ftrace_find_callable_addr(rec, NULL, &addr))
-		return -EINVAL;
-
-	old = aarch64_insn_gen_branch_imm(pc, old_addr,
-					  AARCH64_INSN_BRANCH_LINK);
-	new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
-
-	return ftrace_modify_code(pc, old, new, true);
-}
-
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
 /*
  * The compiler has inserted two NOPs before the regular function prologue.
  * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are live,
@@ -279,19 +258,11 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
 void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
 		       struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
-	/*
-	 * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
-	 * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
-	 * in which we can safely modify the LR.
-	 */
-	struct pt_regs *regs = arch_ftrace_get_regs(fregs);
-	unsigned long *parent = (unsigned long *)&procedure_link_pointer(regs);
-
-	prepare_ftrace_return(ip, parent, frame_pointer(regs));
+	prepare_ftrace_return(ip, &fregs->lr, fregs->fp);
 }
 #else
 /*
@@ -323,6 +294,6 @@ int ftrace_disable_ftrace_graph_caller(void)
 {
 	return ftrace_modify_graph_caller(false);
 }
-#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_ARGS */
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 76b41e4ca9fa..acd0d883e9ca 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -497,9 +497,6 @@ static int module_init_ftrace_plt(const Elf_Ehdr *hdr,
 
 	__init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
 
-	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
-		__init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
-
 	mod->arch.ftrace_trampolines = plts;
 #endif
 	return 0;
-- 
2.30.2

Re: [PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()

[Steven Rostedt]

On Mon, 24 Oct 2022 15:08:43 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -429,6 +429,7 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
>  }
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
> +#ifdef CONFIG_FUNCTION_TRACER

Instead of adding the above preprocessor check, the below chunk should be
moved into the CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block above.

-- Steve


>  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>  /*
>   * This must be implemented by the architecture.
> @@ -443,9 +444,10 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
>   * the return from the trampoline jump to the direct caller
>   * instead of going back to the function it just traced.
>   */
> -static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
> +static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs,
>  						 unsigned long addr) { }
>  #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> +#endif /* CONFIG_FUNCTION_TRACER */
>  
>  #ifdef CONFIG_STACK_TRACER
>  
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index fbf2543111c0..234c5414deee 100644

Re: [PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()

[Mark Rutland]

On Mon, Oct 24, 2022 at 10:48:45AM -0400, Steven Rostedt wrote:
> On Mon, 24 Oct 2022 15:08:43 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -429,6 +429,7 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
> >  }
> >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >  
> > +#ifdef CONFIG_FUNCTION_TRACER
> 
> Instead of adding the above preprocessor check, the below chunk should be
> moved into the CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block above.

Sure; but note that doing that naively means 'struct ftrace_regs' won't always
be declared (e.g. if !CONFIG_FUNCTION_TRACER), and will result in warnings, e.g.

|   CC      arch/x86/kernel/asm-offsets.s
| In file included from ./include/linux/kvm_host.h:32,
|                  from arch/x86/kernel/../kvm/vmx/vmx.h:5,
|                  from arch/x86/kernel/asm-offsets.c:22:
| ./include/linux/ftrace.h:444:57: error: ‘struct ftrace_regs’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
|   444 | static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs,
|       |                                                         ^~~~~~~~~~~
| cc1: all warnings being treated as errors
| make[1]: *** [scripts/Makefile.build:118: arch/x86/kernel/asm-offsets.s] Error 1
| make: *** [Makefile:1270: prepare0] Error 2

... so I'll either need to add some ifdeffery, for CONFIG_FUNCTION_TRACER, or I
can hoist the declaration of 'struct ftrace_regs' to not depend on
CONFIG_FUNCTION_TRACER.

I guess the latter is preferable, e.g.

| diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
| index 2b34fec40a39..f201fcbfffb0 100644
| --- a/include/linux/ftrace.h
| +++ b/include/linux/ftrace.h
| @@ -37,9 +37,10 @@ extern void ftrace_boot_snapshot(void);
|  static inline void ftrace_boot_snapshot(void) { }
|  #endif
|  
| -#ifdef CONFIG_FUNCTION_TRACER
|  struct ftrace_ops;
|  struct ftrace_regs;
| +
| +#ifdef CONFIG_FUNCTION_TRACER
|  /*
|   * If the arch's mcount caller does not support all of ftrace's
|   * features, then it must call an indirect function that

... so I've done that locally for now.

Thanks,
Mark.

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Masami Hiramatsu]

Hi Mark,

On Mon, 24 Oct 2022 15:08:45 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> In subsequent patches we'll arrange for architectures to have an
> ftrace_regs which is entirely distinct from pt_regs. In preparation for
> this, we need to minimize the use of pt_regs to where strictly necessary
> in the core ftrace code.
> 
> This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> these can always be used on any ftrace_regs, and when
> CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> available. A new ftrace_regs_has_args(fregs) helper is added which code
> can use to check when these are usable.

Can you also add the ftrace_regs_query_register_offset() as a wrapper of
regs_query_register_offset()? I would like to use it for fprobe_events.

Thank you,

> 
> Co-developed-by: Florent Revest <revest@chromium.org>
> Signed-off-by: Florent Revest <revest@chromium.org>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>  arch/s390/include/asm/ftrace.h    | 17 +++++++++++++++++
>  arch/x86/include/asm/ftrace.h     | 14 ++++++++++++++
>  include/linux/ftrace.h            | 27 +++++++++++++++++++++++++++
>  kernel/trace/Kconfig              |  6 +++---
>  5 files changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index c3eb48f67566..faecb20d78bf 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -44,6 +44,23 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>  	regs_set_return_ip(&fregs->regs, ip);
>  }
>  
> +static __always_inline unsigned long
> +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> +{
> +	return instruction_pointer(&fregs->regs)
> +}
> +
> +#define ftrace_regs_get_argument(fregs, n) \
> +	regs_get_kernel_argument(&(fregs)->regs, n)
> +#define ftrace_regs_get_stack_pointer(fregs) \
> +	kernel_stack_pointer(&(fregs)->regs)
> +#define ftrace_regs_return_value(fregs) \
> +	regs_return_value(&(fregs)->regs)
> +#define ftrace_regs_set_return_value(fregs, ret) \
> +	regs_set_return_value(&(fregs)->regs, ret)
> +#define ftrace_override_function_with_return(fregs) \
> +	override_function_with_return(&(fregs)->regs)
> +
>  struct ftrace_ops;
>  
>  #define ftrace_graph_func ftrace_graph_func
> diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> index b8957882404f..5fdc806458aa 100644
> --- a/arch/s390/include/asm/ftrace.h
> +++ b/arch/s390/include/asm/ftrace.h
> @@ -54,6 +54,12 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
>  	return NULL;
>  }
>  
> +static __always_inline unsigned long
> +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> +{
> +	return fregs->regs.psw.addr;
> +}
> +
>  static __always_inline void
>  ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>  				    unsigned long ip)
> @@ -61,6 +67,17 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>  	fregs->regs.psw.addr = ip;
>  }
>  
> +#define ftrace_regs_get_argument(fregs, n) \
> +	regs_get_kernel_argument(&(fregs)->regs, n)
> +#define ftrace_regs_get_stack_pointer(fregs) \
> +	kernel_stack_pointer(&(fregs)->regs)
> +#define ftrace_regs_return_value(fregs) \
> +	regs_return_value(&(fregs)->regs)
> +#define ftrace_regs_set_return_value(fregs, ret) \
> +	regs_set_return_value(&(fregs)->regs, ret)
> +#define ftrace_override_function_with_return(fregs) \
> +	override_function_with_return(&(fregs)->regs)
> +
>  /*
>   * When an ftrace registered caller is tracing a function that is
>   * also set by a register_ftrace_direct() call, it needs to be
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index b73e858bd96f..b3737b42e8a1 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -51,6 +51,20 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
>  #define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
>  	do { (fregs)->regs.ip = (_ip); } while (0)
>  
> +#define ftrace_regs_get_instruction_pointer(fregs) \
> +	((fregs)->regs.ip)
> +
> +#define ftrace_regs_get_argument(fregs, n) \
> +	regs_get_kernel_argument(&(fregs)->regs, n)
> +#define ftrace_regs_get_stack_pointer(fregs) \
> +	kernel_stack_pointer(&(fregs)->regs)
> +#define ftrace_regs_return_value(fregs) \
> +	regs_return_value(&(fregs)->regs)
> +#define ftrace_regs_set_return_value(fregs, ret) \
> +	regs_set_return_value(&(fregs)->regs, ret)
> +#define ftrace_override_function_with_return(fregs) \
> +	override_function_with_return(&(fregs)->regs)
> +
>  struct ftrace_ops;
>  #define ftrace_graph_func ftrace_graph_func
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index e9905f741916..3b13e3c21438 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -125,6 +125,33 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
>  	return arch_ftrace_get_regs(fregs);
>  }
>  
> +/*
> + * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
> + * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.
> + */
> +static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
> +{
> +	if (IS_ENABLED(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS))
> +		return true;
> +
> +	return ftrace_get_regs(fregs) != NULL;
> +}
> +
> +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +#define ftrace_regs_get_instruction_pointer(fregs) \
> +	instruction_pointer(ftrace_get_regs(fregs))
> +#define ftrace_regs_get_argument(fregs, n) \
> +	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
> +#define ftrace_regs_get_stack_pointer(fregs) \
> +	kernel_stack_pointer(ftrace_get_regs(fregs))
> +#define ftrace_regs_return_value(fregs) \
> +	regs_return_value(ftrace_get_regs(fregs))
> +#define ftrace_regs_set_return_value(fregs, ret) \
> +	regs_set_return_value(ftrace_get_regs(fregs), ret)
> +#define ftrace_override_function_with_return(fregs) \
> +	override_function_with_return(ftrace_get_regs(fregs))
> +#endif
> +
>  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
>  			      struct ftrace_ops *op, struct ftrace_regs *fregs);
>  
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index e9e95c790b8e..2c6611c13f99 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -46,10 +46,10 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
>  	bool
>  	help
>  	 If this is set, then arguments and stack can be found from
> -	 the pt_regs passed into the function callback regs parameter
> +	 the ftrace_regs passed into the function callback regs parameter
>  	 by default, even without setting the REGS flag in the ftrace_ops.
> -	 This allows for use of regs_get_kernel_argument() and
> -	 kernel_stack_pointer().
> +	 This allows for use of ftrace_regs_get_argument() and
> +	 ftrace_regs_get_stack_pointer().
>  
>  config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
>  	bool
> -- 
> 2.30.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Mark Rutland]

On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> Hi Mark,
> 
> On Mon, 24 Oct 2022 15:08:45 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > In subsequent patches we'll arrange for architectures to have an
> > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > this, we need to minimize the use of pt_regs to where strictly necessary
> > in the core ftrace code.
> > 
> > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > these can always be used on any ftrace_regs, and when
> > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > can use to check when these are usable.
> 
> Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> regs_query_register_offset()? I would like to use it for fprobe_events.

Sure!

Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?

I ask because if neither of those are the case, with FTRACE_WITH_REGS,
ftrace_regs_query_register_offset() would accept names of registers which might
not have been sampled, and could give offsets to uninitialized memory.

Atop that, I'm not exactly sure what to implement for powerpc/s390/x86 here. If
those might be used without a full pt_regs, I think
ftrace_regs_query_register_offset() should also take the fregs as a parameter
and use that to check which registers are available.

... does that make sense to you?

Thanks,
Mark.

> 
> Thank you,
> 
> > 
> > Co-developed-by: Florent Revest <revest@chromium.org>
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
> >  arch/s390/include/asm/ftrace.h    | 17 +++++++++++++++++
> >  arch/x86/include/asm/ftrace.h     | 14 ++++++++++++++
> >  include/linux/ftrace.h            | 27 +++++++++++++++++++++++++++
> >  kernel/trace/Kconfig              |  6 +++---
> >  5 files changed, 78 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> > index c3eb48f67566..faecb20d78bf 100644
> > --- a/arch/powerpc/include/asm/ftrace.h
> > +++ b/arch/powerpc/include/asm/ftrace.h
> > @@ -44,6 +44,23 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> >  	regs_set_return_ip(&fregs->regs, ip);
> >  }
> >  
> > +static __always_inline unsigned long
> > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > +{
> > +	return instruction_pointer(&fregs->regs)
> > +}
> > +
> > +#define ftrace_regs_get_argument(fregs, n) \
> > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > +#define ftrace_regs_get_stack_pointer(fregs) \
> > +	kernel_stack_pointer(&(fregs)->regs)
> > +#define ftrace_regs_return_value(fregs) \
> > +	regs_return_value(&(fregs)->regs)
> > +#define ftrace_regs_set_return_value(fregs, ret) \
> > +	regs_set_return_value(&(fregs)->regs, ret)
> > +#define ftrace_override_function_with_return(fregs) \
> > +	override_function_with_return(&(fregs)->regs)
> > +
> >  struct ftrace_ops;
> >  
> >  #define ftrace_graph_func ftrace_graph_func
> > diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> > index b8957882404f..5fdc806458aa 100644
> > --- a/arch/s390/include/asm/ftrace.h
> > +++ b/arch/s390/include/asm/ftrace.h
> > @@ -54,6 +54,12 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
> >  	return NULL;
> >  }
> >  
> > +static __always_inline unsigned long
> > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > +{
> > +	return fregs->regs.psw.addr;
> > +}
> > +
> >  static __always_inline void
> >  ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> >  				    unsigned long ip)
> > @@ -61,6 +67,17 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> >  	fregs->regs.psw.addr = ip;
> >  }
> >  
> > +#define ftrace_regs_get_argument(fregs, n) \
> > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > +#define ftrace_regs_get_stack_pointer(fregs) \
> > +	kernel_stack_pointer(&(fregs)->regs)
> > +#define ftrace_regs_return_value(fregs) \
> > +	regs_return_value(&(fregs)->regs)
> > +#define ftrace_regs_set_return_value(fregs, ret) \
> > +	regs_set_return_value(&(fregs)->regs, ret)
> > +#define ftrace_override_function_with_return(fregs) \
> > +	override_function_with_return(&(fregs)->regs)
> > +
> >  /*
> >   * When an ftrace registered caller is tracing a function that is
> >   * also set by a register_ftrace_direct() call, it needs to be
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index b73e858bd96f..b3737b42e8a1 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -51,6 +51,20 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
> >  #define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
> >  	do { (fregs)->regs.ip = (_ip); } while (0)
> >  
> > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > +	((fregs)->regs.ip)
> > +
> > +#define ftrace_regs_get_argument(fregs, n) \
> > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > +#define ftrace_regs_get_stack_pointer(fregs) \
> > +	kernel_stack_pointer(&(fregs)->regs)
> > +#define ftrace_regs_return_value(fregs) \
> > +	regs_return_value(&(fregs)->regs)
> > +#define ftrace_regs_set_return_value(fregs, ret) \
> > +	regs_set_return_value(&(fregs)->regs, ret)
> > +#define ftrace_override_function_with_return(fregs) \
> > +	override_function_with_return(&(fregs)->regs)
> > +
> >  struct ftrace_ops;
> >  #define ftrace_graph_func ftrace_graph_func
> >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index e9905f741916..3b13e3c21438 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -125,6 +125,33 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
> >  	return arch_ftrace_get_regs(fregs);
> >  }
> >  
> > +/*
> > + * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
> > + * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.
> > + */
> > +static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
> > +{
> > +	if (IS_ENABLED(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS))
> > +		return true;
> > +
> > +	return ftrace_get_regs(fregs) != NULL;
> > +}
> > +
> > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > +	instruction_pointer(ftrace_get_regs(fregs))
> > +#define ftrace_regs_get_argument(fregs, n) \
> > +	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
> > +#define ftrace_regs_get_stack_pointer(fregs) \
> > +	kernel_stack_pointer(ftrace_get_regs(fregs))
> > +#define ftrace_regs_return_value(fregs) \
> > +	regs_return_value(ftrace_get_regs(fregs))
> > +#define ftrace_regs_set_return_value(fregs, ret) \
> > +	regs_set_return_value(ftrace_get_regs(fregs), ret)
> > +#define ftrace_override_function_with_return(fregs) \
> > +	override_function_with_return(ftrace_get_regs(fregs))
> > +#endif
> > +
> >  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
> >  			      struct ftrace_ops *op, struct ftrace_regs *fregs);
> >  
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index e9e95c790b8e..2c6611c13f99 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -46,10 +46,10 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
> >  	bool
> >  	help
> >  	 If this is set, then arguments and stack can be found from
> > -	 the pt_regs passed into the function callback regs parameter
> > +	 the ftrace_regs passed into the function callback regs parameter
> >  	 by default, even without setting the REGS flag in the ftrace_ops.
> > -	 This allows for use of regs_get_kernel_argument() and
> > -	 kernel_stack_pointer().
> > +	 This allows for use of ftrace_regs_get_argument() and
> > +	 ftrace_regs_get_stack_pointer().
> >  
> >  config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
> >  	bool
> > -- 
> > 2.30.2
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()

[Florent Revest]

On Mon, Oct 24, 2022 at 4:08 PM Mark Rutland <mark.rutland@arm.com> wrote:
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2487,14 +2487,13 @@ ftrace_add_rec_direct(unsigned long ip, unsigned long addr,
>  static void call_direct_funcs(unsigned long ip, unsigned long pip,
>                               struct ftrace_ops *ops, struct ftrace_regs *fregs)
>  {
> -       struct pt_regs *regs = ftrace_get_regs(fregs);

Was this ftrace_get_regs() the only reason why we have
config DYNAMIC_FTRACE_WITH_DIRECT_CALLS
    depends on DYNAMIC_FTRACE_WITH_REGS
?

If we no longer use it, maybe this should be:
    depends on DYNAMIC_FTRACE_WITH_REGS || DYNAMIC_FTRACE_WITH_ARGS
?

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Florent Revest]

On Tue, Oct 25, 2022 at 12:30 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> > Hi Mark,
> >
> > On Mon, 24 Oct 2022 15:08:45 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > > In subsequent patches we'll arrange for architectures to have an
> > > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > > this, we need to minimize the use of pt_regs to where strictly necessary
> > > in the core ftrace code.
> > >
> > > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > > these can always be used on any ftrace_regs, and when
> > > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > > can use to check when these are usable.
> >
> > Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> > regs_query_register_offset()? I would like to use it for fprobe_events.
>
> Sure!
>
> Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
> pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?

Currently, we have:
config FPROBE
    depends on DYNAMIC_FTRACE_WITH_REGS

Because fprobe registers its ftrace ops with FTRACE_OPS_FL_SAVE_REGS
and calls ftrace_get_regs() to give pt_regs to registered fprobe
callbacks.

We'll need to refactor fprobe a bit to support ||
DYNAMIC_FTRACE_WITH_ARGS and therefore work on arm64.

> I ask because if neither of those are the case, with FTRACE_WITH_REGS,
> ftrace_regs_query_register_offset() would accept names of registers which might
> not have been sampled, and could give offsets to uninitialized memory.

Indeed, if one were to call the regs_query_register_offset() on a
pt_regs that comes out of a ftrace trampoline with FTRACE_WITH_REGS,
for example in a fprobe callback, one could get offsets to
uninitialized memory already (for the registers we can't get outside
of an exception handler on arm64 for example iiuc)

And it'd get way worse with FTRACE_WITH_ARGS if we implement
ftrace_regs_query_register_offset() by calling
regs_query_register_offset() for ftrace_regs that contain sparse
pt_regs.

> Atop that, I'm not exactly sure what to implement for powerpc/s390/x86 here. If
> those might be used without a full pt_regs, I think
> ftrace_regs_query_register_offset() should also take the fregs as a parameter
> and use that to check which registers are available.

I think it would make sense for a ftrace_regs_query_register_offset()
to only return offsets to the registers that are actually saved by the
arch in a ftrace_regs (whether that's WITH_ARGS or WITH_REGS).

But that also means that if we introduce "fprobe_events" in the
tracing sysfs interface, we can't have it support a %REG syntax
compatible with the one in "kprobe_events" anyway.

Masami, how about having "fprobe_events" only support $argN, $retval
etc but no %REG, from the beginning ? Then it would be clear to users
that fprobe can not guarantee registers and we'd never have to fake
registers when we don't have them. Users would have to make a decision
between using fprobe which is fast but only has arguments and return
value and kprobe which is slow but has all registers.

I realize this has consequences for the kretprobe and rethook
unification plan but if we have fprobe_events support %REG at the
beginning, we'd have to break it at some point down the line anyway
right ?

> ... does that make sense to you?
>
> Thanks,
> Mark.

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Masami Hiramatsu]

On Tue, 25 Oct 2022 11:30:38 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> > Hi Mark,
> > 
> > On Mon, 24 Oct 2022 15:08:45 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > In subsequent patches we'll arrange for architectures to have an
> > > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > > this, we need to minimize the use of pt_regs to where strictly necessary
> > > in the core ftrace code.
> > > 
> > > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > > these can always be used on any ftrace_regs, and when
> > > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > > can use to check when these are usable.
> > 
> > Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> > regs_query_register_offset()? I would like to use it for fprobe_events.
> 
> Sure!
> 
> Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
> pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?

No, please return -ENOENT or any error value if the given register
is not saved on arm64. Others will just return
 regs_query_register_offset(&fregs->regs, name). That is enough
at this moment. Later we can improve it.

> I ask because if neither of those are the case, with FTRACE_WITH_REGS,
> ftrace_regs_query_register_offset() would accept names of registers which might
> not have been sampled, and could give offsets to uninitialized memory.

Currently fprobe depends on CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS, but
in the future, I will move it on WITH_ARGS.

> Atop that, I'm not exactly sure what to implement for powerpc/s390/x86 here. If
> those might be used without a full pt_regs, I think
> ftrace_regs_query_register_offset() should also take the fregs as a parameter
> and use that to check which registers are available.
> 
> ... does that make sense to you?

Yeah, that is OK. I think only arm64 changes the ftrace_regs not wraps
pt_regs. So there is no problem even if we access the empty register.
Only arm64 implementation is different, so it should have different
implementation.

Thank you,

> 
> Thanks,
> Mark.
> 
> > 
> > Thank you,
> > 
> > > 
> > > Co-developed-by: Florent Revest <revest@chromium.org>
> > > Signed-off-by: Florent Revest <revest@chromium.org>
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > >  arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
> > >  arch/s390/include/asm/ftrace.h    | 17 +++++++++++++++++
> > >  arch/x86/include/asm/ftrace.h     | 14 ++++++++++++++
> > >  include/linux/ftrace.h            | 27 +++++++++++++++++++++++++++
> > >  kernel/trace/Kconfig              |  6 +++---
> > >  5 files changed, 78 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> > > index c3eb48f67566..faecb20d78bf 100644
> > > --- a/arch/powerpc/include/asm/ftrace.h
> > > +++ b/arch/powerpc/include/asm/ftrace.h
> > > @@ -44,6 +44,23 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  	regs_set_return_ip(&fregs->regs, ip);
> > >  }
> > >  
> > > +static __always_inline unsigned long
> > > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > > +{
> > > +	return instruction_pointer(&fregs->regs)
> > > +}
> > > +
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  struct ftrace_ops;
> > >  
> > >  #define ftrace_graph_func ftrace_graph_func
> > > diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> > > index b8957882404f..5fdc806458aa 100644
> > > --- a/arch/s390/include/asm/ftrace.h
> > > +++ b/arch/s390/include/asm/ftrace.h
> > > @@ -54,6 +54,12 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
> > >  	return NULL;
> > >  }
> > >  
> > > +static __always_inline unsigned long
> > > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > > +{
> > > +	return fregs->regs.psw.addr;
> > > +}
> > > +
> > >  static __always_inline void
> > >  ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  				    unsigned long ip)
> > > @@ -61,6 +67,17 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  	fregs->regs.psw.addr = ip;
> > >  }
> > >  
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  /*
> > >   * When an ftrace registered caller is tracing a function that is
> > >   * also set by a register_ftrace_direct() call, it needs to be
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index b73e858bd96f..b3737b42e8a1 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -51,6 +51,20 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
> > >  #define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
> > >  	do { (fregs)->regs.ip = (_ip); } while (0)
> > >  
> > > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > > +	((fregs)->regs.ip)
> > > +
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  struct ftrace_ops;
> > >  #define ftrace_graph_func ftrace_graph_func
> > >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index e9905f741916..3b13e3c21438 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -125,6 +125,33 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
> > >  	return arch_ftrace_get_regs(fregs);
> > >  }
> > >  
> > > +/*
> > > + * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
> > > + * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.
> > > + */
> > > +static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
> > > +{
> > > +	if (IS_ENABLED(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS))
> > > +		return true;
> > > +
> > > +	return ftrace_get_regs(fregs) != NULL;
> > > +}
> > > +
> > > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > > +	instruction_pointer(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(ftrace_get_regs(fregs), ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(ftrace_get_regs(fregs))
> > > +#endif
> > > +
> > >  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
> > >  			      struct ftrace_ops *op, struct ftrace_regs *fregs);
> > >  
> > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > > index e9e95c790b8e..2c6611c13f99 100644
> > > --- a/kernel/trace/Kconfig
> > > +++ b/kernel/trace/Kconfig
> > > @@ -46,10 +46,10 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > >  	bool
> > >  	help
> > >  	 If this is set, then arguments and stack can be found from
> > > -	 the pt_regs passed into the function callback regs parameter
> > > +	 the ftrace_regs passed into the function callback regs parameter
> > >  	 by default, even without setting the REGS flag in the ftrace_ops.
> > > -	 This allows for use of regs_get_kernel_argument() and
> > > -	 kernel_stack_pointer().
> > > +	 This allows for use of ftrace_regs_get_argument() and
> > > +	 ftrace_regs_get_stack_pointer().
> > >  
> > >  config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
> > >  	bool
> > > -- 
> > > 2.30.2
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Masami Hiramatsu]

On Tue, 25 Oct 2022 15:20:57 +0200
Florent Revest <revest@chromium.org> wrote:

> On Tue, Oct 25, 2022 at 12:30 PM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> > > Hi Mark,
> > >
> > > On Mon, 24 Oct 2022 15:08:45 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > > In subsequent patches we'll arrange for architectures to have an
> > > > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > > > this, we need to minimize the use of pt_regs to where strictly necessary
> > > > in the core ftrace code.
> > > >
> > > > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > > > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > > > these can always be used on any ftrace_regs, and when
> > > > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > > > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > > > can use to check when these are usable.
> > >
> > > Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> > > regs_query_register_offset()? I would like to use it for fprobe_events.
> >
> > Sure!
> >
> > Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
> > pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?

No, if the register is NOT saved, just return -ENOENT.

> 
> Currently, we have:
> config FPROBE
>     depends on DYNAMIC_FTRACE_WITH_REGS
> 
> Because fprobe registers its ftrace ops with FTRACE_OPS_FL_SAVE_REGS
> and calls ftrace_get_regs() to give pt_regs to registered fprobe
> callbacks.
> 
> We'll need to refactor fprobe a bit to support ||
> DYNAMIC_FTRACE_WITH_ARGS and therefore work on arm64.

Yeah, that is what I will do.

> > I ask because if neither of those are the case, with FTRACE_WITH_REGS,
> > ftrace_regs_query_register_offset() would accept names of registers which might
> > not have been sampled, and could give offsets to uninitialized memory.
> 
> Indeed, if one were to call the regs_query_register_offset() on a
> pt_regs that comes out of a ftrace trampoline with FTRACE_WITH_REGS,
> for example in a fprobe callback, one could get offsets to
> uninitialized memory already (for the registers we can't get outside
> of an exception handler on arm64 for example iiuc)
> 
> And it'd get way worse with FTRACE_WITH_ARGS if we implement
> ftrace_regs_query_register_offset() by calling
> regs_query_register_offset() for ftrace_regs that contain sparse
> pt_regs.

Ah, I got what you meant. If you consider that case, it can return
-ENOTSUPPORT for FTRACE_WITH_ARGS case at this moment. I'll fill it
afterwards.

> 
> > Atop that, I'm not exactly sure what to implement for powerpc/s390/x86 here. If
> > those might be used without a full pt_regs, I think
> > ftrace_regs_query_register_offset() should also take the fregs as a parameter
> > and use that to check which registers are available.
> 
> I think it would make sense for a ftrace_regs_query_register_offset()
> to only return offsets to the registers that are actually saved by the
> arch in a ftrace_regs (whether that's WITH_ARGS or WITH_REGS).
> 
> But that also means that if we introduce "fprobe_events" in the
> tracing sysfs interface, we can't have it support a %REG syntax
> compatible with the one in "kprobe_events" anyway.
> 
> Masami, how about having "fprobe_events" only support $argN, $retval
> etc but no %REG, from the beginning ? Then it would be clear to users
> that fprobe can not guarantee registers and we'd never have to fake
> registers when we don't have them. Users would have to make a decision
> between using fprobe which is fast but only has arguments and return
> value and kprobe which is slow but has all registers.

Hmm, for the first implementation, it is OK. But later I need to
implement the register access, because, debuginfo maps it differently.
I would like to arrow perf-probe to set it up eventually.

> I realize this has consequences for the kretprobe and rethook
> unification plan but if we have fprobe_events support %REG at the
> beginning, we'd have to break it at some point down the line anyway
> right ?

No, because %REG support doesn't mean we guarantee to access
all registers. We can just use %REG as alias of $argN :)

Thank you,

> 
> > ... does that make sense to you?
> >
> > Thanks,
> > Mark.


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()

[Mark Rutland]

On Mon, Oct 24, 2022 at 06:04:49PM +0100, Mark Rutland wrote:
> On Mon, Oct 24, 2022 at 10:48:45AM -0400, Steven Rostedt wrote:
> > On Mon, 24 Oct 2022 15:08:43 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -429,6 +429,7 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
> > >  }
> > >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> > >  
> > > +#ifdef CONFIG_FUNCTION_TRACER
> > 
> > Instead of adding the above preprocessor check, the below chunk should be
> > moved into the CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block above.
> 
> Sure; but note that doing that naively means 'struct ftrace_regs' won't always
> be declared (e.g. if !CONFIG_FUNCTION_TRACER), and will result in warnings, e.g.
> 
> |   CC      arch/x86/kernel/asm-offsets.s
> | In file included from ./include/linux/kvm_host.h:32,
> |                  from arch/x86/kernel/../kvm/vmx/vmx.h:5,
> |                  from arch/x86/kernel/asm-offsets.c:22:
> | ./include/linux/ftrace.h:444:57: error: ‘struct ftrace_regs’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
> |   444 | static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs,
> |       |                                                         ^~~~~~~~~~~
> | cc1: all warnings being treated as errors
> | make[1]: *** [scripts/Makefile.build:118: arch/x86/kernel/asm-offsets.s] Error 1
> | make: *** [Makefile:1270: prepare0] Error 2
> 
> ... so I'll either need to add some ifdeffery, for CONFIG_FUNCTION_TRACER, or I
> can hoist the declaration of 'struct ftrace_regs' to not depend on
> CONFIG_FUNCTION_TRACER.
> 
> I guess the latter is preferable, e.g.
> 
> | diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> | index 2b34fec40a39..f201fcbfffb0 100644
> | --- a/include/linux/ftrace.h
> | +++ b/include/linux/ftrace.h
> | @@ -37,9 +37,10 @@ extern void ftrace_boot_snapshot(void);
> |  static inline void ftrace_boot_snapshot(void) { }
> |  #endif
> |  
> | -#ifdef CONFIG_FUNCTION_TRACER
> |  struct ftrace_ops;
> |  struct ftrace_regs;
> | +
> | +#ifdef CONFIG_FUNCTION_TRACER
> |  /*
> |   * If the arch's mcount caller does not support all of ftrace's
> |   * features, then it must call an indirect function that
> 
> ... so I've done that locally for now.

The kbuild robot complained that this led to x86 getting multiple definitions
of arch_ftrace_set_direct_caller(), so I've also added the below:

| diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
| index 788e7c1f6463c..6f9b9fee04132 100644
| --- a/arch/x86/include/asm/ftrace.h
| +++ b/arch/x86/include/asm/ftrace.h
| @@ -59,6 +59,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
|  #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
|  #endif
|  
| +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
|  /*
|   * When a ftrace registered caller is tracing a function that is
|   * also set by a register_ftrace_direct() call, it needs to be
| @@ -71,6 +72,7 @@ static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs, unsi
|         /* Emulate a call */
|         fregs->regs.orig_ax = addr;
|  }
| +#endif
|  
|  #ifdef CONFIG_DYNAMIC_FTRACE

... AFAICT s390 doesn't need similar treatment.

Thanks,
Mark.

Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

[Mark Rutland]

On Wed, Oct 26, 2022 at 12:17:54AM +0900, Masami Hiramatsu wrote:
> On Tue, 25 Oct 2022 11:30:38 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> > > Hi Mark,
> > > 
> > > On Mon, 24 Oct 2022 15:08:45 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > > In subsequent patches we'll arrange for architectures to have an
> > > > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > > > this, we need to minimize the use of pt_regs to where strictly necessary
> > > > in the core ftrace code.
> > > > 
> > > > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > > > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > > > these can always be used on any ftrace_regs, and when
> > > > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > > > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > > > can use to check when these are usable.
> > > 
> > > Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> > > regs_query_register_offset()? I would like to use it for fprobe_events.
> > 
> > Sure!
> > 
> > Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
> > pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?
> 
> No, please return -ENOENT or any error value if the given register
> is not saved on arm64.

Sure, that's what I intend to implement for arm64. I'll use -EINVAL to match
the existing regs_query_register_offset() logic.

> Others will just return regs_query_register_offset(&fregs->regs, name). That
> is enough at this moment. Later we can improve it.

Sorry, what I was trying to ask was whether fprobe currently always set
FTRACE_OPS_FL_SAVE_REGS (which AFAICT it does); so I now agree that's
sufficient -- sorry for the noise!

Thanks,
Mark.