linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] firmware_loader: Align .builtin_fw to 8
@ 2020-12-03 17:05 Fangrui Song
  2020-12-03 18:00 ` Arnd Bergmann
  2020-12-03 19:04 ` Nick Desaulniers
  0 siblings, 2 replies; 11+ messages in thread
From: Fangrui Song @ 2020-12-03 17:05 UTC (permalink / raw)
  To: Arnd Bergmann, linux-arch
  Cc: linux-kernel, clang-built-linux, Nathan Chancellor,
	Nick Desaulniers, Fangrui Song, kernel test robot

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] firmware_loader: Align .builtin_fw to 8
  2020-12-03 17:05 [PATCH] firmware_loader: Align .builtin_fw to 8 Fangrui Song
@ 2020-12-03 18:00 ` Arnd Bergmann
  2020-12-03 19:04 ` Nick Desaulniers
  1 sibling, 0 replies; 11+ messages in thread
From: Arnd Bergmann @ 2020-12-03 18:00 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Arnd Bergmann, linux-arch, linux-kernel, clang-built-linux,
	Nathan Chancellor, Nick Desaulniers, kernel test robot

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] firmware_loader: Align .builtin_fw to 8
  2020-12-03 17:05 [PATCH] firmware_loader: Align .builtin_fw to 8 Fangrui Song
  2020-12-03 18:00 ` Arnd Bergmann
@ 2020-12-03 19:04 ` Nick Desaulniers
  2020-12-03 20:27   ` Fangrui Song
  1 sibling, 1 reply; 11+ messages in thread
From: Nick Desaulniers @ 2020-12-03 19:04 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Arnd Bergmann, linux-arch, LKML, clang-built-linux,
	Nathan Chancellor, kernel test robot, dwmw, Peter Zijlstra,
	Kees Cook

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] firmware_loader: Align .builtin_fw to 8
  2020-12-03 19:04 ` Nick Desaulniers
@ 2020-12-03 20:27   ` Fangrui Song
  2020-12-08  5:46     ` [PATCH v2] " Fangrui Song
  0 siblings, 1 reply; 11+ messages in thread
From: Fangrui Song @ 2020-12-03 20:27 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Arnd Bergmann, linux-arch, LKML, clang-built-linux,
	Nathan Chancellor, kernel test robot, dwmw, Peter Zijlstra,
	Kees Cook, Sam Ravnborg, Jakub Jelinek

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2020-12-03 20:27   ` Fangrui Song
@ 2020-12-08  5:46     ` Fangrui Song
  2020-12-08 17:59       ` Nick Desaulniers
                         ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Fangrui Song @ 2020-12-08  5:46 UTC (permalink / raw)
  To: Arnd Bergmann, linux-arch
  Cc: linux-kernel, clang-built-linux, Nathan Chancellor,
	Nick Desaulniers, Fangrui Song, kernel test robot

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2020-12-08  5:46     ` [PATCH v2] " Fangrui Song
@ 2020-12-08 17:59       ` Nick Desaulniers
  2021-01-05 17:44       ` Doug Anderson
  2021-02-05 19:08       ` Nathan Chancellor
  2 siblings, 0 replies; 11+ messages in thread
From: Nick Desaulniers @ 2020-12-08 17:59 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Arnd Bergmann, linux-arch, LKML, clang-built-linux,
	Nathan Chancellor, kernel test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2020-12-08  5:46     ` [PATCH v2] " Fangrui Song
  2020-12-08 17:59       ` Nick Desaulniers
@ 2021-01-05 17:44       ` Doug Anderson
  2021-01-05 19:20         ` Nick Desaulniers
  2021-02-05 19:08       ` Nathan Chancellor
  2 siblings, 1 reply; 11+ messages in thread
From: Doug Anderson @ 2021-01-05 17:44 UTC (permalink / raw)
  To: Fangrui Song, Arnd Bergmann
  Cc: linux-arch, LKML, clang-built-linux, Nathan Chancellor,
	Nick Desaulniers, kernel test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2021-01-05 17:44       ` Doug Anderson
@ 2021-01-05 19:20         ` Nick Desaulniers
  2021-01-22 19:04           ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Nick Desaulniers @ 2021-01-05 19:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Fangrui Song, Arnd Bergmann, linux-arch, LKML, clang-built-linux,
	Nathan Chancellor, kernel test robot, Doug Anderson

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2021-01-05 19:20         ` Nick Desaulniers
@ 2021-01-22 19:04           ` Doug Anderson
  2021-02-05 17:19             ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Doug Anderson @ 2021-01-22 19:04 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andrew Morton, Fangrui Song, Arnd Bergmann, linux-arch, LKML,
	clang-built-linux, Nathan Chancellor, kernel test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2021-01-22 19:04           ` Doug Anderson
@ 2021-02-05 17:19             ` Doug Anderson
  0 siblings, 0 replies; 11+ messages in thread
From: Doug Anderson @ 2021-02-05 17:19 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andrew Morton, Fangrui Song, Arnd Bergmann, linux-arch, LKML,
	clang-built-linux, Nathan Chancellor, kernel test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] firmware_loader: Align .builtin_fw to 8
  2020-12-08  5:46     ` [PATCH v2] " Fangrui Song
  2020-12-08 17:59       ` Nick Desaulniers
  2021-01-05 17:44       ` Doug Anderson
@ 2021-02-05 19:08       ` Nathan Chancellor
  2 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2021-02-05 19:08 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, linux-arch, linux-kernel, clang-built-linux,
	Nick Desaulniers, kernel test robot, Fangrui Song

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-02-05 19:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 17:05 [PATCH] firmware_loader: Align .builtin_fw to 8 Fangrui Song
2020-12-03 18:00 ` Arnd Bergmann
2020-12-03 19:04 ` Nick Desaulniers
2020-12-03 20:27   ` Fangrui Song
2020-12-08  5:46     ` [PATCH v2] " Fangrui Song
2020-12-08 17:59       ` Nick Desaulniers
2021-01-05 17:44       ` Doug Anderson
2021-01-05 19:20         ` Nick Desaulniers
2021-01-22 19:04           ` Doug Anderson
2021-02-05 17:19             ` Doug Anderson
2021-02-05 19:08       ` Nathan Chancellor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).