Message-Id: <DA9JTYA0EQU8.26M0ZX80FOBWY@nvidia.com>
Date: Fri, 30 May 2025 23:02:02 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Jason Gunthorpe" <jgg@...pe.ca>, "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>
Cc: <dakr@...nel.org>, <lyude@...hat.com>, "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 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>,
    }

You can then implement `SGTablePages` on anything you want to DMA map. Say a
list of pages (using newtype on purpose):

    struct PagesArray(KVec<Page>);

    impl SGTablePages for PagesArray {
        fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page> {
            self.0.iter().map(|page| unsafe { &*page.as_ptr() })
        }

        fn range(&self) -> Range<usize> {
            0..(PAGE_SIZE * self.0.len())
        }
    }

Or a pinned `VVec`:

    impl<T> SGTablePages for Pin<VVec<T>> {
        fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page> {
            // Number of pages covering `self`
            (0..self.len().next_multiple_of(PAGE_SIZE))
                .into_iter()
                // pointer to virtual address of page
                .map(|i| unsafe { self.as_ptr().add(PAGE_SIZE * i) } as *const c_void)
                // convert virtual address to page
                .map(|ptr| unsafe { &*bindings::vmalloc_to_page(ptr) })
        }

        fn range(&self) -> Range<usize> {
            0..self.len()
        }
    }

You can store these into `SGTable::pages` and ensure (unless I missed
something) that its memory stays valid, while providing the material to
initialize the `sg_table`.

`SGTable` could provide an accessor to `pages` so the CPU can read/write the
data when DMA is not active (maybe also calling `dma_sync_*` as appropriate?).
Or maybe users could put the backing object behind a smart pointer for
concurrent accesses and pass that to `SGTable`.

One nice thing with this approach is that users don't have to figure out
themselves how to obtain the page list for their buffer if it already has a
`SGTablePages` implementation, like `VVec` does.

Note that although the code above builds for me, I probably got a few things
wrong - maybe `SGTablePages` should be `unsafe`, maybe also I am misusing
`Pin`, or overlooked a few usecases that would be impossible to implement using
this scheme. Hopefully we can get more feedback to validate or reject this
idea.

Powered by blists - more mailing lists