Message ID | 20200924001715.30975-2-andre.przywara@arm.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Series | qemu-arm64: Allow booting via Trusted Firmware | expand |
On 24/09/2020 01:17, Andre Przywara wrote: > When the actual offset between link and runtime address is zero, there > is no need for patching up U-Boot early when running with > CONFIG_POSITION_INDEPENDENT. That turns out to be not fully true. Some toolchains (all Linaro cross compilers?) don't handle this well, they keep the original locations in the binary uninitialised, and rely on the reldyn fixup table to patch in the actual values. Other compilers (GCC 9.2 vanilla, Ubuntu GCC 7.5.0, Arm website 9.2) fill in the addresses both into the binary and the fixup, so this patch works. It seems to be fixed by enabling CONFIG_STATIC_RELA? I see it's disabled for CONFIG_POSITION_INDEPENDENT, what was the reason behind that? Cheers, Andre. > > Skip the whole routine when the distance is 0. > > This helps when U-Boot is loaded into ROM, or in otherwise sensitive > memory locations. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arch/arm/cpu/armv8/start.S | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S > index 002698b501c..02b952bb328 100644 > --- a/arch/arm/cpu/armv8/start.S > +++ b/arch/arm/cpu/armv8/start.S > @@ -66,7 +66,8 @@ save_boot_params_ret: > pie_fixup: > adr x0, _start /* x0 <- Runtime value of _start */ > ldr x1, _TEXT_BASE /* x1 <- Linked value of _start */ > - sub x9, x0, x1 /* x9 <- Run-vs-link offset */ > + subs x9, x0, x1 /* x9 <- Run-vs-link offset */ > + beq pie_fixup_done > adr x2, __rel_dyn_start /* x2 <- Runtime &__rel_dyn_start */ > adr x3, __rel_dyn_end /* x3 <- Runtime &__rel_dyn_end */ > pie_fix_loop: >
On Thu, 24 Sep 2020 at 16:45, André Przywara <andre.przywara@arm.com> wrote: > > On 24/09/2020 01:17, Andre Przywara wrote: > > When the actual offset between link and runtime address is zero, there > > is no need for patching up U-Boot early when running with > > CONFIG_POSITION_INDEPENDENT. > > That turns out to be not fully true. > Some toolchains (all Linaro cross compilers?) don't handle this well, > they keep the original locations in the binary uninitialised, and rely > on the reldyn fixup table to patch in the actual values. > Other compilers (GCC 9.2 vanilla, Ubuntu GCC 7.5.0, Arm website 9.2) > fill in the addresses both into the binary and the fixup, so this patch > works. > This was changed in binutils-2.27, to align with other architectures. A new command line option for ld '--no-apply-dynamic-relocs' was also added at the same time, to revert to the old behavior.
On 9/24/20 8:45 AM, André Przywara wrote: > On 24/09/2020 01:17, Andre Przywara wrote: >> When the actual offset between link and runtime address is zero, there >> is no need for patching up U-Boot early when running with >> CONFIG_POSITION_INDEPENDENT. > > That turns out to be not fully true. > Some toolchains (all Linaro cross compilers?) don't handle this well, > they keep the original locations in the binary uninitialised, and rely > on the reldyn fixup table to patch in the actual values. > Other compilers (GCC 9.2 vanilla, Ubuntu GCC 7.5.0, Arm website 9.2) > fill in the addresses both into the binary and the fixup, so this patch > works. > > It seems to be fixed by enabling CONFIG_STATIC_RELA? > I see it's disabled for CONFIG_POSITION_INDEPENDENT, what was the reason > behind that? I can't remember for sure. I tried re-enabling STATIC_RELA at both the upstream commit where POSITION_INDEPENDENT was first implemented and at top-of-tree, and ran test/py on Tegra, and it works fine. Perhaps I just figured there was no need to perform the static relocations since the code would always do it? I figure it's safe to re-enable it, i.e. decouple the two config options completely.
On 24/09/2020 21:22, Stephen Warren wrote: Hi Stephen, > On 9/24/20 8:45 AM, André Przywara wrote: >> On 24/09/2020 01:17, Andre Przywara wrote: >>> When the actual offset between link and runtime address is zero, there >>> is no need for patching up U-Boot early when running with >>> CONFIG_POSITION_INDEPENDENT. >> >> That turns out to be not fully true. >> Some toolchains (all Linaro cross compilers?) don't handle this well, >> they keep the original locations in the binary uninitialised, and rely >> on the reldyn fixup table to patch in the actual values. >> Other compilers (GCC 9.2 vanilla, Ubuntu GCC 7.5.0, Arm website 9.2) >> fill in the addresses both into the binary and the fixup, so this patch >> works. >> >> It seems to be fixed by enabling CONFIG_STATIC_RELA? >> I see it's disabled for CONFIG_POSITION_INDEPENDENT, what was the reason >> behind that? > > I can't remember for sure. I tried re-enabling STATIC_RELA at both the > upstream commit where POSITION_INDEPENDENT was first implemented and at > top-of-tree, and ran test/py on Tegra, and it works fine. Perhaps I just > figured there was no need to perform the static relocations since the > code would always do it? I figure it's safe to re-enable it, i.e. > decouple the two config options completely. Many thanks for the reply and the test! And yeah, I was hoping that it was just for "because we don't need to". I will send a patch to always use STATIC_RELA for arm64. As Ard pointed out, it should actually not be needed anymore (for newer binutils), but the Linaro toolchains miss this change, and quite some people use them. Cheers, Andre
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S index 002698b501c..02b952bb328 100644 --- a/arch/arm/cpu/armv8/start.S +++ b/arch/arm/cpu/armv8/start.S @@ -66,7 +66,8 @@ save_boot_params_ret: pie_fixup: adr x0, _start /* x0 <- Runtime value of _start */ ldr x1, _TEXT_BASE /* x1 <- Linked value of _start */ - sub x9, x0, x1 /* x9 <- Run-vs-link offset */ + subs x9, x0, x1 /* x9 <- Run-vs-link offset */ + beq pie_fixup_done adr x2, __rel_dyn_start /* x2 <- Runtime &__rel_dyn_start */ adr x3, __rel_dyn_end /* x3 <- Runtime &__rel_dyn_end */ pie_fix_loop:
When the actual offset between link and runtime address is zero, there is no need for patching up U-Boot early when running with CONFIG_POSITION_INDEPENDENT. Skip the whole routine when the distance is 0. This helps when U-Boot is loaded into ROM, or in otherwise sensitive memory locations. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm/cpu/armv8/start.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)