-
Notifications
You must be signed in to change notification settings - Fork 10
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
CRAM/CRRL should depend on base and length #96
Comments
Hi @vmurali and thanks for raising this. CRRL and CRAM are part of the (draft) upstream riscv-cheri spec. and are designed for allocators so they can increase the size of a requested allocation to meet alignment requirements. I believe the cheriot-rtos allocator uses them in that way, so we can't just redefine them. The code you're looking at does look a bit suspicious and I'm looking into it. The situation is slightly different than the one the allocator faces so it's possible we're using the instructions incorrectly. We already defined #74 to handle another specific situation but I don't think that applies here. If the loader is the only place where this situation arises I doubt a new instruction is appropriate as it's not a fast path, but we do need a way of doing it correctly and would ideally avoiding embedding too much knowledge of the capability encoding (e.g. the precision) into the loader. Therefore, I think this is probably an RTOS issue, not an ISA one, but we'll see. Some observations:
|
I did see
The semantics in the heap (boot.cc) seems to be (based on my chat with Wes offline):
|
I should read their spec before it's too late :( . If the goal is to use it for malloc-like allocators, then the semantics for that seems to be:
Here |
Regarding I am not entirely sure about a counter example to the heap bounds but here's what I discussed with Wes offline: if
|
It's interesting to note that only the following cases seem to be useful:
The current version of |
I see the current version of |
I don't think that's possible with the current code? (Note what it calls
We check, with
I'd add "allocate precisely The "smaller than
Do you have other uses in mind?
I believe (but @davidchisnall is more authoritative) that the loader presumes that the compiler has generally laid objects out such that the inexact bounds computation does not violate disjointness. |
Right, it relies on
Where do you actually use CSetBoundsExact? This was my search result: https://github.com/search?q=repo%3ACHERIoT-Platform%2Fcheriot-rtos+set_exact&type=code
No, those were the uses:
|
We sure do like our C++ wrappers. Unfortunately, they make code more readable at the expense of being essentially unsearchable without compiler support, and AFAIK nobody's DTRT for tooling here. This search finds many (but probably not all) of the uses of assignment to |
I think this is an interesting analysis and would add to it a bit. The ISA doesn't have explicit support the first two at the moment and it's interesting to ask why we don't seem to have needed them.
I analysed an objdump of the test suite and made some notes on current usage of set bounds instructions:
|
I think MinimumSuperset and MaximumSubset can share a lot of hardware. So it may not be that bad to implement this.
Assuming MaximumSubset shares code with MinimumSuperset and hence not complex, we can skip this. But I think this can share hardware with RoundBaseUp - essentially check the largest exponent for size against the alignment of top instead of alignment of base.
Okay I understand the need to use this for sub-object bounds, especially with array splicing.
Yeah I saw
I haven't seen the allocator code; I am guessing it's a bumping allocator (may not be linear bumping, but you still need to know the bounds of the surrounding object to allocate correctly). How do you guarantee the base is correctly aligned when using crrl and cram? |
First, an apology: the allocator code is (and has been for a long while) sort of half way through a refactoring from a very C-style codebase into a C++ codebase and it shows. The allocator is essentially When servicing an allocation request, we round up the size using CRRL and ensure that it is aligned according to CRAM. |
I tried to understand the allocator code, but I am being too stupid today to fully understand it. But the gist seems to be as follows: malloc tries to find an object in the heap with alignment given by CRAM, and minimum size given by CRRL. If such an object doesn't exist, https://github.com/CHERIoT-Platform/cheriot-rtos/blob/main/sdk/core/allocator/alloc.h#L2708 will fail. If so, the heap allocator does not need anything other than the current CRAM and CRRL. Regarding the distinction between MaximumSubset and RoundBaseUp, the implementation of MaximumSubset seems to be simple at least with the bounds invariant in https://github.com/CHERIoT-Platform/cheriot-sail/pulls. Given a
You don't have to worry about top being aligned with what was requested (i.e. final |
No, not stupid. The code is... dense and could be significantly more readable. Your comment about |
Given how CRAM and CRRL is used in https://github.com/CHERIoT-Platform/cheriot-rtos/blob/c994de3d1a476d824ffb7ab8711a52307ac7a9f0/sdk/core/loader/boot.cc#L502-L513, the base is also needed to get a valid
roundedBase
androundedSize
that encompasses the heap.Moreover there are a bunch of related issues in that code:
roundedSize
should be calculated using howgetRepresentableLength
is implemented incheriot-sail/src/cheri_cap_common.sail
Line 853 in 81bf3d2
<=
given that it is rounded down? Moreover, this invariant would also be easily violated if the suppled base and size are not already sufficiently aligned.All these problems could be solved (and you can get rid of the invariants) if CRAM and CRRL takes a base as an argument and returns a valid size/mask to work manipulate the input base/size.
@nwf @rmn30 @davidchisnall
The text was updated successfully, but these errors were encountered: