v7-M: Fixing XIP when the kernel is in ROM

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

I've been trying to make my ARM v7-M LPC43xx board
boot a XIP kernel from flash. Currently, this seems
to be broken in mainline due to this:

arch/arm/mm/proc-v7m.S
[..]
        @ SVC to run the kernel in this mode
        badr    r1, 1f
        ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
        str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
        mov     r6, lr                  @ save LR
        mov     r7, sp                  @ save SP
	ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
        cpsie   i
        svc     #0
1:      cpsid   i
        str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
        mov     lr, r6                  @ restore LR
        mov     sp, r7                  @ restore SP

Here, a temporary stack is prepared before making a
supervisor call (SVC) to switch to handler mode.

The temporary stack is allocated in the .text.init section
and so this doesn't work when the kernel is executing from ROM.

A similar problem has been reported for v7:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357106.html

While trying to come up with a proper fix, I've noticed how
the stack doesn't seem to be used.

So, I've been trying to understand why the need for the temporary
stack@all, but I still can't get it. 

The below patch seems to work just fine, and allows to boot a
LPC43xx kernel either as XIP from ROM or non-XIP from RAM.

However, I'm still wondering if the stack is really unused or not,
so any lights that can be shed here will be appreciated.

Thanks!

>From a7c880c73b8ad2e4c4b07f4d11809ea541a65e1d Mon Sep 17 00:00:00 2001
From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date: Sat, 24 Oct 2015 13:27:27 -0300
Subject: [PATCH] ARM: Don't prepare any temporary stack in __v7m_setup

Since __v7m_setup() is implemented as the PROCINFO_INITFUNC
called in head-nommu.S it's called at the very beggining to
do some very basic setup.

The function prepares a temporary stack in the .text.init
section before calling SVC. However, this stack seems to
be completely unused and hence is not needed.

Moreover, this breaks on XIP kernels, when the text is in ROM.
Hence, this commit simply removes the temporary stack setup.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/arm/mm/proc-v7m.S | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
index 67d9209077c6..6a383e619a0c 100644
--- a/arch/arm/mm/proc-v7m.S
+++ b/arch/arm/mm/proc-v7m.S
@@ -103,7 +103,6 @@ __v7m_setup:
 	str	r1, [r12, #11 * 4]	@ write the temporary SVC vector entry
 	mov	r6, lr			@ save LR
 	mov	r7, sp			@ save SP
-	ldr	sp, =__v7m_setup_stack_top
 	cpsie	i
 	svc	#0
 1:	cpsid	i
@@ -123,11 +122,6 @@ __v7m_setup:
 	ret	lr
 ENDPROC(__v7m_setup)
 
-	.align 2
-__v7m_setup_stack:
-	.space	4 * 8				@ 8 registers
-__v7m_setup_stack_top:
-
 	define_processor_functions v7m, dabort=nommu_early_abort, pabort=legacy_pabort, nommu=1
 
 	.section ".rodata"
-- 
2.5.2

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello,

On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
> I've been trying to make my ARM v7-M LPC43xx board
> boot a XIP kernel from flash. Currently, this seems

I admit I didn't update my efm32 machine for quite some time, but this
can only boot with XIP.

> to be broken in mainline due to this:
> 
> arch/arm/mm/proc-v7m.S
> [..]
>         @ SVC to run the kernel in this mode
>         badr    r1, 1f
>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>         mov     r6, lr                  @ save LR
>         mov     r7, sp                  @ save SP
> 	ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
If you make the tab above 8 spaces the line will align in the git
commit, too.
Hmm, this line is there from the beginning (i.e. 55bdd6941165 ("ARM: Add
base support for ARMv7-M")).

>         cpsie   i
>         svc     #0
> 1:      cpsid   i
>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
>         mov     lr, r6                  @ restore LR
>         mov     sp, r7                  @ restore SP
> 
> Here, a temporary stack is prepared before making a
> supervisor call (SVC) to switch to handler mode.
> 
> The temporary stack is allocated in the .text.init section
> and so this doesn't work when the kernel is executing from ROM.

If sp isn't used, how does it break you setup? Did you try to put a
watchpoint to the location in question?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

On 26 October 2015 at 05:05, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
>> I've been trying to make my ARM v7-M LPC43xx board
>> boot a XIP kernel from flash. Currently, this seems
>
> I admit I didn't update my efm32 machine for quite some time, but this
> can only boot with XIP.
>

Executing directly from read-only memory? Hmm, that's odd.

>> to be broken in mainline due to this:
>>
>> arch/arm/mm/proc-v7m.S
>> [..]
>>         @ SVC to run the kernel in this mode
>>         badr    r1, 1f
>>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
>>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>>         mov     r6, lr                  @ save LR
>>         mov     r7, sp                  @ save SP
>>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
> If you make the tab above 8 spaces the line will align in the git
> commit, too.
> Hmm, this line is there from the beginning (i.e. 55bdd6941165 ("ARM: Add
> base support for ARMv7-M")).
>

Yes, I know. And was there in Catalin's first patches, hence why I'm asking :-)

>>         cpsie   i
>>         svc     #0
>> 1:      cpsid   i
>>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
>>         mov     lr, r6                  @ restore LR
>>         mov     sp, r7                  @ restore SP
>>
>> Here, a temporary stack is prepared before making a
>> supervisor call (SVC) to switch to handler mode.
>>
>> The temporary stack is allocated in the .text.init section
>> and so this doesn't work when the kernel is executing from ROM.
>
> If sp isn't used, how does it break you setup?

Well, the supervisor call uses the stack, but not the Linux code.
>From the Application Note 179:

""
2.7. Supervisor Calls (SVC)
[..]
On the Cortex-M3, the core saves the argument registers to the stack
on the initial exception entry.
A late-arriving exception, taken before the first instruction of the
SVC handler executes,
might corrupt the copy of the arguments still held in R0 to R3. This
means that the stack copy
of the arguments must be used by the SVC handler
""

> Did you try to put a watchpoint to the location in question?
>

Sort of. I used a low level assembly call to printuart and followed
the execution
to that instruction. The CPU never seems to get pass the supervisor call.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

(Let's Cc the maintainers for the rest of the cortex-M platforms.
Hopefully they can help review/test the proposed patch.)

On 26 October 2015 at 10:12, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 26 October 2015 at 05:05, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
>> Hello,
>>
>> On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
>>> I've been trying to make my ARM v7-M LPC43xx board
>>> boot a XIP kernel from flash. Currently, this seems
>>
>> I admit I didn't update my efm32 machine for quite some time, but this
>> can only boot with XIP.
>>
>
> Executing directly from read-only memory? Hmm, that's odd.
>
>>> to be broken in mainline due to this:
>>>
>>> arch/arm/mm/proc-v7m.S
>>> [..]
>>>         @ SVC to run the kernel in this mode
>>>         badr    r1, 1f
>>>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
>>>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>>>         mov     r6, lr                  @ save LR
>>>         mov     r7, sp                  @ save SP
>>>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
>> If you make the tab above 8 spaces the line will align in the git
>> commit, too.
>> Hmm, this line is there from the beginning (i.e. 55bdd6941165 ("ARM: Add
>> base support for ARMv7-M")).
>>
>
> Yes, I know. And was there in Catalin's first patches, hence why I'm asking :-)
>
>>>         cpsie   i
>>>         svc     #0
>>> 1:      cpsid   i
>>>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
>>>         mov     lr, r6                  @ restore LR
>>>         mov     sp, r7                  @ restore SP
>>>
>>> Here, a temporary stack is prepared before making a
>>> supervisor call (SVC) to switch to handler mode.
>>>
>>> The temporary stack is allocated in the .text.init section
>>> and so this doesn't work when the kernel is executing from ROM.
>>
>> If sp isn't used, how does it break you setup?
>
> Well, the supervisor call uses the stack, but not the Linux code.
> From the Application Note 179:
>
> ""
> 2.7. Supervisor Calls (SVC)
> [..]
> On the Cortex-M3, the core saves the argument registers to the stack
> on the initial exception entry.
> A late-arriving exception, taken before the first instruction of the
> SVC handler executes,
> might corrupt the copy of the arguments still held in R0 to R3. This
> means that the stack copy
> of the arguments must be used by the SVC handler
> ""
>
>> Did you try to put a watchpoint to the location in question?
>>
>
> Sort of. I used a low level assembly call to printuart and followed
> the execution
> to that instruction. The CPU never seems to get pass the supervisor call.
> --
> Ezequiel Garc?a, VanguardiaSur
> www.vanguardiasur.com.ar



-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Maxime Coquelin]

Hi Ezequiel,

On 10/27/2015 04:35 PM, Ezequiel Garcia wrote:
>> >>>The temporary stack is allocated in the .text.init section
>> >>>and so this doesn't work when the kernel is executing from ROM.
> >>
> >>If sp isn't used, how does it break you setup?
STM32 machine works fine with XIP from internal flash memory, so as Uwe, 
I'm surprised it solves your problem.

I will have a try with your patch later today.

Regards,
Maxime

v7-M: Fixing XIP when the kernel is in ROM

[Maxime Coquelin]

2015-10-27 17:03 GMT+01:00 Maxime Coquelin <maxime.coquelin@st.com>:
> Hi Ezequiel,
>
> On 10/27/2015 04:35 PM, Ezequiel Garcia wrote:
>>>
>>> >>>The temporary stack is allocated in the .text.init section
>>> >>>and so this doesn't work when the kernel is executing from ROM.
>>
>> >>
>> >>If sp isn't used, how does it break you setup?
>
> STM32 machine works fine with XIP from internal flash memory, so as Uwe, I'm
> surprised it solves your problem.
>
> I will have a try with your patch later today.

Just tested, and confirm it works with (and without) your patch in XIP on STM32.

Regards,
Maxime

v7-M: Fixing XIP when the kernel is in ROM

[Stefan Agner]

On 2015-10-27 13:25, Maxime Coquelin wrote:
> 2015-10-27 17:03 GMT+01:00 Maxime Coquelin <maxime.coquelin@st.com>:
>> Hi Ezequiel,
>>
>> On 10/27/2015 04:35 PM, Ezequiel Garcia wrote:
>>>>
>>>> >>>The temporary stack is allocated in the .text.init section
>>>> >>>and so this doesn't work when the kernel is executing from ROM.
>>>
>>> >>
>>> >>If sp isn't used, how does it break you setup?
>>
>> STM32 machine works fine with XIP from internal flash memory, so as Uwe, I'm
>> surprised it solves your problem.
>>
>> I will have a try with your patch later today.
> 
> Just tested, and confirm it works with (and without) your patch in XIP on STM32.

It probably depends what exactly happens when the CPU core tries to use
the stack. I mean, the CPU core will issue a write to the flash, and how
that behaves is probably implementation specific. However, I would
expect that it should lead to some kind of fault... Do we have fault
handlers at that time?

Anyway, I guess Ezequiel's fix is the right thing to do...

--
Stefan

v7-M: Fixing XIP when the kernel is in ROM

[Maxime Coquelin]

2015-10-27 21:33 GMT+01:00 Stefan Agner <stefan@agner.ch>:
> On 2015-10-27 13:25, Maxime Coquelin wrote:
>> 2015-10-27 17:03 GMT+01:00 Maxime Coquelin <maxime.coquelin@st.com>:
>>> Hi Ezequiel,
>>>
>>> On 10/27/2015 04:35 PM, Ezequiel Garcia wrote:
>>>>>
>>>>> >>>The temporary stack is allocated in the .text.init section
>>>>> >>>and so this doesn't work when the kernel is executing from ROM.
>>>>
>>>> >>
>>>> >>If sp isn't used, how does it break you setup?
>>>
>>> STM32 machine works fine with XIP from internal flash memory, so as Uwe, I'm
>>> surprised it solves your problem.
>>>
>>> I will have a try with your patch later today.
>>
>> Just tested, and confirm it works with (and without) your patch in XIP on STM32.
>
> It probably depends what exactly happens when the CPU core tries to use
> the stack. I mean, the CPU core will issue a write to the flash, and how
> that behaves is probably implementation specific. However, I would
> expect that it should lead to some kind of fault... Do we have fault
> handlers at that time?

Yes we have fault handlers at that time.
I agree the behaviour is probably implementation specific.

>
> Anyway, I guess Ezequiel's fix is the right thing to do...
Or, maybe we should put the stack in RAM as Uwe suggest?

Regards,
Maxime

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

On 27 October 2015 at 18:33, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
> 2015-10-27 21:33 GMT+01:00 Stefan Agner <stefan@agner.ch>:
>> On 2015-10-27 13:25, Maxime Coquelin wrote:
>>> 2015-10-27 17:03 GMT+01:00 Maxime Coquelin <maxime.coquelin@st.com>:
>>>> Hi Ezequiel,
>>>>
>>>> On 10/27/2015 04:35 PM, Ezequiel Garcia wrote:
>>>>>>
>>>>>> >>>The temporary stack is allocated in the .text.init section
>>>>>> >>>and so this doesn't work when the kernel is executing from ROM.
>>>>>
>>>>> >>
>>>>> >>If sp isn't used, how does it break you setup?
>>>>
>>>> STM32 machine works fine with XIP from internal flash memory, so as Uwe, I'm
>>>> surprised it solves your problem.
>>>>
>>>> I will have a try with your patch later today.
>>>
>>> Just tested, and confirm it works with (and without) your patch in XIP on STM32.
>>
>> It probably depends what exactly happens when the CPU core tries to use
>> the stack. I mean, the CPU core will issue a write to the flash, and how
>> that behaves is probably implementation specific. However, I would
>> expect that it should lead to some kind of fault... Do we have fault
>> handlers at that time?
>
> Yes we have fault handlers at that time.
> I agree the behaviour is probably implementation specific.
>
>>
>> Anyway, I guess Ezequiel's fix is the right thing to do...
> Or, maybe we should put the stack in RAM as Uwe suggest?
>

It seems the current fix would be used as-is (no need to
set a temporary stack before the SVC call).

And on top of that, we need a to set a proper stack as soon
as the kernel takes control.

Am I right?
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Maxime Coquelin]

2015-10-27 22:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 27 October 2015 at 18:33, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
>> 2015-10-27 21:33 GMT+01:00 Stefan Agner <stefan@agner.ch>:
...
>>> Anyway, I guess Ezequiel's fix is the right thing to do...
>> Or, maybe we should put the stack in RAM as Uwe suggest?
>>
>
> It seems the current fix would be used as-is (no need to
> set a temporary stack before the SVC call).
>
> And on top of that, we need a to set a proper stack as soon
> as the kernel takes control.
>
> Am I right?
IMHO, yes, that's how I understand it.

Maxime

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

On 27 October 2015 at 18:52, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
> 2015-10-27 22:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 27 October 2015 at 18:33, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
>>> 2015-10-27 21:33 GMT+01:00 Stefan Agner <stefan@agner.ch>:
> ...
>>>> Anyway, I guess Ezequiel's fix is the right thing to do...
>>> Or, maybe we should put the stack in RAM as Uwe suggest?
>>>
>>
>> It seems the current fix would be used as-is (no need to
>> set a temporary stack before the SVC call).
>>
>> And on top of that, we need a to set a proper stack as soon
>> as the kernel takes control.
>>
>> Am I right?
> IMHO, yes, that's how I understand it.
>

After some deeper code parsing, it seems the stack
is set in __mmap_switched.

Although it seems that happens after __v7m_setup,
ARM noMMU has worked like this since ages now, is v7-M
really different to need a special treatment?

-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello Ezequiel,

On Tue, Oct 27, 2015 at 07:08:46PM -0300, Ezequiel Garcia wrote:
> On 27 October 2015 at 18:52, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
> > 2015-10-27 22:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> >> On 27 October 2015 at 18:33, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote:
> >>> 2015-10-27 21:33 GMT+01:00 Stefan Agner <stefan@agner.ch>:
> > ...
> >>>> Anyway, I guess Ezequiel's fix is the right thing to do...
> >>> Or, maybe we should put the stack in RAM as Uwe suggest?
> >>>
> >>
> >> It seems the current fix would be used as-is (no need to
> >> set a temporary stack before the SVC call).
> >>
> >> And on top of that, we need a to set a proper stack as soon
> >> as the kernel takes control.
> >>
> >> Am I right?
> > IMHO, yes, that's how I understand it.
> >
> 
> After some deeper code parsing, it seems the stack
> is set in __mmap_switched.
> 
> Although it seems that happens after __v7m_setup,
> ARM noMMU has worked like this since ages now, is v7-M
> really different to need a special treatment?

Yeah, v7-M needs stack in it's setup function, while earlier no-MMU
machines don't. (I didn't check that though.)

So the current situation is that in svc something is written to
__v7m_setup_stack which doesn't seem to hurt in practise on most
machines, but on your's is provokes a hang.

With your patch the same something is written to whereever sp points to
when the kernel is entered. This is hardly better.

So the right fix is to move __v7m_setup_stack to .data I guess.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

v7-M: Fixing XIP when the kernel is in ROM

[Chris Brandt]

> So the right fix is to move __v7m_setup_stack to .data I guess.


Since my set of patches went nowhere, last week we had a look at doing just that (using a pre-allocated stack in .data instead of hard coding to the top of PLAT_PHYS_OFFSET).

Here's the code we came up with. Seems to work on XIP and non-XIP builds as well as SMP and non-SMP.


Maybe you can try this technique to allocate the temporary stack in the data section.


(and if your patch gets in...then maybe I'll try submitting again...)


-Chris




arch/arm/mm/proc-v7.S |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

--- 0001/arch/arm/mm/proc-v7.S
+++ work/arch/arm/mm/proc-v7.S	2015-10-28 15:44:48.980513000 +0900
@@ -274,7 +274,15 @@ __v7_ca15mp_setup:
 __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
-1:	adr	r12, __v7_setup_stack		@ the local stack
+1:	adr	r11, __v7_setup_stack_ptr	@ pointer to local stack
+	ldmia	r11, {r0, r12}
+#ifdef CONFIG_XIP_KERNEL
+	ldr	r11, =PLAT_PHYS_OFFSET		@ fixed address
+#else
+	sub	r11, r11, r0			@ position independent offset
+#endif	
+	add	r12, r12, r11			@ phys address
+	sub	r12, #PAGE_OFFSET
 	stmia	r12, {r0-r5, lr}		@ v7_invalidate_l1 touches r0-r6
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, lr}
@@ -415,7 +423,15 @@ __v7_pj4b_setup:
 #endif /* CONFIG_CPU_PJ4B */
 
 __v7_setup:
-	adr	r12, __v7_setup_stack		@ the local stack
+	adr	r11, __v7_setup_stack_ptr	@ pointer to local stack
+	ldmia	r11, {r0, r12}
+#ifdef CONFIG_XIP_KERNEL
+	ldr	r11, =PLAT_PHYS_OFFSET		@ fixed address
+#else
+	sub	r11, r11, r0			@ position independent offset
+#endif	
+	add	r12, r12, r11			@ phys address
+	sub	r12, #PAGE_OFFSET
 	stmia	r12, {r0-r5, lr}		@ v7_invalidate_l1 touches r0-r6
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, lr}
@@ -482,6 +498,11 @@ __errata_finish:
 	ret	lr				@ return to head.S:__ret
 ENDPROC(__v7_setup)
 
+__v7_setup_stack_ptr:
+	.long	.
+	.long	__v7_setup_stack
+
+	.data
 	.align	2
 __v7_setup_stack:
 	.space	4 * 7				@ 12 registers

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello, 

On Tue, Nov 03, 2015 at 05:52:53PM +0000, Chris Brandt wrote:
> > So the right fix is to move __v7m_setup_stack to .data I guess.
> 
> 
> Since my set of patches went nowhere, last week we had a look at doing just that (using a pre-allocated stack in .data instead of hard coding to the top of PLAT_PHYS_OFFSET).
> 
> Here's the code we came up with. Seems to work on XIP and non-XIP builds as well as SMP and non-SMP.
> 
> 
> Maybe you can try this technique to allocate the temporary stack in the data section.
I think for v7-M it's easier. Just move the stack to .data and use it
from there where the linker put it to.

> arch/arm/mm/proc-v7.S |   25 +++++++++++++++++++++++--
I would have expected a patch to v7m in this thread.

You seem to fix a different problem so I suggest you start a new thread
with a subject that gets the attention from people that know about the
v7-[AR] details.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

v7-M: Fixing XIP when the kernel is in ROM

[Russell King - ARM Linux]

On Tue, Nov 03, 2015 at 09:09:01PM +0100, Uwe Kleine-K?nig wrote:
> Hello, 
> 
> On Tue, Nov 03, 2015 at 05:52:53PM +0000, Chris Brandt wrote:
> > > So the right fix is to move __v7m_setup_stack to .data I guess.
> > 
> > 
> > Since my set of patches went nowhere, last week we had a look at doing just that (using a pre-allocated stack in .data instead of hard coding to the top of PLAT_PHYS_OFFSET).
> > 
> > Here's the code we came up with. Seems to work on XIP and non-XIP builds as well as SMP and non-SMP.
> > 
> > 
> > Maybe you can try this technique to allocate the temporary stack in the data section.
> I think for v7-M it's easier. Just move the stack to .data and use it
> from there where the linker put it to.

Why waste memory - we already have a stack allocated in the .data segment
already.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello Ezequiel,

On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
> I've been trying to make my ARM v7-M LPC43xx board
> boot a XIP kernel from flash. Currently, this seems
> to be broken in mainline due to this:
> 
> arch/arm/mm/proc-v7m.S
> [..]
>         @ SVC to run the kernel in this mode
>         badr    r1, 1f
>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>         mov     r6, lr                  @ save LR
>         mov     r7, sp                  @ save SP
> 	ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!

How does this fail for you?

>         cpsie   i
>         svc     #0
> 1:      cpsid   i
>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
>         mov     lr, r6                  @ restore LR
>         mov     sp, r7                  @ restore SP
> 
> Here, a temporary stack is prepared before making a
> supervisor call (SVC) to switch to handler mode.

OK, the effect of svc is that something is written to where sp points
to. On my efm32 nothing obvious happens when something random is written
there. I guess if that results in some CFI commands I have a problem
though. What about your machine?

> The temporary stack is allocated in the .text.init section
> and so this doesn't work when the kernel is executing from ROM.
> 
> A similar problem has been reported for v7:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357106.html
> 
> While trying to come up with a proper fix, I've noticed how
> the stack doesn't seem to be used.
> 
> So, I've been trying to understand why the need for the temporary
> stack at all, but I still can't get it. 
> 
> The below patch seems to work just fine, and allows to boot a
> LPC43xx kernel either as XIP from ROM or non-XIP from RAM.
> 
> However, I'm still wondering if the stack is really unused or not,
> so any lights that can be shed here will be appreciated.
> 
> Thanks!
> 
> From a7c880c73b8ad2e4c4b07f4d11809ea541a65e1d Mon Sep 17 00:00:00 2001
> From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Date: Sat, 24 Oct 2015 13:27:27 -0300
> Subject: [PATCH] ARM: Don't prepare any temporary stack in __v7m_setup
> 
> Since __v7m_setup() is implemented as the PROCINFO_INITFUNC
> called in head-nommu.S it's called at the very beggining to
> do some very basic setup.
> 
> The function prepares a temporary stack in the .text.init
> section before calling SVC. However, this stack seems to
> be completely unused and hence is not needed.
> 
> Moreover, this breaks on XIP kernels, when the text is in ROM.
> Hence, this commit simply removes the temporary stack setup.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  arch/arm/mm/proc-v7m.S | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
> index 67d9209077c6..6a383e619a0c 100644
> --- a/arch/arm/mm/proc-v7m.S
> +++ b/arch/arm/mm/proc-v7m.S
> @@ -103,7 +103,6 @@ __v7m_setup:
>  	str	r1, [r12, #11 * 4]	@ write the temporary SVC vector entry
>  	mov	r6, lr			@ save LR
>  	mov	r7, sp			@ save SP
> -	ldr	sp, =__v7m_setup_stack_top
>  	cpsie	i
>  	svc	#0
>  1:	cpsid	i
> @@ -123,11 +122,6 @@ __v7m_setup:
>  	ret	lr
>  ENDPROC(__v7m_setup)
>  
> -	.align 2
> -__v7m_setup_stack:
> -	.space	4 * 8				@ 8 registers
> -__v7m_setup_stack_top:
> -

The effect of your patch is that the value of sp as it is when
__v7m_setup is entered is used. I didn't check, but I wouldn't be
surprised if that's the value of sp when the boot loader gave control to
Linux. This might or might not work. Something more robust would be
better of course.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

v7-M: Fixing XIP when the kernel is in ROM

[Ezequiel Garcia]

On 27 October 2015 at 17:21, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Ezequiel,
>
> On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
>> I've been trying to make my ARM v7-M LPC43xx board
>> boot a XIP kernel from flash. Currently, this seems
>> to be broken in mainline due to this:
>>
>> arch/arm/mm/proc-v7m.S
>> [..]
>>         @ SVC to run the kernel in this mode
>>         badr    r1, 1f
>>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
>>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>>         mov     r6, lr                  @ save LR
>>         mov     r7, sp                  @ save SP
>>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
>
> How does this fail for you?
>

My CPU just seems to stall.
I've added calls to the printch in arch/arm/kernel/debug.S
and can't get past the SVC call.

As Stefan suggested, maybe a fault handler is called,
but I'm not getting any fault message printed.

>>         cpsie   i
>>         svc     #0
>> 1:      cpsid   i
>>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
>>         mov     lr, r6                  @ restore LR
>>         mov     sp, r7                  @ restore SP
>>
>> Here, a temporary stack is prepared before making a
>> supervisor call (SVC) to switch to handler mode.
>
> OK, the effect of svc is that something is written to where sp points
> to. On my efm32 nothing obvious happens when something random is written
> there. I guess if that results in some CFI commands I have a problem
> though. What about your machine?
>

Good, I was under the same impression. My only explanation for the fact
that Maxime and you seem to have no problem was that a store
to the internal flash doesn't stall the MCU.

On my side, I have U-Boot and the DTB on the internal flash,
and the kernel is on SPIFI, which can be configured to allow
code execution.

Haven't checked what the effect is if you try to write there.
In any case, it seems wrong to have the stack point to flash.

>> The temporary stack is allocated in the .text.init section
>> and so this doesn't work when the kernel is executing from ROM.
>>
>> A similar problem has been reported for v7:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357106.html
>>
>> While trying to come up with a proper fix, I've noticed how
>> the stack doesn't seem to be used.
>>
>> So, I've been trying to understand why the need for the temporary
>> stack at all, but I still can't get it.
>>
>> The below patch seems to work just fine, and allows to boot a
>> LPC43xx kernel either as XIP from ROM or non-XIP from RAM.
>>
>> However, I'm still wondering if the stack is really unused or not,
>> so any lights that can be shed here will be appreciated.
>>
>> Thanks!
>>
>> From a7c880c73b8ad2e4c4b07f4d11809ea541a65e1d Mon Sep 17 00:00:00 2001
>> From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Date: Sat, 24 Oct 2015 13:27:27 -0300
>> Subject: [PATCH] ARM: Don't prepare any temporary stack in __v7m_setup
>>
>> Since __v7m_setup() is implemented as the PROCINFO_INITFUNC
>> called in head-nommu.S it's called at the very beggining to
>> do some very basic setup.
>>
>> The function prepares a temporary stack in the .text.init
>> section before calling SVC. However, this stack seems to
>> be completely unused and hence is not needed.
>>
>> Moreover, this breaks on XIP kernels, when the text is in ROM.
>> Hence, this commit simply removes the temporary stack setup.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  arch/arm/mm/proc-v7m.S | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
>> index 67d9209077c6..6a383e619a0c 100644
>> --- a/arch/arm/mm/proc-v7m.S
>> +++ b/arch/arm/mm/proc-v7m.S
>> @@ -103,7 +103,6 @@ __v7m_setup:
>>       str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
>>       mov     r6, lr                  @ save LR
>>       mov     r7, sp                  @ save SP
>> -     ldr     sp, =__v7m_setup_stack_top
>>       cpsie   i
>>       svc     #0
>>  1:   cpsid   i
>> @@ -123,11 +122,6 @@ __v7m_setup:
>>       ret     lr
>>  ENDPROC(__v7m_setup)
>>
>> -     .align 2
>> -__v7m_setup_stack:
>> -     .space  4 * 8                           @ 8 registers
>> -__v7m_setup_stack_top:
>> -
>
> The effect of your patch is that the value of sp as it is when
> __v7m_setup is entered is used. I didn't check, but I wouldn't be
> surprised if that's the value of sp when the boot loader gave control to
> Linux. This might or might not work. Something more robust would be
> better of course.
>

Well, this is where I'm a bit lost, as I was unable to find where
the stack is setup in Linux.

But on the other side, the current code, just sets a *temporary*
stack to fit the 8 registers needed for the SVC call. The stack
pointer is restored right after that.

So in reality, the effect of the patch above is to require some
additional space for the already in-use stack.

I can't see why it wouldn't be robust.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello Ezequiel,

On Tue, Oct 27, 2015 at 05:57:46PM -0300, Ezequiel Garcia wrote:
> On 27 October 2015 at 17:21, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hello Ezequiel,
> >
> > On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
> >> I've been trying to make my ARM v7-M LPC43xx board
> >> boot a XIP kernel from flash. Currently, this seems
> >> to be broken in mainline due to this:
> >>
> >> arch/arm/mm/proc-v7m.S
> >> [..]
> >>         @ SVC to run the kernel in this mode
> >>         badr    r1, 1f
> >>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
> >>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
> >>         mov     r6, lr                  @ save LR
> >>         mov     r7, sp                  @ save SP
> >>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
> >
> > How does this fail for you?
> >
> 
> My CPU just seems to stall.
> I've added calls to the printch in arch/arm/kernel/debug.S
> and can't get past the SVC call.
Can you try to add something like:

	stmdb	sp!, {r0-r4}

after setting sp as above and check if that makes the machine hang, too?

> As Stefan suggested, maybe a fault handler is called,
> but I'm not getting any fault message printed.

Hmm, the fault handler should be up, didn't verify though.

> >>         cpsie   i
> >>         svc     #0
> >> 1:      cpsid   i
> >>         str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry
> >>         mov     lr, r6                  @ restore LR
> >>         mov     sp, r7                  @ restore SP
> >>
> >> Here, a temporary stack is prepared before making a
> >> supervisor call (SVC) to switch to handler mode.
> >
> > OK, the effect of svc is that something is written to where sp points
> > to. On my efm32 nothing obvious happens when something random is written
> > there. I guess if that results in some CFI commands I have a problem
> > though. What about your machine?
> >
> 
> Good, I was under the same impression. My only explanation for the fact
> that Maxime and you seem to have no problem was that a store
> to the internal flash doesn't stall the MCU.
> 
> On my side, I have U-Boot and the DTB on the internal flash,
> and the kernel is on SPIFI, which can be configured to allow
> code execution.
> 
> Haven't checked what the effect is if you try to write there.
> In any case, it seems wrong to have the stack point to flash.

That's right, so we need some writeable memory there. Where should we
take that from?

> >> The temporary stack is allocated in the .text.init section
> >> and so this doesn't work when the kernel is executing from ROM.
> >> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
> >> index 67d9209077c6..6a383e619a0c 100644
> >> --- a/arch/arm/mm/proc-v7m.S
> >> +++ b/arch/arm/mm/proc-v7m.S
> >> @@ -103,7 +103,6 @@ __v7m_setup:
> >>       str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
> >>       mov     r6, lr                  @ save LR
> >>       mov     r7, sp                  @ save SP
> >> -     ldr     sp, =__v7m_setup_stack_top
> >>       cpsie   i
> >>       svc     #0
> >>  1:   cpsid   i
> >> @@ -123,11 +122,6 @@ __v7m_setup:
> >>       ret     lr
> >>  ENDPROC(__v7m_setup)
> >>
> >> -     .align 2
> >> -__v7m_setup_stack:
> >> -     .space  4 * 8                           @ 8 registers
> >> -__v7m_setup_stack_top:
> >> -
> >
> > The effect of your patch is that the value of sp as it is when
> > __v7m_setup is entered is used. I didn't check, but I wouldn't be
> > surprised if that's the value of sp when the boot loader gave control to
> > Linux. This might or might not work. Something more robust would be
> > better of course.
> >
> 
> Well, this is where I'm a bit lost, as I was unable to find where
> the stack is setup in Linux.

Hmm, that's long time ago that I worked on that code. Didn't find it but
only looked a few minutes. Looking at arch/arm/kernel/head-nommu.S it
seems __lookup_processor_type is called without explicitly initializing
sp first. Hmm.

> But on the other side, the current code, just sets a *temporary*
> stack to fit the 8 registers needed for the SVC call. The stack
> pointer is restored right after that.
> 
> So in reality, the effect of the patch above is to require some
> additional space for the already in-use stack.

With your patch and on top of arch/arm/kernel/head-nommu.S adding

	ldr	sp, =(some_address_in_ROM)

crashes your kernel, right? This means we either have to find a save
location to put the stack to, or need to document that the sp register
needs to be setup to have some stack available when the kernel image is
jumped into. I don't like the latter.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

v7-M: Fixing XIP when the kernel is in ROM

[Russell King - ARM Linux]

On Tue, Oct 27, 2015 at 10:20:20PM +0100, Uwe Kleine-K?nig wrote:
> Hello Ezequiel,
> 
> On Tue, Oct 27, 2015 at 05:57:46PM -0300, Ezequiel Garcia wrote:
> > On 27 October 2015 at 17:21, Uwe Kleine-K?nig
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > Hello Ezequiel,
> > >
> > > On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
> > >> I've been trying to make my ARM v7-M LPC43xx board
> > >> boot a XIP kernel from flash. Currently, this seems
> > >> to be broken in mainline due to this:
> > >>
> > >> arch/arm/mm/proc-v7m.S
> > >> [..]
> > >>         @ SVC to run the kernel in this mode
> > >>         badr    r1, 1f
> > >>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
> > >>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
> > >>         mov     r6, lr                  @ save LR
> > >>         mov     r7, sp                  @ save SP
> > >>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
> > >
> > > How does this fail for you?
> > >
> > 
> > My CPU just seems to stall.
> > I've added calls to the printch in arch/arm/kernel/debug.S
> > and can't get past the SVC call.
> Can you try to add something like:
> 
> 	stmdb	sp!, {r0-r4}

Wait one moment, and try reading the code.

The call path to this point is: entry text at stext in head-nommu.S
which goes on to call __v7m_setup.

Nothing in that path sets up a stack, so the value of 'sp' is
undefined - it's entirely whatever's left in the register after the
boot loader has passed control to the kernel.

So, it's pointless proc-v7m.S saving and restoring the stack pointer
around this.  It also means that there probably isn't a reliable
stack here.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

v7-M: Fixing XIP when the kernel is in ROM

[Uwe Kleine-König]

Hello Russell,

On Tue, Oct 27, 2015 at 10:40:19PM +0000, Russell King - ARM Linux wrote:
> On Tue, Oct 27, 2015 at 10:20:20PM +0100, Uwe Kleine-K?nig wrote:
> > Hello Ezequiel,
> > 
> > On Tue, Oct 27, 2015 at 05:57:46PM -0300, Ezequiel Garcia wrote:
> > > On 27 October 2015 at 17:21, Uwe Kleine-K?nig
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > Hello Ezequiel,
> > > >
> > > > On Sun, Oct 25, 2015 at 10:27:10PM -0300, Ezequiel Garcia wrote:
> > > >> I've been trying to make my ARM v7-M LPC43xx board
> > > >> boot a XIP kernel from flash. Currently, this seems
> > > >> to be broken in mainline due to this:
> > > >>
> > > >> arch/arm/mm/proc-v7m.S
> > > >> [..]
> > > >>         @ SVC to run the kernel in this mode
> > > >>         badr    r1, 1f
> > > >>         ldr     r5, [r12, #11 * 4]      @ read the SVC vector entry
> > > >>         str     r1, [r12, #11 * 4]      @ write the temporary SVC vector entry
> > > >>         mov     r6, lr                  @ save LR
> > > >>         mov     r7, sp                  @ save SP
> > > >>       ldr     sp, =__v7m_setup_stack_top @ <<< Breaks XIP!
> > > >
> > > > How does this fail for you?
> > > >
> > > 
> > > My CPU just seems to stall.
> > > I've added calls to the printch in arch/arm/kernel/debug.S
> > > and can't get past the SVC call.
> > Can you try to add something like:
> > 
> > 	stmdb	sp!, {r0-r4}
> 
> Wait one moment, and try reading the code.
> 
> The call path to this point is: entry text at stext in head-nommu.S
> which goes on to call __v7m_setup.
> 
> Nothing in that path sets up a stack, so the value of 'sp' is
> undefined - it's entirely whatever's left in the register after the
> boot loader has passed control to the kernel.

Yeah, that's what I found, too. With this test I want to find out if
it's really the write to ROM memory that makes the machine hang.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |