Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Dec 2, 2024 at 1:03 PM Asahi Lina <lina@asahilina.net> wrote:
>
> On 11/19/24 8:24 PM, Abdiel Janulgue wrote:
> > This series aims to add support for pages that are not constructed by an
> > instance of the rust Page abstraction, for example those returned by
> > vmalloc_to_page() or virt_to_page().
> >
> > Changes sinve v3:
> > - Use the struct page's reference count to decide when to free the
> >   allocation (Alice Ryhl, Boqun Feng).
> > - Make Page::page_slice_to_page handle virt_to_page cases as well
> >   (Danilo Krummrich).
> > - Link to v2: https://lore.kernel.org/lkml/20241022224832.1505432-1-abdiel.janulgue@gmail.com/
> >
> > Changes since v2:
> > - Use Owned and Ownable types for constructing Page as suggested in
> >   instad of using ptr::read().
> > - Link to v1: https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/
> >
> > Abdiel Janulgue (2):
> >   rust: page: use the page's reference count to decide when to free the
> >     allocation
> >   rust: page: Extend support to existing struct page mappings
> >
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/helpers/page.c             |  20 +++++
> >  rust/kernel/page.rs             | 135 ++++++++++++++++++++++++++++----
> >  3 files changed, 142 insertions(+), 14 deletions(-)
> >
> >
> > base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
>
> Just wanted to comment on an upcoming use case I have that will need
> this, to make sure we're aligned. I want to use the page allocator to
> manage GPU page tables (currently done via an io-pgtable patch and
> abstraction but that's going away because it turned out to be too
> intrusive to upstream).
>
> Since I'm dealing with page tables which are their own tree ownership
> structure, and I don't want to duplicate management of the page life
> cycles, this means I need to be able to:
>
> - Convert a Rust-allocated and owned page *into* its physical address
> (page_to_phys()).
> - Convert a physical address *into* a Rust-allocated and owned page
> (phys_to_page()).
> - Borrow a Rust Page from a physical address (so I can do read/write
> operations on its data without intending to destroy it).
>
> Conceptually, the first two are like ARef::into_raw() and
> ARef::from_raw() (or Box for that matter), while the third would
> basically return a &Page with an arbitrary lifetime (up to the caller to
> enforce the rules). The latter two would be unsafe functions by nature,
> of course.
>
> I think this would work just as well with some kind of Owned/Ownable
> solution. Basically, I just need to be able to express the two concepts
> of "Page owned and allocated by Rust" and "Page borrowed from a physical
> address".

I actually think the Owned/Ownable solution is even better for what
you need, because having a borrowed reference to the current Page
abstraction is pretty awkward as it assumes that the page is always
owned.

Alice

> This maps to pagetable management like this:
> - On PT allocation, a Page is allocated, cleared, and turned into its
> physical address (to be populated in the parent PTE or top-level TTB)
> - On PT free, a page physical address is converted back to a Page, its
> PTEs are walked to recursively free child PTs or verify they are empty
> entries for leaf PTs (invariant: no leaf PTEs, all mappings should be
> removed before PT free) and dropped.
> - On PT walk/PTE insertion and removal, a physical address is borrowed
> as a Page, then `Page::with_page_mapped()` is used to perform R/W
> operations on the PTEs contained within.
>
> Tying the lifetime of actual leaf data pages mapped into the page table
> to the page table itself is a higher-level concern that isn't relevant
> here, drm_gpuvm handles that part and those pages are not allocated
> directly via the page allocator, but rather as GEM objects which
> ultimately come from shmem)
>
> (Note: this hardware is always 64-bit without highmem so those concerns
> don't apply here.)
>
> ~~ Lina
>