| Message ID | 20241113064028.2795128-1-xur@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] MIPS: move _stext definition to vmlinux.lds.S | expand |
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 >
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
" 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
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
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
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
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
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