Received-SPF: pass (google.com: domain of linux-kernel+bounces-157081-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Apr 2024 23:31:56 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Florent Revest <revest@chromium.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Steven Rostedt
 <rostedt@goodmis.org>, linux-trace-kernel@vger.kernel.org, LKML
 <linux-kernel@vger.kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,
 bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>, Alexei
 Starovoitov <ast@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Arnaldo
 Carvalho de Melo <acme@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Alan Maguire <alan.maguire@oracle.com>, Mark Rutland
 <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, Thomas
 Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v9 01/36] tracing: Add a comment about ftrace_regs
 definition
Message-Id: <20240424233156.67b39d2a3485069fed156308@kernel.org>
In-Reply-To: <CABRcYmK+JMLDf41csuZB4aJSb9956wTy=5rpB1YDsSv0-MoZHg@mail.gmail.com>
References: <171318533841.254850.15841395205784342850.stgit@devnote2>
	<171318535003.254850.2125783941049872788.stgit@devnote2>
	<CABRcYmK_Btem8cBbz=j==RWxw11PQ8cNAUshNA540VD3O=2WEQ@mail.gmail.com>
	<CABRcYmK+JMLDf41csuZB4aJSb9956wTy=5rpB1YDsSv0-MoZHg@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On Wed, 24 Apr 2024 15:19:24 +0200
Florent Revest <revest@chromium.org> wrote:

> On Wed, Apr 24, 2024 at 2:23 PM Florent Revest <revest@chromium.org> wrote:
> >
> > On Mon, Apr 15, 2024 at 2:49 PM Masami Hiramatsu (Google)
> > <mhiramat@kernel.org> wrote:
> > >
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > >
> > > To clarify what will be expected on ftrace_regs, add a comment to the
> > > architecture independent definition of the ftrace_regs.
> > >
> > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > > ---
> > >  Changes in v8:
> > >   - Update that the saved registers depends on the context.
> > >  Changes in v3:
> > >   - Add instruction pointer
> > >  Changes in v2:
> > >   - newly added.
> > > ---
> > >  include/linux/ftrace.h |   26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > >
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 54d53f345d14..b81f1afa82a1 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -118,6 +118,32 @@ extern int ftrace_enabled;
> > >
> > >  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > >
> > > +/**
> > > + * ftrace_regs - ftrace partial/optimal register set
> > > + *
> > > + * ftrace_regs represents a group of registers which is used at the
> > > + * function entry and exit. There are three types of registers.
> > > + *
> > > + * - Registers for passing the parameters to callee, including the stack
> > > + *   pointer. (e.g. rcx, rdx, rdi, rsi, r8, r9 and rsp on x86_64)
> > > + * - Registers for passing the return values to caller.
> > > + *   (e.g. rax and rdx on x86_64)
> >
> > Ooc, have we ever considered skipping argument registers that are not
> > return value registers in the exit code paths ? For example, why would
> > we want to save rdi in a return handler ?
> >
> > But if we want to avoid the situation of having "sparse ftrace_regs"
> > all over again, we'd have to split ftrace_regs into a ftrace_args_regs
> > and a ftrace_ret_regs which would make this refactoring even more
> > painful, just to skip a few instructions. :|
> >
> > I don't necessarily think it's worth it, I just wanted to make sure
> > this was considered.
> 
> Ah, well, I just reached patch 22 and noticed that there you add add:
> 
> + * 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.
> 
> So ftrace_regs can be a a sparse structure then. That's fair enough with me! ;)

Yes, and in this patch, I explained that too :)

> + * 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.)
> + * On the function exit, only registers which is used for return values
> > + * are restored.

So the function exit, ftrace_regs will be sparse.

Thank you,

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