Re: [PATCH v14 03/19] function_graph: Pass ftrace_regs to entryfunc

Re: [PATCH v14 03/19] function_graph: Pass ftrace_regs to entryfunc

[Steven Rostedt]

Can I get an Acked-by from the AARCH64 maintainers for this patch?

Thanks!

-- Steve


On Fri, 13 Sep 2024 00:08:40 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Pass ftrace_regs to the fgraph_ops::entryfunc(). If ftrace_regs is not
> available, it passes a NULL instead. User callback function can access
> some registers (including return address) via this ftrace_regs.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v11:
>   - Update for the latest for-next branch.
>  Changes in v8:
>   - Just pass ftrace_regs to the handler instead of adding a new
>     entryregfunc.
>   - Update riscv ftrace_graph_func().
>  Changes in v3:
>   - Update for new multiple fgraph.
> ---
>  arch/arm64/kernel/ftrace.c               |   20 +++++++++++-
>  arch/loongarch/kernel/ftrace_dyn.c       |   10 +++++-
>  arch/powerpc/kernel/trace/ftrace.c       |    2 +
>  arch/powerpc/kernel/trace/ftrace_64_pg.c |   10 ++++--
>  arch/riscv/kernel/ftrace.c               |   17 ++++++++++
>  arch/x86/kernel/ftrace.c                 |   50 +++++++++++++++++++++---------
>  include/linux/ftrace.h                   |   18 ++++++++---
>  kernel/trace/fgraph.c                    |   23 ++++++++------
>  kernel/trace/ftrace.c                    |    3 +-
>  kernel/trace/trace.h                     |    3 +-
>  kernel/trace/trace_functions_graph.c     |    3 +-
>  kernel/trace/trace_irqsoff.c             |    3 +-
>  kernel/trace/trace_sched_wakeup.c        |    3 +-
>  kernel/trace/trace_selftest.c            |    8 +++--
>  14 files changed, 128 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index a650f5e11fc5..bc647b725e6a 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -481,7 +481,25 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>  {
> -	prepare_ftrace_return(ip, &fregs->lr, fregs->fp);
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	unsigned long frame_pointer = fregs->fp;
> +	unsigned long *parent = &fregs->lr;
> +	unsigned long old;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	/*
> +	 * Note:
> +	 * No protection against faulting at *parent, which may be seen
> +	 * on other archs. It's unlikely on AArch64.
> +	 */
> +	old = *parent;
> +
> +	if (!function_graph_enter_regs(old, ip, frame_pointer,
> +				       (void *)frame_pointer, fregs)) {
> +		*parent = return_hooker;
> +	}
>  }
>  #else
>  /*
> diff --git a/arch/loongarch/kernel/ftrace_dyn.c b/arch/loongarch/kernel/ftrace_dyn.c
> index bff058317062..966e0f7f7aca 100644
> --- a/arch/loongarch/kernel/ftrace_dyn.c
> +++ b/arch/loongarch/kernel/ftrace_dyn.c
> @@ -243,8 +243,16 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  {
>  	struct pt_regs *regs = &fregs->regs;
>  	unsigned long *parent = (unsigned long *)&regs->regs[1];
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	unsigned long old;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	old = *parent;
>  
> -	prepare_ftrace_return(ip, (unsigned long *)parent);
> +	if (!function_graph_enter_regs(old, ip, 0, parent, fregs))
> +		*parent = return_hooker;
>  }
>  #else
>  static int ftrace_modify_graph_caller(bool enable)
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index d8d6b4fd9a14..a1a0e0b57662 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -434,7 +434,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  	if (bit < 0)
>  		goto out;
>  
> -	if (!function_graph_enter(parent_ip, ip, 0, (unsigned long *)sp))
> +	if (!function_graph_enter_regs(parent_ip, ip, 0, (unsigned long *)sp, fregs))
>  		parent_ip = ppc_function_entry(return_to_handler);
>  
>  	ftrace_test_recursion_unlock(bit);
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.c b/arch/powerpc/kernel/trace/ftrace_64_pg.c
> index 12fab1803bcf..4ae9eeb1c8f1 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_pg.c
> +++ b/arch/powerpc/kernel/trace/ftrace_64_pg.c
> @@ -800,7 +800,8 @@ int ftrace_disable_ftrace_graph_caller(void)
>   * in current thread info. Return the address we want to divert to.
>   */
>  static unsigned long
> -__prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp)
> +__prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp,
> +			struct ftrace_regs *fregs)
>  {
>  	unsigned long return_hooker;
>  	int bit;
> @@ -817,7 +818,7 @@ __prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp
>  
>  	return_hooker = ppc_function_entry(return_to_handler);
>  
> -	if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
> +	if (!function_graph_enter_regs(parent, ip, 0, (unsigned long *)sp, fregs))
>  		parent = return_hooker;
>  
>  	ftrace_test_recursion_unlock(bit);
> @@ -829,13 +830,14 @@ __prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>  {
> -	fregs->regs.link = __prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
> +	fregs->regs.link = __prepare_ftrace_return(parent_ip, ip,
> +						   fregs->regs.gpr[1], fregs);
>  }
>  #else
>  unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>  				    unsigned long sp)
>  {
> -	return __prepare_ftrace_return(parent, ip, sp);
> +	return __prepare_ftrace_return(parent, ip, sp, NULL);
>  }
>  #endif
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 4b95c574fd04..f5c3b5a752b0 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -214,7 +214,22 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>  {
> -	prepare_ftrace_return(&fregs->ra, ip, fregs->s0);
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	unsigned long frame_pointer = fregs->s0;
> +	unsigned long *parent = &fregs->ra;
> +	unsigned long old;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	/*
> +	 * We don't suffer access faults, so no extra fault-recovery assembly
> +	 * is needed here.
> +	 */
> +	old = *parent;
> +
> +	if (!function_graph_enter_regs(old, ip, frame_pointer, parent, fregs))
> +		*parent = return_hooker;
>  }
>  #else /* CONFIG_DYNAMIC_FTRACE_WITH_ARGS */
>  extern void ftrace_graph_call(void);
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8da0e66ca22d..decf4c11dcf3 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -605,16 +605,8 @@ int ftrace_disable_ftrace_graph_caller(void)
>  }
>  #endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>  
> -/*
> - * Hook the return address and push it in the stack of return addrs
> - * in current thread info.
> - */
> -void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
> -			   unsigned long frame_pointer)
> +static inline bool skip_ftrace_return(void)
>  {
> -	unsigned long return_hooker = (unsigned long)&return_to_handler;
> -	int bit;
> -
>  	/*
>  	 * When resuming from suspend-to-ram, this function can be indirectly
>  	 * called from early CPU startup code while the CPU is in real mode,
> @@ -624,13 +616,28 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
>  	 * This check isn't as accurate as virt_addr_valid(), but it should be
>  	 * good enough for this purpose, and it's fast.
>  	 */
> -	if (unlikely((long)__builtin_frame_address(0) >= 0))
> -		return;
> +	if ((long)__builtin_frame_address(0) >= 0)
> +		return true;
>  
> -	if (unlikely(ftrace_graph_is_dead()))
> -		return;
> +	if (ftrace_graph_is_dead())
> +		return true;
> +
> +	if (atomic_read(&current->tracing_graph_pause))
> +		return true;
> +	return false;
> +}
> +
> +/*
> + * Hook the return address and push it in the stack of return addrs
> + * in current thread info.
> + */
> +void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
> +			   unsigned long frame_pointer)
> +{
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	int bit;
>  
> -	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +	if (unlikely(skip_ftrace_return()))
>  		return;
>  
>  	bit = ftrace_test_recursion_trylock(ip, *parent);
> @@ -649,8 +656,21 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  {
>  	struct pt_regs *regs = &fregs->regs;
>  	unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	unsigned long *parent = (unsigned long *)stack;
> +	int bit;
>  
> -	prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> +	if (unlikely(skip_ftrace_return()))
> +		return;
> +
> +	bit = ftrace_test_recursion_trylock(ip, *parent);
> +	if (bit < 0)
> +		return;
> +
> +	if (!function_graph_enter_regs(*parent, ip, 0, parent, fregs))
> +		*parent = return_hooker;
> +
> +	ftrace_test_recursion_unlock(bit);
>  }
>  #endif
>  
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index f84fb9635fb0..1fe49a28de2d 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1063,9 +1063,12 @@ struct fgraph_ops;
>  typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *,
>  				       struct fgraph_ops *); /* return */
>  typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *,
> -				      struct fgraph_ops *); /* entry */
> +				      struct fgraph_ops *,
> +				      struct ftrace_regs *); /* entry */
>  
> -extern int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace, struct fgraph_ops *gops);
> +extern int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
> +				   struct fgraph_ops *gops,
> +				   struct ftrace_regs *fregs);
>  bool ftrace_pids_enabled(struct ftrace_ops *ops);
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> @@ -1108,8 +1111,15 @@ struct ftrace_ret_stack {
>  extern void return_to_handler(void);
>  
>  extern int
> -function_graph_enter(unsigned long ret, unsigned long func,
> -		     unsigned long frame_pointer, unsigned long *retp);
> +function_graph_enter_regs(unsigned long ret, unsigned long func,
> +			  unsigned long frame_pointer, unsigned long *retp,
> +			  struct ftrace_regs *fregs);
> +
> +static inline int function_graph_enter(unsigned long ret, unsigned long func,
> +				       unsigned long fp, unsigned long *retp)
> +{
> +	return function_graph_enter_regs(ret, func, fp, retp, NULL);
> +}
>  
>  struct ftrace_ret_stack *
>  ftrace_graph_get_ret_stack(struct task_struct *task, int skip);
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index d7d4fb403f6f..0322c5723748 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -290,7 +290,8 @@ static inline unsigned long make_data_type_val(int idx, int size, int offset)
>  }
>  
>  /* ftrace_graph_entry set to this to tell some archs to run function graph */
> -static int entry_run(struct ftrace_graph_ent *trace, struct fgraph_ops *ops)
> +static int entry_run(struct ftrace_graph_ent *trace, struct fgraph_ops *ops,
> +		     struct ftrace_regs *fregs)
>  {
>  	return 0;
>  }
> @@ -484,7 +485,7 @@ int __weak ftrace_disable_ftrace_graph_caller(void)
>  #endif
>  
>  int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
> -			    struct fgraph_ops *gops)
> +			    struct fgraph_ops *gops, struct ftrace_regs *fregs)
>  {
>  	return 0;
>  }
> @@ -612,8 +613,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func,
>  #endif
>  
>  /* If the caller does not use ftrace, call this function. */
> -int function_graph_enter(unsigned long ret, unsigned long func,
> -			 unsigned long frame_pointer, unsigned long *retp)
> +int function_graph_enter_regs(unsigned long ret, unsigned long func,
> +			      unsigned long frame_pointer, unsigned long *retp,
> +			      struct ftrace_regs *fregs)
>  {
>  	struct ftrace_graph_ent trace;
>  	unsigned long bitmap = 0;
> @@ -631,7 +633,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
>  	if (static_branch_likely(&fgraph_do_direct)) {
>  		int save_curr_ret_stack = current->curr_ret_stack;
>  
> -		if (static_call(fgraph_func)(&trace, fgraph_direct_gops))
> +		if (static_call(fgraph_func)(&trace, fgraph_direct_gops, fregs))
>  			bitmap |= BIT(fgraph_direct_gops->idx);
>  		else
>  			/* Clear out any saved storage */
> @@ -649,7 +651,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
>  
>  			save_curr_ret_stack = current->curr_ret_stack;
>  			if (ftrace_ops_test(&gops->ops, func, NULL) &&
> -			    gops->entryfunc(&trace, gops))
> +			    gops->entryfunc(&trace, gops, fregs))
>  				bitmap |= BIT(i);
>  			else
>  				/* Clear out any saved storage */
> @@ -925,7 +927,7 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
>  
>  static struct ftrace_ops graph_ops = {
>  	.func			= ftrace_graph_func,
> -	.flags			= FTRACE_OPS_GRAPH_STUB,
> +	.flags			= FTRACE_OPS_GRAPH_STUB | FTRACE_OPS_FL_SAVE_ARGS,
>  #ifdef FTRACE_GRAPH_TRAMP_ADDR
>  	.trampoline		= FTRACE_GRAPH_TRAMP_ADDR,
>  	/* trampoline_size is only needed for dynamically allocated tramps */
> @@ -935,7 +937,8 @@ static struct ftrace_ops graph_ops = {
>  void fgraph_init_ops(struct ftrace_ops *dst_ops,
>  		     struct ftrace_ops *src_ops)
>  {
> -	dst_ops->flags = FTRACE_OPS_FL_PID | FTRACE_OPS_GRAPH_STUB;
> +	dst_ops->flags = FTRACE_OPS_FL_PID | FTRACE_OPS_GRAPH_STUB |
> +			 FTRACE_OPS_FL_SAVE_ARGS;
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	if (src_ops) {
> @@ -1119,7 +1122,7 @@ void ftrace_graph_exit_task(struct task_struct *t)
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  static int fgraph_pid_func(struct ftrace_graph_ent *trace,
> -			   struct fgraph_ops *gops)
> +			   struct fgraph_ops *gops, struct ftrace_regs *fregs)
>  {
>  	struct trace_array *tr = gops->ops.private;
>  	int pid;
> @@ -1133,7 +1136,7 @@ static int fgraph_pid_func(struct ftrace_graph_ent *trace,
>  			return 0;
>  	}
>  
> -	return gops->saved_func(trace, gops);
> +	return gops->saved_func(trace, gops, fregs);
>  }
>  
>  void fgraph_update_pid_func(void)
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4c28dd177ca6..775040a9f541 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -821,7 +821,8 @@ void ftrace_graph_graph_time_control(bool enable)
>  }
>  
>  static int profile_graph_entry(struct ftrace_graph_ent *trace,
> -			       struct fgraph_ops *gops)
> +			       struct fgraph_ops *gops,
> +			       struct ftrace_regs *fregs)
>  {
>  	struct ftrace_ret_stack *ret_stack;
>  
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index bd3e3069300e..28d8ad5e31e6 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -683,7 +683,8 @@ void trace_default_header(struct seq_file *m);
>  void print_trace_header(struct seq_file *m, struct trace_iterator *iter);
>  
>  void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops);
> -int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops);
> +int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
> +		      struct ftrace_regs *fregs);
>  
>  void tracing_start_cmdline_record(void);
>  void tracing_stop_cmdline_record(void);
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 13d0387ac6a6..b9785fc919c9 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -128,7 +128,8 @@ static inline int ftrace_graph_ignore_irqs(void)
>  }
>  
>  int trace_graph_entry(struct ftrace_graph_ent *trace,
> -		      struct fgraph_ops *gops)
> +		      struct fgraph_ops *gops,
> +		      struct ftrace_regs *fregs)
>  {
>  	unsigned long *task_var = fgraph_get_task_var(gops);
>  	struct trace_array *tr = gops->private;
> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
> index fce064e20570..ad739d76fc86 100644
> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -176,7 +176,8 @@ static int irqsoff_display_graph(struct trace_array *tr, int set)
>  }
>  
>  static int irqsoff_graph_entry(struct ftrace_graph_ent *trace,
> -			       struct fgraph_ops *gops)
> +			       struct fgraph_ops *gops,
> +			       struct ftrace_regs *fregs)
>  {
>  	struct trace_array *tr = irqsoff_trace;
>  	struct trace_array_cpu *data;
> diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
> index 130ca7e7787e..23360a2700de 100644
> --- a/kernel/trace/trace_sched_wakeup.c
> +++ b/kernel/trace/trace_sched_wakeup.c
> @@ -113,7 +113,8 @@ static int wakeup_display_graph(struct trace_array *tr, int set)
>  }
>  
>  static int wakeup_graph_entry(struct ftrace_graph_ent *trace,
> -			      struct fgraph_ops *gops)
> +			      struct fgraph_ops *gops,
> +			      struct ftrace_regs *fregs)
>  {
>  	struct trace_array *tr = wakeup_trace;
>  	struct trace_array_cpu *data;
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index c4ad7cd7e778..89067f02094a 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -773,7 +773,8 @@ struct fgraph_fixture {
>  };
>  
>  static __init int store_entry(struct ftrace_graph_ent *trace,
> -			      struct fgraph_ops *gops)
> +			      struct fgraph_ops *gops,
> +			      struct ftrace_regs *fregs)
>  {
>  	struct fgraph_fixture *fixture = container_of(gops, struct fgraph_fixture, gops);
>  	const char *type = fixture->store_type_name;
> @@ -1024,7 +1025,8 @@ static unsigned int graph_hang_thresh;
>  
>  /* Wrap the real function entry probe to avoid possible hanging */
>  static int trace_graph_entry_watchdog(struct ftrace_graph_ent *trace,
> -				      struct fgraph_ops *gops)
> +				      struct fgraph_ops *gops,
> +				      struct ftrace_regs *fregs)
>  {
>  	/* This is harmlessly racy, we want to approximately detect a hang */
>  	if (unlikely(++graph_hang_thresh > GRAPH_MAX_FUNC_TEST)) {
> @@ -1038,7 +1040,7 @@ static int trace_graph_entry_watchdog(struct ftrace_graph_ent *trace,
>  		return 0;
>  	}
>  
> -	return trace_graph_entry(trace, gops);
> +	return trace_graph_entry(trace, gops, fregs);
>  }
>  
>  static struct fgraph_ops fgraph_ops __initdata  = {

Re: [PATCH v14 05/19] function_graph: Pass ftrace_regs to retfunc

[Steven Rostedt]

Can I get an Acked-by from the AARCH64 maintainers for this patch?

Thanks!

-- Steve

[ Note this is modifies the return side ]

On Fri, 13 Sep 2024 00:09:02 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Pass ftrace_regs to the fgraph_ops::retfunc(). If ftrace_regs is not
> available, it passes a NULL instead. User callback function can access
> some registers (including return address) via this ftrace_regs.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v8:
>   - Pass ftrace_regs to retfunc, instead of adding retregfunc.
>  Changes in v6:
>   - update to use ftrace_regs_get_return_value() because of reordering
>     patches.
>  Changes in v3:
>   - Update for new multiple fgraph.
>   - Save the return address to instruction pointer in ftrace_regs.
> ---
>  include/linux/ftrace.h               |    3 ++-
>  kernel/trace/fgraph.c                |   16 +++++++++++-----
>  kernel/trace/ftrace.c                |    3 ++-
>  kernel/trace/trace.h                 |    3 ++-
>  kernel/trace/trace_functions_graph.c |    7 ++++---
>  kernel/trace/trace_irqsoff.c         |    3 ++-
>  kernel/trace/trace_sched_wakeup.c    |    3 ++-
>  kernel/trace/trace_selftest.c        |    3 ++-
>  8 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 13987cd63553..e7c41d9988e1 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1069,7 +1069,8 @@ struct fgraph_ops;
>  
>  /* Type of the callback handlers for tracing function graph*/
>  typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *,
> -				       struct fgraph_ops *); /* return */
> +				       struct fgraph_ops *,
> +				       struct ftrace_regs *); /* return */
>  typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *,
>  				      struct fgraph_ops *,
>  				      struct ftrace_regs *); /* entry */
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 30bebe43607d..6a3e2db16aa4 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -297,7 +297,8 @@ static int entry_run(struct ftrace_graph_ent *trace, struct fgraph_ops *ops,
>  }
>  
>  /* ftrace_graph_return set to this to tell some archs to run function graph */
> -static void return_run(struct ftrace_graph_ret *trace, struct fgraph_ops *ops)
> +static void return_run(struct ftrace_graph_ret *trace, struct fgraph_ops *ops,
> +		       struct ftrace_regs *fregs)
>  {
>  }
>  
> @@ -491,7 +492,8 @@ int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
>  }
>  
>  static void ftrace_graph_ret_stub(struct ftrace_graph_ret *trace,
> -				  struct fgraph_ops *gops)
> +				  struct fgraph_ops *gops,
> +				  struct ftrace_regs *fregs)
>  {
>  }
>  
> @@ -787,6 +789,9 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
>  	}
>  
>  	trace.rettime = trace_clock_local();
> +	if (fregs)
> +		ftrace_regs_set_instruction_pointer(fregs, ret);
> +
>  #ifdef CONFIG_FUNCTION_GRAPH_RETVAL
>  	trace.retval = ftrace_regs_get_return_value(fregs);
>  #endif
> @@ -796,7 +801,7 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
>  #ifdef CONFIG_HAVE_STATIC_CALL
>  	if (static_branch_likely(&fgraph_do_direct)) {
>  		if (test_bit(fgraph_direct_gops->idx, &bitmap))
> -			static_call(fgraph_retfunc)(&trace, fgraph_direct_gops);
> +			static_call(fgraph_retfunc)(&trace, fgraph_direct_gops, fregs);
>  	} else
>  #endif
>  	{
> @@ -806,7 +811,7 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
>  			if (gops == &fgraph_stub)
>  				continue;
>  
> -			gops->retfunc(&trace, gops);
> +			gops->retfunc(&trace, gops, fregs);
>  		}
>  	}
>  
> @@ -956,7 +961,8 @@ void ftrace_graph_sleep_time_control(bool enable)
>   * Simply points to ftrace_stub, but with the proper protocol.
>   * Defined by the linker script in linux/vmlinux.lds.h
>   */
> -void ftrace_stub_graph(struct ftrace_graph_ret *trace, struct fgraph_ops *gops);
> +void ftrace_stub_graph(struct ftrace_graph_ret *trace, struct fgraph_ops *gops,
> +		       struct ftrace_regs *fregs);
>  
>  /* The callbacks that hook a function */
>  trace_func_graph_ret_t ftrace_graph_return = ftrace_stub_graph;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 775040a9f541..fd6c5a50c5e5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -840,7 +840,8 @@ static int profile_graph_entry(struct ftrace_graph_ent *trace,
>  }
>  
>  static void profile_graph_return(struct ftrace_graph_ret *trace,
> -				 struct fgraph_ops *gops)
> +				 struct fgraph_ops *gops,
> +				 struct ftrace_regs *fregs)
>  {
>  	struct ftrace_ret_stack *ret_stack;
>  	struct ftrace_profile_stat *stat;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 28d8ad5e31e6..f4a3f75bd916 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -682,7 +682,8 @@ void trace_latency_header(struct seq_file *m);
>  void trace_default_header(struct seq_file *m);
>  void print_trace_header(struct seq_file *m, struct trace_iterator *iter);
>  
> -void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops);
> +void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops,
> +			struct ftrace_regs *fregs);
>  int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
>  		      struct ftrace_regs *fregs);
>  
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index b9785fc919c9..241407000109 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -240,7 +240,7 @@ void __trace_graph_return(struct trace_array *tr,
>  }
>  
>  void trace_graph_return(struct ftrace_graph_ret *trace,
> -			struct fgraph_ops *gops)
> +			struct fgraph_ops *gops, struct ftrace_regs *fregs)
>  {
>  	unsigned long *task_var = fgraph_get_task_var(gops);
>  	struct trace_array *tr = gops->private;
> @@ -270,7 +270,8 @@ void trace_graph_return(struct ftrace_graph_ret *trace,
>  }
>  
>  static void trace_graph_thresh_return(struct ftrace_graph_ret *trace,
> -				      struct fgraph_ops *gops)
> +				      struct fgraph_ops *gops,
> +				      struct ftrace_regs *fregs)
>  {
>  	ftrace_graph_addr_finish(gops, trace);
>  
> @@ -283,7 +284,7 @@ static void trace_graph_thresh_return(struct ftrace_graph_ret *trace,
>  	    (trace->rettime - trace->calltime < tracing_thresh))
>  		return;
>  	else
> -		trace_graph_return(trace, gops);
> +		trace_graph_return(trace, gops, fregs);
>  }
>  
>  static struct fgraph_ops funcgraph_ops = {
> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
> index ad739d76fc86..504de7a05498 100644
> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -208,7 +208,8 @@ static int irqsoff_graph_entry(struct ftrace_graph_ent *trace,
>  }
>  
>  static void irqsoff_graph_return(struct ftrace_graph_ret *trace,
> -				 struct fgraph_ops *gops)
> +				 struct fgraph_ops *gops,
> +				 struct ftrace_regs *fregs)
>  {
>  	struct trace_array *tr = irqsoff_trace;
>  	struct trace_array_cpu *data;
> diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
> index 23360a2700de..9ffbd9326898 100644
> --- a/kernel/trace/trace_sched_wakeup.c
> +++ b/kernel/trace/trace_sched_wakeup.c
> @@ -144,7 +144,8 @@ static int wakeup_graph_entry(struct ftrace_graph_ent *trace,
>  }
>  
>  static void wakeup_graph_return(struct ftrace_graph_ret *trace,
> -				struct fgraph_ops *gops)
> +				struct fgraph_ops *gops,
> +				struct ftrace_regs *fregs)
>  {
>  	struct trace_array *tr = wakeup_trace;
>  	struct trace_array_cpu *data;
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index 89067f02094a..1ebd0899238f 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -807,7 +807,8 @@ static __init int store_entry(struct ftrace_graph_ent *trace,
>  }
>  
>  static __init void store_return(struct ftrace_graph_ret *trace,
> -				struct fgraph_ops *gops)
> +				struct fgraph_ops *gops,
> +				struct ftrace_regs *fregs)
>  {
>  	struct fgraph_fixture *fixture = container_of(gops, struct fgraph_fixture, gops);
>  	const char *type = fixture->store_type_name;

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Steven Rostedt]

Can I get an Acked-by from the AARCH64 maintainers for this patch?

Thanks!

-- Steve


On Fri, 13 Sep 2024 00:08:51 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Use ftrace_regs instead of fgraph_ret_regs for tracing return value
> on function_graph tracer because of simplifying the callback interface.
> 
> The CONFIG_HAVE_FUNCTION_GRAPH_RETVAL is also replaced by
> CONFIG_HAVE_FUNCTION_GRAPH_FREGS.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v8:
>   - Newly added.
> ---
>  arch/arm64/Kconfig                  |    1 +
>  arch/arm64/include/asm/ftrace.h     |   23 ++++++-----------------
>  arch/arm64/kernel/asm-offsets.c     |   12 ------------
>  arch/arm64/kernel/entry-ftrace.S    |   32 ++++++++++++++++++--------------
>  arch/loongarch/Kconfig              |    2 +-
>  arch/loongarch/include/asm/ftrace.h |   24 ++----------------------
>  arch/loongarch/kernel/asm-offsets.c |   12 ------------
>  arch/loongarch/kernel/mcount.S      |   17 ++++++++++-------
>  arch/loongarch/kernel/mcount_dyn.S  |   14 +++++++-------
>  arch/riscv/Kconfig                  |    2 +-
>  arch/riscv/include/asm/ftrace.h     |   26 +++++---------------------
>  arch/riscv/kernel/mcount.S          |   24 +++++++++++++-----------
>  arch/s390/Kconfig                   |    2 +-
>  arch/s390/include/asm/ftrace.h      |   26 +++++++++-----------------
>  arch/s390/kernel/asm-offsets.c      |    6 ------
>  arch/s390/kernel/mcount.S           |    9 +++++----
>  arch/x86/Kconfig                    |    2 +-
>  arch/x86/include/asm/ftrace.h       |   22 ++--------------------
>  arch/x86/kernel/ftrace_32.S         |   15 +++++++++------
>  arch/x86/kernel/ftrace_64.S         |   17 +++++++++--------
>  include/linux/ftrace.h              |   14 +++++++++++---
>  kernel/trace/Kconfig                |    4 ++--
>  kernel/trace/fgraph.c               |   21 +++++++++------------
>  23 files changed, 122 insertions(+), 205 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a2f8ff354ca6..17947f625b06 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -211,6 +211,7 @@ config ARM64
>  	select HAVE_FTRACE_MCOUNT_RECORD
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_FUNCTION_ERROR_INJECTION
> +	select HAVE_FUNCTION_GRAPH_FREGS
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_GRAPH_RETVAL
>  	select HAVE_GCC_PLUGINS
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index dc9cf0bd2a4c..dffaab3dd1f1 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -126,6 +126,12 @@ ftrace_override_function_with_return(struct ftrace_regs *fregs)
>  	fregs->pc = fregs->lr;
>  }
>  
> +static __always_inline unsigned long
> +ftrace_regs_get_frame_pointer(const struct ftrace_regs *fregs)
> +{
> +	return fregs->fp;
> +}
> +
>  int ftrace_regs_query_register_offset(const char *name);
>  
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> @@ -183,23 +189,6 @@ static inline bool arch_syscall_match_sym_name(const char *sym,
>  
>  #ifndef __ASSEMBLY__
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -struct fgraph_ret_regs {
> -	/* x0 - x7 */
> -	unsigned long regs[8];
> -
> -	unsigned long fp;
> -	unsigned long __unused;
> -};
> -
> -static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->regs[0];
> -}
> -
> -static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->fp;
> -}
>  
>  void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  			   unsigned long frame_pointer);
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 27de1dddb0ab..9e03c9a7e5c3 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -201,18 +201,6 @@ int main(void)
>    DEFINE(FTRACE_OPS_FUNC,		offsetof(struct ftrace_ops, func));
>  #endif
>    BLANK();
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -  DEFINE(FGRET_REGS_X0,			offsetof(struct fgraph_ret_regs, regs[0]));
> -  DEFINE(FGRET_REGS_X1,			offsetof(struct fgraph_ret_regs, regs[1]));
> -  DEFINE(FGRET_REGS_X2,			offsetof(struct fgraph_ret_regs, regs[2]));
> -  DEFINE(FGRET_REGS_X3,			offsetof(struct fgraph_ret_regs, regs[3]));
> -  DEFINE(FGRET_REGS_X4,			offsetof(struct fgraph_ret_regs, regs[4]));
> -  DEFINE(FGRET_REGS_X5,			offsetof(struct fgraph_ret_regs, regs[5]));
> -  DEFINE(FGRET_REGS_X6,			offsetof(struct fgraph_ret_regs, regs[6]));
> -  DEFINE(FGRET_REGS_X7,			offsetof(struct fgraph_ret_regs, regs[7]));
> -  DEFINE(FGRET_REGS_FP,			offsetof(struct fgraph_ret_regs, fp));
> -  DEFINE(FGRET_REGS_SIZE,		sizeof(struct fgraph_ret_regs));
> -#endif
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>    DEFINE(FTRACE_OPS_DIRECT_CALL,	offsetof(struct ftrace_ops, direct_call));
>  #endif
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index f0c16640ef21..169ccf600066 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -329,24 +329,28 @@ SYM_FUNC_END(ftrace_stub_graph)
>   * @fp is checked against the value passed by ftrace_graph_caller().
>   */
>  SYM_CODE_START(return_to_handler)
> -	/* save return value regs */
> -	sub sp, sp, #FGRET_REGS_SIZE
> -	stp x0, x1, [sp, #FGRET_REGS_X0]
> -	stp x2, x3, [sp, #FGRET_REGS_X2]
> -	stp x4, x5, [sp, #FGRET_REGS_X4]
> -	stp x6, x7, [sp, #FGRET_REGS_X6]
> -	str x29,    [sp, #FGRET_REGS_FP]	// parent's fp
> +	/* Make room for ftrace_regs */
> +	sub	sp, sp, #FREGS_SIZE
> +
> +	/* Save return value regs */
> +	stp	x0, x1, [sp, #FREGS_X0]
> +	stp	x2, x3, [sp, #FREGS_X2]
> +	stp	x4, x5, [sp, #FREGS_X4]
> +	stp	x6, x7, [sp, #FREGS_X6]
> +
> +	/* Save the callsite's FP */
> +	str	x29, [sp, #FREGS_FP]
>  
>  	mov	x0, sp
> -	bl	ftrace_return_to_handler	// addr = ftrace_return_to_hander(regs);
> +	bl	ftrace_return_to_handler	// addr = ftrace_return_to_hander(fregs);
>  	mov	x30, x0				// restore the original return address
>  
> -	/* restore return value regs */
> -	ldp x0, x1, [sp, #FGRET_REGS_X0]
> -	ldp x2, x3, [sp, #FGRET_REGS_X2]
> -	ldp x4, x5, [sp, #FGRET_REGS_X4]
> -	ldp x6, x7, [sp, #FGRET_REGS_X6]
> -	add sp, sp, #FGRET_REGS_SIZE
> +	/* Restore return value regs */
> +	ldp	x0, x1, [sp, #FREGS_X0]
> +	ldp	x2, x3, [sp, #FREGS_X2]
> +	ldp	x4, x5, [sp, #FREGS_X4]
> +	ldp	x6, x7, [sp, #FREGS_X6]
> +	add	sp, sp, #FREGS_SIZE
>  
>  	ret
>  SYM_CODE_END(return_to_handler)
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index 70f169210b52..974f08f65f63 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -131,7 +131,7 @@ config LOONGARCH
>  	select HAVE_FTRACE_MCOUNT_RECORD
>  	select HAVE_FUNCTION_ARG_ACCESS_API
>  	select HAVE_FUNCTION_ERROR_INJECTION
> -	select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> +	select HAVE_FUNCTION_GRAPH_FREGS
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_GCC_PLUGINS
> diff --git a/arch/loongarch/include/asm/ftrace.h b/arch/loongarch/include/asm/ftrace.h
> index 6f8517d59954..1a73f35ea9af 100644
> --- a/arch/loongarch/include/asm/ftrace.h
> +++ b/arch/loongarch/include/asm/ftrace.h
> @@ -77,6 +77,8 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs, unsigned long ip)
>  	override_function_with_return(&(fregs)->regs)
>  #define ftrace_regs_query_register_offset(name) \
>  	regs_query_register_offset(name)
> +#define ftrace_regs_get_frame_pointer(fregs) \
> +	((fregs)->regs.regs[22])
>  
>  #define ftrace_graph_func ftrace_graph_func
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> @@ -99,26 +101,4 @@ __arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
>  
>  #endif /* CONFIG_FUNCTION_TRACER */
>  
> -#ifndef __ASSEMBLY__
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -struct fgraph_ret_regs {
> -	/* a0 - a1 */
> -	unsigned long regs[2];
> -
> -	unsigned long fp;
> -	unsigned long __unused;
> -};
> -
> -static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->regs[0];
> -}
> -
> -static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->fp;
> -}
> -#endif /* ifdef CONFIG_FUNCTION_GRAPH_TRACER */
> -#endif
> -
>  #endif /* _ASM_LOONGARCH_FTRACE_H */
> diff --git a/arch/loongarch/kernel/asm-offsets.c b/arch/loongarch/kernel/asm-offsets.c
> index bee9f7a3108f..714f5b5f1956 100644
> --- a/arch/loongarch/kernel/asm-offsets.c
> +++ b/arch/loongarch/kernel/asm-offsets.c
> @@ -279,18 +279,6 @@ static void __used output_pbe_defines(void)
>  }
>  #endif
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -static void __used output_fgraph_ret_regs_defines(void)
> -{
> -	COMMENT("LoongArch fgraph_ret_regs offsets.");
> -	OFFSET(FGRET_REGS_A0, fgraph_ret_regs, regs[0]);
> -	OFFSET(FGRET_REGS_A1, fgraph_ret_regs, regs[1]);
> -	OFFSET(FGRET_REGS_FP, fgraph_ret_regs, fp);
> -	DEFINE(FGRET_REGS_SIZE, sizeof(struct fgraph_ret_regs));
> -	BLANK();
> -}
> -#endif
> -
>  static void __used output_kvm_defines(void)
>  {
>  	COMMENT("KVM/LoongArch Specific offsets.");
> diff --git a/arch/loongarch/kernel/mcount.S b/arch/loongarch/kernel/mcount.S
> index 3015896016a0..b6850503e061 100644
> --- a/arch/loongarch/kernel/mcount.S
> +++ b/arch/loongarch/kernel/mcount.S
> @@ -79,10 +79,11 @@ SYM_FUNC_START(ftrace_graph_caller)
>  SYM_FUNC_END(ftrace_graph_caller)
>  
>  SYM_FUNC_START(return_to_handler)
> -	PTR_ADDI	sp, sp, -FGRET_REGS_SIZE
> -	PTR_S		a0, sp, FGRET_REGS_A0
> -	PTR_S		a1, sp, FGRET_REGS_A1
> -	PTR_S		zero, sp, FGRET_REGS_FP
> +	/* Save return value regs */
> +	PTR_ADDI	sp, sp, -PT_SIZE
> +	PTR_S		a0, sp, PT_R4
> +	PTR_S		a1, sp, PT_R5
> +	PTR_S		zero, sp, PT_R22
>  
>  	move		a0, sp
>  	bl		ftrace_return_to_handler
> @@ -90,9 +91,11 @@ SYM_FUNC_START(return_to_handler)
>  	/* Restore the real parent address: a0 -> ra */
>  	move		ra, a0
>  
> -	PTR_L		a0, sp, FGRET_REGS_A0
> -	PTR_L		a1, sp, FGRET_REGS_A1
> -	PTR_ADDI	sp, sp, FGRET_REGS_SIZE
> +	/* Restore return value regs */
> +	PTR_L		a0, sp, PT_R4
> +	PTR_L		a1, sp, PT_R5
> +	PTR_ADDI	sp, sp, PT_SIZE
> +
>  	jr		ra
>  SYM_FUNC_END(return_to_handler)
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> diff --git a/arch/loongarch/kernel/mcount_dyn.S b/arch/loongarch/kernel/mcount_dyn.S
> index 0c65cf09110c..d6b474ad1d5e 100644
> --- a/arch/loongarch/kernel/mcount_dyn.S
> +++ b/arch/loongarch/kernel/mcount_dyn.S
> @@ -140,19 +140,19 @@ SYM_CODE_END(ftrace_graph_caller)
>  SYM_CODE_START(return_to_handler)
>  	UNWIND_HINT_UNDEFINED
>  	/* Save return value regs */
> -	PTR_ADDI	sp, sp, -FGRET_REGS_SIZE
> -	PTR_S		a0, sp, FGRET_REGS_A0
> -	PTR_S		a1, sp, FGRET_REGS_A1
> -	PTR_S		zero, sp, FGRET_REGS_FP
> +	PTR_ADDI	sp, sp, -PT_SIZE
> +	PTR_S		a0, sp, PT_R4
> +	PTR_S		a1, sp, PT_R5
> +	PTR_S		zero, sp, PT_R22
>  
>  	move		a0, sp
>  	bl		ftrace_return_to_handler
>  	move		ra, a0
>  
>  	/* Restore return value regs */
> -	PTR_L		a0, sp, FGRET_REGS_A0
> -	PTR_L		a1, sp, FGRET_REGS_A1
> -	PTR_ADDI	sp, sp, FGRET_REGS_SIZE
> +	PTR_L		a0, sp, PT_R4
> +	PTR_L		a1, sp, PT_R5
> +	PTR_ADDI	sp, sp, PT_SIZE
>  
>  	jr		ra
>  SYM_CODE_END(return_to_handler)
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 0f3cd7c3a436..6e8422269ba4 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -140,7 +140,7 @@ config RISCV
>  	select HAVE_DYNAMIC_FTRACE_WITH_ARGS if HAVE_DYNAMIC_FTRACE
>  	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
>  	select HAVE_FUNCTION_GRAPH_TRACER
> -	select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> +	select HAVE_FUNCTION_GRAPH_FREGS
>  	select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
>  	select HAVE_EBPF_JIT if MMU
>  	select HAVE_GUP_FAST if MMU
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 2cddd79ff21b..e9f364ce9fe8 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -164,6 +164,11 @@ static __always_inline unsigned long ftrace_regs_get_stack_pointer(const struct
>  	return fregs->sp;
>  }
>  
> +static __always_inline unsigned long ftrace_regs_get_frame_pointer(const struct ftrace_regs *fregs)
> +{
> +	return fregs->s0;
> +}
> +
>  static __always_inline unsigned long ftrace_regs_get_argument(struct ftrace_regs *fregs,
>  							      unsigned int n)
>  {
> @@ -204,25 +209,4 @@ static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs, unsi
>  
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
> -#ifndef __ASSEMBLY__
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -struct fgraph_ret_regs {
> -	unsigned long a1;
> -	unsigned long a0;
> -	unsigned long s0;
> -	unsigned long ra;
> -};
> -
> -static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->a0;
> -}
> -
> -static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->s0;
> -}
> -#endif /* ifdef CONFIG_FUNCTION_GRAPH_TRACER */
> -#endif
> -
>  #endif /* _ASM_RISCV_FTRACE_H */
> diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> index 3a42f6287909..068168046e0e 100644
> --- a/arch/riscv/kernel/mcount.S
> +++ b/arch/riscv/kernel/mcount.S
> @@ -12,6 +12,8 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/ftrace.h>
>  
> +#define ABI_SIZE_ON_STACK	80
> +
>  	.text
>  
>  	.macro SAVE_ABI_STATE
> @@ -26,12 +28,12 @@
>  	 * register if a0 was not saved.
>  	 */
>  	.macro SAVE_RET_ABI_STATE
> -	addi	sp, sp, -4*SZREG
> -	REG_S	s0, 2*SZREG(sp)
> -	REG_S	ra, 3*SZREG(sp)
> -	REG_S	a0, 1*SZREG(sp)
> -	REG_S	a1, 0*SZREG(sp)
> -	addi	s0, sp, 4*SZREG
> +	addi	sp, sp, -ABI_SIZE_ON_STACK
> +	REG_S	ra, 1*SZREG(sp)
> +	REG_S	s0, 8*SZREG(sp)
> +	REG_S	a0, 10*SZREG(sp)
> +	REG_S	a1, 11*SZREG(sp)
> +	addi	s0, sp, ABI_SIZE_ON_STACK
>  	.endm
>  
>  	.macro RESTORE_ABI_STATE
> @@ -41,11 +43,11 @@
>  	.endm
>  
>  	.macro RESTORE_RET_ABI_STATE
> -	REG_L	ra, 3*SZREG(sp)
> -	REG_L	s0, 2*SZREG(sp)
> -	REG_L	a0, 1*SZREG(sp)
> -	REG_L	a1, 0*SZREG(sp)
> -	addi	sp, sp, 4*SZREG
> +	REG_L	ra, 1*SZREG(sp)
> +	REG_L	s0, 8*SZREG(sp)
> +	REG_L	a0, 10*SZREG(sp)
> +	REG_L	a1, 11*SZREG(sp)
> +	addi	sp, sp, ABI_SIZE_ON_STACK
>  	.endm
>  
>  SYM_TYPED_FUNC_START(ftrace_stub)
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index a822f952f64a..12e942cfbcde 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -184,7 +184,7 @@ config S390
>  	select HAVE_FTRACE_MCOUNT_RECORD
>  	select HAVE_FUNCTION_ARG_ACCESS_API
>  	select HAVE_FUNCTION_ERROR_INJECTION
> -	select HAVE_FUNCTION_GRAPH_RETVAL
> +	select HAVE_FUNCTION_GRAPH_FREGS
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_GCC_PLUGINS
> diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> index de76c21eb4a3..9cdd48a46bf7 100644
> --- a/arch/s390/include/asm/ftrace.h
> +++ b/arch/s390/include/asm/ftrace.h
> @@ -49,23 +49,6 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
>  	return NULL;
>  }
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -struct fgraph_ret_regs {
> -	unsigned long gpr2;
> -	unsigned long fp;
> -};
> -
> -static __always_inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->gpr2;
> -}
> -
> -static __always_inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->fp;
> -}
> -#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> -
>  static __always_inline unsigned long
>  ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
>  {
> @@ -92,6 +75,15 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>  #define ftrace_regs_query_register_offset(name) \
>  	regs_query_register_offset(name)
>  
> +static __always_inline unsigned long
> +ftrace_regs_get_frame_pointer(struct ftrace_regs *fregs)
> +{
> +	unsigned long *sp;
> +
> +	sp = (void *)ftrace_regs_get_stack_pointer(fregs);
> +	return sp[0];	/* return backchain */
> +}
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>  /*
>   * When an ftrace registered caller is tracing a function that is
> diff --git a/arch/s390/kernel/asm-offsets.c b/arch/s390/kernel/asm-offsets.c
> index ffa0dd2dbaac..d38ed80615d5 100644
> --- a/arch/s390/kernel/asm-offsets.c
> +++ b/arch/s390/kernel/asm-offsets.c
> @@ -179,12 +179,6 @@ int main(void)
>  	DEFINE(OLDMEM_SIZE, PARMAREA + offsetof(struct parmarea, oldmem_size));
>  	DEFINE(COMMAND_LINE, PARMAREA + offsetof(struct parmarea, command_line));
>  	DEFINE(MAX_COMMAND_LINE_SIZE, PARMAREA + offsetof(struct parmarea, max_command_line_size));
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	/* function graph return value tracing */
> -	OFFSET(__FGRAPH_RET_GPR2, fgraph_ret_regs, gpr2);
> -	OFFSET(__FGRAPH_RET_FP, fgraph_ret_regs, fp);
> -	DEFINE(__FGRAPH_RET_SIZE, sizeof(struct fgraph_ret_regs));
> -#endif
>  	OFFSET(__FTRACE_REGS_PT_REGS, ftrace_regs, regs);
>  	DEFINE(__FTRACE_REGS_SIZE, sizeof(struct ftrace_regs));
>  
> diff --git a/arch/s390/kernel/mcount.S b/arch/s390/kernel/mcount.S
> index ae4d4fd9afcd..cda798b976de 100644
> --- a/arch/s390/kernel/mcount.S
> +++ b/arch/s390/kernel/mcount.S
> @@ -133,14 +133,15 @@ SYM_CODE_END(ftrace_common)
>  SYM_FUNC_START(return_to_handler)
>  	stmg	%r2,%r5,32(%r15)
>  	lgr	%r1,%r15
> -	aghi	%r15,-(STACK_FRAME_OVERHEAD+__FGRAPH_RET_SIZE)
> +# Allocate ftrace_regs + backchain on the stack
> +	aghi	%r15,-STACK_FRAME_SIZE_FREGS
>  	stg	%r1,__SF_BACKCHAIN(%r15)
>  	la	%r3,STACK_FRAME_OVERHEAD(%r15)
> -	stg	%r1,__FGRAPH_RET_FP(%r3)
> -	stg	%r2,__FGRAPH_RET_GPR2(%r3)
> +	stg	%r2,(__SF_GPRS+2*8)(%r15)
> +	stg	%r15,(__SF_GPRS+15*8)(%r15)
>  	lgr	%r2,%r3
>  	brasl	%r14,ftrace_return_to_handler
> -	aghi	%r15,STACK_FRAME_OVERHEAD+__FGRAPH_RET_SIZE
> +	aghi	%r15,STACK_FRAME_SIZE_FREGS
>  	lgr	%r14,%r2
>  	lmg	%r2,%r5,32(%r15)
>  	BR_EX	%r14
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 007bab9f2a0e..047384e4d93a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -228,7 +228,7 @@ config X86
>  	select HAVE_GUP_FAST
>  	select HAVE_FENTRY			if X86_64 || DYNAMIC_FTRACE
>  	select HAVE_FTRACE_MCOUNT_RECORD
> -	select HAVE_FUNCTION_GRAPH_RETVAL	if HAVE_FUNCTION_GRAPH_TRACER
> +	select HAVE_FUNCTION_GRAPH_FREGS	if HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_GRAPH_TRACER	if X86_32 || (X86_64 && DYNAMIC_FTRACE)
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_GCC_PLUGINS
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index 78f6a200e15b..669771ef3b5b 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -64,6 +64,8 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
>  	override_function_with_return(&(fregs)->regs)
>  #define ftrace_regs_query_register_offset(name) \
>  	regs_query_register_offset(name)
> +#define ftrace_regs_get_frame_pointer(fregs) \
> +	frame_pointer(&(fregs)->regs)
>  
>  struct ftrace_ops;
>  #define ftrace_graph_func ftrace_graph_func
> @@ -148,24 +150,4 @@ static inline bool arch_trace_is_compat_syscall(struct pt_regs *regs)
>  #endif /* !COMPILE_OFFSETS */
>  #endif /* !__ASSEMBLY__ */
>  
> -#ifndef __ASSEMBLY__
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -struct fgraph_ret_regs {
> -	unsigned long ax;
> -	unsigned long dx;
> -	unsigned long bp;
> -};
> -
> -static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->ax;
> -}
> -
> -static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> -{
> -	return ret_regs->bp;
> -}
> -#endif /* ifdef CONFIG_FUNCTION_GRAPH_TRACER */
> -#endif
> -
>  #endif /* _ASM_X86_FTRACE_H */
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index 58d9ed50fe61..4b265884d06c 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -23,6 +23,8 @@ SYM_FUNC_START(__fentry__)
>  SYM_FUNC_END(__fentry__)
>  EXPORT_SYMBOL(__fentry__)
>  
> +#define FRAME_SIZE	PT_OLDSS+4
> +
>  SYM_CODE_START(ftrace_caller)
>  
>  #ifdef CONFIG_FRAME_POINTER
> @@ -187,14 +189,15 @@ SYM_CODE_END(ftrace_graph_caller)
>  
>  .globl return_to_handler
>  return_to_handler:
> -	pushl	$0
> -	pushl	%edx
> -	pushl	%eax
> +	subl	$(FRAME_SIZE), %esp
> +	movl	$0, PT_EBP(%esp)
> +	movl	%edx, PT_EDX(%esp)
> +	movl	%eax, PT_EAX(%esp)
>  	movl	%esp, %eax
>  	call	ftrace_return_to_handler
>  	movl	%eax, %ecx
> -	popl	%eax
> -	popl	%edx
> -	addl	$4, %esp		# skip ebp
> +	movl	%eax, PT_EAX(%esp)
> +	movl	%edx, PT_EDX(%esp)
> +	addl	$(FRAME_SIZE), %esp
>  	JMP_NOSPEC ecx
>  #endif
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index 214f30e9f0c0..d51647228596 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -348,21 +348,22 @@ STACK_FRAME_NON_STANDARD_FP(__fentry__)
>  SYM_CODE_START(return_to_handler)
>  	UNWIND_HINT_UNDEFINED
>  	ANNOTATE_NOENDBR
> -	subq  $24, %rsp
>  
> -	/* Save the return values */
> -	movq %rax, (%rsp)
> -	movq %rdx, 8(%rsp)
> -	movq %rbp, 16(%rsp)
> +	/* Save ftrace_regs for function exit context  */
> +	subq $(FRAME_SIZE), %rsp
> +
> +	movq %rax, RAX(%rsp)
> +	movq %rdx, RDX(%rsp)
> +	movq %rbp, RBP(%rsp)
>  	movq %rsp, %rdi
>  
>  	call ftrace_return_to_handler
>  
>  	movq %rax, %rdi
> -	movq 8(%rsp), %rdx
> -	movq (%rsp), %rax
> +	movq RDX(%rsp), %rdx
> +	movq RAX(%rsp), %rax
>  
> -	addq $24, %rsp
> +	addq $(FRAME_SIZE), %rsp
>  	/*
>  	 * Jump back to the old return address. This cannot be JMP_NOSPEC rdi
>  	 * since IBT would demand that contain ENDBR, which simply isn't so for
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1fe49a28de2d..13987cd63553 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -43,9 +43,8 @@ struct dyn_ftrace;
>  
>  char *arch_ftrace_match_adjust(char *str, const char *search);
>  
> -#ifdef CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
> -struct fgraph_ret_regs;
> -unsigned long ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs);
> +#ifdef CONFIG_HAVE_FUNCTION_GRAPH_FREGS
> +unsigned long ftrace_return_to_handler(struct ftrace_regs *fregs);
>  #else
>  unsigned long ftrace_return_to_handler(unsigned long frame_pointer);
>  #endif
> @@ -134,6 +133,13 @@ extern int ftrace_enabled;
>   * Also, architecture dependent fields can be used for internal process.
>   * (e.g. orig_ax on x86_64)
>   *
> + * Basically, ftrace_regs stores the registers related to the context.
> + * On function entry, registers for function parameters and hooking the
> + * function call are stored, and on function exit, registers for function
> + * return value and frame pointers are stored.
> + *
> + * And also, it dpends on the context that which registers are restored
> + * from the ftrace_regs.
>   * On the function entry, those registers will be restored except for
>   * the stack pointer, so that user can change the function parameters
>   * and instruction pointer (e.g. live patching.)
> @@ -191,6 +197,8 @@ static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
>  	override_function_with_return(ftrace_get_regs(fregs))
>  #define ftrace_regs_query_register_offset(name) \
>  	regs_query_register_offset(name)
> +#define ftrace_regs_get_frame_pointer(fregs) \
> +	frame_pointer(&(fregs)->regs)
>  #endif
>  
>  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 721c3b221048..ab277eff80dc 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -31,7 +31,7 @@ config HAVE_FUNCTION_GRAPH_TRACER
>  	help
>  	  See Documentation/trace/ftrace-design.rst
>  
> -config HAVE_FUNCTION_GRAPH_RETVAL
> +config HAVE_FUNCTION_GRAPH_FREGS
>  	bool
>  
>  config HAVE_DYNAMIC_FTRACE
> @@ -232,7 +232,7 @@ config FUNCTION_GRAPH_TRACER
>  
>  config FUNCTION_GRAPH_RETVAL
>  	bool "Kernel Function Graph Return Value"
> -	depends on HAVE_FUNCTION_GRAPH_RETVAL
> +	depends on HAVE_FUNCTION_GRAPH_FREGS
>  	depends on FUNCTION_GRAPH_TRACER
>  	default n
>  	help
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 0322c5723748..30bebe43607d 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -763,15 +763,12 @@ static struct notifier_block ftrace_suspend_notifier = {
>  	.notifier_call = ftrace_suspend_notifier_call,
>  };
>  
> -/* fgraph_ret_regs is not defined without CONFIG_FUNCTION_GRAPH_RETVAL */
> -struct fgraph_ret_regs;
> -
>  /*
>   * Send the trace to the ring-buffer.
>   * @return the original return address.
>   */
> -static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs,
> -						unsigned long frame_pointer)
> +static inline unsigned long
> +__ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointer)
>  {
>  	struct ftrace_ret_stack *ret_stack;
>  	struct ftrace_graph_ret trace;
> @@ -791,7 +788,7 @@ static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs
>  
>  	trace.rettime = trace_clock_local();
>  #ifdef CONFIG_FUNCTION_GRAPH_RETVAL
> -	trace.retval = fgraph_ret_regs_return_value(ret_regs);
> +	trace.retval = ftrace_regs_get_return_value(fregs);
>  #endif
>  
>  	bitmap = get_bitmap_bits(current, offset);
> @@ -826,14 +823,14 @@ static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs
>  }
>  
>  /*
> - * After all architecures have selected HAVE_FUNCTION_GRAPH_RETVAL, we can
> - * leave only ftrace_return_to_handler(ret_regs).
> + * After all architecures have selected HAVE_FUNCTION_GRAPH_FREGS, we can
> + * leave only ftrace_return_to_handler(fregs).
>   */
> -#ifdef CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
> -unsigned long ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs)
> +#ifdef CONFIG_HAVE_FUNCTION_GRAPH_FREGS
> +unsigned long ftrace_return_to_handler(struct ftrace_regs *fregs)
>  {
> -	return __ftrace_return_to_handler(ret_regs,
> -				fgraph_ret_regs_frame_pointer(ret_regs));
> +	return __ftrace_return_to_handler(fregs,
> +				ftrace_regs_get_frame_pointer(fregs));
>  }
>  #else
>  unsigned long ftrace_return_to_handler(unsigned long frame_pointer)

Re: [PATCH v14 08/19] tracing: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Steven Rostedt]

Can I get an Acked-by from the AARCH64 maintainers for this patch?

Thanks!

-- Steve


On Fri, 13 Sep 2024 00:09:35 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add ftrace_partial_regs() which converts the ftrace_regs to pt_regs.
> This is for the eBPF which needs this to keep the same pt_regs interface
> to access registers.
> Thus when replacing the pt_regs with ftrace_regs in fprobes (which is
> used by kprobe_multi eBPF event), this will be used.
> 
> If the architecture defines its own ftrace_regs, this copies partial
> registers to pt_regs and returns it. If not, ftrace_regs is the same as
> pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Acked-by: Florent Revest <revest@chromium.org>
> ---
>  Changes in v14:
>   - Add riscv change.
>  Changes in v8:
>   - Add the reason why this required in changelog.
>  Changes from previous series: NOTHING, just forward ported.
> ---
>  arch/arm64/include/asm/ftrace.h |   11 +++++++++++
>  arch/riscv/include/asm/ftrace.h |   12 ++++++++++++
>  include/linux/ftrace.h          |   17 +++++++++++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index dffaab3dd1f1..5cd587afab6d 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -132,6 +132,17 @@ ftrace_regs_get_frame_pointer(const struct ftrace_regs *fregs)
>  	return fregs->fp;
>  }
>  
> +static __always_inline struct pt_regs *
> +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> +{
> +	memcpy(regs->regs, fregs->regs, sizeof(u64) * 9);
> +	regs->sp = fregs->sp;
> +	regs->pc = fregs->pc;
> +	regs->regs[29] = fregs->fp;
> +	regs->regs[30] = fregs->lr;
> +	return regs;
> +}
> +
>  int ftrace_regs_query_register_offset(const char *name);
>  
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index e9f364ce9fe8..897779dec402 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -193,6 +193,18 @@ static __always_inline void ftrace_override_function_with_return(struct ftrace_r
>  	fregs->epc = fregs->ra;
>  }
>  
> +static __always_inline struct pt_regs *
> +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> +{
> +	memcpy(&regs->a0, fregs->args, sizeof(u64) * 8);
> +	regs->epc = fregs->epc;
> +	regs->ra = fregs->ra;
> +	regs->sp = fregs->sp;
> +	regs->s0 = fregs->s0;
> +	regs->t1 = fregs->t1;
> +	return regs;
> +}
> +
>  int ftrace_regs_query_register_offset(const char *name);
>  
>  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index a1b2ef492c7f..bd9a26bdf660 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -176,6 +176,23 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
>  	return arch_ftrace_get_regs(fregs);
>  }
>  
> +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || \
> +	defined(CONFIG_HAVE_PT_REGS_TO_FTRACE_REGS_CAST)
> +
> +static __always_inline struct pt_regs *
> +ftrace_partial_regs(struct ftrace_regs *fregs, struct pt_regs *regs)
> +{
> +	/*
> +	 * If CONFIG_HAVE_PT_REGS_TO_FTRACE_REGS_CAST=y, ftrace_regs memory
> +	 * layout is the same as pt_regs. So always returns that address.
> +	 * Since arch_ftrace_get_regs() will check some members and may return
> +	 * NULL, we can not use it.
> +	 */
> +	return &fregs->regs;
> +}
> +
> +#endif /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS || CONFIG_HAVE_PT_REGS_TO_FTRACE_REGS_CAST */
> +
>  /*
>   * 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.

Re: [PATCH v14 03/19] function_graph: Pass ftrace_regs to entryfunc

[Will Deacon]

On Sun, Sep 15, 2024 at 04:46:14AM -0400, Steven Rostedt wrote:
> Can I get an Acked-by from the AARCH64 maintainers for this patch?

Sorry, I wasn't CC'd on the thread, so I missed this.

> On Fri, 13 Sep 2024 00:08:40 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Pass ftrace_regs to the fgraph_ops::entryfunc(). If ftrace_regs is not
> > available, it passes a NULL instead. User callback function can access
> > some registers (including return address) via this ftrace_regs.

Under which circumstances is 'ftrace_regs' NULL?

The arm64 implementation of ftrace_graph_func() is:

> > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> > index a650f5e11fc5..bc647b725e6a 100644
> > --- a/arch/arm64/kernel/ftrace.c
> > +++ b/arch/arm64/kernel/ftrace.c
> > @@ -481,7 +481,25 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> >  {
> > -	prepare_ftrace_return(ip, &fregs->lr, fregs->fp);
> > +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> > +	unsigned long frame_pointer = fregs->fp;

Which dereferences the unchecked pointer here ^^.

Will

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Will Deacon]

On Sun, Sep 15, 2024 at 05:11:44AM -0400, Steven Rostedt wrote:
> On Fri, 13 Sep 2024 00:08:51 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Use ftrace_regs instead of fgraph_ret_regs for tracing return value
> > on function_graph tracer because of simplifying the callback interface.
> > 
> > The CONFIG_HAVE_FUNCTION_GRAPH_RETVAL is also replaced by
> > CONFIG_HAVE_FUNCTION_GRAPH_FREGS.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  Changes in v8:
> >   - Newly added.
> > ---
> >  arch/arm64/Kconfig                  |    1 +
> >  arch/arm64/include/asm/ftrace.h     |   23 ++++++-----------------
> >  arch/arm64/kernel/asm-offsets.c     |   12 ------------
> >  arch/arm64/kernel/entry-ftrace.S    |   32 ++++++++++++++++++--------------

The arm64 part looks good to me, although passing a partially-populated
struct out of asm feels like it's going to cause us hard-to-debug
problems down the line if any of those extra fields get used. How hard
would it be to poison the unpopulated members of 'ftrace_regs'?

Will

Re: [PATCH v14 05/19] function_graph: Pass ftrace_regs to retfunc

[Will Deacon]

On Sun, Sep 15, 2024 at 04:49:20AM -0400, Steven Rostedt wrote:
> On Fri, 13 Sep 2024 00:09:02 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Pass ftrace_regs to the fgraph_ops::retfunc(). If ftrace_regs is not
> > available, it passes a NULL instead. User callback function can access
> > some registers (including return address) via this ftrace_regs.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  Changes in v8:
> >   - Pass ftrace_regs to retfunc, instead of adding retregfunc.
> >  Changes in v6:
> >   - update to use ftrace_regs_get_return_value() because of reordering
> >     patches.
> >  Changes in v3:
> >   - Update for new multiple fgraph.
> >   - Save the return address to instruction pointer in ftrace_regs.
> > ---
> >  include/linux/ftrace.h               |    3 ++-
> >  kernel/trace/fgraph.c                |   16 +++++++++++-----
> >  kernel/trace/ftrace.c                |    3 ++-
> >  kernel/trace/trace.h                 |    3 ++-
> >  kernel/trace/trace_functions_graph.c |    7 ++++---
> >  kernel/trace/trace_irqsoff.c         |    3 ++-
> >  kernel/trace/trace_sched_wakeup.c    |    3 ++-
> >  kernel/trace/trace_selftest.c        |    3 ++-
> >  8 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 13987cd63553..e7c41d9988e1 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -1069,7 +1069,8 @@ struct fgraph_ops;
> >  
> >  /* Type of the callback handlers for tracing function graph*/
> >  typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *,
> > -				       struct fgraph_ops *); /* return */
> > +				       struct fgraph_ops *,
> > +				       struct ftrace_regs *); /* return */
> >  typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *,
> >  				      struct fgraph_ops *,
> >  				      struct ftrace_regs *); /* entry */
> > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > index 30bebe43607d..6a3e2db16aa4 100644
> > --- a/kernel/trace/fgraph.c
> > +++ b/kernel/trace/fgraph.c
> > @@ -297,7 +297,8 @@ static int entry_run(struct ftrace_graph_ent *trace, struct fgraph_ops *ops,
> >  }
> >  
> >  /* ftrace_graph_return set to this to tell some archs to run function graph */
> > -static void return_run(struct ftrace_graph_ret *trace, struct fgraph_ops *ops)
> > +static void return_run(struct ftrace_graph_ret *trace, struct fgraph_ops *ops,
> > +		       struct ftrace_regs *fregs)
> >  {
> >  }
> >  
> > @@ -491,7 +492,8 @@ int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
> >  }
> >  
> >  static void ftrace_graph_ret_stub(struct ftrace_graph_ret *trace,
> > -				  struct fgraph_ops *gops)
> > +				  struct fgraph_ops *gops,
> > +				  struct ftrace_regs *fregs)
> >  {
> >  }
> >  
> > @@ -787,6 +789,9 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> >  	}
> >  
> >  	trace.rettime = trace_clock_local();
> > +	if (fregs)
> > +		ftrace_regs_set_instruction_pointer(fregs, ret);

Where does the instruction pointer get used after this? The arm64
'return_to_handler' function doesn't look at it when we return.

Will

Re: [PATCH v14 08/19] tracing: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Will Deacon]

On Sun, Sep 15, 2024 at 05:22:04AM -0400, Steven Rostedt wrote:
> On Fri, 13 Sep 2024 00:09:35 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Add ftrace_partial_regs() which converts the ftrace_regs to pt_regs.
> > This is for the eBPF which needs this to keep the same pt_regs interface
> > to access registers.
> > Thus when replacing the pt_regs with ftrace_regs in fprobes (which is
> > used by kprobe_multi eBPF event), this will be used.
> > 
> > If the architecture defines its own ftrace_regs, this copies partial
> > registers to pt_regs and returns it. If not, ftrace_regs is the same as
> > pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > Acked-by: Florent Revest <revest@chromium.org>
> > ---
> >  Changes in v14:
> >   - Add riscv change.
> >  Changes in v8:
> >   - Add the reason why this required in changelog.
> >  Changes from previous series: NOTHING, just forward ported.
> > ---
> >  arch/arm64/include/asm/ftrace.h |   11 +++++++++++
> >  arch/riscv/include/asm/ftrace.h |   12 ++++++++++++
> >  include/linux/ftrace.h          |   17 +++++++++++++++++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> > index dffaab3dd1f1..5cd587afab6d 100644
> > --- a/arch/arm64/include/asm/ftrace.h
> > +++ b/arch/arm64/include/asm/ftrace.h
> > @@ -132,6 +132,17 @@ ftrace_regs_get_frame_pointer(const struct ftrace_regs *fregs)
> >  	return fregs->fp;
> >  }
> >  
> > +static __always_inline struct pt_regs *
> > +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> > +{
> > +	memcpy(regs->regs, fregs->regs, sizeof(u64) * 9);
> > +	regs->sp = fregs->sp;
> > +	regs->pc = fregs->pc;
> > +	regs->regs[29] = fregs->fp;
> > +	regs->regs[30] = fregs->lr;
> > +	return regs;
> > +}

Ah, I guess this is where we pick up the lr that was set in patch 5.

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH v14 03/19] function_graph: Pass ftrace_regs to entryfunc

[Steven Rostedt]

On Tue, 17 Sep 2024 09:26:25 +0100
Will Deacon <will@kernel.org> wrote:

> On Sun, Sep 15, 2024 at 04:46:14AM -0400, Steven Rostedt wrote:
> > Can I get an Acked-by from the AARCH64 maintainers for this patch?  
> 
> Sorry, I wasn't CC'd on the thread, so I missed this.

Here's the lore link that starts it all:

  https://lore.kernel.org/all/172615368656.133222.2336770908714920670.stgit@devnote2/

There's also a v15:

   https://lore.kernel.org/linux-trace-kernel/172639136989.366111.11359590127009702129.stgit@devnote2/

> 
> > On Fri, 13 Sep 2024 00:08:40 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> >   
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Pass ftrace_regs to the fgraph_ops::entryfunc(). If ftrace_regs is not
> > > available, it passes a NULL instead. User callback function can access
> > > some registers (including return address) via this ftrace_regs.  
> 
> Under which circumstances is 'ftrace_regs' NULL?

When the arch does not define: HAVE_DYNAMIC_FTRACE_WITH_ARGS or
HAVE_DYNAMIC_FTRACE_WITH_REGS

If HAVE_DYNAMIC_FTRACE_WITH_REGS is defined but not the WITH_ARGS, and the
ops used to register the function callback does not set FTRACE_OPS_FL_SAVE_REGS.

Otherwise it should be set.

> 
> The arm64 implementation of ftrace_graph_func() is:
> 
> > > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> > > index a650f5e11fc5..bc647b725e6a 100644
> > > --- a/arch/arm64/kernel/ftrace.c
> > > +++ b/arch/arm64/kernel/ftrace.c
> > > @@ -481,7 +481,25 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> > >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> > >  {
> > > -	prepare_ftrace_return(ip, &fregs->lr, fregs->fp);
> > > +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> > > +	unsigned long frame_pointer = fregs->fp;  
> 
> Which dereferences the unchecked pointer here ^^.

Just above the visible portion of the patch, we have:

#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)
{

Which means fregs will be available. The "WITH_ARGS" config tells us that
the arch will supply the function with valid fregs to the ftrace callback
(which this is).

-- Steve

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Steven Rostedt]

On Tue, 17 Sep 2024 10:55:39 +0100
Will Deacon <will@kernel.org> wrote:

> The arm64 part looks good to me, although passing a partially-populated
> struct out of asm feels like it's going to cause us hard-to-debug
> problems down the line if any of those extra fields get used. How hard
> would it be to poison the unpopulated members of 'ftrace_regs'?

The purpose of creating ftrace_regs was to allow a partially populated
pt_regs to be sent around, as Thomas Gleixner and Peter Zijlstra were
against using pt_regs that were not fully populated. Hence, I created
"ftrace_regs" for this purpose.

ftrace_regs should never be accessed via its internal elements but only with
its accessor functions, as depending on the arch or functionality used, the
content of the structure should never be trusted. The accessor functions
will do all the verification needed.

I may add some compiler hacks to enforce this. Something like:

struct ftrace_regs {
	void *nothing_to_see_here;
};

And then change the arch code to be something like:

// in arch/arm64/include/asm/ftrace.h:

struct arch_ftrace_regs {
        /* x0 - x8 */
        unsigned long regs[9];

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
        unsigned long direct_tramp;
#else
        unsigned long __unused;
#endif

        unsigned long fp;
        unsigned long lr;

        unsigned long sp;
        unsigned long pc;
};

#define get_arch_ftrace_regs(fregs) ((struct arch_ftrace_regs *)(fregs))

static __always_inline void
ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
                                    unsigned long pc)
{
	struct arch_ftrace_regs *afregs = get_ftrace_regs(fregs);
        afregs->pc = pc;
}


-- Steve

Re: [PATCH v14 05/19] function_graph: Pass ftrace_regs to retfunc

[Steven Rostedt]

On Tue, 17 Sep 2024 11:08:48 +0100
Will Deacon <will@kernel.org> wrote:

> > > @@ -787,6 +789,9 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> > >  	}
> > >  
> > >  	trace.rettime = trace_clock_local();
> > > +	if (fregs)
> > > +		ftrace_regs_set_instruction_pointer(fregs, ret);  
> 
> Where does the instruction pointer get used after this? The arm64
> 'return_to_handler' function doesn't look at it when we return.

It's for the hooks to the return instruction. kretprobes will start using
function graph tracer to hook to a return of a function (via fprobes), and
the callbacks will need access to the return pointer. The callbacks get
passed the ftrace_regs, and this is how they can see what the function is
returning to. For example, BPF programs will need this.

So it's not needed for the infrastructure, only the callbacks that hook to
it.

-- Steve

Re: [PATCH v14 03/19] function_graph: Pass ftrace_regs to entryfunc

[Masami Hiramatsu]

On Mon, 30 Sep 2024 14:46:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 17 Sep 2024 09:26:25 +0100
> Will Deacon <will@kernel.org> wrote:
> 
> > On Sun, Sep 15, 2024 at 04:46:14AM -0400, Steven Rostedt wrote:
> > > Can I get an Acked-by from the AARCH64 maintainers for this patch?  
> > 
> > Sorry, I wasn't CC'd on the thread, so I missed this.
> 
> Here's the lore link that starts it all:
> 
>   https://lore.kernel.org/all/172615368656.133222.2336770908714920670.stgit@devnote2/
> 
> There's also a v15:
> 
>    https://lore.kernel.org/linux-trace-kernel/172639136989.366111.11359590127009702129.stgit@devnote2/
> 
> > 
> > > On Fri, 13 Sep 2024 00:08:40 +0900
> > > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > >   
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > 
> > > > Pass ftrace_regs to the fgraph_ops::entryfunc(). If ftrace_regs is not
> > > > available, it passes a NULL instead. User callback function can access
> > > > some registers (including return address) via this ftrace_regs.  
> > 
> > Under which circumstances is 'ftrace_regs' NULL?
> 
> When the arch does not define: HAVE_DYNAMIC_FTRACE_WITH_ARGS or
> HAVE_DYNAMIC_FTRACE_WITH_REGS
> 
> If HAVE_DYNAMIC_FTRACE_WITH_REGS is defined but not the WITH_ARGS, and the
> ops used to register the function callback does not set FTRACE_OPS_FL_SAVE_REGS.
> 
> Otherwise it should be set.

I will add this condition in the description in next version.

Thank you,


> 
> > 
> > The arm64 implementation of ftrace_graph_func() is:
> > 
> > > > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> > > > index a650f5e11fc5..bc647b725e6a 100644
> > > > --- a/arch/arm64/kernel/ftrace.c
> > > > +++ b/arch/arm64/kernel/ftrace.c
> > > > @@ -481,7 +481,25 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> > > >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > > >  		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> > > >  {
> > > > -	prepare_ftrace_return(ip, &fregs->lr, fregs->fp);
> > > > +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> > > > +	unsigned long frame_pointer = fregs->fp;  
> > 
> > Which dereferences the unchecked pointer here ^^.
> 
> Just above the visible portion of the patch, we have:
> 
> #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)
> {
> 
> Which means fregs will be available. The "WITH_ARGS" config tells us that
> the arch will supply the function with valid fregs to the ftrace callback
> (which this is).
> 
> -- Steve
> 


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

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Masami Hiramatsu]

On Mon, 30 Sep 2024 14:55:48 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 17 Sep 2024 10:55:39 +0100
> Will Deacon <will@kernel.org> wrote:
> 
> > The arm64 part looks good to me, although passing a partially-populated
> > struct out of asm feels like it's going to cause us hard-to-debug
> > problems down the line if any of those extra fields get used. How hard
> > would it be to poison the unpopulated members of 'ftrace_regs'?
> 
> The purpose of creating ftrace_regs was to allow a partially populated
> pt_regs to be sent around, as Thomas Gleixner and Peter Zijlstra were
> against using pt_regs that were not fully populated. Hence, I created
> "ftrace_regs" for this purpose.
> 
> ftrace_regs should never be accessed via its internal elements but only with
> its accessor functions, as depending on the arch or functionality used, the
> content of the structure should never be trusted. The accessor functions
> will do all the verification needed.
> 
> I may add some compiler hacks to enforce this. Something like:
> 
> struct ftrace_regs {
> 	void *nothing_to_see_here;
> };

Yeah, OK. But sizeof(fregs) may be changed. (Shouldn't we do too?)

> 
> And then change the arch code to be something like:
> 
> // in arch/arm64/include/asm/ftrace.h:
> 
> struct arch_ftrace_regs {
>         /* x0 - x8 */
>         unsigned long regs[9];
> 
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>         unsigned long direct_tramp;
> #else
>         unsigned long __unused;
> #endif
> 
>         unsigned long fp;
>         unsigned long lr;
> 
>         unsigned long sp;
>         unsigned long pc;
> };

And if it is pt_regs compatible, 

#define arch_ftrace_regs pt_regs

?

> 
> #define get_arch_ftrace_regs(fregs) ((struct arch_ftrace_regs *)(fregs))
> 
> static __always_inline void
> ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
>                                     unsigned long pc)
> {
> 	struct arch_ftrace_regs *afregs = get_ftrace_regs(fregs);
>         afregs->pc = pc;
> }
> 
> 
> -- Steve


Thanks,

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

Re: [PATCH v14 05/19] function_graph: Pass ftrace_regs to retfunc

[Masami Hiramatsu]

On Mon, 30 Sep 2024 15:03:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 17 Sep 2024 11:08:48 +0100
> Will Deacon <will@kernel.org> wrote:
> 
> > > > @@ -787,6 +789,9 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> > > >  	}
> > > >  
> > > >  	trace.rettime = trace_clock_local();
> > > > +	if (fregs)
> > > > +		ftrace_regs_set_instruction_pointer(fregs, ret);  
> > 
> > Where does the instruction pointer get used after this? The arm64
> > 'return_to_handler' function doesn't look at it when we return.
> 
> It's for the hooks to the return instruction. kretprobes will start using

not kretprobes, but fprobe. kretprobes continue using rethook.

> function graph tracer to hook to a return of a function (via fprobes), and
> the callbacks will need access to the return pointer. The callbacks get
> passed the ftrace_regs, and this is how they can see what the function is
> returning to. For example, BPF programs will need this.
> 
> So it's not needed for the infrastructure, only the callbacks that hook to
> it.

Yes, it will be used for showing where to return in the fprobe exit event.
More specifically, in the fprobe_return()@kernel/trace/fprobe.c in PATCH 13/19,
it is extracted from fregs.

+static void fprobe_return(struct ftrace_graph_ret *trace,
+			  struct fgraph_ops *gops,
+			  struct ftrace_regs *fregs)
+{
+	unsigned long *fgraph_data = NULL;
+	unsigned long ret_ip;
+	unsigned long val;
+	struct fprobe *fp;
+	int size, curr;
+	int size_words;
+
+	fgraph_data = (unsigned long *)fgraph_retrieve_data(gops->idx, &size);
+	if (WARN_ON_ONCE(!fgraph_data))
 		return;
+	size_words = SIZE_IN_LONG(size);
+	ret_ip = ftrace_regs_get_instruction_pointer(fregs);
+

Thank you,

> 
> -- Steve


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

Re: [PATCH v14 08/19] tracing: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs

[Masami Hiramatsu]

On Tue, 17 Sep 2024 11:14:54 +0100
Will Deacon <will@kernel.org> wrote:

> On Sun, Sep 15, 2024 at 05:22:04AM -0400, Steven Rostedt wrote:
> > On Fri, 13 Sep 2024 00:09:35 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > 
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Add ftrace_partial_regs() which converts the ftrace_regs to pt_regs.
> > > This is for the eBPF which needs this to keep the same pt_regs interface
> > > to access registers.
> > > Thus when replacing the pt_regs with ftrace_regs in fprobes (which is
> > > used by kprobe_multi eBPF event), this will be used.
> > > 
> > > If the architecture defines its own ftrace_regs, this copies partial
> > > registers to pt_regs and returns it. If not, ftrace_regs is the same as
> > > pt_regs and ftrace_partial_regs() will return ftrace_regs::regs.
> > > 
> > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > Acked-by: Florent Revest <revest@chromium.org>
> > > ---
> > >  Changes in v14:
> > >   - Add riscv change.
> > >  Changes in v8:
> > >   - Add the reason why this required in changelog.
> > >  Changes from previous series: NOTHING, just forward ported.
> > > ---
> > >  arch/arm64/include/asm/ftrace.h |   11 +++++++++++
> > >  arch/riscv/include/asm/ftrace.h |   12 ++++++++++++
> > >  include/linux/ftrace.h          |   17 +++++++++++++++++
> > >  3 files changed, 40 insertions(+)
> > > 
> > > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> > > index dffaab3dd1f1..5cd587afab6d 100644
> > > --- a/arch/arm64/include/asm/ftrace.h
> > > +++ b/arch/arm64/include/asm/ftrace.h
> > > @@ -132,6 +132,17 @@ ftrace_regs_get_frame_pointer(const struct ftrace_regs *fregs)
> > >  	return fregs->fp;
> > >  }
> > >  
> > > +static __always_inline struct pt_regs *
> > > +ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
> > > +{
> > > +	memcpy(regs->regs, fregs->regs, sizeof(u64) * 9);
> > > +	regs->sp = fregs->sp;
> > > +	regs->pc = fregs->pc;
> > > +	regs->regs[29] = fregs->fp;
> > > +	regs->regs[30] = fregs->lr;
> > > +	return regs;
> > > +}
> 
> Ah, I guess this is where we pick up the lr that was set in patch 5.

Yeah, this one also copies the fregs->pc to regs->pc, mainly for bpf.

> 
> Acked-by: Will Deacon <will@kernel.org>

Thank you!

> 
> Will


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

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Steven Rostedt]

On Wed, 2 Oct 2024 08:10:37 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > 
> > I may add some compiler hacks to enforce this. Something like:
> > 
> > struct ftrace_regs {
> > 	void *nothing_to_see_here;
> > };  
> 
> Yeah, OK. But sizeof(fregs) may be changed. (Shouldn't we do too?)

Honestly, I don't think anything should be doing a sizeof(struct ftrace_regs)

Heck, perhaps we should make it totally zero!

  struct ftrace_regs {
	long nothing_here[];
  };

If someone needs to allocate, then we could provide a:

	ftrace_regs_size()

helper function.

> 
> > 
> > And then change the arch code to be something like:
> > 
> > // in arch/arm64/include/asm/ftrace.h:
> > 
> > struct arch_ftrace_regs {
> >         /* x0 - x8 */
> >         unsigned long regs[9];
> > 
> > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >         unsigned long direct_tramp;
> > #else
> >         unsigned long __unused;
> > #endif
> > 
> >         unsigned long fp;
> >         unsigned long lr;
> > 
> >         unsigned long sp;
> >         unsigned long pc;
> > };  
> 
> And if it is pt_regs compatible, 
> 
> #define arch_ftrace_regs pt_regs
> 
> ?
> 

Only if it is fully pt_regs compatible.

-- Steve

Re: [PATCH v14 04/19] function_graph: Replace fgraph_ret_regs with ftrace_regs

[Masami Hiramatsu]

On Tue, 1 Oct 2024 19:32:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 2 Oct 2024 08:10:37 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > 
> > > I may add some compiler hacks to enforce this. Something like:
> > > 
> > > struct ftrace_regs {
> > > 	void *nothing_to_see_here;
> > > };  
> > 
> > Yeah, OK. But sizeof(fregs) may be changed. (Shouldn't we do too?)
> 
> Honestly, I don't think anything should be doing a sizeof(struct ftrace_regs)
> 
> Heck, perhaps we should make it totally zero!
> 
>   struct ftrace_regs {
> 	long nothing_here[];
>   };
> 
> If someone needs to allocate, then we could provide a:
> 
> 	ftrace_regs_size()
> 
> helper function.

Ah, Indeed.

> 
> > 
> > > 
> > > And then change the arch code to be something like:
> > > 
> > > // in arch/arm64/include/asm/ftrace.h:
> > > 
> > > struct arch_ftrace_regs {
> > >         /* x0 - x8 */
> > >         unsigned long regs[9];
> > > 
> > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > >         unsigned long direct_tramp;
> > > #else
> > >         unsigned long __unused;
> > > #endif
> > > 
> > >         unsigned long fp;
> > >         unsigned long lr;
> > > 
> > >         unsigned long sp;
> > >         unsigned long pc;
> > > };  
> > 
> > And if it is pt_regs compatible, 
> > 
> > #define arch_ftrace_regs pt_regs
> > 
> > ?
> > 
> 
> Only if it is fully pt_regs compatible.

Yeah, OK, this is good idea.

Thank you,

> 
> -- Steve


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