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.
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
arch/rv32i: Don't sort PMP regions when we configure the MPU #2021
Conversation
I did try this and this plus #2020, and the arty still seems to stack overflow (i.e. my app doesn't run). |
Interesting, can you diff the stack outputs between these patches and the PMP array changes that get it to work? |
on master:
I confirmed the arty does not work with these patches, or with these plus #2019. However, adding my "I guess?" patch doesn't fix it once adding these patches.... With trace syscalls on, explicitly calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch doesn't work. Because we hardcode the location of the APP_MEMORY region, we need that region to always stay in the same place. So we can't store a sorted list of regions.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Previously we would sort the PMP regions everytime we configured the MPU. This was wasteful in terms of instructions but also wasted stack space, probably because we cloned the PMPConfig regions. This patch changes the sorting to only happen after a change is made to the regions. This should save the number of times we have to sort, remove sorting out of the critical path and saves a lot of stack space. See below for the stack changes: ### Release build ### ## Before this patch ## 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17hb3b05a589563feedE 0 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17hd6078caa7928d4d8E 592 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h5967285bd6ec5881E ## After this patch ## 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17hb3b05a589563feedE 16 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17hd6078caa7928d4d8E 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h5967285bd6ec5881E ### Debug build ### ## Before this patch ## 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17h272c094b5b64d63bE 96 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$26allocate_app_memory_region17h56b3decc4c649f8cE 32 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17h55a48db93ba6d713E 592 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h32d25c65ffe73910E ## After this patch ## 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17h272c094b5b64d63bE 96 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$26allocate_app_memory_region17h56b3decc4c649f8cE 32 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17h55a48db93ba6d713E 80 _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h32d25c65ffe73910E NOTE: All above numbers already include: tock#2020 I haven't compared instruction diffs, but I'm assuming we reduce the number of instructions run on app swap as well. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
ced1826
to
bed1b92
Compare
Ah great catch! I have updated the PR with a fix for that and actually tested it this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works!
Great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Pull Request Overview
Previously we would sort the PMP regions everytime we configured the
MPU. This was wasteful in terms of instructions but also wasted stack
space, probably because we cloned the PMPConfig regions.
This patch changes the sorting to only happen after a change is made to
the regions. This should save the number of times we have to sort,
remove sorting out of the critical path and saves a lot of stack space.
See below for the stack changes:
NOTE: All above numbers already include: #2020
I haven't compared instruction diffs, but I'm assuming we reduce the
number of instructions run on app swap as well.
Testing Strategy
Tested running apps on QEMU OpenTitan.
TODO or Help Wanted
Documentation Updated
/docs
, or no updates are required.Formatting
make prepush
.