Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3 5/5] LoongArch: Add generic ex-handler unwind in
 prologue unwinder
To:     Huacai Chen <chenhuacai@kernel.org>,
        Jinyang He <hejinyang@loongson.cn>
Cc:     WANG Xuerui <kernel@xen0n.name>, 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>
References: <20230112004048.26191-1-hejinyang@loongson.cn>
 <CAAhV-H417wC+Bh6-EeD6sKmqgrJDP8kQPFZ9xu19Ev9Egdu1YQ@mail.gmail.com>
From:   Qing Zhang <zhangqing@loongson.cn>
Message-ID: <e471ad49-8b2e-179f-45c7-ba1f4ff94101@loongson.cn>
Date:   Tue, 17 Jan 2023 15:41:25 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <CAAhV-H417wC+Bh6-EeD6sKmqgrJDP8kQPFZ9xu19Ev9Egdu1YQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Huacai

On 2023/1/17 下午2:25, Huacai Chen wrote:
> On Thu, Jan 12, 2023 at 8:41 AM Jinyang He <hejinyang@loongson.cn> wrote:
>>
>> When exception is triggered, code flow go handle_\exception in some
>> cases. One of stackframe in this case as follows,
>>
>> high -> +-------+
>>          | REGS  |  <- a pt_regs
>>          |       |
>>          |       |  <- ex trigger
>>          | REGS  |  <- ex pt_regs   <-+
>>          |       |                    |
>>          |       |                    |
>> low  -> +-------+           ->unwind-+
>>
>> When unwinder unwind to handler_\exception it cannot go on prologue
>> analysis. It is asynchronous code flow, we should get the next frame
>> PC from regs->csr_era but not from regs->regs[1]. And we copy the
>> handler codes to eentry in the early time and copy the handler codes
>> to NUMA-relative memory named pcpu_handlers if NUMA is enabled. Thus,
>> unwinder cannot unwind normally. Therefore, try to give some hint in
>> handler_\exception and fixup it in unwind_next_frame.
>>
>> Reported-by: Qing Zhang <zhangqing@loongson.cn>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>>   arch/loongarch/include/asm/unwind.h     |   2 +-
>>   arch/loongarch/kernel/genex.S           |   3 +
>>   arch/loongarch/kernel/unwind_prologue.c | 100 +++++++++++++++++++++---
>>   arch/loongarch/mm/tlb.c                 |   2 +-
>>   4 files changed, 92 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
>> index cb428e1b19af..a38eb152aefb 100644
>> --- a/arch/loongarch/include/asm/unwind.h
>> +++ b/arch/loongarch/include/asm/unwind.h
>> @@ -22,7 +22,7 @@ struct unwind_state {
>>          char type; /* UNWINDER_XXX */
>>          struct stack_info stack_info;
>>          struct task_struct *task;
>> -       bool first, error, is_ftrace;
>> +       bool first, error, reset;
> Hi, Qing,
> Do you think is_reset is better than reset here?
ok, It depends on you.

Thanks
-Qing

>>          int graph_idx;
>>          unsigned long sp, pc, ra;
>>   };
>> diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
>> index 75e5be807a0d..7e5c293ed89f 100644
>> --- a/arch/loongarch/kernel/genex.S
>> +++ b/arch/loongarch/kernel/genex.S
>> @@ -67,14 +67,17 @@ SYM_FUNC_END(except_vec_cex)
>>          .macro  BUILD_HANDLER exception handler prep
>>          .align  5
>>          SYM_FUNC_START(handle_\exception)
>> +       666:
>>          BACKUP_T0T1
>>          SAVE_ALL
>>          build_prep_\prep
>>          move    a0, sp
>>          la.abs  t0, do_\handler
>>          jirl    ra, t0, 0
>> +       668:
>>          RESTORE_ALL_AND_RET
> I think the 668 label should be after RESTORE_ALL_AND_RET, right?
> 
> Huacai
> 
>>          SYM_FUNC_END(handle_\exception)
>> +       SYM_DATA(unwind_hint_\exception, .word 668b - 666b)
>>          .endm
>>
>>          BUILD_HANDLER ade ade badv
>> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
>> index e6c3f2ee507c..e52c18c2b604 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -2,21 +2,100 @@
>>   /*
>>    * Copyright (C) 2022 Loongson Technology Corporation Limited
>>    */
>> +#include <linux/cpumask.h>
>>   #include <linux/ftrace.h>
>>   #include <linux/kallsyms.h>
>>
>>   #include <asm/inst.h>
>> +#include <asm/loongson.h>
>>   #include <asm/ptrace.h>
>> +#include <asm/setup.h>
>>   #include <asm/unwind.h>
>>
>> -static inline void unwind_state_fixup(struct unwind_state *state)
>> +extern const int unwind_hint_ade;
>> +extern const int unwind_hint_ale;
>> +extern const int unwind_hint_bp;
>> +extern const int unwind_hint_fpe;
>> +extern const int unwind_hint_fpu;
>> +extern const int unwind_hint_lsx;
>> +extern const int unwind_hint_lasx;
>> +extern const int unwind_hint_lbt;
>> +extern const int unwind_hint_ri;
>> +extern const int unwind_hint_watch;
>> +extern unsigned long eentry;
>> +#ifdef CONFIG_NUMA
>> +extern unsigned long pcpu_handlers[NR_CPUS];
>> +#endif
>> +
>> +static inline bool scan_handler(unsigned long entry_offset)
>>   {
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -       static unsigned long ftrace = (unsigned long)ftrace_call + 4;
>> +       int idx, offset;
>>
>> -       if (state->pc == ftrace)
>> -               state->is_ftrace = true;
>> +       if (entry_offset >= EXCCODE_INT_START * VECSIZE)
>> +               return false;
>> +
>> +       idx = entry_offset / VECSIZE;
>> +       offset = entry_offset % VECSIZE;
>> +       switch (idx) {
>> +       case EXCCODE_ADE:
>> +               return offset == unwind_hint_ade;
>> +       case EXCCODE_ALE:
>> +               return offset == unwind_hint_ale;
>> +       case EXCCODE_BP:
>> +               return offset == unwind_hint_bp;
>> +       case EXCCODE_FPE:
>> +               return offset == unwind_hint_fpe;
>> +       case EXCCODE_FPDIS:
>> +               return offset == unwind_hint_fpu;
>> +       case EXCCODE_LSXDIS:
>> +               return offset == unwind_hint_lsx;
>> +       case EXCCODE_LASXDIS:
>> +               return offset == unwind_hint_lasx;
>> +       case EXCCODE_BTDIS:
>> +               return offset == unwind_hint_lbt;
>> +       case EXCCODE_INE:
>> +               return offset == unwind_hint_ri;
>> +       case EXCCODE_WATCH:
>> +               return offset == unwind_hint_watch;
>> +       default:
>> +               return false;
>> +       }
>> +}
>> +
>> +static inline bool fix_exceptions(unsigned long pc)
>> +{
>> +#ifdef CONFIG_NUMA
>> +       int cpu;
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               if (!pcpu_handlers[cpu])
>> +                       continue;
>> +               if (scan_handler(pc - pcpu_handlers[cpu]))
>> +                       return true;
>> +       }
>>   #endif
>> +       return scan_handler(pc - eentry);
>> +}
>> +
>> +/*
>> + * As we meet ftrace_regs_entry, reset first flag like first doing
>> + * tracing. Prologue analysis will stop soon because PC is at entry.
>> + */
>> +static inline bool fix_ftrace(unsigned long pc)
>> +{
>> +#ifdef CONFIG_DYNAMIC_FTRACE
>> +       return pc == (unsigned long)ftrace_call + LOONGARCH_INSN_SIZE;
>> +#else
>> +       return false;
>> +#endif
>> +}
>> +
>> +static inline bool unwind_state_fixup(struct unwind_state *state)
>> +{
>> +       if (!fix_exceptions(state->pc) && !fix_ftrace(state->pc))
>> +               return false;
>> +       state->reset = true;
>> +       return true;
>>   }
>>
>>   /*
>> @@ -39,14 +118,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
>>          if (state->sp >= info->end || state->sp < info->begin)
>>                  return false;
>>
>> -       if (state->is_ftrace) {
>> -               /*
>> -                * As we meet ftrace_regs_entry, reset first flag like first doing
>> -                * tracing. Prologue analysis will stop soon because PC is at entry.
>> -                */
>> +       if (state->reset) {
>>                  regs = (struct pt_regs *)state->sp;
>>                  state->first = true;
>> -               state->is_ftrace = false;
>> +               state->reset = false;
>>                  state->pc = regs->csr_era;
>>                  state->ra = regs->regs[1];
>>                  state->sp = regs->regs[3];
>> @@ -111,8 +186,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
>>
>>   out:
>>          state->first = false;
>> -       unwind_state_fixup(state);
>> -       return !!__kernel_text_address(state->pc);
>> +       return unwind_state_fixup(state) || __kernel_text_address(state->pc);
>>   }
>>
>>   static bool next_frame(struct unwind_state *state)
>> diff --git a/arch/loongarch/mm/tlb.c b/arch/loongarch/mm/tlb.c
>> index da3681f131c8..8bad6b0cff59 100644
>> --- a/arch/loongarch/mm/tlb.c
>> +++ b/arch/loongarch/mm/tlb.c
>> @@ -251,7 +251,7 @@ static void output_pgtable_bits_defines(void)
>>   }
>>
>>   #ifdef CONFIG_NUMA
>> -static unsigned long pcpu_handlers[NR_CPUS];
>> +unsigned long pcpu_handlers[NR_CPUS];
>>   #endif
>>   extern long exception_handlers[VECSIZE * 128 / sizeof(long)];
>>
>> --
>> 2.34.3
>>
>>