Re: [RFC PATCH 1/2] rust: add initial scatterlist bindings

[Abdiel Janulgue]

On 14/05/2025 15:50, Alexandre Courbot wrote:
> On Wed May 14, 2025 at 5:29 PM JST, Alexandre Courbot wrote:
> > > +/// The base interface for a scatter-gather table of DMA address spans.
> > > +///
> > > +/// This structure represents the Rust abstraction for a C `struct sg_table`. This implementation
> > > +/// abstracts the usage of an already existing C `struct sg_table` within Rust code that we get
> > > +/// passed from the C side.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// The `sg_table` pointer is valid for the lifetime of an SGTable instance.
> > > +#[repr(transparent)]
> > > +pub struct SGTable(Opaque<bindings::sg_table>);
> > > +
> > > +impl SGTable {
> > > +    /// Convert a raw `struct sg_table *` to a `&'a SGTable`.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is initialized and valid for
> > > +    /// the lifetime of the returned reference.
> > > +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
> > > +        // SAFETY: Guaranteed by the safety requirements of the function.
> > > +        unsafe { &*ptr.cast() }
> > > +    }
> > > +
> > > +    /// Obtain the raw `struct sg_table *`.
> > > +    pub fn as_raw(&self) -> *mut bindings::sg_table {
> > > +        self.0.get()
> > > +    }
> > > +
> > > +    /// Returns a mutable iterator over the scather-gather table.
> > > +    pub fn iter_mut(&mut self) -> SGTableIterMut<'_> {
> > > +        SGTableIterMut {
> > > +            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
> > > +            pos: Some(unsafe { SGEntry::as_mut((*self.0.get()).sgl) }),
> > > +        }
> > > +    }
> > > +
> > > +    /// Returns an iterator over the scather-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.0.get()).sgl) }),
> > > +        }
> > > +    }
> >
> > I think Jason mentioned this already, but you should really have two
> > iterators, one for the CPU side and one for the device side. The two
> > lists are not even guaranteed to be the same size IIUC, so having both
> > lists in the same iterator is a receipe for confusion and bugs.
> >
> > I have an (absolutely awful) implementation of that if you want to take
> > a look:
> >
> > https://github.com/Gnurou/linux/blob/nova-gsp/drivers/gpu/nova-core/firmware/radix3.rs#L200
> >
> > It's probably wrong in many places, and I just wrote it as a temporary
> > alternative until this series lands, but please steal any idea that you
> > think is reusable.
> >
> > There is also the fact that SG tables are not always necessarily mapped
> > on the device side, so we would have to handle that as well, e.g.
> > through a typestate or maybe by just returning a dedicated error in that
> > case.
>
> Gave this some more thought, and basically it appears this is a
> two-parts problem:
>
> 1) Iterating over an already-existing sg_table (which might have been
>     created by your `as_ref` function, although as Daniel suggested it
>     needs a better name),
> 2) Building a sg_table.
>
> The C API for both is a bit quirky, but 1) looks the most pressing to
> address and should let us jump to 2) with a decent base.
>
> Since an sg_table can exist in two states (mapped or unmapped), I think
> it is a good candidate for the typestate pattern, i.e. `SgTable` can be
> either `SgTable<Unmapped>` or `SgTable<Mapped>`, the state allowing us
> to limit the availability of some methods. For instance, an iterator
> over the DMA addresses only makes sense in the `Mapped` state.
>
> A `SgTable<Unmapped>` can turn into a `SgTable<Mapped>` through its
> `map(self, device: &Device)` method (and vice-versa via an `unmap`
> method for `SgTable<Mapped>`. This has the benefit of not binding the
> `SgTable` to a device until we need to map it. `SgTable<Unmapped>` could
> also implement `Clone` for convenience, but not `SgTable<Mapped>`.
>
> Then there are the iterators. All SgTables can iterate over the CPU
> addresses, but only `SgTable<Mapped>` provides a DMA addresses iterator.
> The items for each iterator would be their own type, containing only the
> information needed (or references to the appropriate fields of the
> `struct scatterlist`).
>
> Mapped tables should be immutable, so a mutable iterator to CPU
> addresses would only be provided in the `Unmapped` state - if we want
> to allow mutability at all.

Good suggestions, I have a quick PoC based on this and this actually 
works. Need to clean it up a bit for v2.

/Abdiel

> Because the tricky part of building or modifying a SG table is
> preventing it from reaching an invalid state. I don't have a good idea
> yet of how this should be done, and there are many different ways to
> build a SG table - one or several builder types can be involved here,
> that output the `SgTable` in their final stage. Probably people more
> acquainted with the scatterlist API have ideas.