Skip to contentSkip to conversation

omap assembly fixes for CONFIG_DEBUG_RODATA

PATCH Superseded Tony Lindgren 4 participants 11 messages Jan 19, 2016

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Tony Lindgren

author maintainer
#0 lore raw

cover 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
author maintainer
#1 lore raw

PATCH 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.

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]>
---
 arch/arm/mach-omap2/sleep34xx.S | 22 ----------------------
 1 file changed, 22 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +0 -22
index eafd120..787cfda 100644
@@ -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
author maintainer
#2 lore raw

PATCH 2/5 ARM: OMAP2+: Fix l2dis_3630 for rodata

In reply to Tony Lindgren

We don't want to write to .text section. Let's move l2dis_3630 to .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]>
---
 arch/arm/mach-omap2/sleep34xx.S | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +13 -3
index 787cfda..f7c7bf8 100644
@@ -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
#3 lore raw

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_3630 to .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]>
---
 arch/arm/mach-omap2/sleep34xx.S | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +0 -0
index 787cfda..f7c7bf8 100644

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 -1
 	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?

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 -1
 	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

Ditto here.

Quoted 17 lines
arch/arm/mach-omap2/sleep34xx.S +4 -0
 	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
author maintainer
#4 lore raw

PATCH 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 -0
 	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
 
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_3630 rather than its content.

Ugh yup a ldr missing missing there. Will check the whole series and repost. Thanks for checking!

Regards,

Tony

author maintainer
#5 lore raw

PATCH 3/5 ARM: OMAP2+: Fix save_secure_ram_context for rodata

In reply to Tony Lindgren

We 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.

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]>
---
 arch/arm/mach-omap2/sleep34xx.S | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +8 -2
index f7c7bf8..042e4fb 100644
@@ -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
#6 lore raw

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_context into .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]>
---
  arch/arm/mach-omap2/sleep34xx.S | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +0 -0
index f7c7bf8..042e4fb 100644

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

author maintainer
#7 lore raw

PATCH 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 gets
   s/,/./?

Will fix thanks.

Tony

author maintainer
#8 lore raw

PATCH 4/5 ARM: OMAP2+: Fix l2_inv_api_params for rodata

In reply to Tony Lindgren

We don't want to write to .text, so let's move l2_inv_api_params to .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]>
---
 arch/arm/mach-omap2/sleep34xx.S | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +9 -3
index 042e4fb..7b12c6b 100644
@@ -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
author maintainer
#9 lore raw

PATCH 5/5 ARM: OMAP2+: Fix ppa_zero_params and ppa_por_params for rodata

In reply to Tony Lindgren

We 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.

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]>
---
 arch/arm/mach-omap2/sleep44xx.S | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)
arch/arm/mach-omap2/sleep44xx.S +17 -8
index 9b09d85..74ab3d1 100644
@@ -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
#10 lore raw

cover omap assembly fixes for CONFIG_DEBUG_RODATA

In reply to Tony Lindgren
Quoted 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_RODATA enabled 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+: 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(-)

--
2.7.0.rc3

Signature · 2 lines
Kees Cook
Chrome OS & Brillo Security