Re: [PATCH v2 8/8] LoongArch: Add ORC unwinder support

[Huacai Chen]

+CC Jinyang

On Sat, Oct 14, 2023 at 5:21 PM Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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.

>
> >> +       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
>
> Thanks,
> Tiezhu
>
>