Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <a802eefebee4d2c01f479a7d3f2008fdd32ce270.1654702810.git.jpoimboe@kernel.org>
In-Reply-To: <a802eefebee4d2c01f479a7d3f2008fdd32ce270.1654702810.git.jpoimboe@kernel.org>
From:   Joe Damato <jdamato@fastly.com>
Date:   Thu, 9 Jun 2022 01:12:09 +0300
Message-ID: <CALALjgy6=Ebi-k-YrSsEozW3Yy4KJGWLiH_5M8i4neEd9ozj_A@mail.gmail.com>
Subject: Re: [PATCH] x86/mm: Fix RESERVE_BRK() for older binutils
To:     Josh Poimboeuf <jpoimboe@kernel.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        jiangshanlai@gmail.com, bp@suse.de, brgerst@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 08, 2022 at 08:40:42AM -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.
>
> Fix it (and further simplify RESERVE_BRK) by taking a completely
> different approach.  Instead of specifying the "nobits" at compile-time,
> do so at link-time.  Then the variable can just be allocated in C.
>
> Note this 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@fastly.com>
> Fixes: a1e2c031ec39 ("x86/mm: Simplify RESERVE_BRK()")
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  arch/x86/include/asm/setup.h  | 35 +++++++++++++++++------------------
>  arch/x86/kernel/vmlinux.lds.S |  2 +-
>  2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 7590ac2570b9..ef9bb6caad8c 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -108,19 +108,11 @@ 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.
> - *
> - * 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.
> + * Reserve space in the .brk section.  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(".brk_reservation") __aligned(1) __used       \
> +     static char __brk_##name[size]
>
>  extern void probe_roms(void);
>  #ifdef __i386__
> @@ -133,12 +125,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 .brk_reservation, "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/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index f5f6dc2e8007..9b63f8a00b4f 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -385,7 +385,7 @@ 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 */
> --
> 2.34.3
>

I applied the 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
now builds successfully for me.

The resulting kernel boots fine on the machine, as well.

Tested-by: Joe Damato <jdamato@fastly.com>