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