* [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 5:24 ` Qing Zhang
2022-12-15 4:01 ` [PATCH 2/6] LoongArch: Use correct sp value to get graph addr in unwinder guess Jinyang He
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
At unwind_start, it is better to try to get its frame info even we not
support regs rather than get them outside. So that we can simply use
unwind_{start, next_frame, done} outside.
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/kernel/process.c | 12 +++---------
arch/loongarch/kernel/unwind_guess.c | 6 ++++++
arch/loongarch/kernel/unwind_prologue.c | 16 +++++++++++++---
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
index 502b8b950057..6ef45174ad35 100644
--- a/arch/loongarch/kernel/process.c
+++ b/arch/loongarch/kernel/process.c
@@ -197,20 +197,14 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
unsigned long __get_wchan(struct task_struct *task)
{
- unsigned long pc;
+ unsigned long pc = 0;
struct unwind_state state;
if (!try_get_task_stack(task))
return 0;
- unwind_start(&state, task, NULL);
- state.sp = thread_saved_fp(task);
- get_stack_info(state.sp, state.task, &state.stack_info);
- state.pc = thread_saved_ra(task);
-#ifdef CONFIG_UNWINDER_PROLOGUE
- state.type = UNWINDER_PROLOGUE;
-#endif
- for (; !unwind_done(&state); unwind_next_frame(&state)) {
+ for (unwind_start(&state, task, NULL); !unwind_done(&state);
+ unwind_next_frame(&state)) {
pc = unwind_get_return_address(&state);
if (!pc)
break;
diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
index e2d2e4f3001f..e03864511582 100644
--- a/arch/loongarch/kernel/unwind_guess.c
+++ b/arch/loongarch/kernel/unwind_guess.c
@@ -26,6 +26,12 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
if (regs) {
state->sp = regs->regs[3];
state->pc = regs->csr_era;
+ } else if (task == current) {
+ state->sp = (unsigned long)__builtin_frame_address(0);
+ state->pc = (unsigned long)__builtin_return_address(0);
+ } else {
+ state->sp = thread_saved_fp(task);
+ state->pc = thread_saved_ra(task);
}
state->task = task;
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index 0f8d1451ebb8..9d51ea37782e 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -141,12 +141,22 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
struct pt_regs *regs)
{
memset(state, 0, sizeof(*state));
+ state->type = UNWINDER_PROLOGUE;
- if (regs && __kernel_text_address(regs->csr_era)) {
- state->pc = regs->csr_era;
+ if (regs) {
state->sp = regs->regs[3];
+ state->pc = regs->csr_era;
state->ra = regs->regs[1];
- state->type = UNWINDER_PROLOGUE;
+ if (!__kernel_text_address(state->pc))
+ state->type = UNWINDER_GUESS;
+ } else if (task == current) {
+ state->sp = (unsigned long)__builtin_frame_address(0);
+ state->pc = (unsigned long)__builtin_return_address(0);
+ state->ra = 0;
+ } else {
+ state->sp = thread_saved_fp(task);
+ state->pc = thread_saved_ra(task);
+ state->ra = 0;
}
state->task = task;
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported
2022-12-15 4:01 ` [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported Jinyang He
@ 2022-12-15 5:24 ` Qing Zhang
2022-12-15 5:55 ` Jinyang He
0 siblings, 1 reply; 19+ messages in thread
From: Qing Zhang @ 2022-12-15 5:24 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Hi, Jinyang
On 2022/12/15 下午12:01, Jinyang He wrote:
> At unwind_start, it is better to try to get its frame info even we not
> support regs rather than get them outside. So that we can simply use
> unwind_{start, next_frame, done} outside.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> arch/loongarch/kernel/process.c | 12 +++---------
> arch/loongarch/kernel/unwind_guess.c | 6 ++++++
> arch/loongarch/kernel/unwind_prologue.c | 16 +++++++++++++---
> 3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
> index 502b8b950057..6ef45174ad35 100644
> --- a/arch/loongarch/kernel/process.c
> +++ b/arch/loongarch/kernel/process.c
> @@ -197,20 +197,14 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>
> unsigned long __get_wchan(struct task_struct *task)
> {
> - unsigned long pc;
> + unsigned long pc = 0;
> struct unwind_state state;
>
> if (!try_get_task_stack(task))
> return 0;
>
> - unwind_start(&state, task, NULL);
> - state.sp = thread_saved_fp(task);
> - get_stack_info(state.sp, state.task, &state.stack_info);
> - state.pc = thread_saved_ra(task);
> -#ifdef CONFIG_UNWINDER_PROLOGUE
> - state.type = UNWINDER_PROLOGUE;
> -#endif
> - for (; !unwind_done(&state); unwind_next_frame(&state)) {
> + for (unwind_start(&state, task, NULL); !unwind_done(&state);
> + unwind_next_frame(&state)) {
> pc = unwind_get_return_address(&state);
> if (!pc)
> break;
> diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
> index e2d2e4f3001f..e03864511582 100644
> --- a/arch/loongarch/kernel/unwind_guess.c
> +++ b/arch/loongarch/kernel/unwind_guess.c
> @@ -26,6 +26,12 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
> if (regs) {
> state->sp = regs->regs[3];
> state->pc = regs->csr_era;
> + } else if (task == current) {
> + state->sp = (unsigned long)__builtin_frame_address(0);
> + state->pc = (unsigned long)__builtin_return_address(0);
> + } else {
> + state->sp = thread_saved_fp(task);
> + state->pc = thread_saved_ra(task);
>
The case for tasks is already handled in stacktrace.c, which is reusable
code:
void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
struct task_struct *task, struct pt_regs *regs)
{
...
if (task == current) {
regs->regs[3] = (unsigned long)__builtin_frame_address(0);
regs->csr_era = (unsigned long)__builtin_return_address(0);
} else {
regs->regs[3] = thread_saved_fp(task);
regs->csr_era = thread_saved_ra(task);
}
...
for (unwind_start(&state, task, regs);
!unwind_done(&state); unwind_next_frame(&state)) {
addr = unwind_get_return_address(&state);
...
}
>
> state->task = task;
> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> index 0f8d1451ebb8..9d51ea37782e 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -141,12 +141,22 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> memset(state, 0, sizeof(*state));
> + state->type = UNWINDER_PROLOGUE;
>
> - if (regs && __kernel_text_address(regs->csr_era)) {
> - state->pc = regs->csr_era;
> + if (regs) {
> state->sp = regs->regs[3];
> + state->pc = regs->csr_era;
> state->ra = regs->regs[1];
> - state->type = UNWINDER_PROLOGUE;
> + if (!__kernel_text_address(state->pc))
> + state->type = UNWINDER_GUESS;
> + } else if (task == current) {
> + state->sp = (unsigned long)__builtin_frame_address(0);
> + state->pc = (unsigned long)__builtin_return_address(0);
> + state->ra = 0;
> + } else {
> + state->sp = thread_saved_fp(task);
> + state->pc = thread_saved_ra(task);
> + state->ra = 0;
> }
>
also...
> state->task = task;
>
Thanks,
-Qing
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported
2022-12-15 5:24 ` Qing Zhang
@ 2022-12-15 5:55 ` Jinyang He
0 siblings, 0 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-15 5:55 UTC (permalink / raw)
To: Qing Zhang, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
On 2022-12-15 13:24, Qing Zhang wrote:
> Hi, Jinyang
>
> On 2022/12/15 下午12:01, Jinyang He wrote:
>> At unwind_start, it is better to try to get its frame info even we not
>> support regs rather than get them outside. So that we can simply use
>> unwind_{start, next_frame, done} outside.
>>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>> arch/loongarch/kernel/process.c | 12 +++---------
>> arch/loongarch/kernel/unwind_guess.c | 6 ++++++
>> arch/loongarch/kernel/unwind_prologue.c | 16 +++++++++++++---
>> 3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/process.c
>> b/arch/loongarch/kernel/process.c
>> index 502b8b950057..6ef45174ad35 100644
>> --- a/arch/loongarch/kernel/process.c
>> +++ b/arch/loongarch/kernel/process.c
>> @@ -197,20 +197,14 @@ int copy_thread(struct task_struct *p, const
>> struct kernel_clone_args *args)
>> unsigned long __get_wchan(struct task_struct *task)
>> {
>> - unsigned long pc;
>> + unsigned long pc = 0;
>> struct unwind_state state;
>> if (!try_get_task_stack(task))
>> return 0;
>> - unwind_start(&state, task, NULL);
>> - state.sp = thread_saved_fp(task);
>> - get_stack_info(state.sp, state.task, &state.stack_info);
>> - state.pc = thread_saved_ra(task);
>> -#ifdef CONFIG_UNWINDER_PROLOGUE
>> - state.type = UNWINDER_PROLOGUE;
>> -#endif
>> - for (; !unwind_done(&state); unwind_next_frame(&state)) {
>> + for (unwind_start(&state, task, NULL); !unwind_done(&state);
>> + unwind_next_frame(&state)) {
>> pc = unwind_get_return_address(&state);
>> if (!pc)
>> break;
>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>> b/arch/loongarch/kernel/unwind_guess.c
>> index e2d2e4f3001f..e03864511582 100644
>> --- a/arch/loongarch/kernel/unwind_guess.c
>> +++ b/arch/loongarch/kernel/unwind_guess.c
>> @@ -26,6 +26,12 @@ void unwind_start(struct unwind_state *state,
>> struct task_struct *task,
>> if (regs) {
>> state->sp = regs->regs[3];
>> state->pc = regs->csr_era;
>> + } else if (task == current) {
>> + state->sp = (unsigned long)__builtin_frame_address(0);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> + } else {
>> + state->sp = thread_saved_fp(task);
>> + state->pc = thread_saved_ra(task);
> The case for tasks is already handled in stacktrace.c, which is
> reusable code:
> void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> struct task_struct *task, struct pt_regs *regs)
> {
> ...
>
> if (task == current) {
> regs->regs[3] = (unsigned long)__builtin_frame_address(0);
> regs->csr_era = (unsigned long)__builtin_return_address(0);
> } else {
> regs->regs[3] = thread_saved_fp(task);
> regs->csr_era = thread_saved_ra(task);
> }
> ...
> for (unwind_start(&state, task, regs);
> !unwind_done(&state); unwind_next_frame(&state)) {
> addr = unwind_get_return_address(&state);
> ...
> }
>
>> state->task = task;
>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>> b/arch/loongarch/kernel/unwind_prologue.c
>> index 0f8d1451ebb8..9d51ea37782e 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -141,12 +141,22 @@ void unwind_start(struct unwind_state *state,
>> struct task_struct *task,
>> struct pt_regs *regs)
>> {
>> memset(state, 0, sizeof(*state));
>> + state->type = UNWINDER_PROLOGUE;
>> - if (regs && __kernel_text_address(regs->csr_era)) {
>> - state->pc = regs->csr_era;
>> + if (regs) {
>> state->sp = regs->regs[3];
>> + state->pc = regs->csr_era;
>> state->ra = regs->regs[1];
>> - state->type = UNWINDER_PROLOGUE;
>> + if (!__kernel_text_address(state->pc))
>> + state->type = UNWINDER_GUESS;
>> + } else if (task == current) {
>> + state->sp = (unsigned long)__builtin_frame_address(0);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> + state->ra = 0;
>> + } else {
>> + state->sp = thread_saved_fp(task);
>> + state->pc = thread_saved_ra(task);
>> + state->ra = 0;
>> }
> also...
>> state->task = task;
>>
That looks similar, but stacktrace actually provides the regs parameter.
Unwind_{start, next_frame, get_return_address} is API to others, such the
GPL module. Btw, if we want to drop the redundant codes in arch_stack_walk
and keep same result, use "cookie->skip += 1", I think, but beyond this
series.
Thanks,
Jinyang
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/6] LoongArch: Use correct sp value to get graph addr in unwinder guess
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
2022-12-15 4:01 ` [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 4:01 ` [PATCH 3/6] LoongArch: Adjust PC value when unwind next frame in prologue unwinder Jinyang He
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
The stack frame when function_graph enable like follows,
--------- <- function sp_on_entry
|
|
|
FAKE_RA <- sp_on_entry - sizeof(pt_regs) + PT_R1
|
--------- <- sp_on_entry - sizeof(pt_regs)
So if we want to get the &FAKE_RA we should get sp_on_entry first.
In unwinder_prologue case, we can get the sp_on_entry as state->sp,
because we try to calculate each CFA and the ra saved address.
But in unwinder_guess case, we cannot get it because we do not try
to calculate the CFA. Although LoongArch have not fixed frame, the
$ra is saved at CFA - 8 in most cases, we can try guess, too.
As we store the pc in state, we not need to dereference state->sp, too.
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/include/asm/ftrace.h | 2 --
arch/loongarch/include/asm/unwind.h | 9 +++++++++
arch/loongarch/kernel/unwind_guess.c | 12 ++++--------
arch/loongarch/kernel/unwind_prologue.c | 22 ++++++----------------
4 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/arch/loongarch/include/asm/ftrace.h b/arch/loongarch/include/asm/ftrace.h
index 90f9d3399b2a..3418d32d4fc7 100644
--- a/arch/loongarch/include/asm/ftrace.h
+++ b/arch/loongarch/include/asm/ftrace.h
@@ -10,8 +10,6 @@
#define FTRACE_REGS_PLT_IDX 1
#define NR_FTRACE_PLTS 2
-#define GRAPH_FAKE_OFFSET (sizeof(struct pt_regs) - offsetof(struct pt_regs, regs[1]))
-
#ifdef CONFIG_FUNCTION_TRACER
#define MCOUNT_INSN_SIZE 4 /* sizeof mcount call */
diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
index f2b52b9ea93d..6ece48f0ff77 100644
--- a/arch/loongarch/include/asm/unwind.h
+++ b/arch/loongarch/include/asm/unwind.h
@@ -7,8 +7,10 @@
#ifndef _ASM_UNWIND_H
#define _ASM_UNWIND_H
+#include <linux/ftrace.h>
#include <linux/sched.h>
+#include <asm/ptrace.h>
#include <asm/stacktrace.h>
enum unwinder_type {
@@ -40,4 +42,11 @@ static inline bool unwind_error(struct unwind_state *state)
return state->error;
}
+#define GRAPH_FAKE_OFFSET (sizeof(struct pt_regs) - offsetof(struct pt_regs, regs[1]))
+static inline unsigned long unwind_graph_addr(struct unwind_state *state,
+ unsigned long pc, unsigned long cfa)
+{
+ return ftrace_graph_ret_addr(state->task, &state->graph_idx,
+ pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
+}
#endif /* _ASM_UNWIND_H */
diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
index e03864511582..8ce32c37c587 100644
--- a/arch/loongarch/kernel/unwind_guess.c
+++ b/arch/loongarch/kernel/unwind_guess.c
@@ -11,10 +11,7 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
{
if (unwind_done(state))
return 0;
- else if (state->first)
- return state->pc;
-
- return *(unsigned long *)(state->sp);
+ return state->pc;
}
EXPORT_SYMBOL_GPL(unwind_get_return_address);
@@ -36,7 +33,7 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
state->task = task;
state->first = true;
-
+ state->pc = unwind_graph_addr(state, state->pc, state->sp);
get_stack_info(state->sp, state->task, &state->stack_info);
if (!unwind_done(state) && !__kernel_text_address(state->pc))
@@ -60,9 +57,8 @@ bool unwind_next_frame(struct unwind_state *state)
state->sp < info->end;
state->sp += sizeof(unsigned long)) {
addr = *(unsigned long *)(state->sp);
- state->pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- addr, (unsigned long *)(state->sp - GRAPH_FAKE_OFFSET));
- if (__kernel_text_address(addr))
+ state->pc = unwind_graph_addr(state, addr, state->sp + 8);
+ if (__kernel_text_address(state->pc))
return true;
}
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index 9d51ea37782e..35cab7f77c6b 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -21,16 +21,9 @@ static inline void unwind_state_fixup(struct unwind_state *state)
unsigned long unwind_get_return_address(struct unwind_state *state)
{
-
if (unwind_done(state))
return 0;
- else if (state->type)
- return state->pc;
- else if (state->first)
- return state->pc;
-
- return *(unsigned long *)(state->sp);
-
+ return state->pc;
}
EXPORT_SYMBOL_GPL(unwind_get_return_address);
@@ -43,9 +36,8 @@ static bool unwind_by_guess(struct unwind_state *state)
state->sp < info->end;
state->sp += sizeof(unsigned long)) {
addr = *(unsigned long *)(state->sp);
- state->pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- addr, (unsigned long *)(state->sp - GRAPH_FAKE_OFFSET));
- if (__kernel_text_address(addr))
+ state->pc = unwind_graph_addr(state, addr, state->sp + 8);
+ if (__kernel_text_address(state->pc))
return true;
}
@@ -161,7 +153,7 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
state->task = task;
state->first = true;
-
+ state->pc = unwind_graph_addr(state, state->pc, state->sp);
get_stack_info(state->sp, state->task, &state->stack_info);
if (!unwind_done(state) && !__kernel_text_address(state->pc))
@@ -188,8 +180,7 @@ bool unwind_next_frame(struct unwind_state *state)
case UNWINDER_PROLOGUE:
if (unwind_by_prologue(state)) {
- state->pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- state->pc, (unsigned long *)(state->sp - GRAPH_FAKE_OFFSET));
+ state->pc = unwind_graph_addr(state, state->pc, state->sp);
return true;
}
@@ -204,8 +195,7 @@ bool unwind_next_frame(struct unwind_state *state)
state->first = true;
state->ra = regs->regs[1];
state->sp = regs->regs[3];
- state->pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- pc, (unsigned long *)(state->sp - GRAPH_FAKE_OFFSET));
+ state->pc = pc;
get_stack_info(state->sp, state->task, info);
return true;
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 3/6] LoongArch: Adjust PC value when unwind next frame in prologue unwinder
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
2022-12-15 4:01 ` [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported Jinyang He
2022-12-15 4:01 ` [PATCH 2/6] LoongArch: Use correct sp value to get graph addr in unwinder guess Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 4:01 ` [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder Jinyang He
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
When state->first is not set, the PC is a return address in the
previous frame. We need to adjust it value in case overflow to the
next symbol.
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/kernel/unwind_prologue.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index 35cab7f77c6b..d464c533c64f 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -48,7 +48,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
{
long frame_ra = -1;
unsigned long frame_size = 0;
- unsigned long size, offset, pc = state->pc;
+ unsigned long size, offset, pc;
struct pt_regs *regs;
struct stack_info *info = &state->stack_info;
union loongarch_instruction *ip, *ip_end;
@@ -70,6 +70,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
return true;
}
+ /* When first is not set, the PC is a return address in the previous frame.
+ * We need to adjust it value in case overflow to the next symbol.
+ */
+ pc = state->pc - (state->first ? 0 : LOONGARCH_INSN_SIZE);
if (!kallsyms_lookup_size_offset(pc, &size, &offset))
return false;
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
` (2 preceding siblings ...)
2022-12-15 4:01 ` [PATCH 3/6] LoongArch: Adjust PC value when unwind next frame in prologue unwinder Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 9:15 ` Qing Zhang
2022-12-19 3:28 ` Qing Zhang
2022-12-15 4:01 ` [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default Jinyang He
2022-12-15 4:01 ` [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder Jinyang He
5 siblings, 2 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
The prolugue unwinder rely on symbol info. When PC is not in kernel
text address, it cannot find relative symbol info and it will be broken.
The guess unwinder will be used in this case. And the guess unwinder
codes in prolugue unwinder is redundant. Strip it out and set the
unwinder info in unwind_state.
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/include/asm/unwind.h | 22 ++++
arch/loongarch/kernel/Makefile | 3 +-
arch/loongarch/kernel/unwind.c | 52 +++++++++
arch/loongarch/kernel/unwind_guess.c | 41 ++-----
arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
5 files changed, 135 insertions(+), 118 deletions(-)
create mode 100644 arch/loongarch/kernel/unwind.c
diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
index 6ece48f0ff77..a16aff1d086a 100644
--- a/arch/loongarch/include/asm/unwind.h
+++ b/arch/loongarch/include/asm/unwind.h
@@ -18,6 +18,8 @@ enum unwinder_type {
UNWINDER_PROLOGUE,
};
+struct unwinder_ops;
+
struct unwind_state {
char type; /* UNWINDER_XXX */
struct stack_info stack_info;
@@ -25,8 +27,22 @@ struct unwind_state {
bool first, error, is_ftrace;
int graph_idx;
unsigned long sp, pc, ra;
+ const struct unwinder_ops *ops;
+};
+
+struct unwinder_ops {
+ void (*unwind_start)(struct unwind_state *state,
+ struct task_struct *task, struct pt_regs *regs);
+ bool (*unwind_next_frame)(struct unwind_state *state);
+ unsigned long (*unwind_get_return_address)(struct unwind_state *state);
};
+extern const struct unwinder_ops *default_unwinder;
+extern const struct unwinder_ops unwinder_guess;
+#ifdef CONFIG_UNWINDER_PROLOGUE
+extern const struct unwinder_ops unwinder_prologue;
+#endif
+
void unwind_start(struct unwind_state *state,
struct task_struct *task, struct pt_regs *regs);
bool unwind_next_frame(struct unwind_state *state);
@@ -49,4 +65,10 @@ static inline unsigned long unwind_graph_addr(struct unwind_state *state,
return ftrace_graph_ret_addr(state->task, &state->graph_idx,
pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
}
+
+static inline void unwind_register_unwinder(struct unwind_state *state,
+ const struct unwinder_ops *unwinder)
+{
+ state->ops = unwinder;
+}
#endif /* _ASM_UNWIND_H */
diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
index 7ca65195f7f8..cb6029ea3ea9 100644
--- a/arch/loongarch/kernel/Makefile
+++ b/arch/loongarch/kernel/Makefile
@@ -8,7 +8,7 @@ extra-y := vmlinux.lds
obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
- alternative.o unaligned.o
+ alternative.o unaligned.o unwind.o unwind_guess.o
obj-$(CONFIG_ACPI) += acpi.o
obj-$(CONFIG_EFI) += efi.o
@@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
-obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
new file mode 100644
index 000000000000..568c6fe707d1
--- /dev/null
+++ b/arch/loongarch/kernel/unwind.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Loongson Technology Corporation Limited
+ */
+#include <asm/unwind.h>
+
+#if defined(CONFIG_UNWINDER_GUESS)
+const struct unwinder_ops *default_unwinder = &unwinder_guess;
+#elif defined(CONFIG_UNWINDER_PROLOGUE)
+const struct unwinder_ops *default_unwinder = &unwinder_prologue;
+#endif
+
+unsigned long unwind_get_return_address(struct unwind_state *state)
+{
+ if (!state->ops || unwind_done(state))
+ return 0;
+ return state->ops->unwind_get_return_address(state);
+}
+EXPORT_SYMBOL_GPL(unwind_get_return_address);
+
+void unwind_start(struct unwind_state *state, struct task_struct *task,
+ struct pt_regs *regs)
+{
+ memset(state, 0, sizeof(*state));
+ unwind_register_unwinder(state, default_unwinder);
+ if (regs) {
+ state->sp = regs->regs[3];
+ state->pc = regs->csr_era;
+ state->ra = regs->regs[1];
+ } else if (task == current) {
+ state->sp = (unsigned long)__builtin_frame_address(0);
+ state->pc = (unsigned long)__builtin_return_address(0);
+ state->ra = 0;
+ } else {
+ state->sp = thread_saved_fp(task);
+ state->pc = thread_saved_ra(task);
+ state->ra = 0;
+ }
+ state->task = task;
+ get_stack_info(state->sp, state->task, &state->stack_info);
+ state->pc = unwind_graph_addr(state, state->pc, state->sp);
+ state->ops->unwind_start(state, task, regs);
+}
+EXPORT_SYMBOL_GPL(unwind_start);
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+ if (!state->ops || unwind_done(state))
+ return false;
+ return state->ops->unwind_next_frame(state);
+}
+EXPORT_SYMBOL_GPL(unwind_next_frame);
diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
index 8ce32c37c587..b7ca2b88ac63 100644
--- a/arch/loongarch/kernel/unwind_guess.c
+++ b/arch/loongarch/kernel/unwind_guess.c
@@ -7,51 +7,23 @@
#include <asm/unwind.h>
-unsigned long unwind_get_return_address(struct unwind_state *state)
+static unsigned long get_return_address(struct unwind_state *state)
{
- if (unwind_done(state))
- return 0;
return state->pc;
}
-EXPORT_SYMBOL_GPL(unwind_get_return_address);
-void unwind_start(struct unwind_state *state, struct task_struct *task,
+static void start(struct unwind_state *state, struct task_struct *task,
struct pt_regs *regs)
{
- memset(state, 0, sizeof(*state));
-
- if (regs) {
- state->sp = regs->regs[3];
- state->pc = regs->csr_era;
- } else if (task == current) {
- state->sp = (unsigned long)__builtin_frame_address(0);
- state->pc = (unsigned long)__builtin_return_address(0);
- } else {
- state->sp = thread_saved_fp(task);
- state->pc = thread_saved_ra(task);
- }
-
- state->task = task;
- state->first = true;
- state->pc = unwind_graph_addr(state, state->pc, state->sp);
- get_stack_info(state->sp, state->task, &state->stack_info);
-
if (!unwind_done(state) && !__kernel_text_address(state->pc))
unwind_next_frame(state);
}
-EXPORT_SYMBOL_GPL(unwind_start);
-bool unwind_next_frame(struct unwind_state *state)
+static bool next_frame(struct unwind_state *state)
{
struct stack_info *info = &state->stack_info;
unsigned long addr;
- if (unwind_done(state))
- return false;
-
- if (state->first)
- state->first = false;
-
do {
for (state->sp += sizeof(unsigned long);
state->sp < info->end;
@@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
return false;
}
-EXPORT_SYMBOL_GPL(unwind_next_frame);
+
+const struct unwinder_ops unwinder_guess = {
+ .unwind_start = start,
+ .unwind_next_frame = next_frame,
+ .unwind_get_return_address = get_return_address,
+};
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index d464c533c64f..9677e13c4b4c 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -9,6 +9,8 @@
#include <asm/ptrace.h>
#include <asm/unwind.h>
+static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
+
static inline void unwind_state_fixup(struct unwind_state *state)
{
#ifdef CONFIG_DYNAMIC_FTRACE
@@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct unwind_state *state)
#endif
}
-unsigned long unwind_get_return_address(struct unwind_state *state)
+static unsigned long get_return_address(struct unwind_state *state)
{
- if (unwind_done(state))
- return 0;
return state->pc;
}
-EXPORT_SYMBOL_GPL(unwind_get_return_address);
-
-static bool unwind_by_guess(struct unwind_state *state)
-{
- struct stack_info *info = &state->stack_info;
- unsigned long addr;
-
- for (state->sp += sizeof(unsigned long);
- state->sp < info->end;
- state->sp += sizeof(unsigned long)) {
- addr = *(unsigned long *)(state->sp);
- state->pc = unwind_graph_addr(state, addr, state->sp + 8);
- if (__kernel_text_address(state->pc))
- return true;
- }
-
- return false;
-}
+/*
+ * LoongArch function prologue like follows,
+ * [others instructions not use stack var]
+ * addi.d sp, sp, -imm
+ * st.d xx, sp, offset <- save callee saved regs and
+ * st.d yy, sp, offset save ra if function is nest.
+ * [others instructions]
+ */
static bool unwind_by_prologue(struct unwind_state *state)
{
long frame_ra = -1;
@@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
ip++;
}
+ /*
+ * Not find stack alloc action, PC may be in a leaf function. Only the
+ * first being true is reasonable, otherwise indicate analysis is broken.
+ */
if (!frame_size) {
if (state->first)
goto first;
@@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
ip++;
}
+ /* Not find save $ra action, PC may be in a leaf function, too. */
if (frame_ra < 0) {
if (state->first) {
state->sp = state->sp + frame_size;
@@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct unwind_state *state)
return false;
}
- if (state->first)
- state->first = false;
-
state->pc = *(unsigned long *)(state->sp + frame_ra);
state->sp = state->sp + frame_size;
goto out;
first:
- state->first = false;
- if (state->pc == state->ra)
- return false;
-
state->pc = state->ra;
out:
+ state->first = false;
unwind_state_fixup(state);
return !!__kernel_text_address(state->pc);
}
-void unwind_start(struct unwind_state *state, struct task_struct *task,
+static void start(struct unwind_state *state, struct task_struct *task,
struct pt_regs *regs)
{
- memset(state, 0, sizeof(*state));
- state->type = UNWINDER_PROLOGUE;
-
- if (regs) {
- state->sp = regs->regs[3];
- state->pc = regs->csr_era;
- state->ra = regs->regs[1];
- if (!__kernel_text_address(state->pc))
- state->type = UNWINDER_GUESS;
- } else if (task == current) {
- state->sp = (unsigned long)__builtin_frame_address(0);
- state->pc = (unsigned long)__builtin_return_address(0);
- state->ra = 0;
- } else {
- state->sp = thread_saved_fp(task);
- state->pc = thread_saved_ra(task);
- state->ra = 0;
- }
-
- state->task = task;
state->first = true;
- state->pc = unwind_graph_addr(state, state->pc, state->sp);
- get_stack_info(state->sp, state->task, &state->stack_info);
- if (!unwind_done(state) && !__kernel_text_address(state->pc))
- unwind_next_frame(state);
+ /*
+ * The current PC is not kernel text address, we cannot find its
+ * relative symbol. Thus, prologue analysis will be broken. Luckly,
+ * we can use the guard unwinder.
+ */
+ if (!__kernel_text_address(state->pc)) {
+ unwind_register_unwinder(state, guard_unwinder);
+ if (!unwind_done(state))
+ unwind_next_frame(state);
+ }
}
-EXPORT_SYMBOL_GPL(unwind_start);
-bool unwind_next_frame(struct unwind_state *state)
+static bool next_frame(struct unwind_state *state)
{
struct stack_info *info = &state->stack_info;
struct pt_regs *regs;
unsigned long pc;
- if (unwind_done(state))
- return false;
-
do {
- switch (state->type) {
- case UNWINDER_GUESS:
- state->first = false;
- if (unwind_by_guess(state))
- return true;
- break;
-
- case UNWINDER_PROLOGUE:
- if (unwind_by_prologue(state)) {
- state->pc = unwind_graph_addr(state, state->pc, state->sp);
- return true;
- }
+ if (unwind_by_prologue(state)) {
+ state->pc = unwind_graph_addr(state, state->pc, state->sp);
+ return true;
+ }
- if (info->type == STACK_TYPE_IRQ &&
- info->end == state->sp) {
- regs = (struct pt_regs *)info->next_sp;
- pc = regs->csr_era;
+ if (info->type == STACK_TYPE_IRQ &&
+ info->end == state->sp) {
+ regs = (struct pt_regs *)info->next_sp;
+ pc = regs->csr_era;
- if (user_mode(regs) || !__kernel_text_address(pc))
- return false;
+ if (user_mode(regs) || !__kernel_text_address(pc))
+ return false;
- state->first = true;
- state->ra = regs->regs[1];
- state->sp = regs->regs[3];
- state->pc = pc;
- get_stack_info(state->sp, state->task, info);
+ state->first = true;
+ state->ra = regs->regs[1];
+ state->sp = regs->regs[3];
+ state->pc = pc;
+ get_stack_info(state->sp, state->task, info);
- return true;
- }
+ return true;
}
state->sp = info->next_sp;
@@ -212,4 +174,9 @@ bool unwind_next_frame(struct unwind_state *state)
return false;
}
-EXPORT_SYMBOL_GPL(unwind_next_frame);
+
+const struct unwinder_ops unwinder_prologue = {
+ .unwind_start = start,
+ .unwind_next_frame = next_frame,
+ .unwind_get_return_address = get_return_address,
+};
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-15 4:01 ` [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder Jinyang He
@ 2022-12-15 9:15 ` Qing Zhang
2022-12-16 1:40 ` Jinyang He
2022-12-19 3:28 ` Qing Zhang
1 sibling, 1 reply; 19+ messages in thread
From: Qing Zhang @ 2022-12-15 9:15 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Hi, Jinyang
On 2022/12/15 下午12:01, Jinyang He wrote:
> The prolugue unwinder rely on symbol info. When PC is not in kernel
> text address, it cannot find relative symbol info and it will be broken.
> The guess unwinder will be used in this case. And the guess unwinder
> codes in prolugue unwinder is redundant. Strip it out and set the
> unwinder info in unwind_state.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> arch/loongarch/include/asm/unwind.h | 22 ++++
> arch/loongarch/kernel/Makefile | 3 +-
> arch/loongarch/kernel/unwind.c | 52 +++++++++
> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
> 5 files changed, 135 insertions(+), 118 deletions(-)
> create mode 100644 arch/loongarch/kernel/unwind.c
>
> diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
> index 6ece48f0ff77..a16aff1d086a 100644
> --- a/arch/loongarch/include/asm/unwind.h
> +++ b/arch/loongarch/include/asm/unwind.h
> @@ -18,6 +18,8 @@ enum unwinder_type {
> UNWINDER_PROLOGUE,
> };
>
> +struct unwinder_ops;
> +
> struct unwind_state {
> char type; /* UNWINDER_XXX */
> struct stack_info stack_info;
> @@ -25,8 +27,22 @@ struct unwind_state {
> bool first, error, is_ftrace;
> int graph_idx;
> unsigned long sp, pc, ra;
> + const struct unwinder_ops *ops;
> +};
> +
> +struct unwinder_ops {
> + void (*unwind_start)(struct unwind_state *state,
> + struct task_struct *task, struct pt_regs *regs);
> + bool (*unwind_next_frame)(struct unwind_state *state);
> + unsigned long (*unwind_get_return_address)(struct unwind_state *state);
> };
>
> +extern const struct unwinder_ops *default_unwinder;
> +extern const struct unwinder_ops unwinder_guess;
> +#ifdef CONFIG_UNWINDER_PROLOGUE
> +extern const struct unwinder_ops unwinder_prologue;
> +#endif
> +
> void unwind_start(struct unwind_state *state,
> struct task_struct *task, struct pt_regs *regs);
> bool unwind_next_frame(struct unwind_state *state);
> @@ -49,4 +65,10 @@ static inline unsigned long unwind_graph_addr(struct unwind_state *state,
> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
> }
> +
> +static inline void unwind_register_unwinder(struct unwind_state *state,
> + const struct unwinder_ops *unwinder)
> +{
> + state->ops = unwinder;
> +}
> #endif /* _ASM_UNWIND_H */
> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
> index 7ca65195f7f8..cb6029ea3ea9 100644
> --- a/arch/loongarch/kernel/Makefile
> +++ b/arch/loongarch/kernel/Makefile
> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
> elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
> - alternative.o unaligned.o
> + alternative.o unaligned.o unwind.o unwind_guess.o
>
> obj-$(CONFIG_ACPI) += acpi.o
> obj-$(CONFIG_EFI) += efi.o
> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>
> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>
> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
> new file mode 100644
> index 000000000000..568c6fe707d1
> --- /dev/null
> +++ b/arch/loongarch/kernel/unwind.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Loongson Technology Corporation Limited
> + */
> +#include <asm/unwind.h>
> +
> +#if defined(CONFIG_UNWINDER_GUESS)
> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
> +#endif
> +
> +unsigned long unwind_get_return_address(struct unwind_state *state)
> +{
> + if (!state->ops || unwind_done(state))
> + return 0;
> + return state->ops->unwind_get_return_address(state);
> +}
> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> +
> +void unwind_start(struct unwind_state *state, struct task_struct *task,
> + struct pt_regs *regs)
> +{
> + memset(state, 0, sizeof(*state));
> + unwind_register_unwinder(state, default_unwinder);
> + if (regs) {
> + state->sp = regs->regs[3];
> + state->pc = regs->csr_era;
> + state->ra = regs->regs[1];
> + } else if (task == current) {
> + state->sp = (unsigned long)__builtin_frame_address(0);
> + state->pc = (unsigned long)__builtin_return_address(0);
> + state->ra = 0;
> + } else {
> + state->sp = thread_saved_fp(task);
> + state->pc = thread_saved_ra(task);
> + state->ra = 0;
> + }
> + state->task = task;
> + get_stack_info(state->sp, state->task, &state->stack_info);
> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
> + state->ops->unwind_start(state, task, regs);
> +}
> +EXPORT_SYMBOL_GPL(unwind_start);
> +
> +bool unwind_next_frame(struct unwind_state *state)
> +{
> + if (!state->ops || unwind_done(state))
> + return false;
> + return state->ops->unwind_next_frame(state);
> +}
> +EXPORT_SYMBOL_GPL(unwind_next_frame);
> diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
> index 8ce32c37c587..b7ca2b88ac63 100644
> --- a/arch/loongarch/kernel/unwind_guess.c
> +++ b/arch/loongarch/kernel/unwind_guess.c
> @@ -7,51 +7,23 @@
>
> #include <asm/unwind.h>
>
> -unsigned long unwind_get_return_address(struct unwind_state *state)
> +static unsigned long get_return_address(struct unwind_state *state)
> {
> - if (unwind_done(state))
> - return 0;
> return state->pc;
> }
> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>
> -void unwind_start(struct unwind_state *state, struct task_struct *task,
> +static void start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> - memset(state, 0, sizeof(*state));
> -
> - if (regs) {
> - state->sp = regs->regs[3];
> - state->pc = regs->csr_era;
> - } else if (task == current) {
> - state->sp = (unsigned long)__builtin_frame_address(0);
> - state->pc = (unsigned long)__builtin_return_address(0);
> - } else {
> - state->sp = thread_saved_fp(task);
> - state->pc = thread_saved_ra(task);
> - }
> -
> - state->task = task;
> - state->first = true;
> - state->pc = unwind_graph_add(state, state->pc, state->sp);
Do we need to unwind_graph_add again here? unwinder_guess and
unwind_prologue have already been done.
> - get_stack_info(state->sp, state->task, &state->stack_info);
> -
> if (!unwind_done(state) && !__kernel_text_address(state->pc))
> unwind_next_frame(state);
> }
> -EXPORT_SYMBOL_GPL(unwind_start);
>
> -bool unwind_next_frame(struct unwind_state *state)
> +static bool next_frame(struct unwind_state *state)
> {
> struct stack_info *info = &state->stack_info;
> unsigned long addr;
>
> - if (unwind_done(state))
> - return false;
> -
> - if (state->first)
> - state->first = false;
> -
> do {
> for (state->sp += sizeof(unsigned long);
> state->sp < info->end;
> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>
> return false;
> }
> -EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +const struct unwinder_ops unwinder_guess = {
> + .unwind_start = start,
> + .unwind_next_frame = next_frame,
> + .unwind_get_return_address = get_return_address,
> +};
> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> index d464c533c64f..9677e13c4b4c 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -9,6 +9,8 @@
> #include <asm/ptrace.h>
> #include <asm/unwind.h>
>
> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
> +
> static inline void unwind_state_fixup(struct unwind_state *state)
> {
> #ifdef CONFIG_DYNAMIC_FTRACE
> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct unwind_state *state)
> #endif
> }
>
> -unsigned long unwind_get_return_address(struct unwind_state *state)
> +static unsigned long get_return_address(struct unwind_state *state)
> {
> - if (unwind_done(state))
> - return 0;
> return state->pc;
> }
> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
> -
> -static bool unwind_by_guess(struct unwind_state *state)
> -{
> - struct stack_info *info = &state->stack_info;
> - unsigned long addr;
> -
> - for (state->sp += sizeof(unsigned long);
> - state->sp < info->end;
> - state->sp += sizeof(unsigned long)) {
> - addr = *(unsigned long *)(state->sp);
> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
> - if (__kernel_text_address(state->pc))
> - return true;
> - }
> -
> - return false;
> -}
>
> +/*
> + * LoongArch function prologue like follows,
> + * [others instructions not use stack var]
> + * addi.d sp, sp, -imm
> + * st.d xx, sp, offset <- save callee saved regs and
> + * st.d yy, sp, offset save ra if function is nest.
> + * [others instructions]
> + */
> static bool unwind_by_prologue(struct unwind_state *state)
> {
> long frame_ra = -1;
> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
> ip++;
> }
>
> + /*
> + * Not find stack alloc action, PC may be in a leaf function. Only the
> + * first being true is reasonable, otherwise indicate analysis is broken.
> + */
> if (!frame_size) {
> if (state->first)
> goto first;
> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
> ip++;
> }
>
> + /* Not find save $ra action, PC may be in a leaf function, too. */
> if (frame_ra < 0) {
> if (state->first) {
> state->sp = state->sp + frame_size;
> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct unwind_state *state)
> return false;
> }
>
> - if (state->first)
> - state->first = false;
> -
> state->pc = *(unsigned long *)(state->sp + frame_ra);
> state->sp = state->sp + frame_size;
> goto out;
>
> first:
> - state->first = false;
> - if (state->pc == state->ra)
> - return false;
> -
> state->pc = state->ra;
>
> out:
> + state->first = false;
> unwind_state_fixup(state);
> return !!__kernel_text_address(state->pc);
> }
>
> -void unwind_start(struct unwind_state *state, struct task_struct *task,
> +static void start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> - memset(state, 0, sizeof(*state));
> - state->type = UNWINDER_PROLOGUE;
> -
> - if (regs) {
> - state->sp = regs->regs[3];
> - state->pc = regs->csr_era;
> - state->ra = regs->regs[1];
> - if (!__kernel_text_address(state->pc))
> - state->type = UNWINDER_GUESS;
> - } else if (task == current) {
> - state->sp = (unsigned long)__builtin_frame_address(0);
> - state->pc = (unsigned long)__builtin_return_address(0);
> - state->ra = 0;
> - } else {
> - state->sp = thread_saved_fp(task);
> - state->pc = thread_saved_ra(task);
> - state->ra = 0;
> - }
> -
> - state->task = task;
> state->first = true;
> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
> - get_stack_info(state->sp, state->task, &state->stack_info);
>
> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
> - unwind_next_frame(state);
> + /*
> + * The current PC is not kernel text address, we cannot find its
> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
> + * we can use the guard unwinder.
> + */
> + if (!__kernel_text_address(state->pc)) {
> + unwind_register_unwinder(state, guard_unwinder);
Just add a comment here. Instead of using guard_unwinder, can we still
use guess_unwinder?
Thanks,
-Qing
> + if (!unwind_done(state))
> + unwind_next_frame(state);
> + }
> }
> -EXPORT_SYMBOL_GPL(unwind_start);
>
> -bool unwind_next_frame(struct unwind_state *state)
> +static bool next_frame(struct unwind_state *state)
> {
> struct stack_info *info = &state->stack_info;
> struct pt_regs *regs;
> unsigned long pc;
>
> - if (unwind_done(state))
> - return false;
> -
> do {
> - switch (state->type) {
> - case UNWINDER_GUESS:
> - state->first = false;
> - if (unwind_by_guess(state))
> - return true;
> - break;
> -
> - case UNWINDER_PROLOGUE:
> - if (unwind_by_prologue(state)) {
> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
> - return true;
> - }
> + if (unwind_by_prologue(state)) {
> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
> + return true;
> + }
>
> - if (info->type == STACK_TYPE_IRQ &&
> - info->end == state->sp) {
> - regs = (struct pt_regs *)info->next_sp;
> - pc = regs->csr_era;
> + if (info->type == STACK_TYPE_IRQ &&
> + info->end == state->sp) {
> + regs = (struct pt_regs *)info->next_sp;
> + pc = regs->csr_era;
>
> - if (user_mode(regs) || !__kernel_text_address(pc))
> - return false;
> + if (user_mode(regs) || !__kernel_text_address(pc))
> + return false;
>
> - state->first = true;
> - state->ra = regs->regs[1];
> - state->sp = regs->regs[3];
> - state->pc = pc;
> - get_stack_info(state->sp, state->task, info);
> + state->first = true;
> + state->ra = regs->regs[1];
> + state->sp = regs->regs[3];
> + state->pc = pc;
> + get_stack_info(state->sp, state->task, info);
>
> - return true;
> - }
> + return true;
> }
>
> state->sp = info->next_sp;
> @@ -212,4 +174,9 @@ bool unwind_next_frame(struct unwind_state *state)
>
> return false;
> }
> -EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +const struct unwinder_ops unwinder_prologue = {
> + .unwind_start = start,
> + .unwind_next_frame = next_frame,
> + .unwind_get_return_address = get_return_address,
> +};
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-15 9:15 ` Qing Zhang
@ 2022-12-16 1:40 ` Jinyang He
2022-12-16 2:14 ` Qing Zhang
0 siblings, 1 reply; 19+ messages in thread
From: Jinyang He @ 2022-12-16 1:40 UTC (permalink / raw)
To: Qing Zhang, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
On 2022-12-15 17:15, Qing Zhang wrote:
> Hi, Jinyang
>
> On 2022/12/15 下午12:01, Jinyang He wrote:
>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>> text address, it cannot find relative symbol info and it will be broken.
>> The guess unwinder will be used in this case. And the guess unwinder
>> codes in prolugue unwinder is redundant. Strip it out and set the
>> unwinder info in unwind_state.
>>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>> arch/loongarch/include/asm/unwind.h | 22 ++++
>> arch/loongarch/kernel/Makefile | 3 +-
>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
>> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
>> 5 files changed, 135 insertions(+), 118 deletions(-)
>> create mode 100644 arch/loongarch/kernel/unwind.c
>>
>> diff --git a/arch/loongarch/include/asm/unwind.h
>> b/arch/loongarch/include/asm/unwind.h
>> index 6ece48f0ff77..a16aff1d086a 100644
>> --- a/arch/loongarch/include/asm/unwind.h
>> +++ b/arch/loongarch/include/asm/unwind.h
>> @@ -18,6 +18,8 @@ enum unwinder_type {
>> UNWINDER_PROLOGUE,
>> };
>> +struct unwinder_ops;
>> +
>> struct unwind_state {
>> char type; /* UNWINDER_XXX */
>> struct stack_info stack_info;
>> @@ -25,8 +27,22 @@ struct unwind_state {
>> bool first, error, is_ftrace;
>> int graph_idx;
>> unsigned long sp, pc, ra;
>> + const struct unwinder_ops *ops;
>> +};
>> +
>> +struct unwinder_ops {
>> + void (*unwind_start)(struct unwind_state *state,
>> + struct task_struct *task, struct pt_regs *regs);
>> + bool (*unwind_next_frame)(struct unwind_state *state);
>> + unsigned long (*unwind_get_return_address)(struct unwind_state
>> *state);
>> };
>> +extern const struct unwinder_ops *default_unwinder;
>> +extern const struct unwinder_ops unwinder_guess;
>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>> +extern const struct unwinder_ops unwinder_prologue;
>> +#endif
>> +
>> void unwind_start(struct unwind_state *state,
>> struct task_struct *task, struct pt_regs *regs);
>> bool unwind_next_frame(struct unwind_state *state);
>> @@ -49,4 +65,10 @@ static inline unsigned long
>> unwind_graph_addr(struct unwind_state *state,
>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
>> }
>> +
>> +static inline void unwind_register_unwinder(struct unwind_state *state,
>> + const struct unwinder_ops *unwinder)
>> +{
>> + state->ops = unwinder;
>> +}
>> #endif /* _ASM_UNWIND_H */
>> diff --git a/arch/loongarch/kernel/Makefile
>> b/arch/loongarch/kernel/Makefile
>> index 7ca65195f7f8..cb6029ea3ea9 100644
>> --- a/arch/loongarch/kernel/Makefile
>> +++ b/arch/loongarch/kernel/Makefile
>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o
>> entry.o genex.o \
>> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o
>> switch.o \
>> elf.o syscall.o signal.o time.o topology.o inst.o
>> ptrace.o vdso.o \
>> - alternative.o unaligned.o
>> + alternative.o unaligned.o unwind.o unwind_guess.o
>> obj-$(CONFIG_ACPI) += acpi.o
>> obj-$(CONFIG_EFI) += efi.o
>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>> diff --git a/arch/loongarch/kernel/unwind.c
>> b/arch/loongarch/kernel/unwind.c
>> new file mode 100644
>> index 000000000000..568c6fe707d1
>> --- /dev/null
>> +++ b/arch/loongarch/kernel/unwind.c
>> @@ -0,0 +1,52 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>> + */
>> +#include <asm/unwind.h>
>> +
>> +#if defined(CONFIG_UNWINDER_GUESS)
>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>> +#endif
>> +
>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>> +{
>> + if (!state->ops || unwind_done(state))
>> + return 0;
>> + return state->ops->unwind_get_return_address(state);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>> +
>> +void unwind_start(struct unwind_state *state, struct task_struct *task,
>> + struct pt_regs *regs)
>> +{
>> + memset(state, 0, sizeof(*state));
>> + unwind_register_unwinder(state, default_unwinder);
>> + if (regs) {
>> + state->sp = regs->regs[3];
>> + state->pc = regs->csr_era;
>> + state->ra = regs->regs[1];
>> + } else if (task == current) {
>> + state->sp = (unsigned long)__builtin_frame_address(0);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> + state->ra = 0;
>> + } else {
>> + state->sp = thread_saved_fp(task);
>> + state->pc = thread_saved_ra(task);
>> + state->ra = 0;
>> + }
>> + state->task = task;
>> + get_stack_info(state->sp, state->task, &state->stack_info);
>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> + state->ops->unwind_start(state, task, regs);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_start);
>> +
>> +bool unwind_next_frame(struct unwind_state *state)
>> +{
>> + if (!state->ops || unwind_done(state))
>> + return false;
>> + return state->ops->unwind_next_frame(state);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>> b/arch/loongarch/kernel/unwind_guess.c
>> index 8ce32c37c587..b7ca2b88ac63 100644
>> --- a/arch/loongarch/kernel/unwind_guess.c
>> +++ b/arch/loongarch/kernel/unwind_guess.c
>> @@ -7,51 +7,23 @@
>> #include <asm/unwind.h>
>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>> +static unsigned long get_return_address(struct unwind_state *state)
>> {
>> - if (unwind_done(state))
>> - return 0;
>> return state->pc;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>> -void unwind_start(struct unwind_state *state, struct task_struct
>> *task,
>> +static void start(struct unwind_state *state, struct task_struct *task,
>> struct pt_regs *regs)
>> {
>> - memset(state, 0, sizeof(*state));
>> -
>> - if (regs) {
>> - state->sp = regs->regs[3];
>> - state->pc = regs->csr_era;
>> - } else if (task == current) {
>> - state->sp = (unsigned long)__builtin_frame_address(0);
>> - state->pc = (unsigned long)__builtin_return_address(0);
>> - } else {
>> - state->sp = thread_saved_fp(task);
>> - state->pc = thread_saved_ra(task);
>> - }
>> -
>> - state->task = task;
>> - state->first = true;
>> - state->pc = unwind_graph_add(state, state->pc, state->sp);
>
> Do we need to unwind_graph_add again here? unwinder_guess and
> unwind_prologue have already been done.
Hi, Qing
Sorry I don't what meanning here, as here is a delete line.
>
>> - get_stack_info(state->sp, state->task, &state->stack_info);
>> -
>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>> unwind_next_frame(state);
>> }
>> -EXPORT_SYMBOL_GPL(unwind_start);
>> -bool unwind_next_frame(struct unwind_state *state)
>> +static bool next_frame(struct unwind_state *state)
>> {
>> struct stack_info *info = &state->stack_info;
>> unsigned long addr;
>> - if (unwind_done(state))
>> - return false;
>> -
>> - if (state->first)
>> - state->first = false;
>> -
>> do {
>> for (state->sp += sizeof(unsigned long);
>> state->sp < info->end;
>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>> return false;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>> +
>> +const struct unwinder_ops unwinder_guess = {
>> + .unwind_start = start,
>> + .unwind_next_frame = next_frame,
>> + .unwind_get_return_address = get_return_address,
>> +};
>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>> b/arch/loongarch/kernel/unwind_prologue.c
>> index d464c533c64f..9677e13c4b4c 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -9,6 +9,8 @@
>> #include <asm/ptrace.h>
>> #include <asm/unwind.h>
>> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>> +
>> static inline void unwind_state_fixup(struct unwind_state *state)
>> {
>> #ifdef CONFIG_DYNAMIC_FTRACE
>> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct
>> unwind_state *state)
>> #endif
>> }
>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>> +static unsigned long get_return_address(struct unwind_state *state)
>> {
>> - if (unwind_done(state))
>> - return 0;
>> return state->pc;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>> -
>> -static bool unwind_by_guess(struct unwind_state *state)
>> -{
>> - struct stack_info *info = &state->stack_info;
>> - unsigned long addr;
>> -
>> - for (state->sp += sizeof(unsigned long);
>> - state->sp < info->end;
>> - state->sp += sizeof(unsigned long)) {
>> - addr = *(unsigned long *)(state->sp);
>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>> - if (__kernel_text_address(state->pc))
>> - return true;
>> - }
>> -
>> - return false;
>> -}
>> +/*
>> + * LoongArch function prologue like follows,
>> + * [others instructions not use stack var]
>> + * addi.d sp, sp, -imm
>> + * st.d xx, sp, offset <- save callee saved regs and
>> + * st.d yy, sp, offset save ra if function is nest.
>> + * [others instructions]
>> + */
>> static bool unwind_by_prologue(struct unwind_state *state)
>> {
>> long frame_ra = -1;
>> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state
>> *state)
>> ip++;
>> }
>> + /*
>> + * Not find stack alloc action, PC may be in a leaf function.
>> Only the
>> + * first being true is reasonable, otherwise indicate analysis
>> is broken.
>> + */
>> if (!frame_size) {
>> if (state->first)
>> goto first;
>> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct
>> unwind_state *state)
>> ip++;
>> }
>> + /* Not find save $ra action, PC may be in a leaf function,
>> too. */
>> if (frame_ra < 0) {
>> if (state->first) {
>> state->sp = state->sp + frame_size;
>> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct
>> unwind_state *state)
>> return false;
>> }
>> - if (state->first)
>> - state->first = false;
>> -
>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>> state->sp = state->sp + frame_size;
>> goto out;
>> first:
>> - state->first = false;
>> - if (state->pc == state->ra)
>> - return false;
>> -
>> state->pc = state->ra;
>> out:
>> + state->first = false;
>> unwind_state_fixup(state);
>> return !!__kernel_text_address(state->pc);
>> }
>> -void unwind_start(struct unwind_state *state, struct task_struct
>> *task,
>> +static void start(struct unwind_state *state, struct task_struct *task,
>> struct pt_regs *regs)
>> {
>> - memset(state, 0, sizeof(*state));
>> - state->type = UNWINDER_PROLOGUE;
>> -
>> - if (regs) {
>> - state->sp = regs->regs[3];
>> - state->pc = regs->csr_era;
>> - state->ra = regs->regs[1];
>> - if (!__kernel_text_address(state->pc))
>> - state->type = UNWINDER_GUESS;
>> - } else if (task == current) {
>> - state->sp = (unsigned long)__builtin_frame_address(0);
>> - state->pc = (unsigned long)__builtin_return_address(0);
>> - state->ra = 0;
>> - } else {
>> - state->sp = thread_saved_fp(task);
>> - state->pc = thread_saved_ra(task);
>> - state->ra = 0;
>> - }
>> -
>> - state->task = task;
>> state->first = true;
>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> - get_stack_info(state->sp, state->task, &state->stack_info);
>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>> - unwind_next_frame(state);
>> + /*
>> + * The current PC is not kernel text address, we cannot find its
>> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
>> + * we can use the guard unwinder.
>> + */
>> + if (!__kernel_text_address(state->pc)) {
>> + unwind_register_unwinder(state, guard_unwinder);
>
> Just add a comment here. Instead of using guard_unwinder, can we still
> use guess_unwinder?
Yes, and I'll use '&guess_unwinder' in next version. And I'll
drop unwind type in next version, too.
Thanks,
Jinyang
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-16 1:40 ` Jinyang He
@ 2022-12-16 2:14 ` Qing Zhang
2022-12-17 1:47 ` Jinyang He
0 siblings, 1 reply; 19+ messages in thread
From: Qing Zhang @ 2022-12-16 2:14 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Hi, Jinyang
On 2022/12/16 上午9:40, Jinyang He wrote:
> On 2022-12-15 17:15, Qing Zhang wrote:
>
>> Hi, Jinyang
>>
>> On 2022/12/15 下午12:01, Jinyang He wrote:
>>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>>> text address, it cannot find relative symbol info and it will be broken.
>>> The guess unwinder will be used in this case. And the guess unwinder
>>> codes in prolugue unwinder is redundant. Strip it out and set the
>>> unwinder info in unwind_state.
>>>
>>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>>> ---
>>> arch/loongarch/include/asm/unwind.h | 22 ++++
>>> arch/loongarch/kernel/Makefile | 3 +-
>>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>>> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
>>> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
>>> 5 files changed, 135 insertions(+), 118 deletions(-)
>>> create mode 100644 arch/loongarch/kernel/unwind.c
>>>
>>> diff --git a/arch/loongarch/include/asm/unwind.h
>>> b/arch/loongarch/include/asm/unwind.h
>>> index 6ece48f0ff77..a16aff1d086a 100644
>>> --- a/arch/loongarch/include/asm/unwind.h
>>> +++ b/arch/loongarch/include/asm/unwind.h
>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>> UNWINDER_PROLOGUE,
>>> };
>>> +struct unwinder_ops;
>>> +
>>> struct unwind_state {
>>> char type; /* UNWINDER_XXX */
>>> struct stack_info stack_info;
>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>> bool first, error, is_ftrace;
>>> int graph_idx;
>>> unsigned long sp, pc, ra;
>>> + const struct unwinder_ops *ops;
>>> +};
>>> +
>>> +struct unwinder_ops {
>>> + void (*unwind_start)(struct unwind_state *state,
>>> + struct task_struct *task, struct pt_regs *regs);
>>> + bool (*unwind_next_frame)(struct unwind_state *state);
>>> + unsigned long (*unwind_get_return_address)(struct unwind_state
>>> *state);
>>> };
>>> +extern const struct unwinder_ops *default_unwinder;
>>> +extern const struct unwinder_ops unwinder_guess;
>>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>>> +extern const struct unwinder_ops unwinder_prologue;
>>> +#endif
>>> +
>>> void unwind_start(struct unwind_state *state,
>>> struct task_struct *task, struct pt_regs *regs);
>>> bool unwind_next_frame(struct unwind_state *state);
>>> @@ -49,4 +65,10 @@ static inline unsigned long
>>> unwind_graph_addr(struct unwind_state *state,
>>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>>> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
>>> }
>>> +
>>> +static inline void unwind_register_unwinder(struct unwind_state *state,
>>> + const struct unwinder_ops *unwinder)
>>> +{
>>> + state->ops = unwinder;
>>> +}
>>> #endif /* _ASM_UNWIND_H */
>>> diff --git a/arch/loongarch/kernel/Makefile
>>> b/arch/loongarch/kernel/Makefile
>>> index 7ca65195f7f8..cb6029ea3ea9 100644
>>> --- a/arch/loongarch/kernel/Makefile
>>> +++ b/arch/loongarch/kernel/Makefile
>>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o
>>> entry.o genex.o \
>>> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o
>>> switch.o \
>>> elf.o syscall.o signal.o time.o topology.o inst.o
>>> ptrace.o vdso.o \
>>> - alternative.o unaligned.o
>>> + alternative.o unaligned.o unwind.o unwind_guess.o
>>> obj-$(CONFIG_ACPI) += acpi.o
>>> obj-$(CONFIG_EFI) += efi.o
>>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>>> diff --git a/arch/loongarch/kernel/unwind.c
>>> b/arch/loongarch/kernel/unwind.c
>>> new file mode 100644
>>> index 000000000000..568c6fe707d1
>>> --- /dev/null
>>> +++ b/arch/loongarch/kernel/unwind.c
>>> @@ -0,0 +1,52 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>>> + */
>>> +#include <asm/unwind.h>
>>> +
>>> +#if defined(CONFIG_UNWINDER_GUESS)
>>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>>> +#endif
>>> +
>>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +{
>>> + if (!state->ops || unwind_done(state))
>>> + return 0;
>>> + return state->ops->unwind_get_return_address(state);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> +
>>> +void unwind_start(struct unwind_state *state, struct task_struct *task,
>>> + struct pt_regs *regs)
>>> +{
>>> + memset(state, 0, sizeof(*state));
>>> + unwind_register_unwinder(state, default_unwinder);
>>> + if (regs) {
>>> + state->sp = regs->regs[3];
>>> + state->pc = regs->csr_era;
>>> + state->ra = regs->regs[1];
>>> + } else if (task == current) {
>>> + state->sp = (unsigned long)__builtin_frame_address(0);
>>> + state->pc = (unsigned long)__builtin_return_address(0);
>>> + state->ra = 0;
>>> + } else {
>>> + state->sp = thread_saved_fp(task);
>>> + state->pc = thread_saved_ra(task);
>>> + state->ra = 0;
>>> + }
>>> + state->task = task;
>>> + get_stack_info(state->sp, state->task, &state->stack_info);
>>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
here. :)
>>> + state->ops->unwind_start(state, task, regs);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_start);
>>> +
>>> +bool unwind_next_frame(struct unwind_state *state)
>>> +{
>>> + if (!state->ops || unwind_done(state))
>>> + return false;
>>> + return state->ops->unwind_next_frame(state);
>>> +}
>>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>>> b/arch/loongarch/kernel/unwind_guess.c
>>> index 8ce32c37c587..b7ca2b88ac63 100644
>>> --- a/arch/loongarch/kernel/unwind_guess.c
>>> +++ b/arch/loongarch/kernel/unwind_guess.c
>>> @@ -7,51 +7,23 @@
>>> #include <asm/unwind.h>
>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +static unsigned long get_return_address(struct unwind_state *state)
>>> {
>>> - if (unwind_done(state))
>>> - return 0;
>>> return state->pc;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>> *task,
>>> +static void start(struct unwind_state *state, struct task_struct *task,
>>> struct pt_regs *regs)
>>> {
>>> - memset(state, 0, sizeof(*state));
>>> -
>>> - if (regs) {
>>> - state->sp = regs->regs[3];
>>> - state->pc = regs->csr_era;
>>> - } else if (task == current) {
>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>> - } else {
>>> - state->sp = thread_saved_fp(task);
>>> - state->pc = thread_saved_ra(task);
>>> - }
>>> -
>>> - state->task = task;
>>> - state->first = true;
>>> - state->pc = unwind_graph_add(state, state->pc, state->sp);
>>
>> Do we need to unwind_graph_add again here? unwinder_guess and
>> unwind_prologue have already been done.
>
> Hi, Qing
>
>
> Sorry I don't what meanning here, as here is a delete line.
>
Sorry, It's the unwind_start up here.
>
>>
>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>> -
>>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>> unwind_next_frame(state);
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_start);
>>> -bool unwind_next_frame(struct unwind_state *state)
>>> +static bool next_frame(struct unwind_state *state)
>>> {
>>> struct stack_info *info = &state->stack_info;
>>> unsigned long addr;
>>> - if (unwind_done(state))
>>> - return false;
>>> -
>>> - if (state->first)
>>> - state->first = false;
>>> -
>>> do {
>>> for (state->sp += sizeof(unsigned long);
>>> state->sp < info->end;
>>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>> return false;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>>> +
>>> +const struct unwinder_ops unwinder_guess = {
>>> + .unwind_start = start,
>>> + .unwind_next_frame = next_frame,
>>> + .unwind_get_return_address = get_return_address,
>>> +};
>>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>>> b/arch/loongarch/kernel/unwind_prologue.c
>>> index d464c533c64f..9677e13c4b4c 100644
>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>> @@ -9,6 +9,8 @@
>>> #include <asm/ptrace.h>
>>> #include <asm/unwind.h>
>>> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>> +
>>> static inline void unwind_state_fixup(struct unwind_state *state)
>>> {
>>> #ifdef CONFIG_DYNAMIC_FTRACE
>>> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct
>>> unwind_state *state)
>>> #endif
>>> }
>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>> +static unsigned long get_return_address(struct unwind_state *state)
>>> {
>>> - if (unwind_done(state))
>>> - return 0;
>>> return state->pc;
>>> }
>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>> -
>>> -static bool unwind_by_guess(struct unwind_state *state)
>>> -{
>>> - struct stack_info *info = &state->stack_info;
>>> - unsigned long addr;
>>> -
>>> - for (state->sp += sizeof(unsigned long);
>>> - state->sp < info->end;
>>> - state->sp += sizeof(unsigned long)) {
>>> - addr = *(unsigned long *)(state->sp);
>>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>>> - if (__kernel_text_address(state->pc))
>>> - return true;
>>> - }
>>> -
>>> - return false;
>>> -}
>>> +/*
>>> + * LoongArch function prologue like follows,
>>> + * [others instructions not use stack var]
>>> + * addi.d sp, sp, -imm
>>> + * st.d xx, sp, offset <- save callee saved regs and
>>> + * st.d yy, sp, offset save ra if function is nest.
>>> + * [others instructions]
>>> + */
>>> static bool unwind_by_prologue(struct unwind_state *state)
>>> {
>>> long frame_ra = -1;
>>> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state
>>> *state)
>>> ip++;
>>> }
>>> + /*
>>> + * Not find stack alloc action, PC may be in a leaf function.
>>> Only the
>>> + * first being true is reasonable, otherwise indicate analysis
>>> is broken.
>>> + */
>>> if (!frame_size) {
>>> if (state->first)
>>> goto first;
>>> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct
>>> unwind_state *state)
>>> ip++;
>>> }
>>> + /* Not find save $ra action, PC may be in a leaf function,
>>> too. */
>>> if (frame_ra < 0) {
>>> if (state->first) {
>>> state->sp = state->sp + frame_size;
>>> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct
>>> unwind_state *state)
>>> return false;
>>> }
>>> - if (state->first)
>>> - state->first = false;
>>> -
>>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>>> state->sp = state->sp + frame_size;
>>> goto out;
>>> first:
>>> - state->first = false;
>>> - if (state->pc == state->ra)
>>> - return false;
>>> -
>>> state->pc = state->ra;
>>> out:
>>> + state->first = false;
>>> unwind_state_fixup(state);
>>> return !!__kernel_text_address(state->pc);
>>> }
>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>> *task,
>>> +static void start(struct unwind_state *state, struct task_struct *task,
>>> struct pt_regs *regs)
>>> {
>>> - memset(state, 0, sizeof(*state));
>>> - state->type = UNWINDER_PROLOGUE;
>>> -
>>> - if (regs) {
>>> - state->sp = regs->regs[3];
>>> - state->pc = regs->csr_era;
>>> - state->ra = regs->regs[1];
>>> - if (!__kernel_text_address(state->pc))
>>> - state->type = UNWINDER_GUESS;
>>> - } else if (task == current) {
>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>> - state->ra = 0;
>>> - } else {
>>> - state->sp = thread_saved_fp(task);
>>> - state->pc = thread_saved_ra(task);
>>> - state->ra = 0;
>>> - }
>>> -
>>> - state->task = task;
>>> state->first = true;
>>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>> - unwind_next_frame(state);
>>> + /*
>>> + * The current PC is not kernel text address, we cannot find its
>>> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
>>> + * we can use the guard unwinder.
>>> + */
>>> + if (!__kernel_text_address(state->pc)) {
>>> + unwind_register_unwinder(state, guard_unwinder);
>>
>> Just add a comment here. Instead of using guard_unwinder, can we still
>> use guess_unwinder?
>
> Yes, and I'll use '&guess_unwinder' in next version. And I'll
> drop unwind type in next version, too.
>
>
> Thanks,
>
> Jinyang
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-16 2:14 ` Qing Zhang
@ 2022-12-17 1:47 ` Jinyang He
2022-12-17 2:48 ` Qing Zhang
0 siblings, 1 reply; 19+ messages in thread
From: Jinyang He @ 2022-12-17 1:47 UTC (permalink / raw)
To: Qing Zhang, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
On 2022-12-16 10:14, Qing Zhang wrote:
> Hi, Jinyang
>
> On 2022/12/16 上午9:40, Jinyang He wrote:
>> On 2022-12-15 17:15, Qing Zhang wrote:
>>
>>> Hi, Jinyang
>>>
>>> On 2022/12/15 下午12:01, Jinyang He wrote:
>>>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>>>> text address, it cannot find relative symbol info and it will be
>>>> broken.
>>>> The guess unwinder will be used in this case. And the guess unwinder
>>>> codes in prolugue unwinder is redundant. Strip it out and set the
>>>> unwinder info in unwind_state.
>>>>
>>>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>>>> ---
>>>> arch/loongarch/include/asm/unwind.h | 22 ++++
>>>> arch/loongarch/kernel/Makefile | 3 +-
>>>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>>>> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
>>>> arch/loongarch/kernel/unwind_prologue.c | 135
>>>> +++++++++---------------
>>>> 5 files changed, 135 insertions(+), 118 deletions(-)
>>>> create mode 100644 arch/loongarch/kernel/unwind.c
>>>>
>>>> diff --git a/arch/loongarch/include/asm/unwind.h
>>>> b/arch/loongarch/include/asm/unwind.h
>>>> index 6ece48f0ff77..a16aff1d086a 100644
>>>> --- a/arch/loongarch/include/asm/unwind.h
>>>> +++ b/arch/loongarch/include/asm/unwind.h
>>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>>> UNWINDER_PROLOGUE,
>>>> };
>>>> +struct unwinder_ops;
>>>> +
>>>> struct unwind_state {
>>>> char type; /* UNWINDER_XXX */
>>>> struct stack_info stack_info;
>>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>>> bool first, error, is_ftrace;
>>>> int graph_idx;
>>>> unsigned long sp, pc, ra;
>>>> + const struct unwinder_ops *ops;
>>>> +};
>>>> +
>>>> +struct unwinder_ops {
>>>> + void (*unwind_start)(struct unwind_state *state,
>>>> + struct task_struct *task, struct pt_regs *regs);
>>>> + bool (*unwind_next_frame)(struct unwind_state *state);
>>>> + unsigned long (*unwind_get_return_address)(struct unwind_state
>>>> *state);
>>>> };
>>>> +extern const struct unwinder_ops *default_unwinder;
>>>> +extern const struct unwinder_ops unwinder_guess;
>>>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>>>> +extern const struct unwinder_ops unwinder_prologue;
>>>> +#endif
>>>> +
>>>> void unwind_start(struct unwind_state *state,
>>>> struct task_struct *task, struct pt_regs *regs);
>>>> bool unwind_next_frame(struct unwind_state *state);
>>>> @@ -49,4 +65,10 @@ static inline unsigned long
>>>> unwind_graph_addr(struct unwind_state *state,
>>>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>>>> pc, (unsigned long *)(cfa -
>>>> GRAPH_FAKE_OFFSET));
>>>> }
>>>> +
>>>> +static inline void unwind_register_unwinder(struct unwind_state
>>>> *state,
>>>> + const struct unwinder_ops *unwinder)
>>>> +{
>>>> + state->ops = unwinder;
>>>> +}
>>>> #endif /* _ASM_UNWIND_H */
>>>> diff --git a/arch/loongarch/kernel/Makefile
>>>> b/arch/loongarch/kernel/Makefile
>>>> index 7ca65195f7f8..cb6029ea3ea9 100644
>>>> --- a/arch/loongarch/kernel/Makefile
>>>> +++ b/arch/loongarch/kernel/Makefile
>>>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>>>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o
>>>> entry.o genex.o \
>>>> traps.o irq.o idle.o process.o dma.o mem.o io.o
>>>> reset.o switch.o \
>>>> elf.o syscall.o signal.o time.o topology.o inst.o
>>>> ptrace.o vdso.o \
>>>> - alternative.o unaligned.o
>>>> + alternative.o unaligned.o unwind.o unwind_guess.o
>>>> obj-$(CONFIG_ACPI) += acpi.o
>>>> obj-$(CONFIG_EFI) += efi.o
>>>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>>>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>>>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>>>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>>>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>>>> diff --git a/arch/loongarch/kernel/unwind.c
>>>> b/arch/loongarch/kernel/unwind.c
>>>> new file mode 100644
>>>> index 000000000000..568c6fe707d1
>>>> --- /dev/null
>>>> +++ b/arch/loongarch/kernel/unwind.c
>>>> @@ -0,0 +1,52 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>>>> + */
>>>> +#include <asm/unwind.h>
>>>> +
>>>> +#if defined(CONFIG_UNWINDER_GUESS)
>>>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>>>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>>>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>>>> +#endif
>>>> +
>>>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>>>> +{
>>>> + if (!state->ops || unwind_done(state))
>>>> + return 0;
>>>> + return state->ops->unwind_get_return_address(state);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>> +
>>>> +void unwind_start(struct unwind_state *state, struct task_struct
>>>> *task,
>>>> + struct pt_regs *regs)
>>>> +{
>>>> + memset(state, 0, sizeof(*state));
>>>> + unwind_register_unwinder(state, default_unwinder);
>>>> + if (regs) {
>>>> + state->sp = regs->regs[3];
>>>> + state->pc = regs->csr_era;
>>>> + state->ra = regs->regs[1];
>>>> + } else if (task == current) {
>>>> + state->sp = (unsigned long)__builtin_frame_address(0);
>>>> + state->pc = (unsigned long)__builtin_return_address(0);
>>>> + state->ra = 0;
>>>> + } else {
>>>> + state->sp = thread_saved_fp(task);
>>>> + state->pc = thread_saved_ra(task);
>>>> + state->ra = 0;
>>>> + }
>>>> + state->task = task;
>>>> + get_stack_info(state->sp, state->task, &state->stack_info);
>>>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
>
> here. :)
The PC from branch 'task == current' is the RA of current frame. And the
PC from branch 'regs' is regs->csr_era, which may be RA of frame where
to get the regs->csr_era. They all may traced by function_graph.
Ftrace_graph_ret_addr() returns orignal addr and not destroys result, if
the addr was not traced, so calling it for deal with above cases.
Thanks,
Jinyang
>
>>>> + state->ops->unwind_start(state, task, regs);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(unwind_start);
>>>> +
>>>> +bool unwind_next_frame(struct unwind_state *state)
>>>> +{
>>>> + if (!state->ops || unwind_done(state))
>>>> + return false;
>>>> + return state->ops->unwind_next_frame(state);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>>>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>>>> b/arch/loongarch/kernel/unwind_guess.c
>>>> index 8ce32c37c587..b7ca2b88ac63 100644
>>>> --- a/arch/loongarch/kernel/unwind_guess.c
>>>> +++ b/arch/loongarch/kernel/unwind_guess.c
>>>> @@ -7,51 +7,23 @@
>>>> #include <asm/unwind.h>
>>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>>> +static unsigned long get_return_address(struct unwind_state *state)
>>>> {
>>>> - if (unwind_done(state))
>>>> - return 0;
>>>> return state->pc;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>>> *task,
>>>> +static void start(struct unwind_state *state, struct task_struct
>>>> *task,
>>>> struct pt_regs *regs)
>>>> {
>>>> - memset(state, 0, sizeof(*state));
>>>> -
>>>> - if (regs) {
>>>> - state->sp = regs->regs[3];
>>>> - state->pc = regs->csr_era;
>>>> - } else if (task == current) {
>>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>>> - } else {
>>>> - state->sp = thread_saved_fp(task);
>>>> - state->pc = thread_saved_ra(task);
>>>> - }
>>>> -
>>>> - state->task = task;
>>>> - state->first = true;
>>>> - state->pc = unwind_graph_add(state, state->pc, state->sp);
>>>
>>> Do we need to unwind_graph_add again here? unwinder_guess and
>>> unwind_prologue have already been done.
>>
>> Hi, Qing
>>
>>
>> Sorry I don't what meanning here, as here is a delete line.
>>
> Sorry, It's the unwind_start up here.
>>
>>>
>>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>>> -
>>>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>>> unwind_next_frame(state);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(unwind_start);
>>>> -bool unwind_next_frame(struct unwind_state *state)
>>>> +static bool next_frame(struct unwind_state *state)
>>>> {
>>>> struct stack_info *info = &state->stack_info;
>>>> unsigned long addr;
>>>> - if (unwind_done(state))
>>>> - return false;
>>>> -
>>>> - if (state->first)
>>>> - state->first = false;
>>>> -
>>>> do {
>>>> for (state->sp += sizeof(unsigned long);
>>>> state->sp < info->end;
>>>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>>> return false;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>>>> +
>>>> +const struct unwinder_ops unwinder_guess = {
>>>> + .unwind_start = start,
>>>> + .unwind_next_frame = next_frame,
>>>> + .unwind_get_return_address = get_return_address,
>>>> +};
>>>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>>>> b/arch/loongarch/kernel/unwind_prologue.c
>>>> index d464c533c64f..9677e13c4b4c 100644
>>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>>> @@ -9,6 +9,8 @@
>>>> #include <asm/ptrace.h>
>>>> #include <asm/unwind.h>
>>>> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>>> +
>>>> static inline void unwind_state_fixup(struct unwind_state *state)
>>>> {
>>>> #ifdef CONFIG_DYNAMIC_FTRACE
>>>> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct
>>>> unwind_state *state)
>>>> #endif
>>>> }
>>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>>> +static unsigned long get_return_address(struct unwind_state *state)
>>>> {
>>>> - if (unwind_done(state))
>>>> - return 0;
>>>> return state->pc;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>> -
>>>> -static bool unwind_by_guess(struct unwind_state *state)
>>>> -{
>>>> - struct stack_info *info = &state->stack_info;
>>>> - unsigned long addr;
>>>> -
>>>> - for (state->sp += sizeof(unsigned long);
>>>> - state->sp < info->end;
>>>> - state->sp += sizeof(unsigned long)) {
>>>> - addr = *(unsigned long *)(state->sp);
>>>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>>>> - if (__kernel_text_address(state->pc))
>>>> - return true;
>>>> - }
>>>> -
>>>> - return false;
>>>> -}
>>>> +/*
>>>> + * LoongArch function prologue like follows,
>>>> + * [others instructions not use stack var]
>>>> + * addi.d sp, sp, -imm
>>>> + * st.d xx, sp, offset <- save callee saved regs and
>>>> + * st.d yy, sp, offset save ra if function is nest.
>>>> + * [others instructions]
>>>> + */
>>>> static bool unwind_by_prologue(struct unwind_state *state)
>>>> {
>>>> long frame_ra = -1;
>>>> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct
>>>> unwind_state *state)
>>>> ip++;
>>>> }
>>>> + /*
>>>> + * Not find stack alloc action, PC may be in a leaf function.
>>>> Only the
>>>> + * first being true is reasonable, otherwise indicate analysis
>>>> is broken.
>>>> + */
>>>> if (!frame_size) {
>>>> if (state->first)
>>>> goto first;
>>>> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct
>>>> unwind_state *state)
>>>> ip++;
>>>> }
>>>> + /* Not find save $ra action, PC may be in a leaf function,
>>>> too. */
>>>> if (frame_ra < 0) {
>>>> if (state->first) {
>>>> state->sp = state->sp + frame_size;
>>>> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct
>>>> unwind_state *state)
>>>> return false;
>>>> }
>>>> - if (state->first)
>>>> - state->first = false;
>>>> -
>>>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>>>> state->sp = state->sp + frame_size;
>>>> goto out;
>>>> first:
>>>> - state->first = false;
>>>> - if (state->pc == state->ra)
>>>> - return false;
>>>> -
>>>> state->pc = state->ra;
>>>> out:
>>>> + state->first = false;
>>>> unwind_state_fixup(state);
>>>> return !!__kernel_text_address(state->pc);
>>>> }
>>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>>> *task,
>>>> +static void start(struct unwind_state *state, struct task_struct
>>>> *task,
>>>> struct pt_regs *regs)
>>>> {
>>>> - memset(state, 0, sizeof(*state));
>>>> - state->type = UNWINDER_PROLOGUE;
>>>> -
>>>> - if (regs) {
>>>> - state->sp = regs->regs[3];
>>>> - state->pc = regs->csr_era;
>>>> - state->ra = regs->regs[1];
>>>> - if (!__kernel_text_address(state->pc))
>>>> - state->type = UNWINDER_GUESS;
>>>> - } else if (task == current) {
>>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>>> - state->ra = 0;
>>>> - } else {
>>>> - state->sp = thread_saved_fp(task);
>>>> - state->pc = thread_saved_ra(task);
>>>> - state->ra = 0;
>>>> - }
>>>> -
>>>> - state->task = task;
>>>> state->first = true;
>>>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>>> - unwind_next_frame(state);
>>>> + /*
>>>> + * The current PC is not kernel text address, we cannot find its
>>>> + * relative symbol. Thus, prologue analysis will be broken.
>>>> Luckly,
>>>> + * we can use the guard unwinder.
>>>> + */
>>>> + if (!__kernel_text_address(state->pc)) {
>>>> + unwind_register_unwinder(state, guard_unwinder);
>>>
>>> Just add a comment here. Instead of using guard_unwinder, can we still
>>> use guess_unwinder?
>>
>> Yes, and I'll use '&guess_unwinder' in next version. And I'll
>> drop unwind type in next version, too.
>>
>>
>> Thanks,
>>
>> Jinyang
>>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-17 1:47 ` Jinyang He
@ 2022-12-17 2:48 ` Qing Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Qing Zhang @ 2022-12-17 2:48 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
On 2022/12/17 上午9:47, Jinyang He wrote:
> On 2022-12-16 10:14, Qing Zhang wrote:
>
>> Hi, Jinyang
>>
>> On 2022/12/16 上午9:40, Jinyang He wrote:
>>> On 2022-12-15 17:15, Qing Zhang wrote:
>>>
>>>> Hi, Jinyang
>>>>
>>>> On 2022/12/15 下午12:01, Jinyang He wrote:
>>>>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>>>>> text address, it cannot find relative symbol info and it will be
>>>>> broken.
>>>>> The guess unwinder will be used in this case. And the guess unwinder
>>>>> codes in prolugue unwinder is redundant. Strip it out and set the
>>>>> unwinder info in unwind_state.
>>>>>
>>>>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>>>>> ---
>>>>> arch/loongarch/include/asm/unwind.h | 22 ++++
>>>>> arch/loongarch/kernel/Makefile | 3 +-
>>>>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>>>>> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
>>>>> arch/loongarch/kernel/unwind_prologue.c | 135
>>>>> +++++++++---------------
>>>>> 5 files changed, 135 insertions(+), 118 deletions(-)
>>>>> create mode 100644 arch/loongarch/kernel/unwind.c
>>>>>
>>>>> diff --git a/arch/loongarch/include/asm/unwind.h
>>>>> b/arch/loongarch/include/asm/unwind.h
>>>>> index 6ece48f0ff77..a16aff1d086a 100644
>>>>> --- a/arch/loongarch/include/asm/unwind.h
>>>>> +++ b/arch/loongarch/include/asm/unwind.h
>>>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>>>> UNWINDER_PROLOGUE,
>>>>> };
>>>>> +struct unwinder_ops;
>>>>> +
>>>>> struct unwind_state {
>>>>> char type; /* UNWINDER_XXX */
>>>>> struct stack_info stack_info;
>>>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>>>> bool first, error, is_ftrace;
>>>>> int graph_idx;
>>>>> unsigned long sp, pc, ra;
>>>>> + const struct unwinder_ops *ops;
>>>>> +};
>>>>> +
>>>>> +struct unwinder_ops {
>>>>> + void (*unwind_start)(struct unwind_state *state,
>>>>> + struct task_struct *task, struct pt_regs *regs);
>>>>> + bool (*unwind_next_frame)(struct unwind_state *state);
>>>>> + unsigned long (*unwind_get_return_address)(struct unwind_state
>>>>> *state);
>>>>> };
>>>>> +extern const struct unwinder_ops *default_unwinder;
>>>>> +extern const struct unwinder_ops unwinder_guess;
>>>>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>>>>> +extern const struct unwinder_ops unwinder_prologue;
>>>>> +#endif
>>>>> +
>>>>> void unwind_start(struct unwind_state *state,
>>>>> struct task_struct *task, struct pt_regs *regs);
>>>>> bool unwind_next_frame(struct unwind_state *state);
>>>>> @@ -49,4 +65,10 @@ static inline unsigned long
>>>>> unwind_graph_addr(struct unwind_state *state,
>>>>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>>>>> pc, (unsigned long *)(cfa -
>>>>> GRAPH_FAKE_OFFSET));
>>>>> }
>>>>> +
>>>>> +static inline void unwind_register_unwinder(struct unwind_state
>>>>> *state,
>>>>> + const struct unwinder_ops *unwinder)
>>>>> +{
>>>>> + state->ops = unwinder;
>>>>> +}
>>>>> #endif /* _ASM_UNWIND_H */
>>>>> diff --git a/arch/loongarch/kernel/Makefile
>>>>> b/arch/loongarch/kernel/Makefile
>>>>> index 7ca65195f7f8..cb6029ea3ea9 100644
>>>>> --- a/arch/loongarch/kernel/Makefile
>>>>> +++ b/arch/loongarch/kernel/Makefile
>>>>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>>>>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o
>>>>> entry.o genex.o \
>>>>> traps.o irq.o idle.o process.o dma.o mem.o io.o
>>>>> reset.o switch.o \
>>>>> elf.o syscall.o signal.o time.o topology.o inst.o
>>>>> ptrace.o vdso.o \
>>>>> - alternative.o unaligned.o
>>>>> + alternative.o unaligned.o unwind.o unwind_guess.o
>>>>> obj-$(CONFIG_ACPI) += acpi.o
>>>>> obj-$(CONFIG_EFI) += efi.o
>>>>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>>>>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>>>>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>>>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>>>>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>>>>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>>>>> diff --git a/arch/loongarch/kernel/unwind.c
>>>>> b/arch/loongarch/kernel/unwind.c
>>>>> new file mode 100644
>>>>> index 000000000000..568c6fe707d1
>>>>> --- /dev/null
>>>>> +++ b/arch/loongarch/kernel/unwind.c
>>>>> @@ -0,0 +1,52 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>>>>> + */
>>>>> +#include <asm/unwind.h>
>>>>> +
>>>>> +#if defined(CONFIG_UNWINDER_GUESS)
>>>>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>>>>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>>>>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>>>>> +#endif
>>>>> +
>>>>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>>>>> +{
>>>>> + if (!state->ops || unwind_done(state))
>>>>> + return 0;
>>>>> + return state->ops->unwind_get_return_address(state);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>>> +
>>>>> +void unwind_start(struct unwind_state *state, struct task_struct
>>>>> *task,
>>>>> + struct pt_regs *regs)
>>>>> +{
>>>>> + memset(state, 0, sizeof(*state));
>>>>> + unwind_register_unwinder(state, default_unwinder);
>>>>> + if (regs) {
>>>>> + state->sp = regs->regs[3];
>>>>> + state->pc = regs->csr_era;
>>>>> + state->ra = regs->regs[1];
>>>>> + } else if (task == current) {
>>>>> + state->sp = (unsigned long)__builtin_frame_address(0);
>>>>> + state->pc = (unsigned long)__builtin_return_address(0);
>>>>> + state->ra = 0;
>>>>> + } else {
>>>>> + state->sp = thread_saved_fp(task);
>>>>> + state->pc = thread_saved_ra(task);
>>>>> + state->ra = 0;
>>>>> + }
>>>>> + state->task = task;
>>>>> + get_stack_info(state->sp, state->task, &state->stack_info);
>>>>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
>>
>> here. :)
>
> The PC from branch 'task == current' is the RA of current frame. And the
> PC from branch 'regs' is regs->csr_era, which may be RA of frame where
> to get the regs->csr_era. They all may traced by function_graph.
> Ftrace_graph_ret_addr() returns orignal addr and not destroys result, if
> the addr was not traced, so calling it for deal with above cases.
>
ok, you are right. The first state>pc obtained by cat/proc/self/stack
after the function graph is a "return to handler".
Thank,
-Qing
> Thanks,
>
> Jinyang
>
>>
>>>>> + state->ops->unwind_start(state, task, regs);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(unwind_start);
>>>>> +
>>>>> +bool unwind_next_frame(struct unwind_state *state)
>>>>> +{
>>>>> + if (!state->ops || unwind_done(state))
>>>>> + return false;
>>>>> + return state->ops->unwind_next_frame(state);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>>>>> diff --git a/arch/loongarch/kernel/unwind_guess.c
>>>>> b/arch/loongarch/kernel/unwind_guess.c
>>>>> index 8ce32c37c587..b7ca2b88ac63 100644
>>>>> --- a/arch/loongarch/kernel/unwind_guess.c
>>>>> +++ b/arch/loongarch/kernel/unwind_guess.c
>>>>> @@ -7,51 +7,23 @@
>>>>> #include <asm/unwind.h>
>>>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>>>> +static unsigned long get_return_address(struct unwind_state *state)
>>>>> {
>>>>> - if (unwind_done(state))
>>>>> - return 0;
>>>>> return state->pc;
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>>>> *task,
>>>>> +static void start(struct unwind_state *state, struct task_struct
>>>>> *task,
>>>>> struct pt_regs *regs)
>>>>> {
>>>>> - memset(state, 0, sizeof(*state));
>>>>> -
>>>>> - if (regs) {
>>>>> - state->sp = regs->regs[3];
>>>>> - state->pc = regs->csr_era;
>>>>> - } else if (task == current) {
>>>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>>>> - } else {
>>>>> - state->sp = thread_saved_fp(task);
>>>>> - state->pc = thread_saved_ra(task);
>>>>> - }
>>>>> -
>>>>> - state->task = task;
>>>>> - state->first = true;
>>>>> - state->pc = unwind_graph_add(state, state->pc, state->sp);
>>>>
>>>> Do we need to unwind_graph_add again here? unwinder_guess and
>>>> unwind_prologue have already been done.
>>>
>>> Hi, Qing
>>>
>>>
>>> Sorry I don't what meanning here, as here is a delete line.
>>>
>> Sorry, It's the unwind_start up here.
>>>
>>>>
>>>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>>>> -
>>>>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>>>> unwind_next_frame(state);
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(unwind_start);
>>>>> -bool unwind_next_frame(struct unwind_state *state)
>>>>> +static bool next_frame(struct unwind_state *state)
>>>>> {
>>>>> struct stack_info *info = &state->stack_info;
>>>>> unsigned long addr;
>>>>> - if (unwind_done(state))
>>>>> - return false;
>>>>> -
>>>>> - if (state->first)
>>>>> - state->first = false;
>>>>> -
>>>>> do {
>>>>> for (state->sp += sizeof(unsigned long);
>>>>> state->sp < info->end;
>>>>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>>>> return false;
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>>>>> +
>>>>> +const struct unwinder_ops unwinder_guess = {
>>>>> + .unwind_start = start,
>>>>> + .unwind_next_frame = next_frame,
>>>>> + .unwind_get_return_address = get_return_address,
>>>>> +};
>>>>> diff --git a/arch/loongarch/kernel/unwind_prologue.c
>>>>> b/arch/loongarch/kernel/unwind_prologue.c
>>>>> index d464c533c64f..9677e13c4b4c 100644
>>>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>>>> @@ -9,6 +9,8 @@
>>>>> #include <asm/ptrace.h>
>>>>> #include <asm/unwind.h>
>>>>> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>>>> +
>>>>> static inline void unwind_state_fixup(struct unwind_state *state)
>>>>> {
>>>>> #ifdef CONFIG_DYNAMIC_FTRACE
>>>>> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct
>>>>> unwind_state *state)
>>>>> #endif
>>>>> }
>>>>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>>>>> +static unsigned long get_return_address(struct unwind_state *state)
>>>>> {
>>>>> - if (unwind_done(state))
>>>>> - return 0;
>>>>> return state->pc;
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>>>> -
>>>>> -static bool unwind_by_guess(struct unwind_state *state)
>>>>> -{
>>>>> - struct stack_info *info = &state->stack_info;
>>>>> - unsigned long addr;
>>>>> -
>>>>> - for (state->sp += sizeof(unsigned long);
>>>>> - state->sp < info->end;
>>>>> - state->sp += sizeof(unsigned long)) {
>>>>> - addr = *(unsigned long *)(state->sp);
>>>>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>>>>> - if (__kernel_text_address(state->pc))
>>>>> - return true;
>>>>> - }
>>>>> -
>>>>> - return false;
>>>>> -}
>>>>> +/*
>>>>> + * LoongArch function prologue like follows,
>>>>> + * [others instructions not use stack var]
>>>>> + * addi.d sp, sp, -imm
>>>>> + * st.d xx, sp, offset <- save callee saved regs and
>>>>> + * st.d yy, sp, offset save ra if function is nest.
>>>>> + * [others instructions]
>>>>> + */
>>>>> static bool unwind_by_prologue(struct unwind_state *state)
>>>>> {
>>>>> long frame_ra = -1;
>>>>> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct
>>>>> unwind_state *state)
>>>>> ip++;
>>>>> }
>>>>> + /*
>>>>> + * Not find stack alloc action, PC may be in a leaf function.
>>>>> Only the
>>>>> + * first being true is reasonable, otherwise indicate analysis
>>>>> is broken.
>>>>> + */
>>>>> if (!frame_size) {
>>>>> if (state->first)
>>>>> goto first;
>>>>> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct
>>>>> unwind_state *state)
>>>>> ip++;
>>>>> }
>>>>> + /* Not find save $ra action, PC may be in a leaf function,
>>>>> too. */
>>>>> if (frame_ra < 0) {
>>>>> if (state->first) {
>>>>> state->sp = state->sp + frame_size;
>>>>> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct
>>>>> unwind_state *state)
>>>>> return false;
>>>>> }
>>>>> - if (state->first)
>>>>> - state->first = false;
>>>>> -
>>>>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>>>>> state->sp = state->sp + frame_size;
>>>>> goto out;
>>>>> first:
>>>>> - state->first = false;
>>>>> - if (state->pc == state->ra)
>>>>> - return false;
>>>>> -
>>>>> state->pc = state->ra;
>>>>> out:
>>>>> + state->first = false;
>>>>> unwind_state_fixup(state);
>>>>> return !!__kernel_text_address(state->pc);
>>>>> }
>>>>> -void unwind_start(struct unwind_state *state, struct task_struct
>>>>> *task,
>>>>> +static void start(struct unwind_state *state, struct task_struct
>>>>> *task,
>>>>> struct pt_regs *regs)
>>>>> {
>>>>> - memset(state, 0, sizeof(*state));
>>>>> - state->type = UNWINDER_PROLOGUE;
>>>>> -
>>>>> - if (regs) {
>>>>> - state->sp = regs->regs[3];
>>>>> - state->pc = regs->csr_era;
>>>>> - state->ra = regs->regs[1];
>>>>> - if (!__kernel_text_address(state->pc))
>>>>> - state->type = UNWINDER_GUESS;
>>>>> - } else if (task == current) {
>>>>> - state->sp = (unsigned long)__builtin_frame_address(0);
>>>>> - state->pc = (unsigned long)__builtin_return_address(0);
>>>>> - state->ra = 0;
>>>>> - } else {
>>>>> - state->sp = thread_saved_fp(task);
>>>>> - state->pc = thread_saved_ra(task);
>>>>> - state->ra = 0;
>>>>> - }
>>>>> -
>>>>> - state->task = task;
>>>>> state->first = true;
>>>>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>>>>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>>>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>>>>> - unwind_next_frame(state);
>>>>> + /*
>>>>> + * The current PC is not kernel text address, we cannot find its
>>>>> + * relative symbol. Thus, prologue analysis will be broken.
>>>>> Luckly,
>>>>> + * we can use the guard unwinder.
>>>>> + */
>>>>> + if (!__kernel_text_address(state->pc)) {
>>>>> + unwind_register_unwinder(state, guard_unwinder);
>>>>
>>>> Just add a comment here. Instead of using guard_unwinder, can we still
>>>> use guess_unwinder?
>>>
>>> Yes, and I'll use '&guess_unwinder' in next version. And I'll
>>> drop unwind type in next version, too.
>>>
>>>
>>> Thanks,
>>>
>>> Jinyang
>>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
2022-12-15 4:01 ` [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder Jinyang He
2022-12-15 9:15 ` Qing Zhang
@ 2022-12-19 3:28 ` Qing Zhang
1 sibling, 0 replies; 19+ messages in thread
From: Qing Zhang @ 2022-12-19 3:28 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Hi, Jinyang
On 2022/12/15 下午12:01, Jinyang He wrote:
> The prolugue unwinder rely on symbol info. When PC is not in kernel
> text address, it cannot find relative symbol info and it will be broken.
> The guess unwinder will be used in this case. And the guess unwinder
> codes in prolugue unwinder is redundant. Strip it out and set the
> unwinder info in unwind_state.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> arch/loongarch/include/asm/unwind.h | 22 ++++
> arch/loongarch/kernel/Makefile | 3 +-
> arch/loongarch/kernel/unwind.c | 52 +++++++++
> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
> 5 files changed, 135 insertions(+), 118 deletions(-)
> create mode 100644 arch/loongarch/kernel/unwind.c
>
> diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
> index 6ece48f0ff77..a16aff1d086a 100644
> --- a/arch/loongarch/include/asm/unwind.h
> +++ b/arch/loongarch/include/asm/unwind.h
> @@ -18,6 +18,8 @@ enum unwinder_type {
> UNWINDER_PROLOGUE,
> };
>
> +struct unwinder_ops;
> +
> struct unwind_state {
> char type; /* UNWINDER_XXX *
we don't need type anymore.
Thanks,
-Qing
> struct stack_info stack_info;
> @@ -25,8 +27,22 @@ struct unwind_state {
> bool first, error, is_ftrace;
> int graph_idx;
> unsigned long sp, pc, ra;
> + const struct unwinder_ops *ops;
> +};
> +
> +struct unwinder_ops {
> + void (*unwind_start)(struct unwind_state *state,
> + struct task_struct *task, struct pt_regs *regs);
> + bool (*unwind_next_frame)(struct unwind_state *state);
> + unsigned long (*unwind_get_return_address)(struct unwind_state *state);
> };
>
> +extern const struct unwinder_ops *default_unwinder;
> +extern const struct unwinder_ops unwinder_guess;
> +#ifdef CONFIG_UNWINDER_PROLOGUE
> +extern const struct unwinder_ops unwinder_prologue;
> +#endif
> +
> void unwind_start(struct unwind_state *state,
> struct task_struct *task, struct pt_regs *regs);
> bool unwind_next_frame(struct unwind_state *state);
> @@ -49,4 +65,10 @@ static inline unsigned long unwind_graph_addr(struct unwind_state *state,
> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
> }
> +
> +static inline void unwind_register_unwinder(struct unwind_state *state,
> + const struct unwinder_ops *unwinder)
> +{
> + state->ops = unwinder;
> +}
> #endif /* _ASM_UNWIND_H */
> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
> index 7ca65195f7f8..cb6029ea3ea9 100644
> --- a/arch/loongarch/kernel/Makefile
> +++ b/arch/loongarch/kernel/Makefile
> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
> elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
> - alternative.o unaligned.o
> + alternative.o unaligned.o unwind.o unwind_guess.o
>
> obj-$(CONFIG_ACPI) += acpi.o
> obj-$(CONFIG_EFI) += efi.o
> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>
> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
>
> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
> new file mode 100644
> index 000000000000..568c6fe707d1
> --- /dev/null
> +++ b/arch/loongarch/kernel/unwind.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Loongson Technology Corporation Limited
> + */
> +#include <asm/unwind.h>
> +
> +#if defined(CONFIG_UNWINDER_GUESS)
> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
> +#endif
> +
> +unsigned long unwind_get_return_address(struct unwind_state *state)
> +{
> + if (!state->ops || unwind_done(state))
> + return 0;
> + return state->ops->unwind_get_return_address(state);
> +}
> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> +
> +void unwind_start(struct unwind_state *state, struct task_struct *task,
> + struct pt_regs *regs)
> +{
> + memset(state, 0, sizeof(*state));
> + unwind_register_unwinder(state, default_unwinder);
> + if (regs) {
> + state->sp = regs->regs[3];
> + state->pc = regs->csr_era;
> + state->ra = regs->regs[1];
> + } else if (task == current) {
> + state->sp = (unsigned long)__builtin_frame_address(0);
> + state->pc = (unsigned long)__builtin_return_address(0);
> + state->ra = 0;
> + } else {
> + state->sp = thread_saved_fp(task);
> + state->pc = thread_saved_ra(task);
> + state->ra = 0;
> + }
> + state->task = task;
> + get_stack_info(state->sp, state->task, &state->stack_info);
> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
> + state->ops->unwind_start(state, task, regs);
> +}
> +EXPORT_SYMBOL_GPL(unwind_start);
> +
> +bool unwind_next_frame(struct unwind_state *state)
> +{
> + if (!state->ops || unwind_done(state))
> + return false;
> + return state->ops->unwind_next_frame(state);
> +}
> +EXPORT_SYMBOL_GPL(unwind_next_frame);
> diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
> index 8ce32c37c587..b7ca2b88ac63 100644
> --- a/arch/loongarch/kernel/unwind_guess.c
> +++ b/arch/loongarch/kernel/unwind_guess.c
> @@ -7,51 +7,23 @@
>
> #include <asm/unwind.h>
>
> -unsigned long unwind_get_return_address(struct unwind_state *state)
> +static unsigned long get_return_address(struct unwind_state *state)
> {
> - if (unwind_done(state))
> - return 0;
> return state->pc;
> }
> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>
> -void unwind_start(struct unwind_state *state, struct task_struct *task,
> +static void start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> - memset(state, 0, sizeof(*state));
> -
> - if (regs) {
> - state->sp = regs->regs[3];
> - state->pc = regs->csr_era;
> - } else if (task == current) {
> - state->sp = (unsigned long)__builtin_frame_address(0);
> - state->pc = (unsigned long)__builtin_return_address(0);
> - } else {
> - state->sp = thread_saved_fp(task);
> - state->pc = thread_saved_ra(task);
> - }
> -
> - state->task = task;
> - state->first = true;
> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
> - get_stack_info(state->sp, state->task, &state->stack_info);
> -
> if (!unwind_done(state) && !__kernel_text_address(state->pc))
> unwind_next_frame(state);
> }
> -EXPORT_SYMBOL_GPL(unwind_start);
>
> -bool unwind_next_frame(struct unwind_state *state)
> +static bool next_frame(struct unwind_state *state)
> {
> struct stack_info *info = &state->stack_info;
> unsigned long addr;
>
> - if (unwind_done(state))
> - return false;
> -
> - if (state->first)
> - state->first = false;
> -
> do {
> for (state->sp += sizeof(unsigned long);
> state->sp < info->end;
> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>
> return false;
> }
> -EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +const struct unwinder_ops unwinder_guess = {
> + .unwind_start = start,
> + .unwind_next_frame = next_frame,
> + .unwind_get_return_address = get_return_address,
> +};
> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> index d464c533c64f..9677e13c4b4c 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -9,6 +9,8 @@
> #include <asm/ptrace.h>
> #include <asm/unwind.h>
>
> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
> +
> static inline void unwind_state_fixup(struct unwind_state *state)
> {
> #ifdef CONFIG_DYNAMIC_FTRACE
> @@ -19,31 +21,19 @@ static inline void unwind_state_fixup(struct unwind_state *state)
> #endif
> }
>
> -unsigned long unwind_get_return_address(struct unwind_state *state)
> +static unsigned long get_return_address(struct unwind_state *state)
> {
> - if (unwind_done(state))
> - return 0;
> return state->pc;
> }
> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
> -
> -static bool unwind_by_guess(struct unwind_state *state)
> -{
> - struct stack_info *info = &state->stack_info;
> - unsigned long addr;
> -
> - for (state->sp += sizeof(unsigned long);
> - state->sp < info->end;
> - state->sp += sizeof(unsigned long)) {
> - addr = *(unsigned long *)(state->sp);
> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
> - if (__kernel_text_address(state->pc))
> - return true;
> - }
> -
> - return false;
> -}
>
> +/*
> + * LoongArch function prologue like follows,
> + * [others instructions not use stack var]
> + * addi.d sp, sp, -imm
> + * st.d xx, sp, offset <- save callee saved regs and
> + * st.d yy, sp, offset save ra if function is nest.
> + * [others instructions]
> + */
> static bool unwind_by_prologue(struct unwind_state *state)
> {
> long frame_ra = -1;
> @@ -89,6 +79,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
> ip++;
> }
>
> + /*
> + * Not find stack alloc action, PC may be in a leaf function. Only the
> + * first being true is reasonable, otherwise indicate analysis is broken.
> + */
> if (!frame_size) {
> if (state->first)
> goto first;
> @@ -106,6 +100,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
> ip++;
> }
>
> + /* Not find save $ra action, PC may be in a leaf function, too. */
> if (frame_ra < 0) {
> if (state->first) {
> state->sp = state->sp + frame_size;
> @@ -114,96 +109,63 @@ static bool unwind_by_prologue(struct unwind_state *state)
> return false;
> }
>
> - if (state->first)
> - state->first = false;
> -
> state->pc = *(unsigned long *)(state->sp + frame_ra);
> state->sp = state->sp + frame_size;
> goto out;
>
> first:
> - state->first = false;
> - if (state->pc == state->ra)
> - return false;
> -
> state->pc = state->ra;
>
> out:
> + state->first = false;
> unwind_state_fixup(state);
> return !!__kernel_text_address(state->pc);
> }
>
> -void unwind_start(struct unwind_state *state, struct task_struct *task,
> +static void start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> - memset(state, 0, sizeof(*state));
> - state->type = UNWINDER_PROLOGUE;
> -
> - if (regs) {
> - state->sp = regs->regs[3];
> - state->pc = regs->csr_era;
> - state->ra = regs->regs[1];
> - if (!__kernel_text_address(state->pc))
> - state->type = UNWINDER_GUESS;
> - } else if (task == current) {
> - state->sp = (unsigned long)__builtin_frame_address(0);
> - state->pc = (unsigned long)__builtin_return_address(0);
> - state->ra = 0;
> - } else {
> - state->sp = thread_saved_fp(task);
> - state->pc = thread_saved_ra(task);
> - state->ra = 0;
> - }
> -
> - state->task = task;
> state->first = true;
> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
> - get_stack_info(state->sp, state->task, &state->stack_info);
>
> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
> - unwind_next_frame(state);
> + /*
> + * The current PC is not kernel text address, we cannot find its
> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
> + * we can use the guard unwinder.
> + */
> + if (!__kernel_text_address(state->pc)) {
> + unwind_register_unwinder(state, guard_unwinder);
> + if (!unwind_done(state))
> + unwind_next_frame(state);
> + }
> }
> -EXPORT_SYMBOL_GPL(unwind_start);
>
> -bool unwind_next_frame(struct unwind_state *state)
> +static bool next_frame(struct unwind_state *state)
> {
> struct stack_info *info = &state->stack_info;
> struct pt_regs *regs;
> unsigned long pc;
>
> - if (unwind_done(state))
> - return false;
> -
> do {
> - switch (state->type) {
> - case UNWINDER_GUESS:
> - state->first = false;
> - if (unwind_by_guess(state))
> - return true;
> - break;
> -
> - case UNWINDER_PROLOGUE:
> - if (unwind_by_prologue(state)) {
> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
> - return true;
> - }
> + if (unwind_by_prologue(state)) {
> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
> + return true;
> + }
>
> - if (info->type == STACK_TYPE_IRQ &&
> - info->end == state->sp) {
> - regs = (struct pt_regs *)info->next_sp;
> - pc = regs->csr_era;
> + if (info->type == STACK_TYPE_IRQ &&
> + info->end == state->sp) {
> + regs = (struct pt_regs *)info->next_sp;
> + pc = regs->csr_era;
>
> - if (user_mode(regs) || !__kernel_text_address(pc))
> - return false;
> + if (user_mode(regs) || !__kernel_text_address(pc))
> + return false;
>
> - state->first = true;
> - state->ra = regs->regs[1];
> - state->sp = regs->regs[3];
> - state->pc = pc;
> - get_stack_info(state->sp, state->task, info);
> + state->first = true;
> + state->ra = regs->regs[1];
> + state->sp = regs->regs[3];
> + state->pc = pc;
> + get_stack_info(state->sp, state->task, info);
>
> - return true;
> - }
> + return true;
> }
>
> state->sp = info->next_sp;
> @@ -212,4 +174,9 @@ bool unwind_next_frame(struct unwind_state *state)
>
> return false;
> }
> -EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +const struct unwinder_ops unwinder_prologue = {
> + .unwind_start = start,
> + .unwind_next_frame = next_frame,
> + .unwind_get_return_address = get_return_address,
> +};
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
` (3 preceding siblings ...)
2022-12-15 4:01 ` [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 6:15 ` Huacai Chen
2022-12-15 4:01 ` [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder Jinyang He
5 siblings, 1 reply; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Inspired by MIPS, add cmdline parameter named 'raw_show_trace' to
enable guess unwinder in prologue unwinder unwind_start() default.
In some cases the guess is more efficient than prologue.
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/kernel/unwind_prologue.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index 9677e13c4b4c..441641227c10 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -122,11 +122,22 @@ static bool unwind_by_prologue(struct unwind_state *state)
return !!__kernel_text_address(state->pc);
}
+static int raw_show_trace;
+static int __init set_raw_show_trace(char *str)
+{
+ raw_show_trace = 1;
+ return 1;
+}
+__setup("raw_show_trace", set_raw_show_trace);
+
static void start(struct unwind_state *state, struct task_struct *task,
struct pt_regs *regs)
{
state->first = true;
+ if (raw_show_trace)
+ unwind_register_unwinder(state, guard_unwinder);
+
/*
* The current PC is not kernel text address, we cannot find its
* relative symbol. Thus, prologue analysis will be broken. Luckly,
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default
2022-12-15 4:01 ` [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default Jinyang He
@ 2022-12-15 6:15 ` Huacai Chen
2022-12-15 6:48 ` Jinyang He
0 siblings, 1 reply; 19+ messages in thread
From: Huacai Chen @ 2022-12-15 6:15 UTC (permalink / raw)
To: Jinyang He
Cc: WANG Xuerui, Qing Zhang, loongarch, linux-kernel, Steven Rostedt,
Masami Hiramatsu, Mark Rutland
Hi, Jinyang,
Is this patch really necessary?
Huacai
On Thu, Dec 15, 2022 at 12:02 PM Jinyang He <hejinyang@loongson.cn> wrote:
>
> Inspired by MIPS, add cmdline parameter named 'raw_show_trace' to
> enable guess unwinder in prologue unwinder unwind_start() default.
> In some cases the guess is more efficient than prologue.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> arch/loongarch/kernel/unwind_prologue.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> index 9677e13c4b4c..441641227c10 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -122,11 +122,22 @@ static bool unwind_by_prologue(struct unwind_state *state)
> return !!__kernel_text_address(state->pc);
> }
>
> +static int raw_show_trace;
> +static int __init set_raw_show_trace(char *str)
> +{
> + raw_show_trace = 1;
> + return 1;
> +}
> +__setup("raw_show_trace", set_raw_show_trace);
> +
> static void start(struct unwind_state *state, struct task_struct *task,
> struct pt_regs *regs)
> {
> state->first = true;
>
> + if (raw_show_trace)
> + unwind_register_unwinder(state, guard_unwinder);
> +
> /*
> * The current PC is not kernel text address, we cannot find its
> * relative symbol. Thus, prologue analysis will be broken. Luckly,
> --
> 2.34.3
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default
2022-12-15 6:15 ` Huacai Chen
@ 2022-12-15 6:48 ` Jinyang He
0 siblings, 0 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-15 6:48 UTC (permalink / raw)
To: Huacai Chen
Cc: WANG Xuerui, Qing Zhang, loongarch, linux-kernel, Steven Rostedt,
Masami Hiramatsu, Mark Rutland
On 2022-12-15 14:15, Huacai Chen wrote:
> Hi, Jinyang,
>
> Is this patch really necessary?
Of course not. When I developed and tested this series, I always compile
the kernel twice, maybe more :-(, to do comparisons between the result from
guess and result from prologue to determine whether prologue is effective.
Also, the guess_unwinder might be more reliable if function_graph is
enabled, because prologue_unwinder may stop by strange code flow. If you
think it is meaningless, I'll remove it in the next version. It doesn't
affect other patches in this series.
Thanks,
Jinyang
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder
2022-12-15 4:01 [PATCH 0/6] LoongArch: Some fix and new features for unwinders Jinyang He
` (4 preceding siblings ...)
2022-12-15 4:01 ` [PATCH 5/6] LoongArch: Add raw_show_trace to enable guess unwinder default Jinyang He
@ 2022-12-15 4:01 ` Jinyang He
2022-12-15 12:04 ` Qing Zhang
5 siblings, 1 reply; 19+ messages in thread
From: Jinyang He @ 2022-12-15 4:01 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui, Qing Zhang
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
When exception is triggered, code flow go handle_\exception in some
cases. One of stackframe in this case as follows,
high -> +-------+
| REGS | <- a pt_regs
| |
| | <- ex trigger
| REGS | <- ex pt_regs <-+
| | |
| | |
low -> +-------+ ->unwind-+
When unwinder unwind to handler_\exception it cannot go on prologue
analysis. It is asynchronous code flow, we should get the next frame
PC from regs->csr_era but not from regs->regs[1]. And we copy the
handler codes to eentry in the early time and copy the handler codes
to NUMA-relative memory named pcpu_handlers if NUMA is enabled. Thus,
unwinder cannot unwind normally. Therefore, try to give some hint in
handler_\exception and fixup it in unwind_next_frame.
Reported-by: Qing Zhang <zhangqing@loongson.cn>
Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
arch/loongarch/include/asm/unwind.h | 2 +-
arch/loongarch/kernel/genex.S | 3 +
arch/loongarch/kernel/unwind_prologue.c | 100 +++++++++++++++++++++---
arch/loongarch/mm/tlb.c | 2 +-
4 files changed, 92 insertions(+), 15 deletions(-)
diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
index a16aff1d086a..c02cb3b39fe2 100644
--- a/arch/loongarch/include/asm/unwind.h
+++ b/arch/loongarch/include/asm/unwind.h
@@ -24,7 +24,7 @@ struct unwind_state {
char type; /* UNWINDER_XXX */
struct stack_info stack_info;
struct task_struct *task;
- bool first, error, is_ftrace;
+ bool first, error, reset;
int graph_idx;
unsigned long sp, pc, ra;
const struct unwinder_ops *ops;
diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
index 75e5be807a0d..7e5c293ed89f 100644
--- a/arch/loongarch/kernel/genex.S
+++ b/arch/loongarch/kernel/genex.S
@@ -67,14 +67,17 @@ SYM_FUNC_END(except_vec_cex)
.macro BUILD_HANDLER exception handler prep
.align 5
SYM_FUNC_START(handle_\exception)
+ 666:
BACKUP_T0T1
SAVE_ALL
build_prep_\prep
move a0, sp
la.abs t0, do_\handler
jirl ra, t0, 0
+ 668:
RESTORE_ALL_AND_RET
SYM_FUNC_END(handle_\exception)
+ SYM_DATA(unwind_hint_\exception, .word 668b - 666b)
.endm
BUILD_HANDLER ade ade badv
diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
index 441641227c10..c34bb035ac56 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -2,23 +2,102 @@
/*
* Copyright (C) 2022 Loongson Technology Corporation Limited
*/
+#include <linux/cpumask.h>
#include <linux/ftrace.h>
#include <linux/kallsyms.h>
#include <asm/inst.h>
+#include <asm/loongson.h>
#include <asm/ptrace.h>
+#include <asm/setup.h>
#include <asm/unwind.h>
static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
-static inline void unwind_state_fixup(struct unwind_state *state)
+extern const int unwind_hint_ade;
+extern const int unwind_hint_ale;
+extern const int unwind_hint_bp;
+extern const int unwind_hint_fpe;
+extern const int unwind_hint_fpu;
+extern const int unwind_hint_lsx;
+extern const int unwind_hint_lasx;
+extern const int unwind_hint_lbt;
+extern const int unwind_hint_ri;
+extern const int unwind_hint_watch;
+extern unsigned long eentry;
+#ifdef CONFIG_NUMA
+extern unsigned long pcpu_handlers[NR_CPUS];
+#endif
+
+static inline bool scan_handler(unsigned long entry_offset)
{
-#ifdef CONFIG_DYNAMIC_FTRACE
- static unsigned long ftrace = (unsigned long)ftrace_call + 4;
+ int idx, offset;
- if (state->pc == ftrace)
- state->is_ftrace = true;
+ if (entry_offset >= EXCCODE_INT_START * VECSIZE)
+ return false;
+
+ idx = entry_offset / VECSIZE;
+ offset = entry_offset % VECSIZE;
+ switch (idx) {
+ case EXCCODE_ADE:
+ return offset == unwind_hint_ade;
+ case EXCCODE_ALE:
+ return offset == unwind_hint_ale;
+ case EXCCODE_BP:
+ return offset == unwind_hint_bp;
+ case EXCCODE_FPE:
+ return offset == unwind_hint_fpe;
+ case EXCCODE_FPDIS:
+ return offset == unwind_hint_fpu;
+ case EXCCODE_LSXDIS:
+ return offset == unwind_hint_lsx;
+ case EXCCODE_LASXDIS:
+ return offset == unwind_hint_lasx;
+ case EXCCODE_BTDIS:
+ return offset == unwind_hint_lbt;
+ case EXCCODE_INE:
+ return offset == unwind_hint_ri;
+ case EXCCODE_WATCH:
+ return offset == unwind_hint_watch;
+ default:
+ return false;
+ }
+}
+
+static inline bool fix_exceptions(unsigned long pc)
+{
+#ifdef CONFIG_NUMA
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ if (!pcpu_handlers[cpu])
+ continue;
+ if (scan_handler(pc - pcpu_handlers[cpu]))
+ return true;
+ }
#endif
+ return scan_handler(pc - eentry);
+}
+
+/*
+ * As we meet ftrace_regs_entry, reset first flag like first doing
+ * tracing. Prologue analysis will stop soon because PC is at entry.
+ */
+static inline bool fix_ftrace(unsigned long pc)
+{
+#ifdef CONFIG_DYNAMIC_FTRACE
+ return pc == (unsigned long)ftrace_call + LOONGARCH_INSN_SIZE;
+#else
+ return false;
+#endif
+}
+
+static inline bool unwind_state_fixup(struct unwind_state *state)
+{
+ if (!fix_exceptions(state->pc) && !fix_ftrace(state->pc))
+ return false;
+ state->reset = true;
+ return true;
}
static unsigned long get_return_address(struct unwind_state *state)
@@ -46,14 +125,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
if (state->sp >= info->end || state->sp < info->begin)
return false;
- if (state->is_ftrace) {
- /*
- * As we meet ftrace_regs_entry, reset first flag like first doing
- * tracing. Prologue analysis will stop soon because PC is at entry.
- */
+ if (state->reset) {
regs = (struct pt_regs *)state->sp;
state->first = true;
- state->is_ftrace = false;
+ state->reset = false;
state->pc = regs->csr_era;
state->ra = regs->regs[1];
state->sp = regs->regs[3];
@@ -118,8 +193,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
out:
state->first = false;
- unwind_state_fixup(state);
- return !!__kernel_text_address(state->pc);
+ return unwind_state_fixup(state) || __kernel_text_address(state->pc);
}
static int raw_show_trace;
diff --git a/arch/loongarch/mm/tlb.c b/arch/loongarch/mm/tlb.c
index da3681f131c8..8bad6b0cff59 100644
--- a/arch/loongarch/mm/tlb.c
+++ b/arch/loongarch/mm/tlb.c
@@ -251,7 +251,7 @@ static void output_pgtable_bits_defines(void)
}
#ifdef CONFIG_NUMA
-static unsigned long pcpu_handlers[NR_CPUS];
+unsigned long pcpu_handlers[NR_CPUS];
#endif
extern long exception_handlers[VECSIZE * 128 / sizeof(long)];
--
2.34.3
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder
2022-12-15 4:01 ` [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder Jinyang He
@ 2022-12-15 12:04 ` Qing Zhang
2022-12-16 1:44 ` Jinyang He
0 siblings, 1 reply; 19+ messages in thread
From: Qing Zhang @ 2022-12-15 12:04 UTC (permalink / raw)
To: Jinyang He, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
Hi, Jinyang
On 2022/12/15 下午12:01, Jinyang He wrote:
> When exception is triggered, code flow go handle_\exception in some
> cases. One of stackframe in this case as follows,
>
> high -> +-------+
> | REGS | <- a pt_regs
> | |
> | | <- ex trigger
> | REGS | <- ex pt_regs <-+
> | | |
> | | |
> low -> +-------+ ->unwind-+
>
> When unwinder unwind to handler_\exception it cannot go on prologue
> analysis. It is asynchronous code flow, we should get the next frame
> PC from regs->csr_era but not from regs->regs[1]. And we copy the
> handler codes to eentry in the early time and copy the handler codes
> to NUMA-relative memory named pcpu_handlers if NUMA is enabled. Thus,
> unwinder cannot unwind normally. Therefore, try to give some hint in
> handler_\exception and fixup it in unwind_next_frame.
>
> Reported-by: Qing Zhang <zhangqing@loongson.cn>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
> arch/loongarch/include/asm/unwind.h | 2 +-
> arch/loongarch/kernel/genex.S | 3 +
> arch/loongarch/kernel/unwind_prologue.c | 100 +++++++++++++++++++++---
> arch/loongarch/mm/tlb.c | 2 +-
> 4 files changed, 92 insertions(+), 15 deletions(-)
>
The others look good to me, but there is still a small problem:
When I tested hw_breakpoint.ko with prologue unwinder,
sometimes output address [<9000000100302724>] 0x9000000100302724, eg:
CPU: 3 PID: 0
But some processes are normal, eg: CPU: 0 PID: 0
[27.655549] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.1.0-rc8 #9
[27.655552] Hardware name: Loongson Loongson-3A5000-7A1000-1w-A2101/
Loongson-LS3A5000-7A1000-1w-A2101, BIOS
vUDK2018-LoongArch-V2.0.pre-beta8 06/15/2022
[27.655604]...
[27.655606] Call Trace:
[27.655607] [<9000000000222f88>] show_stack+0x60/0x184
[27.655613] [<90000000010e9b8c>] dump_stack_lvl+0x60/0x88
[27.655618] [] sample_hbp_handler+0x30/0x4c [data_breakpoint]
[27.655626] [<900000000037c8a0>] __perf_event_overflow+0x84/0x26c
[27.655629] [<900000000038980c>] perf_bp_event+0xc0/0xc8
[27.655633] [<900000000022e3bc>] watchpoint_handler+0x54/0x88
[27.655637] [<90000000010ea2f8>] do_watch+0x30/0x48
[27.655640] [<9000000100302724>] 0x9000000100302724 // Not natural
[27.655642] [<9000000000ab4cbc>] add_interrupt_randomness+0x60/0xbc
[27.655646] [<90000000002a0fa0>] handle_irq_event_percpu+0x28/0x70
[27.655650] [<90000000002a6f9c>] handle_percpu_irq+0x54/0x88
[27.655652] [<90000000002a025c>] generic_handle_domain_irq+0x28/0x40
[27.655655] [<9000000000995458>] handle_cpu_irq+0x68/0xa4
[27.655658] [<90000000010ea8dc>] handle_loongarch_irq+0x34/0x4c
[27.655661] [<90000000010ea974>] do_vint+0x80/0xb4
[27.655664] [<90000000002216a0>] __arch_cpu_idle+0x20/0x24
[27.655667] [<90000000010f8178>] default_idle_call+0x30/0x58
[27.655670] [<90000000002825cc>] do_idle+0xb4/0x118
[27.655674] [<900000000028281c>] cpu_startup_entry+0x20/0x24
[27.655677] [<900000000022b198>] start_secondary+0x9c/0xa4
[27.655679] [<90000000010eb124>] smpboot_entry+0x60/0x64
[27.658940] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.1.0-rc8 #9
...
[28.229978] Call Trace:
[28.229979] [<9000000000222f88>] show_stack+0x60/0x184
[28.237503] [<90000000010e9b8c>] dump_stack_lvl+0x60/0x88
[28.242866] [] sample_hbp_handler+0x30/0x4c [data_breakpoint]
[28.250132] [<900000000037c8a0>] __perf_event_overflow+0x84/0x26c
[28.256186] [<900000000038980c>] perf_bp_event+0xc0/0xc8
[28.261462] [<900000000022e3bc>] watchpoint_handler+0x54/0x88
[28.267170] [<90000000010ea2f8>] do_watch+0x30/0x48
[28.272013] [<90000000017d2724>] exception_handlers+0x2724/0x1000 //...
[28.278155] [<9000000000ab4cbc>] add_interrupt_randomness+0x60/0xbc
[28.284381] [<90000000002a0fa0>] handle_irq_event_percpu+0x28/0x70
[28.290520] [<90000000002a6f9c>] handle_percpu_irq+0x54/0x88
[28.296140] [<90000000002a025c>] generic_handle_domain_irq+0x28/0x40
[28.302452] [<9000000000995458>] handle_cpu_irq+0x68/0xa4
[28.307813] [<90000000010ea8dc>] handle_loongarch_irq+0x34/0x4c
[28.313693] [<90000000010ea974>] do_vint+0x80/0xb4
[28.318450] [<90000000002216a0>] __arch_cpu_idle+0x20/0x24
[28.323897] [<90000000010f8178>] default_idle_call+0x30/0x58
[28.329518] [<90000000002825cc>] do_idle+0xb4/0x118
[28.334361] [<900000000028281c>] cpu_startup_entry+0x20/0x24
[28.339982] [<90000000010ec0dc>] kernel_init+0x0/0x110
[28.345085] [<90000000011106f8>] arch_post_acpi_subsys_init+0x0/0x4
Maybe sometimes assembly kallsyms is not recognized, let me think...
Thanks,
-Qing
> diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
> index a16aff1d086a..c02cb3b39fe2 100644
> --- a/arch/loongarch/include/asm/unwind.h
> +++ b/arch/loongarch/include/asm/unwind.h
> @@ -24,7 +24,7 @@ struct unwind_state {
> char type; /* UNWINDER_XXX */
> struct stack_info stack_info;
> struct task_struct *task;
> - bool first, error, is_ftrace;
> + bool first, error, reset;
> int graph_idx;
> unsigned long sp, pc, ra;
> const struct unwinder_ops *ops;
> diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
> index 75e5be807a0d..7e5c293ed89f 100644
> --- a/arch/loongarch/kernel/genex.S
> +++ b/arch/loongarch/kernel/genex.S
> @@ -67,14 +67,17 @@ SYM_FUNC_END(except_vec_cex)
> .macro BUILD_HANDLER exception handler prep
> .align 5
> SYM_FUNC_START(handle_\exception)
> + 666:
> BACKUP_T0T1
> SAVE_ALL
> build_prep_\prep
> move a0, sp
> la.abs t0, do_\handler
> jirl ra, t0, 0
> + 668:
> RESTORE_ALL_AND_RET
> SYM_FUNC_END(handle_\exception)
> + SYM_DATA(unwind_hint_\exception, .word 668b - 666b)
> .endm
>
> BUILD_HANDLER ade ade badv
> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> index 441641227c10..c34bb035ac56 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -2,23 +2,102 @@
> /*
> * Copyright (C) 2022 Loongson Technology Corporation Limited
> */
> +#include <linux/cpumask.h>
> #include <linux/ftrace.h>
> #include <linux/kallsyms.h>
>
> #include <asm/inst.h>
> +#include <asm/loongson.h>
> #include <asm/ptrace.h>
> +#include <asm/setup.h>
> #include <asm/unwind.h>
>
> static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>
> -static inline void unwind_state_fixup(struct unwind_state *state)
> +extern const int unwind_hint_ade;
> +extern const int unwind_hint_ale;
> +extern const int unwind_hint_bp;
> +extern const int unwind_hint_fpe;
> +extern const int unwind_hint_fpu;
> +extern const int unwind_hint_lsx;
> +extern const int unwind_hint_lasx;
> +extern const int unwind_hint_lbt;
> +extern const int unwind_hint_ri;
> +extern const int unwind_hint_watch;
> +extern unsigned long eentry;
> +#ifdef CONFIG_NUMA
> +extern unsigned long pcpu_handlers[NR_CPUS];
> +#endif
> +
> +static inline bool scan_handler(unsigned long entry_offset)
> {
> -#ifdef CONFIG_DYNAMIC_FTRACE
> - static unsigned long ftrace = (unsigned long)ftrace_call + 4;
> + int idx, offset;
>
> - if (state->pc == ftrace)
> - state->is_ftrace = true;
> + if (entry_offset >= EXCCODE_INT_START * VECSIZE)
> + return false;
> +
> + idx = entry_offset / VECSIZE;
> + offset = entry_offset % VECSIZE;
> + switch (idx) {
> + case EXCCODE_ADE:
> + return offset == unwind_hint_ade;
> + case EXCCODE_ALE:
> + return offset == unwind_hint_ale;
> + case EXCCODE_BP:
> + return offset == unwind_hint_bp;
> + case EXCCODE_FPE:
> + return offset == unwind_hint_fpe;
> + case EXCCODE_FPDIS:
> + return offset == unwind_hint_fpu;
> + case EXCCODE_LSXDIS:
> + return offset == unwind_hint_lsx;
> + case EXCCODE_LASXDIS:
> + return offset == unwind_hint_lasx;
> + case EXCCODE_BTDIS:
> + return offset == unwind_hint_lbt;
> + case EXCCODE_INE:
> + return offset == unwind_hint_ri;
> + case EXCCODE_WATCH:
> + return offset == unwind_hint_watch;
> + default:
> + return false;
> + }
> +}
> +
> +static inline bool fix_exceptions(unsigned long pc)
> +{
> +#ifdef CONFIG_NUMA
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + if (!pcpu_handlers[cpu])
> + continue;
> + if (scan_handler(pc - pcpu_handlers[cpu]))
> + return true;
> + }
> #endif
> + return scan_handler(pc - eentry);
> +}
> +
> +/*
> + * As we meet ftrace_regs_entry, reset first flag like first doing
> + * tracing. Prologue analysis will stop soon because PC is at entry.
> + */
> +static inline bool fix_ftrace(unsigned long pc)
> +{
> +#ifdef CONFIG_DYNAMIC_FTRACE
> + return pc == (unsigned long)ftrace_call + LOONGARCH_INSN_SIZE;
> +#else
> + return false;
> +#endif
> +}
> +
> +static inline bool unwind_state_fixup(struct unwind_state *state)
> +{
> + if (!fix_exceptions(state->pc) && !fix_ftrace(state->pc))
> + return false;
> + state->reset = true;
> + return true;
> }
>
> static unsigned long get_return_address(struct unwind_state *state)
> @@ -46,14 +125,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
> if (state->sp >= info->end || state->sp < info->begin)
> return false;
>
> - if (state->is_ftrace) {
> - /*
> - * As we meet ftrace_regs_entry, reset first flag like first doing
> - * tracing. Prologue analysis will stop soon because PC is at entry.
> - */
> + if (state->reset) {
> regs = (struct pt_regs *)state->sp;
> state->first = true;
> - state->is_ftrace = false;
> + state->reset = false;
> state->pc = regs->csr_era;
> state->ra = regs->regs[1];
> state->sp = regs->regs[3];
> @@ -118,8 +193,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
>
> out:
> state->first = false;
> - unwind_state_fixup(state);
> - return !!__kernel_text_address(state->pc);
> + return unwind_state_fixup(state) || __kernel_text_address(state->pc);
> }
>
> static int raw_show_trace;
> diff --git a/arch/loongarch/mm/tlb.c b/arch/loongarch/mm/tlb.c
> index da3681f131c8..8bad6b0cff59 100644
> --- a/arch/loongarch/mm/tlb.c
> +++ b/arch/loongarch/mm/tlb.c
> @@ -251,7 +251,7 @@ static void output_pgtable_bits_defines(void)
> }
>
> #ifdef CONFIG_NUMA
> -static unsigned long pcpu_handlers[NR_CPUS];
> +unsigned long pcpu_handlers[NR_CPUS];
> #endif
> extern long exception_handlers[VECSIZE * 128 / sizeof(long)];
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/6] LoongArch: Add generic ex-handler unwind in prologue unwinder
2022-12-15 12:04 ` Qing Zhang
@ 2022-12-16 1:44 ` Jinyang He
0 siblings, 0 replies; 19+ messages in thread
From: Jinyang He @ 2022-12-16 1:44 UTC (permalink / raw)
To: Qing Zhang, Huacai Chen, WANG Xuerui
Cc: loongarch, linux-kernel, Steven Rostedt, Masami Hiramatsu, Mark Rutland
On 2022-12-15 20:04, Qing Zhang wrote:
> Hi, Jinyang
>
> On 2022/12/15 下午12:01, Jinyang He wrote:
>> When exception is triggered, code flow go handle_\exception in some
>> cases. One of stackframe in this case as follows,
>>
>> high -> +-------+
>> | REGS | <- a pt_regs
>> | |
>> | | <- ex trigger
>> | REGS | <- ex pt_regs <-+
>> | | |
>> | | |
>> low -> +-------+ ->unwind-+
>>
>> When unwinder unwind to handler_\exception it cannot go on prologue
>> analysis. It is asynchronous code flow, we should get the next frame
>> PC from regs->csr_era but not from regs->regs[1]. And we copy the
>> handler codes to eentry in the early time and copy the handler codes
>> to NUMA-relative memory named pcpu_handlers if NUMA is enabled. Thus,
>> unwinder cannot unwind normally. Therefore, try to give some hint in
>> handler_\exception and fixup it in unwind_next_frame.
>>
>> Reported-by: Qing Zhang <zhangqing@loongson.cn>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>> arch/loongarch/include/asm/unwind.h | 2 +-
>> arch/loongarch/kernel/genex.S | 3 +
>> arch/loongarch/kernel/unwind_prologue.c | 100 +++++++++++++++++++++---
>> arch/loongarch/mm/tlb.c | 2 +-
>> 4 files changed, 92 insertions(+), 15 deletions(-)
>>
> The others look good to me, but there is still a small problem:
> When I tested hw_breakpoint.ko with prologue unwinder,
> sometimes output address [<9000000100302724>] 0x9000000100302724, eg:
> CPU: 3 PID: 0
> But some processes are normal, eg: CPU: 0 PID: 0
>
> [27.655549] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.1.0-rc8 #9
> [27.655552] Hardware name: Loongson Loongson-3A5000-7A1000-1w-A2101/
> Loongson-LS3A5000-7A1000-1w-A2101, BIOS
> vUDK2018-LoongArch-V2.0.pre-beta8 06/15/2022
>
> [27.655604]...
> [27.655606] Call Trace:
> [27.655607] [<9000000000222f88>] show_stack+0x60/0x184
> [27.655613] [<90000000010e9b8c>] dump_stack_lvl+0x60/0x88
> [27.655618] [] sample_hbp_handler+0x30/0x4c [data_breakpoint]
> [27.655626] [<900000000037c8a0>] __perf_event_overflow+0x84/0x26c
> [27.655629] [<900000000038980c>] perf_bp_event+0xc0/0xc8
> [27.655633] [<900000000022e3bc>] watchpoint_handler+0x54/0x88
> [27.655637] [<90000000010ea2f8>] do_watch+0x30/0x48
> [27.655640] [<9000000100302724>] 0x9000000100302724 // Not natural
> [27.655642] [<9000000000ab4cbc>] add_interrupt_randomness+0x60/0xbc
> [27.655646] [<90000000002a0fa0>] handle_irq_event_percpu+0x28/0x70
> [27.655650] [<90000000002a6f9c>] handle_percpu_irq+0x54/0x88
> [27.655652] [<90000000002a025c>] generic_handle_domain_irq+0x28/0x40
> [27.655655] [<9000000000995458>] handle_cpu_irq+0x68/0xa4
> [27.655658] [<90000000010ea8dc>] handle_loongarch_irq+0x34/0x4c
> [27.655661] [<90000000010ea974>] do_vint+0x80/0xb4
> [27.655664] [<90000000002216a0>] __arch_cpu_idle+0x20/0x24
> [27.655667] [<90000000010f8178>] default_idle_call+0x30/0x58
> [27.655670] [<90000000002825cc>] do_idle+0xb4/0x118
> [27.655674] [<900000000028281c>] cpu_startup_entry+0x20/0x24
> [27.655677] [<900000000022b198>] start_secondary+0x9c/0xa4
> [27.655679] [<90000000010eb124>] smpboot_entry+0x60/0x64
>
>
> [27.658940] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.1.0-rc8 #9
> ...
> [28.229978] Call Trace:
> [28.229979] [<9000000000222f88>] show_stack+0x60/0x184
> [28.237503] [<90000000010e9b8c>] dump_stack_lvl+0x60/0x88
> [28.242866] [] sample_hbp_handler+0x30/0x4c [data_breakpoint]
> [28.250132] [<900000000037c8a0>] __perf_event_overflow+0x84/0x26c
> [28.256186] [<900000000038980c>] perf_bp_event+0xc0/0xc8
> [28.261462] [<900000000022e3bc>] watchpoint_handler+0x54/0x88
> [28.267170] [<90000000010ea2f8>] do_watch+0x30/0x48
> [28.272013] [<90000000017d2724>] exception_handlers+0x2724/0x1000 //...
There is not in kernel text section but in kernel bss section. Because
the boot cpu set csr.eentry to eentry and set others cpus set csr.eentry
to pcpu_handlers[cpu]. All of these eentry are not in orginal position.
So we cannot find its real symbol. But I still give a chance to go on
and record
PC value when unwind_state_fixup return true in unwind_by_prologue().
Thanks,
Jinyang
> [28.278155] [<9000000000ab4cbc>] add_interrupt_randomness+0x60/0xbc
> [28.284381] [<90000000002a0fa0>] handle_irq_event_percpu+0x28/0x70
> [28.290520] [<90000000002a6f9c>] handle_percpu_irq+0x54/0x88
> [28.296140] [<90000000002a025c>] generic_handle_domain_irq+0x28/0x40
> [28.302452] [<9000000000995458>] handle_cpu_irq+0x68/0xa4
> [28.307813] [<90000000010ea8dc>] handle_loongarch_irq+0x34/0x4c
> [28.313693] [<90000000010ea974>] do_vint+0x80/0xb4
> [28.318450] [<90000000002216a0>] __arch_cpu_idle+0x20/0x24
> [28.323897] [<90000000010f8178>] default_idle_call+0x30/0x58
> [28.329518] [<90000000002825cc>] do_idle+0xb4/0x118
> [28.334361] [<900000000028281c>] cpu_startup_entry+0x20/0x24
> [28.339982] [<90000000010ec0dc>] kernel_init+0x0/0x110
> [28.345085] [<90000000011106f8>] arch_post_acpi_subsys_init+0x0/0x4
>
> Maybe sometimes assembly kallsyms is not recognized, let me think...
>
> Thanks,
> -Qing
>
^ permalink raw reply [flat|nested] 19+ messages in thread