arm64: ftrace: fix incorrect output from stack tracer

Function graph tracer modifies a return address (LR) in a stack frame by calling ftrace_prepare_return() in a traced function's function prologue. The current code does this modification before preserving an original address at ftrace_push_return_trace() and there is always a small window of inconsistency when an interrupt occurs.

This doesn't matter, as far as an interrupt stack is introduced, because stack tracer won't be invoked in an interrupt context. But it would be better to proactively minimize such a window by moving the LR modification after ftrace_push_return_trace().

The change here doesn't make any difference in this patch, but is a preparation for later fixing a problem where stacktrace functions, unwind_frame() and walk_stackframe(), may return useless call stacks under function graph tracer.

Quoted 4 lines · AKASHI Takahiro

The change here doesn't make any difference in this patch, but is a preparation for later fixing a problem where stacktrace functions, unwind_frame() and walk_stackframe(), may return useless call stacks under function graph tracer.

I'm aligned with the argument. The case cannot be handled correctly without ret_stack[] of struct task_struct.

Best Regards
Jungseok Lee

Quoted 6 lines · Jungseok Lee

Quoted 4 lines · AKASHI Takahiro

The change here doesn't make any difference in this patch, but is a preparation for later fixing a problem where stacktrace functions, unwind_frame() and walk_stackframe(), may return useless call stacks under function graph tracer.

Quoted 2 lines

I'm aligned with the argument. The case cannot be handled correctly without ret_stack[] of struct task_struct.

Thanks. I will add some description about why we need 'tsk' in a commit message.

Quoted 7 lines · AKASHI Takahiro

Quoted 6 lines · Jungseok Lee

Quoted 4 lines · AKASHI Takahiro

The change here doesn't make any difference in this patch, but is a preparation for later fixing a problem where stacktrace functions, unwind_frame() and walk_stackframe(), may return useless call stacks under function graph tracer.

Quoted 5 lines

Quoted 2 lines

I'm aligned with the argument. The case cannot be handled correctly without ret_stack[] of struct task_struct.

Thanks. I will add some description about why we need 'tsk' in a commit message.

Ok, so are you planning to repost this series?

Quoted 11 lines · Will Deacon

Quoted 10 lines · AKASHI Takahiro

Quoted 8 lines · Jungseok Lee

Quoted 4 lines · AKASHI Takahiro

The change here doesn't make any difference in this patch, but is a preparation for later fixing a problem where stacktrace functions, unwind_frame() and walk_stackframe(), may return useless call stacks under function graph tracer.

I'm aligned with the argument. The case cannot be handled correctly without ret_stack[] of struct task_struct.

Quoted 4 lines

Quoted 2 lines

Thanks. I will add some description about why we need 'tsk' in a commit message.

Ok, so are you planning to repost this series?

I think this function_graph changes could be factored out from this series. It would be helpful for maintainers and reviewers to take a look at them.

Best Regards
Jungseok Lee

Function graph tracer modifies a return address (LR) in a stack frame to hook a function return. This will result in many useless entries (return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)

For example, in case of a),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo 1 > /proc/sys/kernel/stack_trace_enabled
 $ cat /sys/kernel/debug/tracing/stack_trace
       Depth    Size   Location    (54 entries)
       -----    ----   --------
 0)     4504      16   gic_raise_softirq+0x28/0x150
 1)     4488      80   smp_cross_call+0x38/0xb8
 2)     4408      48   return_to_handler+0x0/0x40
 3)     4360      32   return_to_handler+0x0/0x40
 ...

In case of b),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ perf record -e mem:XXX:x -ag -- sleep 10
 $ perf report
                 ...
                 |          |          |--0.22%-- 0x550f8
                 |          |          |          0x10888
                 |          |          |          el0_svc_naked
                 |          |          |          sys_openat
                 |          |          |          return_to_handler
                 |          |          |          return_to_handler
                 ...

In case of c),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo c > /proc/sysrq-trigger
 ...
 Call trace:
 [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 ...

This patch replaces such entries with real addresses preserved in current->ret_stack[] at unwind_frame(). This way, we can cover all the cases.

Signed-off-by AKASHI Takahiro <[email protected]>

---

arch/arm64/include/asm/ftrace.h     |    2 ++
arch/arm64/include/asm/stacktrace.h |    3 +++
arch/arm64/kernel/perf_callchain.c  |    3 +++
arch/arm64/kernel/process.c         |    3 +++
arch/arm64/kernel/return_address.c  |    3 +++
arch/arm64/kernel/stacktrace.c      |   17 +++++++++++++++++
arch/arm64/kernel/time.c            |    3 +++
arch/arm64/kernel/traps.c           |    3 +++
8 files changed, 37 insertions(+)

arch/arm64/include/asm/ftrace.h +2 -0

index c5534fa..3c60f37 100644
@@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*

arch/arm64/include/asm/stacktrace.h +0 -0

index 6fb61c5..801a16db 100644

arch/arm64/include/asm/stacktrace.h +0 -0

 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);

arch/arm64/kernel/perf_callchain.c +3 -0

index 797220d..ff46654 100644
@@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, callchain_trace, entry);
 }

arch/arm64/kernel/process.c +3 -0

index 98bf546..88d742b 100644
@@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
 	frame.fp = thread_saved_fp(p);
 	frame.sp = thread_saved_sp(p);
 	frame.pc = thread_saved_pc(p);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = p->curr_ret_stack;
+#endif
 	stack_page = (unsigned long)task_stack_page(p);
 	do {
 		if (frame.sp < stack_page ||

arch/arm64/kernel/return_address.c +3 -0

index 07b37ac..1718706 100644
@@ -43,6 +43,9 @@ void *return_address(unsigned int level)
 	frame.fp = (unsigned long)__builtin_frame_address(0);
 	frame.sp = current_stack_pointer;
 	frame.pc = (unsigned long)return_address; /* dummy */
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, save_return_addr, &data);
 

arch/arm64/kernel/stacktrace.c +3 -0

index 9c7acf8..0a39049 100644
@@ -17,6 +17,7 @@
 */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp);
 	frame->pc = *(unsigned long *)(fp + 8);
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (tsk && tsk->ret_stack &&

arch/arm64/kernel/stacktrace.c +4 -0

+
 	/*
 	 * Check whether we are going to walk through from interrupt stack
 	 * to task stack.

@@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)save_stack_trace_tsk;
 	}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = tsk->curr_ret_stack;
+#endif
 
 	walk_stackframe(tsk, &frame, save_trace, &data);
 	if (trace->nr_entries < trace->max_entries)

arch/arm64/kernel/time.c +0 -0

index 6e5c521..5977969 100644

On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:

Hi Akashi,

Quoted 42 lines

Function graph tracer modifies a return address (LR) in a stack frame to hook a function return. This will result in many useless entries (return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)

For example, in case of a),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ echo 1 > /proc/sys/kernel/stack_trace_enabled
  $ cat /sys/kernel/debug/tracing/stack_trace
        Depth    Size   Location    (54 entries)
        -----    ----   --------
  0)     4504      16   gic_raise_softirq+0x28/0x150
  1)     4488      80   smp_cross_call+0x38/0xb8
  2)     4408      48   return_to_handler+0x0/0x40
  3)     4360      32   return_to_handler+0x0/0x40
  ...

In case of b),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ perf record -e mem:XXX:x -ag -- sleep 10
  $ perf report
                  ...
                  |          |          |--0.22%-- 0x550f8
                  |          |          |          0x10888
                  |          |          |          el0_svc_naked
                  |          |          |          sys_openat
                  |          |          |          return_to_handler
                  |          |          |          return_to_handler
                  ...

In case of c),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ echo c > /proc/sysrq-trigger
  ...
  Call trace:
  [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
  [<ffffffc000092250>] return_to_handler+0x0/0x40
  [<ffffffc000092250>] return_to_handler+0x0/0x40
  ...

Quoted 3 lines

This patch replaces such entries with real addresses preserved in current->ret_stack[] at unwind_frame(). This way, we can cover all the cases.

I've observed a strange behavior when playing with case c). Call trace is as follows when function_graph is not used.

Call trace:
  [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30          <- (1)
  [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c            <- (2)
  [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
  [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
  [<fffffe00001f2638>] __vfs_write+0x44/0x104                <- (3)
  [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
  [<fffffe00001f3730>] SyS_write+0x50/0xb0
  [<fffffe00000939ec>] el0_svc_naked+0x20/0x28               <- (4)

When function_graph is set, some entries, such as do_mem_abort, are added between (1) and (2). In addition, entries from (3) to (4) are not printed out. As tracking down elements of ret_stack[], I realize dump_backtrace() has been terminated before reaching to ret_stack[0]. Have you seen this kind of behavior? I believe push & pop operations work correctly.

Please correct me if I'm wrong.

Quoted 30 lines

Signed-off-by AKASHI Takahiro <[email protected]>

---

arch/arm64/include/asm/ftrace.h     |    2 ++
arch/arm64/include/asm/stacktrace.h |    3 +++
arch/arm64/kernel/perf_callchain.c  |    3 +++
arch/arm64/kernel/process.c         |    3 +++
arch/arm64/kernel/return_address.c  |    3 +++
arch/arm64/kernel/stacktrace.c      |   17 +++++++++++++++++
arch/arm64/kernel/time.c            |    3 +++
arch/arm64/kernel/traps.c           |    3 +++
8 files changed, 37 insertions(+)

arch/arm64/include/asm/ftrace.h +2 -0

index c5534fa..3c60f37 100644
@@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*

arch/arm64/include/asm/stacktrace.h +0 -0

index 6fb61c5..801a16db 100644

arch/arm64/include/asm/stacktrace.h struct stackframe { · line 28

Quoted 7 lines

arch/arm64/include/asm/stacktrace.h +3 -0

@@ -22,6 +22,9 @@ struct stackframe {
 	unsigned long fp;
 	unsigned long sp;
 	unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int graph;
+#endif
 };

How about using int instead of unsigned int to align with cure_ret_stack of struct task_struct?

Quoted 59 lines

arch/arm64/include/asm/stacktrace.h +0 -0

 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);

arch/arm64/kernel/perf_callchain.c +3 -0

index 797220d..ff46654 100644
@@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, callchain_trace, entry);
 }

arch/arm64/kernel/process.c +3 -0

index 98bf546..88d742b 100644
@@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
 	frame.fp = thread_saved_fp(p);
 	frame.sp = thread_saved_sp(p);
 	frame.pc = thread_saved_pc(p);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = p->curr_ret_stack;
+#endif
 	stack_page = (unsigned long)task_stack_page(p);
 	do {
 		if (frame.sp < stack_page ||

arch/arm64/kernel/return_address.c +3 -0

index 07b37ac..1718706 100644
@@ -43,6 +43,9 @@ void *return_address(unsigned int level)
 	frame.fp = (unsigned long)__builtin_frame_address(0);
 	frame.sp = current_stack_pointer;
 	frame.pc = (unsigned long)return_address; /* dummy */
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, save_return_addr, &data);
 

arch/arm64/kernel/stacktrace.c +1 -0

index 9c7acf8..0a39049 100644
@@ -17,6 +17,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp);
 	frame->pc = *(unsigned long *)(fp + 8);
 

arch/arm64/kernel/stacktrace.c int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) · line 81

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +12 -0

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (tsk && tsk->ret_stack &&
+			(frame->pc == (unsigned long)return_to_handler)) {
+		/*
+		 * This is a case where function graph tracer has
+		 * modified a return address (LR) in a stack frame
+		 * to hook a function return.
+		 * So replace it to an original value.
+		 */
+		frame->pc = tsk->ret_stack[frame->graph--].ret;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

There is an index check of ret_stack[] in case of x86 [1]. Even though graph is unsigned int, I think we need to check the value of frame->graph before accessing ret_stack[].

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +4 -0

+
 	/*
 	 * Check whether we are going to walk through from interrupt stack
 	 * to task stack.

@@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)save_stack_trace_tsk;
 	}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = tsk->curr_ret_stack;
+#endif
 

arch/arm64/kernel/time.c unsigned long profile_pc(struct pt_regs *regs) · line 57

Quoted 13 lines

arch/arm64/kernel/stacktrace.c +0 -0

 	walk_stackframe(tsk, &frame, save_trace, &data);
 	if (trace->nr_entries < trace->max_entries)

arch/arm64/kernel/time.c +3 -0

index 6e5c521..5977969 100644
@@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = -1; /* no task info */
+#endif

graph is unsigned int type. Is this intentional?

Best Regards
Jungseok Lee

[1] arch/x86/kernel/dumpstack.c

Quoted 69 lines · Jungseok Lee

On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:

Hi Akashi,

Quoted 42 lines

Function graph tracer modifies a return address (LR) in a stack frame to hook a function return. This will result in many useless entries (return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)

For example, in case of a),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo 1 > /proc/sys/kernel/stack_trace_enabled
 $ cat /sys/kernel/debug/tracing/stack_trace
       Depth    Size   Location    (54 entries)
       -----    ----   --------
 0)     4504      16   gic_raise_softirq+0x28/0x150
 1)     4488      80   smp_cross_call+0x38/0xb8
 2)     4408      48   return_to_handler+0x0/0x40
 3)     4360      32   return_to_handler+0x0/0x40
 ...

In case of b),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ perf record -e mem:XXX:x -ag -- sleep 10
 $ perf report
                 ...
                 |          |          |--0.22%-- 0x550f8
                 |          |          |          0x10888
                 |          |          |          el0_svc_naked
                 |          |          |          sys_openat
                 |          |          |          return_to_handler
                 |          |          |          return_to_handler
                 ...

In case of c),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo c > /proc/sysrq-trigger
 ...
 Call trace:
 [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 ...

Quoted 3 lines

This patch replaces such entries with real addresses preserved in current->ret_stack[] at unwind_frame(). This way, we can cover all the cases.

I've observed a strange behavior when playing with case c). Call trace is as follows when function_graph is not used.

Call trace:
 [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30          <- (1)
 [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c            <- (2)
 [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
 [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
 [<fffffe00001f2638>] __vfs_write+0x44/0x104                <- (3)
 [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
 [<fffffe00001f3730>] SyS_write+0x50/0xb0
 [<fffffe00000939ec>] el0_svc_naked+0x20/0x28               <- (4)

When function_graph is set, some entries, such as do_mem_abort, are added between (1) and (2). In addition, entries from (3) to (4) are not printed out. As tracking down elements of ret_stack[], I realize dump_backtrace() has been terminated before reaching to ret_stack[0]. Have you seen this kind of behavior? I believe push & pop operations work correctly.

Quoted 1 line

Please correct me if I'm wrong.

Oops, I mis-interpreted the result output.
You are right.
This can happen because the original dump_backtrace() wants to trace a stack from a function where an exception has taken place, sysrq_handle_crash(), while ret_stack[curr_ret_stack] doesn't point to that function, but the very top of traced functions in callchains, that is, __do_kernel_fault in your case, probably.

So it results in replacing entries of return_to_handler to wrong function addresses.

A fixup! patch attached below (informative only) fixes this issue by once tracing all the functions on a stack, but preventing a top few ones from being printed. But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc as a trapped function because, as I've already mentioned before, we always miss the function when walking through a stack from an exception handler to functions in a thread context.
(Please note that we will introduce a dummy stack frame at interrupt, but not at exception.)

Quoted 100 lines

Quoted 30 lines

Signed-off-by AKASHI Takahiro <[email protected]>

---

arch/arm64/include/asm/ftrace.h     |    2 ++
arch/arm64/include/asm/stacktrace.h |    3 +++
arch/arm64/kernel/perf_callchain.c  |    3 +++
arch/arm64/kernel/process.c         |    3 +++
arch/arm64/kernel/return_address.c  |    3 +++
arch/arm64/kernel/stacktrace.c      |   17 +++++++++++++++++
arch/arm64/kernel/time.c            |    3 +++
arch/arm64/kernel/traps.c           |    3 +++
8 files changed, 37 insertions(+)

arch/arm64/include/asm/ftrace.h +2 -0

index c5534fa..3c60f37 100644
@@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*

arch/arm64/include/asm/stacktrace.h +0 -0

index 6fb61c5..801a16db 100644

arch/arm64/include/asm/stacktrace.h struct stackframe { · line 28

Quoted 7 lines

arch/arm64/include/asm/stacktrace.h +3 -0

@@ -22,6 +22,9 @@ struct stackframe {
 	unsigned long fp;
 	unsigned long sp;
 	unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int graph;
+#endif
 };

How about using int instead of unsigned int to align with cure_ret_stack of struct task_struct?

Quoted 59 lines

arch/arm64/include/asm/stacktrace.h +0 -0

 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);

arch/arm64/kernel/perf_callchain.c +3 -0

index 797220d..ff46654 100644
@@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, callchain_trace, entry);
 }

arch/arm64/kernel/process.c +3 -0

index 98bf546..88d742b 100644
@@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
 	frame.fp = thread_saved_fp(p);
 	frame.sp = thread_saved_sp(p);
 	frame.pc = thread_saved_pc(p);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = p->curr_ret_stack;
+#endif
 	stack_page = (unsigned long)task_stack_page(p);
 	do {
 		if (frame.sp < stack_page ||

arch/arm64/kernel/return_address.c +3 -0

index 07b37ac..1718706 100644
@@ -43,6 +43,9 @@ void *return_address(unsigned int level)
 	frame.fp = (unsigned long)__builtin_frame_address(0);
 	frame.sp = current_stack_pointer;
 	frame.pc = (unsigned long)return_address; /* dummy */
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, save_return_addr, &data);
 

arch/arm64/kernel/stacktrace.c +1 -0

index 9c7acf8..0a39049 100644
@@ -17,6 +17,7 @@
 */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp);
 	frame->pc = *(unsigned long *)(fp + 8);
 

Quoted 16 lines

arch/arm64/kernel/stacktrace.c int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) · line 81

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +12 -0

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (tsk && tsk->ret_stack &&
+			(frame->pc == (unsigned long)return_to_handler)) {
+		/*
+		 * This is a case where function graph tracer has
+		 * modified a return address (LR) in a stack frame
+		 * to hook a function return.
+		 * So replace it to an original value.
+		 */
+		frame->pc = tsk->ret_stack[frame->graph--].ret;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

There is an index check of ret_stack[] in case of x86 [1]. Even though graph is unsigned int, I think we need to check the value of frame->graph before accessing ret_stack[].

I'm not sure that the checking is very useful because, if it happens, it is a bug. It might make sense to avoid a possible panic though.

Quoted 12 lines

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +4 -0

+
 	/*
 	 * Check whether we are going to walk through from interrupt stack
 	 * to task stack.

@@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)save_stack_trace_tsk;
 	}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = tsk->curr_ret_stack;
+#endif
 

Quoted 15 lines

arch/arm64/kernel/time.c unsigned long profile_pc(struct pt_regs *regs) · line 57

Quoted 13 lines

arch/arm64/kernel/stacktrace.c +0 -0

 	walk_stackframe(tsk, &frame, save_trace, &data);
 	if (trace->nr_entries < trace->max_entries)

arch/arm64/kernel/time.c +3 -0

index 6e5c521..5977969 100644
@@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = -1; /* no task info */
+#endif

graph is unsigned int type. Is this intentional?

No. This initialization is, I believe, redundant as it is not checked anywhere, but I will re-think of it along with the checking above.

Thanks,
-Takahiro AKASHI

Quoted 5 lines

Best Regards
Jungseok Lee

[1] arch/x86/kernel/dumpstack.c

----8<----
From 9ebba7167f7838daf68d8231f04141d2f4d4b7b5 Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <[email protected]>
Date: Wed, 25 Nov 2015 13:29:54 +0900
Subject: [PATCH] fixup! arm64: ftrace: fix a stack tracer's output under function graph tracer

Signed-off-by AKASHI Takahiro <[email protected]>

---
arch/arm64/kernel/traps.c | 23 +++++++++++++++++------

1 file changed, 17 insertions(+), 6 deletions(-)

arch/arm64/kernel/traps.c +17 -6

index 46053c2..f140029 100644
@@ -147,17 +147,14 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
 	unsigned long _irq_stack_ptr = per_cpu(irq_stack_ptr, smp_processor_id());
+	int skip;
 
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
 
 	if (!tsk)
 		tsk = current;
 
-	if (regs) {
-		frame.fp = regs->regs[29];
-		frame.sp = regs->sp;
-		frame.pc = regs->pc;
-	} else if (tsk == current) {
+	if (tsk == current) {
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)dump_backtrace;

@@ -173,13 +170,27 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	frame.graph = tsk->curr_ret_stack;
 #endif
 
+	skip = (regs ? 1 : 0);
 	pr_emerg("Call trace:\n");
 	while (1) {
 		unsigned long where = frame.pc;
 		unsigned long stack;
 		int ret;
 
-		dump_backtrace_entry(where);
+		/* skip until specified stack frame */
+		if (!skip)
+			dump_backtrace_entry(where);
+		else if (frame.fp == regs->regs[29]) {
+			skip = 0;
+			/*
+			 * Mostly, this is the case where this function is
+			 * called in panic/abort. As exception handler's
+			 * stack frame does not contain the corresponding pc
+			 * at which an exception has taken place, use regs->pc
+			 * instead.
+			 */
+			dump_backtrace_entry(regs->pc);
+		}
 		ret = unwind_frame(tsk, &frame);
 		if (ret < 0)
 			break;

Signature · 1 line

1.7.9.5

Quoted 70 lines · AKASHI Takahiro

Quoted 69 lines · Jungseok Lee

On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:

Hi Akashi,

Quoted 42 lines

Function graph tracer modifies a return address (LR) in a stack frame to hook a function return. This will result in many useless entries (return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)

For example, in case of a),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ echo 1 > /proc/sys/kernel/stack_trace_enabled
  $ cat /sys/kernel/debug/tracing/stack_trace
        Depth    Size   Location    (54 entries)
        -----    ----   --------
  0)     4504      16   gic_raise_softirq+0x28/0x150
  1)     4488      80   smp_cross_call+0x38/0xb8
  2)     4408      48   return_to_handler+0x0/0x40
  3)     4360      32   return_to_handler+0x0/0x40
  ...

In case of b),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ perf record -e mem:XXX:x -ag -- sleep 10
  $ perf report
                  ...
                  |          |          |--0.22%-- 0x550f8
                  |          |          |          0x10888
                  |          |          |          el0_svc_naked
                  |          |          |          sys_openat
                  |          |          |          return_to_handler
                  |          |          |          return_to_handler
                  ...

In case of c),

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ echo c > /proc/sysrq-trigger
  ...
  Call trace:
  [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
  [<ffffffc000092250>] return_to_handler+0x0/0x40
  [<ffffffc000092250>] return_to_handler+0x0/0x40
  ...

Quoted 3 lines

This patch replaces such entries with real addresses preserved in current->ret_stack[] at unwind_frame(). This way, we can cover all the cases.

I've observed a strange behavior when playing with case c). Call trace is as follows when function_graph is not used.

Call trace:
  [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30          <- (1)
  [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c            <- (2)
  [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
  [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
  [<fffffe00001f2638>] __vfs_write+0x44/0x104                <- (3)
  [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
  [<fffffe00001f3730>] SyS_write+0x50/0xb0
  [<fffffe00000939ec>] el0_svc_naked+0x20/0x28               <- (4)

When function_graph is set, some entries, such as do_mem_abort, are added between (1) and (2). In addition, entries from (3) to (4) are not printed out. As tracking down elements of ret_stack[], I realize dump_backtrace() has been terminated before reaching to ret_stack[0]. Have you seen this kind of behavior? I believe push & pop operations work correctly.

Quoted 8 lines

Quoted 1 line

Please correct me if I'm wrong.

Oops, I mis-interpreted the result output.
You are right.
This can happen because the original dump_backtrace() wants to trace a stack from a function where an exception has taken place, sysrq_handle_crash(), while ret_stack[curr_ret_stack] doesn't point to that function, but the very top of traced functions in callchains, that is, __do_kernel_fault in your case, probably.

Yes, __do_kernel_fault.

Quoted 10 lines

So it results in replacing entries of return_to_handler to wrong function addresses.

A fixup! patch attached below (informative only) fixes this issue by once tracing all the functions on a stack, but preventing a top few ones from being printed. But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc as a trapped function because, as I've already mentioned before, we always miss the function when walking through a stack from an exception handler to functions in a thread context.
(Please note that we will introduce a dummy stack frame at interrupt, but not at exception.)

Thanks for clear explanation!

Since I'm not the only person to experience the above case, I report one more and the last observation ;)

PC and LR information is printed out when function_graph is set.

PC is at sysrq_handle_crash+0x24/0x30
LR is at sysrq_handle_crash+0x10/0x30

The logs are as follows when function_graph turns off.

PC is at sysrq_handle_crash+0x24/0x30
LR is at __handle_sysrq+0x128/0x19c

I think __show_regs() might have a similar problem when retrieving LR according to the below informative patch. Thoughts?

Quoted 226 lines

Quoted 100 lines

Quoted 30 lines

Signed-off-by AKASHI Takahiro <[email protected]>

---

arch/arm64/include/asm/ftrace.h     |    2 ++
arch/arm64/include/asm/stacktrace.h |    3 +++
arch/arm64/kernel/perf_callchain.c  |    3 +++
arch/arm64/kernel/process.c         |    3 +++
arch/arm64/kernel/return_address.c  |    3 +++
arch/arm64/kernel/stacktrace.c      |   17 +++++++++++++++++
arch/arm64/kernel/time.c            |    3 +++
arch/arm64/kernel/traps.c           |    3 +++
8 files changed, 37 insertions(+)

arch/arm64/include/asm/ftrace.h +2 -0

index c5534fa..3c60f37 100644
@@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*

arch/arm64/include/asm/stacktrace.h +0 -0

index 6fb61c5..801a16db 100644

arch/arm64/include/asm/stacktrace.h struct stackframe { · line 28

Quoted 7 lines

arch/arm64/include/asm/stacktrace.h +3 -0

@@ -22,6 +22,9 @@ struct stackframe {
 	unsigned long fp;
 	unsigned long sp;
 	unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int graph;
+#endif
 };

How about using int instead of unsigned int to align with cure_ret_stack of struct task_struct?

Quoted 59 lines

arch/arm64/include/asm/stacktrace.h +0 -0

 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);

arch/arm64/kernel/perf_callchain.c +3 -0

index 797220d..ff46654 100644
@@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, callchain_trace, entry);
 }

arch/arm64/kernel/process.c +3 -0

index 98bf546..88d742b 100644
@@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
 	frame.fp = thread_saved_fp(p);
 	frame.sp = thread_saved_sp(p);
 	frame.pc = thread_saved_pc(p);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = p->curr_ret_stack;
+#endif
 	stack_page = (unsigned long)task_stack_page(p);
 	do {
 		if (frame.sp < stack_page ||

arch/arm64/kernel/return_address.c +3 -0

index 07b37ac..1718706 100644
@@ -43,6 +43,9 @@ void *return_address(unsigned int level)
 	frame.fp = (unsigned long)__builtin_frame_address(0);
 	frame.sp = current_stack_pointer;
 	frame.pc = (unsigned long)return_address; /* dummy */
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = current->curr_ret_stack;
+#endif
 
 	walk_stackframe(current, &frame, save_return_addr, &data);
 

arch/arm64/kernel/stacktrace.c +1 -0

index 9c7acf8..0a39049 100644
@@ -17,6 +17,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp);
 	frame->pc = *(unsigned long *)(fp + 8);
 

Quoted 16 lines

arch/arm64/kernel/stacktrace.c int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) · line 81

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +12 -0

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (tsk && tsk->ret_stack &&
+			(frame->pc == (unsigned long)return_to_handler)) {
+		/*
+		 * This is a case where function graph tracer has
+		 * modified a return address (LR) in a stack frame
+		 * to hook a function return.
+		 * So replace it to an original value.
+		 */
+		frame->pc = tsk->ret_stack[frame->graph--].ret;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

There is an index check of ret_stack[] in case of x86 [1]. Even though graph is unsigned int, I think we need to check the value of frame->graph before accessing ret_stack[].

I'm not sure that the checking is very useful because, if it happens, it is a bug. It might make sense to avoid a possible panic though.

Quoted 12 lines

Quoted 12 lines

arch/arm64/kernel/stacktrace.c +4 -0

+
 	/*
 	 * Check whether we are going to walk through from interrupt stack
 	 * to task stack.

@@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)save_stack_trace_tsk;
 	}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = tsk->curr_ret_stack;
+#endif
 

Quoted 15 lines

arch/arm64/kernel/time.c unsigned long profile_pc(struct pt_regs *regs) · line 57

Quoted 13 lines

arch/arm64/kernel/stacktrace.c +0 -0

 	walk_stackframe(tsk, &frame, save_trace, &data);
 	if (trace->nr_entries < trace->max_entries)

arch/arm64/kernel/time.c +3 -0

index 6e5c521..5977969 100644
@@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
 	frame.fp = regs->regs[29];
 	frame.sp = regs->sp;
 	frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame.graph = -1; /* no task info */
+#endif

graph is unsigned int type. Is this intentional?

No. This initialization is, I believe, redundant as it is not checked anywhere, but I will re-think of it along with the checking above.

Thanks,
-Takahiro AKASHI

Quoted 5 lines

Best Regards
Jungseok Lee

[1] arch/x86/kernel/dumpstack.c

----8<----
From 9ebba7167f7838daf68d8231f04141d2f4d4b7b5 Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <[email protected]>
Date: Wed, 25 Nov 2015 13:29:54 +0900
Subject: [PATCH] fixup! arm64: ftrace: fix a stack tracer's output under function graph tracer

Signed-off-by AKASHI Takahiro <[email protected]>

---
arch/arm64/kernel/traps.c | 23 +++++++++++++++++------

1 file changed, 17 insertions(+), 6 deletions(-)

arch/arm64/kernel/traps.c +17 -7

index 46053c2..f140029 100644
@@ -147,17 +147,14 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
 	unsigned long _irq_stack_ptr = per_cpu(irq_stack_ptr, smp_processor_id());
+	int skip;
 
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
 
 	if (!tsk)
 		tsk = current;
 
-	if (regs) {
-		frame.fp = regs->regs[29];
-		frame.sp = regs->sp;
-		frame.pc = regs->pc;
-	} else if (tsk == current) {
+	if (tsk == current) {
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_stack_pointer;
 		frame.pc = (unsigned long)dump_backtrace;

@@ -173,13 +170,27 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	frame.graph = tsk->curr_ret_stack;
 #endif
 
+	skip = (regs ? 1 : 0);
 	pr_emerg("Call trace:\n");
 	while (1) {
 		unsigned long where = frame.pc;
 		unsigned long stack;
 		int ret;
 
-		dump_backtrace_entry(where);
+		/* skip until specified stack frame */
+		if (!skip)
+			dump_backtrace_entry(where);
+		else if (frame.fp == regs->regs[29]) {
+			skip = 0;
+			/*
+			 * Mostly, this is the case where this function is
+			 * called in panic/abort. As exception handler's
+			 * stack frame does not contain the corresponding pc
+			 * at which an exception has taken place, use regs->pc
+			 * instead.
+			 */
+			dump_backtrace_entry(regs->pc);
+		}
 		ret = unwind_frame(tsk, &frame);
 		if (ret < 0)
 			break;
--

Quoted 1 line

1.7.9.5

This fixes up the issue I mentioned.

Best Regards
Jungseok Lee

Jungseok,

Quoted 108 lines · AKASHI Takahiro

Quoted 70 lines · Jungseok Lee

Quoted 69 lines · AKASHI Takahiro

Hi Akashi,

Quoted 42 lines

Function graph tracer modifies a return address (LR) in a stack frame to hook a function return. This will result in many useless entries (return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)

For example, in case of a),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo 1 > /proc/sys/kernel/stack_trace_enabled
 $ cat /sys/kernel/debug/tracing/stack_trace
       Depth    Size   Location    (54 entries)
       -----    ----   --------
 0)     4504      16   gic_raise_softirq+0x28/0x150
 1)     4488      80   smp_cross_call+0x38/0xb8
 2)     4408      48   return_to_handler+0x0/0x40
 3)     4360      32   return_to_handler+0x0/0x40
 ...

In case of b),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ perf record -e mem:XXX:x -ag -- sleep 10
 $ perf report
                 ...
                 |          |          |--0.22%-- 0x550f8
                 |          |          |          0x10888
                 |          |          |          el0_svc_naked
                 |          |          |          sys_openat
                 |          |          |          return_to_handler
                 |          |          |          return_to_handler
                 ...

In case of c),

 $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
 $ echo c > /proc/sysrq-trigger
 ...
 Call trace:
 [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 [<ffffffc000092250>] return_to_handler+0x0/0x40
 ...

Quoted 3 lines

This patch replaces such entries with real addresses preserved in current->ret_stack[] at unwind_frame(). This way, we can cover all the cases.

I've observed a strange behavior when playing with case c). Call trace is as follows when function_graph is not used.

Call trace:
 [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30          <- (1)
 [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c            <- (2)
 [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
 [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
 [<fffffe00001f2638>] __vfs_write+0x44/0x104                <- (3)
 [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
 [<fffffe00001f3730>] SyS_write+0x50/0xb0
 [<fffffe00000939ec>] el0_svc_naked+0x20/0x28               <- (4)

When function_graph is set, some entries, such as do_mem_abort, are added between (1) and (2). In addition, entries from (3) to (4) are not printed out. As tracking down elements of ret_stack[], I realize dump_backtrace() has been terminated before reaching to ret_stack[0]. Have you seen this kind of behavior? I believe push & pop operations work correctly.

Quoted 8 lines

Quoted 1 line

Please correct me if I'm wrong.

Oops, I mis-interpreted the result output.
You are right.
This can happen because the original dump_backtrace() wants to trace a stack from a function where an exception has taken place, sysrq_handle_crash(), while ret_stack[curr_ret_stack] doesn't point to that function, but the very top of traced functions in callchains, that is, __do_kernel_fault in your case, probably.

Yes, __do_kernel_fault.

Quoted 10 lines

So it results in replacing entries of return_to_handler to wrong function addresses.

A fixup! patch attached below (informative only) fixes this issue by once tracing all the functions on a stack, but preventing a top few ones from being printed. But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc as a trapped function because, as I've already mentioned before, we always miss the function when walking through a stack from an exception handler to functions in a thread context.
(Please note that we will introduce a dummy stack frame at interrupt, but not at exception.)

Thanks for clear explanation!

Since I'm not the only person to experience the above case, I report one more and the last observation ;)

PC and LR information is printed out when function_graph is set.

PC is at sysrq_handle_crash+0x24/0x30
LR is at sysrq_handle_crash+0x10/0x30

The logs are as follows when function_graph turns off.

PC is at sysrq_handle_crash+0x24/0x30
LR is at __handle_sysrq+0x128/0x19c

Quoted 2 lines

I think __show_regs() might have a similar problem when retrieving LR according to the below informative patch. Thoughts?

Well, I believe that it is normal.
Sysrq_handle_crash() is basically a leaf function, but once function or function graph tracer is turned on, ftrace_caller() is called before its function body and so LR has been modified when panic/show_regs().

A function prologue analyzer is a requisite for implementing stack tracer and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode, at least, stp, add, sub and mov instructions.

This patch adds decoders for those instructions, that are used solely by stack tracer for now, but generic enough for other uses.

Reviewed-by Jungseok Lee <[email protected]>

Tested-by Jungseok Lee <[email protected]>

Signed-off-by AKASHI Takahiro <[email protected]>

---
 arch/arm64/include/asm/insn.h |   18 ++++++++
 arch/arm64/kernel/insn.c      |  102 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

arch/arm64/include/asm/insn.h +0 -0

index 30e50eb..8d5c538 100644

arch/arm64/include/asm/insn.h +13 -0

 #undef	__AARCH64_INSN_FUNCS
 

@@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
 u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
 u32 aarch32_insn_mcr_extract_opc2(u32 insn);
 u32 aarch32_insn_mcr_extract_crm(u32 insn);
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type);
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type);
 #endif /* __ASSEMBLY__ */
 
 #endif	/* __ASM_INSN_H */

arch/arm64/kernel/insn.c +1 -0

index c08b9ad..b56a66c 100644
@@ -33,6 +33,7 @@
 #include <asm/insn.h>
 
 #define AARCH64_INSN_SF_BIT	BIT(31)
+#define AARCH64_INSN_S_BIT	BIT(29)
 #define AARCH64_INSN_N_BIT	BIT(22)
 
 static int aarch64_insn_encoding_class[] = {

arch/arm64/kernel/insn.c +13 -0

+{
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:

arch/arm64/kernel/insn.c +6 -0

+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type)
+{

arch/arm64/kernel/insn.c +27 -0

+
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type)
+{
+	int imm;
+
+	if (aarch64_insn_is_stp(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR;
+	else if (aarch64_insn_is_stp_post(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_stp_pre(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
+	else if (aarch64_insn_is_ldp(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR;
+	else if (aarch64_insn_is_ldp_post(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_ldp_pre(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
+	else
+		return 0;
+
+	*variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :

Quoted 21 lines

A function prologue analyzer is a requisite for implementing stack tracer and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode, at least, stp, add, sub and mov instructions.

This patch adds decoders for those instructions, that are used solely by stack tracer for now, but generic enough for other uses.

Reviewed-by Jungseok Lee <[email protected]>

Tested-by Jungseok Lee <[email protected]>

Signed-off-by AKASHI Takahiro <[email protected]>

---
arch/arm64/include/asm/insn.h | 18 ++++++++
arch/arm64/kernel/insn.c | 102 +++++++++++++++++++++++++++++++++++++++++

2 files changed, 120 insertions(+)

arch/arm64/include/asm/insn.h +0 -0

index 30e50eb..8d5c538 100644

arch/arm64/include/asm/insn.h enum aarch64_insn_ldst_type { · line 169

Quoted 5 lines

arch/arm64/include/asm/insn.h +2 -0

@@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
 	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
 	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
 	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+	AARCH64_INSN_LDST_LOAD_PAIR,
+	AARCH64_INSN_LDST_STORE_PAIR,

For consistency with the rest of this header, we should be calling these AARCH64_INSN_LDST_{LOAD,STORE}_PAIR_REG_OFFSET...

arch/arm64/include/asm/insn.h static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \ · line 231

Quoted 9 lines

arch/arm64/include/asm/insn.h +2 -0

 };
 
 enum aarch64_insn_adsb_type {

@@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
 
 __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
 __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
+__AARCH64_INSN_FUNCS(stp,	0x7FC00000, 0x29000000)
+__AARCH64_INSN_FUNCS(ldp,	0x7FC00000, 0x29400000)

... and using stp_reg/ldp_reg here.

arch/arm64/include/asm/insn.h __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F) · line 284

Quoted 8 lines

arch/arm64/include/asm/insn.h +1 -0

 __AARCH64_INSN_FUNCS(stp_post,	0x7FC00000, 0x28800000)
 __AARCH64_INSN_FUNCS(ldp_post,	0x7FC00000, 0x28C00000)
 __AARCH64_INSN_FUNCS(stp_pre,	0x7FC00000, 0x29800000)

@@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
 __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
 __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
 __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F00E0)

Is this encoding correct? I ended up with 0xd69f03e0.

Quoted 16 lines · AKASHI Takahiro

A function prologue analyzer is a requisite for implementing stack tracer and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode, at least, stp, add, sub and mov instructions.

This patch adds decoders for those instructions, that are used solely by stack tracer for now, but generic enough for other uses.

Reviewed-by Jungseok Lee <[email protected]>

Tested-by Jungseok Lee <[email protected]>

Signed-off-by AKASHI Takahiro <[email protected]>

---
  arch/arm64/include/asm/insn.h |   18 ++++++++
  arch/arm64/kernel/insn.c      |  102 +++++++++++++++++++++++++++++++++++++++++
  2 files changed, 120 insertions(+)

arch/arm64/include/asm/insn.h enum aarch64_insn_ldst_type { · line 169

Quoted 10 lines

arch/arm64/include/asm/insn.h +2 -0

index 30e50eb..8d5c538 100644
@@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
  	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
  	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
  	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+	AARCH64_INSN_LDST_LOAD_PAIR,
+	AARCH64_INSN_LDST_STORE_PAIR,

For consistency with the rest of this header, we should be calling these AARCH64_INSN_LDST_{LOAD,STORE}_PAIR_REG_OFFSET...

arch/arm64/include/asm/insn.h static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \ · line 231

Quoted 9 lines

arch/arm64/include/asm/insn.h +2 -0

  };
 
  enum aarch64_insn_adsb_type {

@@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
 
  __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
  __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
+__AARCH64_INSN_FUNCS(stp,	0x7FC00000, 0x29000000)
+__AARCH64_INSN_FUNCS(ldp,	0x7FC00000, 0x29400000)

... and using stp_reg/ldp_reg here.

arch/arm64/include/asm/insn.h __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F) · line 284

Quoted 8 lines

arch/arm64/include/asm/insn.h +1 -0

  __AARCH64_INSN_FUNCS(stp_post,	0x7FC00000, 0x28800000)
  __AARCH64_INSN_FUNCS(ldp_post,	0x7FC00000, 0x28C00000)
  __AARCH64_INSN_FUNCS(stp_pre,	0x7FC00000, 0x29800000)

@@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
  __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
  __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
  __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F00E0)

Is this encoding correct? I ended up with 0xd69f03e0.

Quoted 33 lines

arch/arm64/include/asm/insn.h +13 -0

  #undef	__AARCH64_INSN_FUNCS
 

@@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
  u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
  u32 aarch32_insn_mcr_extract_opc2(u32 insn);
  u32 aarch32_insn_mcr_extract_crm(u32 insn);
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type);
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type);
  #endif /* __ASSEMBLY__ */
 
  #endif	/* __ASM_INSN_H */

arch/arm64/kernel/insn.c +1 -0

index c08b9ad..b56a66c 100644
@@ -33,6 +33,7 @@
  #include <asm/insn.h>
 
  #define AARCH64_INSN_SF_BIT	BIT(31)
+#define AARCH64_INSN_S_BIT	BIT(29)
  #define AARCH64_INSN_N_BIT	BIT(22)
 

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1147

Quoted 8 lines

arch/arm64/kernel/insn.c +3 -0

  static int aarch64_insn_encoding_class[] = {

@@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
  {
  	return insn & CRM_MASK;
  }
+
+enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
+				enum aarch64_insn_register_type type)

Maybe call this aarch64_insn_decode_register? You're basically building decoders for some of the *_encode_* functions we already have.

Quoted 18 lines

arch/arm64/kernel/insn.c +18 -0

+{
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:
+		shift = 10;
+		break;
+	case AARCH64_INSN_REGTYPE_RM:
+		shift = 16;
+		break;

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1170

Quoted 5 lines

arch/arm64/kernel/insn.c +5 -0

+	default:
+		pr_err("%s: unknown register type decoding %d\n", __func__,
+		       type);
+		return ~0L;
+	}

This is largely a copy-paste from aarch64_insn_encode_register. Can you either work with what we have or factor out the common parts, please?

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1175

Quoted 4 lines

arch/arm64/kernel/insn.c +4 -0

+	return (insn & (GENMASK(4, 0) << shift)) >> shift;
+}
+
+int aarch64_insn_decode_add_sub_imm(u32 insn,

This one could be aarch64_insn_extract_add_sub_imm, if you like. Then extract is the converse of gen and decode is the converse of encode.

Quoted 10 lines

arch/arm64/kernel/insn.c +10 -0

+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type)
+{
+	if (aarch64_insn_is_add_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_ADD_SETFLAGS :
+				AARCH64_INSN_ADSB_ADD;

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1191

Quoted 6 lines

arch/arm64/kernel/insn.c +6 -0

+	else if (aarch64_insn_is_sub_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_SUB_SETFLAGS :
+				AARCH64_INSN_ADSB_SUB;
+	else
+		return 0;

Why do we need to return 0 on error? -EINVAL might make more sense.

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1200

Quoted 9 lines

arch/arm64/kernel/insn.c +9 -0

+
+	*variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+					AARCH64_INSN_VARIANT_32BIT;
+
+	*dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
+
+	*src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	/* TODO: ignore shilft field[23:22] */

I don't understand the TODO.

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1201

Quoted 1 line

arch/arm64/kernel/insn.c +1 -0

+	*imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);

We use u64 for imm everywhere else.

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1202

Quoted 2 lines

arch/arm64/kernel/insn.c +2 -0

+	return 1;
+}

Similarly here, you're trying to implement the converse of aarch64_insn_gen_add_sub_imm

Quoted 17 lines

arch/arm64/kernel/insn.c +17 -0

+
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type)
+{
+	int imm;
+
+	if (aarch64_insn_is_stp(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR;
+	else if (aarch64_insn_is_stp_post(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_stp_pre(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1229

Quoted 8 lines

arch/arm64/kernel/insn.c +8 -0

+	else if (aarch64_insn_is_ldp(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR;
+	else if (aarch64_insn_is_ldp_post(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_ldp_pre(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
+	else
+		return 0;

Quoted 5 lines

arch/arm64/kernel/insn.c +5 -0

+	*variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+				AARCH64_INSN_VARIANT_32BIT;
+
+	*reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
+

arch/arm64/kernel/insn.c u32 aarch32_insn_mcr_extract_crm(u32 insn) · line 1241

Quoted 6 lines

arch/arm64/kernel/insn.c +6 -0

+	*reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
+
+	*base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
+	asm("sbfm %0, %0, 0, 6" : "+r" (imm));

What? Can't you write this in C?

Will

The patch ("arm64: ftrace: add arch-specific stack tracer") introduced a function prologue analyzer.

Given that there is no fixed template for a function prologue, at least on gcc for aarch64, a function prologue analyzer may be rather heuristic. So this patch adds a kernel command line option, function_prologue_analyzer_test, in order to run a basic test at startup by executing an analyzer against all the *traceable* functions.

For function_prologue_analyzer_test=2, the output looks like:

      po sp    fp    symbol
      == ==    ==    ======
   0: 0  0x040 0x000 gic_handle_irq+0x20/0xa4
   1: 0  0x040 0x000 gic_handle_irq+0x34/0x114
   2: 0  0x030 0x000 run_init_process+0x14/0x48
   3: 0  0x020 0x000 try_to_run_init_process+0x14/0x58
   4: 0  0x080 0x000 do_one_initcall+0x1c/0x194

...
22959: 0 0x020 0x000 tty_lock_slave+0x14/0x3c
22960: 0 0x020 0x000 tty_unlock_slave+0x14/0x3c
function prologue analyzer test: 0 errors

"po" indicates a position of callsite of mcount(), and should be zero if an analyzer has parsed a function prologue successfully and reached a location where fp is properly updated.
"sp" is a final offset to a parent's fp at the exit of function prologue. "fp" is also an ofset against sp at the exit of function prologue. So normally,
<new sp> = <old fp> + <"sp">
<new fp> = <new sp> - <"fp">

And the last line shows the number of possible errors in the result.

For function_prologue_analyzer_test=1, only the last line will be shown.

Signed-off-by AKASHI Takahiro <[email protected]>

---
arch/arm64/include/asm/insn.h | 2 +-
arch/arm64/kernel/stacktrace.c | 52 ++++++++++++++++++++++++++++++++++++++++

2 files changed, 53 insertions(+), 1 deletion(-)

arch/arm64/include/asm/insn.h +0 -0

index 8d5c538..1dfae16 100644

arch/arm64/include/asm/insn.h +0 -0

 #undef	__AARCH64_INSN_FUNCS
 

arch/arm64/kernel/stacktrace.c +31 -0

index 1d18bc4..19dad62 100644
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/ftrace.h>
+#include <linux/kallsyms.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -348,5 +349,56 @@ void save_stack_trace_sp(struct stack_trace *trace,
 {
 	__save_stack_trace_tsk(current, trace, stack_dump_sp);
 }
+
+static int start_analyzer_test __initdata;
+
+static int __init enable_analyzer_test(char *str)
+{
+	get_option(&str, &start_analyzer_test);
+	return 0;
+}
+early_param("function_prologue_analyzer_test", enable_analyzer_test);
+
+static void __init do_test_function_prologue_analyzer(void)
+{
+	extern unsigned long __start_mcount_loc[];
+	extern unsigned long __stop_mcount_loc[];
+	unsigned long count, i, errors;
+	int print_once;
+
+	count = __stop_mcount_loc - __start_mcount_loc;
+	errors = print_once = 0;
+	for (i = 0; i < count; i++) {
+		unsigned long addr, sp_off, fp_off;
+		int pos;
+		bool check;
+		char buf[60];
+
+		addr = __start_mcount_loc[i];
+		pos = analyze_function_prologue(addr, &sp_off, &fp_off);
+		check = ((pos != 0) || !sp_off || (sp_off <= fp_off));
+		if (check)
+			errors++;

Quoted 47 lines · AKASHI Takahiro

The patch ("arm64: ftrace: add arch-specific stack tracer") introduced a function prologue analyzer.

Given that there is no fixed template for a function prologue, at least on gcc for aarch64, a function prologue analyzer may be rather heuristic. So this patch adds a kernel command line option, function_prologue_analyzer_test, in order to run a basic test at startup by executing an analyzer against all the *traceable* functions.

For function_prologue_analyzer_test=2, the output looks like:

       po sp    fp    symbol
       == ==    ==    ======
    0: 0  0x040 0x000 gic_handle_irq+0x20/0xa4
    1: 0  0x040 0x000 gic_handle_irq+0x34/0x114
    2: 0  0x030 0x000 run_init_process+0x14/0x48
    3: 0  0x020 0x000 try_to_run_init_process+0x14/0x58
    4: 0  0x080 0x000 do_one_initcall+0x1c/0x194
    ...

22959: 0 0x020 0x000 tty_lock_slave+0x14/0x3c
22960: 0 0x020 0x000 tty_unlock_slave+0x14/0x3c
function prologue analyzer test: 0 errors

"po" indicates a position of callsite of mcount(), and should be zero if an analyzer has parsed a function prologue successfully and reached a location where fp is properly updated.
"sp" is a final offset to a parent's fp at the exit of function prologue. "fp" is also an ofset against sp at the exit of function prologue. So normally,
<new sp> = <old fp> + <"sp">
<new fp> = <new sp> - <"fp">

And the last line shows the number of possible errors in the result.

For function_prologue_analyzer_test=1, only the last line will be shown.

Signed-off-by AKASHI Takahiro <[email protected]>

---
arch/arm64/include/asm/insn.h | 2 +-
arch/arm64/kernel/stacktrace.c | 52 ++++++++++++++++++++++++++++++++++++++++

2 files changed, 53 insertions(+), 1 deletion(-)

arch/arm64/include/asm/insn.h +0 -0

index 8d5c538..1dfae16 100644

arch/arm64/include/asm/insn.h __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F) · line 284

Quoted 5 lines

arch/arm64/include/asm/insn.h +1 -1

@@ -281,7 +281,7 @@ __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
 __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
 __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
 __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
-__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F00E0)
+__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F03E0)

How about folding(squashing) this hunk into [PATCH 4/6]?

Quoted 53 lines

arch/arm64/include/asm/insn.h +0 -0

 #undef	__AARCH64_INSN_FUNCS
 

arch/arm64/kernel/stacktrace.c +36 -0

index 1d18bc4..19dad62 100644
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/ftrace.h>
+#include <linux/kallsyms.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 

@@ -348,5 +349,56 @@ void save_stack_trace_sp(struct stack_trace *trace,
 {
 	__save_stack_trace_tsk(current, trace, stack_dump_sp);
 }
+
+static int start_analyzer_test __initdata;
+
+static int __init enable_analyzer_test(char *str)
+{
+	get_option(&str, &start_analyzer_test);
+	return 0;
+}
+early_param("function_prologue_analyzer_test", enable_analyzer_test);
+
+static void __init do_test_function_prologue_analyzer(void)
+{
+	extern unsigned long __start_mcount_loc[];
+	extern unsigned long __stop_mcount_loc[];
+	unsigned long count, i, errors;
+	int print_once;
+
+	count = __stop_mcount_loc - __start_mcount_loc;
+	errors = print_once = 0;
+	for (i = 0; i < count; i++) {
+		unsigned long addr, sp_off, fp_off;
+		int pos;
+		bool check;
+		char buf[60];
+
+		addr = __start_mcount_loc[i];
+		pos = analyze_function_prologue(addr, &sp_off, &fp_off);
+		check = ((pos != 0) || !sp_off || (sp_off <= fp_off));
+		if (check)
+			errors++;
+		if (check || (start_analyzer_test > 1)) {
+			if (!print_once) {
+				pr_debug("       po sp    fp    symbol\n");
+				pr_debug("       == ==    ==    ======\n");
+				print_once++;

arch/arm64/kernel/stacktrace.c void save_stack_trace_sp(struct stack_trace *trace, · line 391

Quoted 5 lines

arch/arm64/kernel/stacktrace.c +5 -0

+			}
+			sprint_symbol(buf, addr);
+			pr_debug("%5ld: %d  0x%03lx 0x%03lx %s\n",
+					i, pos, sp_off, fp_off, buf);
+		}

It would be hard to find out which entry has an error if start_analyer_test is set to 2. How about adding one more column to mark 'OK' or 'NG'?

BTW, as I mentioned in previous thread, I'm also waiting for feedbacks on the analyzer itself :)

Best Regards
Jungseok Lee