[17/30] arm-soc: tegra: make sleep asm code runtime relocatable
diff mbox

Message ID 20170814125411.22604-18-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Aug. 14, 2017, 12:53 p.m. UTC
The PIE kernel build does not allow absolute references encoded in
movw/movt instruction pairs, so use our mov_l macro instead (which
will still use such a pair unless CONFIG_RELOCATABLE is defined)

Also, avoid 32-bit absolute literals to refer to absolute symbols.
Instead, use a 16 bit reference so that PIE linker cannot get
confused whether the symbol reference is subject to relocation at
runtime.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/mach-tegra/sleep-tegra20.S | 22 ++++++++++++--------
 arch/arm/mach-tegra/sleep-tegra30.S |  6 +++---
 arch/arm/mach-tegra/sleep.S         |  4 ++--
 3 files changed, 18 insertions(+), 14 deletions(-)

Comments

Dave Martin Aug. 14, 2017, 2:42 p.m. UTC | #1
On Mon, Aug 14, 2017 at 01:53:58PM +0100, Ard Biesheuvel wrote:
> The PIE kernel build does not allow absolute references encoded in
> movw/movt instruction pairs, so use our mov_l macro instead (which
> will still use such a pair unless CONFIG_RELOCATABLE is defined)
> 
> Also, avoid 32-bit absolute literals to refer to absolute symbols.
> Instead, use a 16 bit reference so that PIE linker cannot get
> confused whether the symbol reference is subject to relocation at
> runtime.

This sounds like we're papering over something.

If the linker is "confused", that sounds like we are either abusing
it somehow, or the linker is broken.


> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/mach-tegra/sleep-tegra20.S | 22 ++++++++++++--------
>  arch/arm/mach-tegra/sleep-tegra30.S |  6 +++---
>  arch/arm/mach-tegra/sleep.S         |  4 ++--
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S
> index 5c8e638ee51a..cab95de5c8f1 100644
> --- a/arch/arm/mach-tegra/sleep-tegra20.S
> +++ b/arch/arm/mach-tegra/sleep-tegra20.S
> @@ -99,7 +99,7 @@ ENTRY(tegra20_cpu_shutdown)
>  	cmp	r0, #0
>  	reteq	lr			@ must not be called for CPU 0
>  	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT
> -	ldr	r2, =__tegra20_cpu1_resettable_status_offset
> +	ldrh	r2, 0f
>  	mov	r12, #CPU_RESETTABLE
>  	strb	r12, [r1, r2]
>  
> @@ -121,6 +121,7 @@ ENTRY(tegra20_cpu_shutdown)
>  	beq	.
>  	ret	lr
>  ENDPROC(tegra20_cpu_shutdown)
> +0:	.short	__tegra20_cpu1_resettable_status_offset
>  #endif
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -181,6 +182,9 @@ ENTRY(tegra_pen_unlock)
>  	ret     lr
>  ENDPROC(tegra_pen_unlock)
>  
> +.L__tegra20_cpu1_resettable_status_offset:
> +	.short	__tegra20_cpu1_resettable_status_offset
> +
>  /*
>   * tegra20_cpu_clear_resettable(void)
>   *
> @@ -189,7 +193,7 @@ ENDPROC(tegra_pen_unlock)
>   */
>  ENTRY(tegra20_cpu_clear_resettable)
>  	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT

Can we simply port mov32 to mov_l?  Or do we hit a problem with
multiplatform kernels where mov_l may involve a literal pool entry
(and does it matter)?


[...]

Cheers
---Dave
Ard Biesheuvel Aug. 14, 2017, 2:49 p.m. UTC | #2
On 14 August 2017 at 15:42, Dave Martin <Dave.Martin@arm.com> wrote:
> On Mon, Aug 14, 2017 at 01:53:58PM +0100, Ard Biesheuvel wrote:
>> The PIE kernel build does not allow absolute references encoded in
>> movw/movt instruction pairs, so use our mov_l macro instead (which
>> will still use such a pair unless CONFIG_RELOCATABLE is defined)
>>
>> Also, avoid 32-bit absolute literals to refer to absolute symbols.
>> Instead, use a 16 bit reference so that PIE linker cannot get
>> confused whether the symbol reference is subject to relocation at
>> runtime.
>
> This sounds like we're papering over something.
>
> If the linker is "confused", that sounds like we are either abusing
> it somehow, or the linker is broken.
>

There is some ambiguity in how SHN_ABS symbols are treated in shared
libraries and PIE executables.

https://sourceware.org/ml/binutils/2012-05/msg00019.html

I haven't confirmed whether it actually causes problems in this
particular case, but it is safer (and not entirely inappropriate) to
use a 16-bit field for a quantity that can easily fit one.


>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/mach-tegra/sleep-tegra20.S | 22 ++++++++++++--------
>>  arch/arm/mach-tegra/sleep-tegra30.S |  6 +++---
>>  arch/arm/mach-tegra/sleep.S         |  4 ++--
>>  3 files changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S
>> index 5c8e638ee51a..cab95de5c8f1 100644
>> --- a/arch/arm/mach-tegra/sleep-tegra20.S
>> +++ b/arch/arm/mach-tegra/sleep-tegra20.S
>> @@ -99,7 +99,7 @@ ENTRY(tegra20_cpu_shutdown)
>>       cmp     r0, #0
>>       reteq   lr                      @ must not be called for CPU 0
>>       mov32   r1, TEGRA_IRAM_RESET_BASE_VIRT
>> -     ldr     r2, =__tegra20_cpu1_resettable_status_offset
>> +     ldrh    r2, 0f
>>       mov     r12, #CPU_RESETTABLE
>>       strb    r12, [r1, r2]
>>
>> @@ -121,6 +121,7 @@ ENTRY(tegra20_cpu_shutdown)
>>       beq     .
>>       ret     lr
>>  ENDPROC(tegra20_cpu_shutdown)
>> +0:   .short  __tegra20_cpu1_resettable_status_offset
>>  #endif
>>
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -181,6 +182,9 @@ ENTRY(tegra_pen_unlock)
>>       ret     lr
>>  ENDPROC(tegra_pen_unlock)
>>
>> +.L__tegra20_cpu1_resettable_status_offset:
>> +     .short  __tegra20_cpu1_resettable_status_offset
>> +
>>  /*
>>   * tegra20_cpu_clear_resettable(void)
>>   *
>> @@ -189,7 +193,7 @@ ENDPROC(tegra_pen_unlock)
>>   */
>>  ENTRY(tegra20_cpu_clear_resettable)
>>       mov32   r1, TEGRA_IRAM_RESET_BASE_VIRT
>
> Can we simply port mov32 to mov_l?  Or do we hit a problem with
> multiplatform kernels where mov_l may involve a literal pool entry
> (and does it matter)?
>

The only place where it matters is in code that lives in idmap.text,
since the relative reference will point to the ID mapped alias of a
section that is not covered by the ID ID map. I think we should be
able to use mov_l everywhere else, and it should do the right thing
for ordinary and PIE builds
Dave Martin Aug. 14, 2017, 3:29 p.m. UTC | #3
On Mon, Aug 14, 2017 at 03:49:15PM +0100, Ard Biesheuvel wrote:
> On 14 August 2017 at 15:42, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Mon, Aug 14, 2017 at 01:53:58PM +0100, Ard Biesheuvel wrote:
> >> The PIE kernel build does not allow absolute references encoded in
> >> movw/movt instruction pairs, so use our mov_l macro instead (which
> >> will still use such a pair unless CONFIG_RELOCATABLE is defined)
> >>
> >> Also, avoid 32-bit absolute literals to refer to absolute symbols.
> >> Instead, use a 16 bit reference so that PIE linker cannot get
> >> confused whether the symbol reference is subject to relocation at
> >> runtime.
> >
> > This sounds like we're papering over something.
> >
> > If the linker is "confused", that sounds like we are either abusing
> > it somehow, or the linker is broken.
> >
> 
> There is some ambiguity in how SHN_ABS symbols are treated in shared
> libraries and PIE executables.
> 
> https://sourceware.org/ml/binutils/2012-05/msg00019.html
> 
> I haven't confirmed whether it actually causes problems in this
> particular case, but it is safer (and not entirely inappropriate) to
> use a 16-bit field for a quantity that can easily fit one.

OK, so, fundamental linker design flaw that cannot be fixed for
historical reasons.  Gotcha.

This "fix" feels like a bit of a hack, since it relies on the absence of
a certain relocation type that could be added later (though it seems
highly unlikely).


Cleaner options would be to expose both symbols that are subtracted,
rather than the result, or to do the subtraction at build time and
generate a header that affected files include (though that's more
invasive on the Makefile side).

May be overkill though.

> 
> >
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/mach-tegra/sleep-tegra20.S | 22 ++++++++++++--------
> >>  arch/arm/mach-tegra/sleep-tegra30.S |  6 +++---
> >>  arch/arm/mach-tegra/sleep.S         |  4 ++--
> >>  3 files changed, 18 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S
> >> index 5c8e638ee51a..cab95de5c8f1 100644
> >> --- a/arch/arm/mach-tegra/sleep-tegra20.S
> >> +++ b/arch/arm/mach-tegra/sleep-tegra20.S
> >> @@ -99,7 +99,7 @@ ENTRY(tegra20_cpu_shutdown)
> >>       cmp     r0, #0
> >>       reteq   lr                      @ must not be called for CPU 0
> >>       mov32   r1, TEGRA_IRAM_RESET_BASE_VIRT
> >> -     ldr     r2, =__tegra20_cpu1_resettable_status_offset
> >> +     ldrh    r2, 0f
> >>       mov     r12, #CPU_RESETTABLE
> >>       strb    r12, [r1, r2]
> >>
> >> @@ -121,6 +121,7 @@ ENTRY(tegra20_cpu_shutdown)
> >>       beq     .
> >>       ret     lr
> >>  ENDPROC(tegra20_cpu_shutdown)
> >> +0:   .short  __tegra20_cpu1_resettable_status_offset
> >>  #endif
> >>
> >>  #ifdef CONFIG_PM_SLEEP
> >> @@ -181,6 +182,9 @@ ENTRY(tegra_pen_unlock)
> >>       ret     lr
> >>  ENDPROC(tegra_pen_unlock)
> >>
> >> +.L__tegra20_cpu1_resettable_status_offset:
> >> +     .short  __tegra20_cpu1_resettable_status_offset
> >> +
> >>  /*
> >>   * tegra20_cpu_clear_resettable(void)
> >>   *
> >> @@ -189,7 +193,7 @@ ENDPROC(tegra_pen_unlock)
> >>   */
> >>  ENTRY(tegra20_cpu_clear_resettable)
> >>       mov32   r1, TEGRA_IRAM_RESET_BASE_VIRT
> >
> > Can we simply port mov32 to mov_l?  Or do we hit a problem with
> > multiplatform kernels where mov_l may involve a literal pool entry
> > (and does it matter)?
> >
> 
> The only place where it matters is in code that lives in idmap.text,
> since the relative reference will point to the ID mapped alias of a
> section that is not covered by the ID ID map. I think we should be
> able to use mov_l everywhere else, and it should do the right thing
> for ordinary and PIE builds

Fair enough.

Cheers
---Dave

Patch
diff mbox

diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S
index 5c8e638ee51a..cab95de5c8f1 100644
--- a/arch/arm/mach-tegra/sleep-tegra20.S
+++ b/arch/arm/mach-tegra/sleep-tegra20.S
@@ -99,7 +99,7 @@  ENTRY(tegra20_cpu_shutdown)
 	cmp	r0, #0
 	reteq	lr			@ must not be called for CPU 0
 	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r2, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r2, 0f
 	mov	r12, #CPU_RESETTABLE
 	strb	r12, [r1, r2]
 
@@ -121,6 +121,7 @@  ENTRY(tegra20_cpu_shutdown)
 	beq	.
 	ret	lr
 ENDPROC(tegra20_cpu_shutdown)
+0:	.short	__tegra20_cpu1_resettable_status_offset
 #endif
 
 #ifdef CONFIG_PM_SLEEP
@@ -181,6 +182,9 @@  ENTRY(tegra_pen_unlock)
 	ret     lr
 ENDPROC(tegra_pen_unlock)
 
+.L__tegra20_cpu1_resettable_status_offset:
+	.short	__tegra20_cpu1_resettable_status_offset
+
 /*
  * tegra20_cpu_clear_resettable(void)
  *
@@ -189,7 +193,7 @@  ENDPROC(tegra_pen_unlock)
  */
 ENTRY(tegra20_cpu_clear_resettable)
 	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r2, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r2, .L__tegra20_cpu1_resettable_status_offset
 	mov	r12, #CPU_NOT_RESETTABLE
 	strb	r12, [r1, r2]
 	ret	lr
@@ -203,7 +207,7 @@  ENDPROC(tegra20_cpu_clear_resettable)
  */
 ENTRY(tegra20_cpu_set_resettable_soon)
 	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r2, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r2, .L__tegra20_cpu1_resettable_status_offset
 	mov	r12, #CPU_RESETTABLE_SOON
 	strb	r12, [r1, r2]
 	ret	lr
@@ -217,7 +221,7 @@  ENDPROC(tegra20_cpu_set_resettable_soon)
  */
 ENTRY(tegra20_cpu_is_resettable_soon)
 	mov32	r1, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r2, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r2, .L__tegra20_cpu1_resettable_status_offset
 	ldrb	r12, [r1, r2]
 	cmp	r12, #CPU_RESETTABLE_SOON
 	moveq	r0, #1
@@ -238,11 +242,11 @@  ENTRY(tegra20_sleep_core_finish)
 	bl	tegra_disable_clean_inv_dcache
 	mov     r0, r4
 
-	mov32	r3, tegra_shut_off_mmu
+	mov_l	r3, tegra_shut_off_mmu
 	add	r3, r3, r0
 
-	mov32	r0, tegra20_tear_down_core
-	mov32	r1, tegra20_iram_start
+	mov_l	r0, tegra20_tear_down_core
+	mov_l	r1, tegra20_iram_start
 	sub	r0, r0, r1
 	mov32	r1, TEGRA_IRAM_LPx_RESUME_AREA
 	add	r0, r0, r1
@@ -265,7 +269,7 @@  ENTRY(tegra20_sleep_cpu_secondary_finish)
 	bl	tegra_disable_clean_inv_dcache
 
 	mov32	r0, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r4, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r4, .L__tegra20_cpu1_resettable_status_offset
 	mov	r3, #CPU_RESETTABLE
 	strb	r3, [r0, r4]
 
@@ -284,7 +288,7 @@  ENTRY(tegra20_sleep_cpu_secondary_finish)
 	bl	tegra_pen_lock
 
 	mov32	r0, TEGRA_IRAM_RESET_BASE_VIRT
-	ldr	r4, =__tegra20_cpu1_resettable_status_offset
+	ldrh	r4, .L__tegra20_cpu1_resettable_status_offset
 	mov	r3, #CPU_NOT_RESETTABLE
 	strb	r3, [r0, r4]
 
diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index dd4a67dabd91..478b2ca3ef6e 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -261,11 +261,11 @@  ENTRY(tegra30_sleep_core_finish)
 	mov32	r6, TEGRA_FLOW_CTRL_BASE
 	mov32	r7, TEGRA_TMRUS_BASE
 
-	mov32	r3, tegra_shut_off_mmu
+	mov_l	r3, tegra_shut_off_mmu
 	add	r3, r3, r0
 
-	mov32	r0, tegra30_tear_down_core
-	mov32	r1, tegra30_iram_start
+	mov_l	r0, tegra30_tear_down_core
+	mov_l	r1, tegra30_iram_start
 	sub	r0, r0, r1
 	mov32	r1, TEGRA_IRAM_LPx_RESUME_AREA
 	add	r0, r0, r1
diff --git a/arch/arm/mach-tegra/sleep.S b/arch/arm/mach-tegra/sleep.S
index 5e3496753df1..785df3edc767 100644
--- a/arch/arm/mach-tegra/sleep.S
+++ b/arch/arm/mach-tegra/sleep.S
@@ -101,11 +101,11 @@  ENTRY(tegra_sleep_cpu_finish)
 	bl	tegra_disable_clean_inv_dcache
 
 	mov	r0, r4
-	mov32	r6, tegra_tear_down_cpu
+	mov_l	r6, tegra_tear_down_cpu
 	ldr	r1, [r6]
 	add	r1, r1, r0
 
-	mov32	r3, tegra_shut_off_mmu
+	mov_l	r3, tegra_shut_off_mmu
 	add	r3, r3, r0
 	mov	r0, r1