Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 24 May 2020 16:49:31 -0700
From:   Fangrui Song <maskray@google.com>
To:     Arvind Sankar <nivedita@alum.mit.edu>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Nick Desaulniers <ndesaulniers@google.com>,
        Dmitry Golovin <dima@golovin.in>,
        clang-built-linux@googlegroups.com,
        Ard Biesheuvel <ardb@kernel.org>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Daniel Kiper <daniel.kiper@oracle.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] x86/boot: Add .text.startup to setup.ld
Message-ID: <20200524234931.onovv64vjvrsfj33@google.com>
References: <CAKwvOd=qB+EoJwfAYUA9Hg7f9op4Q4W+TDnht8pLRG5bPX=29Q@mail.gmail.com>
 <20200524212816.243139-2-nivedita@alum.mit.edu>
 <20200524221326.zo6zthdqecm2ke5t@google.com>
 <20200524230054.GA280334@rani.riverdale.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20200524230054.GA280334@rani.riverdale.lan>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-05-24, Arvind Sankar wrote:
>On Sun, May 24, 2020 at 03:13:26PM -0700, Fangrui Song wrote:
>> On 2020-05-24, Arvind Sankar wrote:
>> >gcc puts the main function into .text.startup when compiled with -Os (or
>> >-O2). This results in arch/x86/boot/main.c having a .text.startup
>> >section which is currently not included explicitly in the linker script
>> >setup.ld in the same directory.
>> >
>> >The BFD linker places this orphan section immediately after .text, so
>> >this still works. However, LLD git, since [1], is choosing to place it
>> >immediately after the .bstext section instead (this is the first code
>> >section). This plays havoc with the section layout that setup.elf
>> >requires to create the setup header, for eg on 64-bit:
>> >
>> >    LD      arch/x86/boot/setup.elf
>> >  ld.lld: error: section .text.startup file range overlaps with .header
>> >  >>> .text.startup range is [0x200040, 0x2001FE]
>> >  >>> .header range is [0x2001EF, 0x20026B]
>> >
>> >  ld.lld: error: section .header file range overlaps with .bsdata
>> >  >>> .header range is [0x2001EF, 0x20026B]
>> >  >>> .bsdata range is [0x2001FF, 0x200398]
>> >
>> >  ld.lld: error: section .bsdata file range overlaps with .entrytext
>> >  >>> .bsdata range is [0x2001FF, 0x200398]
>> >  >>> .entrytext range is [0x20026C, 0x2002D3]
>> >
>> >  ld.lld: error: section .text.startup virtual address range overlaps
>> >  with .header
>> >  >>> .text.startup range is [0x40, 0x1FE]
>> >  >>> .header range is [0x1EF, 0x26B]
>> >
>> >  ld.lld: error: section .header virtual address range overlaps with
>> >  .bsdata
>> >  >>> .header range is [0x1EF, 0x26B]
>> >  >>> .bsdata range is [0x1FF, 0x398]
>> >
>> >  ld.lld: error: section .bsdata virtual address range overlaps with
>> >  .entrytext
>> >  >>> .bsdata range is [0x1FF, 0x398]
>> >  >>> .entrytext range is [0x26C, 0x2D3]
>> >
>> >  ld.lld: error: section .text.startup load address range overlaps with
>> >  .header
>> >  >>> .text.startup range is [0x40, 0x1FE]
>> >  >>> .header range is [0x1EF, 0x26B]
>> >
>> >  ld.lld: error: section .header load address range overlaps with
>> >  .bsdata
>> >  >>> .header range is [0x1EF, 0x26B]
>> >  >>> .bsdata range is [0x1FF, 0x398]
>> >
>> >  ld.lld: error: section .bsdata load address range overlaps with
>> >  .entrytext
>> >  >>> .bsdata range is [0x1FF, 0x398]
>> >  >>> .entrytext range is [0x26C, 0x2D3]
>> >
>> >Explicitly pull .text.startup into the .text output section to avoid
>> >this.
>> >
>> >[1] https://reviews.llvm.org/D75225
>> >
>> >Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
>> >Reviewed-by: Fangrui Song <maskray@google.com>
>> >---
>> > arch/x86/boot/setup.ld | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
>> >index 24c95522f231..ed60abcdb089 100644
>> >--- a/arch/x86/boot/setup.ld
>> >+++ b/arch/x86/boot/setup.ld
>> >@@ -20,7 +20,7 @@ SECTIONS
>> > 	.initdata	: { *(.initdata) }
>> > 	__end_init = .;
>> >
>> >-	.text		: { *(.text) }
>> >+	.text		: { *(.text.startup) *(.text) }
>> > 	.text32		: { *(.text32) }
>> >
>> > 	. = ALIGN(16);
>> >--
>> >2.26.2
>>
>> Should .text.startup* be used instead? If -ffunction-sections is used,
>>
>> // a.c
>> int main() {}
>>
>> gcc -O2 a.c                     # .text.startup
>> gcc -Os a.c                     # .text.startup
>>
>> gcc -O2 -ffunction-sections a.c # .text.startup.main
>> gcc -Os -ffunction-sections a.c # .text.startup.main
>
>It's probably unlikely we'll use function-sections on the setup code,
>but *(.text.*) might be more future-proof, since gcc/clang might grow
>the ability to stick code into .text.hot or .text.unlikely etc
>automatically.

*(.text.*) looks good to me. When you send PATCH v2, feel free to add

(There is indeed no guarantee that future clang FDO will not place it .text.unknown. :)

Reviewed-by: Fangrui Song <maskray@google.com>

>>
>> -----
>>
>> In case anyone wants to CC a GCC dev for the citation that
>>   main compiles to `.text.startup` in -Os or -O2 mode, I have a small request
>>   that `.text.startup.` probably makes more sense. See
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95095
>>
>> I made an llvm change recently https://reviews.llvm.org/D79600