rv32i: Convert PMP disable to a nop #1871
Conversation
We never set the lock bit for PMP (and if we did we can't unset it anyway) so PMP won't apply to Tock only apps. In that case we never need to disable it as it will be fully reconfigured before running the next app. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
alistair23
force-pushed
the
alistair/pmp-disable
branch
from
f2dfcd1
to
3b2d950
Compare
There was a problem hiding this comment.
I think your description would also apply to the cortex-m MPU. I don't think this is the right place for the MPU trait implementations to diverge.
Is there harm in having the PMP be disabled? Or does it just hurt efficiency? #1867 seems like the place to re-work the MPU API now that we have a new implementation and ongoing work around improving syscall efficiency. It seems like we really want disable_mpu() to be disable_mpu_for_kernel(), which again I think would be a no-op in both in-tree MPU implementations.
In general I think we should the disable code around. Particularly since we have bootloaders that we have no idea what they are doing. I recently used this function to ensure the PMP wasn't getting in the way (and if I recall correctly it did have some effect).
|
There is no harm in disabling the PMP, this is just more efficient. We waste a lot of cycles writing all these CSRs. In saying that if someone has set the lock bits, it can not be disabled, so the function as is won't work. #1867 is more focused on ePMP so I would like to keep that a littler separate. #1867 will make no difference to regular PMP.
|
|
Replaced with: #1873 |
1873: PMP and MPU Improvements r=bradjc a=alistair23 ### Pull Request Overview This PR replaces: - #1867 - #1871 And combines a few different MPU/PMP improvements 1. Rename the `enable/disable_mpu()` functions to be `enable/disable_app_mpu()` functions. This is because these functions only enable and disable the MPU for apps. 2. Conver the current `disable_mpu()` function to be `clear_mpu()` and run it at boot. Instead of clearing the MPU/PMP after returning to the kernel from an app let's instead do nothing as we don't need to. We still leave the function around though, encase a future architectures wants to use it. This should improve syscall performance, especially for RISC-V. We also clear the MPU/PMP at boot encase a previous stage has set anything up. 3. We also add new functions in preperation for RISC-V ePMP (see below) #### RISC-V ePMP The RISC-V enhanced PMP spec is making progress and it seems like it is unlikely to have any major changes before ratification. In anticipation of this let's start to prepare adding support to Tock. This PR updates the MPU trait to allow marking regions of kernel memory with permissions. The idea here is that before the MPU is enabled Tock will set read/write/execute permissions for itself. This will limit what Tock itself can access, for example by removing write permissions from code. We can also add a check to see if it is already enabled. If the previous stage has enabled lock down mode we won't set it ourselves. We will have to trust the previous stage (that would have loaded us) to have done it correctly. If no one else has set it, we can set it for enhanced security. Once enabled we can't change the execute permission (read/write can change) so let's enforce that all regions are added before enabling. ### Testing Strategy Running on QEMU, I'll test the Unleashed and Apollo3 as well. ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make format`. - [X] Fixed errors surfaced by `make clippy`. 2080: OpenTitan: Add support for detecting the USB device r=bradjc a=alistair23 ### Pull Request Overview This allows me to see the OpenTitan USB device when running Tock. There still seems to be some timing issues and the device doesn't always appear, but at least this is better then the current nothing. ### Testing Strategy Plugging in the OpenTitan FPGA. ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make prepush`. Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
Pull Request Overview
We never set the lock bit for PMP (and if we did we can't unset it
anyway) so PMP won't apply to Tock only apps. In that case we never need
to disable it as it will be fully reconfigured before running the next
app.
Testing Strategy
This pull request was tested by running an app on the HiFive1 revB and the QEMU CI.
TODO or Help Wanted
Documentation Updated
/docs, or no updates are required.Formatting
make format.make clippy.