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/27 12:53:19 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request, #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
gustavonihei opened a new pull request, #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162
## Summary
This PR intends to fix the usage of PMP driver for RISC-V chips that do not implement page-based virtual memory.
If page-based virtual memory is not implemented, memory accesses check the PMP settings synchronously, so no `SFENCE.VMA` is needed.
Reference:
**3.7.2 Physical Memory Protection and Paging** section from working draft of RISC-V Privileged Spec:
https://github.com/riscv/riscv-isa-manual/releases/download/draft-20220427-db7a4a0/riscv-privileged.pdf
https://github.com/riscv/riscv-isa-manual/blob/master/src/machine.tex#L3788-L3789
## Impact
Enable the PMP driver for RISC-V chips that do not implement page-based virtual memory (e.g. Espressif ESP32-C3).
## Testing
`esp32c3-devkit`
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859798924
##########
arch/risc-v/src/common/riscv_internal.h:
##########
@@ -119,16 +119,16 @@
#define READ_CSR(reg) \
({ \
- uintptr_t reg##_val; \
Review Comment:
I think this should be fine, it just performs a string append, so e.g.
`READ_CSR(mhartid); `
becomes
`uintptr_t mhartdid_val;` 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] gustavonihei commented on a diff in pull request #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859832219
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
> `CONFIG_ARCH_HAVE_S_MODE` was originally intended to indicate software capability, not chip capability. But maybe I could add ARCH_RV_ISA_S like the other chip capabilities are listed for other chips and do `CONFIG_ARCH_HAVE_S_MODE` via another config parameter which depends ARCH_RV_ISA_S. You don't have to wait for this I can do the refactoring.
So, for this PR, I'll just change `CONFIG_ARCH_HAVE_S_MODE` to `CONFIG_ARCH_USE_S_MODE`, which is already used in other places and will probably also change in a future refactor.
I think this little change makes things a little more correct.
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859828302
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
> Volume II: RISC-V Privileged Architectures V1.10 table Table 3.2: Encoding of Extensions field in misa: This table conveniently lists how the capabilities are encoded.
Wow, thanks for pointing this, I wasn't aware of this table. I only considered the table from the **Preface** of the Volume I.
So there is indeed an S module for representing "Supervisor mode implemented".
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859825192
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
I just noticed that the `CONFIG_ARCH_USE_S_MODE` may be more suitable for this purpose.
What do you think about it?
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859805563
##########
arch/risc-v/src/common/riscv_internal.h:
##########
@@ -119,16 +119,16 @@
#define READ_CSR(reg) \
({ \
- uintptr_t reg##_val; \
Review Comment:
Sorry, that's true. I may had faced an unrelated build error and ended-up misreading this part before.
Reverted that change.
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859828302
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
> Volume II: RISC-V Privileged Architectures V1.10 table Table 3.2: Encoding of Extensions field in misa: This table conveniently lists how the capabilities are encoded.
Wow, thanks for pointing this, I wasn't aware of this table. I only considered the table from the **Preface** of the Volume I
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859810549
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
Requirement for sfence.vma here is based on chip capabilities right ? Not utilization of address translations / MMU ?
**TL;DR I think this change is fine as-is.**
A longer read:
`CONFIG_ARCH_HAVE_S_MODE` was originally intended to indicate software capability, not chip capability. But maybe I could add ARCH_RV_ISA_S like the other chip capabilities are listed for other chips and do `CONFIG_ARCH_HAVE_S_MODE` via another config parameter which depends ARCH_RV_ISA_S. You don't have to wait for this I can do the refactoring.
Volume II: RISC-V Privileged Architectures V1.10 table Table 3.2: Encoding of Extensions field in misa: This table conveniently lists how the capabilities are encoded.
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859834307
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
Yes you are absolutely correct that the user level ISA does not mention S as it is part of privileged mode ISA. Table 3.2 lists user + privileged mode encodings both.
Using `CONFIG_ARCH_HAVE_S_MODE` here is totally fine, even though the flag does not really indicate chip capabilities right now (I assume mpfs is not the only risc-v target that has S-mode chip level support).
Further reading of the privileged spec. would actually indicate that when address translations are not used, PMP does not need the sfence.vma flush either (which makes sense).
_Clarified that bare S-mode need not support the SFENCE.VMA instruction._
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162
--
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 #6162: [RISC-V] PMP: Restrict Fence instruction for chips that support S-mode
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6162:
URL: https://github.com/apache/incubator-nuttx/pull/6162#discussion_r859821671
##########
arch/risc-v/src/common/riscv_pmp.c:
##########
@@ -600,9 +600,14 @@ int riscv_config_pmp_region(uintptr_t region, uintptr_t attr,
# error "XLEN of risc-v not supported"
# endif
- /* fence is needed when page-based virtual memory is implemented */
+#ifdef CONFIG_ARCH_HAVE_S_MODE
Review Comment:
I feel that `ARCH_RV_ISA_S` may not be correct, since the RISC-V Spec does not define an **S** ISA extension module. Instead they are described as part of the `Supervisor-level ISA`, so I initially thought that using `CONFIG_ARCH_HAVE_S_MODE` would better reflect the capabilities of a given RISC-V implementation.
--
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