Messages in this thread | | | Subject | Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder | From | Jinyang He <> | Date | Sat, 17 Dec 2022 09:47:45 +0800 |
| |
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 >>
| |