Loader MMU Cleanups (AArch64)#482
Conversation
Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Indanz
left a comment
Indanz
left a comment
There was a problem hiding this comment.
I think comment "loader: cleanup, pte functions return [u8]" makes things worse, not better. The only reason it needs to be an array is because copy_from_slice is used. This comments spreads that local detail to other code with no gain.
Better solution would be to make boot_lvl* u64 arrays or something instead of this weird byte array stuff.
| let pt_index_bits = Self::PAGE_TABLE_INDEX_BITS * (pt_levels - level) as u64; | ||
| let idx = (addr >> (pt_index_bits + Self::PAGE_SHIFT)) % 512; | ||
| let pt_index_bits = PAGE_TABLE_INDEX_BITS * (pt_levels - level) as u64; | ||
| let idx = (addr >> (pt_index_bits + PAGE_SHIFT)) % 512; |
There was a problem hiding this comment.
Shouldn't this be % PAGE_TABLE_INDEX_BITS?
There was a problem hiding this comment.
Do you mean (1 << PAGE_TABLE_INDEX_BITS)?
But yes I haven't touched RISC-V much.
| { | ||
| let text_index_lvl1 = Riscv64::pt_index(num_pt_levels, text_addr, 1); | ||
| let pt_entry = Riscv64::pte_next(boot_lvl2_pt_elf_addr); | ||
| let text_index_lvl1 = riscv64::pt_index(num_pt_levels, text_addr, 1); |
There was a problem hiding this comment.
I'm surprised that the differences aren't be abstracted away and that there is so much Arch specific code left.
(This puzzlement also applies to the seL4 kernel code, but no reason to duplicate that approach elsewhere.)
There was a problem hiding this comment.
Yes, it is true that a lot of it is duplicated. This is by no means done: I didn't even start cleaning up RISC-V much.
But Riscv64 and Aarch64 has different kernel page tables so some has to be different, and the internal layouts need to be different.
There was a problem hiding this comment.
Sure, top level is different and low level is different. But that doesn't mean that all the intermediate code can't be shared...
There was a problem hiding this comment.
I agree. I just haven't done it yet.
midnightveil
force-pushed
the
julia/loader-mmu
branch
from
debbbe2 to
2bfde7f
Compare
Ivan-Velickovic
force-pushed
the
julia/loader-mmu
branch
from
2bfde7f to
002f4ce
Compare
midnightveil
force-pushed
the
julia/loader-mmu
branch
3 times, most recently
from
3200394 to
c40edaa
Compare
Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
midnightveil
force-pushed
the
julia/loader-mmu
branch
from
c40edaa to
68d334c
Compare
This cleans up the setting of the attributes, plus a bugfix: We were previously setting MemAttr == 0b100 from the MT_NORMAL as was used for Stage 1 (AttrIndex via MAIR), but this is a reserved value for the Stage2 MemAttr values. Explicitly handle both stage 1 and stage 2 memattr translations with an enum. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
midnightveil
force-pushed
the
julia/loader-mmu
branch
from
68d334c to
5fa7a63
Compare
3 participants
This is a series of cleanups, except for one commit:
We were previously setting the MemAttr to the Stage 1 AttrIndex values from MAIR_EL1, but since we boot in hyp/stage 2 we were using the reserved 0b100 value (which is constrained unpredictable to behave as one of the other values).