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

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

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.