| Message ID | 20170814125411.22604-18-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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(-)