You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/20 07:37:54 UTC
[GitHub] [incubator-nuttx] minabeoki opened a new pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
minabeoki opened a new pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558
## Summary
* armv7-a has banked registers for each cpu mode.
* NuttX uses SVC mode and USR mode when BUILD_KERNEL=y.
* The exception handler in armv7-a/arm_vectors.S need to maintain both SVC's sp and USR's sp.
* The current implementaion break SVC's sp.
This patch is:
* Correct SVC's sp by following line
```
add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
```
* Do not change sp(=r13) after the above line, so sp is not used after.
* And, in the arm_vectordata, correct ldmia return.
armv7-r is the almost same.
## Impact
armv7-a: BUILD_KERNEL=y
armv7-r: BUILD_PROTECTED=y
## Testing
run programs on custom Cortex-A9 board w/ BUILD_KERNEL=y
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r824582511
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
@minabeoki @anchao could you please take a look if my question is valid?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825695727
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
emm... conclusion is wrong. It seems that the correct banked register cannot be obtained when the PC is in reglist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825716250
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
That is very strange because few lines below we have `ldmia sp, {r0-r15}^` that seems to be working perfectly. So are you saying that `ldmia` has different behavior depending is `sp` or `r14` used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] minabeoki commented on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
minabeoki commented on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046206475
@xiaoxiang781216
> Is this because the old arm doesn't support KERNEL mode?
Yes.
> If so, I would suggest that we make the similar change to the old arm to simplify the potential improvement in the future for the old arm.
OK, I add this by PR #5562
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r824257401
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
Why we can't just `ldmia r14, {r13, r14, r15}^` here instead of
```
ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
add r14, r14, #(4*2) /* (SVC) r14=address of r15 storage */
ldmia r14, {r15}^ /* Return */
```
I see that PC was stored right after LR
```
sub r3, lr, #4
...
add r0, sp, #(4*REG_SP) /* Offset to sp/lr storage */
stmia r0, {r13, r14}^ /* Save user mode r13(sp) and r14(lr) */
add r0, sp, #(4*REG_R15) /* Offset to pc/cpsr storage */
stmia r0, {r3, r4} /* Save r15(pc), and the CPSR */
```
Or I'm missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046225333
@minabeoki
We would appreciate it if you could add the summary/impact/testing to the commit messages next time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046185753
@minabeoki do we need modify old arm too?
https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/arm/arm_vectors.S
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825670700
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
Anyway this code is already remove by your latest PR. So this is not relevant anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046190260
> @xiaoxiang781216 This problem is only when using both SVC and USR modes. In case of old arm's arm_vectors.S, only SVC mode is used. So, we do not need to modify.
Is this because the old arm doesn't support KERNEL mode? If so, I would suggest that we make the similar change to the old arm to simplify the potential improvement in the future for the old arm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825669289
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
I think here is an answer:
https://developer.arm.com/documentation/dui0588/b/ARM-and-Thumb-Instructions/LDM-and-STM
```
^
is an optional suffix, available in ARM state only. You must not use it in User mode or System mode. It has the following purposes:
If the instruction is LDM (with any addressing mode) and reglist contains the PC (R15), in addition to the normal multiple register transfer, the SPSR is copied into the CPSR. This is for returning from exception handlers. Use this only from exception modes.
Otherwise, data is transferred into or out of the User mode registers instead of the current mode registers.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825670700
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
Anyway this code is already remove by your latest PR. So this is not relevant anymore.
But I still do not understand why `ldmia r14, {r13, r14, r15}^` can't be used in such situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046190260
> @xiaoxiang781216 This problem is only when using both SVC and USR modes. In case of old arm's arm_vectors.S, only SVC mode is used. So, we do not need to modify.
Is this because the old arm doesn't support KERNEL mode? If so, I would suggest that we make the similar change to the old arm to simplify the potential improvement for the old arm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] minabeoki commented on pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
minabeoki commented on pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#issuecomment-1046188059
@xiaoxiang781216
This problem is only when using both SVC and USR modes.
In case of old arm's arm_vectors.S, only SVC mode is used.
So, we do not need to modify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5558: armv7-a/r: fix SVC's sp restore in arm_vectors.S
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5558:
URL: https://github.com/apache/incubator-nuttx/pull/5558#discussion_r825687031
##########
File path: arch/arm/src/armv7-a/arm_vectors.S
##########
@@ -239,15 +239,16 @@ arm_vectorirq:
cmp r2, #PSR_MODE_USR /* User mode? */
bne .Lirqleavesvc /* Branch if not user mode */
+ add sp, sp, #XCPTCONTEXT_SIZE /* Restore SVC's sp */
+
/* ldmia with ^ will return the user mode registers (provided that r15
* is not in the register list).
*/
- mov r13, r0 /* (SVC) R13=Register storage area */
- ldmia r13, {r0-r12} /* Restore common R0-R12 */
- add r14, r13, #(4*REG_R13) /* (SVC) R14=address of R13/R14 storage */
- ldmia r14, {r13, r14}^ /* Restore user mode R13/R14 */
- add r14, r13, #(4*REG_R15) /* (SVC) R14=address of R15 storage */
+ mov r14, r0 /* (SVC) r14=Register storage area */
+ ldmia r14!, {r0-r12} /* Restore common r0-r12 */
+ ldmia r14, {r13, r14}^ /* Restore user mode r13/r14 */
Review comment:
https://developer.arm.com/documentation/dui0497/a/BABCAEDD
`The accesses happens in order of increasing register numbers, with the lowest numbered register using the lowest memory address and the highest number register using the highest memory address. `
`ldmia r14, {r13, r14, r15}^`
If this syntax is used, R15 will be pushed first, which will immediately change the PC, and R13 and R14 will not be updated:
```
[ 2.380000] [ 4] [ ALERT] SYSCALL Entry: regs: 0x80202708 cmd: 4
[ 2.380000] [ 4] [ ALERT] R0: 00000004 80001939 00000001 80202018 00000000 00000000 00000000 802027d0
[ 2.380000] [ 4] [ ALERT] R8: 00000000 00000000 00000000 00000000 00000000 802027d0 1080f738 1080f738
[ 2.380000] [ 4] [ ALERT] CPSR: 00000073
[ 2.380000] [ 4] [ ALERT] SYSCALL Exit: regs: 0x80202708
[ 2.380000] [ 4] [ ALERT] R0: 1 80202018 1 80202018 0 0 0 802027d0
[ 2.380000] [ 4] [ ALERT] R8: 0 0 0 0 0 802027d0 1080f738 80001939
[ 2.380000] [ 4] [ ALERT] CPSR: 00000070
[ 2.390000] [ 4] [ EMERG] arm_dataabort: Data abort. PC: 80001938 DFAR: fffffff8 DFSR: 00000805
[ 2.390000] [ 4] [ EMERG] up_assert: Assertion failed at file:armv7-a/arm_dataabort.c line: 161 task: /mnt/nfs/bin/init
[ 2.390000] [ 4] [ EMERG] arm_registerdump: R0: 00000001 R1: 80202018 R2: 00000001 R3: 80202018
[ 2.390000] [ 4] [ EMERG] arm_registerdump: R4: 00000000 R5: 00000000 R6: 00000000 FP: 802027d0
[ 2.390000] [ 4] [ EMERG] arm_registerdump: R8: 00000000 SB: 00000000 SL: 00000000 R11: 00000000
[ 2.390000] [ 4] [ EMERG] arm_registerdump: IP: 00000000 SP: 00000000 LR: 00000000 PC: 80001938
```
SP/LR is NULL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org