Re: [RFC PATCH 24/32] x86/ftrace: Enable HAVE_FUNCTION_GRAPH_FREGS

[Steven Rostedt]

On Sun, 5 Nov 2023 18:25:36 +0100
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Mon, Nov 06, 2023 at 01:11:21AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > Support HAVE_FUNCTION_GRAPH_FREGS on x86-64, which saves ftrace_regs
> > on the stack in ftrace_graph return trampoline so that the callbacks
> > can access registers via ftrace_regs APIs.  
> 
> What is ftrace_regs ? If I look at arch/x86/include/asm/ftrace.h it's a
> pointless wrapper around pt_regs.
> 
> Can we please remove the pointless wrappery and call it what it is?

A while back ago when I introduced FTRACE_WITH_ARGS, it would have all
ftrace callbacks get a pt_regs, but it would be partially filled for
those that did not specify the "REGS" flag when registering the
callback. You and Thomas complained that it would be a bug to return
pt_regs that was not full because something might read the non filled
registers and think they were valid.

To solve this, I came up with ftrace_regs to only hold the registers
that were required for function parameters (including the stack
pointer). You could then call arch_ftrace_get_regs(ftrace_regs) and if
this "wrapper" had all valid pt_regs registers, then it would return
the pt_regs, otherwise it would return NULL, and you would need to use
the ftrace_regs accessor calls to get the function registers. You and
Thomas agreed with this.

You even Acked the patch:

commit 02a474ca266a47ea8f4d5a11f4ffa120f83730ad
Author: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
Date:   Tue Oct 27 10:55:55 2020 -0400

    ftrace/x86: Allow for arguments to be passed in to ftrace_regs by default
    
    Currently, the only way to get access to the registers of a function via a
    ftrace callback is to set the "FL_SAVE_REGS" bit in the ftrace_ops. But as this
    saves all regs as if a breakpoint were to trigger (for use with kprobes), it
    is expensive.
    
    The regs are already saved on the stack for the default ftrace callbacks, as
    that is required otherwise a function being traced will get the wrong
    arguments and possibly crash. And on x86, the arguments are already stored
    where they would be on a pt_regs structure to use that code for both the
    regs version of a callback, it makes sense to pass that information always
    to all functions.
    
    If an architecture does this (as x86_64 now does), it is to set
    HAVE_DYNAMIC_FTRACE_WITH_ARGS, and this will let the generic code that it
    could have access to arguments without having to set the flags.
    
    This also includes having the stack pointer being saved, which could be used
    for accessing arguments on the stack, as well as having the function graph
    tracer not require its own trampoline!
    
    Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
    Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>


-- Steve