[PATCH -next 0/7] riscv: Improvments for stacktrace

[PATCH -next 0/7] riscv: Improvments for stacktrace

[Chen Zhongjin]

Currently, the stacktrace with FRAME_POINTER on riscv has some problem:

1. stacktrace will stop at irq so it can't get the stack frames before
irq entry.
2. stacktrace can't unwind all the real stack frames when there is
k{ret}probes or ftrace.

These are mainly becase when there is a pt_regs on stack, we can't unwind
the stack frame as normal function.

Some architectures (e.g. arm64) create a extra stackframe inside pt_regs.
However this doesn't work for riscv because the ra is not ensured to be
pushed to stack. As explained in:
commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")

So, I choosed the method of x86 that, if there is a pt_regs on stack,
we encoded the frame pointer and save it. When unwinding stack frame,
we can get pt_regs and registers required for unwinding stacks.

In addition, the patch set contains some refactoring of stacktrace.c to
keep the stacktrace code on riscv consistent with other architectures.

Chen Zhongjin (7):
  riscv: stacktrace: Replace walk_stackframe with arch_stack_walk
  riscv: stacktrace: Introduce unwind functions
  riscv: stacktrace: Save pt_regs in encoded fp on irq entry
  riscv: syscall: Don't clobber s0 when syscall
  riscv: stacktrace: Implement stacktrace for irq
  riscv: stacktrace: Fix unwinding on ftrace_regs_call
  riscv: stacktrace: Fix unwinding on __kretporbe_trampoline

 arch/riscv/include/asm/frame.h                |  45 +++++
 arch/riscv/include/asm/stacktrace.h           |  13 +-
 arch/riscv/kernel/entry.S                     |  23 +--
 arch/riscv/kernel/mcount-dyn.S                |   8 +
 arch/riscv/kernel/perf_callchain.c            |   2 +-
 arch/riscv/kernel/probes/kprobes_trampoline.S |   8 +
 arch/riscv/kernel/stacktrace.c                | 155 ++++++++++++------
 7 files changed, 195 insertions(+), 59 deletions(-)
 create mode 100644 arch/riscv/include/asm/frame.h

-- 
2.17.1

[PATCH -next 1/7] riscv: stacktrace: Replace walk_stackframe with arch_stack_walk

[Chen Zhongjin]

walk_stackframe can be all replaced by arch_stack_walk.

Since walk_stackframe is only called by arch_stack_walk and their only
difference is the argument sequence.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/include/asm/stacktrace.h |  2 --
 arch/riscv/kernel/perf_callchain.c  |  2 +-
 arch/riscv/kernel/stacktrace.c      | 29 +++++++++++++----------------
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index 3450c1912afd..b6cd3eddfd38 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -11,8 +11,6 @@ struct stackframe {
 	unsigned long ra;
 };
 
-extern void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-				    bool (*fn)(void *, unsigned long), void *arg);
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 			   const char *loglvl);
 
diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
index 3348a61de7d9..c023e0b1eb81 100644
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
@@ -74,5 +74,5 @@ static bool fill_callchain(void *entry, unsigned long pc)
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 			   struct pt_regs *regs)
 {
-	walk_stackframe(NULL, regs, fill_callchain, entry);
+	arch_stack_walk(fill_callchain, entry, NULL, regs);
 }
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 08d11a53f39e..b51e32d50a0e 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -16,8 +16,9 @@
 
 #ifdef CONFIG_FRAME_POINTER
 
-void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-			     bool (*fn)(void *, unsigned long), void *arg)
+noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
+		  void *cookie, struct task_struct *task,
+		  struct pt_regs *regs)
 {
 	unsigned long fp, sp, pc;
 	int level = 0;
@@ -29,7 +30,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 	} else if (task == NULL || task == current) {
 		fp = (unsigned long)__builtin_frame_address(0);
 		sp = current_stack_pointer;
-		pc = (unsigned long)walk_stackframe;
+		pc = (unsigned long)arch_stack_walk;
 	} else {
 		/* task blocked in __switch_to */
 		fp = task->thread.s[0];
@@ -41,7 +42,8 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		unsigned long low, high;
 		struct stackframe *frame;
 
-		if (unlikely(!__kernel_text_address(pc) || (level++ >= 1 && !fn(arg, pc))))
+		if (unlikely(!__kernel_text_address(pc) ||
+		   (level++ >= 1 && !consume_entry(cookie, pc))))
 			break;
 
 		/* Validate frame pointer */
@@ -66,8 +68,9 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 
 #else /* !CONFIG_FRAME_POINTER */
 
-void notrace walk_stackframe(struct task_struct *task,
-	struct pt_regs *regs, bool (*fn)(void *, unsigned long), void *arg)
+noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
+		  void *cookie, struct task_struct *task,
+		  struct pt_regs *regs)
 {
 	unsigned long sp, pc;
 	unsigned long *ksp;
@@ -77,7 +80,7 @@ void notrace walk_stackframe(struct task_struct *task,
 		pc = instruction_pointer(regs);
 	} else if (task == NULL || task == current) {
 		sp = current_stack_pointer;
-		pc = (unsigned long)walk_stackframe;
+		pc = (unsigned long)arch_stack_walk;
 	} else {
 		/* task blocked in __switch_to */
 		sp = task->thread.sp;
@@ -89,7 +92,7 @@ void notrace walk_stackframe(struct task_struct *task,
 
 	ksp = (unsigned long *)sp;
 	while (!kstack_end(ksp)) {
-		if (__kernel_text_address(pc) && unlikely(!fn(arg, pc)))
+		if (__kernel_text_address(pc) && unlikely(!consume_entry(cookie, pc)))
 			break;
 		pc = (*ksp++) - 0x4;
 	}
@@ -108,7 +111,7 @@ static bool print_trace_address(void *arg, unsigned long pc)
 noinline void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 		    const char *loglvl)
 {
-	walk_stackframe(task, regs, print_trace_address, (void *)loglvl);
+	arch_stack_walk(print_trace_address, (void *)loglvl, task, regs);
 }
 
 void show_stack(struct task_struct *task, unsigned long *sp, const char *loglvl)
@@ -133,13 +136,7 @@ unsigned long __get_wchan(struct task_struct *task)
 
 	if (!try_get_task_stack(task))
 		return 0;
-	walk_stackframe(task, NULL, save_wchan, &pc);
+	arch_stack_walk(save_wchan, &pc, task, NULL);
 	put_task_stack(task);
 	return pc;
 }
-
-noinline void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
-		     struct task_struct *task, struct pt_regs *regs)
-{
-	walk_stackframe(task, regs, consume_entry, cookie);
-}
-- 
2.17.1

[PATCH -next 2/7] riscv: stacktrace: Introduce unwind functions

[Chen Zhongjin]

Now all riscv unwinding code is inside arch_stack_walk. It's
not same as other architectures.

Make some refactoring, to move unwinding code into unwind() and
unwind_next() functions, which walks through all stack frames
or single frame.

This patch only moves code but doesn't make any logical change.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/include/asm/stacktrace.h |   7 ++
 arch/riscv/kernel/stacktrace.c      | 104 ++++++++++++++++++----------
 2 files changed, 74 insertions(+), 37 deletions(-)

diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index b6cd3eddfd38..a39e4ef1dbd5 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -11,6 +11,13 @@ struct stackframe {
 	unsigned long ra;
 };
 
+struct unwind_state {
+	unsigned long fp;
+	unsigned long sp;
+	unsigned long pc;
+	struct pt_regs *regs;
+};
+
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 			   const char *loglvl);
 
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index b51e32d50a0e..e84e21868a3e 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -16,54 +16,84 @@
 
 #ifdef CONFIG_FRAME_POINTER
 
-noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
-		  void *cookie, struct task_struct *task,
-		  struct pt_regs *regs)
+static int notrace unwind_next(struct unwind_state *state)
 {
-	unsigned long fp, sp, pc;
-	int level = 0;
+	unsigned long low, high, fp;
+	struct stackframe *frame;
 
-	if (regs) {
-		fp = frame_pointer(regs);
-		sp = user_stack_pointer(regs);
-		pc = instruction_pointer(regs);
-	} else if (task == NULL || task == current) {
-		fp = (unsigned long)__builtin_frame_address(0);
-		sp = current_stack_pointer;
-		pc = (unsigned long)arch_stack_walk;
+	fp = state->fp;
+
+	/* Validate frame pointer */
+	low = state->sp + sizeof(struct stackframe);
+	high = ALIGN(low, THREAD_SIZE);
+
+	if (fp < low || fp > high || fp & 0x7)
+		return -EINVAL;
+
+	/* Unwind stack frame */
+	frame = (struct stackframe *)fp - 1;
+	state->sp = fp;
+
+	if (state->regs && state->regs->epc == state->pc &&
+		fp & 0x7) {
+		state->fp = frame->ra;
+		state->pc = state->regs->ra;
 	} else {
-		/* task blocked in __switch_to */
-		fp = task->thread.s[0];
-		sp = task->thread.sp;
-		pc = task->thread.ra;
+		state->fp = frame->fp;
+		state->pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
+							(unsigned long *)fp - 1);
 	}
 
-	for (;;) {
-		unsigned long low, high;
-		struct stackframe *frame;
+	return 0;
+}
 
-		if (unlikely(!__kernel_text_address(pc) ||
-		   (level++ >= 1 && !consume_entry(cookie, pc))))
+static void notrace unwind(struct unwind_state *state,
+				stack_trace_consume_fn consume_entry, void *cookie)
+{
+	while (1) {
+		int ret;
+
+		if (!__kernel_text_address(state->pc))
+			break;
+
+		if (!consume_entry(cookie, state->pc))
 			break;
 
-		/* Validate frame pointer */
-		low = sp + sizeof(struct stackframe);
-		high = ALIGN(sp, THREAD_SIZE);
-		if (unlikely(fp < low || fp > high || fp & 0x7))
+		ret = unwind_next(state);
+		if (ret < 0)
 			break;
-		/* Unwind stack frame */
-		frame = (struct stackframe *)fp - 1;
-		sp = fp;
-		if (regs && (regs->epc == pc) && (frame->fp & 0x7)) {
-			fp = frame->ra;
-			pc = regs->ra;
-		} else {
-			fp = frame->fp;
-			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
-						   (unsigned long *)(fp - 8));
-		}
+	}
+}
+
+noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
+		  void *cookie, struct task_struct *task,
+		  struct pt_regs *regs)
+{
+	struct unwind_state state;
+
+	if (task == NULL)
+		task = current;
 
+	if (regs) {
+		state.fp = frame_pointer(regs);
+		state.sp = user_stack_pointer(regs);
+		state.pc = instruction_pointer(regs);
+		state.regs = regs;
+	} else if (task == current) {
+		state.fp = (unsigned long)__builtin_frame_address(0);
+		state.sp = current_stack_pointer;
+		state.pc = (unsigned long)arch_stack_walk;
+
+		/* skip frame of arch_stack_walk */
+		unwind_next(&state);
+	} else {
+		/* task blocked in __switch_to */
+		state.fp = task->thread.s[0];
+		state.sp = task->thread.sp;
+		state.pc = task->thread.ra;
 	}
+
+	unwind(&state, consume_entry, cookie);
 }
 
 #else /* !CONFIG_FRAME_POINTER */
-- 
2.17.1

[PATCH -next 3/7] riscv: stacktrace: Save pt_regs in encoded fp on irq entry

[Chen Zhongjin]

To support stack unwinding at irq entry, the position of pt_regs saved
on stack is nessesary. Because for some functions, compiler only push
s0/fp on stack without ra.

As the situation described in
commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")

When irq happens there, the the function frame looks like:

prev function	|       ...       |
		|                 |
normal function +-----------------+
		| ra to prev      |
		| s0 of prev      |
		|       ...       |<-+
leaf function	+-----------------+  |
		| s0 of normal    |  |
		| empty slot      |  |
irq pt_regs	+-----------------+  |
		| epc (ra to leaf)|  |
		| ra  (ra to norm)|  |
		| ...             |  |
                | s0 of leaf      |--+
		| ...             |
		+-----------------+

If the position of register in pt_regs is {epc, s0}, we can easily
unwind from irq frame to leaf function, as normal functions do.

However when unwinding from unwinding from leaf to normal, beacause
(ra to norm) is saved in pt_regs, but not stackframe of leaf, we
have to get pt_regs for that.

To get pt_regs position on stack, we can save the encoded *pt_regs
in s0, as x86 architecture did. Then we can get s0, epc and ra
easily.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/include/asm/frame.h | 45 ++++++++++++++++++++++++++++++++++
 arch/riscv/kernel/entry.S      |  3 +++
 2 files changed, 48 insertions(+)
 create mode 100644 arch/riscv/include/asm/frame.h

diff --git a/arch/riscv/include/asm/frame.h b/arch/riscv/include/asm/frame.h
new file mode 100644
index 000000000000..2a1f45cf3a4e
--- /dev/null
+++ b/arch/riscv/include/asm/frame.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_RISCV_FRAME_H
+#define _ASM_RISCV_FRAME_H
+
+#include <asm/asm.h>
+
+#ifdef CONFIG_FRAME_POINTER
+
+#ifdef __ASSEMBLY__
+
+/*
+ * This is a sneaky trick to help the unwinder find pt_regs on the stack.  The
+ * frame pointer is replaced with an encoded pointer to pt_regs.  The encoding
+ * is just setting the LSB, which makes it an invalid stack address and is also
+ * a signal to the unwinder that it's a pt_regs pointer in disguise.
+ *
+ * This macro must be used when sp point to pt_regs
+ */
+.macro ENCODE_FRAME_POINTER
+	add s0, sp, 0x1
+.endm
+
+#else /* !__ASSEMBLY__ */
+
+#define ENCODE_FRAME_POINTER			\
+	"add s0, sp, 0x1\n\t"
+
+#endif /* __ASSEMBLY__ */
+
+#else /* !CONFIG_FRAME_POINTER */
+
+#ifdef __ASSEMBLY__
+
+.macro ENCODE_FRAME_POINTER ptregs_offset=0
+.endm
+
+#else /* !__ASSEMBLY */
+
+#define ENCODE_FRAME_POINTER
+
+#endif /* !__ASSEMBLY */
+
+#endif /* CONFIG_FRAME_POINTER */
+
+#endif /* _ASM_RISCV_FRAME_H */
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index b9eda3fcbd6d..ecb15c7430b4 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -13,6 +13,7 @@
 #include <asm/thread_info.h>
 #include <asm/asm-offsets.h>
 #include <asm/errata_list.h>
+#include <asm/frame.h>
 
 #if !IS_ENABLED(CONFIG_PREEMPTION)
 .set resume_kernel, restore_all
@@ -95,6 +96,8 @@ _save_context:
 	REG_S s4, PT_CAUSE(sp)
 	REG_S s5, PT_TP(sp)
 
+	ENCODE_FRAME_POINTER
+
 	/*
 	 * Set the scratch register to 0, so that if a recursive exception
 	 * occurs, the exception vector knows it came from the kernel
-- 
2.17.1

[PATCH -next 4/7] riscv: syscall: Don't clobber s0 when syscall

[Chen Zhongjin]

syscall uses s0 to load address of sys_call_table.

Since now we uses s0 to save pt_regs for unwinding, clobber
s0 can make unwinder treat s0 as pt_regs address. Use s1 for
this job.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/kernel/entry.S | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index ecb15c7430b4..a3b14a649782 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -205,33 +205,33 @@ handle_syscall:
 check_syscall_nr:
 	/* Check to make sure we don't jump to a bogus syscall number. */
 	li t0, __NR_syscalls
-	la s0, sys_ni_syscall
+	la s1, sys_ni_syscall
 	/*
 	 * Syscall number held in a7.
 	 * If syscall number is above allowed value, redirect to ni_syscall.
 	 */
 	bgeu a7, t0, 3f
 #ifdef CONFIG_COMPAT
-	REG_L s0, PT_STATUS(sp)
-	srli s0, s0, SR_UXL_SHIFT
-	andi s0, s0, (SR_UXL >> SR_UXL_SHIFT)
+	REG_L s1, PT_STATUS(sp)
+	srli s1, s1, SR_UXL_SHIFT
+	andi s1, s1, (SR_UXL >> SR_UXL_SHIFT)
 	li t0, (SR_UXL_32 >> SR_UXL_SHIFT)
-	sub t0, s0, t0
+	sub t0, s1, t0
 	bnez t0, 1f
 
 	/* Call compat_syscall */
-	la s0, compat_sys_call_table
+	la s1, compat_sys_call_table
 	j 2f
 1:
 #endif
 	/* Call syscall */
-	la s0, sys_call_table
+	la s1, sys_call_table
 2:
 	slli t0, a7, RISCV_LGPTR
-	add s0, s0, t0
-	REG_L s0, 0(s0)
+	add s1, s1, t0
+	REG_L s1, 0(s1)
 3:
-	jalr s0
+	jalr s1
 
 ret_from_syscall:
 	/* Set user a0 to kernel a0 */
-- 
2.17.1

[PATCH -next 5/7] riscv: stacktrace: Implement stacktrace for irq

[Chen Zhongjin]

After adding encoded fp onto stack to record pt_regs, now the
unwinder have ability to unwind frame through irq.

There is two steps to unwind irq frame and the interrupted frame:

1. When there is an encoded fp on stack, we can get the pt_regs
and unwind frame by (regs->epc) and (regs->s0).

2. To unwind the interrupted frame, there is two possibilities,
we can determine the situation by checking whether the value in
frame->ra position is a fp value.

If there is a fp in ra position:
We are inside a leaf frame and there is only fp on ra position.
Get fp from ra position and get next pc from pt_regs.
Else:
Just get fp and next pc from stack frame.

Stacktrace before this patch:

 Call Trace:
  ...
  [<ffffffff800aa692>] __flush_smp_call_function_queue+0xde/0x1fa
  [<ffffffff800ab404>] generic_smp_call_function_single_interrupt+0x22/0x2a
  [<ffffffff800077b2>] handle_IPI+0xaa/0x108
  [<ffffffff803f827e>] riscv_intc_irq+0x56/0x6e
  [<ffffffff808d94b6>] generic_handle_arch_irq+0x4c/0x76
  [<ffffffff80003ad0>] ret_from_exception+0x0/0xc

Stacktrace after this patch:

 Call Trace:
  ...
  [<ffffffff800aa6da>] __flush_smp_call_function_queue+0xde/0x1fa
  [<ffffffff800ab44c>] generic_smp_call_function_single_interrupt+0x22/0x2a
  [<ffffffff800077fa>] handle_IPI+0xaa/0x108
  [<ffffffff803f82c6>] riscv_intc_irq+0x56/0x6e
  [<ffffffff808d94fe>] generic_handle_arch_irq+0x4c/0x76
  [<ffffffff80003ad0>] ret_from_exception+0x0/0xc
+ [<ffffffff80003d52>] arch_cpu_idle+0x22/0x28
+ [<ffffffff808e23a8>] default_idle_call+0x44/0xee
+ [<ffffffff80056ece>] do_idle+0x116/0x126
+ [<ffffffff8005706e>] cpu_startup_entry+0x36/0x38
+ [<ffffffff808d99ae>] kernel_init+0x0/0x15a
+ [<ffffffff80a007a0>] arch_post_acpi_subsys_init+0x0/0x38
+ [<ffffffff80a0100c>] start_kernel+0x7c4/0x7f2

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/kernel/stacktrace.c | 45 ++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index e84e21868a3e..976dc298ab3b 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -16,29 +16,60 @@
 
 #ifdef CONFIG_FRAME_POINTER
 
+static struct pt_regs *decode_frame_pointer(unsigned long fp)
+{
+	if (!(fp & 0x1))
+		return NULL;
+
+	return (struct pt_regs *)(fp & ~0x1);
+}
+
 static int notrace unwind_next(struct unwind_state *state)
 {
 	unsigned long low, high, fp;
 	struct stackframe *frame;
+	struct pt_regs *regs;
 
-	fp = state->fp;
+	regs = decode_frame_pointer(state->fp);
 
 	/* Validate frame pointer */
-	low = state->sp + sizeof(struct stackframe);
+	if (regs) {
+		if user_mode(regs)
+			return -1;
+
+		fp = (unsigned long)regs;
+		low = state->sp;
+	} else {
+		fp = state->fp;
+		low = state->sp + sizeof(struct stackframe);
+	}
 	high = ALIGN(low, THREAD_SIZE);
 
 	if (fp < low || fp > high || fp & 0x7)
 		return -EINVAL;
 
-	/* Unwind stack frame */
 	frame = (struct stackframe *)fp - 1;
 	state->sp = fp;
 
-	if (state->regs && state->regs->epc == state->pc &&
-		fp & 0x7) {
-		state->fp = frame->ra;
-		state->pc = state->regs->ra;
+	if (regs) {
+	/* Unwind from irq to interrupted function */
+		state->fp = regs->s0;
+		state->pc = regs->epc;
+		state->regs = regs;
+	} else if (state->regs && state->regs->epc == state->pc) {
+	/* Unwind from interrupted function to caller*/
+		if (frame->ra < low || frame->ra > high) {
+		/* normal function */
+			state->fp = frame->fp;
+			state->pc = frame->ra;
+		} else {
+		/* leaf function */
+			state->fp = frame->ra;
+			state->pc = state->regs->ra;
+		}
+		state->regs = NULL;
 	} else {
+	/* Unwind from normal stack frame */
 		state->fp = frame->fp;
 		state->pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
 							(unsigned long *)fp - 1);
-- 
2.17.1

[PATCH -next 6/7] riscv: stacktrace: Fix unwinding on ftrace_regs_call

[Chen Zhongjin]

When unwinding on ftrace_regs_call, the traced function will be skipped
because ftrace_regs_caller doesn't save the fp and ra.

Save the encoded fp so that we can get the pt_regs so that we can unwind
from ftrace_regs_call to the traced function.

Also the pt_regs->status should be set as kernel mode.

Stacktrace before this patch:

 Call Trace:
  ...
  [<ffffffff0161a0e0>] handler_pre+0x30/0x4a [kprobe_unwind]
  [<ffffffff800bce92>] aggr_pre_handler+0x60/0x94
  [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
  [<ffffffff80008e7e>] ftrace_regs_call+0x8/0x10
  [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
  [<ffffffff8008a4e6>] do_init_module+0x56/0x210
 ...

Stacktrace after this patch:

 Call Trace:
  ...
  [<ffffffff016150e0>] handler_pre+0x30/0x4a [kprobe_unwind]
  [<ffffffff800bce96>] aggr_pre_handler+0x60/0x94
  [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
  [<ffffffff80008e82>] ftrace_regs_call+0x8/0x10
+ [<ffffffff01615000>] empty_call+0x0/0x1e [kprobe_unwind]
  [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
  [<ffffffff8008a4ea>] do_init_module+0x56/0x210
  ...

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/kernel/mcount-dyn.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index d171eca623b6..56a4014c392f 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -10,6 +10,8 @@
 #include <asm/asm-offsets.h>
 #include <asm-generic/export.h>
 #include <asm/ftrace.h>
+#include <asm/frame.h>
+#include <asm/csr.h>
 
 	.text
 
@@ -97,6 +99,11 @@
 	REG_S x29, PT_T4(sp)
 	REG_S x30, PT_T5(sp)
 	REG_S x31, PT_T6(sp)
+
+#ifdef CONFIG_FRAME_POINTER
+	li s0, SR_PP
+	REG_S s0, PT_STATUS(sp)
+#endif
 	.endm
 
 	.macro RESTORE_ALL
@@ -172,6 +179,7 @@ ENDPROC(ftrace_caller)
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 ENTRY(ftrace_regs_caller)
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 
 	addi	a0, ra, -FENTRY_RA_OFFSET
 	la	a1, function_trace_op
-- 
2.17.1

[PATCH -next 7/7] riscv: stacktrace: Fix unwinding on __kretporbe_trampoline

[Chen Zhongjin]

When unwinding on __kretprobe_trampoline, the caller of traced function
will be skipped because unwinder doesn't read the saved pt_regs.

Things going like this:

caller's caller		|       ...                 |<---+
caller			+---------------------------+    |
			| ra caller's caller        |    |
			| s0 of caller's caller     |    |
			|       ...                 |    |
probed func returned	+---------------------------+    |
__kretprobe_trampoline	| pt_regs:                  |    |
			| epc caller                |    |
                        | ra  __kretprobe_trampoline|    |
			|       ...                 |    |
                        | s0 of caller              | {ra, fp}
			|       ...                 |

Since from caller to __kretprobe_trampoline, the {ra, fp} are not
changed, unwinder will go directly to caller's caller.

Now we can have an ENCODED_FRAME_POINTER on stack and read the pt_regs,
kretporbe will set the epc to correct_ret_addr so that we can unwind
to the correct caller.

Stacktrace before this patch:

 Call Trace:
  ...
  [<ffffffff800d5d48>] __kretprobe_trampoline_handler+0xc2/0x13e
  [<ffffffff808b766c>] trampoline_probe_handler+0x30/0x46
  [<ffffffff800070de>] __kretprobe_trampoline+0x52/0x92
  [<ffffffff0163809c>] kprobe_init+0x9c/0x1000 [kprobe_unwind]
  [<ffffffff800027c8>] do_one_initcall+0x4c/0x1f2
  ...

Stacktrace after this patch:

 Call Trace:
  ...
  [<ffffffff800d5d48>] __kretprobe_trampoline_handler+0xc2/0x13e
  [<ffffffff808b766c>] trampoline_probe_handler+0x30/0x46
  [<ffffffff800070de>] __kretprobe_trampoline+0x52/0x92
+ [<ffffffff01633076>] the_caller+0x2c/0x38 [kprobe_unwind]
  [<ffffffff0163809c>] kprobe_init+0x9c/0x1000 [kprobe_unwind]
  [<ffffffff800027c8>] do_one_initcall+0x4c/0x1f2
  ...

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/riscv/include/asm/stacktrace.h           | 4 ++++
 arch/riscv/kernel/probes/kprobes_trampoline.S | 8 ++++++++
 arch/riscv/kernel/stacktrace.c                | 5 +++++
 3 files changed, 17 insertions(+)

diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index a39e4ef1dbd5..506c7c38b6cb 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -16,6 +16,10 @@ struct unwind_state {
 	unsigned long sp;
 	unsigned long pc;
 	struct pt_regs *regs;
+#ifdef CONFIG_KRETPROBES
+	struct llist_node *kr_cur;
+	struct task_struct *task;
+#endif
 };
 
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
diff --git a/arch/riscv/kernel/probes/kprobes_trampoline.S b/arch/riscv/kernel/probes/kprobes_trampoline.S
index 7bdb09ded39b..3c0677a714a6 100644
--- a/arch/riscv/kernel/probes/kprobes_trampoline.S
+++ b/arch/riscv/kernel/probes/kprobes_trampoline.S
@@ -6,6 +6,8 @@
 
 #include <asm/asm.h>
 #include <asm/asm-offsets.h>
+#include <asm/frame.h>
+#include <asm/csr.h>
 
 	.text
 	.altmacro
@@ -79,6 +81,12 @@ ENTRY(__kretprobe_trampoline)
 	addi sp, sp, -(PT_SIZE_ON_STACK)
 	save_all_base_regs
 
+#ifdef CONFIG_FRAME_POINTER
+	li s0, SR_PP
+	REG_S s0, PT_STATUS(sp)
+	ENCODE_FRAME_POINTER
+#endif
+
 	move a0, sp /* pt_regs */
 
 	call trampoline_probe_handler
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 976dc298ab3b..53edc685ca18 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -11,6 +11,7 @@
 #include <linux/sched/task_stack.h>
 #include <linux/stacktrace.h>
 #include <linux/ftrace.h>
+#include <linux/kprobes.h>
 
 #include <asm/stacktrace.h>
 
@@ -123,6 +124,10 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 		state.sp = task->thread.sp;
 		state.pc = task->thread.ra;
 	}
+#ifdef CONFIG_KRETPROBES
+	state.kr_cur = NULL;
+	state.task = task;
+#endif
 
 	unwind(&state, consume_entry, cookie);
 }
-- 
2.17.1

Re: [PATCH -next 4/7] riscv: syscall: Don't clobber s0 when syscall

[Guo Ren]

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> syscall uses s0 to load address of sys_call_table.
>
> Since now we uses s0 to save pt_regs for unwinding, clobber
> s0 can make unwinder treat s0 as pt_regs address. Use s1 for
> this job.
This patch does not make sense. Why couldn't we use s1 for pt_regs?

No need to modify the entry.S here.

>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/riscv/kernel/entry.S | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index ecb15c7430b4..a3b14a649782 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -205,33 +205,33 @@ handle_syscall:
>  check_syscall_nr:
>         /* Check to make sure we don't jump to a bogus syscall number. */
>         li t0, __NR_syscalls
> -       la s0, sys_ni_syscall
> +       la s1, sys_ni_syscall
>         /*
>          * Syscall number held in a7.
>          * If syscall number is above allowed value, redirect to ni_syscall.
>          */
>         bgeu a7, t0, 3f
>  #ifdef CONFIG_COMPAT
> -       REG_L s0, PT_STATUS(sp)
> -       srli s0, s0, SR_UXL_SHIFT
> -       andi s0, s0, (SR_UXL >> SR_UXL_SHIFT)
> +       REG_L s1, PT_STATUS(sp)
> +       srli s1, s1, SR_UXL_SHIFT
> +       andi s1, s1, (SR_UXL >> SR_UXL_SHIFT)
>         li t0, (SR_UXL_32 >> SR_UXL_SHIFT)
> -       sub t0, s0, t0
> +       sub t0, s1, t0
>         bnez t0, 1f
>
>         /* Call compat_syscall */
> -       la s0, compat_sys_call_table
> +       la s1, compat_sys_call_table
>         j 2f
>  1:
>  #endif
>         /* Call syscall */
> -       la s0, sys_call_table
> +       la s1, sys_call_table
>  2:
>         slli t0, a7, RISCV_LGPTR
> -       add s0, s0, t0
> -       REG_L s0, 0(s0)
> +       add s1, s1, t0
> +       REG_L s1, 0(s1)
>  3:
> -       jalr s0
> +       jalr s1
>
>  ret_from_syscall:
>         /* Set user a0 to kernel a0 */
> --
> 2.17.1
>


-- 
Best Regards
 Guo Ren

Re: [PATCH -next 6/7] riscv: stacktrace: Fix unwinding on ftrace_regs_call

[Guo Ren]

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> When unwinding on ftrace_regs_call, the traced function will be skipped
> because ftrace_regs_caller doesn't save the fp and ra.
>
> Save the encoded fp so that we can get the pt_regs so that we can unwind
> from ftrace_regs_call to the traced function.
>
> Also the pt_regs->status should be set as kernel mode.
>
> Stacktrace before this patch:
>
>  Call Trace:
>   ...
>   [<ffffffff0161a0e0>] handler_pre+0x30/0x4a [kprobe_unwind]
>   [<ffffffff800bce92>] aggr_pre_handler+0x60/0x94
>   [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
>   [<ffffffff80008e7e>] ftrace_regs_call+0x8/0x10
>   [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
>   [<ffffffff8008a4e6>] do_init_module+0x56/0x210
>  ...
>
> Stacktrace after this patch:
>
>  Call Trace:
>   ...
>   [<ffffffff016150e0>] handler_pre+0x30/0x4a [kprobe_unwind]
>   [<ffffffff800bce96>] aggr_pre_handler+0x60/0x94
>   [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
>   [<ffffffff80008e82>] ftrace_regs_call+0x8/0x10
> + [<ffffffff01615000>] empty_call+0x0/0x1e [kprobe_unwind]
>   [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
>   [<ffffffff8008a4ea>] do_init_module+0x56/0x210
>   ...
>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/riscv/kernel/mcount-dyn.S | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> index d171eca623b6..56a4014c392f 100644
> --- a/arch/riscv/kernel/mcount-dyn.S
> +++ b/arch/riscv/kernel/mcount-dyn.S
> @@ -10,6 +10,8 @@
>  #include <asm/asm-offsets.h>
>  #include <asm-generic/export.h>
>  #include <asm/ftrace.h>
> +#include <asm/frame.h>
> +#include <asm/csr.h>
>
>         .text
>
> @@ -97,6 +99,11 @@
>         REG_S x29, PT_T4(sp)
>         REG_S x30, PT_T5(sp)
>         REG_S x31, PT_T6(sp)
> +
> +#ifdef CONFIG_FRAME_POINTER
> +       li s0, SR_PP
> +       REG_S s0, PT_STATUS(sp)
Change another register.

> +#endif
>         .endm
>
>         .macro RESTORE_ALL
> @@ -172,6 +179,7 @@ ENDPROC(ftrace_caller)
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  ENTRY(ftrace_regs_caller)
>         SAVE_ALL
> +       ENCODE_FRAME_POINTER
Maybe the patch should merge with others, so separated make it hard to review.

>
>         addi    a0, ra, -FENTRY_RA_OFFSET
>         la      a1, function_trace_op
> --
> 2.17.1
>


--
Best Regards
 Guo Ren

Re: [PATCH -next 3/7] riscv: stacktrace: Save pt_regs in encoded fp on irq entry

[Guo Ren]

Reviewed-by: Guo Ren <guoren@kernel.org>

ENCODE fp with BIT[0] is okay.

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> To support stack unwinding at irq entry, the position of pt_regs saved
> on stack is nessesary. Because for some functions, compiler only push
> s0/fp on stack without ra.
>
> As the situation described in
> commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")
>
> When irq happens there, the the function frame looks like:
>
> prev function   |       ...       |
>                 |                 |
> normal function +-----------------+
>                 | ra to prev      |
>                 | s0 of prev      |
>                 |       ...       |<-+
> leaf function   +-----------------+  |
>                 | s0 of normal    |  |
>                 | empty slot      |  |
> irq pt_regs     +-----------------+  |
>                 | epc (ra to leaf)|  |
>                 | ra  (ra to norm)|  |
>                 | ...             |  |
>                 | s0 of leaf      |--+
>                 | ...             |
>                 +-----------------+
>
> If the position of register in pt_regs is {epc, s0}, we can easily
> unwind from irq frame to leaf function, as normal functions do.
>
> However when unwinding from unwinding from leaf to normal, beacause
> (ra to norm) is saved in pt_regs, but not stackframe of leaf, we
> have to get pt_regs for that.
>
> To get pt_regs position on stack, we can save the encoded *pt_regs
> in s0, as x86 architecture did. Then we can get s0, epc and ra
> easily.
>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/riscv/include/asm/frame.h | 45 ++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/entry.S      |  3 +++
>  2 files changed, 48 insertions(+)
>  create mode 100644 arch/riscv/include/asm/frame.h
>
> diff --git a/arch/riscv/include/asm/frame.h b/arch/riscv/include/asm/frame.h
> new file mode 100644
> index 000000000000..2a1f45cf3a4e
> --- /dev/null
> +++ b/arch/riscv/include/asm/frame.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_FRAME_H
> +#define _ASM_RISCV_FRAME_H
> +
> +#include <asm/asm.h>
> +
> +#ifdef CONFIG_FRAME_POINTER
> +
> +#ifdef __ASSEMBLY__
> +
> +/*
> + * This is a sneaky trick to help the unwinder find pt_regs on the stack.  The
> + * frame pointer is replaced with an encoded pointer to pt_regs.  The encoding
> + * is just setting the LSB, which makes it an invalid stack address and is also
> + * a signal to the unwinder that it's a pt_regs pointer in disguise.
> + *
> + * This macro must be used when sp point to pt_regs
> + */
> +.macro ENCODE_FRAME_POINTER
> +       add s0, sp, 0x1
> +.endm
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define ENCODE_FRAME_POINTER                   \
> +       "add s0, sp, 0x1\n\t"
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#else /* !CONFIG_FRAME_POINTER */
> +
> +#ifdef __ASSEMBLY__
> +
> +.macro ENCODE_FRAME_POINTER ptregs_offset=0
> +.endm
> +
> +#else /* !__ASSEMBLY */
> +
> +#define ENCODE_FRAME_POINTER
> +
> +#endif /* !__ASSEMBLY */
> +
> +#endif /* CONFIG_FRAME_POINTER */
> +
> +#endif /* _ASM_RISCV_FRAME_H */
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index b9eda3fcbd6d..ecb15c7430b4 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -13,6 +13,7 @@
>  #include <asm/thread_info.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/errata_list.h>
> +#include <asm/frame.h>
>
>  #if !IS_ENABLED(CONFIG_PREEMPTION)
>  .set resume_kernel, restore_all
> @@ -95,6 +96,8 @@ _save_context:
>         REG_S s4, PT_CAUSE(sp)
>         REG_S s5, PT_TP(sp)
>
> +       ENCODE_FRAME_POINTER
> +
>         /*
>          * Set the scratch register to 0, so that if a recursive exception
>          * occurs, the exception vector knows it came from the kernel
> --
> 2.17.1
>


-- 
Best Regards
 Guo Ren

Re: [PATCH -next 4/7] riscv: syscall: Don't clobber s0 when syscall

[Guo Ren]

On Wed, Sep 21, 2022 at 9:30 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
> >
> > syscall uses s0 to load address of sys_call_table.
> >
> > Since now we uses s0 to save pt_regs for unwinding, clobber
> > s0 can make unwinder treat s0 as pt_regs address. Use s1 for
> > this job.
> This patch does not make sense. Why couldn't we use s1 for pt_regs?
Seems s0->fp can't be replaced. I take back the question.

>
> No need to modify the entry.S here.
>
> >
> > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> > ---
> >  arch/riscv/kernel/entry.S | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index ecb15c7430b4..a3b14a649782 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -205,33 +205,33 @@ handle_syscall:
> >  check_syscall_nr:
> >         /* Check to make sure we don't jump to a bogus syscall number. */
> >         li t0, __NR_syscalls
> > -       la s0, sys_ni_syscall
> > +       la s1, sys_ni_syscall
> >         /*
> >          * Syscall number held in a7.
> >          * If syscall number is above allowed value, redirect to ni_syscall.
> >          */
> >         bgeu a7, t0, 3f
> >  #ifdef CONFIG_COMPAT
> > -       REG_L s0, PT_STATUS(sp)
> > -       srli s0, s0, SR_UXL_SHIFT
> > -       andi s0, s0, (SR_UXL >> SR_UXL_SHIFT)
> > +       REG_L s1, PT_STATUS(sp)
> > +       srli s1, s1, SR_UXL_SHIFT
> > +       andi s1, s1, (SR_UXL >> SR_UXL_SHIFT)
> >         li t0, (SR_UXL_32 >> SR_UXL_SHIFT)
> > -       sub t0, s0, t0
> > +       sub t0, s1, t0
> >         bnez t0, 1f
> >
> >         /* Call compat_syscall */
> > -       la s0, compat_sys_call_table
> > +       la s1, compat_sys_call_table
> >         j 2f
> >  1:
> >  #endif
> >         /* Call syscall */
> > -       la s0, sys_call_table
> > +       la s1, sys_call_table
> >  2:
> >         slli t0, a7, RISCV_LGPTR
> > -       add s0, s0, t0
> > -       REG_L s0, 0(s0)
> > +       add s1, s1, t0
> > +       REG_L s1, 0(s1)
> >  3:
> > -       jalr s0
> > +       jalr s1
> >
> >  ret_from_syscall:
> >         /* Set user a0 to kernel a0 */
> > --
> > 2.17.1
> >
>
>
> --
> Best Regards
>  Guo Ren



-- 
Best Regards
 Guo Ren

Re: [PATCH -next 0/7] riscv: Improvments for stacktrace

[Guo Ren]

Some modifications are related to the patch series [1] [2], please take a look.

[1] https://lore.kernel.org/linux-riscv/20220918155246.1203293-1-guoren@kernel.org/
[2] https://lore.kernel.org/linux-riscv/20220916103817.9490-1-guoren@kernel.org/

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> Currently, the stacktrace with FRAME_POINTER on riscv has some problem:
>
> 1. stacktrace will stop at irq so it can't get the stack frames before
> irq entry.
> 2. stacktrace can't unwind all the real stack frames when there is
> k{ret}probes or ftrace.
>
> These are mainly becase when there is a pt_regs on stack, we can't unwind
> the stack frame as normal function.
>
> Some architectures (e.g. arm64) create a extra stackframe inside pt_regs.
> However this doesn't work for riscv because the ra is not ensured to be
> pushed to stack. As explained in:
> commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")
>
> So, I choosed the method of x86 that, if there is a pt_regs on stack,
> we encoded the frame pointer and save it. When unwinding stack frame,
> we can get pt_regs and registers required for unwinding stacks.
>
> In addition, the patch set contains some refactoring of stacktrace.c to
> keep the stacktrace code on riscv consistent with other architectures.
>
> Chen Zhongjin (7):
>   riscv: stacktrace: Replace walk_stackframe with arch_stack_walk
>   riscv: stacktrace: Introduce unwind functions
>   riscv: stacktrace: Save pt_regs in encoded fp on irq entry
>   riscv: syscall: Don't clobber s0 when syscall
>   riscv: stacktrace: Implement stacktrace for irq
>   riscv: stacktrace: Fix unwinding on ftrace_regs_call
>   riscv: stacktrace: Fix unwinding on __kretporbe_trampoline
>
>  arch/riscv/include/asm/frame.h                |  45 +++++
>  arch/riscv/include/asm/stacktrace.h           |  13 +-
>  arch/riscv/kernel/entry.S                     |  23 +--
>  arch/riscv/kernel/mcount-dyn.S                |   8 +
>  arch/riscv/kernel/perf_callchain.c            |   2 +-
>  arch/riscv/kernel/probes/kprobes_trampoline.S |   8 +
>  arch/riscv/kernel/stacktrace.c                | 155 ++++++++++++------
>  7 files changed, 195 insertions(+), 59 deletions(-)
>  create mode 100644 arch/riscv/include/asm/frame.h
>
> --
> 2.17.1
>


--
Best Regards
 Guo Ren

Re: [PATCH -next 6/7] riscv: stacktrace: Fix unwinding on ftrace_regs_call

[Chen Zhongjin]

Hi,

On 2022/9/21 9:43, Guo Ren wrote:
> On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>> When unwinding on ftrace_regs_call, the traced function will be skipped
>> because ftrace_regs_caller doesn't save the fp and ra.
>>
>> Save the encoded fp so that we can get the pt_regs so that we can unwind
>> from ftrace_regs_call to the traced function.
>>
>> Also the pt_regs->status should be set as kernel mode.
>>
>> Stacktrace before this patch:
>>
>>   Call Trace:
>>    ...
>>    [<ffffffff0161a0e0>] handler_pre+0x30/0x4a [kprobe_unwind]
>>    [<ffffffff800bce92>] aggr_pre_handler+0x60/0x94
>>    [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
>>    [<ffffffff80008e7e>] ftrace_regs_call+0x8/0x10
>>    [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
>>    [<ffffffff8008a4e6>] do_init_module+0x56/0x210
>>   ...
>>
>> Stacktrace after this patch:
>>
>>   Call Trace:
>>    ...
>>    [<ffffffff016150e0>] handler_pre+0x30/0x4a [kprobe_unwind]
>>    [<ffffffff800bce96>] aggr_pre_handler+0x60/0x94
>>    [<ffffffff80006df0>] kprobe_ftrace_handler+0x13e/0x188
>>    [<ffffffff80008e82>] ftrace_regs_call+0x8/0x10
>> + [<ffffffff01615000>] empty_call+0x0/0x1e [kprobe_unwind]
>>    [<ffffffff80002540>] do_one_initcall+0x4c/0x1f2
>>    [<ffffffff8008a4ea>] do_init_module+0x56/0x210
>>    ...
>>
>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>> ---
>>   arch/riscv/kernel/mcount-dyn.S | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
>> index d171eca623b6..56a4014c392f 100644
>> --- a/arch/riscv/kernel/mcount-dyn.S
>> +++ b/arch/riscv/kernel/mcount-dyn.S
>> @@ -10,6 +10,8 @@
>>   #include <asm/asm-offsets.h>
>>   #include <asm-generic/export.h>
>>   #include <asm/ftrace.h>
>> +#include <asm/frame.h>
>> +#include <asm/csr.h>
>>
>>          .text
>>
>> @@ -97,6 +99,11 @@
>>          REG_S x29, PT_T4(sp)
>>          REG_S x30, PT_T5(sp)
>>          REG_S x31, PT_T6(sp)
>> +
>> +#ifdef CONFIG_FRAME_POINTER
>> +       li s0, SR_PP
>> +       REG_S s0, PT_STATUS(sp)
> Change another register.
>
>> +#endif
>>          .endm
>>
>>          .macro RESTORE_ALL
>> @@ -172,6 +179,7 @@ ENDPROC(ftrace_caller)
>>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>   ENTRY(ftrace_regs_caller)
>>          SAVE_ALL
>> +       ENCODE_FRAME_POINTER
> Maybe the patch should merge with others, so separated make it hard to review.

Thanks for review!

Happy to see you have approved the ENCODE FP part.

The patches is separated just because I wanted to paste the call trace 
result in commit message.

I'll merge the ENCODE_FRAME_POINTER patches all together and do this in 
the cover.


Also noticed another email for your ongoing patches. I'll review them 
and reply later.


Best,

Chen

Re: [PATCH -next 0/7] riscv: Improvments for stacktrace

[Chen Zhongjin]

Hi,

On 2022/9/21 10:30, Guo Ren wrote:
> Some modifications are related to the patch series [1] [2], please take a look.
>
> [1] https://lore.kernel.org/linux-riscv/20220918155246.1203293-1-guoren@kernel.org/
> [2] https://lore.kernel.org/linux-riscv/20220916103817.9490-1-guoren@kernel.org/

I have tested my patch on your branches.

For the ftrace one it works properly and passed FTRACE_STARTUP_TEST.

For the entry one I proposed some advice in another reply, with that my 
patch also works well.


I'll post v2 patch which is totally dependent to yours. And after your 
patch applied, the stacktrace

may need to be updated again, for example, print the type of current 
unwinding stack.


Best,

Chen

> On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>> Currently, the stacktrace with FRAME_POINTER on riscv has some problem:
>>
>> 1. stacktrace will stop at irq so it can't get the stack frames before
>> irq entry.
>> 2. stacktrace can't unwind all the real stack frames when there is
>> k{ret}probes or ftrace.
>>
>> These are mainly becase when there is a pt_regs on stack, we can't unwind
>> the stack frame as normal function.
>>
>> Some architectures (e.g. arm64) create a extra stackframe inside pt_regs.
>> However this doesn't work for riscv because the ra is not ensured to be
>> pushed to stack. As explained in:
>> commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")
>>
>> So, I choosed the method of x86 that, if there is a pt_regs on stack,
>> we encoded the frame pointer and save it. When unwinding stack frame,
>> we can get pt_regs and registers required for unwinding stacks.
>>
>> In addition, the patch set contains some refactoring of stacktrace.c to
>> keep the stacktrace code on riscv consistent with other architectures.
>>
>> Chen Zhongjin (7):
>>    riscv: stacktrace: Replace walk_stackframe with arch_stack_walk
>>    riscv: stacktrace: Introduce unwind functions
>>    riscv: stacktrace: Save pt_regs in encoded fp on irq entry
>>    riscv: syscall: Don't clobber s0 when syscall
>>    riscv: stacktrace: Implement stacktrace for irq
>>    riscv: stacktrace: Fix unwinding on ftrace_regs_call
>>    riscv: stacktrace: Fix unwinding on __kretporbe_trampoline
>>
>>   arch/riscv/include/asm/frame.h                |  45 +++++
>>   arch/riscv/include/asm/stacktrace.h           |  13 +-
>>   arch/riscv/kernel/entry.S                     |  23 +--
>>   arch/riscv/kernel/mcount-dyn.S                |   8 +
>>   arch/riscv/kernel/perf_callchain.c            |   2 +-
>>   arch/riscv/kernel/probes/kprobes_trampoline.S |   8 +
>>   arch/riscv/kernel/stacktrace.c                | 155 ++++++++++++------
>>   7 files changed, 195 insertions(+), 59 deletions(-)
>>   create mode 100644 arch/riscv/include/asm/frame.h
>>
>> --
>> 2.17.1
>>
>
> --
> Best Regards
>   Guo Ren