make VirtAddr generic over address validity#586
make VirtAddr generic over address validity#586Freax13 wants to merge 1 commit intorust-osdev:masterfrom
Conversation
As proposed by Joe and Philipp, this patch adds a generic parameter for VirtAddr. This generic parameter can be used to enforce validity for addresses with 4 level paging or 5 level paging. This patch is still a work in progress because I wanted to make that this is what you had in mind before I spend more time on it. I slightly modified the VirtValidity interface to because check_virt_addr by itself cannot be used to implement truncation, but truncation can be used to implement check_virt_addr. That's just a minor implementation detail though, we can discuss this later. TODO: - Add a feature to gate enabling the const_trait_impl feature. - Fix up the remaining code. I didn't adjust all of the code to take a generic parameter for the validity yet. As of right now, there are a ton of compilation errors. - Figure out how to make the mapper impls work with VirtValidity. VirtValidity by itself doesn't tell the mappers how many levels there are. I don't expect this to be difficult to solve. - Default the validity generic parameter to Virt48. Currently, I intentionally left the default out, so that the compiler would tell me about all the code that needed to become aware of it. Rust doesn't allow defaulted generic parameters after non-defaulted ones, so I currently put the validity parameter before the page size parameter for Page. We'll probably want to swap that once we set the default. - Test that the Add/Sub/Step impls work and fix them if they don't. Up until now I mostly focused on fixing compilation errors and not so much on the code actually working. - Add type aliases? User would probably benefit from VirtAddr48, VirtAddr57, Page48, and Page57. - Do the same thing for PhysAddr. - Rebase this on next. This will very likely be a breaking change.
phil-opp
left a comment
phil-opp
left a comment
There was a problem hiding this comment.
Sorry for the slow reply!
I think the approach in general looks good, it just adds quite a bit of noise.
How about we add a non-generic RawVirtAddr type to avoid the generic read methods? This RawVirtAddr could implement TryInto for the two possible VirtAddr<V> types so that you can do the conversion yourself.
For the corresponding write methods, we could take an Into<RawVirtAddr> as argument and implement this trait for all VirtAddr variants.
| unsafe { | ||
| let val: u64; | ||
| asm!(concat!("rd", $name, "base {}"), out(reg) val, options(nomem, nostack, preserves_flags)); | ||
| VirtAddr::new_unsafe(val) |
There was a problem hiding this comment.
This might be problematic if the value is a 5-level address and V is a 48 bit address.
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| #[repr(transparent)] | ||
| pub struct VirtAddr(u64); | ||
| pub struct VirtAddr<T: VirtValidity>(u64, PhantomData<T>); |
There was a problem hiding this comment.
Can we set the default for T to Virt48?
There was a problem hiding this comment.
Yes, it's on my TODO list.
| #![warn(missing_docs)] | ||
| #![deny(missing_debug_implementations)] | ||
| #![deny(unsafe_op_in_unsafe_fn)] | ||
| #![feature(const_trait_impl)] |
There was a problem hiding this comment.
Is there a way to implement this feature on stable?
There was a problem hiding this comment.
Not with the current interface, although another interface certainly could work.
For example we could define the trait like this:
pub trait VirtValidity {
/// Bitmask of all the bits that may be set in the address.
const MASK: u64;
}This would work on stable, but would limit the VirtValidity traits usability somewhat because the bitmask would have to be defined at compile time. This would allow implementing Virt48 and Virt57, but not VirtRt.
|
I think we should take a step back and think about how this is going to be used. Based on the discussion in #435, my impression was that the goal of this approach is to help support LA48 and LA57 within the same binary, but I'm still not sure how that's supposed to work. Here's the problem I have: Personally, I like using the What do we expect users to do? Do we expect them to use the wider LA57 address types and manually check that the upper bits are not set when running in LA48 mode? Do we expect them to use the wider LA57 address types and simply not care if the upper bits are set when running in LA48 mode? Do we expect them to write generic code supporting both and switching between the two instantiations based on a runtime flag? Do we expect them to use |
2 participants
As proposed by Joe and Philipp, this patch adds a generic parameter for VirtAddr. This generic parameter can be used to enforce validity for addresses with 4 level paging or 5 level paging.
This patch is still a work in progress because I wanted to make that this is what you had in mind before I spend more time on it.
I slightly modified the VirtValidity interface to because check_virt_addr by itself cannot be used to implement truncation, but truncation can be used to implement check_virt_addr. That's just a minor implementation detail though, we can discuss this later.
TODO:
Closes #435