[PATCH] firmware_loader: Align .builtin_fw to 8

[PATCH] firmware_loader: Align .builtin_fw to 8

[Fangrui Song]

arm64 references the start address of .builtin_fw (__start_builtin_fw)
with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
relocations. The compiler is allowed to emit the
R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
include/linux/firmware.h is 8-byte aligned.

The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
multiple of 8, which may not be the case if .builtin_fw is empty.
Unconditionally align .builtin_fw to fix the linker error.

Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
Link: https://github.com/ClangBuiltLinux/linux/issues/1204
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Fangrui Song <maskray@google.com>
---
 include/asm-generic/vmlinux.lds.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535..3cd4bd1193ab 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -459,6 +459,7 @@
 	}								\
 									\
 	/* Built-in firmware blobs */					\
+	ALIGN_FUNCTION();						\
 	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
 		__start_builtin_fw = .;					\
 		KEEP(*(.builtin_fw))					\
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH] firmware_loader: Align .builtin_fw to 8

[Arnd Bergmann]

On Thu, Dec 3, 2020 at 6:05 PM 'Fangrui Song' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> arm64 references the start address of .builtin_fw (__start_builtin_fw)
> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> relocations. The compiler is allowed to emit the
> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> include/linux/firmware.h is 8-byte aligned.
>
> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> multiple of 8, which may not be the case if .builtin_fw is empty.
> Unconditionally align .builtin_fw to fix the linker error.
>
> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fangrui Song <maskray@google.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

I found the same thing in randconfig testing, but you beat me to
sending the fix.

Re: [PATCH] firmware_loader: Align .builtin_fw to 8

[Nick Desaulniers]

On Thu, Dec 3, 2020 at 9:05 AM Fangrui Song <maskray@google.com> wrote:
>
> arm64 references the start address of .builtin_fw (__start_builtin_fw)
> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> relocations. The compiler is allowed to emit the
> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> include/linux/firmware.h is 8-byte aligned.
>
> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> multiple of 8, which may not be the case if .builtin_fw is empty.
> Unconditionally align .builtin_fw to fix the linker error.
>
> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fangrui Song <maskray@google.com>
> ---
>  include/asm-generic/vmlinux.lds.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index b2b3d81b1535..3cd4bd1193ab 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -459,6 +459,7 @@
>         }                                                               \
>                                                                         \
>         /* Built-in firmware blobs */                                   \
> +       ALIGN_FUNCTION();                                               \

Thanks for the patch!

I'm going to repeat my question from the above link
(https://github.com/ClangBuiltLinux/linux/issues/1204#issuecomment-737610582)
just in case it's not naive:

ALIGN_FUNCTION() C preprocessor macro seems to be used to realign
code, while STRUCT_ALIGN() seems to be used to realign data.  It looks
to me like only data is put into .builtin_fw.  If these relocations
require an alignment of 8, than multiples of 8 should also be fine
(STRUCT_ALIGN in 32 for all toolchain version, except gcc 4.9 which is
64; both are multiples of 8 though).  It looks like only structs are
placed in .builtin_fw; ie. data.  In that case, I worry that using
ALIGN_FUNCTION/8 might actually be under-aligning data in this
section.

Though, in https://github.com/ClangBuiltLinux/linux/issues/1204#issuecomment-737625134
you're comment:

>> In GNU ld, the empty .builtin_fw is removed

So that's a difference in behavior between ld.bfd and ld.lld, which is
fine, but it makes me wonder whether we should instead or additionally
be discarding this section explicitly via linker script when
CONFIG_FW_LOADER is not set?


>         .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>                 __start_builtin_fw = .;                                 \
>                 KEEP(*(.builtin_fw))                                    \
> --
> 2.29.2.576.ga3fc446d84-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] firmware_loader: Align .builtin_fw to 8

[Fangrui Song]

On 2020-12-03, Nick Desaulniers wrote:
>On Thu, Dec 3, 2020 at 9:05 AM Fangrui Song <maskray@google.com> wrote:
>>
>> arm64 references the start address of .builtin_fw (__start_builtin_fw)
>> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
>> relocations. The compiler is allowed to emit the
>> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
>> include/linux/firmware.h is 8-byte aligned.
>>
>> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
>> multiple of 8, which may not be the case if .builtin_fw is empty.
>> Unconditionally align .builtin_fw to fix the linker error.
>>
>> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Fangrui Song <maskray@google.com>
>> ---
>>  include/asm-generic/vmlinux.lds.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index b2b3d81b1535..3cd4bd1193ab 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -459,6 +459,7 @@
>>         }                                                               \
>>                                                                         \
>>         /* Built-in firmware blobs */                                   \
>> +       ALIGN_FUNCTION();                                               \
>
>Thanks for the patch!
>
>I'm going to repeat my question from the above link
>(https://github.com/ClangBuiltLinux/linux/issues/1204#issuecomment-737610582)
>just in case it's not naive:
>
>ALIGN_FUNCTION() C preprocessor macro seems to be used to realign
>code, while STRUCT_ALIGN() seems to be used to realign data.  It looks
>to me like only data is put into .builtin_fw.  If these relocations
>require an alignment of 8, than multiples of 8 should also be fine
>(STRUCT_ALIGN in 32 for all toolchain version, except gcc 4.9 which is
>64; both are multiples of 8 though).  It looks like only structs are
>placed in .builtin_fw; ie. data.  In that case, I worry that using
>ALIGN_FUNCTION/8 might actually be under-aligning data in this
>section.

Regarding STRUCT_ALIGN (32 for GCC>4.9) in
include/asm-generic/vmlinux.lds.h, it is probably not suitable for
.builtin_fw

* Its comment is a bit unclear. It probably should mention that the
   32-byte overalignment is only for global structure variables which are
   at least 32 byte large. But this is just my observation. Adding a GCC
   maintainer to comment on this.
* Even if GCC does overalign defined global struct variables, it is unlikely
   that GCC will leverage this property for undefined `extern struct
   builtin_fw __start_builtin_fw[]` (drivers/base/firmware_loader/main.c)

To make .builtin_fw aligned, I agree that ALIGN_FUNCTION() is probably a
misuse. Maybe I should just use `. = ALIGN(8)` if the kernel linker
script prefers `. = ALIGN(8)` to an output section alignment
(https://sourceware.org/binutils/docs/ld/Output-Section-Description.html#Output-Section-Description
https://lld.llvm.org/ELF/linker_script.html#output-section-alignment)

>Though, in https://github.com/ClangBuiltLinux/linux/issues/1204#issuecomment-737625134
>you're comment:
>
>>> In GNU ld, the empty .builtin_fw is removed
>
>So that's a difference in behavior between ld.bfd and ld.lld, which is
>fine, but it makes me wonder whether we should instead or additionally
>be discarding this section explicitly via linker script when
>CONFIG_FW_LOADER is not set?

Short answer: No, we should not discard .builtin_fw

   .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {
   __start_builtin_fw = .; ... }

In LLD, either a section reference (`ADDR(.builtin_fw)`) or a
non-PROVIDE symbol assignment __start_builtin_fw makes the section non-discardable.

It can be argued that discarding an output section with a symbol
assignment (GNU ld) is strange because the symbol (st_shndx) will be
defined relative to an arbitrary unrelated section. Retaining the
section can avoid some other issues.

>>         .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>                 __start_builtin_fw = .;                                 \
>>                 KEEP(*(.builtin_fw))                                    \
>> --
>> 2.29.2.576.ga3fc446d84-goog
>>
>
>
>-- 
>Thanks,
>~Nick Desaulniers

[PATCH v2] firmware_loader: Align .builtin_fw to 8

[Fangrui Song]

arm64 references the start address of .builtin_fw (__start_builtin_fw)
with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
relocations. The compiler is allowed to emit the
R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
include/linux/firmware.h is 8-byte aligned.

The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
multiple of 8, which may not be the case if .builtin_fw is empty.
Unconditionally align .builtin_fw to fix the linker error. 32-bit
architectures could use ALIGN(4) but that would add unnecessary
complexity, so just use ALIGN(8).

Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
Link: https://github.com/ClangBuiltLinux/linux/issues/1204
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Fangrui Song <maskray@google.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>

---
Change in v2:
* Use output section alignment instead of inappropriate ALIGN_FUNCTION()
---
 include/asm-generic/vmlinux.lds.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535..b97c628ad91f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -459,7 +459,7 @@
 	}								\
 									\
 	/* Built-in firmware blobs */					\
-	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
+	.builtin_fw : AT(ADDR(.builtin_fw) - LOAD_OFFSET) ALIGN(8) {	\
 		__start_builtin_fw = .;					\
 		KEEP(*(.builtin_fw))					\
 		__end_builtin_fw = .;					\
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Nick Desaulniers]

On Mon, Dec 7, 2020 at 9:46 PM Fangrui Song <maskray@google.com> wrote:
>
> arm64 references the start address of .builtin_fw (__start_builtin_fw)
> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> relocations. The compiler is allowed to emit the
> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> include/linux/firmware.h is 8-byte aligned.
>
> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> multiple of 8, which may not be the case if .builtin_fw is empty.
> Unconditionally align .builtin_fw to fix the linker error. 32-bit
> architectures could use ALIGN(4) but that would add unnecessary
> complexity, so just use ALIGN(8).
>
> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fangrui Song <maskray@google.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> ---
> Change in v2:
> * Use output section alignment instead of inappropriate ALIGN_FUNCTION()

Cool, this approach is what we already use for other global arrays;
such as __tracepoints_ptrs.  (I wonder why we don't use 4B alignment
for 32b...but 8 is a multiple of 4, so should be fine for 32b
targets).

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Build+Boot tested aarch64 and x86_64 with ld.lld and ld.bfd. Did not
test loading builtin firmware.

> ---
>  include/asm-generic/vmlinux.lds.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index b2b3d81b1535..b97c628ad91f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -459,7 +459,7 @@
>         }                                                               \
>                                                                         \
>         /* Built-in firmware blobs */                                   \
> -       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
> +       .builtin_fw : AT(ADDR(.builtin_fw) - LOAD_OFFSET) ALIGN(8) {    \
>                 __start_builtin_fw = .;                                 \
>                 KEEP(*(.builtin_fw))                                    \
>                 __end_builtin_fw = .;                                   \
> --
> 2.29.2.576.ga3fc446d84-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Doug Anderson]

Hi,

On Mon, Dec 7, 2020 at 9:49 PM Fangrui Song <maskray@google.com> wrote:
>
> arm64 references the start address of .builtin_fw (__start_builtin_fw)
> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> relocations. The compiler is allowed to emit the
> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> include/linux/firmware.h is 8-byte aligned.
>
> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> multiple of 8, which may not be the case if .builtin_fw is empty.
> Unconditionally align .builtin_fw to fix the linker error. 32-bit
> architectures could use ALIGN(4) but that would add unnecessary
> complexity, so just use ALIGN(8).
>
> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fangrui Song <maskray@google.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> ---
> Change in v2:
> * Use output section alignment instead of inappropriate ALIGN_FUNCTION()
> ---
>  include/asm-generic/vmlinux.lds.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Douglas Anderson <dianders@chromium.org>

For whatever reason this is hitting developers on Chrome OS a whole
lot suddenly.  Any chance it could be landed?  Which tree should it go
through?

-Doug

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Nick Desaulniers]

On Tue, Jan 5, 2021 at 9:45 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Dec 7, 2020 at 9:49 PM Fangrui Song <maskray@google.com> wrote:
> >
> > arm64 references the start address of .builtin_fw (__start_builtin_fw)
> > with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> > relocations. The compiler is allowed to emit the
> > R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> > include/linux/firmware.h is 8-byte aligned.
> >
> > The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> > multiple of 8, which may not be the case if .builtin_fw is empty.
> > Unconditionally align .builtin_fw to fix the linker error. 32-bit
> > architectures could use ALIGN(4) but that would add unnecessary
> > complexity, so just use ALIGN(8).
> >
> > Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Fangrui Song <maskray@google.com>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> >
> > ---
> > Change in v2:
> > * Use output section alignment instead of inappropriate ALIGN_FUNCTION()
> > ---
> >  include/asm-generic/vmlinux.lds.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Tested-by: Douglas Anderson <dianders@chromium.org>
>
> For whatever reason this is hitting developers on Chrome OS a whole
> lot suddenly.  Any chance it could be landed?  Which tree should it go
> through?

Andrew,
Would you mind picking up this patch for us, please?
https://lore.kernel.org/lkml/20201208054646.2913063-1-maskray@google.com/

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Doug Anderson]

Hi,

On Tue, Jan 5, 2021 at 11:20 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Jan 5, 2021 at 9:45 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Dec 7, 2020 at 9:49 PM Fangrui Song <maskray@google.com> wrote:
> > >
> > > arm64 references the start address of .builtin_fw (__start_builtin_fw)
> > > with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> > > relocations. The compiler is allowed to emit the
> > > R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> > > include/linux/firmware.h is 8-byte aligned.
> > >
> > > The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> > > multiple of 8, which may not be the case if .builtin_fw is empty.
> > > Unconditionally align .builtin_fw to fix the linker error. 32-bit
> > > architectures could use ALIGN(4) but that would add unnecessary
> > > complexity, so just use ALIGN(8).
> > >
> > > Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Fangrui Song <maskray@google.com>
> > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > >
> > > ---
> > > Change in v2:
> > > * Use output section alignment instead of inappropriate ALIGN_FUNCTION()
> > > ---
> > >  include/asm-generic/vmlinux.lds.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> >
> > For whatever reason this is hitting developers on Chrome OS a whole
> > lot suddenly.  Any chance it could be landed?  Which tree should it go
> > through?
>
> Andrew,
> Would you mind picking up this patch for us, please?
> https://lore.kernel.org/lkml/20201208054646.2913063-1-maskray@google.com/

I just synced today and I'm still hitting this error when building
mainline.  Perhaps Andrew is busy and someone else can pick it up?
It'd be nice to get this into v5.11

-Doug

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Doug Anderson]

Hi,

On Fri, Jan 22, 2021 at 11:04 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jan 5, 2021 at 11:20 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Tue, Jan 5, 2021 at 9:45 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Dec 7, 2020 at 9:49 PM Fangrui Song <maskray@google.com> wrote:
> > > >
> > > > arm64 references the start address of .builtin_fw (__start_builtin_fw)
> > > > with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> > > > relocations. The compiler is allowed to emit the
> > > > R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> > > > include/linux/firmware.h is 8-byte aligned.
> > > >
> > > > The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> > > > multiple of 8, which may not be the case if .builtin_fw is empty.
> > > > Unconditionally align .builtin_fw to fix the linker error. 32-bit
> > > > architectures could use ALIGN(4) but that would add unnecessary
> > > > complexity, so just use ALIGN(8).
> > > >
> > > > Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Signed-off-by: Fangrui Song <maskray@google.com>
> > > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > ---
> > > > Change in v2:
> > > > * Use output section alignment instead of inappropriate ALIGN_FUNCTION()
> > > > ---
> > > >  include/asm-generic/vmlinux.lds.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > For whatever reason this is hitting developers on Chrome OS a whole
> > > lot suddenly.  Any chance it could be landed?  Which tree should it go
> > > through?
> >
> > Andrew,
> > Would you mind picking up this patch for us, please?
> > https://lore.kernel.org/lkml/20201208054646.2913063-1-maskray@google.com/
>
> I just synced today and I'm still hitting this error when building
> mainline.  Perhaps Andrew is busy and someone else can pick it up?
> It'd be nice to get this into v5.11

I hate to be a broken record, but I synced and built mainline today
(v5.11-rc6-139-gdd86e7fa07a3) and I'm still hitting this.  It feels
like we need an alternate way to get this landed...

-Doug

Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8

[Nathan Chancellor]

On Mon, Dec 07, 2020 at 09:46:46PM -0800, Fangrui Song wrote:
> arm64 references the start address of .builtin_fw (__start_builtin_fw)
> with a pair of R_AARCH64_ADR_PREL_PG_HI21/R_AARCH64_LDST64_ABS_LO12_NC
> relocations. The compiler is allowed to emit the
> R_AARCH64_LDST64_ABS_LO12_NC relocation because struct builtin_fw in
> include/linux/firmware.h is 8-byte aligned.
> 
> The R_AARCH64_LDST64_ABS_LO12_NC relocation requires the address to be a
> multiple of 8, which may not be the case if .builtin_fw is empty.
> Unconditionally align .builtin_fw to fix the linker error. 32-bit
> architectures could use ALIGN(4) but that would add unnecessary
> complexity, so just use ALIGN(8).
> 
> Fixes: 5658c76 ("firmware: allow firmware files to be built into kernel image")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1204
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fangrui Song <maskray@google.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Nathan Chancellor <nathan@kernel.org>

Andrew, is there any way to get this picked up with the following tags
for 5.11?

Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

If not, please let me know and I can try to shuffle this along to Linus
next week.

> 
> ---
> Change in v2:
> * Use output section alignment instead of inappropriate ALIGN_FUNCTION()
> ---
>  include/asm-generic/vmlinux.lds.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index b2b3d81b1535..b97c628ad91f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -459,7 +459,7 @@
>  	}								\
>  									\
>  	/* Built-in firmware blobs */					\
> -	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> +	.builtin_fw : AT(ADDR(.builtin_fw) - LOAD_OFFSET) ALIGN(8) {	\
>  		__start_builtin_fw = .;					\
>  		KEEP(*(.builtin_fw))					\
>  		__end_builtin_fw = .;					\
> -- 
> 2.29.2.576.ga3fc446d84-goog
>