Date:   Sun, 15 Oct 2023 20:57:54 +0800
From:   Jinyang He <hejinyang@...ngson.cn>
To:     Huacai Chen <chenhuacai@...nel.org>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     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

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.

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