Add remove_mpu_region() functionality to mirror allocate_region() #2728
Conversation
|
Do we have an example on when this would be used? For tests you can add a OpenTitan unit test adding and removing a region. That would allow testing with 2.0 |
We have an out-of-tree component that dispatches large buffers from the kernel to applications. We don't want to copy these large buffers, so we temporarily give a particular application access to the buffer that is in the kernel region. When the app is done with it, and before the kernel can reuse it, the kernel will unmap that buffer from the application.
I found the |
|
I will hopefully be able to test this with RISCV based core with Tock 2.0 very soon. Could you help me test the CortexM version (or should I just remove that code change from this PR)? |
|
Is this something we want in mainline then? Mainline is not going to pass kernel regions to applications so we don't have a use for removing mpu regions. Whoops, I wasn't thinking. The unit tests aren't helpful for PMP tests as you are right they don't apply to M mode only. In terms of adding a test your best bet is to add support to libtock-c and then add an example app. At that point it's easy for others to run the test (I can run it on a Cortex-M board as well). |
|
I was finally able to test this on a RISC-V system with a new kernel. It works :) I didn't have the patch locally and I was getting MPU failures when trying to add the same region again. I don't have a cortex M system to test with. Should we
Thoughts? |
|
What is the easiest way for someone to test for you on cortex-m? Just modify a kernel to add/remove/add an MPU region and verify it works? |
Yes, Trying adding the same mpu region twice. The second add should fail. Then try to remove the region between the two add calls, and the second add should succeed. Also it is worth testing that you cannot access that buffer while it is removed in the app (should get an app hardware fault) |
|
Marking last call. Planning to merge soon (once rebased) unless someone has an objection. |
|
I can offer to try and test it tomorrow on an nRF52. |
Is that something I do as the PR owner, or the core team does before the merge? I am more thank happy to rebase; just let me know what the expectations are :) |
|
You as the owner, generally :). |
If someone calls `allocate_region`, then they might also want to remove that region later. Add a `remove_mpu_region` that takes in the output of the `allotcate_region` call and will un map the memory from the process. Change-Id: I4dc558dae34509a9a7f71193fb558f1f8c9aa929
Add support to remove a memory region from the region list Change-Id: I58b0fc42c23ff4e928f4cc620d5b7e56a489af29
Add support to remove a memory region from the region list Change-Id: Ife20d9dc60f6f9d46bae047841e5d1c88436ebf1
|
Good to know :) Rebase done. |
|
Can we add a unit test to make sure that: add(1)
remove(1)
add(1)works. and that add(1)
add(1)Doesn't? Even if we don't test accesses from usersapce it will be nice to know that we don't get errors |
What test harness would I be doing that from? Wouldn't that just be testing the implementation of the host emulation chip support? |
|
I believe Alistair is referring to the QEMU-based unit tests in |
|
I am referring to the unit tests in By adding unit tests we would be testing that future changes to PMP/ePMP don't cause regressions inside the Tock kernel. |
|
I tested this on cortex-m today (Imix). add/remove/add works as expected, and add/add fails as expected. In order to run this test I had to modify the core ipc code, as I think there is nowhere else in the code where we add MPU regions dynamically today, and then run the IPC apps. I took a stab at adding unit tests, but got frustrated enough trying to fix the QEMU test infrastructure that I gave up (and I don't have a hardware OT board). |
|
Is this able to land now? |
|
bors r+ |
If someone calls `allocate_region`, then they might also want to remove that region later. Add a `remove_mpu_region` that takes in the output of the `allotcate_region` call and will un map the memory from the process. SOURCE=FROMPULL(tock#2728) Change-Id: I4dc558dae34509a9a7f71193fb558f1f8c9aa929 (cherry picked from commit 64327899f22cbe1fd00d31cdf20bad05054fa213) Reviewed-on: https://chrome-internal-review.googlesource.com/c/ti50/third_party/tock/tock/+/4103388 Tested-by: Jett Rink <jettrink@google.com> Commit-Queue: Jett Rink <jettrink@google.com> Reviewed-by: Hudson Ayers <hudsonayers@google.com> Reviewed-by: Andrey Pronin <apronin@google.com>
Add support to remove a memory region from the region list SOURCE=FROMPULL(tock#2728) Change-Id: Ife20d9dc60f6f9d46bae047841e5d1c88436ebf1 (cherry picked from commit f5d2246ebd79dabf574fbf2ccacca961cee020e9) Reviewed-on: https://chrome-internal-review.googlesource.com/c/ti50/third_party/tock/tock/+/4103389 Tested-by: Jett Rink <jettrink@google.com> Commit-Queue: Jett Rink <jettrink@google.com> Reviewed-by: Hudson Ayers <hudsonayers@google.com> Reviewed-by: Andrey Pronin <apronin@google.com>
Add support to remove a memory region from the region list SOURCE=FROMPULL(tock#2728) Change-Id: I58b0fc42c23ff4e928f4cc620d5b7e56a489af29 (cherry picked from commit bdb8599bd2a529239a2ff617e8763d0c9906c6c6) Reviewed-on: https://chrome-internal-review.googlesource.com/c/ti50/third_party/tock/tock/+/4115825 Tested-by: Jett Rink <jettrink@google.com> Auto-Submit: Jett Rink <jettrink@google.com> Reviewed-by: Vadim Sukhomlinov <sukhomlinov@google.com> Commit-Queue: Jett Rink <jettrink@google.com>
Pull Request Overview
When a users maps a new mpu region to a process with
allocate_region, they may want to later remove that region. This PR adds the appropriate removal function to the process interface and implements the new MPU helper function (remove_memory_region) for the MPU implementation that already implementallocate_regionThis can wait until after Tock OS 2.0 release.
Testing Strategy
Help Wanted
Documentation Updated
Formatting
make prepush.