riscv: pmp: enable stackguard without multithreading

[f0rget-the-sad]

Without multithreading only two stacks present: ISR and main. As any stack they also could overflow, so it make sense to add stack guard for them also.

Remove stack guard dependency on multithreading and mark Z_RISCV_STACK_GUARD_SIZE bytes at the beginning of stack as read-only region with PMP entry.

[cfriedt]

We need to have some guarantee that this code has test coverage.

addressed

[fkokosinski]

The code itself LGTM, but I agree with @cfriedt - we'd need to have some testing for this.

There're at least 3 places where we could add/modify a test:

1. tests/kernel/threads/no-multithreading - it's already enabled on RISC-V QEMU-based targets, but I'm not sure if this is the right place to add memory protection related tests. Probably not.
2. tests/kernel/mem_protect/mem_protect - we could add a RISC-V specific no multithreading testcase here
3. we could add a separate arch-specific test in e.g. tests/arch/riscv/pmp (ARM has one in zephyr/tests/arch/arm/arm_mem_protect).

It should probably be both 2 and 3.

[f0rget-the-sad]

The code itself LGTM, but I agree with @cfriedt - we'd need to have some testing for this.

There're at least 3 places where we could add/modify a test:

1. `tests/kernel/threads/no-multithreading` - it's already enabled on RISC-V QEMU-based targets, but I'm not sure if this is the right place to add memory protection related tests. Probably not.

2. `tests/kernel/mem_protect/mem_protect` - we could add a RISC-V specific no multithreading testcase here

3. we could add a separate arch-specific test in e.g. `tests/arch/riscv/pmp` (ARM has one in `zephyr/tests/arch/arm/arm_mem_protect`).

It should probably be both 2 and 3.

Thank you for pointing in right direction, I'll work on extending the test cases in next commit.

[fkokosinski]

Thanks a lot for adding the test

[fkokosinski]

... but if we want to go this route, then I'd rather use EXTRA_CFLAGS here and change VIA_TWISTER to something sounding less generic

[f0rget-the-sad]

Done.

[ycsin]

could have used Kconfig instead? i.e. CONFIG_TEST_ISR_STACK_GUARD / CONFIG_TEST_MAIN_STACK_GUARD, and enable it here

[f0rget-the-sad]

I think Kconfig option usually meant to allow user to tweak something.
Here it's already possible to select different tests by name, so I don't see benefits in using Kconfig.

[github-actions]

Hi @f0rget-the-sad!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

[fabiobaltieri]

Hi @f0rget-the-sad, unfortunately this PR broke the project CI run on main (https://github.com/zephyrproject-rtos/zephyr/actions/runs/11934344226), the fix is not obvious to me so I've opened a revert PR #81673.

Would you be able to troubleshoot and re-submit a new version that works in CI? (or open a hotfix if that's obvious to you).

Apologies for the extra work.

[f0rget-the-sad]

This PR passed all checks triggered by CI: https://github.com/zephyrproject-rtos/zephyr/actions/runs/11893366338
I guess main CI runs even more tests, so question is: how do I trigger the main CI before merge?

[fabiobaltieri]

I guess main CI runs even more tests, so question is: how do I trigger the main CI before merge?

Yeah the test sets can be different, you can get the list of failed tests from https://github.com/zephyrproject-rtos/zephyr/runs/33270031021 and run those manually, for example for qemu_riscv64/qemu_virt_riscv64:tests/kernel/fatal/no-multithreading/kernel.no-mt.arbitrary_reason you can use

$ west build -p -b qemu_riscv64/qemu_virt_riscv64 -T tests/kernel/fatal/no-multithreading/kernel.no-mt.arbitrary_reason
$ west build -t run
...
 FAIL - test_fatal in 0.000 seconds
===================================================================
===================================================================
PROJECT EXECUTION FAILED

[f0rget-the-sad]

Ok, I manually tested tests/kernel/fatal/no-multithreading, tests/kernel/threads/no-multithreading and tests/lib/multi_heap (seems like all failures reported by main CI)with this hotfix: #81676
Let me know if there are other failures.

[fabiobaltieri]

@f0rget-the-sad thanks for the quick turnaround.