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 2021/02/01 08:45:03 UTC
[GitHub] [incubator-nuttx] tommythorn opened a new issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
tommythorn opened a new issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786
I was just browsing the NuttX code when I saw
```
+ /* Issue the ECALL opcode to perform a SW interrupt to the OS */
+
+ ecall
+
+ /* The actual interrupt may not a occur for a few more cycles. Let's
+ * put a few nop's here in hope that the SW interrupt occurs during
+ * the sequence of nops.
+ */
+
+ nop
+ nop
```
First of all, as the "E" implies, `ECALL` isn't a software interrupt, but an exception and thus it's synchronous.
The comment is wrong and the nops are pointless.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-771033940
I think this is another case of the MIPS port heritage.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-771380651
If I remeber corrrectly, RV64 already implement ECALL and the separation of userspace and kernel. Actually, RV32 and RV64 has common but diverse feature set. It's better to merge two arch into one like https://github.com/apache/incubator-nuttx/issues/2593 then:
1. 32bit and 64bit has the same and union feature set
2. New feature don't need duplicate on two arch 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] tommythorn commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
tommythorn commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-770686858
This is my proposed but untested fix:
```
diff --git a/arch/risc-v/src/rv32im/riscv_syscall.S b/arch/risc-v/src/rv32im/riscv_syscall.S
index 5545fa7ffa..b3cc9c2ad5 100644
--- a/arch/risc-v/src/rv32im/riscv_syscall.S
+++ b/arch/risc-v/src/rv32im/riscv_syscall.S
@@ -79,9 +79,6 @@
* up_syscall4 - System call SYS_ argument and four additional parameters.
* up_syscall5 - System call SYS_ argument and five additional parameters.
*
- * Assumption:
- * All interrupts are disabled except for the software interrupts.
- *
****************************************************************************/
.text
@@ -93,19 +90,10 @@ sys_call3: /* a0 holds the syscall number, arguments in a1, a2, and a3 */
sys_call4: /* a0 holds the syscall number, arguments in a1, a2, a3 and a4 */
sys_call5: /* a0 holds the syscall number, arguments in a1, a2, a3, a4 and a5 */
- /* Issue the ECALL opcode to perform a SW interrupt to the OS */
+ /* Issue the ECALL opcode to perform a trap to the OS */
ecall
- /* The actual interrupt may not a occur for a few more cycles. Let's
- * put a few nop's here in hope that the SW interrupt occurs during
- * the sequence of nops.
- */
-
- nop
- nop
-
- /* Then return with the result of the software interrupt in v0 */
+ /* Then return with the result of the syscall in v0 */
ret
- nop
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-771380651
If I remeber corrrectly, RV64 already implement ECALL and the separation of userspace and kernel. Actually, RV32 and RV64 has common but diverse feature set. It's better to merge two arch into one like https://github.com/apache/incubator-nuttx/issues/2593.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-770698076
I believe that you are correct from the ISA
> ECALL and EBREAK cause the receiving privilege mode’s epc register to be set to the address of the ECALL or EBREAK instruction itself, not the address of the following instruction. As ECALL and EBREAK cause synchronous exceptions, they are not considered to retire, and should not increment the minstret CSR.
I also looked and this also aligns with my assembly I wrote for testing ECALL on a custom RISCV core.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-771380651
If I remeber corrrectly, RV64 already implement ECALL and the separation of userspace and kernel. Actually, RV32 and RV64 has common but diverse feature set. It's better to merge two arch into one like https://github.com/apache/incubator-nuttx/issues/2593 then:
1. 32bit and 64bit has the same and union feature set
2. New feature don't need duplicate on two arch 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
btashton commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-770698076
I believe that you are correct from the ISA
```
ECALL and EBREAK cause the receiving privilege mode’s epc register to be set to the address of the ECALL or EBREAK instruction itself, not the address of the following instruction. As ECALL and EBREAK cause synchronous exceptions, they are not considered to retire, and should not increment the minstret CSR.
```
I also looked and this also aligns with my assembly I wrote for testing a custom RISCV core.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-771380651
If I remeber corrrectly, RV64 already implement ECALL and the separation of userspace and kernel. Actually, RV32 and RV64 has common but diverse feature set. It's better to merge two arch into one like https://github.com/apache/incubator-nuttx/issues/2593.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] tommythorn commented on issue #2786: arch/risc-v/src/rv32im/riscv_syscall.S have bogus nops
Posted by GitBox <gi...@apache.org>.
tommythorn commented on issue #2786:
URL: https://github.com/apache/incubator-nuttx/issues/2786#issuecomment-770701488
FWIW, it looks like all the sys_callX functions could be inlined, that is replaced with `asm("ecall")`, but cycles saved might be too trivial to matter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org