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/07/12 13:46:19 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request, #6603: ESP32: Add support for Protected Mode
gustavonihei opened a new pull request, #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603
## Summary
This PR intends to add Protected Mode support for **ESP32** by relying on the **PID Controller** peripheral for implementing isolation between Kernel and Userspace.
By working together with the MMU and Static MPUs of the ESP32, the PID Controller is able to restrict the application access to peripherals, on-chip memories (Internal ROM and Internal SRAM) and off-chip memories (External Flash and PSRAM).
### Note
- The PID Controller driver is in **EXPERIMENTAL** state, so please consider the Protected Mode feature for ESP32 a **Proof-of-Concept**.
- The PID Controller **does not** prevent the application from accessing CPU System Registers.
## Impact
ESP32 only, but changes should not affect existing targets running under Flat mode.
## Testing
`esp32-devkitc:knsh`, successful execution of `ostest` 25 times in a row.
```bash
./tools/configure.sh -E esp32-devkitc:knsh
make bootloader
make download ESPTOOL_BINDIR=. ESPTOOL_PORT=<PORT> -j 6
```
## Note
`make bootloader` is **required** for building a patched IDFboot bootloader from source.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919406362
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> low/raise cpu/dsp priority is normally done by core self
How about prefixing it with `up_` and declaring it on `include/nuttx/arch.h`?
Then, at architectural level, there could exist weak implementation of those functions which, if needed, could be overriden by chip-specific ones (the case for Espressif Xtensa-based chips).
The RISC-V implementation manipulates the `STATUS_PPP` flag and could also benefit from this abstraction.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r920059974
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
@xiaoxiang781216 I've renamed the `mpu_` prefix to `xtensa_`, PTAL.
Also, I've added some information about the `knsh` defconfig into the Documentation page.
--
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 #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603
--
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 #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919596291
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> > low/raise cpu/dsp priority is normally done by core self
>
> How about prefixing it with `up_` and declaring it on `include/nuttx/arch.h`?
but @patacongo prefer arch specific interface use arm_, riscv_ or xtensa_ prefix, and the common interface shared by all arch use up_ prefix.
> Then, at architectural level, there could exist weak implementation of those functions which, if needed, could be overriden by chip-specific ones (the case for Espressif chips).
Weak symbol isn't work very well with the static library sometime:
https://stackoverflow.com/questions/13089166/how-to-make-gcc-link-strong-symbol-in-static-library-to-overwrite-weak-symbol
> The RISC-V implementation manipulates the `STATUS_PPP` flag and could also benefit from this abstraction.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919406362
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> low/raise cpu/dsp priority is normally done by core self
How about prefixing it with `up_` and declaring it on `include/nuttx/arch.h`?
Then, at architectural level, there could exist weak implementation of those functions which, if needed, could be overriden by chip-specific implementation (the case for Espressif Xtensa-based chips).
The RISC-V implementation manipulates the `STATUS_PPP` flag and could also benefit from this abstraction.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919406362
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> low/raise cpu/dsp priority is normally done by core self
How about prefixing it with `up_` and declaring it on `include/nuttx/arch.h`?
Then, at architectural level, there could exist weak implementation of those functions which, if needed, could be overriden by chip-specific ones (the case for Espressif chips).
The RISC-V implementation manipulates the `STATUS_PPP` flag and could also benefit from this abstraction.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919283352
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
I preferred to not prefix it with `xtensa_` to avoid confusion and also to prevent any kind of clash with the Xtensa MPU block, which ESP32 does not embed.
Newer Xtensa-based Espressif chips (ESP32-S2 and ESP32-S3) contain the PMS peripheral, which also performs this hardware and software resource management.
--
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 #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919278152
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
xtensa_? MPU is normally used to change the memory region protection attribute, low/raise cpu/dsp priority is normally done by core self.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919283352
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
I preferred to not prefix it with `xtensa_` to avoid confusion and also to prevent any kind of clash with the Xtensa MPU block, which ESP32 does not embed.
Newer Xtensa-based Espressif chips (ESP32-S2 and ESP32-S3) contain the PMS peripheral, which also performs this hardware and software resource management in a similar manner.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919406362
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> low/raise cpu/dsp priority is normally done by core self
How about prefixing it with `up_` and declaring it on `include/nuttx/arch.h`?
Then, at architectural level, there could exist a weak implementation of those functions which, if needed, could be overriden by chip-specific implementation (the case for Espressif Xtensa-based chips).
The RISC-V implementation manipulates the `STATUS_PPP` flag and could also benefit from this abstraction.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919212361
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
What would be your suggestion? I had spent some time thinking, but prefixing it with `mpu_` was the one I considered most suitable.
--
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 #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r919108957
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
should we use a general prefix other than mpu? I am afraid that other chip may implement the protection by the different hardware module.
--
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] gustavonihei commented on a diff in pull request #6603: ESP32: Add support for Protected Mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6603:
URL: https://github.com/apache/incubator-nuttx/pull/6603#discussion_r920031930
##########
arch/xtensa/src/common/xtensa_schedsigaction.c:
##########
@@ -155,6 +156,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
(PS_INTLEVEL(XCHAL_EXCM_LEVEL) | PS_UM |
PS_WOE | PS_CALLINC(1));
#endif
+#ifndef CONFIG_BUILD_FLAT
+ mpu_raiseprivilege(CURRENT_REGS);
Review Comment:
> but @patacongo prefer arch specific interface use arm_, riscv_ or xtensa_ prefix, and the common interface shared by all arch use up_ prefix.
I agree, my suggestion was bad, after all, those symbols are only used within the architecture implementation so they shouldn't be present on the interface `arch.h`.
> Weak symbol isn't work very well with the static library sometime:
https://stackoverflow.com/questions/13089166/how-to-make-gcc-link-strong-symbol-in-static-library-to-overwrite-weak-symbol
This shouldn't be a problem to NuttX build system, since all the libraries are passed to the linker within a group (`--start-group`/`--end-group`).
--
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