rust: page: Add support for vmalloc_to_page

rust: page: Add support for vmalloc_to_page

[Abdiel Janulgue]

This series aims to add support for pages that are not allocated by an instance of the Page
abstraction, specifically those returned by vmalloc_to_page().

This patch series is sent in the context of developing a Nova driver WIP feature where we
load the the GSP firmware into an sg table [1].

[1] https://gitlab.freedesktop.org/abj/nova-drm

Abdiel Janulgue (3):
  rust: page: replace the page pointer wrapper with Opaque
  rust: page: Extend support to vmalloc_to_page
  rust: page: Add page_slice_to_page

 rust/kernel/page.rs | 111 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 102 insertions(+), 9 deletions(-)


base-commit: 673d1648244c3840043e09a784164b38c2e2efb9
-- 
2.34.1

[PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Abdiel Janulgue]

Replace NonNull with Opaque to make it possible to cast to a Page pointer
from a raw struct page pointer.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 208a006d587c..08ff09a25223 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -8,8 +8,9 @@
     error::code::*,
     error::Result,
     uaccess::UserSliceReader,
+    types::Opaque,
 };
-use core::ptr::{self, NonNull};
+use core::ptr::{self};
 
 /// A bitwise shift for the page size.
 pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
@@ -25,8 +26,9 @@
 /// # Invariants
 ///
 /// The pointer is valid, and has ownership over the page.
+#[repr(transparent)]
 pub struct Page {
-    page: NonNull<bindings::page>,
+    page: Opaque<bindings::page>,
 }
 
 // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
@@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
         // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
         // is always safe to call this method.
         let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
-        let page = NonNull::new(page).ok_or(AllocError)?;
+        if page.is_null() {
+            return Err(AllocError);
+        }
+        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
+        let ptr = page.cast::<Self>();
         // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
         // allocated page. We transfer that ownership to the new `Page` object.
-        Ok(Self { page })
+        // SAFETY: According to invariant above ptr is valid.
+        Ok(unsafe { ptr::read(ptr) })
     }
 
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
-        self.page.as_ptr()
+        self.page.get()
     }
 
     /// Runs a piece of code with this page mapped to an address.
@@ -245,6 +252,6 @@ pub unsafe fn copy_from_user_slice_raw(
 impl Drop for Page {
     fn drop(&mut self) {
         // SAFETY: By the type invariants, we have ownership of the page and can free it.
-        unsafe { bindings::__free_pages(self.page.as_ptr(), 0) };
+        unsafe { bindings::__free_pages(self.page.get(), 0) };
     }
 }
-- 
2.34.1

[PATCH 2/3] rust: page: Extend support to vmalloc_to_page

[Abdiel Janulgue]

Extend Page to support pages that are not allocated by the constructor, for
example, those returned by vmalloc_to_page(). Since we don't own those pages
we shouldn't Drop them either. Hence we take advantage of the switch to Opaque
so we can cast to a Page pointer from a struct page pointer and be able to
retrieve the reference on an existing struct page mapping. In this case
Drop will not be called since we are not instantiating a new Page instance.

Note that we maintain ownership semantics for self-allocated pages (e.g. Drop
still works as usual in this case).

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 08ff09a25223..a84f382daf5c 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -21,11 +21,12 @@
 /// A bitmask that gives the page containing a given address.
 pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
 
-/// A pointer to a page that owns the page allocation.
+/// A pointer to a page that may own the page allocation.
 ///
 /// # Invariants
 ///
-/// The pointer is valid, and has ownership over the page.
+/// The pointer is valid, and has ownership over the page if the page is allocated by this
+/// abstraction.
 #[repr(transparent)]
 pub struct Page {
     page: Opaque<bindings::page>,
@@ -78,6 +79,33 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
         Ok(unsafe { ptr::read(ptr) })
     }
 
+    /// This is just a wrapper to vmalloc_to_page which returns an existing page mapping, hence
+    /// we don't take ownership of the page. Returns an error if the pointer is null or if it
+    /// is not returned by vmalloc().
+    pub fn vmalloc_to_page<'a>(
+        cpu_addr: *const core::ffi::c_void
+    ) -> Result<&'a Self, AllocError>
+    {
+        if cpu_addr.is_null() {
+            return Err(AllocError);
+        }
+        // SAFETY: We've checked that the pointer is not null, so it is safe to call this method.
+        if unsafe { !bindings::is_vmalloc_addr(cpu_addr) } {
+            return Err(AllocError);
+        }
+        // SAFETY: We've initially ensured the pointer argument to this function is not null and
+        // checked for the requirement that the buffer passed to it should be allocated by vmalloc,
+        // so it is safe to call this method.
+        let page = unsafe { bindings::vmalloc_to_page(cpu_addr) };
+        if page.is_null() {
+            return Err(AllocError);
+        }
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
+        // SAFETY: We just successfully allocated a page, therefore dereferencing
+        // the page pointer is valid.
+        Ok(unsafe { &*page.cast() })
+    }
+
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
         self.page.get()
-- 
2.34.1

[PATCH 3/3] rust: page: Add page_slice_to_page

[Abdiel Janulgue]

Make it convenient to convert buffers into page-sized vmalloc'd chunks which can
then be used in vmalloc_to_page().

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index a84f382daf5c..490685144694 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -3,7 +3,7 @@
 //! Kernel page allocation and management.
 
 use crate::{
-    alloc::{AllocError, Flags},
+    alloc::{AllocError, Flags, VVec, flags::*},
     bindings,
     error::code::*,
     error::Result,
@@ -106,6 +106,27 @@ pub fn vmalloc_to_page<'a>(
         Ok(unsafe { &*page.cast() })
     }
 
+    /// A convenience wrapper to vmalloc_to_page which ensures it takes a page-sized buffer
+    /// represented by `PageSlice`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::page::*;
+    ///
+    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
+    /// let data: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2];
+    /// let buf: &[u8] = &data;
+    ///
+    /// let pages: VVec<PageSlice> = buf.try_into()?;
+    /// let page = Page::page_slice_to_page(&pages[0])?;
+    /// # Ok(()) }
+    /// ```
+    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self, AllocError>
+    {
+        Self::vmalloc_to_page(page.0.as_ptr() as _)
+    }
+
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
         self.page.get()
@@ -277,6 +298,42 @@ pub unsafe fn copy_from_user_slice_raw(
     }
 }
 
+/// A page-aligned, page-sized object. This is used for convenience to convert a large buffer into
+/// an array of page-sized chunks which can then be used in `Page::page_slice_to_page` wrapper.
+///
+// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal
+// integer argument for the `repr(align)` attribute.
+#[repr(align(4096))]
+pub struct PageSlice([u8; PAGE_SIZE]);
+
+impl TryFrom<&[u8]> for VVec<PageSlice> {
+    type Error = AllocError;
+
+    fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+        let mut k = VVec::new();
+        let aligned_len: usize;
+
+        // Ensure the size is page-aligned.
+        match core::alloc::Layout::from_size_align(val.len(), PAGE_SIZE) {
+            Ok(align) => { aligned_len = align.pad_to_align().size() },
+            Err(_) => { return Err(AllocError) },
+        };
+        let pages = aligned_len >> PAGE_SHIFT;
+        match k.reserve(pages, GFP_KERNEL) {
+            Ok(_) => {
+                // SAFETY: from above, the length should be equal to the vector's capacity
+                unsafe { k.set_len(pages); }
+                // SAFETY: src buffer sized val.len() does not overlap with dst buffer since
+                // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE.
+                unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8,
+                                                  val.len()) };
+            },
+            Err(_) => { return Err(AllocError) },
+        };
+        Ok(k)
+    }
+}
+
 impl Drop for Page {
     fn drop(&mut self) {
         // SAFETY: By the type invariants, we have ownership of the page and can free it.
-- 
2.34.1

Re: rust: page: Add support for vmalloc_to_page

[Miguel Ojeda]

On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> base-commit: 673d1648244c3840043e09a784164b38c2e2efb9

Quick note: this is a `rust-next` commit, but this looks like it is
meant to be on top of Danilo's `Allocator` series -- e.g. we don't
have e.g. `VVec` there yet so it cannot build (but we will have that
soon).

By the way, cover letters normally go with [PATCH 0/...]. Since you
have the base commit, I guess you used `--cover-letter` but changed
the title maybe?

Thanks!

Cheers,
Miguel

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Alice Ryhl]

On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Replace NonNull with Opaque to make it possible to cast to a Page pointer
> from a raw struct page pointer.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/page.rs | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index 208a006d587c..08ff09a25223 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -8,8 +8,9 @@
>      error::code::*,
>      error::Result,
>      uaccess::UserSliceReader,
> +    types::Opaque,
>  };
> -use core::ptr::{self, NonNull};
> +use core::ptr::{self};
>
>  /// A bitwise shift for the page size.
>  pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
> @@ -25,8 +26,9 @@
>  /// # Invariants
>  ///
>  /// The pointer is valid, and has ownership over the page.
> +#[repr(transparent)]
>  pub struct Page {
> -    page: NonNull<bindings::page>,
> +    page: Opaque<bindings::page>,
>  }
>
>  // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
> @@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
>          // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
>          // is always safe to call this method.
>          let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> -        let page = NonNull::new(page).ok_or(AllocError)?;
> +        if page.is_null() {
> +            return Err(AllocError);
> +        }
> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        let ptr = page.cast::<Self>();
>          // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
>          // allocated page. We transfer that ownership to the new `Page` object.
> -        Ok(Self { page })
> +        // SAFETY: According to invariant above ptr is valid.
> +        Ok(unsafe { ptr::read(ptr) })

Using `ptr::read` on the page is definitely not okay. That duplicates
the contents of the `struct page`. You'll need some sort of pointer
type around `Page` instead.

Alice

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Boqun Feng]

On Tue, Oct 08, 2024 at 08:58:56AM +0200, Alice Ryhl wrote:
> On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
> >
> > Replace NonNull with Opaque to make it possible to cast to a Page pointer
> > from a raw struct page pointer.
> >
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/page.rs | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index 208a006d587c..08ff09a25223 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -8,8 +8,9 @@
> >      error::code::*,
> >      error::Result,
> >      uaccess::UserSliceReader,
> > +    types::Opaque,
> >  };
> > -use core::ptr::{self, NonNull};
> > +use core::ptr::{self};
> >
> >  /// A bitwise shift for the page size.
> >  pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
> > @@ -25,8 +26,9 @@
> >  /// # Invariants
> >  ///
> >  /// The pointer is valid, and has ownership over the page.
> > +#[repr(transparent)]
> >  pub struct Page {
> > -    page: NonNull<bindings::page>,
> > +    page: Opaque<bindings::page>,
> >  }
> >
> >  // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
> > @@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
> >          // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
> >          // is always safe to call this method.
> >          let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> > -        let page = NonNull::new(page).ok_or(AllocError)?;
> > +        if page.is_null() {
> > +            return Err(AllocError);
> > +        }
> > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> > +        let ptr = page.cast::<Self>();
> >          // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
> >          // allocated page. We transfer that ownership to the new `Page` object.
> > -        Ok(Self { page })
> > +        // SAFETY: According to invariant above ptr is valid.
> > +        Ok(unsafe { ptr::read(ptr) })
> 
> Using `ptr::read` on the page is definitely not okay. That duplicates
> the contents of the `struct page`. You'll need some sort of pointer
> type around `Page` instead.
> 

Agreed. So may I suggest we introduce `Owned` type and `Ownable` trait
[1]? `alloc_page()` can be refactor to return a `Result<Owned<Self>,
AllocError>`.

[1]: https://lore.kernel.org/rust-for-linux/ZnCzLIly3DRK2eab@boqun-archlinux/

Regards,
Boqun

> Alice

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Abdiel Janulgue]

On 08/10/2024 10:04, Boqun Feng wrote:
> On Tue, Oct 08, 2024 at 08:58:56AM +0200, Alice Ryhl wrote:
>> On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
>> <abdiel.janulgue@gmail.com> wrote:
>>>
>>> Replace NonNull with Opaque to make it possible to cast to a Page pointer
>>> from a raw struct page pointer.
>>>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>>> ---
>>>   rust/kernel/page.rs | 19 +++++++++++++------
>>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
>>> index 208a006d587c..08ff09a25223 100644
>>> --- a/rust/kernel/page.rs
>>> +++ b/rust/kernel/page.rs
>>> @@ -8,8 +8,9 @@
>>>       error::code::*,
>>>       error::Result,
>>>       uaccess::UserSliceReader,
>>> +    types::Opaque,
>>>   };
>>> -use core::ptr::{self, NonNull};
>>> +use core::ptr::{self};
>>>
>>>   /// A bitwise shift for the page size.
>>>   pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
>>> @@ -25,8 +26,9 @@
>>>   /// # Invariants
>>>   ///
>>>   /// The pointer is valid, and has ownership over the page.
>>> +#[repr(transparent)]
>>>   pub struct Page {
>>> -    page: NonNull<bindings::page>,
>>> +    page: Opaque<bindings::page>,
>>>   }
>>>
>>>   // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
>>> @@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
>>>           // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
>>>           // is always safe to call this method.
>>>           let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
>>> -        let page = NonNull::new(page).ok_or(AllocError)?;
>>> +        if page.is_null() {
>>> +            return Err(AllocError);
>>> +        }
>>> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
>>> +        let ptr = page.cast::<Self>();
>>>           // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
>>>           // allocated page. We transfer that ownership to the new `Page` object.
>>> -        Ok(Self { page })
>>> +        // SAFETY: According to invariant above ptr is valid.
>>> +        Ok(unsafe { ptr::read(ptr) })
>>
>> Using `ptr::read` on the page is definitely not okay. That duplicates
>> the contents of the `struct page`. You'll need some sort of pointer
>> type around `Page` instead.
>>
> 
> Agreed. So may I suggest we introduce `Owned` type and `Ownable` trait
> [1]? `alloc_page()` can be refactor to return a `Result<Owned<Self>,
> AllocError>`.
> 
> [1]: https://lore.kernel.org/rust-for-linux/ZnCzLIly3DRK2eab@boqun-archlinux/

Thanks for the feedback. How do you propose we move forward, do I take a 
stab at implementing `Owned` type and `Ownable` trait?

Regards,
Abdiel


> 
> Regards,
> Boqun
> 
>> Alice
> 

Re: rust: page: Add support for vmalloc_to_page

[Abdiel Janulgue]

On 07/10/2024 23:58, Miguel Ojeda wrote:
> On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> base-commit: 673d1648244c3840043e09a784164b38c2e2efb9
> 
> Quick note: this is a `rust-next` commit, but this looks like it is
> meant to be on top of Danilo's `Allocator` series -- e.g. we don't
> have e.g. `VVec` there yet so it cannot build (but we will have that
> soon).
> 
> By the way, cover letters normally go with [PATCH 0/...]. Since you
> have the base commit, I guess you used `--cover-letter` but changed
> the title maybe?
> 

Sorry about that, my bad, I edited the cover by mistake. Will fix this 
in next update.

Abdiel

> Thanks!
> 
> Cheers,
> Miguel

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Boqun Feng]

On Tue, Oct 08, 2024 at 02:29:47PM +0300, Abdiel Janulgue wrote:
> 
> 
> On 08/10/2024 10:04, Boqun Feng wrote:
> > On Tue, Oct 08, 2024 at 08:58:56AM +0200, Alice Ryhl wrote:
> > > On Mon, Oct 7, 2024 at 10:28 PM Abdiel Janulgue
> > > <abdiel.janulgue@gmail.com> wrote:
> > > > 
> > > > Replace NonNull with Opaque to make it possible to cast to a Page pointer
> > > > from a raw struct page pointer.
> > > > 
> > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > > > ---
> > > >   rust/kernel/page.rs | 19 +++++++++++++------
> > > >   1 file changed, 13 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > > > index 208a006d587c..08ff09a25223 100644
> > > > --- a/rust/kernel/page.rs
> > > > +++ b/rust/kernel/page.rs
> > > > @@ -8,8 +8,9 @@
> > > >       error::code::*,
> > > >       error::Result,
> > > >       uaccess::UserSliceReader,
> > > > +    types::Opaque,
> > > >   };
> > > > -use core::ptr::{self, NonNull};
> > > > +use core::ptr::{self};
> > > > 
> > > >   /// A bitwise shift for the page size.
> > > >   pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
> > > > @@ -25,8 +26,9 @@
> > > >   /// # Invariants
> > > >   ///
> > > >   /// The pointer is valid, and has ownership over the page.
> > > > +#[repr(transparent)]
> > > >   pub struct Page {
> > > > -    page: NonNull<bindings::page>,
> > > > +    page: Opaque<bindings::page>,
> > > >   }
> > > > 
> > > >   // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
> > > > @@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
> > > >           // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
> > > >           // is always safe to call this method.
> > > >           let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> > > > -        let page = NonNull::new(page).ok_or(AllocError)?;
> > > > +        if page.is_null() {
> > > > +            return Err(AllocError);
> > > > +        }
> > > > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> > > > +        let ptr = page.cast::<Self>();
> > > >           // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
> > > >           // allocated page. We transfer that ownership to the new `Page` object.
> > > > -        Ok(Self { page })
> > > > +        // SAFETY: According to invariant above ptr is valid.
> > > > +        Ok(unsafe { ptr::read(ptr) })
> > > 
> > > Using `ptr::read` on the page is definitely not okay. That duplicates
> > > the contents of the `struct page`. You'll need some sort of pointer
> > > type around `Page` instead.
> > > 
> > 
> > Agreed. So may I suggest we introduce `Owned` type and `Ownable` trait
> > [1]? `alloc_page()` can be refactor to return a `Result<Owned<Self>,
> > AllocError>`.
> > 
> > [1]: https://lore.kernel.org/rust-for-linux/ZnCzLIly3DRK2eab@boqun-archlinux/
> 
> Thanks for the feedback. How do you propose we move forward, do I take a
> stab at implementing `Owned` type and `Ownable` trait?

If you're interested, go ahead ;-)

Regards,
Boqun

> 
> Regards,
> Abdiel
> 
> 
> > 
> > Regards,
> > Boqun
> > 
> > > Alice
> > 

Re: [PATCH 3/3] rust: page: Add page_slice_to_page

[kernel test robot]

Hi Abdiel,

kernel test robot noticed the following build errors:

[auto build test ERROR on rust/rust-next]
[also build test ERROR on linus/master v6.12-rc2 next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abdiel-Janulgue/rust-page-Extend-support-to-vmalloc_to_page/20241008-052805
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20241007202752.3096472-4-abdiel.janulgue%40gmail.com
patch subject: [PATCH 3/3] rust: page: Add page_slice_to_page
config: x86_64-randconfig-014-20241011 (https://download.01.org/0day-ci/archive/20241011/202410111810.hiN1ns3g-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241011/202410111810.hiN1ns3g-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410111810.hiN1ns3g-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0432]: unresolved import `crate::alloc::VVec`
   --> rust/kernel/page.rs:6:32
   |
   6 |     alloc::{AllocError, Flags, VVec, flags::*},
   |                                ^^^^ no `VVec` in `alloc`

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Fiona Behrens]

On 7 Oct 2024, at 22:27, Abdiel Janulgue wrote:

> Replace NonNull with Opaque to make it possible to cast to a Page pointer
> from a raw struct page pointer.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/page.rs | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index 208a006d587c..08ff09a25223 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -8,8 +8,9 @@
>      error::code::*,
>      error::Result,
>      uaccess::UserSliceReader,
> +    types::Opaque,
>  };
> -use core::ptr::{self, NonNull};
> +use core::ptr::{self};
>
>  /// A bitwise shift for the page size.
>  pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
> @@ -25,8 +26,9 @@
>  /// # Invariants
>  ///
>  /// The pointer is valid, and has ownership over the page.
> +#[repr(transparent)]
>  pub struct Page {
> -    page: NonNull<bindings::page>,
> +    page: Opaque<bindings::page>,

Still not to sure where to encode pinning in the type api. Looking into the C struct I see a union that sometimes holds a list head, should this then be a pinned thing in this type?

 Fiona

>  }
>
>  // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
> @@ -65,15 +67,20 @@ pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
>          // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
>          // is always safe to call this method.
>          let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> -        let page = NonNull::new(page).ok_or(AllocError)?;
> +        if page.is_null() {
> +            return Err(AllocError);
> +        }
> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        let ptr = page.cast::<Self>();
>          // INVARIANT: We just successfully allocated a page, so we now have ownership of the newly
>          // allocated page. We transfer that ownership to the new `Page` object.
> -        Ok(Self { page })
> +        // SAFETY: According to invariant above ptr is valid.
> +        Ok(unsafe { ptr::read(ptr) })
>      }
>
>      /// Returns a raw pointer to the page.
>      pub fn as_ptr(&self) -> *mut bindings::page {
> -        self.page.as_ptr()
> +        self.page.get()
>      }
>
>      /// Runs a piece of code with this page mapped to an address.
> @@ -245,6 +252,6 @@ pub unsafe fn copy_from_user_slice_raw(
>  impl Drop for Page {
>      fn drop(&mut self) {
>          // SAFETY: By the type invariants, we have ownership of the page and can free it.
> -        unsafe { bindings::__free_pages(self.page.as_ptr(), 0) };
> +        unsafe { bindings::__free_pages(self.page.get(), 0) };
>      }
>  }
> -- 
> 2.34.1

Re: [PATCH 1/3] rust: page: replace the page pointer wrapper with Opaque

[Alice Ryhl]

On Fri, Oct 11, 2024 at 1:07 PM Fiona Behrens <me@kloenk.dev> wrote:
>
>
>
> On 7 Oct 2024, at 22:27, Abdiel Janulgue wrote:
>
> > Replace NonNull with Opaque to make it possible to cast to a Page pointer
> > from a raw struct page pointer.
> >
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/page.rs | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index 208a006d587c..08ff09a25223 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -8,8 +8,9 @@
> >      error::code::*,
> >      error::Result,
> >      uaccess::UserSliceReader,
> > +    types::Opaque,
> >  };
> > -use core::ptr::{self, NonNull};
> > +use core::ptr::{self};
> >
> >  /// A bitwise shift for the page size.
> >  pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
> > @@ -25,8 +26,9 @@
> >  /// # Invariants
> >  ///
> >  /// The pointer is valid, and has ownership over the page.
> > +#[repr(transparent)]
> >  pub struct Page {
> > -    page: NonNull<bindings::page>,
> > +    page: Opaque<bindings::page>,
>
> Still not to sure where to encode pinning in the type api. Looking into the C struct I see a union that sometimes holds a list head, should this then be a pinned thing in this type?

As long as you only hand out immutable references to it, nothing
further is necessary.

Alice