[1/5] arm64: PIE: Skip fixups if distance is zero

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(-)

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