diff mbox

[v3] ARM: xip: Use correct symbol for end of ROM marker

Message ID alpine.LFD.2.20.1511181156250.22569@knanqh.ubzr (mailing list archive)
State Changes Requested
Delegated to: Simon Horman
Headers show

Commit Message

Nicolas Pitre Nov. 18, 2015, 5:01 p.m. UTC 
On Wed, 18 Nov 2015, Nicolas Pitre wrote:

> On Wed, 18 Nov 2015, Magnus Damm wrote:
> 
> > Thanks for your take on this. I did a couple of local implementations
> > before submitting, and one of the issues I ran into was the need to
> > get rid of PAGE_OFFSET due to the code running without MMU enabled. I
> > suppose that is taken care of the "__v7_setup_stack - ." calculation
> > above?
> 
> Yes.  That provides the offset from __v7_setup_stack_ptr to reach 
> __v7_setup_stack. And __v7_setup_stack_ptr is obtained with adr which is 
> relative to the current pc. So this works whether or not the MMU is 
> enabled.

Here's the patch with proper changelog, etc.  I don't have XIP capable 
hardware to test it with though.

------ >8
From: Nicolas Pitre <nicolas.pitre@linaro.org>
Subject: [PATCH] ARM: proc-v7*.S: don't locate temporary stack space in .text section

Both proc-v7.S and proc-v7m.S are using a small temporary stack to
preserve register content within their respective setup code. This
stack is located in the .text section which is normally meant to be
read-only.  This is even more true in the context of an XIP kernel
where .text is accessed from ROM directly.

Move that temporary stack to the .bss section and get its address in
a position independent way.

While at it, one comments was updated to reflect reality, and the list
of saved registers in the proc-v7.S case is updated to match the comment
next to it for coherency.

Signed-off-by: Nicolas Pitre <nico@linaro.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Chris Brandt Nov. 18, 2015, 7:12 p.m. UTC | #1 
Hi Nicolas,

> Here's the patch with proper changelog, etc.  I don't have XIP
> capable hardware to test it with though.


I'm testing it now...but it's crashing.

I fired up GDB, so here the reason:


__v7_ca17mp_setup:
	mov	r10, #0
1:	adr	r0, __v7_setup_stack_ptr
r0=0x18213df4

	ldr	r12, [r0]
r12=0x10174cc

	add	r12, r12, r0			@ the local stack
r12=0x1922b2c0

	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
	bl      v7_invalidate_l1


0x1922b2c0 is NOT RAM....it's nothing.


As point of reference, here's the memory map of my XIP system:
  Physical ROM address: 0x18000000 (I have my XIP kernel starting at 0x18200000)
  Physical RAM address: 0x20000000
  Virtual ROM  address: 0xBF0000000
  Virtual RAM  address: 0xC00000000


Basically, you made the same mistake that Magnus first did: You can't rely on the current PC address to obtain an address in physical RAM because the ROM virt-to-phys relationship is different than the RAM virt-to-phys relationship.

No matter how you look at it, this early in boot, the only 'thing' that really knows where physical RAM is located is PLAT_PHYS_OFFSET.

That's why both Magnus's an my patches (that I both tested) contain PLAT_PHYS_OFFSET.


The only other 'slight' piece of info would be the DTB pointer is still sitting in R2 from boot, and at least for my system, that is in RAM. However, the DTB could be in ROM if the user wanted it to be, and I also think that this code might run twice in a SMP system...so even R2 would not be reliable. So...forget I mentioned it...


I don't see any way around not having a "#ifdef XIP_KERNEL" statement in order to expose PLAT_PHYS_OFFSET so you can properly do your translation math.


Chris


ps, I will add that I appreciate your efforts with this.

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 18, 2015, 8:23 p.m. UTC | #2 
On Wed, 18 Nov 2015, Chris Brandt wrote:

> Hi Nicolas,
> 
> > Here's the patch with proper changelog, etc.  I don't have XIP
> > capable hardware to test it with though.
> 
> 
> I'm testing it now...but it's crashing.
> 
> I fired up GDB, so here the reason:
> 
> 
> __v7_ca17mp_setup:
> 	mov	r10, #0
> 1:	adr	r0, __v7_setup_stack_ptr
> r0=0x18213df4
> 
> 	ldr	r12, [r0]
> r12=0x10174cc
> 
> 	add	r12, r12, r0			@ the local stack
> r12=0x1922b2c0
> 
> 	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
> 	bl      v7_invalidate_l1
> 
> 
> 0x1922b2c0 is NOT RAM....it's nothing.
> 
> 
> As point of reference, here's the memory map of my XIP system:
>   Physical ROM address: 0x18000000 (I have my XIP kernel starting at 0x18200000)
>   Physical RAM address: 0x20000000
>   Virtual ROM  address: 0xBF0000000
>   Virtual RAM  address: 0xC00000000
> 
> 
> Basically, you made the same mistake that Magnus first did: You can't 
> rely on the current PC address to obtain an address in physical RAM 
> because the ROM virt-to-phys relationship is different than the RAM 
> virt-to-phys relationship.

Crap... you're right of course.  I suspect a couple other places might 
have problems as they use similar constructs. See kernel/sleep.S for 
example.

Probably the  best way to fix it would be something like:

in asm/memory.h or similar:

#ifdef CONFIG_XIP_KERNEL
#define PHYS_OFFSET_FIXUP \
	( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \
	  PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR )
#else
#define PHYS_OFFSET_FIXUP 0
#endif

And then, after my patch is applied, changing:

__v7_setup_stack_ptr:
        .word   __v7_setup_stack - .

into:

__v7_setup_stack_ptr:
        .word   __v7_setup_stack - . + PHYS_OFFSET_FIXUP

should do the trick. This way it'll work for all those places where the 
code is getting at the data area when the MMU is off with no XIP 
conditionals in the code.

I think my patch should be applied as is (minus the mention of XIP) to 
remove the write access to the .text area for the general case which is 
a worthy goal in itself.  We did a bunch of similar cleanups a while 
ago.

Then another patch could bring all those places XIP compatible with the 
simple addition of that PHYS_OFFSET_FIXUP constant.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 18, 2015, 8:51 p.m. UTC | #3 
> Probably the best way to fix it would be something like:
>
> in asm/memory.h or similar:
>
> #ifdef CONFIG_XIP_KERNEL
> #define PHYS_OFFSET_FIXUP \
>	( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \
>	  PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define
> PHYS_OFFSET_FIXUP 0 #endif
>
> And then, after my patch is applied, changing:
>
> __v7_setup_stack_ptr:
>        .word   __v7_setup_stack - .
>
> into:
>
> __v7_setup_stack_ptr:
>        .word   __v7_setup_stack - . + PHYS_OFFSET_FIXUP
>
> should do the trick. This way it'll work for all those places where
> the code is getting at the data area when the MMU is off with no XIP
> conditionals in the code.


Tested....it works!



Here's your new results:

__v7_ca17mp_setup:
	mov	r10, #0
1:	adr	r0, __v7_setup_stack_ptr
r0=0x18213df4

	ldr	r12, [r0]
r12=0x7e174cc

	add	r12, r12, r0			@ the local stack
r12=0x2002b2c0

	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
	bl      v7_invalidate_l1


0x2002b2c0 is indeed the correct physical address of __v7_setup_stack_ptr.

So, you still need to use #ifdef XIP_KERNEL and PLAT_PHYS_OFFSET....but...you bury it in another file. I'm good with that!!


> Then another patch could bring all those places XIP compatible with
> the simple addition of that PHYS_OFFSET_FIXUP constant.

Yes, the less CONFIG_XIP_KERNEL is sprinkled around the arm tree code, the better.

Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 18, 2015, 9:36 p.m. UTC | #4 
On Wed, 18 Nov 2015, Chris Brandt wrote:

> > Probably the best way to fix it would be something like:
> >
> > in asm/memory.h or similar:
> >
> > #ifdef CONFIG_XIP_KERNEL
> > #define PHYS_OFFSET_FIXUP \
> >	( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \
> >	  PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define
> > PHYS_OFFSET_FIXUP 0 #endif
> >
> > And then, after my patch is applied, changing:
> >
> > __v7_setup_stack_ptr:
> >        .word   __v7_setup_stack - .
> >
> > into:
> >
> > __v7_setup_stack_ptr:
> >        .word   __v7_setup_stack - . + PHYS_OFFSET_FIXUP
> >
> > should do the trick. This way it'll work for all those places where
> > the code is getting at the data area when the MMU is off with no XIP
> > conditionals in the code.
> 
> 
> Tested....it works!

Excellent.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Jan. 29, 2016, 9:12 p.m. UTC | #5 
On Wed, 18 Nov 2015, Nicolas Pitre wrote:

> On Wed, 18 Nov 2015, Chris Brandt wrote:
>
> > > Probably the best way to fix it would be something like:
> > >
> > > in asm/memory.h or similar:
> > >
> > > #ifdef CONFIG_XIP_KERNEL
> > > #define PHYS_OFFSET_FIXUP \
> > >	( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \
> > >	  PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define  
> > >PHYS_OFFSET_FIXUP 0 #endif
> > >
> > > And then, after my patch is applied, changing:
> > >
> > > __v7_setup_stack_ptr:
> > >        .word   __v7_setup_stack - .
> > >
> > > into:
> > >
> > > __v7_setup_stack_ptr:
> > >        .word   __v7_setup_stack - . + PHYS_OFFSET_FIXUP
> > >
> > > should do the trick. This way it'll work for all those places where 
> > > the code is getting at the data area when the MMU is off with no XIP 
> > > conditionals in the code.
> > 
> > 
> > Tested....it works!
>
> Excellent.
>
>
> Nicolas


Nicolas,

I applied your PHYS_OFFSET_FIXUP macro code (shown above) to the current tree which already has your commit "ARM: 8453/2: proc-v7.S: don't locate temporary stack space in .text section".

After testing, it shows the same results for an XIP_KERNEL build as it did in November:

   No PHYS_OFFSET_FIXUP = crash

   with PHYS_OFFSET_FIXUP = good


Can I submit a patch with your code for review and inclusion to the kernel?


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Jan. 29, 2016, 9:17 p.m. UTC | #6 
On Fri, 29 Jan 2016, Chris Brandt wrote:

> On Wed, 18 Nov 2015, Nicolas Pitre wrote:
> 
> > On Wed, 18 Nov 2015, Chris Brandt wrote:
> >
> > > > Probably the best way to fix it would be something like:
> > > >
> > > > in asm/memory.h or similar:
> > > >
> > > > #ifdef CONFIG_XIP_KERNEL
> > > > #define PHYS_OFFSET_FIXUP \
> > > >	( XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) - PAGE_OFFSET + \
> > > >	  PLAT_PHYS_OFFSET - CONFIG_XIP_PHYS_ADDR ) #else #define  
> > > >PHYS_OFFSET_FIXUP 0 #endif
> > > >
> > > > And then, after my patch is applied, changing:
> > > >
> > > > __v7_setup_stack_ptr:
> > > >        .word   __v7_setup_stack - .
> > > >
> > > > into:
> > > >
> > > > __v7_setup_stack_ptr:
> > > >        .word   __v7_setup_stack - . + PHYS_OFFSET_FIXUP
> > > >
> > > > should do the trick. This way it'll work for all those places where 
> > > > the code is getting at the data area when the MMU is off with no XIP 
> > > > conditionals in the code.
> > > 
> > > 
> > > Tested....it works!
> >
> > Excellent.
> >
> >
> > Nicolas
> 
> 
> Nicolas,
> 
> I applied your PHYS_OFFSET_FIXUP macro code (shown above) to the current tree which already has your commit "ARM: 8453/2: proc-v7.S: don't locate temporary stack space in .text section".
> 
> After testing, it shows the same results for an XIP_KERNEL build as it did in November:
> 
>    No PHYS_OFFSET_FIXUP = crash
> 
>    with PHYS_OFFSET_FIXUP = good
> 
> 
> Can I submit a patch with your code for review and inclusion to the kernel?

Sure.

Signed-off-by: Nicolas Pitre <nico@linaro.org>


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index de2b246fed..2d0ac32320 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -274,10 +274,12 @@  __v7_ca15mp_setup:
 __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
-1:	adr	r12, __v7_setup_stack		@ the local stack
-	stmia	r12, {r0-r5, lr}		@ v7_invalidate_l1 touches r0-r6
+1:	adr	r0, __v7_setup_stack_ptr
+	ldr	r12, [r0]
+	add	r12, r12, r0			@ the local stack
+	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
 	bl      v7_invalidate_l1
-	ldmia	r12, {r0-r5, lr}
+	ldmia	r12, {r1-r6, lr}
 #ifdef CONFIG_SMP
 	ALT_SMP(mrc	p15, 0, r0, c1, c0, 1)
 	ALT_UP(mov	r0, #(1 << 6))		@ fake it for UP
@@ -415,10 +417,12 @@  __v7_pj4b_setup:
 #endif /* CONFIG_CPU_PJ4B */
 
 __v7_setup:
-	adr	r12, __v7_setup_stack		@ the local stack
-	stmia	r12, {r0-r5, lr}		@ v7_invalidate_l1 touches r0-r6
+	adr	r0, __v7_setup_stack_ptr
+	ldr	r12, [r0]
+	add	r12, r12, r0			@ the local stack
+	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
 	bl      v7_invalidate_l1
-	ldmia	r12, {r0-r5, lr}
+	ldmia	r12, {r1-r6, lr}
 
 __v7_setup_cont:
 	and	r0, r9, #0xff000000		@ ARM?
@@ -480,11 +484,16 @@  __errata_finish:
 	orr	r0, r0, r6			@ set them
  THUMB(	orr	r0, r0, #1 << 30	)	@ Thumb exceptions
 	ret	lr				@ return to head.S:__ret
+
+	.align	2
+__v7_setup_stack_ptr:
+	.word	__v7_setup_stack - .
 ENDPROC(__v7_setup)
 
+	.bss
 	.align	2
 __v7_setup_stack:
-	.space	4 * 7				@ 12 registers
+	.space	4 * 7				@ 7 registers
 
 	__INITDATA
 
diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
index 67d9209077..8cd465927a 100644
--- a/arch/arm/mm/proc-v7m.S
+++ b/arch/arm/mm/proc-v7m.S
@@ -123,10 +123,12 @@  __v7m_setup:
 	ret	lr
 ENDPROC(__v7m_setup)
 
+	.pushsection .bss
 	.align 2
 __v7m_setup_stack:
 	.space	4 * 8				@ 8 registers
 __v7m_setup_stack_top:
+	.previous
 
 	define_processor_functions v7m, dabort=nommu_early_abort, pabort=legacy_pabort, nommu=1