riscv: pmp: Add support for custom user-defined PMP entry#96241
riscv: pmp: Add support for custom user-defined PMP entry#96241nashif merged 7 commits intozephyrproject-rtos:mainfrom
Conversation
be6bbb4 to
edfc116
Compare
|
Hi, thanks for the PR! I see that we're limiting the user to just one custom protected memory region. Is this due to limitations imposed by Kconfig? Could we use devicetree for defining those regions instead? That would allow the user to specify more than one protected memory region. |
I am just defining only a new one because this is something that our application requires. users of the entry can get the config values from the device tree. Do you have any specific suggestion of how to generalize extracting things from a device tree early in the abstraction phase? |
We could start using |
61fc494 to
7219bdd
Compare
ok, I implemented it using |
4e71970 to
7bf83a8
Compare
jimmyzhe
left a comment
There was a problem hiding this comment.
Besides these comments, I tested these changes and LGTM. Thanks.
|
Please someone needs to explain why device-tree derrived PMP entries
need to be installed per thread. This makes no sense to me.
Those are global attributes and therefore should be part of the global
PMP set established once in `z_riscv_pmp_init()` and never touched
afterwards. Unless I'm missing something, those aren't per-thread
attributes.
|
This change is based on my #96241 (comment). I noticed some issues when set device-tree derrived PMP as global entry. For example, when I updated qemu_riscv32 in this case: In both cases set the RAM region with R, W permissions and overlap Zephyr program's stack, .data .bss ... . In these cases, set device-tree derrived PMP as global entry make stack guard and userspace fail because:
Based on these cases, I think device-tree derrived PMP regions should be per-thread entry. |
|
jimmyzhe left a comment (zephyrproject-rtos/zephyr#96241)
In both cases set the RAM region with R, W permissions and overlap Zephyr program's stack, .data .bss ... .
These configurations may look redundant, but I think they are still valid use cases.
Why would they be valid use cases?
|
npitre
left a comment
There was a problem hiding this comment.
OK, I see the problem with user threads. I agree that they shouldn't be
granted access to custom PMP regions.
The best solution to this, I think, would be to have 2 global_pmp_end_index
variables: one for user mode and one for kernel mode (say
global_pmp_u_end_index and global_pmp_m_end_index), the former being a
subset of the later. And when there are no custom PMP entries they would
be equivalent.
This way the global PMP entries would always contain the custom ones
but they would be ignored when a user thread is prepared as simply
as this in z_riscv_pmp_usermode_prepare():
z_riscv_pmp_thread_init(global_pmp_u_end_index, PMP_U_MODE(thread));
Yeah, thanks for pointing this out. After thinking about it again, this is really an unusual use case. In normal cases, application don't need to set attribute for the program’s RAM because program’s RAM is usually on the main memory region rather than in the I/O region in RISC-V (Main Memory versus I/O Regions). If an application wants to reconfigure part of memory to I/O region with specific permissions, it should split that region instead of overlapping it, something like: So, a normal use case should not makes device-tree derrived PMP entries overlap thread stack guard entry.
Right, this approach is better. Set these entries as global entries for kernel mode helps reduce context switch latency. Thanks for the approach. |
|
@npitre I responded to all of your comment. The PR has been restructured. Your review is appreciated. |
|
@jimmyzhe Your concerns has been addresses. Your review is appreciated. |
|
@fkokosinski I responded to all reviewers feedback. Your review is appreciated. |
Rename the `z_riscv_pmp_stackguard_*` functions to `z_riscv_pmp_kernelmode_*`. This change better reflects that these functions are used for general kernel mode PMP configuration, not strictly limited to stack guard purposes. Call sites in fatal.c, isr.S, and switch.S have been updated accordingly. Signed-off-by: Firas Sammoura <[email protected]>
Introduce `CONFIG_PMP_KERNEL_MODE_DYNAMIC` to enable dynamic configuration and activation of Machine mode PMP entries. This allows PMP settings to be managed efficiently during transitions between kernel and thread contexts. Signed-off-by: Firas Sammoura <[email protected]>
npitre
left a comment
There was a problem hiding this comment.
Some comments:
The commit log says: "the pmp_cfg array in z_riscv_pmp_init() is
initialized to zero to prevent writing uninitialized stack data to unused
PMP entries." This is unnecessary as that's what the third argument to
write_pmp_entries() is already meant for.
You do:
- pmp_addr[global_pmp_end_index - 1] = global_pmp_last_addr;
+ pmp_addr[pmp_end_index - 1] = (pmp_end_index == global_pmp_m_end_index)
+ ? global_pmp_m_last_addr
+ : global_pmp_u_last_addr;
This is getting somewhat ugly. Suggestion:
enum {
M_MODE = 0,
#if defined(CONFIG_USERSPACE)
U_MODE,
#endif
MODE_TOTAL
} pmp_mode;
[...]
static unsigned long global_pmp_last_addr[MODE_TOTAL];
static unsigned int global_pmp_end_index[MODE_TOTAL];
[...]
static inline unsigned int z_riscv_pmp_thread_init(enum pmp_mode mode,
unsigned long *pmp_addr,
unsigned long *pmp_cfg,
unsigned int index_limit)
{
unsigned int pmp_end_index = global_pmp_end_index[mode];
[...]
pmp_addr[pmp_end_index - 1] = global_pmp_last_addr[mode];
return pmp_end_index;
}
[...]
void z_riscv_pmp_kernelmode_prepare(struct k_thread *thread)
{
unsigned int index = z_riscv_pmp_thread_init(M_MODE, PMP_M_MODE(thread));
[...]
In z_riscv_pmp_thread_init() you have:
#if defined(CONFIG_MEM_ATTR) && defined(CONFIG_USERSPACE)
/*
* Retrieve the memory attribute region's pmpaddr entries to handle the
* switch back from user mode.
*/
memcpy(&pmp_addr[global_pmp_u_end_index], mem_attr_pmp_addr,
(global_pmp_m_end_index - global_pmp_u_end_index) * PMPCFG_STRIDE);
#endif
This should be done only for mode == M_MODE.
In arch/riscv/core/fatal.c you do:
-#ifdef CONFIG_PMP_STACK_GUARD
+#if defined(CONFIG_PMP_STACK_GUARD) && defined(CONFIG_MULTITHREADING)
What's the reason for this?
In z_riscv_pmp_kernelmode_prepare():
+#if defined(CONFIG_PMP_STACK_GUARD) && defined(CONFIG_MULTITHREADING)
Can't we protect against overflow even when multithreading is disabled?
Split global PMP state variables (index and last address) into mode-specific counterparts to correctly track the end of global PMP ranges for both M-mode (kernel) and U-mode (userspace). This ensures correct per-thread PMP initialization when configuring mode-specific dynamic PMP entries. Signed-off-by: Firas Sammoura <[email protected]>
When CONFIG_SMP is enabled, per-CPU IRQ stack guards are added. To prevent unintended TOR (Top of Range) entry sharing, the PMP address entry preceding each guard region in `pmp_addr` is marked with -1L. The previously used index to access `pmp_addr` could become stale, as additional PMP entries may be allocated after its initial calculation but before the SMP loop for IRQ guards. Signed-off-by: Firas Sammoura <[email protected]>
…utes The Physical Memory Protection (PMP) initialization is updated to support custom entries defined in the Device Tree (DT) using the `zephyr,memattr` property, contingent on `CONFIG_MEM_ATTR` being enabled. A new function, `set_pmp_mem_attr()`, iterates over DT-defined regions and programs PMP entries in `z_riscv_pmp_init()`, allowing for early, flexible, and hardware-specific R/W/X protection for critical memory areas. DT-based entries are also installed in `z_riscv_pmp_kernelmode_prepare()` for thread-specific configuration. The logic for the temporary PMP "catch-all" entry is adjusted to account for new DT entries. Furthermore, the PMP domain resync logic now masks user partition permissions against DT-defined region permissions, preventing privilege escalation. `CONFIG_RISCV_PMP` is updated to select `PMP_KERNEL_MODE_DYNAMIC` if `MEM_ATTR`. Finally, the `pmp_cfg` array in `z_riscv_pmp_init()` is initialized to zero to prevent writing uninitialized stack data to unused PMP entries. Signed-off-by: Firas Sammoura <[email protected]>
The logic to decode PMP addressing modes (**TOR**, **NA4**, **NAPOT**) into physical start and end addresses was previously embedded in `print_pmp_entries()`. Extract this calculation into a new static helper function, `pmp_decode_region()`, to significantly improve the readability and modularity of the PMP debug printing code. The new helper function is fully self-contained and exposes a defined API for the PMP address decoding logic. This enables **direct reuse** in **unit tests** (e.g., using **Ztest**) to verify the core address calculation accuracy for all PMP modes and boundary conditions, independent of the main PMP initialization or logging path. Signed-off-by: Firas Sammoura <[email protected]>
…state This commit implements a new unit test suite to validate the integration of Device Tree memory attributes (`zephyr,memory-attr`) with the RISC-V Physical Memory Protection (PMP) hardware. The test suite includes: 1. **`test_pmp_devicetree_memattr_config`**: Verifies that the PMP Control and Status Registers (CSRs) are programmed correctly based on the memory regions defined with `zephyr,memory-attr` in the Device Tree. It iterates through the active PMP entries and asserts a match against the expected DT-defined regions. 2. **`test_riscv_mprv_mpp_config`**: Checks the initial state of the Machine Privilege Register Virtualization (MPRV) bit and Machine Previous Privilege (MPP) field in the `mstatus` CSR to ensure PMP is configured for correct privilege level switching during boot. 3. **`test_dt_pmp_perm_conversion`**: Validates the `DT_MEM_RISCV_TO_PMP_PERM` macro to ensure the conversion from Device Tree memory attribute flags to RISC-V PMP permission bits (R/W/X) is correct. Signed-off-by: Firas Sammoura <[email protected]>
|
|
Thanks Nicolas for the feedback. Here is my response:
[FS] I believe this question was discussed in this comment with @jimmyzhe
[FS] Thanks for the suggestion! I agree that using an enum cleans up the code significantly. I've implemented this by creating the pmp_mode enum and leveraging it to unify the handling of M-mode and U-mode PMP entries, just as you outlined. The new structure is: A modified PR has been uploaded with these changes.
[FS] Acknowledged. This has been implemented for M-mode.
[FS] This code block is intended to remove the per-thread stack guard PMP entry following a thread stack overflow event, a requirement tracked in a related issue (e.g., #75960). This logic is only relevant when
[FS] Per-thread stack guards are only required in a multithreaded context. If |
|
when this PR will merge? |
|
@jimmyzhe when this PR will merge? |
|
@fkokosinski |



This series significantly upgrades the RISC-V Physical Memory Protection (PMP) implementation by introducing dynamic configuration and Device Tree integration.
Key changes:
zephyr,memattrDevice Tree property. The PMP domain resync logic is updated to respect these DT-defined regions.CONFIG_PMP_KERNEL_MODE_DYNAMICto allow dynamic configuration and activation of Machine mode PMP entries for better context switching support.M-mode(kernel) andU-mode(userspace). This ensures correct per-thread PMP initialization and accurate tracking of global PMP ranges across privilege levels.SMPconfiguration to properly manage per-CPU IRQ stack guards.stackguardtokernelmode) to reflect their general kernel configuration role.