Add tests for all of the ARM cache operations on all kinds of frame mappings. #94
Conversation
Ivan-Velickovic
force-pushed
the
arm_invalidate_test
branch
4 times, most recently
from
44a45ce to
3e71462
Compare
There was a problem hiding this comment.
Other that the nitpicks everything seems good to go.
An alternative approach would be to have one test that remaps the same page instead, with one test function that takes two arguments: The expected result for invalidate and the expected result for all other operations. That would avoid a lot of duplicate code, but I'm not sure it's an overall improvement, as this version is very straightforward and easy to check.
There was a problem hiding this comment.
Looks good to me, thanks for doing this.
One observation is that the kernel doesn't track whether a mapping is cached or uncached. I wouldn't expect the architecture to have a problem with cache maintenance operations on mappings that are uncached, but maybe the seL4 design would wish to block cache maintenance operations on uncached mappings done through the vspace cap?
If seL4 doesn't care and the hardware doesn't care, why bother? |
It's not blocking now, but the point is that there could be an ambient authority situation where an uncached mapping implies an authority to manage caches for that mapped region. But you're right its probably a non-issue at this point because seL4 already doesn't care in other ways (ie there's no rights for restricting the ability to map something cached vs uncached). |
Ivan-Velickovic
force-pushed
the
arm_invalidate_test
branch
2 times, most recently
from
8add1e5 to
1c9b9a0
Compare
|
@Indanz can you please check if you're now happy with Ivan's changes? |
|
While we're expecting the tests to currently fail, it looks to me like some of the operations are returning a different kind of error, e.g. cap lookup fault instead of illegal operation. |
yea I don't know what's going on. I'll have a look tomorrow. |
| seL4_CPtr frame; | ||
| uintptr_t vstart; | ||
| int err; | ||
| void *vaddr; | ||
| reservation_t reservation; | ||
| vka_t *vka = &env->vka; | ||
|
|
||
| reservation = vspace_reserve_range(&env->vspace, PAGE_SIZE_4K, seL4_AllRights, 1, &vaddr); | ||
| assert(reservation.res); | ||
|
|
||
| vstart = (uintptr_t)vaddr; | ||
| assert(IS_ALIGNED(vstart, seL4_PageBits)); | ||
|
|
||
| /* Create a frame, but deliberately do not create a mapping for it. */ | ||
| frame = vka_alloc_frame_leaky(vka, PAGE_BITS_4K); | ||
| test_assert(frame != seL4_CapNull); |
There was a problem hiding this comment.
I'd move this into a separate function which with as parameter the rights to pass to vspace_reserve_range and call it in all the tests. That would save a lot of code duplication and make it easier to see the bits that change between the tests. At least I think it's all identical otherwise.
If not doing this, at least make all the code exactly the same without subtle differences for no reason, so it's easier to see it's identical.
lsf37
added
hw-test
Signed-off-by: Ivan-Velickovic <[email protected]>
Signed-off-by: Ivan Velickovic <[email protected]>
Signed-off-by: Ivan Velickovic <[email protected]>
On armv7-a and in EL-2, the corresponding operations are safe for the kernel to perform. Signed-off-by: Gerwin Klein <[email protected]>
lsf37
force-pushed
the
arm_invalidate_test
branch
from
129a97a to
13b9de7
Compare
|
(rebased and tweaked under which configs we should expect an error) |
|
Now passes on all runs for all boards. We still need to fix up the review comments, though. |
This PR aims to add test coverage for seL4/seL4#214. More specifically. it implement tests for the scenarios outlined here: seL4/seL4#214 (comment).
I have left out testing of
VMKernelReadOnlymappings as I believe there is no way to produce this mapping from user-space.