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;
MIME-Version: 1.0
References: <1693290485-3516-1-git-send-email-yangtiezhu@loongson.cn>
 <b826520103c7462b0c2f10a7bc69347799d1217b.camel@xry111.site> <a4bd839b-d55e-07e5-67a5-016d77f77112@loongson.cn>
In-Reply-To: <a4bd839b-d55e-07e5-67a5-016d77f77112@loongson.cn>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Tue, 29 Aug 2023 20:47:32 +0800
Message-ID: <CAAhV-H7BDyeU5UNOV2QJm+vOKyk1_1K2bnS8TKMyLUZ57A4++g@mail.gmail.com>
Subject: Re: [PATCH] LoongArch: Define the symbol fault as a local label in fpu.S
To:     Tiezhu Yang <yangtiezhu@loongson.cn>
Cc:     Xi Ruoyao <xry111@xry111.site>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org, loongson-kernel@lists.loongnix.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 29, 2023 at 8:40=E2=80=AFPM Tiezhu Yang <yangtiezhu@loongson.cn=
> wrote:
>
>
>
> On 08/29/2023 02:45 PM, Xi Ruoyao wrote:
> > On Tue, 2023-08-29 at 14:28 +0800, Tiezhu Yang wrote:
> >> The initial aim is to silence the following objtool warnings:
> >>
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_fp_context() fa=
lls through to next function fault()
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_fp_context()=
 falls through to next function fault()
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_lsx_context() f=
alls through to next function fault()
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_lsx_context(=
) falls through to next function fault()
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_lasx_context() =
falls through to next function fault()
> >>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_lasx_context=
() falls through to next function fault()
> >>
> >> Obviously, the symbol fault is not a function, it is just a local labe=
l,
> >
> > Hmm why?  To me this seems a function.  We don't branch to it but store
> > its address (a "function pointer") in the extable.
> >
> > And these warnings do not make any sense to me:
> >
> > /* snip */
> >
> >> diff --git a/arch/loongarch/kernel/fpu.S b/arch/loongarch/kernel/fpu.S
> >> index b4032de..7defe50 100644
> >> --- a/arch/loongarch/kernel/fpu.S
> >> +++ b/arch/loongarch/kernel/fpu.S
> >> @@ -521,7 +521,7 @@ SYM_FUNC_START(_restore_lasx_context)
> >>         jr      ra
> >
> > _restore_lasx_context returns with this instruction.  How can it fall
> > through into fault?
> >
> >>  SYM_FUNC_END(_restore_lasx_context)
> >
> >> -SYM_FUNC_START(fault)
> >> +SYM_CODE_START_LOCAL(fault)
> >>         li.w    a0, -EFAULT                             # failure
> >>         jr      ra
> >> -SYM_FUNC_END(fault)
> >> +SYM_CODE_END(fault)
> >
>
> In the current code, SYM_FUNC_END() defines the symbol as SYM_T_FUNC
> which is STT_FUNC, the objtool warnings are generated through the
> following code.
>
> arch/loongarch/include/asm/linkage.h
> #define SYM_FUNC_END(name)                              \
>         .cfi_endproc;                                   \
>         SYM_END(name, SYM_T_FUNC)
>
> include/linux/linkage.h
> /* SYM_T_FUNC -- type used by assembler to mark functions */
> #ifndef SYM_T_FUNC
> #define SYM_T_FUNC                              STT_FUNC
> #endif
>
> tools/objtool/include/objtool/check.h
> static inline struct symbol *insn_func(struct instruction *insn)
> {
>         struct symbol *sym =3D insn->sym;
>
>         if (sym && sym->type !=3D STT_FUNC)
>                 sym =3D NULL;
>
>         return sym;
> }
>
> tools/objtool/check.c
> static int validate_branch(struct objtool_file *file, struct symbol *func=
,
>                            struct instruction *insn, struct insn_state st=
ate)
> {
> ...
>         while (1) {
>                 next_insn =3D next_insn_to_validate(file, insn);
>
>                 if (func && insn_func(insn) && func !=3D insn_func(insn)-=
>pfunc) {
>                         /* Ignore KCFI type preambles, which always fall =
through */
>                         if (!strncmp(func->name, "__cfi_", 6) ||
>                             !strncmp(func->name, "__pfx_", 6))
>                                 return 0;
>
>                         WARN("%s() falls through to next function %s()",
>                              func->name, insn_func(insn)->name);
>                         return 1;
>                 }
> ...
> }
>
> We can see that the fixup can be a local label in the following code.
>
> arch/loongarch/include/asm/asm-extable.h
> #define __ASM_EXTABLE_RAW(insn, fixup, type, data)      \
>         .pushsection    __ex_table, "a";                \
>         .balign         4;                              \
>         .long           ((insn) - .);                   \
>         .long           ((fixup) - .);                  \
>         .short          (type);                         \
>         .short          (data);                         \
>         .popsection;
>
>         .macro          _asm_extable, insn, fixup
>         __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0)
>         .endm
>
> Like arch/loongarch/lib/*.S does, I prefer the following changes,
> if you are ok, I will send v2 later.
>
> diff --git a/arch/loongarch/kernel/fpu.S b/arch/loongarch/kernel/fpu.S
> index b4032deb8e3b..3177725ea832 100644
> --- a/arch/loongarch/kernel/fpu.S
> +++ b/arch/loongarch/kernel/fpu.S
> @@ -22,7 +22,7 @@
>
>          .macro  EX insn, reg, src, offs
>   .ex\@: \insn   \reg, \src, \offs
> -       _asm_extable .ex\@, fault
> +       _asm_extable .ex\@, .L_fixup_fpu_fault
>          .endm
>
>          .macro sc_save_fp base
> @@ -521,7 +521,6 @@ SYM_FUNC_START(_restore_lasx_context)
>          jr      ra
>   SYM_FUNC_END(_restore_lasx_context)
>
> -SYM_FUNC_START(fault)
> +.L_fixup_fpu_fault:
>          li.w    a0, -EFAULT                             # failure
>          jr      ra
> -SYM_FUNC_END(fault)
You can only fix the fpu.S, lbt.S part can be squashed to its own patch.

Huacai

> diff --git a/arch/loongarch/kernel/lbt.S b/arch/loongarch/kernel/lbt.S
> index ecf08bbff650..402eb8ec4024 100644
> --- a/arch/loongarch/kernel/lbt.S
> +++ b/arch/loongarch/kernel/lbt.S
> @@ -16,7 +16,7 @@
>
>          .macro  EX insn, reg, src, offs
>   .ex\@: \insn   \reg, \src, \offs
> -       _asm_extable .ex\@, lbt_fault
> +       _asm_extable .ex\@, .L_fixup_lbt_fault
>          .endm
>
>   /*
> @@ -150,7 +150,6 @@ SYM_FUNC_START(_restore_ftop_context)
>          jr              ra
>   SYM_FUNC_END(_restore_ftop_context)
>
> -SYM_FUNC_START(lbt_fault)
> +.L_fixup_lbt_fault:
>          li.w            a0, -EFAULT             # failure
>          jr              ra
> -SYM_FUNC_END(lbt_fault)
>
> Thanks,
> Tiezhu
>