lkml.org 
[lkml]   [2022]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder
From
Date
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

\
 
 \ /
  Last update: 2022-12-16 02:41    [W:0.587 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site