diff mbox series

[1/8] ARM: ARMv7-M: Fix register restore corrupt after svc call

Message ID 1614758717-18223-2-git-send-email-dillon.minfei@gmail.com
State New
Headers show
Series [1/8] ARM: ARMv7-M: Fix register restore corrupt after svc call | expand

Commit Message

Dillon Min March 3, 2021, 8:05 a.m. UTC 
From: dillon min <dillon.minfei@gmail.com>

For some case, kernel not boot by u-boot(single thread),
but by rtos , as most rtos use pendsv to do context switch.

So, we need add an lr check after svc call, to find out should
use psp or msp. else register restore after svc call might be
corrupted.

Fixes: b70cd406d7fe ("ARM: 8671/1: V7M: Preserve registers across switch from Thread to Handler mode")
Signed-off-by: dillon min <dillon.minfei@gmail.com>
---
 arch/arm/mm/proc-v7m.S | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Vladimir Murzin March 3, 2021, 9:52 a.m. UTC | #1 
On 3/3/21 8:05 AM, dillon.minfei@gmail.com wrote:
> From: dillon min <dillon.minfei@gmail.com>

> 

> For some case, kernel not boot by u-boot(single thread),

> but by rtos , as most rtos use pendsv to do context switch.



Hmm, does it mean that it starts kernel from process context?

I'd assume that it is not only kernel who expects MSP. So, what
if RTOS you mentioned want to boot other RTOS (even itself)? What
if you have no access to the source code for those RTOS(es) to
patch MSP/PSP switch?

I'd very much prefer to keep stack switching logic outside kernel,
say, in some shim which RTOS/bootloader can maintain.

Cheers
Vladimir

> 

> So, we need add an lr check after svc call, to find out should

> use psp or msp. else register restore after svc call might be

> corrupted.

> 

> Fixes: b70cd406d7fe ("ARM: 8671/1: V7M: Preserve registers across switch from Thread to Handler mode")

> Signed-off-by: dillon min <dillon.minfei@gmail.com>

> ---

>  arch/arm/mm/proc-v7m.S | 5 ++++-

>  1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S

> index 84459c1d31b8..c93d2757312d 100644

> --- a/arch/arm/mm/proc-v7m.S

> +++ b/arch/arm/mm/proc-v7m.S

> @@ -137,7 +137,10 @@ __v7m_setup_cont:

>  1:	cpsid	i

>  	/* Calculate exc_ret */

>  	orr	r10, lr, #EXC_RET_THREADMODE_PROCESSSTACK

> -	ldmia	sp, {r0-r3, r12}

> +	tst	lr, #EXC_RET_STACK_MASK

> +	mrsne	r4, psp

> +	moveq	r4, sp

> +	ldmia	r4!, {r0-r3, r12}

>  	str	r5, [r12, #11 * 4]	@ restore the original SVC vector entry

>  	mov	lr, r6			@ restore LR

>  

>
Dillon Min March 3, 2021, 1:35 p.m. UTC | #2 
Hi Vladimir,

Thanks for the review.

On Wed, Mar 3, 2021 at 5:52 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>

> On 3/3/21 8:05 AM, dillon.minfei@gmail.com wrote:

> > From: dillon min <dillon.minfei@gmail.com>

> >

> > For some case, kernel not boot by u-boot(single thread),

> > but by rtos , as most rtos use pendsv to do context switch.

>

>

> Hmm, does it mean that it starts kernel from process context?

   Yes, kernel might be started from process context, since u-boot not
switch context, so kernel always startup under msp.
>

> I'd assume that it is not only kernel who expects MSP. So, what

> if RTOS you mentioned want to boot other RTOS (even itself)? What

> if you have no access to the source code for those RTOS(es) to

> patch MSP/PSP switch?


My case is a little complicated.
stm32h7 only have 128Kbytes internal flash, can't store u-boot.bin (>200K),
so, set a bootloader (rt-thread rtos) to internal flash, load
linux/u-boot from serial port via ymodem
store to qspi flash(8Mbytes), then jump to u-boot.

qspi flash layout:
0 - 512K:    u-boot
512K- 8M : kernel(xip)

load process : rt-thread -> u-boot -> linux

before add psp/msp check after svc call, register restore corrupt.
add a printhex8 around svc call, found the sp stack is 0x24040000c0ffcff8
it should be 0xc0ffcdf8c0ffcff8. 0x24040000 is the sp stack address
assigned by u-boot
i've no idea how it's become to u-boot's sp.

I have the rtos code, and will try to fix it on the rtos side.

Can you give more explanation about why linux relies on MSP ? thanks

>

> I'd very much prefer to keep stack switching logic outside kernel,

> say, in some shim which RTOS/bootloader can maintain.

>

> Cheers

> Vladimir

>

> >

> > So, we need add an lr check after svc call, to find out should

> > use psp or msp. else register restore after svc call might be

> > corrupted.

> >

> > Fixes: b70cd406d7fe ("ARM: 8671/1: V7M: Preserve registers across switch from Thread to Handler mode")

> > Signed-off-by: dillon min <dillon.minfei@gmail.com>

> > ---

> >  arch/arm/mm/proc-v7m.S | 5 ++++-

> >  1 file changed, 4 insertions(+), 1 deletion(-)

> >

> > diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S

> > index 84459c1d31b8..c93d2757312d 100644

> > --- a/arch/arm/mm/proc-v7m.S

> > +++ b/arch/arm/mm/proc-v7m.S

> > @@ -137,7 +137,10 @@ __v7m_setup_cont:

> >  1:   cpsid   i

> >       /* Calculate exc_ret */

> >       orr     r10, lr, #EXC_RET_THREADMODE_PROCESSSTACK

> > -     ldmia   sp, {r0-r3, r12}

> > +     tst     lr, #EXC_RET_STACK_MASK

> > +     mrsne   r4, psp

> > +     moveq   r4, sp

> > +     ldmia   r4!, {r0-r3, r12}

> >       str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry

> >       mov     lr, r6                  @ restore LR

> >

> >

>
Vladimir Murzin March 3, 2021, 2:19 p.m. UTC | #3 
On 3/3/21 1:35 PM, dillon min wrote:
> Hi Vladimir,

> 

> Thanks for the review.

> 

> On Wed, Mar 3, 2021 at 5:52 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:

>>

>> On 3/3/21 8:05 AM, dillon.minfei@gmail.com wrote:

>>> From: dillon min <dillon.minfei@gmail.com>

>>>

>>> For some case, kernel not boot by u-boot(single thread),

>>> but by rtos , as most rtos use pendsv to do context switch.

>>

>>

>> Hmm, does it mean that it starts kernel from process context?

>    Yes, kernel might be started from process context, since u-boot not

> switch context, so kernel always startup under msp.

>>

>> I'd assume that it is not only kernel who expects MSP. So, what

>> if RTOS you mentioned want to boot other RTOS (even itself)? What

>> if you have no access to the source code for those RTOS(es) to

>> patch MSP/PSP switch?

> 

> My case is a little complicated.

> stm32h7 only have 128Kbytes internal flash, can't store u-boot.bin (>200K),

> so, set a bootloader (rt-thread rtos) to internal flash, load

> linux/u-boot from serial port via ymodem

> store to qspi flash(8Mbytes), then jump to u-boot.

> 

> qspi flash layout:

> 0 - 512K:    u-boot

> 512K- 8M : kernel(xip)

> 

> load process : rt-thread -> u-boot -> linux

> 

> before add psp/msp check after svc call, register restore corrupt.

> add a printhex8 around svc call, found the sp stack is 0x24040000c0ffcff8

> it should be 0xc0ffcdf8c0ffcff8. 0x24040000 is the sp stack address

> assigned by u-boot

> i've no idea how it's become to u-boot's sp.

> 

> I have the rtos code, and will try to fix it on the rtos side.


That would be great!

> 

> Can you give more explanation about why linux relies on MSP ? thanks


MSP is what set from boot, thus it is natural assumption that boot code
would preserve that illusion.

I'd guess that kernel is in line in such assumption across different
(RT)OS capable to run on M-class cores (please, note that some variants
might not have two stack pointers)

Cheers
Vladimir

> 

>>

>> I'd very much prefer to keep stack switching logic outside kernel,

>> say, in some shim which RTOS/bootloader can maintain.

>>

>> Cheers

>> Vladimir

>>

>>>

>>> So, we need add an lr check after svc call, to find out should

>>> use psp or msp. else register restore after svc call might be

>>> corrupted.

>>>

>>> Fixes: b70cd406d7fe ("ARM: 8671/1: V7M: Preserve registers across switch from Thread to Handler mode")

>>> Signed-off-by: dillon min <dillon.minfei@gmail.com>

>>> ---

>>>  arch/arm/mm/proc-v7m.S | 5 ++++-

>>>  1 file changed, 4 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S

>>> index 84459c1d31b8..c93d2757312d 100644

>>> --- a/arch/arm/mm/proc-v7m.S

>>> +++ b/arch/arm/mm/proc-v7m.S

>>> @@ -137,7 +137,10 @@ __v7m_setup_cont:

>>>  1:   cpsid   i

>>>       /* Calculate exc_ret */

>>>       orr     r10, lr, #EXC_RET_THREADMODE_PROCESSSTACK

>>> -     ldmia   sp, {r0-r3, r12}

>>> +     tst     lr, #EXC_RET_STACK_MASK

>>> +     mrsne   r4, psp

>>> +     moveq   r4, sp

>>> +     ldmia   r4!, {r0-r3, r12}

>>>       str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry

>>>       mov     lr, r6                  @ restore LR

>>>

>>>

>>

>
Dillon Min March 4, 2021, 5:42 a.m. UTC | #4 
On Wed, Mar 3, 2021 at 10:19 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>

> On 3/3/21 1:35 PM, dillon min wrote:

> > Hi Vladimir,

> >

> > Thanks for the review.

> >

> > On Wed, Mar 3, 2021 at 5:52 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:

> >>

> >> On 3/3/21 8:05 AM, dillon.minfei@gmail.com wrote:

> >>> From: dillon min <dillon.minfei@gmail.com>

> >>>

> >>> For some case, kernel not boot by u-boot(single thread),

> >>> but by rtos , as most rtos use pendsv to do context switch.

> >>

> >>

> >> Hmm, does it mean that it starts kernel from process context?

> >    Yes, kernel might be started from process context, since u-boot not

> > switch context, so kernel always startup under msp.

> >>

> >> I'd assume that it is not only kernel who expects MSP. So, what

> >> if RTOS you mentioned want to boot other RTOS (even itself)? What

> >> if you have no access to the source code for those RTOS(es) to

> >> patch MSP/PSP switch?

> >

> > My case is a little complicated.

> > stm32h7 only have 128Kbytes internal flash, can't store u-boot.bin (>200K),

> > so, set a bootloader (rt-thread rtos) to internal flash, load

> > linux/u-boot from serial port via ymodem

> > store to qspi flash(8Mbytes), then jump to u-boot.

> >

> > qspi flash layout:

> > 0 - 512K:    u-boot

> > 512K- 8M : kernel(xip)

> >

> > load process : rt-thread -> u-boot -> linux

> >

> > before add psp/msp check after svc call, register restore corrupt.

> > add a printhex8 around svc call, found the sp stack is 0x24040000c0ffcff8

> > it should be 0xc0ffcdf8c0ffcff8. 0x24040000 is the sp stack address

> > assigned by u-boot

> > i've no idea how it's become to u-boot's sp.

> >

> > I have the rtos code, and will try to fix it on the rtos side.

>

> That would be great!

>

> >

> > Can you give more explanation about why linux relies on MSP ? thanks

>

> MSP is what set from boot, thus it is natural assumption that boot code

> would preserve that illusion.

>

> I'd guess that kernel is in line in such assumption across different

> (RT)OS capable to run on M-class cores (please, note that some variants

> might not have two stack pointers)

>

Okay， got it. after adding msp/psp switch code in RTOS, now the kernel
can be loaded normally
without any modification.

So, just drop the changes in proc-v7m.S.

Thanks.
> Cheers

> Vladimir

>

> >

> >>

> >> I'd very much prefer to keep stack switching logic outside kernel,

> >> say, in some shim which RTOS/bootloader can maintain.

> >>

> >> Cheers

> >> Vladimir

> >>

> >>>

> >>> So, we need add an lr check after svc call, to find out should

> >>> use psp or msp. else register restore after svc call might be

> >>> corrupted.

> >>>

> >>> Fixes: b70cd406d7fe ("ARM: 8671/1: V7M: Preserve registers across switch from Thread to Handler mode")

> >>> Signed-off-by: dillon min <dillon.minfei@gmail.com>

> >>> ---

> >>>  arch/arm/mm/proc-v7m.S | 5 ++++-

> >>>  1 file changed, 4 insertions(+), 1 deletion(-)

> >>>

> >>> diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S

> >>> index 84459c1d31b8..c93d2757312d 100644

> >>> --- a/arch/arm/mm/proc-v7m.S

> >>> +++ b/arch/arm/mm/proc-v7m.S

> >>> @@ -137,7 +137,10 @@ __v7m_setup_cont:

> >>>  1:   cpsid   i

> >>>       /* Calculate exc_ret */

> >>>       orr     r10, lr, #EXC_RET_THREADMODE_PROCESSSTACK

> >>> -     ldmia   sp, {r0-r3, r12}

> >>> +     tst     lr, #EXC_RET_STACK_MASK

> >>> +     mrsne   r4, psp

> >>> +     moveq   r4, sp

> >>> +     ldmia   r4!, {r0-r3, r12}

> >>>       str     r5, [r12, #11 * 4]      @ restore the original SVC vector entry

> >>>       mov     lr, r6                  @ restore LR

> >>>

> >>>

> >>

> >

>
Vladimir Murzin March 4, 2021, 9:02 a.m. UTC | #5 
On 3/4/21 5:42 AM, dillon min wrote:
> Okay， got it. after adding msp/psp switch code in RTOS, now the kernel

> can be loaded normally

> without any modification.


Yay!

> 

> So, just drop the changes in proc-v7m.S.


Glad to see they are not strictly necessary :)

Thanks
Vladimir
diff mbox series

Patch

diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
index 84459c1d31b8..c93d2757312d 100644
--- a/arch/arm/mm/proc-v7m.S
+++ b/arch/arm/mm/proc-v7m.S
@@ -137,7 +137,10 @@  __v7m_setup_cont:
 1:	cpsid	i
 	/* Calculate exc_ret */
 	orr	r10, lr, #EXC_RET_THREADMODE_PROCESSSTACK
-	ldmia	sp, {r0-r3, r12}
+	tst	lr, #EXC_RET_STACK_MASK
+	mrsne	r4, psp
+	moveq	r4, sp
+	ldmia	r4!, {r0-r3, r12}
 	str	r5, [r12, #11 * 4]	@ restore the original SVC vector entry
 	mov	lr, r6			@ restore LR