[PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Rong Xu]

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.

Signed-off-by: Rong Xu <xur@google.com>
Reported-by: Klara Modin <klarasmodin@gmail.com>
Tested-by: Klara Modin <klarasmodin@gmail.com>
---
V2 Changelog:
Incorporated Masahiro Yamada's suggestions:
1. Refined the commit message
2. Removed unnecessary comments
3. Use a standardized way for _stext definition
---
 arch/mips/kernel/head.S        | 2 --
 arch/mips/kernel/vmlinux.lds.S | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
index b825ed4476c7..e90695b2b60e 100644
--- a/arch/mips/kernel/head.S
+++ b/arch/mips/kernel/head.S
@@ -67,8 +67,6 @@
 	.fill	0x400
 #endif
 
-EXPORT(_stext)
-
 #ifdef CONFIG_BOOT_RAW
 	/*
 	 * Give us a fighting chance of running if execution beings at the
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

base-commit: 06513ddaf77b8f49ef8540c92d92c9ef0ad49426
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Masahiro Yamada]

Hi Thomas,

This patch is necessary to avoid regression in my kbuild tree.

Your Ack is appreciated.




On Wed, Nov 13, 2024 at 3:40 PM Rong Xu <xur@google.com> wrote:
>
> 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.
>
> Signed-off-by: Rong Xu <xur@google.com>
> Reported-by: Klara Modin <klarasmodin@gmail.com>
> Tested-by: Klara Modin <klarasmodin@gmail.com>
> ---
> V2 Changelog:
> Incorporated Masahiro Yamada's suggestions:
> 1. Refined the commit message
> 2. Removed unnecessary comments
> 3. Use a standardized way for _stext definition
> ---
>  arch/mips/kernel/head.S        | 2 --
>  arch/mips/kernel/vmlinux.lds.S | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
> index b825ed4476c7..e90695b2b60e 100644
> --- a/arch/mips/kernel/head.S
> +++ b/arch/mips/kernel/head.S
> @@ -67,8 +67,6 @@
>         .fill   0x400
>  #endif
>
> -EXPORT(_stext)
> -
>  #ifdef CONFIG_BOOT_RAW
>         /*
>          * Give us a fighting chance of running if execution beings at the
> 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
>
> base-commit: 06513ddaf77b8f49ef8540c92d92c9ef0ad49426
> --
> 2.47.0.338.g60cca15819-goog
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Maciej W. Rozycki]

On Tue, 12 Nov 2024, Rong Xu wrote:

> 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.

 Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it 
for environments where there's no entry point is available and execution 
starts from the beginning of the image.  See the comment right below your 
change.

> Move the _stext definition to the linker script to enforce an explicit
> ordering.

 So if you say that the link order is fragile (which it may well be), then 
that problem has to be fixed instead, likely with the linker script too, 
and then perhaps an ASSERT placed there to verify that it has worked and 
`_stext' refers to the beginning, taking into account what follows too.

 Also note that `_stext' currently points beyond the space reserved for
exception handlers.  Have you analysed what the consequences would be if 
it was moved ahead of it, which your change does AFAICT?

  Maciej

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Rong Xu]

"

On Wed, Nov 13, 2024 at 7:55 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Tue, 12 Nov 2024, Rong Xu wrote:
>
> > 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.
>
>  Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it
> for environments where there's no entry point is available and execution
> starts from the beginning of the image.  See the comment right below your
> change.
>
When you said "arch/mips/kernel/head.S does mean to be linked first", is it
a hard requirement in mips? This patch only moves _stext but leaves other
symbols from heads.S for TEXT_TEXT macro to order. For example,
__kernel_entry is placed in the middle of the text segment.

If we want head.S to be linked first, I can change the patch to place
all symbols from head.S before TEXT_TEXT.
This way, we can also resolve your concerns about moving _stext byond
the exception handlers.

I tried that method, and the symbol order is like the following:
0100000 T _text
80100400 T __kernel_entry
80100400 T _stext
80100410 T file_ra_state_init
80100450 T readahead_expand
80100710 t read_pages
80100a04 T page_cache_ra_unbounded
80100c10 t do_page_cache_ra
80100c9c T force_page_cache_ra
80100d50 T page_cache_ra_order
80100d8c T page_cache_sync_ra
80100ff4 T page_cache_async_ra
801011b8 T ksys_readahead
801012cc T __se_sys_readahead
801012cc T sys_readahead
801012e0 T __split_text_end
801012e0 T __split_text_start
801012e0 t __sync
...
Note that the reserve space for exceptions is before _stext

> > Move the _stext definition to the linker script to enforce an explicit
> > ordering.
>
>  So if you say that the link order is fragile (which it may well be), then
> that problem has to be fixed instead, likely with the linker script too,

I think what we meant was the way the current vmlinux.lds.S (linker
script) written
makes the symbol ordering fragile. This patch is to fix this.

> and then perhaps an ASSERT placed there to verify that it has worked and
> `_stext' refers to the beginning, taking into account what follows too.
>
>  Also note that `_stext' currently points beyond the space reserved for
> exception handlers.  Have you analysed what the consequences would be if
> it was moved ahead of it, which your change does AFAICT?

I did not analyze this. I think it's fine. From section boundaries guideline in
include/asm-generic/sections.h
It seems ok to me that exception handlers are included in [_stext, _etext].
But I admit that I'm not an expert here.
We can use the method I mentioned above to keep current behavior.

>   Maciej

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Masahiro Yamada]

On Thu, Nov 14, 2024 at 12:55 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Tue, 12 Nov 2024, Rong Xu wrote:
>
> > 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.
>
>  Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it
> for environments where there's no entry point is available and execution
> starts from the beginning of the image.  See the comment right below your
> change.
>
> > Move the _stext definition to the linker script to enforce an explicit
> > ordering.
>
>  So if you say that the link order is fragile (which it may well be), then
> that problem has to be fixed instead, likely with the linker script too,
> and then perhaps an ASSERT placed there to verify that it has worked and
> `_stext' refers to the beginning, taking into account what follows too.


arch/mips/kernel/head.S is always passed as the first object
in the link command because it is listed in scripts/head-object-list.txt

What you missed to understand is, the .text section of the first object
is NOT guaranteed to be placed at the start of the image.


Assume, we pass 3 objects, head.o, foo.o, bar.o to the linker
in this order.

- head.o  contains a .text section
- foo.o contains .text and .text.hot sections
- bar.o contains .text and .text.hot sections


The output will contain the sections in this order:
   foo.o#.text.hot
   bar.o#.text.hot
   head.o#.text
   foo.o#.text
   bar.o#.text


This result comes from the fact that TEXT_MAIN
is not necessarily placed first.

See the macro in include/asm-generic/vmlinux.lds.h

#define TEXT_TEXT                                                       \
                ALIGN_FUNCTION();                                       \
                *(.text.hot .text.hot.*)                                \
                *(TEXT_MAIN .text.fixup)                                \
                *(.text.unlikely .text.unlikely.*)                      \
                *(.text.unknown .text.unknown.*)                        \




BTW, "head.o must be passed to the linker as the first object"
is a bad convention in old days.
If you expect the entry point at the beginning of the kernel image,
it must be marked as __HEAD, which is placed in the .head.text section.

See commit ce697ccee1a8

Well-maintained architectures got rid of
stupid "head.o must be passed first" requirement:

 - 2348e6bf4421
 - 994b7ac1697b
 - 5353fff29e42

If MIPS migrates to the cleaner __HEAD solution,
it will be appreciated, but this is another story.




>  Also note that `_stext' currently points beyond the space reserved for
> exception handlers.  Have you analysed what the consequences would be if
> it was moved ahead of it, which your change does AFAICT?
>
>   Maciej







-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Masahiro Yamada]

On Thu, Nov 14, 2024 at 3:13 AM Rong Xu <xur@google.com> wrote:
>
> "
>
> On Wed, Nov 13, 2024 at 7:55 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> > On Tue, 12 Nov 2024, Rong Xu wrote:
> >
> > > 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.
> >
> >  Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it
> > for environments where there's no entry point is available and execution
> > starts from the beginning of the image.  See the comment right below your
> > change.
> >
> When you said "arch/mips/kernel/head.S does mean to be linked first", is it
> a hard requirement in mips? This patch only moves _stext but leaves other
> symbols from heads.S for TEXT_TEXT macro to order. For example,
> __kernel_entry is placed in the middle of the text segment.
>
> If we want head.S to be linked first, I can change the patch to place
> all symbols from head.S before TEXT_TEXT.

No change is needed for this.

arch/mips/kernel/head.o is always passed to the linker first.

This is guaranteed by scripts/head-object-list.txt





-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Maciej W. Rozycki]

On Thu, 14 Nov 2024, Masahiro Yamada wrote:

> > > 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.
> >
> >  Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it
> > for environments where there's no entry point is available and execution
> > starts from the beginning of the image.  See the comment right below your
> > change.
> >
> > > Move the _stext definition to the linker script to enforce an explicit
> > > ordering.
> >
> >  So if you say that the link order is fragile (which it may well be), then
> > that problem has to be fixed instead, likely with the linker script too,
> > and then perhaps an ASSERT placed there to verify that it has worked and
> > `_stext' refers to the beginning, taking into account what follows too.
> 
> 
> arch/mips/kernel/head.S is always passed as the first object
> in the link command because it is listed in scripts/head-object-list.txt
> 
> What you missed to understand is, the .text section of the first object
> is NOT guaranteed to be placed at the start of the image.

 I know how linker scripts work, thank you very much.  However there was 
nothing to understand from the commit description as hardly any has been 
given.

> Assume, we pass 3 objects, head.o, foo.o, bar.o to the linker
> in this order.
> 
> - head.o  contains a .text section
> - foo.o contains .text and .text.hot sections
> - bar.o contains .text and .text.hot sections
> 
> 
> The output will contain the sections in this order:
>    foo.o#.text.hot
>    bar.o#.text.hot
>    head.o#.text
>    foo.o#.text
>    bar.o#.text
> 
> 
> This result comes from the fact that TEXT_MAIN
> is not necessarily placed first.
> 
> See the macro in include/asm-generic/vmlinux.lds.h
> 
> #define TEXT_TEXT                                                       \
>                 ALIGN_FUNCTION();                                       \
>                 *(.text.hot .text.hot.*)                                \
>                 *(TEXT_MAIN .text.fixup)                                \
>                 *(.text.unlikely .text.unlikely.*)                      \
>                 *(.text.unknown .text.unknown.*)                        \

 It corresponds to what I suggested in the second paragraph of my previous 
reply, which I retained quoted above for your convenience.  The reviewer 
is not required to give the submitter a complete solution, but rather some 
guidance for the submitter to find the correct solution themselves.

> BTW, "head.o must be passed to the linker as the first object"
> is a bad convention in old days.
> If you expect the entry point at the beginning of the kernel image,
> it must be marked as __HEAD, which is placed in the .head.text section.
> 
> See commit ce697ccee1a8

 I do not disagree, however such details need to be given in the change 
description.  The purpose of the change description is not to repeat in 
the texual form what the patch does, because everyone can see it, but to 
give rationale and any reasoning beyond the change being made, especially 
when fiddling with something that has worked for 30 years now.

 It's not that the old is always good, but at least it has proved in the 
field, so you need to convince the reviewer why the new is more suitable.

> Well-maintained architectures got rid of
> stupid "head.o must be passed first" requirement:
> 
>  - 2348e6bf4421
>  - 994b7ac1697b
>  - 5353fff29e42
> 
> If MIPS migrates to the cleaner __HEAD solution,
> it will be appreciated, but this is another story.

 Patches are welcome.

  Maciej

Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

[Masahiro Yamada]

On Wed, Nov 13, 2024 at 3:40 PM Rong Xu <xur@google.com> wrote:
>
> 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.
>
> Signed-off-by: Rong Xu <xur@google.com>
> Reported-by: Klara Modin <klarasmodin@gmail.com>
> Tested-by: Klara Modin <klarasmodin@gmail.com>
> --

Applied to linux-kbuild.
(inserted before "vmlinux.lds.h: Adjust symbol ordering in text output section")

Thanks.



--
Best Regards
Masahiro Yamada