On Fri, Aug 2, 2024 at 11:39 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote: > > On 02.08.24 09:38, Alice Ryhl wrote: > > This is a follow-up to the page abstractions [1] that were recently > > merged in 6.11. Rust Binder will need these abstractions to manipulate > > the vma in its implementation of the mmap fop on the Binder file. > > > > This patch is based on Wedson's implementation on the old rust branch, > > but has been changed significantly. All mistakes are Alice's. > > > > Link: https://lore.kernel.org/r/20240528-alice-mm-v7-4-78222c31b8f4@xxxxxxxxxx [1] > > Co-developed-by: Wedson Almeida Filho <wedsonaf@xxxxxxxxx> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@xxxxxxxxx> > > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > > I have one nit below, with that fixed: > > Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx> > > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > > new file mode 100644 > > index 000000000000..ec8cadb09626 > > --- /dev/null > > +++ b/rust/kernel/mm/virt.rs > > @@ -0,0 +1,204 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +// Copyright (C) 2024 Google LLC. > > + > > +//! Virtual memory. > > + > > +use crate::{ > > + bindings, > > + error::{to_result, Result}, > > + page::Page, > > + types::Opaque, > > +}; > > + > > +/// A wrapper for the kernel's `struct vm_area_struct`. > > +/// > > +/// It represents an area of virtual memory. > > +/// > > +/// # Invariants > > +/// > > +/// * If the caller has shared access to this type, then they must hold the mmap read lock. > > +/// * If the caller has exclusive access to this type, then they must hold the mmap write lock. > > +#[repr(transparent)] > > +pub struct VmArea { > > + vma: Opaque<bindings::vm_area_struct>, > > +} > > + > > +impl VmArea { > > + /// 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 write lock) is held for at least the duration of 'a. > > + #[inline] > > + pub unsafe fn from_raw_vma<'a>(vma: *const bindings::vm_area_struct) -> &'a Self { > > I think this also should be named `from_raw`. > > I took a look at your conversation with Christian Brauner and I > personally don't see the benefit of `File::from_raw_file` over > `File::from_raw`. To me it's clear that this function takes some raw C > structure that represents a `File` and turns it into a `File`. > In the situation where there are multiple ways of creating something > from different C structs, I think we should include the name. But if > there is only one possible struct, then the name should be `from_raw`. > > Do you think we should re-open that discussion/start a new one on a > naming convention for this? Oh, I actually intended to rename it here too. I just forgot that VmArea also had a from_raw. Alice
