kern: permit USlices to span contiguous MPU regions #1674
Conversation
cbiffle
force-pushed
the
cbiffle/lease-overlap-regions
branch
from
4bda7ea
to
38d89d8
Compare
|
Looks like the LPC55 apps need their kernels embiggened again. |
|
Alright, addressed those comments by adding lots of comments (and changing one line of actual code). PTAL. |
This part, I am still fixing. |
cbiffle
force-pushed
the
cbiffle/lease-overlap-regions
branch
3 times, most recently
from
72fe016
to
32daba4
Compare
|
This is ready to go, PTAL. |
Now that the build system has started packing tasks more cleverly, MPU region boundaries can appear in the middle of task RAM and Flash in positions that are difficult for the program to predict. This means the program can't safely lend any section of RAM or Flash without risking drawing the ire of the kernel, which --- owing to the limitations of earlier versions of the build system --- would not accept any access that spanned a region. This fixes that. To make fixing that relatively inexpensive, this also sorts task region tables by base address, so we can do a linear scan with early exit. See #1672 for more details and analysis.
cbiffle
force-pushed
the
cbiffle/lease-overlap-regions
branch
from
32daba4
to
cdbdbd3
Compare
| /// This must be consistent with the base/end addr implementations, such | ||
| /// that `is_empty <==> base_addr == end_addr`. | ||
| fn is_empty(&self) -> bool; |
There was a problem hiding this comment.
we could have a default implementation of this that actually does that?
| /// This must be consistent with the base/end addr implementations, such | |
| /// that `is_empty <==> base_addr == end_addr`. | |
| fn is_empty(&self) -> bool; | |
| /// This must be consistent with the base/end addr implementations, such | |
| /// that `is_empty <==> base_addr == end_addr`. | |
| fn is_empty(&self) -> bool { | |
| self.base_addr() == self.end_addr() | |
| } |
although perhaps checking if self.len == 0 is a bit faster, because the implementation will probably represent the slice as a base + len rather than base + end. maybe what we really should do is structure the trait as having a fn base_addr and fn len and having default impls of end_addr and is_empty? that way, it seems harder for a user to violate invariants.
There was a problem hiding this comment.
I could be convinced, but, I'm kind of concerned about default implementations here accidentally making their way into the kernel. This is why I haven't provided any.
As for rephrasing it in terms of base+len... that would accurately describe the types in the kernel, though the kernel's end_addr routine has been optimized to avoid the checked arithmetic, so we'd still need to be able to provide an end_addr override somehow, and I think that winds up meaning a default impl.
| /// `table` must be sorted by region base address, and the regions in the table | ||
| /// must not overlap. |
There was a problem hiding this comment.
would it make sense to debug_assert! that the table is sorted? do we ever actually build the kernel with debug assertions enabled?
There was a problem hiding this comment.
We don't build the kernel with debug assertions, no. I thought about this, and it would find bugs in the test fixture in this crate, which is something? But it wouldn't detect bugs in the kernel.
Now that the build system has started packing tasks more cleverly, MPU region boundaries can appear in the middle of task RAM and Flash in positions that are difficult for the program to predict. This means the program can't safely lend any section of RAM or Flash without risking drawing the ire of the kernel, which --- owing to the limitations of earlier versions of the build system --- would not accept any access that spanned a region.
This fixes that. To make fixing that relatively inexpensive, this also sorts task region tables by base address, so we can do a linear scan with early exit.
See #1672 for more details and analysis.