riscv: Optimize function trace

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
					...
				change ftrace function
					...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

func:
xxx
ret

Fixes afc76b8b8011 ("riscv: Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")

Signed-off-by Andy Chiu <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/riscv/Kconfig +1 -1

index e2b656043abf..ee0d39b26794 100644
@@ -138,7 +138,7 @@ config RISCV
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
 	select HAVE_FUNCTION_GRAPH_TRACER
-	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
+	select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
 
 config ARCH_MMAP_RND_BITS_MIN
 	default 18 if 64BIT

Signature · 1 line

2.36.1

Hi Guo,

Quoted 19 lines · On Thu, Jan 12, 2023 at 04:05:57AM -0500, [email protected] wrote:

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
					...
				change ftrace function
					...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

Quoted 3 lines

func:
xxx
ret

As mentioned on the last posting, I don't think this is sufficient to fix the issue. I've replied with more detail there:

https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/

Even in a non-preemptible SMP kernel, if one CPU can be in the middle of executing the ftrace_prologue while another CPU is patching the ftrace_prologue, you have the exact same issue.

For example, if CPU X is in the prologue fetches the old AUIPC and the new JALR (because it races with CPU Y modifying those), CPU X will branch to the wrong address. The race window is much smaller in the absence of preemption, but it's still there (and will be exacerbated in virtual machines since the hypervisor can preempt a vCPU at any time).

Quoted 35 lines · Mark Rutland

Hi Guo,

Quoted 19 lines · On Thu, Jan 12, 2023 at 04:05:57AM -0500, [email protected] wrote:

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
                                    ...
                            change ftrace function
                                    ...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

Quoted 3 lines

func:
xxx
ret

As mentioned on the last posting, I don't think this is sufficient to fix the issue. I've replied with more detail there:

https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/

Even in a non-preemptible SMP kernel, if one CPU can be in the middle of executing the ftrace_prologue while another CPU is patching the ftrace_prologue, you have the exact same issue.

Quoted 5 lines

For example, if CPU X is in the prologue fetches the old AUIPC and the new JALR (because it races with CPU Y modifying those), CPU X will branch to the wrong address. The race window is much smaller in the absence of preemption, but it's still there (and will be exacerbated in virtual machines since the hypervisor can preempt a vCPU at any time).

With that in mind, I think your current implementation of ftrace_make_call() and ftrace_make_nop() have a simlar bug. A caller might execute:

NOP // not yet patched to AUIPC

< AUIPC and JALR instructions both patched >

JALR

... and go to the wrong place.

Assuming individual instruction fetches are atomic, and that you only ever branch to the same trampoline, you could fix that by always leaving the AUIPC in place, so that you only patch the JALR to enable/disable the callsite.

Depending on your calling convention, if you have two free GPRs, you might be able to avoid the stacking of RA by always saving it to a GPR in the callsite, using a different GPR for the address generation, and having the ftrace trampoline restore the original RA value, e.g.

        MV      GPR1, ra
        AUIPC   GPR2, high_bits_of(ftrace_caller)
        JALR    ra, high_bits(GPR2)                     // only patch this

... which'd save an instruction per callsite.

Thanks,
Mark.

Hi Guo,

Quoted 19 lines · On Thu, Jan 12, 2023 at 04:05:57AM -0500, [email protected] wrote:

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
                                      ...
                              change ftrace function
                                      ...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

Quoted 3 lines

func:
xxx
ret

As mentioned on the last posting, I don't think this is sufficient to fix the issue. I've replied with more detail there:

https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/

Even in a non-preemptible SMP kernel, if one CPU can be in the middle of executing the ftrace_prologue while another CPU is patching the ftrace_prologue, you have the exact same issue.

For example, if CPU X is in the prologue fetches the old AUIPC and the new JALR (because it races with CPU Y modifying those), CPU X will branch to the wrong address. The race window is much smaller in the absence of preemption, but it's still there (and will be exacerbated in virtual machines since the hypervisor can preempt a vCPU at any time).

Note that the above is even assuming that instruction fetches are atomic, which I'm not sure is the case; for example arm64 has special CMODX / "Concurrent MODification and eXecutuion of instructions" rules which mean only certain instructions can be patched atomically.

Either I'm missing something that provides mutual exclusion between the patching and execution of the ftrace_prologue, or this patch is not sufficient.

Thanks,
Mark.

Quoted 23 lines

Fixes afc76b8b8011 ("riscv: Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")

Signed-off-by Andy Chiu <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/riscv/Kconfig +1 -2

index e2b656043abf..ee0d39b26794 100644
@@ -138,7 +138,7 @@ config RISCV
      select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
      select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
      select HAVE_FUNCTION_GRAPH_TRACER
-     select HAVE_FUNCTION_TRACER if !XIP_KERNEL
+     select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
 
 config ARCH_MMAP_RND_BITS_MIN
      default 18 if 64BIT
--

2.36.1

Quoted 42 lines · Mark Rutland

Hi Guo,

Quoted 19 lines · On Thu, Jan 12, 2023 at 04:05:57AM -0500, [email protected] wrote:

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
                                      ...
                              change ftrace function
                                      ...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

Quoted 3 lines

func:
xxx
ret

As mentioned on the last posting, I don't think this is sufficient to fix the issue. I've replied with more detail there:

https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/

Even in a non-preemptible SMP kernel, if one CPU can be in the middle of executing the ftrace_prologue while another CPU is patching the ftrace_prologue, you have the exact same issue.

For example, if CPU X is in the prologue fetches the old AUIPC and the new JALR (because it races with CPU Y modifying those), CPU X will branch to the wrong address. The race window is much smaller in the absence of preemption, but it's still there (and will be exacerbated in virtual machines since the hypervisor can preempt a vCPU at any time).

Quoted 7 lines

Note that the above is even assuming that instruction fetches are atomic, which I'm not sure is the case; for example arm64 has special CMODX / "Concurrent MODification and eXecutuion of instructions" rules which mean only certain instructions can be patched atomically.

Either I'm missing something that provides mutual exclusion between the patching and execution of the ftrace_prologue, or this patch is not sufficient.

This patch is sufficient because riscv isn't the same as arm64. It uses default arch_ftrace_update_code, which uses stop_machine. See kernel/trace/ftrace.c:
void __weak arch_ftrace_update_code(int command)

{
        ftrace_run_stop_machine(command);
}

ps:
Yes, it's not good, and it's expensive.

Quoted 50 lines · Guo Ren

Quoted 42 lines · Mark Rutland

Hi Guo,

Quoted 19 lines · On Thu, Jan 12, 2023 at 04:05:57AM -0500, [email protected] wrote:

From: Andy Chiu <[email protected]>

In RISCV, we must use an AUIPC + JALR pair to encode an immediate, forming a jump that jumps to an address over 4K. This may cause errors if we want to enable kernel preemption and remove dependency from patching code with stop_machine(). For example, if a task was switched out on auipc. And, if we changed the ftrace function before it was switched back, then it would jump to an address that has updated 11:0 bits mixing with previous XLEN:12 part.

p: patched area performed by dynamic ftrace
ftrace_prologue:

p|      REG_S   ra, -SZREG(sp)
p|      auipc   ra, 0x? ------------> preempted
                                      ...
                              change ftrace function
                                      ...
p|      jalr    -?(ra) <------------- switched back
p|      REG_L   ra, -SZREG(sp)

Quoted 3 lines

func:
xxx
ret

As mentioned on the last posting, I don't think this is sufficient to fix the issue. I've replied with more detail there:

https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/

Even in a non-preemptible SMP kernel, if one CPU can be in the middle of executing the ftrace_prologue while another CPU is patching the ftrace_prologue, you have the exact same issue.

For example, if CPU X is in the prologue fetches the old AUIPC and the new JALR (because it races with CPU Y modifying those), CPU X will branch to the wrong address. The race window is much smaller in the absence of preemption, but it's still there (and will be exacerbated in virtual machines since the hypervisor can preempt a vCPU at any time).

Quoted 7 lines

Note that the above is even assuming that instruction fetches are atomic, which I'm not sure is the case; for example arm64 has special CMODX / "Concurrent MODification and eXecutuion of instructions" rules which mean only certain instructions can be patched atomically.

Either I'm missing something that provides mutual exclusion between the patching and execution of the ftrace_prologue, or this patch is not sufficient.

Quoted 7 lines

This patch is sufficient because riscv isn't the same as arm64. It uses default arch_ftrace_update_code, which uses stop_machine. See kernel/trace/ftrace.c:
void __weak arch_ftrace_update_code(int command)

{
        ftrace_run_stop_machine(command);
}

Ah; sorry, I had misunderstood here, since the commit message spoke in terms of removing that.

As long as stop_machine() is used I agree this is safe; sorry for the noise.

Quoted 3 lines

ps:
Yes, it's not good, and it's expensive.

We can't have everything! :)

Thanks,
Mark.

From: Guo Ren <[email protected]>

Use a temporary register to reduce the size of detour code from 16 bytes to 8 bytes. The previous implementation is from 'commit afc76b8b8011 ("riscv:
Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")'.

Before the patch:
<func_prolog>:

  0: REG_S  ra, -SZREG(sp)
  4: auipc  ra, ?
  8: jalr   ?(ra)

12: REG_L ra, -SZREG(sp)
(func_boddy)

After the patch:
<func_prolog>:
0: auipc t0, ?
4: jalr t0, ?(t0)
(func_boddy)

This patch not just reduces the size of detour code, but also fixes an important issue:

An Ftrace callback registered with FTRACE_OPS_FL_IPMODIFY flag can actually change the instruction pointer, e.g. to "replace" the given kernel function with a new one, which is needed for livepatching, etc.

In this case, the trampoline (ftrace_regs_caller) would not return to <func_prolog+12> but would rather jump to the new function. So, "REG_L ra, -SZREG(sp)" would not run and the original return address would not be restored. The kernel is likely to hang or crash as a result.

This can be easily demonstrated if one tries to "replace", say, cmdline_proc_show() with a new function with the same signature using instruction_pointer_set(&fregs->regs, new_func_addr) in the Ftrace callback.

Link https://lore.kernel.org/linux-riscv/[email protected]/ open here

Link https://lore.kernel.org/linux-riscv/[email protected]/ open here

Co-developed-by Song Shuai <[email protected]>

Signed-off-by Song Shuai <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Cc Evgenii Shatokhin <[email protected]>

---
  arch/riscv/Makefile             |  4 +-
  arch/riscv/include/asm/ftrace.h | 50 +++++++++++++++++++------
  arch/riscv/kernel/ftrace.c      | 65 ++++++++++-----------------------
  arch/riscv/kernel/mcount-dyn.S  | 42 ++++++++-------------
  4 files changed, 75 insertions(+), 86 deletions(-)

arch/riscv/Makefile +2 -2

index ea5a91da6897..3c9aaf67ed79 100644
@@ -12,9 +12,9 @@ ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
         LDFLAGS_vmlinux := --no-relax
         KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
  ifeq ($(CONFIG_RISCV_ISA_C),y)
-       CC_FLAGS_FTRACE := -fpatchable-function-entry=8
-else
         CC_FLAGS_FTRACE := -fpatchable-function-entry=4
+else
+       CC_FLAGS_FTRACE := -fpatchable-function-entry=2
  endif
  endif
 

arch/riscv/include/asm/ftrace.h +38 -12

index 04dad3380041..9e73922e1e2e 100644
@@ -42,6 +42,14 @@ struct dyn_arch_ftrace {
   * 2) jalr: setting low-12 offset to ra, jump to ra, and set ra to
   *          return address (original pc + 4)
   *
+ *<ftrace enable>:
+ * 0: auipc  t0/ra, 0x?
+ * 4: jalr   t0/ra, ?(t0/ra)
+ *
+ *<ftrace disable>:
+ * 0: nop
+ * 4: nop
+ *
   * Dynamic ftrace generates probes to call sites, so we must deal with
   * both auipc and jalr at the same time.
   */

@@ -52,25 +60,43 @@ struct dyn_arch_ftrace {
  #define AUIPC_OFFSET_MASK      (0xfffff000)
  #define AUIPC_PAD              (0x00001000)
  #define JALR_SHIFT             20
-#define JALR_BASIC             (0x000080e7)
-#define AUIPC_BASIC            (0x00000097)
+#define JALR_RA                        (0x000080e7)
+#define AUIPC_RA               (0x00000097)
+#define JALR_T0                        (0x000282e7)
+#define AUIPC_T0               (0x00000297)
  #define NOP4                   (0x00000013)
 
-#define make_call(caller, callee, call)                                        \
+#define to_jalr_t0(offset)                                             \
+       (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
+
+#define to_auipc_t0(offset)                                            \
+       ((offset & JALR_SIGN_MASK) ?                                    \
+       (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_T0) :       \
+       ((offset & AUIPC_OFFSET_MASK) | AUIPC_T0))
+
+#define make_call_t0(caller, callee, call)                             \
  do {                                                                   \
-       call[0] = to_auipc_insn((unsigned int)((unsigned long)callee -  \
-                               (unsigned long)caller));                \
-       call[1] = to_jalr_insn((unsigned int)((unsigned long)callee -   \
-                              (unsigned long)caller));                 \
+       unsigned int offset =                                           \
+               (unsigned long) callee - (unsigned long) caller;        \
+       call[0] = to_auipc_t0(offset);                                  \
+       call[1] = to_jalr_t0(offset);                                   \
  } while (0)
 
-#define to_jalr_insn(offset)                                           \
-       (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_BASIC)
+#define to_jalr_ra(offset)                                             \
+       (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_RA)
 
-#define to_auipc_insn(offset)                                          \
+#define to_auipc_ra(offset)                                            \
         ((offset & JALR_SIGN_MASK) ?                                    \
-       (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_BASIC) :    \
-       ((offset & AUIPC_OFFSET_MASK) | AUIPC_BASIC))
+       (((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.

arch/riscv/kernel/ftrace.c +19 -46

index 2086f6585773..5bff37af4770 100644
@@ -55,12 +55,15 @@ static int ftrace_check_current_call(unsigned long hook_pos,
  }
 
  static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
-                               bool enable)
+                               bool enable, bool ra)
  {
         unsigned int call[2];
         unsigned int nops[2] = {NOP4, NOP4};
 
-       make_call(hook_pos, target, call);
+       if (ra)
+               make_call_ra(hook_pos, target, call);
+       else
+               make_call_t0(hook_pos, target, call);
 
         /* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
         if (patch_text_nosync

@@ -70,42 +73,13 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
         return 0;
  }
 
-/*
- * Put 5 instructions with 16 bytes at the front of function within
- * patchable function entry nops' area.
- *
- * 0: REG_S  ra, -SZREG(sp)
- * 1: auipc  ra, 0x?
- * 2: jalr   -?(ra)
- * 3: REG_L  ra, -SZREG(sp)
- *
- * So the opcodes is:
- * 0: 0xfe113c23 (sd)/0xfe112e23 (sw)
- * 1: 0x???????? -> auipc
- * 2: 0x???????? -> jalr
- * 3: 0xff813083 (ld)/0xffc12083 (lw)
- */
-#if __riscv_xlen == 64
-#define INSN0  0xfe113c23
-#define INSN3  0xff813083
-#elif __riscv_xlen == 32
-#define INSN0  0xfe112e23
-#define INSN3  0xffc12083
-#endif
-
-#define FUNC_ENTRY_SIZE        16
-#define FUNC_ENTRY_JMP 4
-
  int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
  {
-       unsigned int call[4] = {INSN0, 0, 0, INSN3};
-       unsigned long target = addr;
-       unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
+       unsigned int call[2];
 
-       call[1] = to_auipc_insn((unsigned int)(target - caller));
-       call[2] = to_jalr_insn((unsigned int)(target - caller));
+       make_call_t0(rec->ip, addr, call);
 
-       if (patch_text_nosync((void *)rec->ip, call, FUNC_ENTRY_SIZE))
+       if (patch_text_nosync((void *)rec->ip, call, MCOUNT_INSN_SIZE))
                 return -EPERM;
 
         return 0;

@@ -114,15 +88,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
  int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
                     unsigned long addr)
  {
-       unsigned int nops[4] = {NOP4, NOP4, NOP4, NOP4};
+       unsigned int nops[2] = {NOP4, NOP4};
 
-       if (patch_text_nosync((void *)rec->ip, nops, FUNC_ENTRY_SIZE))
+       if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
                 return -EPERM;
 
         return 0;
  }
 
-
  /*
   * This is called early on, and isn't wrapped by
   * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold

@@ -144,10 +117,10 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
  int ftrace_update_ftrace_func(ftrace_func_t func)
  {
         int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
-                                      (unsigned long)func, true);
+                                      (unsigned long)func, true, true);
         if (!ret) {
                 ret = __ftrace_modify_call((unsigned long)&ftrace_regs_call,
-                                          (unsigned long)func, true);
+                                          (unsigned long)func, true, true);
         }
 
         return ret;

@@ -159,16 +132,16 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
                        unsigned long addr)
  {
         unsigned int call[2];
-       unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
+       unsigned long caller = rec->ip;
         int ret;
 
-       make_call(caller, old_addr, call);
+       make_call_t0(caller, old_addr, call);
         ret = ftrace_check_current_call(caller, call);
 
         if (ret)
                 return ret;
 
-       return __ftrace_modify_call(caller, addr, true);
+       return __ftrace_modify_call(caller, addr, true, false);
  }
  #endif
 

@@ -203,12 +176,12 @@ int ftrace_enable_ftrace_graph_caller(void)
         int ret;
 
         ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
-                                   (unsigned long)&prepare_ftrace_return, true);
+                                   (unsigned long)&prepare_ftrace_return, true, true);
         if (ret)
                 return ret;
 
         return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
-                                   (unsigned long)&prepare_ftrace_return, true);
+                                   (unsigned long)&prepare_ftrace_return, true, true);
  }
 
  int ftrace_disable_ftrace_graph_caller(void)

@@ -216,12 +189,12 @@ int ftrace_disable_ftrace_graph_caller(void)
         int ret;
 
         ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
-                                   (unsigned long)&prepare_ftrace_return, false);
+                                   (unsigned long)&prepare_ftrace_return, false, true);
         if (ret)
                 return ret;
 
         return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
-                                   (unsigned long)&prepare_ftrace_return, false);
+                                   (unsigned long)&prepare_ftrace_return, false, true);
  }
  #endif /* CONFIG_DYNAMIC_FTRACE */
  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */

arch/riscv/kernel/mcount-dyn.S +16 -27

index d171eca623b6..125de818d1ba 100644
@@ -13,8 +13,8 @@
 
         .text
 
-#define FENTRY_RA_OFFSET       12
-#define ABI_SIZE_ON_STACK      72
+#define FENTRY_RA_OFFSET       8
+#define ABI_SIZE_ON_STACK      80
  #define ABI_A0                 0
  #define ABI_A1                 8
  #define ABI_A2                 16

@@ -23,10 +23,10 @@
  #define ABI_A5                 40
  #define ABI_A6                 48
  #define ABI_A7                 56
-#define ABI_RA                 64
+#define ABI_T0                 64
+#define ABI_RA                 72
 
         .macro SAVE_ABI
-       addi    sp, sp, -SZREG
         addi    sp, sp, -ABI_SIZE_ON_STACK
 
         REG_S   a0, ABI_A0(sp)

@@ -37,6 +37,7 @@
         REG_S   a5, ABI_A5(sp)
         REG_S   a6, ABI_A6(sp)
         REG_S   a7, ABI_A7(sp)
+       REG_S   t0, ABI_T0(sp)
         REG_S   ra, ABI_RA(sp)
         .endm
 

@@ -49,24 +50,18 @@
         REG_L   a5, ABI_A5(sp)
         REG_L   a6, ABI_A6(sp)
         REG_L   a7, ABI_A7(sp)
+       REG_L   t0, ABI_T0(sp)
         REG_L   ra, ABI_RA(sp)
 
         addi    sp, sp, ABI_SIZE_ON_STACK
-       addi    sp, sp, SZREG
         .endm
 
  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
         .macro SAVE_ALL
-       addi    sp, sp, -SZREG
         addi    sp, sp, -PT_SIZE_ON_STACK
 
-       REG_S x1,  PT_EPC(sp)
-       addi    sp, sp, PT_SIZE_ON_STACK
-       REG_L x1,  (sp)
-       addi    sp, sp, -PT_SIZE_ON_STACK
+       REG_S t0,  PT_EPC(sp)
         REG_S x1,  PT_RA(sp)
-       REG_L x1,  PT_EPC(sp)
-
         REG_S x2,  PT_SP(sp)
         REG_S x3,  PT_GP(sp)
         REG_S x4,  PT_TP(sp)

@@ -100,15 +95,11 @@
         .endm
 
         .macro RESTORE_ALL
+       REG_L t0,  PT_EPC(sp)
         REG_L x1,  PT_RA(sp)
-       addi    sp, sp, PT_SIZE_ON_STACK
-       REG_S x1,  (sp)
-       addi    sp, sp, -PT_SIZE_ON_STACK
-       REG_L x1,  PT_EPC(sp)
         REG_L x2,  PT_SP(sp)
         REG_L x3,  PT_GP(sp)
         REG_L x4,  PT_TP(sp)
-       REG_L x5,  PT_T0(sp)
         REG_L x6,  PT_T1(sp)
         REG_L x7,  PT_T2(sp)
         REG_L x8,  PT_S0(sp)

@@ -137,17 +128,16 @@
         REG_L x31, PT_T6(sp)
 
         addi    sp, sp, PT_SIZE_ON_STACK
-       addi    sp, sp, SZREG
         .endm
  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 
  ENTRY(ftrace_caller)
         SAVE_ABI
 
-       addi    a0, ra, -FENTRY_RA_OFFSET
+       addi    a0, t0, -FENTRY_RA_OFFSET
         la      a1, function_trace_op
         REG_L   a2, 0(a1)
-       REG_L   a1, ABI_SIZE_ON_STACK(sp)
+       mv      a1, ra
         mv      a3, sp
 
  ftrace_call:

@@ -155,8 +145,8 @@ ftrace_call:
         call    ftrace_stub
 
  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-       addi    a0, sp, ABI_SIZE_ON_STACK
-       REG_L   a1, ABI_RA(sp)
+       addi    a0, sp, ABI_RA
+       REG_L   a1, ABI_T0(sp)
         addi    a1, a1, -FENTRY_RA_OFFSET
  #ifdef HAVE_FUNCTION_GRAPH_FP_TEST
         mv      a2, s0

@@ -166,17 +156,17 @@ ftrace_graph_call:
         call    ftrace_stub
  #endif
         RESTORE_ABI
-       ret
+       jr t0
  ENDPROC(ftrace_caller)
 
  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
  ENTRY(ftrace_regs_caller)
         SAVE_ALL
 
-       addi    a0, ra, -FENTRY_RA_OFFSET
+       addi    a0, t0, -FENTRY_RA_OFFSET
         la      a1, function_trace_op
         REG_L   a2, 0(a1)
-       REG_L   a1, PT_SIZE_ON_STACK(sp)
+       mv      a1, ra
         mv      a3, sp
 
  ftrace_regs_call:

@@ -196,6 +186,6 @@ ftrace_graph_regs_call:
  #endif
 
         RESTORE_ALL
-       ret
+       jr t0
  ENDPROC(ftrace_regs_caller)
  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
--

2.36.1

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
  arch/riscv/Kconfig                          |  2 ++
  samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
  samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
  samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
  samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
  samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
  6 files changed, 142 insertions(+)

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
         select HAVE_POSIX_CPU_TIMERS_TASK_WORK
         select HAVE_REGS_AND_STACK_ACCESS_API
         select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
         select HAVE_STACKPROTECTOR
         select HAVE_SYSCALL_TRACEPOINTS
         select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
  static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
  extern void my_tramp1(void *);
  extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -1

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>
--

2.36.1

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 8 lines

---
  arch/riscv/Kconfig                          |  2 ++
  samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
  samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
  samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
  samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
  samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
  6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
      migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
      migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
      migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
      migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
        rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
           insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
           <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
           <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
        rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 208 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
         select HAVE_POSIX_CPU_TIMERS_TASK_WORK
         select HAVE_REGS_AND_STACK_ACCESS_API
         select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
         select HAVE_STACKPROTECTOR
         select HAVE_SYSCALL_TRACEPOINTS
         select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
  static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
  extern void my_tramp1(void *);
  extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 
  #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
  extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
  #ifdef CONFIG_X86_64
 

Quoted 5 lines

samples/ftrace/ftrace-direct.c +0 -1

  #include <asm/ibt.h>
--

2.36.1

Regards,
Evgenii

Hi, Evgenii:

Quoted 50 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 8 lines

---
   arch/riscv/Kconfig                          |  2 ++
   samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
   samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
   samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
   samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
   samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
   6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
       migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
       migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
       migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
       migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
         rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
            insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
            <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
            <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
         rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do. so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link https://lore.kernel.org/linux-kernel/[email protected]/T/#u open here

Quoted 212 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 208 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
          select HAVE_POSIX_CPU_TIMERS_TASK_WORK
          select HAVE_REGS_AND_STACK_ACCESS_API
          select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
          select HAVE_STACKPROTECTOR
          select HAVE_SYSCALL_TRACEPOINTS
          select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
   static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
   extern void my_tramp1(void *);
   extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

   #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Hi, Song,

Quoted 69 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 17 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
   arch/riscv/Kconfig                          |  2 ++ samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++ samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
   samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++ samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++ samples/ftrace/ftrace-direct.c              | 22 ++++++++++++

Quoted 1 line

   6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         | migration/1-19      [001] .....  3858.532131: my_direct_func1: my direct func1 ip 0
       migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my direct func1 ip ff60000001ba9600
       migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my direct func1 ip ff60000003334d00
       migration/0-15      [000] .....  3858.532232: my_direct_func1: my direct func1 ip 0
         rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my direct func1 ip 0
            insmod-415     [000] .....  3858.532270: my_direct_func1: my direct func1 ip 7fffffffffffffff
            <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my direct func1 ip ff60000001ba9600
            <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my direct func1 ip ff60000001ba9600
         rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

Quoted 2 lines

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

Quoted 225 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do. so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link https://lore.kernel.org/linux-kernel/[email protected]/T/#u open here

Quoted 212 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 208 lines

arch/riscv/Kconfig +0 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV

          select HAVE_POSIX_CPU_TIMERS_TASK_WORK
          select HAVE_REGS_AND_STACK_ACCESS_API
          select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
          select HAVE_STACKPROTECTOR
          select HAVE_SYSCALL_TRACEPOINTS
          select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -0

    #include <asm/ibt.h>

Signature · 1 line

2.36.1

Signature · 2 lines

Thanks,
Song

Regards,
Evgenii

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
   arch/riscv/Kconfig                          |  2 ++
   samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
   samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
   samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
   samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
   samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
   6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
       migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
       migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
       migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
       migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
         rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
            insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
            <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
            <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
         rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
           insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
  rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
           insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
  systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
          systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
           <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
           <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
      rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Quoted 244 lines

Quoted 233 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
          select HAVE_POSIX_CPU_TIMERS_TASK_WORK
          select HAVE_REGS_AND_STACK_ACCESS_API
          select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
          select HAVE_STACKPROTECTOR
          select HAVE_SYSCALL_TRACEPOINTS
          select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
   static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
   extern void my_tramp1(void *);
   extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 
   #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
   extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
   #ifdef CONFIG_X86_64
 

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

   #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Regards,
Evgenii

Quoted 116 lines · Evgenii Shatokhin <[email protected]> 于2023年1月17日周二 16:22写道：

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
    arch/riscv/Kconfig                          |  2 ++
    samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
    samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
    samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
    samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
    samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
    6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
        migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
        migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
        migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
        migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
          rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
             insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
             <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
             <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
          rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
   rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
            insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
   systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
           systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
            <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
            <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
       rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

Quoted 7 lines

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Yes, you're right.

I added 'mv a0,t0' in front of call my_direct_func to pass the address of traced function with mcount offset.

Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Quoted 244 lines

Quoted 244 lines

Quoted 243 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
           select HAVE_POSIX_CPU_TIMERS_TASK_WORK
           select HAVE_REGS_AND_STACK_ACCESS_API
           select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
           select HAVE_STACKPROTECTOR
           select HAVE_SYSCALL_TRACEPOINTS
           select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

    #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Quoted 4 lines

Quoted 2 lines

Regards,
Evgenii

--
Thanks,
Song

Quoted 125 lines · Song Shuai

Quoted 116 lines · Evgenii Shatokhin <[email protected]> 于2023年1月17日周二 16:22写道：

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
    arch/riscv/Kconfig                          |  2 ++
    samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
    samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
    samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
    samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
    samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
    6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
        migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
        migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
        migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
        migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
          rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
             insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
             <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
             <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
          rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
   rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
            insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
   systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
           systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
            <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
            <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
       rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

Quoted 7 lines

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Quoted 7 lines

Yes, you're right.

I added 'mv a0,t0' in front of call my_direct_func to pass the address of traced function with mcount offset.

Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Thank you for the quick fix. This one looks good to me.

ftrace-direct-multi*.ko now report the ip values corresponding to schedule+0x8 and wake_up_process+0x8, which is what was expected here.

One more thing: please change my "Co-developed-by:" into "Tested-by:" in your patch, becase this is what I actually did: tested it and reported the results. I cannot take your credit for development of this patch ;-)

Looking forward for v8 of the series.

Quoted 254 lines

Quoted 244 lines

Quoted 244 lines

Quoted 243 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
           select HAVE_POSIX_CPU_TIMERS_TASK_WORK
           select HAVE_REGS_AND_STACK_ACCESS_API
           select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
           select HAVE_STACKPROTECTOR
           select HAVE_SYSCALL_TRACEPOINTS
           select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

    #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Quoted 4 lines

Quoted 2 lines

Regards,
Evgenii

--
Thanks,
Song

Thx Evgenii & Song,

I got it; it would be put into v8.

Quoted 395 lines · Evgenii Shatokhin

Quoted 125 lines · Song Shuai

Quoted 116 lines · Evgenii Shatokhin <[email protected]> 于2023年1月17日周二 16:22写道：

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
    arch/riscv/Kconfig                          |  2 ++
    samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
    samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
    samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
    samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
    samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
    6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
        migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
        migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
        migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
        migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
          rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
             insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
             <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
             <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
          rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
   rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
            insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
   systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
           systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
            <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
            <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
       rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

Quoted 7 lines

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Quoted 7 lines

Yes, you're right.

I added 'mv a0,t0' in front of call my_direct_func to pass the address of traced function with mcount offset.

Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Thank you for the quick fix. This one looks good to me.

ftrace-direct-multi*.ko now report the ip values corresponding to schedule+0x8 and wake_up_process+0x8, which is what was expected here.

One more thing: please change my "Co-developed-by:" into "Tested-by:" in your patch, becase this is what I actually did: tested it and reported the results. I cannot take your credit for development of this patch ;-)

Looking forward for v8 of the series.

Quoted 250 lines

Quoted 248 lines

Quoted 246 lines

Quoted 235 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
           select HAVE_POSIX_CPU_TIMERS_TASK_WORK
           select HAVE_REGS_AND_STACK_ACCESS_API
           select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
           select HAVE_STACKPROTECTOR
           select HAVE_SYSCALL_TRACEPOINTS
           select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

    #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Regards,
Evgenii

Quoted 6 lines

Quoted 4 lines

--
Thanks,
Song

Signature · 2 lines

Best Regards
 Guo Ren

Quoted 3 lines · On Wed, 18 Jan 2023 22:05:04 PST (-0800), [email protected] wrote:

Thx Evgenii & Song,

I got it; it would be put into v8.

Sorry if I missed it, but I don't see a v8 on patchwork. I queued up the first three patches onto for-next as they seem like pretty independent fixes/cleanups, I'll hold off on the others until v8.

Thanks!

Quoted 409 lines

Quoted 137 lines · Evgenii Shatokhin

Quoted 125 lines · Song Shuai

Quoted 116 lines · Evgenii Shatokhin <[email protected]> 于2023年1月17日周二 16:22写道：

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
    arch/riscv/Kconfig                          |  2 ++
    samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
    samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
    samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
    samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
    samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
    6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
        migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
        migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
        migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
        migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
          rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
             insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
             <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
             <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
          rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
   rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
            insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
   systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
           systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
            <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
            <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
       rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

Quoted 7 lines

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Quoted 7 lines

Yes, you're right.

I added 'mv a0,t0' in front of call my_direct_func to pass the address of traced function with mcount offset.

Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Thank you for the quick fix. This one looks good to me.

Quoted 264 lines

ftrace-direct-multi*.ko now report the ip values corresponding to schedule+0x8 and wake_up_process+0x8, which is what was expected here.

One more thing: please change my "Co-developed-by:" into "Tested-by:" in your patch, becase this is what I actually did: tested it and reported the results. I cannot take your credit for development of this patch ;-)

Looking forward for v8 of the series.

Quoted 254 lines

Quoted 244 lines

Quoted 244 lines

Quoted 243 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
           select HAVE_POSIX_CPU_TIMERS_TASK_WORK
           select HAVE_REGS_AND_STACK_ACCESS_API
           select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
           select HAVE_STACKPROTECTOR
           select HAVE_SYSCALL_TRACEPOINTS
           select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

    #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Quoted 4 lines

Quoted 2 lines

Regards,
Evgenii

--
Thanks,
Song

--
Best Regards
Guo Ren

Hi,Guo:

Quoted 6 lines · Palmer Dabbelt <[email protected]> 于2023年2月18日周六 21:30写道：

Quoted 3 lines · On Wed, 18 Jan 2023 22:05:04 PST (-0800), [email protected] wrote:

Thx Evgenii & Song,

I got it; it would be put into v8.

There were 2 problems for this patch in its V6 and V7 version:

1. build error resulted by including of nospec-branch.h file in V6
- this common fixup patch had been sent to the linux-trace mail
list and would show up in Linux v6.3
- here is the link:
https://lore.kernel.org/lkml/[email protected]/

2. invalid code address reported by Evgenii in V7
- for your convenience, the link to this updated patch is
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Is it time to launch the V8 with this updated patch, or waiting for the common fixup patch to be merged?

Quoted 416 lines

Sorry if I missed it, but I don't see a v8 on patchwork. I queued up the first three patches onto for-next as they seem like pretty independent fixes/cleanups, I'll hold off on the others until v8.

Thanks!

Quoted 409 lines

Quoted 137 lines · Evgenii Shatokhin

Quoted 125 lines · Song Shuai

Quoted 116 lines · Evgenii Shatokhin <[email protected]> 于2023年1月17日周二 16:22写道：

Quoted 79 lines · Evgenii Shatokhin

Hi, Song,

Quoted 63 lines · Song Shuai

Hi, Evgenii:

Quoted 51 lines · Evgenii Shatokhin <[email protected]> 于2023年1月16日周一 14:30写道：

Hi,

Quoted 9 lines · On 12.01.2023 12:06, [email protected] wrote:

From: Song Shuai <[email protected]>

select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for the ftrace-direct*.c files in samples/ftrace/.

Signed-off-by Song Shuai <[email protected]>

Tested-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Quoted 9 lines

---
    arch/riscv/Kconfig                          |  2 ++
    samples/ftrace/ftrace-direct-modify.c       | 33 ++++++++++++++++++
    samples/ftrace/ftrace-direct-multi-modify.c | 37
+++++++++++++++++++++
    samples/ftrace/ftrace-direct-multi.c        | 22 ++++++++++++
    samples/ftrace/ftrace-direct-too.c          | 26 +++++++++++++++
    samples/ftrace/ftrace-direct.c              | 22 ++++++++++++
    6 files changed, 142 insertions(+)

The samples were built OK now, but ftrace-direct-multi and ftrace-direct-multi-modify report incorrect values of ip/pc it seems.

I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked the messages in the trace:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
        migration/1-19      [001] .....  3858.532131: my_direct_func1: my
direct func1 ip 0
        migration/0-15      [000] d.s2.  3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
        migration/0-15      [000] d..2.  3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
        migration/0-15      [000] .....  3858.532232: my_direct_func1: my
direct func1 ip 0
          rcu_sched-14      [001] .....  3858.532257: my_direct_func1: my
direct func1 ip 0
             insmod-415     [000] .....  3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
             <idle>-0       [001] ..s1.  3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
             <idle>-0       [001] dns2.  3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
          rcu_sched-14      [001] .....  3858.539208: my_direct_func1: my

direct func1 ip 0
[...]

Quoted 7 lines

If I understand it right, my_direct_func1() should print the address of some location in the code, probably - at the beginning of the traced functions.

The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code addresses.

Quoted 8 lines

The invalid code address is only printed by accessing the schedule() function's first argument whose address stores in a0 register. While schedule() actually has no parameter declared, so my_direct_func just prints the a0 in the context of the schedule()'s caller and the address maybe varies depending on the caller.

I can't really understand why tracing the first argument of the schedule() function, but it seems nonsense at this point.

The question is, what should be passed as the argument(s) of my_direct_func() in this particular sample module. The kernel docs and commit logs seem to contain no info on that.

Quoted 5 lines

With direct functions, I suppose, the trampoline can pass anything it wants to my_direct_func(), not just the arguments of the traced function.

I'd check what these sample modules do on x86 and would try to match that behaviour on RISC-V.

I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should be passed to my_direct_func().

ftrace-direct-multi.ko:

#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            insmod-10829   [000] d.h1.  1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
   rcu_tasks_kthre-11      [000] .....  1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
            insmod-10829   [000] .....  1719.518708: my_direct_func: ip
ffffffff8828d935
   systemd-journal-293     [001] .....  1719.518823: my_direct_func: ip
ffffffff8828d935
           systemd-1       [000] .....  1719.519141: my_direct_func: ip
ffffffff8828d935
            <idle>-0       [001] ..s1.  1719.521889: my_direct_func: ip
ffffffff87332f45
            <idle>-0       [000] d.s2.  1719.521901: my_direct_func: ip
ffffffff87332f45
       rcu_preempt-15      [001] .....  1719.521917: my_direct_func: ip

ffffffff8828d935
[...]

The ip values are wake_up_process+0x5 and schedule+0x5, the locations where the execution of the traced functions resumes after the Ftrace trampoline has finished.

Quoted 7 lines

The results with ftrace-direct-multi-modify.ko are similar to that.

The samples look like a demonstration, that one can pass anything necessary to the handler in case of "direct" functions.

I suppose, the RISC-V-specific asm code in these two sample modules could be updated to pass the saved pc value to my_direct_func() in a0.

Quoted 7 lines

Yes, you're right.

I added 'mv a0,t0' in front of call my_direct_func to pass the address of traced function with mcount offset.

Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318

Thank you for the quick fix. This one looks good to me.

Quoted 264 lines

ftrace-direct-multi*.ko now report the ip values corresponding to schedule+0x8 and wake_up_process+0x8, which is what was expected here.

One more thing: please change my "Co-developed-by:" into "Tested-by:" in your patch, becase this is what I actually did: tested it and reported the results. I cannot take your credit for development of this patch ;-)

Looking forward for v8 of the series.

Quoted 254 lines

Quoted 244 lines

Quoted 244 lines

Quoted 243 lines

As for this patch, it just impls a simple mcount (direct_caller) to trace kernel functions, and basically saves the necessary ABI, call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.

I started an independent patch to replace schedule with kick_process to make these samples more reasonable. And It has no conflict with the current patch, so we can go on.

Link:
https://lore.kernel.org/linux-kernel/[email protected]/T/#u

Quoted 217 lines

The same issue is with ftrace-direct-multi-modify.ko.

Is anything missing here?

Quoted 213 lines

arch/riscv/Kconfig +2 -0

index 307a9f413edd..e944af44f681 100644
@@ -112,6 +112,8 @@ config RISCV
           select HAVE_POSIX_CPU_TIMERS_TASK_WORK
           select HAVE_REGS_AND_STACK_ACCESS_API
           select HAVE_FUNCTION_ARG_ACCESS_API
+       select HAVE_SAMPLE_FTRACE_DIRECT
+       select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
           select HAVE_STACKPROTECTOR
           select HAVE_SYSCALL_TRACEPOINTS
           select HAVE_RSEQ

samples/ftrace/ftrace-direct-modify.c +33 -0

index de5a0f67f320..be7bf472c3c7 100644
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
 
    static unsigned long my_ip = (unsigned long)schedule;
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func1\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"      addi sp,sp,-16\n"
+"      sd   t0,0(sp)\n"
+"      sd   ra,8(sp)\n"
+"      call my_direct_func2\n"
+"      ld   t0,0(sp)\n"
+"      ld   ra,8(sp)\n"
+"      addi sp,sp,16\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi-modify.c +37 -0

index d52370cad0b6..10884bf418f7 100644
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
    extern void my_tramp1(void *);
    extern void my_tramp2(void *);
 
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection    .text, \"ax\", @progbits\n"
+"      .type           my_tramp1, @function\n"
+"      .globl          my_tramp1\n"
+"   my_tramp1:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func1\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp1, .-my_tramp1\n"
+
+"      .type           my_tramp2, @function\n"
+"      .globl          my_tramp2\n"
+"   my_tramp2:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func2\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"      jr t0\n"
+"      .size           my_tramp2, .-my_tramp2\n"
+"      .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-multi.c +22 -0

index ec1088922517..a35bf43bf6d7 100644
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct-too.c +26 -0

index e13fb59a2b47..3b62e33c2e6d 100644
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-40\n"
+"       sd   a0,0(sp)\n"
+"       sd   a1,8(sp)\n"
+"       sd   a2,16(sp)\n"
+"       sd   t0,24(sp)\n"
+"       sd   ra,32(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   a1,8(sp)\n"
+"       ld   a2,16(sp)\n"
+"       ld   t0,24(sp)\n"
+"       ld   ra,32(sp)\n"
+"       addi sp,sp,40\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 
    #include <asm/ibt.h>

samples/ftrace/ftrace-direct.c +22 -0

index 1f769d0db20f..2cfe5a7d2d70 100644
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
 
    extern void my_tramp(void *);
 
+#ifdef CONFIG_RISCV
+
+asm ("       .pushsection    .text, \"ax\", @progbits\n"
+"       .type           my_tramp, @function\n"
+"       .globl          my_tramp\n"
+"   my_tramp:\n"
+"       addi sp,sp,-24\n"
+"       sd   a0,0(sp)\n"
+"       sd   t0,8(sp)\n"
+"       sd   ra,16(sp)\n"
+"       call my_direct_func\n"
+"       ld   a0,0(sp)\n"
+"       ld   t0,8(sp)\n"
+"       ld   ra,16(sp)\n"
+"       addi sp,sp,24\n"
+"       jr t0\n"
+"       .size           my_tramp, .-my_tramp\n"
+"       .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
    #ifdef CONFIG_X86_64
 

Quoted 3 lines

Quoted 3 lines

samples/ftrace/ftrace-direct.c +0 -1

    #include <asm/ibt.h>
--

2.36.1

--
Thanks,
Song

Quoted 4 lines

Quoted 2 lines

Regards,
Evgenii

--
Thanks,
Song

--
Best Regards
Guo Ren

--
Thanks,
Song