Message-Id: <DC9U87GQ7ONZ.1489DEN1PPUAC@nvidia.com>
Date: Sat, 23 Aug 2025 22:22:47 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>, <akpm@...ux-foundation.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <abdiel.janulgue@...il.com>, <jgg@...pe.ca>, <lyude@...hat.com>,
 <robin.murphy@....com>, <daniel.almeida@...labora.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction
 for sg_table

On Thu Aug 21, 2025 at 1:52 AM 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 type-state
> pattern 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@...il.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---

<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`.
> +    #[inline]
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer

nit: "guarantees".

<snip>
> +impl SGTable {
> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
> +    ///
> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).

nit: "to a".

> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    ///
> +    /// - the `struct sg_table` pointed to by `ptr` is valid for the entire lifetime of `'a`,
> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
> +    #[inline]
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
> +        // to a `struct sg_table` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    #[inline]
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.inner.0.get()
> +    }
> +
> +    fn as_iter(&self) -> SGTableIter<'_> {
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
> +        let ptr = unsafe { (*self.as_raw()).sgl };
> +
> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
> +        let pos = Some(unsafe { SGEntry::from_raw(ptr) });
> +
> +        SGTableIter { pos }
> +    }
> +}
> +
> +/// # Invariants
> +///
> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].

nit: "of the".

> +/// - `sgt` is always DMA mapped.
> +struct DmaMapSgt {

Minor point: I'd call this structure `DmaMappedSgt` to highlight the
fact that it is actively mapped. Or alternatively document it and its
members so that fact is clear.

<snip>
> +impl<'a> IntoIterator for &'a SGTable {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    #[inline]
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.as_iter()
> +    }
> +}

While using this for Nova, I found it a bit unnatural having to call
`into_iter` on references intead of just having an `iter` method.
`into_iter` sounds like the passed object is consumed, while it is
actually its (copied) reference that is. Why not have a regular `iter`
method on `SGTable`? Actually we do have one, but it is called `as_iter`
and is private for some reason. :)

> +
> +/// An [`Iterator`] over the [`SGEntry`] items of an [`SGTable`].
> +pub struct SGTableIter<'a> {
> +    pos: Option<&'a SGEntry>,
> +}
> +
> +impl<'a> Iterator for SGTableIter<'a> {
> +    type Item = &'a SGEntry;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let entry = self.pos?;
> +
> +        // SAFETY: `entry.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        let next = unsafe { bindings::sg_next(entry.as_raw()) };
> +
> +        self.pos = (!next.is_null()).then(|| {
> +            // SAFETY: If `next` is not NULL, `sg_next()` guarantees to return a valid pointer to
> +            // the next `struct scatterlist`.
> +            unsafe { SGEntry::from_raw(next) }
> +        });

This might be missing a stop condition.

For reasons I am not completely clear about, the number of mapped
segments on the device side can be smaller than the number of
scatterlists provided by the sg_table. This is highlighted by the
documentation for `dma_map_sg_attrs` [1] ("Returns the number of mapped
entries (which can be less than nents) on success") and `sg_dma_address`
[2] ("You should only work with the number of sg entries dma_map_sg
returns, or alternatively stop on the first sg_dma_len(sg) which is 0.")

`dma_map_sgtable` stores the result of `dma_map_sg_attrs` into its
`nents` member, and makes use of it in its iterator macros. See how the
"regular" iterator and the "DMA" ones differ in their stop condition:

/*
 * Loop over each sg element in the given sg_table object.
 */
#define for_each_sgtable_sg(sgt, sg, i)		\
	for_each_sg((sgt)->sgl, sg, (sgt)->orig_nents, i)

and

/*
 * Loop over each sg element in the given *DMA mapped* sg_table object.
 * Please use sg_dma_address(sg) and sg_dma_len(sg) to extract DMA addresses
 * of the each element.
 */
#define for_each_sgtable_dma_sg(sgt, sg, i)	\
	for_each_sg((sgt)->sgl, sg, (sgt)->nents, i)

The DMA variant being the only one valid for accessing the DMA address
and DMA length (the C does not enforce this however).

So only calling `sg_next` until we reach the end of the list carries the
risk that we iterate on more items than we should, with the extra ones
having their length at 0 (which is likely to be a no-op, but is still
incorrect or at the very least inefficient). We can avoid this by either
storing the value of `nents` in the iterator, or, (which might be
simpler) follow the advice given by the documentation of
`sg_dma_address` and also stop if the DMA length of the next one is
zero.

[1] https://elixir.bootlin.com/linux/v6.16/source/kernel/dma/mapping.c#L233
[2] https://elixir.bootlin.com/linux/v6.16/source/include/linux/scatterlist.h#L31

Powered by blists - more mailing lists