Message-Id: <DAED5BUK7TUQ.4JRHFMWZ99W3@nvidia.com>
Date: Thu, 05 Jun 2025 14:51:05 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.com>, "Jason Gunthorpe" <jgg@...pe.ca>,
 "Abdiel Janulgue" <abdiel.janulgue@...il.com>
Cc: <dakr@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Valentin Obst"
 <kernel@...entinobst.de>, "open list" <linux-kernel@...r.kernel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>, "Robin Murphy"
 <robin.murphy@....com>, <airlied@...hat.com>,
 <rust-for-linux@...r.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>, "Petr Tesarik" <petr@...arici.cz>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Herbert Xu" <herbert@...dor.apana.org.au>,
 "Sui Jingfeng" <sui.jingfeng@...ux.dev>, "Randy Dunlap"
 <rdunlap@...radead.org>, "Michael Kelley" <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings

On Thu Jun 5, 2025 at 3:21 AM JST, Lyude Paul wrote:
> On Fri, 2025-05-30 at 23:02 +0900, Alexandre Courbot wrote:
>> On Thu May 29, 2025 at 9:45 AM JST, Jason Gunthorpe wrote:
>> > On Thu, May 29, 2025 at 01:14:05AM +0300, Abdiel Janulgue wrote:
>> > > +impl SGEntry<Unmapped> {
>> > > +    /// Set this entry to point at a given page.
>> > > +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
>> > > +        let c: *mut bindings::scatterlist = self.0.get();
>> > > +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
>> > > +        // `Page` invariant also ensures the pointer is valid.
>> > > +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
>> > > +    }
>> > > +}
>> > 
>> > Wrong safety statement. sg_set_page captures the page.as_ptr() inside
>> > the C datastructure so the caller must ensure it holds a reference on
>> > the page while it is contained within the scatterlist.
>> > 
>> > Which this API doesn't force to happen.
>> > 
>> > Most likely for this to work for rust you have to take a page
>> > reference here and ensure the page reference is put back during sg
>> > destruction. A typical normal pattern would 'move' the reference from
>> > the caller into the scatterlist.
>> 
>> As Jason mentioned, we need to make sure that the backing pages don't get
>> dropped while the `SGTable` is alive. The example provided unfortunately fails
>> to do that:
>> 
>>     let sgt = SGTable::alloc_table(4, GFP_KERNEL)?;
>>     let sgt = sgt.init(|iter| {
>>         for sg in iter {
>>             sg.set_page(&Page::alloc_page(GFP_KERNEL)?, PAGE_SIZE as u32, 0);
>>         }
>>         Ok(())
>>     })?;
>> 
>> Here the allocated `Page`s are dropped immediately after their address is
>> written by `set_page`, giving the device access to memory that may now be used
>> for completely different purposes. As long as the `SGTable` exists, the memory
>> it points to must not be released or reallocated in any way.
>> 
>> To that effect, we could simply store the `Page`s into the `SGTable`, but that
>> would cover only one of the many ways they can be constructed. For instance we
>> may want to share a `VVec` with a device and this just won't allow doing it.
>> 
>> So we need a way to keep the provider of the pages alive into the `SGTable`,
>> while also having a convenient way to get its list of pages. Here is rough idea
>> for doing this, it is very crude and probably not bulletproof but hopefully it
>> can constitute a start.
>> 
>> You would have a trait for providing the pages and their range:
>> 
>>     /// Provides a list of pages that can be used to build a `SGTable`.
>>     trait SGTablePages {
>>         /// Returns an iterator to the pages providing the backing memory of `self`.
>>         fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page>;
>>         /// Returns the effective range of the mapping.
>>         fn range(&self) -> Range<usize>;
>>     }
>> 
>> The `SGTable` becomes something like:
>> 
>>     struct SGTable<P: SGTablePages, T: MapState>
>>     {
>>         table: Opaque<bindings::sg_table>,
>>         pages: P,
>>         _s: PhantomData<T>,
>>     }
>
> Hopefully I'm not missing anything here but - I'm not sure how I feel about
> this making assumptions about the memory layout of an sg_table beyond just
> being a struct sg_table. For instance, in the gem shmem helpers I had this for
> exposing the SGTable that is setup for gem shmem objects:
>
> struct OwnedSGTable<T: drm::gem::shmem::DriverObject> {
>     sg_table: NonNull<SGTable>
>     _owner: ARef<Object<T>>
> }
>
> So, I'm not really sure we have any reasonable representation for P here as we
> don't handle the memory allocation for the SGTable.

Maybe I need more context to understand your problem, but the point of
this design is precisely that it doesn't make any assumption about the
memory layout - all `P` needs to do is provide the pages describing the
memory backing.

Assuming that `_owner` here is the owner of the memory, couldn't you
flip your data layout and pass `_owner` (or rather a newtype wrapping
it) to `SGTable`, thus removing the need for a custom type?

Powered by blists - more mailing lists