Re: [PATCH 8/9] [HACK] ARM: imx: work around v7_cpu_resume link error

[Nicolas Pitre <nicolas.pitre@linaro.org>]

On Fri, 15 Feb 2013, Russell King - ARM Linux wrote:

> On Thu, Feb 14, 2013 at 11:47:50PM +0100, Arnd Bergmann wrote:
> > Patch c08e20d24 "arm: Add v7_invalidate_l1 to cache-v7.S"
> > moves the v7_invalidate_l1 symbol out of imx/headsmp.S,
> > which seems to cause a link error because it is now
> > too far away from v7_cpu_resume when building an
> > allyesconfig kernel.
> > 
> > If we move the v7_cpu_resume function from the .data
> > section to .text, that creates another link error
> > for the reference to phys_l2x0_saved_regs, but we
> > can move all of the above to .text.
> > 
> > I believe that this is not a correct bug fix but just
> > a bad workaround, so I'm open to ideas from people
> > who understand the bigger picture.
> 
> Unfortunately, this ends up with writable data in the .text section, which
> is supposed to be read-only.  We should try to preserve that status, even
> though we don't enforce it.
> 
> I guess the problem is that we end up with the .data segment soo far away
> from the .text segment that these branches no longer work (and remember
> that this code executes with the MMU off.)
> 
> The only solution I can think is that we need to do quite a bit of magic
> here to jump to an absolute address, but taking account of the V:P offset.
> That's not going to be particularly nice, and it's going to then also have
> to jump back the other way - to the cpu_resume code which is also in the
> .data section.

Something like this should work:

diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
index 7e49deb128..9de26f3edb 100644
--- a/arch/arm/mach-imx/headsmp.S
+++ b/arch/arm/mach-imx/headsmp.S
@@ -99,8 +99,11 @@ phys_l2x0_saved_regs:
 #endif
 
 ENTRY(v7_cpu_resume)
-	bl	v7_invalidate_l1
+	ldr	ip, 2f
+	mov	lr, pc
+1:	add	pc, pc, ip
 	pl310_resume
 	b	cpu_resume
+2:	.word v7_invalidate_l1 - 1b - 8
 ENDPROC(v7_cpu_resume)
 #endif


However it is probably best to move all the code to the .text section 
where it belongs and fixup the data access instead.  I must plead guilty 
for introducing this idea of putting code in the .data section with the 
SA1100resume code over 10 years ago that everyone copied since.

So here's how I think it should look instead, and how I should have done 
the SA1100 code back then:

diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
index 7e49deb128..38a544a037 100644
--- a/arch/arm/mach-imx/headsmp.S
+++ b/arch/arm/mach-imx/headsmp.S
@@ -73,16 +73,16 @@ ENDPROC(v7_secondary_startup)
 
 #ifdef CONFIG_PM
 /*
- * The following code is located into the .data section.  This is to
- * allow phys_l2x0_saved_regs to be accessed with a relative load
- * as we are running on physical address here.
+ * The following code must assume it is running from physical address
+ * where absolute virtual addresses to the data section have to be
+ * turned into relative ones.
  */
-	.data
-	.align
 
 #ifdef CONFIG_CACHE_L2X0
 	.macro	pl310_resume
-	ldr	r2, phys_l2x0_saved_regs
+	adr	r0, phys_l2x0_saved_ptr_offset
+	ldr	r2, [r0]
+	add	r2, r2, r0
 	ldr	r0, [r2, #L2X0_R_PHY_BASE]	@ get physical base of l2x0
 	ldr	r1, [r2, #L2X0_R_AUX_CTRL]	@ get aux_ctrl value
 	str	r1, [r0, #L2X0_AUX_CTRL]	@ restore aux_ctrl
@@ -90,9 +90,13 @@ ENDPROC(v7_secondary_startup)
 	str	r1, [r0, #L2X0_CTRL]		@ re-enable L2
 	.endm
 
+	.bss
 	.globl	phys_l2x0_saved_regs
 phys_l2x0_saved_regs:
-        .long   0
+        .space   4
+	.previous
+phys_l2x0_saved_ptr_offset:
+	.word	phys_l2x0_saved_regs - .
 #else
 	.macro	pl310_resume
 	.endm

And yet we could dispense with phys_l2x0_saved_regs 
altogether and use l2x0_saved_regs directly here instead.

That doesn't solve the issue of the kernel becoming too large for branch 
displacement fixup though.


Nicolas