From: Jinyang He <hejinyang@loongson.cn>
To: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>,
Qing Zhang <zhangqing@loongson.cn>,
loongarch@lists.linux.dev, linux-kernel@vger.kernel.org,
Steven Rostedt <rostedt@goodmis.org>,
Masami Hiramatsu <mhiramat@kernel.org>,
Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH v2 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
Date: Tue, 3 Jan 2023 16:40:56 +0800 [thread overview]
Message-ID: <ab0b221e-db13-3cac-dca1-bbd67f739ab3@loongson.cn> (raw)
In-Reply-To: <CAAhV-H7J7yWpM-xLpK38gwEw3cMNU-BaxF3atVmDLyRxBcVrLg@mail.gmail.com>
On 2023-01-01 17:56, Huacai Chen wrote:
> Hi, Jinyang,
>
> On Thu, Dec 29, 2022 at 2:15 PM Jinyang He <hejinyang@loongson.cn> 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 | 26 ++++-
>> arch/loongarch/kernel/Makefile | 3 +-
>> arch/loongarch/kernel/traps.c | 3 -
>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>> arch/loongarch/kernel/unwind_guess.c | 41 ++------
>> arch/loongarch/kernel/unwind_prologue.c | 133 +++++++++---------------
>> 6 files changed, 132 insertions(+), 126 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..4a55fd7b77ad 100644
>> --- a/arch/loongarch/include/asm/unwind.h
>> +++ b/arch/loongarch/include/asm/unwind.h
>> @@ -13,20 +13,30 @@
>> #include <asm/ptrace.h>
>> #include <asm/stacktrace.h>
>>
>> -enum unwinder_type {
>> - UNWINDER_GUESS,
>> - UNWINDER_PROLOGUE,
>> -};
>> +struct unwinder_ops;
>>
>> struct unwind_state {
>> - char type; /* UNWINDER_XXX */
>> struct stack_info stack_info;
>> struct task_struct *task;
>> 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 +59,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
> We do have CONFIG_UNWINDER_GUESS and I don't like to build guess
> unwinder unconditionally, maybe we can refactor it to make the
> unwinder still user selectable.
In order to provide backtrace info to the user, prologue unwinder
will use guess unwinder when its first pc is not a kernel address,
such as the first PC is in ebpf generated codes. This patch is intended
to extract guess unwinder codes to avoid redundant codes. And if we provide
a new unwinder in the future like orc unwinder which take effect after
orc_init is set, we can use another backup unwinder like guess unwinder in
case we need backtrace before orc_init being set.
If you mind this, could we just do as follows,
obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o unwind_guess.o
Or copy the guess unwinder codes to unwind_prologue.c and set another
name for guess unwinder.
Thanks,
Jinyang
> Huacai
>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>> diff --git a/arch/loongarch/kernel/traps.c b/arch/loongarch/kernel/traps.c
>> index 3adc44832205..1ea14f6c18d3 100644
>> --- a/arch/loongarch/kernel/traps.c
>> +++ b/arch/loongarch/kernel/traps.c
>> @@ -72,9 +72,6 @@ static void show_backtrace(struct task_struct *task, const struct pt_regs *regs,
>> if (!task)
>> task = current;
>>
>> - if (user_mode(regs))
>> - state.type = UNWINDER_GUESS;
>> -
>> printk("%sCall Trace:", loglvl);
>> for (unwind_start(&state, task, pregs);
>> !unwind_done(&state); unwind_next_frame(&state)) {
>> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
>> new file mode 100644
>> index 000000000000..24d2cf99bfb6
>> --- /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 || task == NULL) {
>> + 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 935d24f8c95c..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 || task == NULL) {
>> - 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 f7994ed05f04..beb57ea24da2 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -19,31 +19,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 +77,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 +98,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 +107,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 || task == NULL) {
>> - 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 guess unwinder.
>> + */
>> + if (!__kernel_text_address(state->pc)) {
>> + unwind_register_unwinder(state, &unwinder_guess);
>> + 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 +172,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
>>
>>
next prev parent reply other threads:[~2023-01-03 8:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-29 6:15 [PATCH v2 0/5] LoongArch: Some fix and new features for unwinders Jinyang He
2022-12-29 6:15 ` [PATCH v2 1/6] LoongArch: Get frame info in unwind_start when regs is not supported Jinyang He
2022-12-29 6:15 ` [PATCH v2 2/6] LoongArch: Use correct sp value to get graph addr in unwinder guess Jinyang He
2022-12-29 6:15 ` [PATCH v2 3/6] LoongArch: Adjust PC value when unwind next frame in prologue unwinder Jinyang He
2022-12-29 6:15 ` [PATCH v2 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder Jinyang He
2023-01-01 9:56 ` Huacai Chen
2023-01-03 8:40 ` Jinyang He [this message]
2023-01-03 8:58 ` Huacai Chen
2022-12-29 6:15 ` [PATCH v2 5/6] LoongArch: Add generic ex-handler unwind in prologue unwinder Jinyang He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab0b221e-db13-3cac-dca1-bbd67f739ab3@loongson.cn \
--to=hejinyang@loongson.cn \
--cc=chenhuacai@kernel.org \
--cc=kernel@xen0n.name \
--cc=linux-kernel@vger.kernel.org \
--cc=loongarch@lists.linux.dev \
--cc=mark.rutland@arm.com \
--cc=mhiramat@kernel.org \
--cc=rostedt@goodmis.org \
--cc=zhangqing@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.