Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cpu/riscv: Add PMP driver #19712

Merged
merged 1 commit into from
Jun 29, 2023
Merged

cpu/riscv: Add PMP driver #19712

merged 1 commit into from
Jun 29, 2023

Conversation

Teufelchen1
Copy link
Contributor

@Teufelchen1 Teufelchen1 commented Jun 6, 2023 •
edited
Loading

Contribution description

Hi! 🐘

this adds a basic RISC-V physical memory protection (PMP) driver to RIOT. Well, 'driver' might be a stretched, feels more like a little utility :)

EDIT: Also added a no-execute RAM option for the hifive & a corresponding test

Since I only have an Hifive rev b, it's only enabled on this board / cpu. I also tested the code on an ESP32-C but the feature can't be enabled there, as cpu/riscv_common/ is not used by the ESP32...

Testing procedure

  • Grab a hifive rev b
  • go to examples/hello-world
  • Add USEMODULES += periph_pmp to the Makefile
  • Include pmp.h in main.c
  • Add code e.g. print_pmpcfg(0);
  • compile & flash & term

You should see something like this:

# Hello World!
# You are running RIOT on a(n) hifive1b board.
# This board features a(n) fe310 MCU.
# pmp00cfg: - R-X OFF   0x00000000 - 0x00000000

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jun 6, 2023
Teufelchen1
Teufelchen1 commented Jun 6, 2023
View reviewed changes
Copy link
Contributor Author

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not perfect yet! 😅

cpu/fe310/include/cpu_conf.h Outdated Show resolved Hide resolved
cpu/riscv_common/include/pmp.h Outdated Show resolved Hide resolved
cpu/riscv_common/include/pmp.h Outdated Show resolved Hide resolved
cpu/riscv_common/include/pmp.h Outdated Show resolved Hide resolved
cpu/riscv_common/periph/pmp.c Show resolved Hide resolved
@Teufelchen1 Teufelchen1 force-pushed the feat/pmpdriver branch 3 times, most recently from 7cdcabc to 3903ec3 Compare June 9, 2023 09:58
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 9, 2023
@benpicco
Copy link
Contributor

benpicco commented Jun 9, 2023

Can you add a test application to show/verify how this works?

@Teufelchen1
Copy link
Contributor Author

Teufelchen1 commented Jun 9, 2023 •
edited
Loading

I can if you insist. Otherwise, the test coverage will be provided in the near future by employing PMP for e.g. no-execute RAM. Similar to how the MPU on ARM Cortex is being tested.

Also, completely forgot about Kconfig 😵‍💫

@riot-ci
Copy link

riot-ci commented Jun 9, 2023 •
edited
Loading

Murdock results

✔️ PASSED

0e83965 cpu/riscv: Add PMP driver

Success Failures Total Runtime
6931 0 6931 10m:46s

Artifacts

@benpicco
Copy link
Contributor

benpicco commented Jun 9, 2023

Ah if this can be covered by the same test as MPU on ARM even better!

@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Jun 9, 2023
@github-actions github-actions bot added Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: doc Area: Documentation Area: tests Area: tests and testing framework labels Jun 12, 2023
@Teufelchen1
Copy link
Contributor Author

The no-execute RAM integration ended up being simpler than expected. I added a commit that also features a test.

I think this is done now. One final review please 😄

benpicco
benpicco reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small comments

cpu/fe310/cpu.c Outdated Show resolved Hide resolved
cpu/riscv_common/irq_arch.c Outdated Show resolved Hide resolved
tests/cpu/pmp_noexec_ram/main.c Show resolved Hide resolved
@Teufelchen1 Teufelchen1 force-pushed the feat/pmpdriver branch 2 times, most recently from e4e4ec6 to a92bfd3 Compare June 21, 2023 13:03
@Teufelchen1
Copy link
Contributor Author

Ping @benpicco

MrKevinWeiss
MrKevinWeiss reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I would add a comment in code about the switch case. If nobody objects I would ok merging tomorrow or so.

core/lib/include/panic.h
@@ -37,13 +37,13 @@ typedef enum {
PANIC_HARD_REBOOT,
PANIC_ASSERT_FAIL,
PANIC_EXPECT_FAIL,
PANIC_MEM_MANAGE, /**< memory management fault */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to move it to expose it in a common way. As long as nobody actually depends on this number or the next few potential codes (which would be platform specific anyways).

It is ok for me but should be considered.

@MrKevinWeiss
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 29, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 755442f into RIOT-OS:master Jun 29, 2023
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrKevinWeiss MrKevinWeiss MrKevinWeiss left review comments

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@smlng smlng Awaiting requested review from smlng

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms
Projects
None yet
Milestone
Release 2023.07
Development

Successfully merging this pull request may close these issues.
5 participants
@Teufelchen1 @benpicco @riot-ci @MrKevinWeiss @bergzand