Re: [PATCH 0/6] rust: page: Support borrowing `struct page` and physaddr conversion

[Asahi Lina]

On 2/5/25 3:39 AM, Jason Gunthorpe wrote:
> On Tue, Feb 04, 2025 at 11:33:24AM +0100, David Hildenbrand wrote:
>> On 03.02.25 10:58, Simona Vetter wrote:
>>> On Sun, Feb 02, 2025 at 10:05:42PM +0900, Asahi Lina wrote:
>>>> This series refactors the existing Page wrapper to support borrowing
>>>> `struct page` objects without ownership on the Rust side, and converting
>>>> page references to/from physical memory addresses.
>>>>
>>>> The series overlaps with the earlier submission in [1] and follows a
>>>> different approach, based on the discussion that happened there.
>>>>
>>>> The primary use case for this is implementing IOMMU-style page table
>>>> management in Rust. This allows drivers for IOMMUs and MMU-containing
>>>> SoC devices to be written in Rust (such as embedded GPUs). The intended
>>>> logic is similar to how ARM SMMU page tables are managed in the
>>>> drivers/iommu tree.
>>>>
>>>> First, introduce a concept of Owned<T> and an Ownable trait. These are
>>>> similar to ARef<T> and AlwaysRefCounted, but are used for types which
>>>> are not ref counted but rather have a single intended owner.
>>>>
>>>> Then, refactor the existing Page support to use the new mechanism. Pages
>>>> returned from the page allocator are not intended to be ref counted by
>>>> consumers (see previous discussion in [1]), so this keeps Rust's view of
>>>> page ownership as a simple "owned or not". Of course, this is still
>>>> composable as Arc<Owned<Page>> if Rust code needs to reference count its
>>>> own Page allocations for whatever reason.
>>>
>>> I think there's a bit a potential mess here because the conversion to
>>> folios isn't far enough yet that we can entirely ignore page refcounts and
>>> just use folio refcounts. But I guess we can deal with that oddity if we
>>> hit it (maybe folio conversion moves fast enough), since this only really
>>> starts to become relevant for hmm/svm gpu stuff.
>>
>> I'll note that in the future only selected things will be folios (e.g.,
>> pagecache, anonymous memory). Everything else will either get a separate
>> memdesc (e.g., ptdesc), or work on bare pages.
>>
>> Likely, when talking about page tables, "ptdesc" might be what you want to
>> allocate here, and not "folios".
> 
> I just posted a series to clean up the iommu code that this is
> cribbing the interface from: add an ioptdesc, remove all the struct
> page from the API and so forth:
> 
> https://lore.kernel.org/all/0-v1-416f64558c7c+2a5-iommu_pages_jgg@xxxxxxxxxx/
> 
> I strongly suggest that rust not expose these old schemes that will
> need another cleanup like the above. Page tables just need an
> allocator using simple void *, kmalloc is good enough for simple
> cases.
> 
> Drivers should not be exposing or touching struct page just to
> implement a page table.

iommu-pages.h looks like a private API internal to drivers/iommu. Is
there a plan to move that out so it can be used by non-iommu drivers?

If you think I should just use kmalloc, then I guess I could literally
just use KBox<PageTable> where PageTable is just a wrapper around [u64;
PTES_PER_PAGE] with an alignment attribute, or something like that. I
guess then just virt_to_phys() and back for the PTEs? So we still need
to add at least trivial wrappers to export those for Rust drivers to use.

I still need to be able to grab pointers to leaf pages for the crashdump
page table walker code though, and I want to keep the pfn and memory
type checks to make sure it won't crash trying to dereference the page
contents. So we also still need those too.

I'm not entirely sure what a higher-level Rust abstraction for this
looks like at this point, or whether it makes sense at all instead of
just trivially wrapping the C helpers... I don't particularly mind just
writing this part of the driver code "like C" (it's pretty much the
lowest-level bit of code in the driver anyway) but I guess we have to do
some thinking here.

Maybe this is one of those cases where we just do it ad-hoc for now, and
wait until a second driver that needs to implement page tables comes
around and we figure out what can be shared then, and what the API
should look like.

Hmm, starting to think maybe these should literally be impls on the
address types (PhysicalAddr::to_pfn(), PhysicalAddr::to_virt(), etc.).
Though that won't work without newtyping everything, but maybe we should?

~~ Lina