Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Aug 26, 2025 at 4:36 PM CEST, Alexandre Courbot wrote:
> On Mon Aug 25, 2025 at 10:24 PM JST, Danilo Krummrich wrote:
>> Add a safe Rust abstraction for the kernel's scatter-gather list
>> facilities (`struct scatterlist` and `struct sg_table`).
>>
>> This commit introduces `SGTable<T>`, a wrapper that uses a generic
>> parameter to provide compile-time guarantees about ownership and lifetime.
>>
>> The abstraction provides two primary states:
>> - `SGTable<Owned<P>>`: Represents a table whose resources are fully
>>   managed by Rust. It takes ownership of a page provider `P`, allocates
>>   the underlying `struct sg_table`, maps it for DMA, and handles all
>>   cleanup automatically upon drop. The DMA mapping's lifetime is tied to
>>   the associated device using `Devres`, ensuring it is correctly unmapped
>>   before the device is unbound.
>> - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
>>   an externally managed `struct sg_table`. It is created from a raw
>>   pointer using `SGTable::as_ref()` and provides a lifetime-bound
>>   reference (`&'a SGTable`) for operations like iteration.
>>
>> The API exposes a safe iterator that yields `&SGEntry` references,
>> allowing drivers to easily access the DMA address and length of each
>> segment in the list.
>>
>> Co-developed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> A few minor things below, but:
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Used successfully on nova-core:
>
> Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks for re-testing!

> I still see mentions of "type state" in the code (and the commit
> message), is this on purpose? I still think this is a misleading use of
> the term, but your call.

I think I changed everything in the commit message, but there are indeed two or
three mentions in the code still.

I'm happy to replace them with "generic parameter", but I do not agree that the
term "type state" is misleading.

It may not be the classical typestate pattern, yet we are representing two
distinct states of a type.

> <snip>
>> +impl SGEntry {
>> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
>> +    /// lifetime `'a`.
>
> Shouldn't the scatterlist also have valid a dma_address and dma_len?

I don't think this is safety relevant from the perspective of Rust.

Also note that if we want to provide this guarantee, we need the caller to
provide the &Device<Bound> in SGTable::iter() the SGTable has been created with.

For the Owned generic parameter this is easy, for the Borrowed one we have no
way to ensure that the &Device<Bound> matches the device the SGTable has been
mapped for.

However, I don't think we have to provide this guarantee, since at this point
all device resources (such as I/O memory) have been revoked from the driver
already. So, effectively, even if a driver would attempt to program invalid DMA
addresses, the driver would be uncapable of doing so anyways.

> <snip>
>> +#[repr(transparent)]
>> +#[pin_data(PinnedDrop)]
>> +struct RawSGTable {
>
> Even if this is for internal use, I think a short comment explaining
> what this is for, and why it needs to be pinned (pointed to by devres)

That's not the reason this structure needs to be pinned. This is the reason for
Devres itself needs to be pinned.

In fact, I think RawSGTable by itself does not need to be pinned.

> would be helpful to people looking at this code.