Refactored the PMP address check for RV32 vs RV64 #4414
Conversation
There was a problem hiding this comment.
Usually I am all for using combinators everywhere. But for these changes specifically, I subjectively feel like they don't help readability (in fact, I think that the if-then-early-return is more readable than the bool::then_some expressions).
Also, I don't think that the if size_of::<usize>() == size_of::<u64> checks result in a runtime check. Those are const expressions that the compiler should be able to evaluate and optimize out. I'd bet this would emit virtually identical binaries; have you been able to observe a difference?
I buy the fact that the word-width checks are not actually required when using a usize mask. I think a good compromise could be to expand the then_some combinators into ifs, but always perform the bit-masking regardless of word size. But I'm open to see what others are thinking.
|
Just some background for my motivation. I am working on extending the PMP to support minimum granularity other than 4 bytes, and it will introduce extra checks in these functions. So I thought that more concise code with all the checks combined would look nicer |
|
Okay, in that case I think it might be best to get that PR up as well, and then we can see the full changeset. I don't mind readability & conciseness improvements at all, but at the same time, we should try to judge whether a given change actually makes code easier to read, and whether it creates unnecessary churn. How do you plan to support PMPs with minimum granularity other than four bytes? That seems like it would be a major change to the existing layering, which is explained here: #3597. It seems like at least the PMPUserMPU would need to change. I'm excited for adding this capability. I invite you to discuss this re-architecture with me (and others, if they're interested) before working on & proposing the full change set. This way we can try to retain the benefits of the current PMP architecture in terms of static checks, compositional modularity, efficiency and readability (esp. over the prior version), while supporting this expanded feature set. |
|
Sure thing. However I do not think it requires major changes to the existing architecture. At the first stage at least my plan was just to reject memory regions not aligned with the minimum granularity defined by the specific hardware. The existing board should feel minimum impact. Then the memory layout (linker scripts) for the kernel and user apps have to be designed with this in mind, but again it is only for the specific boards having this PMP limitation. I guess further complications could come from the grants mechanism, but I didn't look that far. Please let me know if there is something major I missed. |
arch/rv32i/src/pmp.rs
Outdated
| const PMPADDR_RV64_MASK: u64 = 0x003F_FFFF_FFFF_FFFF; | ||
| /// This mask will have the value `0x003F_FFFF_FFFF_FFFF` on RV64 platforms, and | ||
| /// `0xFFFFFFFF` on RV32 platforms. | ||
| const PMPADDR_RV64_MASK: usize = (0x003F_FFFF_FFFF_FFFFu64 & usize::MAX as u64) as usize; |
There was a problem hiding this comment.
This mask is now generic across both RV32 and RV64 platforms. Can you remove RV64 from the name, and in the doc-comment above? Also, please add an empty new-line before your additions to the doc-comment, otherwise this will be not be displayed as a separate paragraph in the rendered Markdown.
arch/rv32i/src/pmp.rs
Outdated
| // Prevent the `&-masking with zero` lint error in case of RV32. | ||
| // The redundant checks in this case are optimized out by the compiler on any 1-3,z opt-level. | ||
| #[allow(clippy::bad_bit_mask)] | ||
| (pmpaddr & !PMPADDR_RV64_MASK == 0).then_some(NAPOTRegionSpec { pmpaddr }) |
There was a problem hiding this comment.
I still don't like it much, but I'm fine with leaving this using .then_some(...).
|
This will probably need to be rebased once #4415 is merged; it's easier to merge that first and then rebase this PR, as the diff here is very small. I'm happy to do the rebase myself. |
Pull Request Overview
Removing runtime checks for the size of
usizein favor of compile-time mask calculation.Some "rustification" of condition checks
Testing Strategy
Unit-tested in isolation. Functionally tested on RV32 QEMU
TODO or Help Wanted
N/A
Documentation Updated
N/A
Formatting
make prepush.