[PATCH v3 0/4] arm64: Large PIE fixes

[PATCH v3 0/4] arm64: Large PIE fixes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This fixes some build issues with large (> 1MB) PIE U-Boot setups.
We also document the 4K aligned load address requirement and
add an early run-time check for it.

As requested by reviewers, I've also added a runtime check for
non-PIE builds to trap the startup sequence early if the start
address doesn't match between run-time and link-time.

Cheers,
Edgar

ChangeLog:
v2 -> v3:
* Add non-PIE start address (run vs link-time) check
* Move 4K aligment Kconfig help description
* Fix load of __bss_start
* Use x0 rather than x1 in PIE load-address check
* Add missing tabs
* Add load-address check for non-PIE
* U-boot -> U-Boot in a few places
* Tweak commit messages

v1 -> v2:
* Fix adr of _start in arch/arm/lib/crt0_64.S
* Add early check and bail out into a WFI loop when not 4K aligned
* Document the 4K alignement requirement in Kconfig

Edgar E. Iglesias (4):
  arm64: Mention 4K aligned load addresses in the PIE Kconfig help
  arm64: Trap PIE builds early if load address is not 4K aligned
  arm64: Add support for larger PIE U-Boot
  arm64: Trap non-PIE builds early if starting from wrong address

 arch/arm/Kconfig           |  4 ++--
 arch/arm/cpu/armv8/start.S | 36 ++++++++++++++++++++++++++++++++++--
 arch/arm/lib/crt0_64.S     |  8 +++++++-
 3 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] arm64: Mention 4K aligned load addresses in the PIE Kconfig help

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Mention the requirement of 4K aligned load addresses in the
help section for the POSITION_INDEPENDENT option.

Suggested-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 arch/arm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 80702c23d3..67286e8b5d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -16,8 +16,8 @@ config POSITION_INDEPENDENT
 	help
 	  U-Boot expects to be linked to a specific hard-coded address, and to
 	  be loaded to and run from that address. This option lifts that
-	  restriction, thus allowing the code to be loaded to and executed
-	  from almost any address. This logic relies on the relocation
+	  restriction, thus allowing the code to be loaded to and executed from
+	  almost any 4K aligned address. This logic relies on the relocation
 	  information that is embedded in the binary to support U-Boot
 	  relocating itself to the top-of-RAM later during execution.
 
-- 
2.25.1

[PATCH v3 2/4] arm64: Trap PIE builds early if load address is not 4K aligned

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

PIE requires a 4K aligned load address. If this is not met, trap
the startup sequence in a WFI loop rather than running into obscure
failures.

Tested-by: Michal Simek <michal.simek@xilinx.com>
Suggested-by: Andr? Przywara <andre.przywara@arm.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 arch/arm/cpu/armv8/start.S | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index 002698b501..85baebc5f7 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -59,6 +59,23 @@ reset:
 save_boot_params_ret:
 
 #if CONFIG_POSITION_INDEPENDENT
+	/* Verify that we're 4K aligned.  */
+	adr	x0, _start
+	ands	x0, x0, #0xfff
+	b.eq	1f
+0:
+	/*
+	 * FATAL, can't continue.
+	 * U-Boot needs to be loaded at a 4K aligned address.
+	 *
+	 * We use ADRP and ADD to load some symbol addresses during startup.
+	 * The ADD uses an absolute (non pc-relative) lo12 relocation
+	 * thus requiring 4K alignment.
+	 */
+	wfi
+	b	0b
+1:
+
 	/*
 	 * Fix .rela.dyn relocations. This allows U-Boot to be loaded to and
 	 * executed at a different address than it was linked at.
-- 
2.25.1

[PATCH v3 3/4] arm64: Add support for larger PIE U-Boot

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Linking a U-Boot larger than 1MB fails with PIE enabled:
u-boot/arch/arm/cpu/armv8/start.S:71:(.text+0x3c): relocation
truncated to fit: R_AARCH64_ADR_PREL_LO21 against symbol `__rel_dyn_end'
defined in .bss_start section in u-boot.

This extends the supported range by using adrp & add to load symbols
early while starting up.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 arch/arm/cpu/armv8/start.S | 6 ++++--
 arch/arm/lib/crt0_64.S     | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index 85baebc5f7..e5c2856cf5 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -84,8 +84,10 @@ 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 */
-	adr	x2, __rel_dyn_start	/* x2 <- Runtime &__rel_dyn_start */
-	adr	x3, __rel_dyn_end	/* x3 <- Runtime &__rel_dyn_end */
+	adrp    x2, __rel_dyn_start     /* x2 <- Runtime &__rel_dyn_start */
+	add     x2, x2, #:lo12:__rel_dyn_start
+	adrp    x3, __rel_dyn_end       /* x3 <- Runtime &__rel_dyn_end */
+	add     x3, x3, #:lo12:__rel_dyn_end
 pie_fix_loop:
 	ldp	x0, x1, [x2], #16	/* (x0, x1) <- (Link location, fixup) */
 	ldr	x4, [x2], #8		/* x4 <- addend */
diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S
index 04afa518ac..9d2319c0e8 100644
--- a/arch/arm/lib/crt0_64.S
+++ b/arch/arm/lib/crt0_64.S
@@ -73,7 +73,12 @@ ENTRY(_main)
 #elif defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
 	ldr	x0, =(CONFIG_SPL_STACK)
 #elif defined(CONFIG_INIT_SP_RELATIVE)
+#if CONFIG_POSITION_INDEPENDENT
+	adrp	x0, __bss_start     /* x0 <- Runtime &__bss_start */
+	add	x0, x0, #:lo12:__bss_start
+#else
 	adr	x0, __bss_start
+#endif
 	add	x0, x0, #CONFIG_SYS_INIT_SP_BSS_OFFSET
 #else
 	ldr	x0, =(CONFIG_SYS_INIT_SP_ADDR)
@@ -102,7 +107,8 @@ ENTRY(_main)
 	adr	lr, relocation_return
 #if CONFIG_POSITION_INDEPENDENT
 	/* Add in link-vs-runtime offset */
-	adr	x0, _start		/* x0 <- Runtime value of _start */
+	adrp	x0, _start		/* x0 <- Runtime value of _start */
+	add	x0, x0, #:lo12:_start
 	ldr	x9, _TEXT_BASE		/* x9 <- Linked value of _start */
 	sub	x9, x9, x0		/* x9 <- Run-vs-link offset */
 	add	lr, lr, x9
-- 
2.25.1

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Trap non-PIE builds early if the start address doesn't match
between run-time and link-time. This will trap the startup
sequence rather than letting it run into obscure errors.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 arch/arm/cpu/armv8/start.S | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index e5c2856cf5..39e1b842c4 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -101,6 +101,19 @@ pie_skip_reloc:
 	cmp	x2, x3
 	b.lo	pie_fix_loop
 pie_fixup_done:
+#else
+	adr	x0, _start
+	ldr	x1, _TEXT_BASE
+	cmp	x0, x1
+	beq	1f
+0:
+	/*
+	 * FATAL, can't continue.
+	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
+	 */
+	wfi
+	b	0b
+1:
 #endif
 
 #ifdef CONFIG_SYS_RESET_SCTRL
-- 
2.25.1

[PATCH v3 0/4] arm64: Large PIE fixes

[Stephen Warren]

On 9/9/20 11:07 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> This fixes some build issues with large (> 1MB) PIE U-Boot setups.
> We also document the 4K aligned load address requirement and
> add an early run-time check for it.
> 
> As requested by reviewers, I've also added a runtime check for
> non-PIE builds to trap the startup sequence early if the start
> address doesn't match between run-time and link-time.

The series,

Reviewed-by: Stephen Warren <swarren@nvidia.com>

[PATCH v3 3/4] arm64: Add support for larger PIE U-Boot

[Michal Simek]

On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Linking a U-Boot larger than 1MB fails with PIE enabled:
> u-boot/arch/arm/cpu/armv8/start.S:71:(.text+0x3c): relocation
> truncated to fit: R_AARCH64_ADR_PREL_LO21 against symbol `__rel_dyn_end'
> defined in .bss_start section in u-boot.
> 
> This extends the supported range by using adrp & add to load symbols
> early while starting up.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  arch/arm/cpu/armv8/start.S | 6 ++++--
>  arch/arm/lib/crt0_64.S     | 8 +++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index 85baebc5f7..e5c2856cf5 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -84,8 +84,10 @@ 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 */
> -	adr	x2, __rel_dyn_start	/* x2 <- Runtime &__rel_dyn_start */
> -	adr	x3, __rel_dyn_end	/* x3 <- Runtime &__rel_dyn_end */
> +	adrp    x2, __rel_dyn_start     /* x2 <- Runtime &__rel_dyn_start */
> +	add     x2, x2, #:lo12:__rel_dyn_start
> +	adrp    x3, __rel_dyn_end       /* x3 <- Runtime &__rel_dyn_end */
> +	add     x3, x3, #:lo12:__rel_dyn_end
>  pie_fix_loop:
>  	ldp	x0, x1, [x2], #16	/* (x0, x1) <- (Link location, fixup) */
>  	ldr	x4, [x2], #8		/* x4 <- addend */
> diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S
> index 04afa518ac..9d2319c0e8 100644
> --- a/arch/arm/lib/crt0_64.S
> +++ b/arch/arm/lib/crt0_64.S
> @@ -73,7 +73,12 @@ ENTRY(_main)
>  #elif defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
>  	ldr	x0, =(CONFIG_SPL_STACK)
>  #elif defined(CONFIG_INIT_SP_RELATIVE)
> +#if CONFIG_POSITION_INDEPENDENT
> +	adrp	x0, __bss_start     /* x0 <- Runtime &__bss_start */
> +	add	x0, x0, #:lo12:__bss_start
> +#else
>  	adr	x0, __bss_start
> +#endif
>  	add	x0, x0, #CONFIG_SYS_INIT_SP_BSS_OFFSET
>  #else
>  	ldr	x0, =(CONFIG_SYS_INIT_SP_ADDR)
> @@ -102,7 +107,8 @@ ENTRY(_main)
>  	adr	lr, relocation_return
>  #if CONFIG_POSITION_INDEPENDENT
>  	/* Add in link-vs-runtime offset */
> -	adr	x0, _start		/* x0 <- Runtime value of _start */
> +	adrp	x0, _start		/* x0 <- Runtime value of _start */
> +	add	x0, x0, #:lo12:_start
>  	ldr	x9, _TEXT_BASE		/* x9 <- Linked value of _start */
>  	sub	x9, x9, x0		/* x9 <- Run-vs-link offset */
>  	add	lr, lr, x9
> 

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Michal Simek]

On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Trap non-PIE builds early if the start address doesn't match
> between run-time and link-time. This will trap the startup
> sequence rather than letting it run into obscure errors.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  arch/arm/cpu/armv8/start.S | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index e5c2856cf5..39e1b842c4 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -101,6 +101,19 @@ pie_skip_reloc:
>  	cmp	x2, x3
>  	b.lo	pie_fix_loop
>  pie_fixup_done:
> +#else
> +	adr	x0, _start
> +	ldr	x1, _TEXT_BASE
> +	cmp	x0, x1
> +	beq	1f
> +0:
> +	/*
> +	 * FATAL, can't continue.
> +	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
> +	 */
> +	wfi
> +	b	0b
> +1:
>  #endif
>  
>  #ifdef CONFIG_SYS_RESET_SCTRL
> 

NACK for this.

1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for
U-Boot proper
2. It likely also breaks TPL flow for the same reason

3. And last thing is that this code is used only for U-Boot proper.
.globl	_TEXT_BASE
_TEXT_BASE:
	.quad	CONFIG_SYS_TEXT_BASE

The fixes are below. Point 3 should be likely be in separate patch
because it is unrelated.

Thanks,
Michal


diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index b37dbabf4d42..cdc609e873fc 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -33,10 +33,11 @@ _start:

        .align 3

+#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
 .globl _TEXT_BASE
 _TEXT_BASE:
        .quad   CONFIG_SYS_TEXT_BASE
-
+#endif
 /*
  * These are defined in the linker script.
  */
@@ -102,6 +103,7 @@ pie_skip_reloc:
        b.lo    pie_fix_loop
 pie_fixup_done:
 #else
+#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
        adr     x0, _start
        ldr     x1, _TEXT_BASE
        cmp     x0, x1
@@ -115,6 +117,7 @@ pie_fixup_done:
        b       0b
 1:
 #endif
+#endif

 #ifdef CONFIG_SYS_RESET_SCTRL
        bl reset_sctrl

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[André Przywara]

On 10/09/2020 13:38, Michal Simek wrote:
> 
> 
> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Trap non-PIE builds early if the start address doesn't match
>> between run-time and link-time. This will trap the startup
>> sequence rather than letting it run into obscure errors.
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  arch/arm/cpu/armv8/start.S | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
>> index e5c2856cf5..39e1b842c4 100644
>> --- a/arch/arm/cpu/armv8/start.S
>> +++ b/arch/arm/cpu/armv8/start.S
>> @@ -101,6 +101,19 @@ pie_skip_reloc:
>>  	cmp	x2, x3
>>  	b.lo	pie_fix_loop
>>  pie_fixup_done:
>> +#else
>> +	adr	x0, _start
>> +	ldr	x1, _TEXT_BASE
>> +	cmp	x0, x1
>> +	beq	1f
>> +0:
>> +	/*
>> +	 * FATAL, can't continue.
>> +	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
>> +	 */
>> +	wfi
>> +	b	0b
>> +1:
>>  #endif
>>  
>>  #ifdef CONFIG_SYS_RESET_SCTRL
>>
> 
> NACK for this.
> 
> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for
> U-Boot proper
> 2. It likely also breaks TPL flow for the same reason
> 
> 3. And last thing is that this code is used only for U-Boot proper.
> .globl	_TEXT_BASE
> _TEXT_BASE:
> 	.quad	CONFIG_SYS_TEXT_BASE
> 
> The fixes are below. Point 3 should be likely be in separate patch
> because it is unrelated.

So if this patch causes issues, can't we just drop it? I mean right now
you will probably just crash anyway if you load it at the wrong address,
but maybe late enough that you get more hints or even some output.

Now this patch makes sure that you don't get anything, so I don't see
how this is really improving the situation. It seems like a case of
"don't fix things that ain't broken".

Cheers,
Andre


> 
> Thanks,
> Michal
> 
> 
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index b37dbabf4d42..cdc609e873fc 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -33,10 +33,11 @@ _start:
> 
>         .align 3
> 
> +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
>  .globl _TEXT_BASE
>  _TEXT_BASE:
>         .quad   CONFIG_SYS_TEXT_BASE
> -
> +#endif
>  /*
>   * These are defined in the linker script.
>   */
> @@ -102,6 +103,7 @@ pie_skip_reloc:
>         b.lo    pie_fix_loop
>  pie_fixup_done:
>  #else
> +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
>         adr     x0, _start
>         ldr     x1, _TEXT_BASE
>         cmp     x0, x1
> @@ -115,6 +117,7 @@ pie_fixup_done:
>         b       0b
>  1:
>  #endif
> +#endif
> 
>  #ifdef CONFIG_SYS_RESET_SCTRL
>         bl reset_sctrl
> 

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Michal Simek]

On 10. 09. 20 15:06, Andr? Przywara wrote:
> On 10/09/2020 13:38, Michal Simek wrote:
>>
>>
>> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Trap non-PIE builds early if the start address doesn't match
>>> between run-time and link-time. This will trap the startup
>>> sequence rather than letting it run into obscure errors.
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>>  arch/arm/cpu/armv8/start.S | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
>>> index e5c2856cf5..39e1b842c4 100644
>>> --- a/arch/arm/cpu/armv8/start.S
>>> +++ b/arch/arm/cpu/armv8/start.S
>>> @@ -101,6 +101,19 @@ pie_skip_reloc:
>>>  	cmp	x2, x3
>>>  	b.lo	pie_fix_loop
>>>  pie_fixup_done:
>>> +#else
>>> +	adr	x0, _start
>>> +	ldr	x1, _TEXT_BASE
>>> +	cmp	x0, x1
>>> +	beq	1f
>>> +0:
>>> +	/*
>>> +	 * FATAL, can't continue.
>>> +	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
>>> +	 */
>>> +	wfi
>>> +	b	0b
>>> +1:
>>>  #endif
>>>  
>>>  #ifdef CONFIG_SYS_RESET_SCTRL
>>>
>>
>> NACK for this.
>>
>> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for
>> U-Boot proper
>> 2. It likely also breaks TPL flow for the same reason
>>
>> 3. And last thing is that this code is used only for U-Boot proper.
>> .globl	_TEXT_BASE
>> _TEXT_BASE:
>> 	.quad	CONFIG_SYS_TEXT_BASE
>>
>> The fixes are below. Point 3 should be likely be in separate patch
>> because it is unrelated.
> 
> So if this patch causes issues, can't we just drop it? I mean right now
> you will probably just crash anyway if you load it at the wrong address,
> but maybe late enough that you get more hints or even some output.
> 
> Now this patch makes sure that you don't get anything, so I don't see
> how this is really improving the situation. It seems like a case of
> "don't fix things that ain't broken".

I am fine with dropping it.
Tom: What do you think?

Thanks,
Michal

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Tom Rini]

On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote:
> 
> 
> On 10. 09. 20 15:06, Andr? Przywara wrote:
> > On 10/09/2020 13:38, Michal Simek wrote:
> >>
> >>
> >> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
> >>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>
> >>> Trap non-PIE builds early if the start address doesn't match
> >>> between run-time and link-time. This will trap the startup
> >>> sequence rather than letting it run into obscure errors.
> >>>
> >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >>> ---
> >>>  arch/arm/cpu/armv8/start.S | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> >>> index e5c2856cf5..39e1b842c4 100644
> >>> --- a/arch/arm/cpu/armv8/start.S
> >>> +++ b/arch/arm/cpu/armv8/start.S
> >>> @@ -101,6 +101,19 @@ pie_skip_reloc:
> >>>  	cmp	x2, x3
> >>>  	b.lo	pie_fix_loop
> >>>  pie_fixup_done:
> >>> +#else
> >>> +	adr	x0, _start
> >>> +	ldr	x1, _TEXT_BASE
> >>> +	cmp	x0, x1
> >>> +	beq	1f
> >>> +0:
> >>> +	/*
> >>> +	 * FATAL, can't continue.
> >>> +	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
> >>> +	 */
> >>> +	wfi
> >>> +	b	0b
> >>> +1:
> >>>  #endif
> >>>  
> >>>  #ifdef CONFIG_SYS_RESET_SCTRL
> >>>
> >>
> >> NACK for this.
> >>
> >> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for
> >> U-Boot proper
> >> 2. It likely also breaks TPL flow for the same reason
> >>
> >> 3. And last thing is that this code is used only for U-Boot proper.
> >> .globl	_TEXT_BASE
> >> _TEXT_BASE:
> >> 	.quad	CONFIG_SYS_TEXT_BASE
> >>
> >> The fixes are below. Point 3 should be likely be in separate patch
> >> because it is unrelated.
> > 
> > So if this patch causes issues, can't we just drop it? I mean right now
> > you will probably just crash anyway if you load it at the wrong address,
> > but maybe late enough that you get more hints or even some output.
> > 
> > Now this patch makes sure that you don't get anything, so I don't see
> > how this is really improving the situation. It seems like a case of
> > "don't fix things that ain't broken".
> 
> I am fine with dropping it.
> Tom: What do you think?

OK, yes, we can set this aside for now at least.  I assume this is all
for v2021.01 anyhow?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200910/f310e4ad/attachment.sig>

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Michal Simek]

On 10. 09. 20 15:50, Tom Rini wrote:
> On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote:
>>
>>
>> On 10. 09. 20 15:06, Andr? Przywara wrote:
>>> On 10/09/2020 13:38, Michal Simek wrote:
>>>>
>>>>
>>>> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>>
>>>>> Trap non-PIE builds early if the start address doesn't
>>>>> match between run-time and link-time. This will trap the
>>>>> startup sequence rather than letting it run into obscure
>>>>> errors.
>>>>>
>>>>> Signed-off-by: Edgar E. Iglesias
>>>>> <edgar.iglesias@xilinx.com> --- arch/arm/cpu/armv8/start.S
>>>>> | 13 +++++++++++++ 1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv8/start.S
>>>>> b/arch/arm/cpu/armv8/start.S index e5c2856cf5..39e1b842c4
>>>>> 100644 --- a/arch/arm/cpu/armv8/start.S +++
>>>>> b/arch/arm/cpu/armv8/start.S @@ -101,6 +101,19 @@
>>>>> pie_skip_reloc: cmp	x2, x3 b.lo	pie_fix_loop
>>>>> pie_fixup_done: +#else +	adr	x0, _start +	ldr	x1,
>>>>> _TEXT_BASE +	cmp	x0, x1 +	beq	1f +0: +	/* +	 * FATAL, can't
>>>>> continue. +	 * U-Boot needs to start executing at
>>>>> CONFIG_SYS_TEXT_BASE. +	 */ +	wfi +	b	0b +1: #endif
>>>>>
>>>>> #ifdef CONFIG_SYS_RESET_SCTRL
>>>>>
>>>>
>>>> NACK for this.
>>>>
>>>> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text
>>>> base for U-Boot proper 2. It likely also breaks TPL flow for
>>>> the same reason
>>>>
>>>> 3. And last thing is that this code is used only for U-Boot
>>>> proper. .globl	_TEXT_BASE _TEXT_BASE: .quad
>>>> CONFIG_SYS_TEXT_BASE
>>>>
>>>> The fixes are below. Point 3 should be likely be in separate
>>>> patch because it is unrelated.
>>>
>>> So if this patch causes issues, can't we just drop it? I mean
>>> right now you will probably just crash anyway if you load it at
>>> the wrong address, but maybe late enough that you get more
>>> hints or even some output.
>>>
>>> Now this patch makes sure that you don't get anything, so I
>>> don't see how this is really improving the situation. It seems
>>> like a case of "don't fix things that ain't broken".
>>
>> I am fine with dropping it. Tom: What do you think?
>
> OK, yes, we can set this aside for now at least.  I assume this is
> all for v2021.01 anyhow?
>

I would target it for 2021.01.

Thanks,
Michal

[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[Edgar E. Iglesias]

On Thu, Sep 10, 2020 at 05:02:56PM +0200, Michal Simek wrote:
> 
> 
> On 10. 09. 20 15:50, Tom Rini wrote:
> > On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote:
> >>
> >>
> >> On 10. 09. 20 15:06, Andr? Przywara wrote:
> >>> On 10/09/2020 13:38, Michal Simek wrote:
> >>>>
> >>>>
> >>>> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
> >>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>>>
> >>>>> Trap non-PIE builds early if the start address doesn't
> >>>>> match between run-time and link-time. This will trap the
> >>>>> startup sequence rather than letting it run into obscure
> >>>>> errors.
> >>>>>
> >>>>> Signed-off-by: Edgar E. Iglesias
> >>>>> <edgar.iglesias@xilinx.com> --- arch/arm/cpu/armv8/start.S
> >>>>> | 13 +++++++++++++ 1 file changed, 13 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/cpu/armv8/start.S
> >>>>> b/arch/arm/cpu/armv8/start.S index e5c2856cf5..39e1b842c4
> >>>>> 100644 --- a/arch/arm/cpu/armv8/start.S +++
> >>>>> b/arch/arm/cpu/armv8/start.S @@ -101,6 +101,19 @@
> >>>>> pie_skip_reloc: cmp	x2, x3 b.lo	pie_fix_loop
> >>>>> pie_fixup_done: +#else +	adr	x0, _start +	ldr	x1,
> >>>>> _TEXT_BASE +	cmp	x0, x1 +	beq	1f +0: +	/* +	 * FATAL, can't
> >>>>> continue. +	 * U-Boot needs to start executing at
> >>>>> CONFIG_SYS_TEXT_BASE. +	 */ +	wfi +	b	0b +1: #endif
> >>>>>
> >>>>> #ifdef CONFIG_SYS_RESET_SCTRL
> >>>>>
> >>>>
> >>>> NACK for this.
> >>>>
> >>>> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text
> >>>> base for U-Boot proper 2. It likely also breaks TPL flow for
> >>>> the same reason
> >>>>
> >>>> 3. And last thing is that this code is used only for U-Boot
> >>>> proper. .globl	_TEXT_BASE _TEXT_BASE: .quad
> >>>> CONFIG_SYS_TEXT_BASE
> >>>>
> >>>> The fixes are below. Point 3 should be likely be in separate
> >>>> patch because it is unrelated.
> >>>
> >>> So if this patch causes issues, can't we just drop it? I mean
> >>> right now you will probably just crash anyway if you load it at
> >>> the wrong address, but maybe late enough that you get more
> >>> hints or even some output.
> >>>
> >>> Now this patch makes sure that you don't get anything, so I
> >>> don't see how this is really improving the situation. It seems
> >>> like a case of "don't fix things that ain't broken".
> >>
> >> I am fine with dropping it. Tom: What do you think?
> >
> > OK, yes, we can set this aside for now at least.  I assume this is
> > all for v2021.01 anyhow?
> >
> 
> I would target it for 2021.01.
>

Dropping #4 and queueing the rest for 2021.01 sounds good to me too.
We can revisit a possible check for non-PIE later.

Cheers,
Edgar

[PATCH v3 0/4] arm64: Large PIE fixes

[Michal Simek]

Hi,

On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> This fixes some build issues with large (> 1MB) PIE U-Boot setups.
> We also document the 4K aligned load address requirement and
> add an early run-time check for it.
> 
> As requested by reviewers, I've also added a runtime check for
> non-PIE builds to trap the startup sequence early if the start
> address doesn't match between run-time and link-time.
> 
> Cheers,
> Edgar
> 
> ChangeLog:
> v2 -> v3:
> * Add non-PIE start address (run vs link-time) check
> * Move 4K aligment Kconfig help description
> * Fix load of __bss_start
> * Use x0 rather than x1 in PIE load-address check
> * Add missing tabs
> * Add load-address check for non-PIE
> * U-boot -> U-Boot in a few places
> * Tweak commit messages
> 
> v1 -> v2:
> * Fix adr of _start in arch/arm/lib/crt0_64.S
> * Add early check and bail out into a WFI loop when not 4K aligned
> * Document the 4K alignement requirement in Kconfig
> 
> Edgar E. Iglesias (4):
>   arm64: Mention 4K aligned load addresses in the PIE Kconfig help
>   arm64: Trap PIE builds early if load address is not 4K aligned
>   arm64: Add support for larger PIE U-Boot
>   arm64: Trap non-PIE builds early if starting from wrong address
> 
>  arch/arm/Kconfig           |  4 ++--
>  arch/arm/cpu/armv8/start.S | 36 ++++++++++++++++++++++++++++++++++--
>  arch/arm/lib/crt0_64.S     |  8 +++++++-
>  3 files changed, 43 insertions(+), 5 deletions(-)
> 

Because these changes are made for supporting Xilinx SOCs 1-3 applied to
my tree. Patch 4 is dropped as was agreed.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs