On Wed, Nov 20, 2024 at 8:20 PM Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> wrote: > > On Wed, Nov 20, 2024 at 02:49:57PM +0000, Alice Ryhl wrote: > > The vm_insert_page method is only usable on vmas with the VM_MIXEDMAP > > flag, so we introduce a new type to keep track of such vmas. > > Worth mentioning that it can be used for VMAs without this flag set, but if > so we must hold a _write_ lock to be able to do so, so it can update the > flag itself, however we intend only to use it with VMAs which already have > this flag set. I got the impression from Jann that the automatically-set-VM_MIXEDMAP thing should only be used during mmap, and that VM_MIXEDMAP should not get set after initialization even with a write lock? https://lore.kernel.org/all/CAG48ez3gXicVYXiPsQDmYuPSsKMbES2KRQDk+0ANWSS0zDkqSw@xxxxxxxxxxxxxx/ > > The approach used in this patch assumes that we will not need to encode > > many flag combinations in the type. I don't think we need to encode more > > than VM_MIXEDMAP and VM_PFNMAP as things are now. However, if that > > becomes necessary, using generic parameters in a single type would scale > > better as the number of flags increases. > > > > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > > --- > > rust/kernel/mm/virt.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 67 insertions(+), 1 deletion(-) > > > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > > index 1e755dca46dd..de7f2338810a 100644 > > --- a/rust/kernel/mm/virt.rs > > +++ b/rust/kernel/mm/virt.rs > > @@ -4,7 +4,14 @@ > > > > //! Virtual memory. > > > > -use crate::{bindings, types::Opaque}; > > +use crate::{ > > + bindings, > > + error::{to_result, Result}, > > + page::Page, > > + types::Opaque, > > +}; > > + > > +use core::ops::Deref; > > > > /// A wrapper for the kernel's `struct vm_area_struct` with read access. > > /// > > @@ -80,6 +87,65 @@ pub fn zap_page_range_single(&self, address: usize, size: usize) { > > ) > > }; > > } > > + > > + /// Check whether the `VM_MIXEDMAP` flag is set. > > + #[inline] > > + pub fn check_mixedmap(&self) -> Option<&VmAreaMixedMap> { > > Nitty + a little bikesheddy (this may be my mistake as I am unfamiliar with > rust idioms also) but shouldn't this be 'as_mixedmap_vma()' or something? You're probably right that this name is more consistent with Rust naming conventions. :) > > + if self.flags() & flags::MIXEDMAP != 0 { > > + // SAFETY: We just checked that `VM_MIXEDMAP` is set. All other requirements are > > + // satisfied by the type invariants of `VmAreaRef`. > > + Some(unsafe { VmAreaMixedMap::from_raw(self.as_ptr()) }) > > + } else { > > + None > > + } > > + } > > +} > > + > > +/// A wrapper for the kernel's `struct vm_area_struct` with read access and `VM_MIXEDMAP` set. > > +/// > > +/// It represents an area of virtual memory. > > +/// > > +/// # Invariants > > +/// > > +/// The caller must hold the mmap read lock or the vma read lock. The `VM_MIXEDMAP` flag must be > > +/// set. > > +#[repr(transparent)] > > +pub struct VmAreaMixedMap { > > + vma: VmAreaRef, > > +} > > + > > +// Make all `VmAreaRef` methods available on `VmAreaMixedMap`. > > +impl Deref for VmAreaMixedMap { > > + type Target = VmAreaRef; > > + > > + #[inline] > > + fn deref(&self) -> &VmAreaRef { > > + &self.vma > > + } > > +} > > Ah OK, thinking back to the 'impl Deref' from the other patch, I guess this > allows you to deref VmAreaMixedMap as a VmAreaRef, does it all sort of > automagically merge methods for you as if they were mix-ins then? Yeah, I use it to "merge" the method sets to avoid duplication. The main limitation is that you can only deref to one other type, so you can't have "diamond inheritance". > > +impl VmAreaMixedMap { > > + /// Access a virtual memory area given a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `vma` is valid for the duration of 'a, and that the mmap read lock > > + /// (or stronger) is held for at least the duration of 'a. The `VM_MIXEDMAP` flag must be set. > > + #[inline] > > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self { > > + // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a. > > + unsafe { &*vma.cast() } > > + } > > + > > + /// Maps a single page at the given address within the virtual memory area. > > + /// > > + /// This operation does not take ownership of the page. > > + #[inline] > > + pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result { > > I'm guessing this 'Result' type encodes 0 vs. -Exxx error codes? In this particular case, yes. More generally, Result is a discriminated union containing either Ok(val) for success or Err(err) with some error type. But the default success type is the unit type () and the default error type is kernel::error::Error which corresponds to errno numbers. In this case, the compiler is clever enough to not use a discriminated union and instead represents Ok(()) as 0 and Err(err) as just the negative errno. This works since kernel::error::Error uses NonZeroI32 as its only field (as of 6.13). > > + // SAFETY: The caller has read access and has verified that `VM_MIXEDMAP` is set. The page > > + // is order 0. The address is checked on the C side so it can take any value. > > + to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address as _, page.as_ptr()) }) > > + } > > } > > It's really nice to abstract this as a seprate type and then to know its > methods only apply in known circumstances... I guess in future we can use > clever generic types when it comes to combinations of characteristics that > would otherwise result in a combinatorial explosion? Yeah, so the alternate approach I mention in the commit message would be to have something like this: struct VmAreaRef<const MIXEDMAP: bool, const PFNMAP: bool, const MAYWRITE: bool> { ... } listing all the flags we care about. This way, we get 2^3 different types by writing just one. (There are a few different variations on how to do this, instead of bools, we may want to allow three options: true, false, unknown.) Alice
