[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALALjgy60jZAOvFfD1HSfPNcSxhtuPq5D_1r=71TthP0B3ipQg@mail.gmail.com>
Date: Thu, 9 Jun 2022 11:03:33 +0300
From: Joe Damato <jdamato@...tly.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Andrew Cooper <Andrew.Cooper3@...rix.com>,
jiangshanlai@...il.com, bp@...e.de, brgerst@...il.com,
Byungchul Park <byungchul.park@....com>
Subject: Re: [PATCH v2] x86/mm: Fix RESERVE_BRK() for older binutils
On Thu, Jun 09, 2022 at 12:17:32AM -0700, Josh Poimboeuf wrote:
> With binutils 2.26, RESERVE_BRK() causes a build failure:
>
> /tmp/ccnGOKZ5.s: Assembler messages:
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: junk at end of line, first unrecognized
> character is `U'
>
> The problem is this line:
>
> RESERVE_BRK(early_pgt_alloc, INIT_PGT_BUF_SIZE)
>
> Specifically, the INIT_PGT_BUF_SIZE macro which (via PAGE_SIZE's use
> _AC()) has a "1UL", which makes older versions of the assembler unhappy.
> Unfortunately the _AC() macro doesn't work for inline asm.
>
> Inline asm was only needed here to convince the toolchain to add the
> STT_NOBITS flag. However, if a C variable is placed in a section whose
> name is prefixed with ".bss", GCC and Clang automatically set
> STT_NOBITS. In fact, ".bss..page_aligned" already relies on this trick.
>
> So fix the build failure (and simplify the macro) by allocating the
> variable in C.
>
> Also, add NOLOAD to the ".brk" output section clause in the linker
> script. This is a failsafe in case the ".bss" prefix magic trick ever
> stops working somehow. If there's a section type mismatch, the GNU
> linker will force the ".brk" output section to be STT_NOBITS. The LLVM
> linker will fail with a "section type mismatch" error.
>
> Note this also changes the name of the variable from .brk.##name to
> __brk_##name. The variable names aren't actually used anywhere, so it's
> harmless.
>
> Reported-by: Joe Damato <jdamato@...tly.com>
> Reported-by: Byungchul Park <byungchul.park@....com>
> Fixes: a1e2c031ec39 ("x86/mm: Simplify RESERVE_BRK()")
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> ---
> arch/x86/include/asm/setup.h | 38 +++++++++++++++++++----------------
> arch/x86/kernel/setup.c | 5 -----
> arch/x86/kernel/vmlinux.lds.S | 4 ++--
> 3 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 7590ac2570b9..f8b9ee97a891 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -108,19 +108,16 @@ extern unsigned long _brk_end;
> void *extend_brk(size_t size, size_t align);
>
> /*
> - * Reserve space in the brk section. The name must be unique within the file,
> - * and somewhat descriptive. The size is in bytes.
> + * Reserve space in the .brk section, which is a block of memory from which the
> + * caller is allowed to allocate very early (before even memblock is available)
> + * by calling extend_brk(). All allocated memory will be eventually converted
> + * to memblock. Any leftover unallocated memory will be freed.
> *
> - * The allocation is done using inline asm (rather than using a section
> - * attribute on a normal variable) in order to allow the use of @nobits, so
> - * that it doesn't take up any space in the vmlinux file.
> + * The size is in bytes.
> */
> -#define RESERVE_BRK(name, size) \
> - asm(".pushsection .brk_reservation,\"aw\",@nobits\n\t" \
> - ".brk." #name ":\n\t" \
> - ".skip " __stringify(size) "\n\t" \
> - ".size .brk." #name ", " __stringify(size) "\n\t" \
> - ".popsection\n\t")
> +#define RESERVE_BRK(name, size) \
> + __section(".bss..brk") __aligned(1) __used \
> + static char __brk_##name[size]
>
> extern void probe_roms(void);
> #ifdef __i386__
> @@ -133,12 +130,19 @@ asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
>
> #endif /* __i386__ */
> #endif /* _SETUP */
> -#else
> -#define RESERVE_BRK(name,sz) \
> - .pushsection .brk_reservation,"aw",@nobits; \
> -.brk.name: \
> -1: .skip sz; \
> - .size .brk.name,.-1b; \
> +
> +#else /* __ASSEMBLY */
> +
> +.macro __RESERVE_BRK name, size
> + .pushsection .bss..brk, "aw"
> +SYM_DATA_START(__brk_\name)
> + .skip \size
> +SYM_DATA_END(__brk_\name)
> .popsection
> +.endm
> +
> +#define RESERVE_BRK(name, size) __RESERVE_BRK name, size
> +
> #endif /* __ASSEMBLY__ */
> +
> #endif /* _ASM_X86_SETUP_H */
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 3ebb85327edb..bd6c6fd373ae 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -67,11 +67,6 @@ RESERVE_BRK(dmi_alloc, 65536);
> #endif
>
>
> -/*
> - * Range of the BSS area. The size of the BSS area is determined
> - * at link time, with RESERVE_BRK() facility reserving additional
> - * chunks.
> - */
> unsigned long _brk_start = (unsigned long)__brk_base;
> unsigned long _brk_end = (unsigned long)__brk_base;
>
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index f5f6dc2e8007..81aba718ecd5 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -385,10 +385,10 @@ SECTIONS
> __end_of_kernel_reserve = .;
>
> . = ALIGN(PAGE_SIZE);
> - .brk : AT(ADDR(.brk) - LOAD_OFFSET) {
> + .brk (NOLOAD) : AT(ADDR(.brk) - LOAD_OFFSET) {
> __brk_base = .;
> . += 64 * 1024; /* 64k alignment slop space */
> - *(.brk_reservation) /* areas brk users have reserved */
> + *(.bss..brk) /* areas brk users have reserved */
> __brk_limit = .;
> }
>
> --
> 2.34.3
>
I applied the v2 patch on top of commit 58f9d52ff689 ("Merge tag
'net-5.19-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net") and the kernel
builds successfully for me.
The resulting kernel boots fine on the machine, as well.
I built the kernel with:
- gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609
- binutils 2.26.1-1ubuntu1~16.04.8+esm1
I have not tested with any other build toolchains.
Tested-by: Joe Damato <jdamato@...tly.com>
Powered by blists - more mailing lists