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/04/19 07:55:35 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request, #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
no1wudi opened a new pull request, #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101
## Summary
Merge status/fpu context save/load to save_ctx/load_ctx to improve performance (no jal instruction).
## Impact
RISC-V
## Testing
QEMU
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852916448
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -84,6 +85,69 @@
REGSTORE x30, REG_X30(\in) /* t5 */
REGSTORE x31, REG_X31(\in) /* t6 */
+ csrr s0, CSR_STATUS
+ REGSTORE s0, REG_INT_CTX(\in) /* status */
+
+ addi s0, \in, XCPTCONTEXT_SIZE
+ REGSTORE s0, REG_X2(\in) /* original SP */
+
+ csrr s0, CSR_EPC
+ REGSTORE s0, REG_EPC(\in) /* exception PC */
+
+#ifdef CONFIG_ARCH_FPU
Review Comment:
This could be made into a separate macro, .macro riscv_savefpu in, etc
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852923425
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -132,6 +202,59 @@
REGLOAD x30, REG_X30(\out) /* t5 */
REGLOAD x31, REG_X31(\out) /* t6 */
+#ifdef CONFIG_ARCH_FPU
+ REGLOAD t0, REG_INT_CTX(\out)
Review Comment:
Having this inside the save/load fpu macro is fine as it only tampers with the saved context, not the status register directly
--
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 merged pull request #6101: arch/risc-v: Change riscv_savefpu/riscv_loadfpu to macro
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101
--
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 a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r853245880
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -83,20 +83,6 @@ exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
save_ctx sp
Review Comment:
could we define three macro: riscv_savectx, riscv_savecpu and riscv_savefpu?
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852919829
##########
arch/risc-v/src/common/supervisor/riscv_dispatch_syscall.S:
##########
@@ -91,11 +91,6 @@ riscv_dispatch_syscall:
addi s0, sp, XCPTCONTEXT_SIZE
Review Comment:
Here save_ctx makes a bit of extra work now due to how you have merged the status / epc handling into save_ctx, same with load_ctx
--
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] no1wudi commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
no1wudi commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r853038296
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -84,6 +85,69 @@
REGSTORE x30, REG_X30(\in) /* t5 */
REGSTORE x31, REG_X31(\in) /* t6 */
+ csrr s0, CSR_STATUS
Review Comment:
Yes, status register should be handling separate at least.
I'll update this PR later
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852915587
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -83,20 +83,6 @@ exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
save_ctx sp
- csrr s0, CSR_STATUS
- REGSTORE s0, REG_INT_CTX(sp) /* status */
-
- addi s0, sp, XCPTCONTEXT_SIZE
- REGSTORE s0, REG_X2(sp) /* original SP */
-
- csrr s0, CSR_EPC
- REGSTORE s0, REG_EPC(sp) /* exception PC */
-
-#ifdef CONFIG_ARCH_FPU
- mv a0, sp
- jal x1, riscv_savefpu /* save FPU context */
Review Comment:
Another option would be to define riscv_savefpu as a macro.
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852921912
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -83,20 +83,6 @@ exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
save_ctx sp
Review Comment:
Not really a part of this commit, but one option is to rename
`save_ctx -> riscv_saveintregs`
`load_ctx -> riscv_loadintregs`
Because that is what they do
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852918550
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -84,6 +85,69 @@
REGSTORE x30, REG_X30(\in) /* t5 */
REGSTORE x31, REG_X31(\in) /* t6 */
+ csrr s0, CSR_STATUS
Review Comment:
I would personally keep the status register and EPC handling separate.
Three things are needed:
- Integer register handling
- Status handling
- FPU handling
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r852916448
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -84,6 +85,69 @@
REGSTORE x30, REG_X30(\in) /* t5 */
REGSTORE x31, REG_X31(\in) /* t6 */
+ csrr s0, CSR_STATUS
+ REGSTORE s0, REG_INT_CTX(\in) /* status */
+
+ addi s0, \in, XCPTCONTEXT_SIZE
+ REGSTORE s0, REG_X2(\in) /* original SP */
+
+ csrr s0, CSR_EPC
+ REGSTORE s0, REG_EPC(\in) /* exception PC */
+
+#ifdef CONFIG_ARCH_FPU
Review Comment:
This could be made into a separate macro, `.macro riscv_savefpu in`, etc
--
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] no1wudi commented on a diff in pull request #6101: arch/risc-v: Change riscv_savefpu/riscv_loadfpu to macro
Posted by GitBox <gi...@apache.org>.
no1wudi commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r853769632
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -83,20 +83,6 @@ exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
save_ctx sp
- csrr s0, CSR_STATUS
- REGSTORE s0, REG_INT_CTX(sp) /* status */
-
- addi s0, sp, XCPTCONTEXT_SIZE
- REGSTORE s0, REG_X2(sp) /* original SP */
-
- csrr s0, CSR_EPC
- REGSTORE s0, REG_EPC(sp) /* exception PC */
-
-#ifdef CONFIG_ARCH_FPU
- mv a0, sp
- jal x1, riscv_savefpu /* save FPU context */
Review Comment:
Now riscv_savefpu/riscv_loadfpu defined as macro
--
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] pussuw commented on a diff in pull request #6101: arch/risc-v: Merge status/fpu context save/load to save_ctx/load_ctx
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6101:
URL: https://github.com/apache/incubator-nuttx/pull/6101#discussion_r853368030
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -83,20 +83,6 @@ exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
save_ctx sp
Review Comment:
It is also possible. However exception_common handler and riscv_dispatch_syscall need different implementations for the status register and epc. riscv_dispatch_syscall needs to emulate the conditions that are automatically set by trap entry otherwise (like MPIE, MPP).
--
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