Hi Masahiro,
Thank you so much for the suggestions. I'll send the updated patch shortly.
Best,
-Rong
On Tue, Nov 12, 2024 at 5:40 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
>
> Please use "MIPS:" for the subject prefix
> because this patch only affects MIPS.
>
>
>
> On Wed, Nov 13, 2024 at 5:08 AM Rong Xu <xur@xxxxxxxxxx> wrote:
> >
> > With commit 622240ea8d71 ("vmlinux.lds.h: Adjust symbol ordering in
> > text output section"), symobls in sections .text.unlikely will be
> > placed before symbols in .text. This can lead to the misplacement
> > of _stext, which resides in the .text section, and result in a boot
> > hang.
> >
> > Explicitly set _stext to the beginning of text output section.
>
>
> I will insert this patch before 622240ea8d71 to avoid breakage.
>
> Please rephrase the commit description without referring to
> 622240ea8d71.
>
>
> For example, you can say as follows:
>
> ------------->-----------
> MIPS: move _stext definition to vmlinux.lds.S
>
> The _stext symbol is intended to reference the start of the text section.
> However, it currently relies on a fragile link order because the existing
> EXPORT(_stext) resides within the .text section, which is not guaranteed
> to be placed first.
>
> Move the _stext definition to the linker script to enforce an explicit
> ordering.
> ------------->-----------
>
>
> Please feel free to update the description, but this must be
> fixed regardless of your patch set.
>
> Even without your patch set, the .text section
> comes after .text.hot. So, it is broken.
>
> #define TEXT_TEXT \
> ALIGN_FUNCTION(); \
> *(.text.hot .text.hot.*) \
> *(TEXT_MAIN .text.fixup) \
>
>
>
>
> > Signed-off-by: Rong Xu <xur@xxxxxxxxxx>
> > Reported-by: Klara Modin <klarasmodin@xxxxxxxxx>
> > Tested-by: Klara Modin <klarasmodin@xxxxxxxxx>
> > ---
> > arch/mips/kernel/vmlinux.lds.S | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> > index 9ff55cb80a64..62c3db869a18 100644
> > --- a/arch/mips/kernel/vmlinux.lds.S
> > +++ b/arch/mips/kernel/vmlinux.lds.S
> > @@ -61,6 +61,11 @@ SECTIONS
> > /* read-only */
> > _text = .; /* Text and read-only data */
> > .text : {
> > + /* Explicitly set _stext to the beginning of text output
> > + section. _stext is in text section and might be matched
> > + after symbols in sections with a specific prefix (like
> > + .text.unlikely). */
>
> I do not think this comment is necessary
> because this is a common pattern.
>
> The typical linker script example is documented.
> https://github.com/torvalds/linux/blob/v6.11-rc7/include/asm-generic/vmlinux.lds.h#L21
>
>
>
>
> Many architectures define _stext in vmlinux.lds.S
> without such verbose comments.
>
> $ find . -name vmlinux.lds.S | xargs grep "_stext ="
> ./arch/mips/kernel/vmlinux.lds.S: _stext = . ;
> ./arch/openrisc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/xtensa/kernel/vmlinux.lds.S: _stext = .;
> ./arch/s390/kernel/vmlinux.lds.S: _stext = .; /* Start of text section */
> ./arch/nios2/kernel/vmlinux.lds.S: _stext = .;
> ./arch/loongarch/kernel/vmlinux.lds.S: _stext = .;
> ./arch/x86/kernel/vmlinux.lds.S: _stext = .;
> ./arch/riscv/kernel/vmlinux.lds.S: _stext = .;
> ./arch/parisc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/csky/kernel/vmlinux.lds.S: _stext = .;
> ./arch/arc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/arm64/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
> ./arch/arm/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
> ./arch/hexagon/kernel/vmlinux.lds.S: _stext = .;
> ./arch/powerpc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/microblaze/kernel/vmlinux.lds.S: _stext = . ;
>
>
>
>
> > + _stext = .;
>
> _etext is defined outside the .text {} block.
>
> If you want "_stext" and "_etext" to look symmetrical,
> perhaps you might want to change like this?
>
>
> diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> index 9ff55cb80a64..d575f945d422 100644
> --- a/arch/mips/kernel/vmlinux.lds.S
> +++ b/arch/mips/kernel/vmlinux.lds.S
> @@ -60,6 +60,7 @@ SECTIONS
> . = LINKER_LOAD_ADDRESS;
> /* read-only */
> _text = .; /* Text and read-only data */
> + _stext = .;
> .text : {
> TEXT_TEXT
> SCHED_TEXT
>
>
>
>
> If you move the _stext definition to the linker script,
> you can remove EXPORT(_stext) from
> arch/mips/kernel/head.S
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
