* [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).