Re: [PATCH] ftrace: riscv: move from REGS to ARGS

From: "Björn Töpel" <bjorn@kernel.org>
To: "Puranjay Mohan" <puranjay@kernel.org>,
	"Alexandre Ghiti" <alexghiti@rivosinc.com>,
	"Andy Chiu" <andy.chiu@sifive.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Masami Hiramatsu" <mhiramat@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Sami Tolvanen" <samitolvanen@google.com>,
	"Guo Ren" <guoren@kernel.org>,
	"Ley Foon Tan" <leyfoon.tan@starfivetech.com>,
	"Deepak Gupta" <debug@rivosinc.com>,
	"Sia Jee Heng" <jeeheng.sia@starfivetech.com>,
	"Song Shuai" <suagrfillet@gmail.com>,
	"Björn Töpel" <bjorn@rivosinc.com>,
	"Clément Léger" <cleger@rivosinc.com>,
	"Al Viro" <viro@zeniv.linux.org.uk>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org
Cc: puranjay12@gmail.com
Subject: Re: [PATCH] ftrace: riscv: move from REGS to ARGS
Date: Wed, 24 Apr 2024 11:13:46 +0200	[thread overview]
Message-ID: <87il0786np.fsf@all.your.base.are.belong.to.us> (raw)
In-Reply-To: <20240405142453.4187-1-puranjay@kernel.org>

Puranjay Mohan <puranjay@kernel.org> writes:

> This commit replaces riscv's support for FTRACE_WITH_REGS with support
> for FTRACE_WITH_ARGS. This is required for the ongoing effort to stop
> relying on stop_machine() for RISCV's implementation of ftrace.
>
> The main relevant benefit that this change will bring for the above
> use-case is that now we don't have separate ftrace_caller and
> ftrace_regs_caller trampolines. This will allow the callsite to call
> ftrace_caller by modifying a single instruction. Now the callsite can
> do something similar to:
>
> When not tracing:            |             When tracing:
>
> func:                                      func:
>   auipc t0, ftrace_caller_top                auipc t0, ftrace_caller_top
>   nop  <=========<Enable/Disable>=========>  jalr  t0, ftrace_caller_bottom
>   [...]                                      [...]
>
> The above assumes that we are dropping the support of calling a direct
> trampoline from the callsite. We need to drop this as the callsite can't
> change the target address to call, it can only enable/disable a call to
> a preset target (ftrace_caller in the above diagram). We can later optimize
> this by calling an intermediate dispatcher trampoline before ftrace_caller.
>
> Currently, ftrace_regs_caller saves all CPU registers in the format of
> struct pt_regs and allows the tracer to modify them. We don't need to
> save all of the CPU registers because at function entry only a subset of
> pt_regs is live:
>
> |----------+----------+---------------------------------------------|
> | Register | ABI Name | Description                                 |
> |----------+----------+---------------------------------------------|
> | x1       | ra       | Return address for traced function          |
> | x2       | sp       | Stack pointer                               |
> | x5       | t0       | Return address for ftrace_caller trampoline |
> | x8       | s0/fp    | Frame pointer                               |
> | x10-11   | a0-1     | Function arguments/return values            |
> | x12-17   | a2-7     | Function arguments                          |
> |----------+----------+---------------------------------------------|
>
> See RISCV calling convention[1] for the above table.
>
> Saving just the live registers decreases the amount of stack space
> required from 288 Bytes to 112 Bytes.
>
> Basic testing was done with this on the VisionFive 2 development board.
>
> Note:
>   - Moving from REGS to ARGS will mean that RISCV will stop supporting
>     KPROBES_ON_FTRACE as it requires full pt_regs to be saved.
>   - KPROBES_ON_FTRACE will be supplanted by FPROBES see [2].

Puranjay,

Apologies for the slow response.

Restating from the RFC; This change is a prerequisite for moving towards
a "FTRACE_WITH_CALL_OPS" model on riscv, and finally getting rid of the
stop_machine() text patching.

I've tested this on QEMU, and on the VisionFive2. No regressions
(FTRACE_STARTUP_*, and kselftest), other than that KPROBES_ON_FTRACE no
longer works. (Which will be addressed by [2]).

Palmer, my preference is that this should go on for-next, rather than
being part of the upcoming text patching support (worked on by Andy),
but really up to you.

Tested-by: Björn Töpel <bjorn@rivosinc.com>
Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv