Messages in this thread | | | From | Huacai Chen <> | Date | Sun, 15 Oct 2023 21:57:54 +0800 | 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@loongson.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@loongson.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@loongson.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 > >> > >> > >
| |