Re: [PATCH v3 1/2] rust: add initial scatterlist abstraction

["Alexandre Courbot" <acourbot@nvidia.com>]

Hi Lyude,

On Fri Jul 25, 2025 at 5:19 AM JST, Lyude Paul wrote:
<snip>
>> +/// Trait implemented by all mapping states.
>> +pub trait MappingState {}
>
> We should make sure this is Sealed so that crates don't try to add
> MappingStates that we're not expecting

Agreed.

<snip>
>> +
>> +/// 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.
>> +pub struct SGTable<T: Borrow<bindings::sg_table>, M: MappingState> {
>> +    /// Mapping state of the underlying `struct sg_table`.
>> +    ///
>> +    /// This defines which methods of `SGTable` are available.
>> +    ///
>> +    /// Declared first so it is dropped before `table`, so we remove the mapping before freeing the
>> +    /// SG table if the latter is owned.
>> +    _mapping: M,
>> +
>> +    /// Something that can borrow the underlying `struct sg_table`.
>> +    table: T,
>> +}
>
> So - I notice that instead of having `SGTable` just hold the `struct sg_table`
> as an Opaque<> we're now relying on wrapping the SGTable around another object
> that defines it's own way of returning an sg_table.
>
> Are we sure we want this? There's a few issues I see here, the first being
> that we're now providing the ability to acquire an immutable reference to a C
> struct to all users (including ones outside of kernel crates) of SGTable.

Note that the Rust `SGTable` itself does not provide access to the
underlying `sg_table` - only the `T` parameter, for internal use - it
isn't exposed to clients of this API.

> Maybe this isn't that bad, but IMO I've always tried to avoid ever exposing
> anything from bindings outside of kernel crates - with the exception of types
> like c_uint and such. My reason has generally keeping things separate, but
> also making sure people don't default to trying to use these structs directly
> in driver code for functionality that they may just want to add into the rust
> bindings.

Hopefully that's not the case, but please feel free to elaborate if I
missed your point. Or maybe the problem is with the fact that
`bindings::sg_table` bleeds into the public type of `SGTable`?

>
> The second being it just feels a bit backwards to me - I'd intuitively expect
> an object to embed an SGTable if it's adding additional functionality on top
> of it, rather than the SGTable embedding the object. Especially considering
> that not all users concerned with SGTables that they get from C code will even
> want their own wrapper type and may just want to return a plain &SGTable.

Mmm I have to admit I am a bit lost here. But FWIW, you can get close to
a plain `&SGTable` by creating a `SGTable<&bindings::sg_table>` - since
`&bindings::sg_table` implements `Borrow<bindings::sg_table>`, this
works as one would expect.

Layout-wise it should even come down to the same, as then the `SGTable`
is just made of a reference to the `sg_table`, the `MappingState` being
a ZST for these cases - so even if you cannot simply cast a pointer to
this type, at the end of the day the result should be identical.

>
> However - if we're expecting implementors of `Borrow<bindings::sg_table>` to
> potentially need to grab locks or such whenever they need to access the
> sg_table, then I suppose Borrow<> makes a lot more sense here and this design
> is probably fine without being inverted.

TBH that's not what I had in mind - I wanted to make sure we can support
both the owned and borrowed scenarios with the same base type.

>
> Also BTW: I assume this might be clear to you already but in case it isn't,
> with the design I suggested above you'd still be able to safely cast from
> `*mut/*const bindings::sg_table` to `&mut/& SGTable<M: MappingState>` since

Is there a benefit to being able to do a cast vs. the construct I
described above?

> MappingState is a ZST. As long as you can guarantee the struct is the same
> size as the one on the C side, you should be good. You might want to add a
> #[repr(transparent)] and a type invariant in the comments mentioning that
> `SGTable` has an identical data layout to `bindings::sg_table` due to
> repr(transparent), but that's really the only downside.

MappingState is a ZST only for borrowed mappings, i.e. mappings that
come from borrowed sg_tables. In case we want to own the SG table, we
also need to be able to unmap it on drop - and this requires some extra
data which we store into `ManagedMapping` mapping state (which is not a
ZST anymore).

Now I think I am lacking a good understanding of the model you have in
mind - could you share a few data structures of what you are thinking
about? I do understand the SGTable simply embedding an
`Opaque<sg_table>`, but that's about it. :)