On Mon, May 5, 2025 at 10:06 PM Alexandre Ghiti <[email protected]> wrote: > > On 23/04/2025 10:22, Alexandre Ghiti wrote: > > Hi Andy, > > > > On 07/04/2025 20:08, Andy Chiu wrote: > >> From: Andy Chiu <[email protected]> > >> > >> We use an AUIPC+JALR pair to jump into a ftrace trampoline. Since > >> instruction fetch can break down to 4 byte at a time, it is impossible > >> to update two instructions without a race. In order to mitigate it, we > >> initialize the patchable entry to AUIPC + NOP4. Then, the run-time code > >> patching can change NOP4 to JALR to eable/disable ftrcae from a > >> function. This limits the reach of each ftrace entry to +-2KB displacing > >> from ftrace_caller. > >> > >> Starting from the trampoline, we add a level of indirection for it to > >> reach ftrace caller target. Now, it loads the target address from a > >> memory location, then perform the jump. This enable the kernel to update > >> the target atomically. > >> > >> The new don't-stop-the-world text patching on change only one RISC-V > >> instruction: > >> > >> | -8: &ftrace_ops of the associated tracer function. > >> | <ftrace enable>: > >> | 0: auipc t0, hi(ftrace_caller) > >> | 4: jalr t0, lo(ftrace_caller) > >> | > >> | -8: &ftrace_nop_ops > >> | <ftrace disable>: > >> | 0: auipc t0, hi(ftrace_caller) > >> | 4: nop > >> > >> This means that f+0x0 is fixed, and should not be claimed by ftrace, > >> e.g. kprobe should be able to put a probe in f+0x0. Thus, we adjust the > >> offset and MCOUNT_INSN_SIZE accordingly. > >> > >> Co-developed-by: Björn Töpel <[email protected]> > >> Signed-off-by: Björn Töpel <[email protected]> > >> Signed-off-by: Andy Chiu <[email protected]> > >> --- > >> Changelog v4: > >> - Include Björn's fix for kprobe > >> - Refactor code for better reading (Robbin, Björn) > >> - Remove make_call_ra and friedns (Björn) > >> - Update comments to match reality (Björn) > >> - Drop code defined by !WITH_ARG > >> - Add a synchronization point when updating ftrace_call_dest (Björn) > >> --- > >> arch/riscv/include/asm/ftrace.h | 49 ++++++------ > >> arch/riscv/kernel/ftrace.c | 130 ++++++++++++++++---------------- > >> arch/riscv/kernel/mcount-dyn.S | 9 +-- > >> 3 files changed, 92 insertions(+), 96 deletions(-) > >> > >> diff --git a/arch/riscv/include/asm/ftrace.h > >> b/arch/riscv/include/asm/ftrace.h > >> index d8b2138bd9c6..6a5c0a7fb826 100644 > >> --- a/arch/riscv/include/asm/ftrace.h > >> +++ b/arch/riscv/include/asm/ftrace.h > >> @@ -20,10 +20,9 @@ extern void *return_address(unsigned int level); > >> #define ftrace_return_address(n) return_address(n) > >> void _mcount(void); > >> -static inline unsigned long ftrace_call_adjust(unsigned long addr) > >> -{ > >> - return addr; > >> -} > >> +unsigned long ftrace_call_adjust(unsigned long addr); > >> +unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip); > >> +#define ftrace_get_symaddr(fentry_ip) > >> arch_ftrace_get_symaddr(fentry_ip) > >> /* > >> * Let's do like x86/arm64 and ignore the compat syscalls. > >> @@ -57,12 +56,21 @@ struct dyn_arch_ftrace { > >> * 2) jalr: setting low-12 offset to ra, jump to ra, and set ra to > >> * return address (original pc + 4) > >> * > >> + * The first 2 instructions for each tracable function is compiled > >> to 2 nop > >> + * instructions. Then, the kernel initializes the first instruction > >> to auipc at > >> + * boot time (<ftrace disable>). The second instruction is patched > >> to jalr to > >> + * start the trace. > >> + * > >> + *<Image>: > >> + * 0: nop > >> + * 4: nop > >> + * > >> *<ftrace enable>: > >> - * 0: auipc t0/ra, 0x? > >> - * 4: jalr t0/ra, ?(t0/ra) > >> + * 0: auipc t0, 0x? > >> + * 4: jalr t0, ?(t0) > >> * > >> *<ftrace disable>: > >> - * 0: nop > >> + * 0: auipc t0, 0x? > >> * 4: nop > >> * > >> * Dynamic ftrace generates probes to call sites, so we must deal with > >> @@ -75,10 +83,9 @@ struct dyn_arch_ftrace { > >> #define AUIPC_OFFSET_MASK (0xfffff000) > >> #define AUIPC_PAD (0x00001000) > >> #define JALR_SHIFT 20 > >> -#define JALR_RA (0x000080e7) > >> -#define AUIPC_RA (0x00000097) > >> #define JALR_T0 (0x000282e7) > >> #define AUIPC_T0 (0x00000297) > >> +#define JALR_RANGE (JALR_SIGN_MASK - 1) > >> #define to_jalr_t0(offset) \ > >> (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0) > >> @@ -96,26 +103,14 @@ do { \ > >> call[1] = to_jalr_t0(offset); \ > >> } while (0) > >> -#define to_jalr_ra(offset) \ > >> - (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_RA) > >> - > >> -#define to_auipc_ra(offset) \ > >> - ((offset & JALR_SIGN_MASK) ? \ > >> - (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_RA) : \ > >> - ((offset & AUIPC_OFFSET_MASK) | AUIPC_RA)) > >> - > >> -#define make_call_ra(caller, callee, call) \ > >> -do { \ > >> - unsigned int offset = \ > >> - (unsigned long) (callee) - (unsigned long) (caller); \ > >> - call[0] = to_auipc_ra(offset); \ > >> - call[1] = to_jalr_ra(offset); \ > >> -} while (0) > >> - > >> /* > >> - * Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes > >> here. > >> + * Only the jalr insn in the auipc+jalr is patched, so we make it 4 > >> + * bytes here. > >> */ > >> -#define MCOUNT_INSN_SIZE 8 > >> +#define MCOUNT_INSN_SIZE 4 > >> +#define MCOUNT_AUIPC_SIZE 4 > >> +#define MCOUNT_JALR_SIZE 4 > >> +#define MCOUNT_NOP4_SIZE 4 > >> #ifndef __ASSEMBLY__ > >> struct dyn_ftrace; > >> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c > >> index 1fd10555c580..cf78eef073a0 100644 > >> --- a/arch/riscv/kernel/ftrace.c > >> +++ b/arch/riscv/kernel/ftrace.c > >> @@ -8,10 +8,21 @@ > >> #include <linux/ftrace.h> > >> #include <linux/uaccess.h> > >> #include <linux/memory.h> > >> +#include <linux/irqflags.h> > >> #include <linux/stop_machine.h> > >> #include <asm/cacheflush.h> > >> #include <asm/text-patching.h> > >> +unsigned long ftrace_call_adjust(unsigned long addr) > >> +{ > >> + return addr + MCOUNT_AUIPC_SIZE; > >> +} > >> + > >> +unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip) > >> +{ > >> + return fentry_ip - MCOUNT_AUIPC_SIZE; > >> +} > >> + > > > > > > Those functions cause the following errors when building with > > !CONFIG_DYNAMIC_FTRACE, but I'm not sure how to fix this: > > > > ../arch/riscv/kernel/ftrace.c: In function 'ftrace_call_adjust': > > ../arch/riscv/kernel/ftrace.c:19:35: error: 'MCOUNT_AUIPC_SIZE' > > undeclared (first use in this function) > > 19 | return addr + 8 + MCOUNT_AUIPC_SIZE; > > | ^~~~~~~~~~~~~~~~~ > > ../arch/riscv/kernel/ftrace.c:19:35: note: each undeclared identifier > > is reported only once for each function it appears in > > CC fs/9p/vfs_dir.o > > ../arch/riscv/kernel/ftrace.c: In function 'arch_ftrace_get_symaddr': > > ../arch/riscv/kernel/ftrace.c:26:28: error: 'MCOUNT_AUIPC_SIZE' > > undeclared (first use in this function) > > 26 | return fentry_ip - MCOUNT_AUIPC_SIZE; > > | ^~~~~~~~~~~~~~~~~ > > CC drivers/pci/pcie/pme.o > > ../arch/riscv/kernel/ftrace.c: In function 'ftrace_call_adjust': > > ../arch/riscv/kernel/ftrace.c:22:1: error: control reaches end of > > non-void function [-Werror=return-type] > > 22 | } > > | ^ > > ../arch/riscv/kernel/ftrace.c: In function 'arch_ftrace_get_symaddr': > > ../arch/riscv/kernel/ftrace.c:27:1: error: control reaches end of > > non-void function [-Werror=return-type] > > 27 | } > > | ^ > > cc1: some warnings being treated as errors > > make[5]: *** [../scripts/Makefile.build:203: > > arch/riscv/kernel/ftrace.o] Error 1 > > > > So I fixed those errors with the following, let me know if that's not > correct: > > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c > index d65f06bfb4573..4c6c24380cfd9 100644 > --- a/arch/riscv/kernel/ftrace.c > +++ b/arch/riscv/kernel/ftrace.c > @@ -13,6 +13,7 @@ > #include <asm/cacheflush.h> > #include <asm/text-patching.h> > > +#ifdef CONFIG_DYNAMIC_FTRACE > unsigned long ftrace_call_adjust(unsigned long addr) > { > if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS)) > @@ -26,7 +27,6 @@ unsigned long arch_ftrace_get_symaddr(unsigned long > fentry_ip) > return fentry_ip - MCOUNT_AUIPC_SIZE; > } > > -#ifdef CONFIG_DYNAMIC_FTRACE > void arch_ftrace_update_code(int command) > { > mutex_lock(&text_mutex); > @@ -191,7 +191,12 @@ int ftrace_update_ftrace_func(ftrace_func_t func) > return 0; > } > > -#endif > +#else /* CONFIG_DYNAMIC_FTRACE */ > +unsigned long ftrace_call_adjust(unsigned long addr) > +{ > + return addr; > +} > +#endif /* CONFIG_DYNAMIC_FTRACE */ > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, >
Hi Alex, Yes, this is valid, thanks for noticing and fixing the error. I would personally prefer leaving the #else /* CONFIG_DYNAMIC_FTRACE */ part in ftrace.h, but it can also come later as a refactor. Or, I can respin the series, with fixes on this and the previous patch, along with some typos and variable declarations that Robin mentioned. Thanks, Andy
