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:8.425 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site