Message-Id: <D9VWA9ZQLY85.277DFA3YTH5R0@nvidia.com>
Date: Wed, 14 May 2025 21:50:15 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alexandre Courbot" <acourbot@...dia.com>, "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>, <dakr@...nel.org>, <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" <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: [RFC PATCH 1/2] rust: add initial scatterlist bindings

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.

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.

Powered by blists - more mailing lists