Received-SPF: pass (google.com: domain of linux-kernel+bounces-127991-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To: Puranjay Mohan <puranjay12@gmail.com>, 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>, =?utf-8?B?QmrDtnJuIFTDtnBlbA==?=
 <bjorn@rivosinc.com>, =?utf-8?Q?Cl=C3=A9ment_L=C3=A9ger?=
 <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: [RFC PATCH] ftrace: riscv: move from REGS to ARGS
In-Reply-To: <20240328141845.128645-1-puranjay12@gmail.com>
References: <20240328141845.128645-1-puranjay12@gmail.com>
Date: Tue, 02 Apr 2024 15:02:41 +0200
Message-ID: <87bk6rsyry.fsf@all.your.base.are.belong.to.us>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Puranjay Mohan <puranjay12@gmail.com> 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  <=3D=3D=3D=3D=3D=3D=3D=3D=3D<Enable/Disable>=3D=3D=3D=3D=3D=3D=3D=
=3D=3D>  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).
>
> 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.

..and FPROBES, but Masami is (still?) working on a series to address
that [1].

My perspective; This is following the work Mark et al has done for
arm64, and it does make sense for RISC-V as well. I'm in favor having
this change as part of the bigger call_ops/text patch change for RISC-V.

[...]

> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftr=
ace.h
> index 1276d7d9ca8b..1e95bf4ded6c 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -124,20 +124,87 @@ struct dyn_ftrace;
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
>  #define ftrace_init_nop ftrace_init_nop
>=20=20
> -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +#define arch_ftrace_get_regs(regs) NULL
>  struct ftrace_ops;
> -struct ftrace_regs;
> +struct ftrace_regs {
> +	unsigned long epc;
> +	unsigned long ra;
> +	unsigned long sp;
> +	unsigned long s0;
> +	unsigned long t1;
> +	union {
> +		unsigned long args[8];
> +		struct {
> +			unsigned long a0;
> +			unsigned long a1;
> +			unsigned long a2;
> +			unsigned long a3;
> +			unsigned long a4;
> +			unsigned long a5;
> +			unsigned long a6;
> +			unsigned long a7;
> +		};
> +	};
> +};
> +
> +static __always_inline unsigned long
> +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> +{
> +	return fregs->epc;
> +}
> +
> +static __always_inline void
> +ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> +				    unsigned long pc)
> +{
> +	fregs->epc =3D pc;
> +}
> +
> +static __always_inline unsigned long
> +ftrace_regs_get_stack_pointer(const struct ftrace_regs *fregs)
> +{
> +	return fregs->sp;
> +}
> +
> +static __always_inline unsigned long
> +ftrace_regs_get_argument(struct ftrace_regs *fregs, unsigned int n)
> +{
> +	if (n < 8)
> +		return fregs->args[n];
> +	return 0;
> +}
> +
> +static __always_inline unsigned long
> +ftrace_regs_get_return_value(const struct ftrace_regs *fregs)
> +{
> +	return fregs->a0;
> +}
> +
> +static __always_inline void
> +ftrace_regs_set_return_value(struct ftrace_regs *fregs,
> +			     unsigned long ret)
> +{
> +	fregs->a0 =3D ret;
> +}
> +
> +static __always_inline void
> +ftrace_override_function_with_return(struct ftrace_regs *fregs)
> +{
> +	fregs->epc =3D fregs->ra;
> +}

Style/nit: All above; Try to use the full 100 chars, and keep the
function name return value on the same line for grepability.


Bj=C3=B6rn

[1] https://lore.kernel.org/all/170887410337.564249.6360118840946697039.stg=
it@devnote2/