Conversation
74bc800 to
6e28cde
Compare
|
@qwandor this still needs more polishing and testing, but may I ask to have a look when you have some free time to verify whether the overall approach and structure is something sensible for |
qwandor
left a comment
There was a problem hiding this comment.
This seems like a broadly reasonable approach to me.
| translation: T, | ||
| asid: usize, | ||
| rootlevel: usize, | ||
| translation_regime: TranslationRegime, |
There was a problem hiding this comment.
This makes it possible to construct a Mapping with a TranslationRegime which doesn't match the attributes type. We should prevent that somehow. Maybe there could be separate types for Stage1TranslationRegime and Stage2TranslationRegime, and they could specify the corresponding attributes type via an associated type?
There was a problem hiding this comment.
That's true - this is a pretty big refactor though, would it make sense to combine it with splitting the Stage 1 attributes into different types and making it a separate PR? I've got an initial version here: m4tx/aarch64-paging@stage-2...m4tx:aarch64-paging:attribute-refactor
There was a problem hiding this comment.
That looks reasonable, I'll review it separately once you send a PR for it.
| El2And0, | ||
| /// Non-secure EL1&0, stage 1. | ||
| El1And0, | ||
| /// Non-secure Stage 2. |
There was a problem hiding this comment.
This applies to the other existing options too, but does secure world actually use different translation regimes to normal world?
| /// Attribute bits for a mapping in a Stage 1 page table. | ||
| #[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct Attributes: usize { | ||
| pub struct Stage1Attributes: usize { |
There was a problem hiding this comment.
Possibly we should split this into two different types, as some of the existing translation regimes have slightly different attribute bits too.
There was a problem hiding this comment.
See the comment above, I've got an initial version of this here: m4tx/aarch64-paging@stage-2...m4tx:aarch64-paging:attribute-refactor
There was a problem hiding this comment.
Thanks, that looks reasonable, I'll review it properly once this is merged.
src/descriptor.rs
Outdated
| const TABLE_OR_PAGE = 1 << 1; | ||
|
|
||
| const MEMATTR_DEVICE_nGnRnE = 0 << 2; | ||
| const MEMATTR_NORMAL = 0xf << 2; // Example value |
There was a problem hiding this comment.
What do you mean by this being an "example value"?
There was a problem hiding this comment.
Sorry, left it after prototyping quickly - I've replaced this with proper attributes now.
src/descriptor.rs
Outdated
|
|
||
| const PHYSICAL_ADDRESS_BITMASK: usize = !(PAGE_SIZE - 1) & !(0xffff << 48); | ||
|
|
||
| pub(crate) fn new(value: usize) -> Self { |
There was a problem hiding this comment.
Can this be const? Then you can use it for EMPTY too.
There was a problem hiding this comment.
Good point - indeed, it can.
qwandor
left a comment
There was a problem hiding this comment.
This looks good to me. @ardbiesheuvel, do you want to have a look at it before we merge it?
Please squash these changes together so I can look at the end result, rather than the journey that brought us there :-) (Multiple patches in a PR is fine, and even preferred, if they consist of logical discrete steps implementing the goal of the PR. Incremental changes in response to review feedback should be squashed together). Thanks. |
|
@ardbiesheuvel sure, there you go! |
src/descriptor.rs
Outdated
| if (old_bits ^ new_bits) & (MEM_ATTR_MASK | SH_MASK) != 0 { | ||
| return false; | ||
| } | ||
| true |
There was a problem hiding this comment.
Does this permit turning a block mapping into a table mapping? That is a problem, because it not only violates BBM, it also corrupts our data structures.
There was a problem hiding this comment.
Ah, that's a very fair point. I've changed this code to be written in an allowlist-style, similarly to the Stage1 implementation, so that even with new attributes potentially being added, it will still be safe.
src/descriptor.rs
Outdated
| if !old.contains(Self::VALID) || !new.contains(Self::VALID) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Are you removing this because it is redundant? That's fine, but please do it in a separate patch.
| return false; | ||
| } | ||
| true | ||
| (!old & new & !allowed_mask).is_empty() && (old & !new & !allowed_mask).is_empty() |
There was a problem hiding this comment.
This could be ((old ^ new) & !allowed_mask).is_empty(), right?
No description provided.