omap assembly fixes for CONFIG_DEBUG_RODATA
Subsystems (3)
Tony Lindgren
maintainercover omap assembly fixes for CONFIG_DEBUG_RODATA
Hi all,
Here are read-only data fixes for omaps discovered few weeks ago with "[PATCH v2] ARM: mm: flip priority of CONFIG_DEBUG_RODATA" that caused issues during deeper idle states on omaps [0].
Please review and test if possible. I'd like to merge these fixes during v4.5-rc cycle to remove these blockers for making it possible to have CONFIG_DEBUG_RODATA enabled by default.
Regards,
Tony
[0] https://lkml.org/lkml/2015/12/2/908
Tony Lindgren (5):
ARM: OMAP2+: Fix wait_dll_lock_timed for rodata
ARM: OMAP2+: Fix l2dis_3630 for rodata
ARM: OMAP2+: Fix save_secure_ram_context for rodata
ARM: OMAP2+: Fix l2_inv_api_params for rodata
ARM: OMAP2+: Fix ppa_zero_params and ppa_por_params for rodata
arch/arm/mach-omap2/sleep34xx.S | 60 ++++++++++++++++++++--------------------- arch/arm/mach-omap2/sleep44xx.S | 25 +++++++++++------ 2 files changed, 47 insertions(+), 38 deletions(-)
Signature · 1 line
2.7.0.rc3
Tony Lindgren
maintainerPATCH 1/5 ARM: OMAP2+: Fix wait_dll_lock_timed for rodata
We don't want to be writing to .text so it can be set rodata. Fix error "Unable to handle kernel paging request at virtual address c012396c" in wait_dll_lock_timed if CONFIG_DEBUG_RODATA is selected.
As these counters are for debugging only and unused, we can just remove them.
--- arch/arm/mach-omap2/sleep34xx.S | 22 ---------------------- 1 file changed, 22 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +0 -22
@@ -289,12 +289,6 @@ wait_sdrc_ready: bic r5, r5, #0x40 str r5, [r4] -/* - * PC-relative stores lead to undefined behaviour in Thumb-2: use a r7 as a - * base instead. - * Be careful not to clobber r7 when maintaing this code. - */ - is_dll_in_lock_mode: /* Is dll in lock mode? */ ldr r4, sdrc_dlla_ctrl
@@ -302,11 +296,7 @@ is_dll_in_lock_mode: tst r5, #0x4 bne exit_nonoff_modes @ Return if locked /* wait till dll locks */ - adr r7, kick_counter wait_dll_lock_timed: - ldr r4, wait_dll_lock_counter - add r4, r4, #1 - str r4, [r7, #wait_dll_lock_counter - kick_counter] ldr r4, sdrc_dlla_status /* Wait 20uS for lock */ mov r6, #8
@@ -330,9 +320,6 @@ kick_dll: orr r6, r6, #(1<<3) @ enable dll str r6, [r4] dsb - ldr r4, kick_counter - add r4, r4, #1 - str r4, [r7] @ kick_counter b wait_dll_lock_timed exit_nonoff_modes:
@@ -360,15 +347,6 @@ sdrc_dlla_status: .word SDRC_DLLA_STATUS_V sdrc_dlla_ctrl: .word SDRC_DLLA_CTRL_V - /* - * When exporting to userspace while the counters are in SRAM, - * these 2 words need to be@the end to facilitate retrival! - */ -kick_counter: - .word 0 -wait_dll_lock_counter: - .word 0 - ENTRY(omap3_do_wfi_sz) .word . - omap3_do_wfi
Signature · 1 line
2.7.0.rc3
Tony Lindgren
maintainerPATCH 2/5 ARM: OMAP2+: Fix l2dis_3630 for rodata
In reply to Tony LindgrenWe don't want to write to .text section. Let's move l2dis_3630 to .data and access it via a pointer.
--- arch/arm/mach-omap2/sleep34xx.S | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +13 -3
@@ -86,7 +86,9 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) stmfd sp!, {lr} @ save registers on stack /* Setup so that we will disable and enable l2 */ mov r1, #0x1 - adrl r2, l2dis_3630 @ may be too distant for plain adr + adrl r3, l2dis_3630_offset @ may be too distant for plain adr + ldr r2, [r3] + add r2, r2, r3 str r1, [r2] ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore)
@@ -415,7 +417,9 @@ ENTRY(omap3_restore) cmp r2, #0x0 @ Check if target power state was OFF or RET bne logic_l1_restore - ldr r0, l2dis_3630 + adr r1, l2dis_3630_offset + ldr r0, [r1] + add r0, r0, r1 cmp r0, #0x1 @ should we disable L2 on 3630? bne skipl2dis mrc p15, 0, r0, c1, c0, 1
@@ -484,7 +488,9 @@ l2_inv_gp: mov r12, #0x2 smc #0 @ Call SMI monitor (smieq) logic_l1_restore: - ldr r1, l2dis_3630 + adr r0, l2dis_3630_offset + ldr r1, [r0] + add r1, r1, r0 cmp r1, #0x1 @ Test if L2 re-enable needed on 3630 bne skipl2reen mrc p15, 0, r1, c1, c0, 1
@@ -513,6 +519,10 @@ control_stat: .word CONTROL_STAT control_mem_rta: .word CONTROL_MEM_RTA_CTRL +l2dis_3630_offset: + .long l2dis_3630 - . + + .data l2dis_3630: .word 0
Signature · 1 line
2.7.0.rc3
PATCH 2/5 ARM: OMAP2+: Fix l2dis_3630 for rodata
Quoted 20 lines · Tony Lindgren
We don't want to write to .text section. Let's move
l2dis_3630to .data and access it via a pointer.Cc Kees Cook <[email protected]>Cc Laura Abbott <[email protected]>Cc Nicolas Pitre <[email protected]>Cc Nishanth Menon <[email protected]>Cc Richard Woodruff <[email protected]>Cc Russell King <[email protected]>Cc Tero Kristo <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- arch/arm/mach-omap2/sleep34xx.S | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)arch/arm/mach-omap2/sleep34xx.S +0
-0
arch/arm/mach-omap2/sleep34xx.S ENTRY(enable_omap3630_toggle_l2_on_restore) · line 92
Quoted 8 lines
arch/arm/mach-omap2/sleep34xx.S +3
-1@@ -86,7 +86,9 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) stmfd sp!, {lr} @ save registers on stack /* Setup so that we will disable and enable l2 */ mov r1, #0x1 - adrl r2, l2dis_3630 @ may be too distant for plain adr + adrl r3, l2dis_3630_offset @ may be too distant for plain adr + ldr r2, [r3] + add r2, r2, r3 str r1, [r2]
You could combine the add with the following str:
str r1, [r2, r3]
arch/arm/mach-omap2/sleep34xx.S ENTRY(omap3_restore) · line 423
Quoted 11 lines
arch/arm/mach-omap2/sleep34xx.S +3
-1ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore)@@ -415,7 +417,9 @@ ENTRY(omap3_restore) cmp r2, #0x0 @ Check if target power state was OFF or RET bne logic_l1_restore - ldr r0, l2dis_3630 + adr r1, l2dis_3630_offset + ldr r0, [r1] + add r0, r0, r1 cmp r0, #0x1 @ should we disable L2 on 3630?
This looks wrong. You're testing the first bit of the address for l2dis_3630 rather than its content.
arch/arm/mach-omap2/sleep34xx.S l2_inv_gp: · line 494
Quoted 11 lines
arch/arm/mach-omap2/sleep34xx.S +3
-1bne skipl2dis mrc p15, 0, r0, c1, c0, 1@@ -484,7 +488,9 @@ l2_inv_gp: mov r12, #0x2 smc #0 @ Call SMI monitor (smieq) logic_l1_restore: - ldr r1, l2dis_3630 + adr r0, l2dis_3630_offset + ldr r1, [r0] + add r1, r1, r0 cmp r1, #0x1 @ Test if L2 re-enable needed on 3630
Ditto here.
Quoted 17 lines
arch/arm/mach-omap2/sleep34xx.S +4
-0bne skipl2reen mrc p15, 0, r1, c1, c0, 1@@ -513,6 +519,10 @@ control_stat: .word CONTROL_STAT control_mem_rta: .word CONTROL_MEM_RTA_CTRL +l2dis_3630_offset: + .long l2dis_3630 - . + + .data l2dis_3630: .word 0Signature · 1 line
2.7.0.rc3
Tony Lindgren
maintainerPATCH 2/5 ARM: OMAP2+: Fix l2dis_3630 for rodata
Quoted 14 lines · Nicolas Pitre
arch/arm/mach-omap2/sleep34xx.S ENTRY(enable_omap3630_toggle_l2_on_restore) · line 92
Quoted 11 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +3
-1@@ -86,7 +86,9 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) stmfd sp!, {lr} @ save registers on stack /* Setup so that we will disable and enable l2 */ mov r1, #0x1 - adrl r2, l2dis_3630 @ may be too distant for plain adr + adrl r3, l2dis_3630_offset @ may be too distant for plain adr + ldr r2, [r3] + add r2, r2, r3 str r1, [r2]You could combine the add with the following str:
OK.
Quoted 8 lines
str r1, [r2, r3]Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S +0
-0ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore)@@ -415,7 +417,9 @@ ENTRY(omap3_restore) cmp r2, #0x0 @ Check if target power state was OFF or RET bne logic_l1_restore
Quoted 8 lines
arch/arm/mach-omap2/sleep34xx.S ENTRY(omap3_restore) · line 423
Quoted 5 lines
arch/arm/mach-omap2/sleep34xx.S +3
-1- ldr r0, l2dis_3630 + adr r1, l2dis_3630_offset + ldr r0, [r1] + add r0, r0, r1 cmp r0, #0x1 @ should we disable L2 on 3630?This looks wrong. You're testing the first bit of the address for
l2dis_3630rather than its content.
Ugh yup a ldr missing missing there. Will check the whole series and repost. Thanks for checking!
Regards,
Tony
Tony Lindgren
maintainerPATCH 3/5 ARM: OMAP2+: Fix save_secure_ram_context for rodata
In reply to Tony LindgrenWe don't want to write to .text and we can move save_secure_ram_context into .data as it all gets copied into SRAM anyways.
--- arch/arm/mach-omap2/sleep34xx.S | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +8 -2
@@ -93,8 +93,12 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore) - .text -/* Function to call rom code to save secure ram context */ +/* + * Function to call rom code to save secure ram context, This gets + * relocated to SRAM, so it can be all in .data section. Otherwise + * we need to initialize api_params separately. + */ + .data .align 3 ENTRY(save_secure_ram_context) stmfd sp!, {r4 - r11, lr} @ save registers on stack
@@ -128,6 +132,8 @@ ENDPROC(save_secure_ram_context) ENTRY(save_secure_ram_context_sz) .word . - save_secure_ram_context + .text + /* * ====================== * == Idle entry point ==
Signature · 1 line
2.7.0.rc3
PATCH 3/5 ARM: OMAP2+: Fix save_secure_ram_context for rodata
Hello.
Quoted 20 lines · Tony Lindgren
We don't want to write to .text and we can move
save_secure_ram_contextinto .data as it all gets copied into SRAM anyways.Cc Kees Cook <[email protected]>Cc Laura Abbott <[email protected]>Cc Nicolas Pitre <[email protected]>Cc Nishanth Menon <[email protected]>Cc Richard Woodruff <[email protected]>Cc Russell King <[email protected]>Cc Tero Kristo <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- arch/arm/mach-omap2/sleep34xx.S | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)arch/arm/mach-omap2/sleep34xx.S +0
-0
arch/arm/mach-omap2/sleep34xx.S ENTRY(enable_omap3630_toggle_l2_on_restore) · line 97
Quoted 7 lines
arch/arm/mach-omap2/sleep34xx.S +2
-2@@ -93,8 +93,12 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore) - .text -/* Function to call rom code to save secure ram context */ +/* + * Function to call rom code to save secure ram context, This gets
s/,/./?
arch/arm/mach-omap2/sleep34xx.S ENTRY(enable_omap3630_toggle_l2_on_restore) · line 104
Quoted 7 lines
arch/arm/mach-omap2/sleep34xx.S +4
-0+ * relocated to SRAM, so it can be all in .data section. Otherwise + * we need to initialize api_params separately. + */ + .data .align 3 ENTRY(save_secure_ram_context) stmfd sp!, {r4 - r11, lr} @ save registers on stack
[...]
MBR, Sergei
Tony Lindgren
maintainerPATCH 3/5 ARM: OMAP2+: Fix save_secure_ram_context for rodata
Quoted 6 lines · Sergei Shtylyov
Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S +0
-0@@ -93,8 +93,12 @@ ENTRY(enable_omap3630_toggle_l2_on_restore) ldmfd sp!, {pc} @ restore regs and return ENDPROC(enable_omap3630_toggle_l2_on_restore)
Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S ENTRY(enable_omap3630_toggle_l2_on_restore) · line 97
Quoted 4 lines
arch/arm/mach-omap2/sleep34xx.S +2
-2- .text -/* Function to call rom code to save secure ram context */ +/* + * Function to call rom code to save secure ram context, This getss/,/./?
Will fix thanks.
Tony
Tony Lindgren
maintainerPATCH 4/5 ARM: OMAP2+: Fix l2_inv_api_params for rodata
In reply to Tony LindgrenWe don't want to write to .text, so let's move l2_inv_api_params to .data and access it via a pointer.
--- arch/arm/mach-omap2/sleep34xx.S | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +9 -3
@@ -437,12 +437,14 @@ skipl2dis: and r1, #0x700 cmp r1, #0x300 beq l2_inv_gp + adr r0, l2_inv_api_params_offset @ r3 points to dummy parameters + ldr r3, [r0] + add r3, r3, r0 mov r0, #40 @ set service ID for PPA mov r12, r0 @ copy secure Service ID in r12 mov r1, #0 @ set task id for ROM code in r1 mov r2, #4 @ set some flags in r2, r6 mov r6, #0xff - adr r3, l2_inv_api_params @ r3 points to dummy parameters dsb @ data write barrier dmb @ data memory barrier smc #1 @ call SMI monitor (smi #1)
@@ -476,8 +478,8 @@ skipl2dis: b logic_l1_restore .align -l2_inv_api_params: - .word 0x1, 0x00 +l2_inv_api_params_offset: + .long l2_inv_api_params - . l2_inv_gp: /* Execute smi to invalidate L2 cache */ mov r12, #0x1 @ set up to invalidate L2
@@ -532,6 +534,10 @@ l2dis_3630_offset: l2dis_3630: .word 0 + .data +l2_inv_api_params: + .word 0x1, 0x00 + /* * Internal functions */
Signature · 1 line
2.7.0.rc3
Tony Lindgren
maintainerPATCH 5/5 ARM: OMAP2+: Fix ppa_zero_params and ppa_por_params for rodata
In reply to Tony LindgrenWe don't want to write to .text so let's move ppa_zero_params and ppa_por_params to .data and access them via pointers.
Note that I have not been able to test as we I don't have a HS omap4 to test with. The code has been changed in similar way as for omap3 though.
--- arch/arm/mach-omap2/sleep44xx.S | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
arch/arm/mach-omap2/sleep44xx.S +17 -8
@@ -29,12 +29,6 @@ dsb .endm -ppa_zero_params: - .word 0x0 - -ppa_por_params: - .word 1, 0 - #ifdef CONFIG_ARCH_OMAP4 /*
@@ -266,7 +260,9 @@ ENTRY(omap4_cpu_resume) beq skip_ns_smp_enable ppa_actrl_retry: mov r0, #OMAP4_PPA_CPU_ACTRL_SMP_INDEX - adr r3, ppa_zero_params @ Pointer to parameters + adr r1, ppa_zero_params_offset @ Pointer to parameters + ldr r3, [r1] + add r3, r3, r1 mov r1, #0x0 @ Process ID mov r2, #0x4 @ Flag mov r6, #0xff
@@ -303,7 +299,9 @@ skip_ns_smp_enable: ldr r0, =OMAP4_PPA_L2_POR_INDEX ldr r1, =OMAP44XX_SAR_RAM_BASE ldr r4, [r1, #L2X0_PREFETCH_CTRL_OFFSET] - adr r3, ppa_por_params + adr r1, ppa_por_params_offset + ldr r3, [r1] + add r3, r3, r1 str r4, [r3, #0x04] mov r1, #0x0 @ Process ID mov r2, #0x4 @ Flag
@@ -328,6 +326,8 @@ skip_l2en: #endif b cpu_resume @ Jump to generic resume +ppa_por_params_offset: + .long ppa_por_params - . ENDPROC(omap4_cpu_resume) #endif /* CONFIG_ARCH_OMAP4 */
@@ -380,4 +380,13 @@ ENTRY(omap_do_wfi) nop ldmfd sp!, {pc} +ppa_zero_params_offset: + .long ppa_zero_params - . ENDPROC(omap_do_wfi) + + .data +ppa_zero_params: + .word 0 + +ppa_por_params: + .word 1, 0
Signature · 1 line
2.7.0.rc3
cover omap assembly fixes for CONFIG_DEBUG_RODATA
In reply to Tony LindgrenQuoted 9 lines · Tony Lindgren
Hi all,
Here are read-only data fixes for omaps discovered few weeks ago with "[PATCH v2] ARM: mm: flip priority of
CONFIG_DEBUG_RODATA" that caused issues during deeper idle states on omaps [0].Please review and test if possible. I'd like to merge these fixes during v4.5-rc cycle to remove these blockers for making it possible to have
CONFIG_DEBUG_RODATAenabled by default.
Yay! :) Thanks for working on this.
-Kees
Quoted 22 lines
Regards,
Tony
[0] https://lkml.org/lkml/2015/12/2/908
Tony Lindgren (5):
ARM: OMAP2+: Fixwait_dll_lock_timedfor rodata
ARM: OMAP2+: Fixl2dis_3630for rodata
ARM: OMAP2+: Fixsave_secure_ram_contextfor rodata
ARM: OMAP2+: Fixl2_inv_api_paramsfor rodata
ARM: OMAP2+: Fixppa_zero_paramsandppa_por_paramsfor rodataarch/arm/mach-omap2/sleep34xx.S | 60 ++++++++++++++++++++--------------------- arch/arm/mach-omap2/sleep44xx.S | 25 +++++++++++------ 2 files changed, 47 insertions(+), 38 deletions(-)--
2.7.0.rc3
Signature · 2 lines
Kees Cook Chrome OS & Brillo Security