On 2020-12-03, Nick Desaulniers wrote:
On Thu, Dec 3, 2020 at 9:05 AM Fangrui Song <maskray@xxxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Fangrui Song <maskray@xxxxxxxxxx>
---
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
