Message-Id: <DBTIUE9O5HWS.2SMVEMRGNCN5A@nvidia.com>
Date: Mon, 04 Aug 2025 18:04:53 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Robin Murphy" <robin.murphy@....com>, "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>, <dakr@...nel.org>, <jgg@...pe.ca>,
 <lyude@...hat.com>
Cc: "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" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Tamir Duberstein" <tamird@...il.com>,
 "FUJITA Tomonori" <fujita.tomonori@...il.com>, "open list"
 <linux-kernel@...r.kernel.org>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Randy Dunlap" <rdunlap@...radead.org>,
 "Herbert Xu" <herbert@...dor.apana.org.au>, "Caleb Sander Mateos"
 <csander@...estorage.com>, "Petr Tesarik" <petr@...arici.cz>, "Sui
 Jingfeng" <sui.jingfeng@...ux.dev>, "Marek Szyprowski"
 <m.szyprowski@...sung.com>, <airlied@...hat.com>, "open list:DMA MAPPING
 HELPERS" <iommu@...ts.linux.dev>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] rust: add initial scatterlist abstraction

On Sat Aug 2, 2025 at 3:26 AM JST, Robin Murphy wrote:
> On 2025-07-18 11:33 am, Abdiel Janulgue wrote:
> [...]
>> +impl<T, M> SGTable<T, M>
>> +where
>> +    T: Borrow<bindings::sg_table>,
>> +    M: MappedState,
>> +{
>> +    /// Returns an immutable iterator over the scatter-gather table.
>> +    pub fn iter(&self) -> SGTableIter<'_> {
>> +        SGTableIter {
>> +            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
>> +            pos: Some(unsafe { SGEntry::as_ref(self.table.borrow().sgl) }),
>> +        }
>> +    }
>> +}
>> +
>> +/// Provides a list of pages that can be used to build a `SGTable`.
>> +pub trait SGTablePages {
>> +    /// Returns an iterator to the pages providing the backing memory of `self`.
>
> This seems a bit unclear. In the C API we have 4 distinct iterators:
>
> for_each_sgtable_sg
> for_each_sgtable_dma_sg
> for_each_sgtable_page
> for_each_sgtable_dma_page
>
> The first one is for iterating the segments in CPU space, i.e. simply 
> each SGEntry, "orig_nents" times from the start or until sg_next() is 
> NULL. This is typically what you want for copying data in and out.
>
> The second is for iterating the DMA segments once mapped, which is then 
> "nents" times from the start or whichever of sg_next() being NULL or 
> sg_dma_len() being 0 happens first (since "nents" <= "orig_nents"). This 
> is typically what you want for building your actual device 
> descriptors/commands from the (dma_address,dma_length) tuples.
>
> The last two are for slightly more specialised users who want to walk 
> the whole list effectvely linearly in literal PAGE_SIZE steps, in either 
> CPU space or (mapped) DMA space respectively.
>
> The comments here rather sound like they're trying to describe #3, but 
> (based on looking at the C calls and taking a vague guess at what all 
> the Rust around them means) the implementation below sure *seems* a lot 
> more like it's actually #1... For a useful abstraction I'd think you'd 
> want at least #1 and #2, however I know the main motivation here is GPU 
> drivers, and they are the primary users of #3 and #4, so I suspect 
> you'll probably want to support both per-SGEntry and per-Page iteration, 
> in both CPU and DMA flavours, sooner rather than later.

This trait is actually to provide a list of pages in order to build a
new SG table, so IIUC #3 is the correct behavior here (for as much as
the comparison holds, since there is no SG table yet when it is called).

As for actual iterators on existing SG tables, it looks like we are
heading towards device-mapping them directly at construction time for
simplicity, since we have found no use yet for non-mapped SG tables.
This should simplify things quite a bit, and in this case the iterator
would implement #2 (and we might add a different one for #1 if the need
arises).

>
>> +    ///
>> +    /// Implementers should return an iterator which provides information regarding each page entry to
>> +    /// build the `SGTable`. The first element in the tuple is a reference to the Page, the second element
>> +    /// as the offset into the page, and the third as the length of data. The fields correspond to the
>> +    /// first three fields of the C `struct scatterlist`.
>> +    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)>;
>> +
>> +    /// Returns the number of pages in the list.
>> +    fn entries(&self) -> usize;
>> +}
>> +
>> +/// An iterator through `SGTable` entries.
>> +pub struct SGTableIter<'a> {
>> +    pos: Option<&'a SGEntry>,
>> +}
>> +
>> +impl<'a> Iterator for SGTableIter<'a> {
>> +    type Item = &'a SGEntry;
>> +
>> +    fn next(&mut self) -> Option<Self::Item> {
>> +        let entry = self.pos;
>> +        // SAFETY: `sg` is an immutable reference and is equivalent to `scatterlist` via its type
>> +        // invariants, so its safe to use with sg_next.
>> +        let next = unsafe { bindings::sg_next(self.pos?.as_raw()) };
>> +
>> +        // SAFETY: `sg_next` returns either a valid pointer to a `scatterlist`, or null if we
>> +        // are at the end of the scatterlist.
>> +        self.pos = (!next.is_null()).then(|| unsafe { SGEntry::as_ref(next) });
>> +        entry
>> +    }
>> +}
>> +
>> +impl<'a, T, M> IntoIterator for &'a SGTable<T, M>
>> +where
>> +    T: Borrow<bindings::sg_table>,
>> +    M: MappedState,
>> +{
>> +    type Item = &'a SGEntry;
>> +    type IntoIter = SGTableIter<'a>;
>> +
>> +    fn into_iter(self) -> Self::IntoIter {
>> +        self.iter()
>> +    }
>> +}
>> +
>> +impl<T> SGTable<T, Unmapped>
>> +where
>> +    T: BorrowMut<bindings::sg_table>,
>> +{
>> +    /// Map this scatter-gather table describing a buffer for DMA by the `Device`.
>> +    ///
>> +    /// To prevent the table from being mapped more than once, this call consumes `self` and transfers
>> +    /// ownership of resources to the new `SGTable<_, ManagedMapping>` object.
>> +    pub fn dma_map(
>> +        mut self,
>> +        dev: &Device<Bound>,
>> +        dir: DmaDataDirection,
>> +    ) -> Result<SGTable<T, ManagedMapping>> {
>> +        // SAFETY: Invariants on `Device<Bound>` and `SGTable` ensures that the pointers are valid.
>> +        let ret = unsafe {
>> +            bindings::dma_map_sgtable(
>> +                dev.as_raw(),
>> +                self.table.borrow_mut(),
>> +                dir as i32,
>> +                bindings::DMA_ATTR_NO_WARN as usize,
>
> This should probably be up to the user rather than baked into the 
> abstraction. For streaming mappings (as opposed to coherent 
> allocations), NO_WARN only has any effect at all for PowerPC and SWIOTLB 
> anyway. And for the latter, the warning is commonly hit by unexpectedly 
> trying to bounce an inappropriately large buffer due to not setting a 
> DMA mask correctly, so more often than not it is helpful.
>
> Users trying cleverer things like i915, where they anticipate failure 
> and can deal with it non-fatally, should of course still have the option 
> to request NO_WARN if they want, but I'd expect them to be in the minority.

Indeed, we can probably take that as a parameter.

Powered by blists - more mailing lists