Date:   Sun, 15 Oct 2023 21:57:54 +0800
From:   Huacai Chen <chenhuacai@...nel.org>
To:     Jinyang He <hejinyang@...ngson.cn>
Cc:     Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v2 8/8] LoongArch: Add ORC unwinder support

Hi, Jinyang,

On Sun, Oct 15, 2023 at 8:58 PM Jinyang He <hejinyang@...ngson.cn> wrote:
>
> On 2023-10-14 19:37, Huacai Chen wrote:
>
> > +CC Jinyang
> >
> > On Sat, Oct 14, 2023 at 5:21 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
> >>
> >>
> >> On 10/11/2023 12:37 PM, Huacai Chen wrote:
> >>> Hi, Tiezhu,
> >>>
> >>> Maybe "LoongArch: Add ORC stack unwinder support" is better.
> >> OK, will modify it.
> >>
> >>> On Mon, Oct 9, 2023 at 9:03 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
> >>>> The kernel CONFIG_UNWINDER_ORC option enables the ORC unwinder, which is
> >>>> similar in concept to a DWARF unwinder. The difference is that the format
> >>>> of the ORC data is much simpler than DWARF, which in turn allows the ORC
> >>>> unwinder to be much simpler and faster.
> >> ...
> >>
> >>>> +ifdef CONFIG_OBJTOOL
> >>>> +# https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=ecb802d02eeb
> >>>> +# https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=816029e06768
> >>>> +ifeq ($(shell as --help 2>&1 | grep -e '-mthin-add-sub'),)
> >>>> +  $(error Sorry, you need a newer gas version with -mthin-add-sub option)
> >>> I prefer no error out here, because without this option we can still
> >>> built a runnable kernel.
> >> I agree with you that it is better to not error out to stop compilation,
> >> but there are many objtool warnings during the compile process with old
> >> binutils, so it is necessary to give a warning so that the users know
> >> what happened and how to fix the lots of objtool warnings.
> >>
> >> That is to say, I would prefer to replace "error" with "warning".
> >>
> >>>> +endif
> >>>> +KBUILD_AFLAGS  += $(call cc-option,-mthin-add-sub) $(call cc-option,-Wa$(comma)-mthin-add-sub)
> >>>> +KBUILD_CFLAGS  += $(call cc-option,-mthin-add-sub) $(call cc-option,-Wa$(comma)-mthin-add-sub)
> >>>> +KBUILD_CFLAGS  += -fno-optimize-sibling-calls -fno-jump-tables -falign-functions=4
> >>>> +endif
> >> ...
> >>
> >>>> +#define ORC_REG_BP                     3
> >>> Use FP instead of BP in this patch, too.
> >> OK, will do it.
> >>
> >>>> +#define ORC_REG_MAX                    4
> >> ...
> >>
> >>>> +.macro UNWIND_HINT_UNDEFINED
> >>>> +       UNWIND_HINT type=UNWIND_HINT_TYPE_UNDEFINED
> >>>> +.endm
> >>> We don't need to set sp_reg=ORC_REG_UNDEFINED for UNWIND_HINT_UNDEFINED?
> >> Yes, no need to set sp_reg, the instructions marked with UNDEFINED
> >> are blind spots in ORC coverage, it is no related with stack trace,
> >> this is similar with x86.
> >>
> >>>> +
> >>>> +.macro UNWIND_HINT_EMPTY
> >>>> +       UNWIND_HINT sp_reg=ORC_REG_UNDEFINED type=UNWIND_HINT_TYPE_CALL
> >>>> +.endm
> >>> We don't need to define UNWIND_HINT_END_OF_STACK?
> >> Yes, it is useless now.
> >>
> >>>> +
> >>>> +.macro UNWIND_HINT_REGS
> >>>> +       UNWIND_HINT sp_reg=ORC_REG_SP type=UNWIND_HINT_TYPE_REGS
> >>>> +.endm
> >>>> +
> >>>> +.macro UNWIND_HINT_FUNC
> >>>> +       UNWIND_HINT sp_reg=ORC_REG_SP type=UNWIND_HINT_TYPE_CALL
> >>>> +.endm
> >>> We don't need to set sp_offset for UNWIND_HINT_REGS and UNWIND_HINT_FUNC?
> >> sp_offset is 0 by default, no need to set it unless you need to change
> >> its value, see include/linux/objtool.h
> >> .macro UNWIND_HINT type:req sp_reg=0 sp_offset=0 signal=0
> >>
> >>>> +
> >>>> +#endif /* __ASSEMBLY__ */
> >> ...
> >>
> >>>> diff --git a/arch/loongarch/kernel/entry.S b/arch/loongarch/kernel/entry.S
> >>>> index 65518bb..e43115f 100644
> >>>> --- a/arch/loongarch/kernel/entry.S
> >>>> +++ b/arch/loongarch/kernel/entry.S
> >>>> @@ -14,11 +14,13 @@
> >>>>   #include <asm/regdef.h>
> >>>>   #include <asm/stackframe.h>
> >>>>   #include <asm/thread_info.h>
> >>>> +#include <asm/unwind_hints.h>
> >>>>
> >>>>          .text
> >>>>          .cfi_sections   .debug_frame
> >>>>          .align  5
> >>>> -SYM_FUNC_START(handle_syscall)
> >>>> +SYM_CODE_START(handle_syscall)
> >>> Why?
> >>>
> >> see include/linux/linkage.h
> >> FUNC -- C-like functions (proper stack frame etc.)
> >> CODE -- non-C code (e.g. irq handlers with different, special stack etc.)
> > Hi, Jinyang,
> >
> > What do you think about it? In our internal repo, most asm functions
> > changed in this patch are still marked with FUNC, not CODE.
>
> Hi, Huacai,
>
>
> As the anotations in the include/linux/linkage.h, CODE should be used for
> exception handler in case where the stack at the start of the handler
> is unbalanced with the stack at the exit. In validate_branch,
> validate_return, and validate_sibling_call it will not check the stack.
> CODE needs HINT to describe the actual stack at the beginning of the CODE.
>
> In objtool's check flow, then entry check FUNC is validate_functions and
> the entry of check CODE is validate_unwind_hints. They actual check function
> is validate_branch. If ignore the stack check, they can get the same ORC
> info in most cases. In the internal repo, limited by what I knew about
> objtool
> at that time, I might have done something wrong.  e.g. NOT_SIBLING_CALL_HINT
> could be a way to ignore stack checks. These exception handler code logic in
> upstream is cleaner than that in the internal repo. So I hope this can be
> fixed in upstream first.
>
> handle_syscall is an example of a FUNC that looks stack balanced. However,
> the RA register at the entry is not the real RA, and its SP is also changed
> from user stack SP to kernel stack SP. So in fact, it is not stack balanced.
> It needs to be marked as CODE, and annotate HINT at the CODE entry to
> describe the actual stack (, usually described as undefined).
>
> In short, objtool is strictly dependent on canonical codes so that it can
> get the ORC information right.
Is the code in tlbex.S the same as handle_syscall()? If so, I suggest
submitting a separate patch to rename FUNC to CODE. That will be easy
to review, and can be upstream earlier because it is independent with
objtool.

Huacai

>
> Thanks,
>
> Jinyang
>
>
> >
> >>>> +       UNWIND_HINT_UNDEFINED
> >>>>          csrrd           t0, PERCPU_BASE_KS
> >> ...
> >>
> >>>> diff --git a/arch/loongarch/kernel/head.S b/arch/loongarch/kernel/head.S
> >>>> index 53b883d..5664390 100644
> >>>> --- a/arch/loongarch/kernel/head.S
> >>>> +++ b/arch/loongarch/kernel/head.S
> >>>> @@ -43,6 +43,7 @@ SYM_DATA(kernel_offset, .long _kernel_offset);
> >>>>          .align 12
> >>>>
> >>>>   SYM_CODE_START(kernel_entry)                   # kernel entry point
> >>>> +       UNWIND_HINT_EMPTY
> >>> I'm not sure but I think this isn't needed, because
> >>> "OBJECT_FILES_NON_STANDARD_head.o               :=y"
> >> Yes, you are right, will remove it.
> >>
> >>>>          /* Config direct window and set PG */
> >> ...
> >>
> >>>>   void __init setup_arch(char **cmdline_p)
> >>>>   {
> >>>> +       unwind_init();
> >>> I think this line should be after cpu_probe().
> >> I am OK to do this change, but if so, there are no stack trace before
> >> cpu_probe() for the early code.
> > As I said before, stack trace needs printk, but printk cannot work
> > before cpu_probe().
> >
> >>>>          cpu_probe();
> >>>>
> >>>>          init_environ();
> >> ...
> >>
> >>>> diff --git a/arch/loongarch/power/Makefile b/arch/loongarch/power/Makefile
> >>>> index 58151d0..bbd1d47 100644
> >>>> --- a/arch/loongarch/power/Makefile
> >>>> +++ b/arch/loongarch/power/Makefile
> >>>> @@ -1,3 +1,5 @@
> >>>> +OBJECT_FILES_NON_STANDARD_suspend_asm.o := y
> >>> hibernate_asm.o has no problem?
> >> Yes, only suspend_asm.o has one warning, just ignore it.
> > What kind of warning? When I submitted the suspend patch, Jinyang told
> > me that with his changes loongarch_suspend_enter() can be a regular
> > function.
> >
> > Huacai
>
> Hi, Tiezhu,
>
> We can think the jirl with link register is a call instruction.
> loongarch_suspend_enter:
>      jirl   a0, t0, 0 /* Call BIOS's STR sleep routine */
> Its link register is a0, (not ra), we also think it is a call
> instruction. The func is also stack banlaced. So the func can be a
> regular function.
>
> Thanks,
>
> Jinyang
>
>
> >> Thanks,
> >> Tiezhu
> >>
> >>
>
>

Powered by blists - more mailing lists