Messages in this thread | | | Subject | Re: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder | From | Qing Zhang <> | Date | Fri, 16 Dec 2022 10:14:40 +0800 |
| |
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 >
| |