[PATCH v2 0/5] Rust infrastructure for sg_table and scatterlist

[PATCH v2 0/5] Rust infrastructure for sg_table and scatterlist

[Danilo Krummrich]

This patch series provides abstractions for struct sg_table and struct
scatterlist.

Abdiel and me agreed for me to take over his previous iterations on this topic.
I decided to send my patches as a new series rather than as a subsequent version
of Abdiel's previous iterations, since the changes I made turned out to be much
closer to a full rewrite.

The most notable differences in design are:

  - SGTable utilizes BorrowedPage, AsPageIter and VmallocPageIter from my patch
    series in [1].

  -  SGTable is a transparent wrapper over either struct Owned<P> (where P is
     the provider of the backing pages) or struct Borrowed, which by itself is a
     transparent wrapper over Opaque<bindings::sg_table>, i.e. either
     SGTable<Owned<P>> or just SGTable (which is equivalent to
     SGTable<Borrowed>.

     - `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.

     - As a consequence, a borrowed SG table can be created with
       SGTable::as_ref(), which returns a &'a SGTable, just like similar
       existing abstractions.

       An owned SGTable is created with SGTable::new(), which returns an
       impl PinInit<SGTable<Owned<P>>, Error>, such that it can be initialized
       directly within existing private data memory allocations while providing
       the required pin guarantees.

  - SGTable<Owned<P>> uses an inner type Devres<DmaMapSgt> to ensure that the
    DMA mapping can't out-live device unbind.

  - SGTable<Owned<P>> uses pin-init for initialization.

This patch series depends on [1] (branch containing the patches in [2]). A
branch containing this series (including dependencies) can be found in [3];
Abdiel's latest series can be found in [4].

[1] https://lore.kernel.org/rust-for-linux/20250820145434.94745-1-dakr@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=page-iter
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=scatterlist
[4] https://lore.kernel.org/lkml/20250718103359.1026240-1-abdiel.janulgue@gmail.com/

Changes in v2:
  - Switch to an enum impl for DmaDirection utilizing compile time boundary
    checks.
  - Add missing Send/ Sync impls.
  - Rename as_ref() to from_raw().
  - Add a bunch of inline annotations.
  - Add a patch to introduce a typedef for dma_addr_t.
  - Let dma_len() return ResourceSize.
  - Add addional invariant to DmaMapSgt.
  - In RawSGTable::new(), pass pages as mutable slice reference.
  - Avoid casts when deriving max_segment in Owned::new().

Danilo Krummrich (5):
  rust: dma: implement DataDirection
  rust: dma: add type alias for bindings::dma_addr_t
  rust: scatterlist: Add type-state abstraction for sg_table
  samples: rust: dma: add sample code for SGTable
  MAINTAINERS: rust: dma: add scatterlist files

 MAINTAINERS                     |   4 +-
 drivers/gpu/nova-core/falcon.rs |   4 +-
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/scatterlist.c      |  24 ++
 rust/kernel/dma.rs              |  86 +++++-
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 475 ++++++++++++++++++++++++++++++++
 samples/rust/rust_dma.rs        |  35 ++-
 9 files changed, 615 insertions(+), 16 deletions(-)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs


base-commit: 27941214d368f3c17ed26a72662fc453bcc81b9d
-- 
2.50.1

[PATCH v2 1/5] rust: dma: implement DataDirection

[Danilo Krummrich]

Add the `DataDirection` struct, a newtype wrapper around the C
`enum dma_data_direction`.

This provides a type-safe Rust interface for specifying the direction of
DMA transfers.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/dma.rs              | 68 +++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 0e140e07758b..c2cc52ee9945 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -47,6 +47,7 @@
 #include <linux/cpumask.h>
 #include <linux/cred.h>
 #include <linux/device/faux.h>
+#include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 2bc8ab51ec28..5daba00ecc78 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -244,6 +244,74 @@ pub mod attrs {
     pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
 }
 
+/// DMA data direction.
+///
+/// Corresponds to the C [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[repr(u32)]
+pub enum DataDirection {
+    /// The DMA mapping is for bidirectional data transfer.
+    ///
+    /// This is used when the buffer can be both read from and written to by the device.
+    /// The cache for the corresponding memory region is both flushed and invalidated.
+    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
+
+    /// The DMA mapping is for data transfer from memory to the device (write).
+    ///
+    /// The CPU has prepared data in the buffer, and the device will read it.
+    /// The cache for the corresponding memory region is flushed.
+    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
+
+    /// The DMA mapping is for data transfer from the device to memory (read).
+    ///
+    /// The device will write data into the buffer for the CPU to read.
+    /// The cache for the corresponding memory region is invalidated before CPU access.
+    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
+
+    /// The DMA mapping is not for data transfer.
+    ///
+    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
+    /// will not perform any cache coherency operations.
+    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
+}
+
+impl DataDirection {
+    /// Casts the bindgen-generated enum type to a `u32` at compile time.
+    ///
+    /// This function will cause a compile-time error if the underlying value of the
+    /// C enum is out of bounds for `u32`.
+    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
+        // CAST: The C standard allows compilers to choose different integer types for enums.
+        // To safely check the value, we cast it to a wide signed integer type (`i128`)
+        // which can hold any standard C integer enum type without truncation.
+        let wide_val = val as i128;
+
+        // Check if the value is outside the valid range for the target type `u32`.
+        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
+        if wide_val < 0 || wide_val > u32::MAX as i128 {
+            // Trigger a compile-time error in a const context.
+            build_error!("C enum value is out of bounds for the target type `u32`.");
+        }
+
+        // CAST: This cast is valid because the check above guarantees that `wide_val`
+        // is within the representable range of `u32`.
+        wide_val as u32
+    }
+}
+
+impl From<DataDirection> for bindings::dma_data_direction {
+    /// Returns the raw representation of [`enum dma_data_direction`].
+    fn from(direction: DataDirection) -> Self {
+        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
+        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
+        // with the enum variants of `DataDirection`, which is a valid assumption given our
+        // compile-time checks.
+        direction as u32 as Self
+    }
+}
+
 /// An abstraction of the `dma_alloc_coherent` API.
 ///
 /// This is an abstraction around the `dma_alloc_coherent` API which is used to allocate and map
-- 
2.50.1

[PATCH v2 2/5] rust: dma: add type alias for bindings::dma_addr_t

[Danilo Krummrich]

Add a type alias for bindings::dma_addr_t (DmaAddress), such that we do
not have to access bindings directly.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/falcon.rs |  4 ++--
 rust/kernel/dma.rs              | 18 ++++++++++++++----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 50437c67c14a..aa36ed8c04ed 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -4,8 +4,8 @@
 
 use core::ops::Deref;
 use hal::FalconHal;
-use kernel::bindings;
 use kernel::device;
+use kernel::dma::DmaAddress;
 use kernel::prelude::*;
 use kernel::time::Delta;
 use kernel::types::ARef;
@@ -443,7 +443,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
                 fw.dma_handle_with_offset(load_offsets.src_start as usize)?,
             ),
         };
-        if dma_start % bindings::dma_addr_t::from(DMA_LEN) > 0 {
+        if dma_start % DmaAddress::from(DMA_LEN) > 0 {
             dev_err!(
                 self.dev,
                 "DMA transfer start addresses must be a multiple of {}",
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 5daba00ecc78..ceb215f80049 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -13,6 +13,16 @@
     types::ARef,
 };
 
+/// DMA address type.
+///
+/// Represents a bus address used for Direct Memory Access (DMA) operations.
+///
+/// This is an alias of the kernel's `dma_addr_t`, which may be `u32` or `u64` depending on
+/// `CONFIG_ARCH_DMA_ADDR_T_64BIT`.
+///
+/// Note that this may be `u64` even on 32-bit architectures.
+pub type DmaAddress = bindings::dma_addr_t;
+
 /// Trait to be implemented by DMA capable bus devices.
 ///
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
@@ -343,7 +353,7 @@ fn from(direction: DataDirection) -> Self {
 // entire `CoherentAllocation` including the allocated memory itself.
 pub struct CoherentAllocation<T: AsBytes + FromBytes> {
     dev: ARef<device::Device>,
-    dma_handle: bindings::dma_addr_t,
+    dma_handle: DmaAddress,
     count: usize,
     cpu_addr: *mut T,
     dma_attrs: Attrs,
@@ -444,7 +454,7 @@ pub fn start_ptr_mut(&mut self) -> *mut T {
 
     /// Returns a DMA handle which may be given to the device as the DMA address base of
     /// the region.
-    pub fn dma_handle(&self) -> bindings::dma_addr_t {
+    pub fn dma_handle(&self) -> DmaAddress {
         self.dma_handle
     }
 
@@ -452,13 +462,13 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
     /// device as the DMA address base of the region.
     ///
     /// Returns `EINVAL` if `offset` is not within the bounds of the allocation.
-    pub fn dma_handle_with_offset(&self, offset: usize) -> Result<bindings::dma_addr_t> {
+    pub fn dma_handle_with_offset(&self, offset: usize) -> Result<DmaAddress> {
         if offset >= self.count {
             Err(EINVAL)
         } else {
             // INVARIANT: The type invariant of `Self` guarantees that `size_of::<T> * count` fits
             // into a `usize`, and `offset` is inferior to `count`.
-            Ok(self.dma_handle + (offset * core::mem::size_of::<T>()) as bindings::dma_addr_t)
+            Ok(self.dma_handle + (offset * core::mem::size_of::<T>()) as DmaAddress)
         }
     }
 
-- 
2.50.1

[PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

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@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/helpers.c     |   1 +
 rust/helpers/scatterlist.c |  24 ++
 rust/kernel/lib.rs         |   1 +
 rust/kernel/scatterlist.rs | 475 +++++++++++++++++++++++++++++++++++++
 4 files changed, 501 insertions(+)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..e94542bf6ea7 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -39,6 +39,7 @@
 #include "rcu.c"
 #include "refcount.c"
 #include "regulator.c"
+#include "scatterlist.c"
 #include "security.c"
 #include "signal.c"
 #include "slab.c"
diff --git a/rust/helpers/scatterlist.c b/rust/helpers/scatterlist.c
new file mode 100644
index 000000000000..80c956ee09ab
--- /dev/null
+++ b/rust/helpers/scatterlist.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-direction.h>
+
+dma_addr_t rust_helper_sg_dma_address(struct scatterlist *sg)
+{
+	return sg_dma_address(sg);
+}
+
+unsigned int rust_helper_sg_dma_len(struct scatterlist *sg)
+{
+	return sg_dma_len(sg);
+}
+
+struct scatterlist *rust_helper_sg_next(struct scatterlist *sg)
+{
+	return sg_next(sg);
+}
+
+void rust_helper_dma_unmap_sgtable(struct device *dev, struct sg_table *sgt,
+				   enum dma_data_direction dir, unsigned long attrs)
+{
+	return dma_unmap_sgtable(dev, sgt, dir, attrs);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..55acbc893736 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -113,6 +113,7 @@
 pub mod rbtree;
 pub mod regulator;
 pub mod revocable;
+pub mod scatterlist;
 pub mod security;
 pub mod seq_file;
 pub mod sizes;
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
new file mode 100644
index 000000000000..371c51222c5c
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,475 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for scatter-gather lists.
+//!
+//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
+//!
+//! Scatter-gather (SG) I/O is a memory access technique that allows devices to perform DMA
+//! operations on data buffers that are not physically contiguous in memory. It works by creating a
+//! "scatter-gather list", an array where each entry specifies the address and length of a
+//! physically contiguous memory segment.
+//!
+//! The device's DMA controller can then read this list and process the segments sequentially as
+//! part of one logical I/O request. This avoids the need for a single, large, physically contiguous
+//! memory buffer, which can be difficult or impossible to allocate.
+//!
+//! This module provides safe Rust abstractions over the kernel's `struct scatterlist` and
+//! `struct sg_table` types.
+//!
+//! The main entry point is the [`SGTable`] type, which represents a complete scatter-gather table.
+//! It can be either:
+//!
+//! - An owned table ([`SGTable<Owned<P>>`]), created from a Rust memory buffer (e.g., [`VVec`]).
+//!   This type manages the allocation of the `struct sg_table`, the DMA mapping of the buffer, and
+//!   the automatic cleanup of all resources.
+//! - A borrowed reference (&[`SGTable`]), which provides safe, read-only access to a table that was
+//!   allocated by other (e.g., C) code.
+//!
+//! Individual entries in the table are represented by [`SGEntry`], which can be accessed by
+//! iterating over an [`SGTable`].
+
+use crate::{
+    alloc,
+    alloc::allocator::VmallocPageIter,
+    bindings,
+    device::{Bound, Device},
+    devres::Devres,
+    dma, error,
+    io::resource::ResourceSize,
+    page,
+    prelude::*,
+    types::{ARef, Opaque},
+};
+use core::{ops::Deref, ptr::NonNull};
+
+/// A single entry in a scatter-gather list.
+///
+/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
+/// for DMA.
+///
+/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
+/// or own [`SGEntry`] objects directly.
+#[repr(transparent)]
+pub struct SGEntry(Opaque<bindings::scatterlist>);
+
+// SAFETY: `SGEntry` can be send to any task.
+unsafe impl Send for SGEntry {}
+
+// SAFETY: `SGEntry` can be accessed concurrently.
+unsafe impl Sync for SGEntry {}
+
+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
+        // to a `struct scatterlist` for the duration of `'a`.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Obtain the raw `struct scatterlist *`.
+    #[inline]
+    fn as_raw(&self) -> *mut bindings::scatterlist {
+        self.0.get()
+    }
+
+    /// Returns the DMA address of this SG entry.
+    ///
+    /// This is the address that the device should use to access the memory segment.
+    #[inline]
+    pub fn dma_address(&self) -> dma::DmaAddress {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
+        unsafe { bindings::sg_dma_address(self.as_raw()) }
+    }
+
+    /// Returns the length of this SG entry in bytes.
+    #[inline]
+    pub fn dma_len(&self) -> ResourceSize {
+        #[allow(clippy::useless_conversion)]
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
+        unsafe { bindings::sg_dma_len(self.as_raw()) }.into()
+    }
+}
+
+/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
+#[repr(transparent)]
+pub struct Borrowed(Opaque<bindings::sg_table>);
+
+// SAFETY: `Borrowed` can be send to any task.
+unsafe impl Send for Borrowed {}
+
+// SAFETY: `Borrowed` can be accessed concurrently.
+unsafe impl Sync for Borrowed {}
+
+/// A scatter-gather table.
+///
+/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
+/// memory segments that can be passed to a device for I/O operations.
+///
+/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
+/// tables.
+///
+///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
+///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
+///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
+///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
+///    external lifetime.
+///
+/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
+#[repr(transparent)]
+#[pin_data]
+pub struct SGTable<T: private::Sealed = Borrowed> {
+    #[pin]
+    inner: T,
+}
+
+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).
+    ///
+    /// # 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`].
+/// - `sgt` is always DMA mapped.
+struct DmaMapSgt {
+    sgt: NonNull<bindings::sg_table>,
+    dev: ARef<Device>,
+    dir: dma::DataDirection,
+}
+
+// SAFETY: `DmaMapSgt` can be send to any task.
+unsafe impl Send for DmaMapSgt {}
+
+// SAFETY: `DmaMapSgt` can be accessed concurrently.
+unsafe impl Sync for DmaMapSgt {}
+
+impl DmaMapSgt {
+    /// # Safety
+    ///
+    /// - `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
+    ///   returned [`DmaMapSgt`].
+    /// - The caller must guarantee that `sgt` remains DMA mapped for the entire lifetime of
+    ///   [`DmaMapSgt`].
+    unsafe fn new(
+        sgt: NonNull<bindings::sg_table>,
+        dev: &Device<Bound>,
+        dir: dma::DataDirection,
+    ) -> Result<Self> {
+        // SAFETY:
+        // - `dev.as_raw()` is a valid pointer to a `struct device`, which is guaranteed to be
+        //   bound to a driver for the duration of this call.
+        // - `sgt` is a valid pointer to a `struct sg_table`.
+        error::to_result(unsafe {
+            bindings::dma_map_sgtable(dev.as_raw(), sgt.as_ptr(), dir.into(), 0)
+        })?;
+
+        // INVARIANT: By the safety requirements of this function it is guaranteed that `sgt` is
+        // valid for the entire lifetime of this object instance.
+        Ok(Self {
+            sgt,
+            dev: dev.into(),
+            dir,
+        })
+    }
+}
+
+impl Drop for DmaMapSgt {
+    #[inline]
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.dev.as_raw()` is a pointer to a valid `struct device`.
+        // - `self.dev` is the same device the mapping has been created for in `Self::new()`.
+        // - `self.sgt.as_ptr()` is a valid pointer to a `struct sg_table` by the type invariants
+        //   of `Self`.
+        // - `self.dir` is the same `dma::DataDirection` the mapping has been created with in
+        //   `Self::new()`.
+        unsafe {
+            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sgt.as_ptr(), self.dir.into(), 0)
+        };
+    }
+}
+
+#[repr(transparent)]
+#[pin_data(PinnedDrop)]
+struct RawSGTable {
+    #[pin]
+    sgt: Opaque<bindings::sg_table>,
+}
+
+// SAFETY: `RawSGTable` can be send to any task.
+unsafe impl Send for RawSGTable {}
+
+// SAFETY: `RawSGTable` can be accessed concurrently.
+unsafe impl Sync for RawSGTable {}
+
+impl RawSGTable {
+    fn new(
+        pages: &mut [*mut bindings::page],
+        size: usize,
+        max_segment: u32,
+        flags: alloc::Flags,
+    ) -> impl PinInit<Self, Error> + '_ {
+        try_pin_init!(Self {
+            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
+                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
+                // produces a NPE.
+                if pages.is_empty() {
+                    return Err(EINVAL);
+                }
+
+                // SAFETY:
+                // - `slot` is a valid pointer to uninitialized memory.
+                // - As by the check above, `pages` is not empty.
+                error::to_result(unsafe {
+                    bindings::sg_alloc_table_from_pages_segment(
+                        slot,
+                        pages.as_mut_ptr(),
+                        pages.len().try_into()?,
+                        0,
+                        size,
+                        max_segment,
+                        flags.as_raw(),
+                    )
+                })
+            }),
+        })
+    }
+
+    #[inline]
+    fn as_raw(&self) -> *mut bindings::sg_table {
+        self.sgt.get()
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RawSGTable {
+    #[inline]
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
+        unsafe { bindings::sg_free_table(self.sgt.get()) };
+    }
+}
+
+/// The [`Owned`] type state of an [`SGTable`].
+///
+/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
+///
+/// - The backing memory pages.
+/// - The `struct sg_table` allocation (`sgt`).
+/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
+///
+/// Users interact with this type through the [`SGTable`] handle and do not need to manage
+/// [`Owned`] directly.
+#[pin_data]
+pub struct Owned<P> {
+    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
+    // `struct sg_table` and finally free the backing pages.
+    #[pin]
+    dma: Devres<DmaMapSgt>,
+    #[pin]
+    sgt: RawSGTable,
+    _pages: P,
+}
+
+// SAFETY: `Owned` can be send to any task if `P` can be send to any task.
+unsafe impl<P: Send> Send for Owned<P> {}
+
+// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
+unsafe impl<P: Sync> Sync for Owned<P> {}
+
+impl<P> Owned<P>
+where
+    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
+{
+    fn new(
+        dev: &Device<Bound>,
+        mut pages: P,
+        dir: dma::DataDirection,
+        flags: alloc::Flags,
+    ) -> Result<impl PinInit<Self, Error> + '_> {
+        let page_iter = pages.page_iter();
+        let size = page_iter.size();
+
+        let mut page_vec: KVec<*mut bindings::page> =
+            KVec::with_capacity(page_iter.page_count(), flags)?;
+
+        for page in page_iter {
+            page_vec.push(page.as_ptr(), flags)?;
+        }
+
+        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
+        // an `unsigned int`.
+        let max_segment = {
+            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
+            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
+            if size == 0 {
+                u32::MAX
+            } else {
+                u32::try_from(size).unwrap_or(u32::MAX)
+            }
+        };
+
+        Ok(try_pin_init!(&this in Self {
+            sgt <- RawSGTable::new(&mut page_vec, size, max_segment, flags),
+            dma <- {
+                // SAFETY: `this` is a valid pointer to uninitialized memory.
+                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
+
+                // SAFETY: `sgt` is guaranteed to be non-null.
+                let sgt = unsafe { NonNull::new_unchecked(sgt) };
+
+                // SAFETY:
+                // - It is guaranteed that the object returned by `DmaMapSgt::new` won't out-live
+                //   `sgt`.
+                // - `sgt` is never DMA unmapped manually.
+                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
+            },
+            _pages: pages,
+        }))
+    }
+}
+
+impl<P> SGTable<Owned<P>>
+where
+    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
+{
+    /// Allocates a new scatter-gather table from the given pages and maps it for DMA.
+    ///
+    /// This constructor creates a new [`SGTable<Owned>`] that takes ownership of `P`.
+    /// It allocates a `struct sg_table`, populates it with entries corresponding to the physical
+    /// pages of `P`, and maps the table for DMA with the specified [`Device`] and
+    /// [`dma::DataDirection`].
+    ///
+    /// The DMA mapping is managed through [`Devres`], ensuring that the DMA mapping is unmapped
+    /// once the associated [`Device`] is unbound, or when the [`SGTable<Owned>`] is dropped.
+    ///
+    /// # Parameters
+    ///
+    /// * `dev`: The [`Device`] that will be performing the DMA.
+    /// * `pages`: The entity providing the backing pages. It must implement [`page::AsPageIter`].
+    ///   The ownership of this entity is moved into the new [`SGTable<Owned>`].
+    /// * `dir`: The [`dma::DataDirection`] of the DMA transfer.
+    /// * `flags`: Allocation flags for internal allocations (e.g., [`GFP_KERNEL`]).
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::{
+    ///     device::{Bound, Device},
+    ///     dma, page,
+    ///     prelude::*,
+    ///     scatterlist::*,
+    /// };
+    ///
+    /// fn test(dev: &Device<Bound>) -> Result {
+    ///     let size = 4 * page::PAGE_SIZE;
+    ///     let pages = VVec::<u8>::with_capacity(size, GFP_KERNEL)?;
+    ///
+    ///     let sgt = KBox::pin_init(SGTable::new(
+    ///         dev,
+    ///         pages,
+    ///         dma::DataDirection::ToDevice,
+    ///         GFP_KERNEL,
+    ///     ), GFP_KERNEL)?;
+    ///
+    ///     Ok(())
+    /// }
+    /// ```
+    pub fn new(
+        dev: &Device<Bound>,
+        pages: P,
+        dir: dma::DataDirection,
+        flags: alloc::Flags,
+    ) -> impl PinInit<Self, Error> + '_ {
+        try_pin_init!(Self {
+            inner <- Owned::new(dev, pages, dir, flags)?
+        })
+    }
+}
+
+impl<P> Deref for SGTable<Owned<P>> {
+    type Target = SGTable;
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: `self.inner.sgt.as_raw()` is a valid pointer to a `struct sg_table` for the
+        // entire lifetime of `self`.
+        unsafe { SGTable::from_raw(self.inner.sgt.as_raw()) }
+    }
+}
+
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Borrowed {}
+    impl<P> Sealed for super::Owned<P> {}
+}
+
+impl<'a> IntoIterator for &'a SGTable {
+    type Item = &'a SGEntry;
+    type IntoIter = SGTableIter<'a>;
+
+    #[inline]
+    fn into_iter(self) -> Self::IntoIter {
+        self.as_iter()
+    }
+}
+
+/// 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) }
+        });
+
+        Some(entry)
+    }
+}
-- 
2.50.1

[PATCH v2 4/5] samples: rust: dma: add sample code for SGTable

[Danilo Krummrich]

Add sample code for allocating and mapping a scatter-gather table
(`SGTable`).

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>
---
 samples/rust/rust_dma.rs | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index c5e7cce68654..d9532bef6d2c 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -7,15 +7,19 @@
 use kernel::{
     bindings,
     device::Core,
-    dma::{CoherentAllocation, Device, DmaMask},
-    pci,
+    dma::{CoherentAllocation, DataDirection, Device, DmaMask},
+    page, pci,
     prelude::*,
+    scatterlist::{Owned, SGTable},
     types::ARef,
 };
 
+#[pin_data(PinnedDrop)]
 struct DmaSampleDriver {
     pdev: ARef<pci::Device>,
     ca: CoherentAllocation<MyStruct>,
+    #[pin]
+    sgt: SGTable<Owned<VVec<u8>>>,
 }
 
 const TEST_VALUES: [(u32, u32); 5] = [
@@ -70,21 +74,30 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
             kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
         }
 
-        let drvdata = KBox::new(
-            Self {
+        let size = 4 * page::PAGE_SIZE;
+        let pages = VVec::with_capacity(size, GFP_KERNEL)?;
+
+        let sgt = SGTable::new(pdev.as_ref(), pages, DataDirection::ToDevice, GFP_KERNEL);
+
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
                 pdev: pdev.into(),
                 ca,
-            },
+                sgt <- sgt,
+            }),
             GFP_KERNEL,
         )?;
 
-        Ok(drvdata.into())
+        Ok(drvdata)
     }
 }
 
-impl Drop for DmaSampleDriver {
-    fn drop(&mut self) {
-        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
+#[pinned_drop]
+impl PinnedDrop for DmaSampleDriver {
+    fn drop(self: Pin<&mut Self>) {
+        let dev = self.pdev.as_ref();
+
+        dev_info!(dev, "Unload DMA test driver.\n");
 
         for (i, value) in TEST_VALUES.into_iter().enumerate() {
             let val0 = kernel::dma_read!(self.ca[i].h);
@@ -99,6 +112,10 @@ fn drop(&mut self) {
                 assert_eq!(val1, value.1);
             }
         }
+
+        for (i, entry) in self.sgt.into_iter().enumerate() {
+            dev_info!(dev, "Entry[{}]: DMA address: {:#x}", i, entry.dma_address());
+        }
     }
 }
 
-- 
2.50.1

[PATCH v2 5/5] MAINTAINERS: rust: dma: add scatterlist files

[Danilo Krummrich]

Rename the "DMA MAPPING HELPERS DEVICE DRIVER API [RUST]" maintainers
entry to "DMA MAPPING & SCATTERLIST API [RUST]" and add the
corresponding scatterlist files.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe168477caa4..65f676b2c304 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7238,7 +7238,7 @@ F:	include/linux/dma-mapping.h
 F:	include/linux/swiotlb.h
 F:	kernel/dma/
 
-DMA MAPPING HELPERS DEVICE DRIVER API [RUST]
+DMA MAPPING & SCATTERLIST API [RUST]
 M:	Abdiel Janulgue <abdiel.janulgue@gmail.com>
 M:	Danilo Krummrich <dakr@kernel.org>
 R:	Daniel Almeida <daniel.almeida@collabora.com>
@@ -7249,7 +7249,9 @@ S:	Supported
 W:	https://rust-for-linux.com
 T:	git https://github.com/Rust-for-Linux/linux.git alloc-next
 F:	rust/helpers/dma.c
+F:	rust/helpers/scatterlist.c
 F:	rust/kernel/dma.rs
+F:	rust/kernel/scatterlist.rs
 F:	samples/rust/rust_dma.rs
 
 DMA-BUF HEAPS FRAMEWORK
-- 
2.50.1

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Daniel Almeida]

Hmm, I see that you’ve just posted a new version. I had v1 open the whole
morning so I missed that. Can you check my nits there?

— Daniel

Re: [PATCH v2 1/5] rust: dma: implement DataDirection

[Alice Ryhl]

On Wed, Aug 20, 2025 at 06:52:55PM +0200, Danilo Krummrich wrote:
> Add the `DataDirection` struct, a newtype wrapper around the C
> `enum dma_data_direction`.
> 
> This provides a type-safe Rust interface for specifying the direction of
> DMA transfers.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 2/5] rust: dma: add type alias for bindings::dma_addr_t

[Alice Ryhl]

On Wed, Aug 20, 2025 at 06:52:56PM +0200, Danilo Krummrich wrote:
> Add a type alias for bindings::dma_addr_t (DmaAddress), such that we do
> not have to access bindings directly.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alice Ryhl]

On Wed, Aug 20, 2025 at 06:52:57PM +0200, 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@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/helpers/helpers.c     |   1 +
>  rust/helpers/scatterlist.c |  24 ++
>  rust/kernel/lib.rs         |   1 +
>  rust/kernel/scatterlist.rs | 475 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 501 insertions(+)
>  create mode 100644 rust/helpers/scatterlist.c
>  create mode 100644 rust/kernel/scatterlist.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41d..e94542bf6ea7 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -39,6 +39,7 @@
>  #include "rcu.c"
>  #include "refcount.c"
>  #include "regulator.c"
> +#include "scatterlist.c"
>  #include "security.c"
>  #include "signal.c"
>  #include "slab.c"
> diff --git a/rust/helpers/scatterlist.c b/rust/helpers/scatterlist.c
> new file mode 100644
> index 000000000000..80c956ee09ab
> --- /dev/null
> +++ b/rust/helpers/scatterlist.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-direction.h>
> +
> +dma_addr_t rust_helper_sg_dma_address(struct scatterlist *sg)
> +{
> +	return sg_dma_address(sg);
> +}
> +
> +unsigned int rust_helper_sg_dma_len(struct scatterlist *sg)
> +{
> +	return sg_dma_len(sg);
> +}
> +
> +struct scatterlist *rust_helper_sg_next(struct scatterlist *sg)
> +{
> +	return sg_next(sg);
> +}
> +
> +void rust_helper_dma_unmap_sgtable(struct device *dev, struct sg_table *sgt,
> +				   enum dma_data_direction dir, unsigned long attrs)
> +{
> +	return dma_unmap_sgtable(dev, sgt, dir, attrs);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ed53169e795c..55acbc893736 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -113,6 +113,7 @@
>  pub mod rbtree;
>  pub mod regulator;
>  pub mod revocable;
> +pub mod scatterlist;
>  pub mod security;
>  pub mod seq_file;
>  pub mod sizes;
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> new file mode 100644
> index 000000000000..371c51222c5c
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,475 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for scatter-gather lists.
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +//!
> +//! Scatter-gather (SG) I/O is a memory access technique that allows devices to perform DMA
> +//! operations on data buffers that are not physically contiguous in memory. It works by creating a
> +//! "scatter-gather list", an array where each entry specifies the address and length of a
> +//! physically contiguous memory segment.
> +//!
> +//! The device's DMA controller can then read this list and process the segments sequentially as
> +//! part of one logical I/O request. This avoids the need for a single, large, physically contiguous
> +//! memory buffer, which can be difficult or impossible to allocate.
> +//!
> +//! This module provides safe Rust abstractions over the kernel's `struct scatterlist` and
> +//! `struct sg_table` types.
> +//!
> +//! The main entry point is the [`SGTable`] type, which represents a complete scatter-gather table.
> +//! It can be either:
> +//!
> +//! - An owned table ([`SGTable<Owned<P>>`]), created from a Rust memory buffer (e.g., [`VVec`]).
> +//!   This type manages the allocation of the `struct sg_table`, the DMA mapping of the buffer, and
> +//!   the automatic cleanup of all resources.
> +//! - A borrowed reference (&[`SGTable`]), which provides safe, read-only access to a table that was
> +//!   allocated by other (e.g., C) code.
> +//!
> +//! Individual entries in the table are represented by [`SGEntry`], which can be accessed by
> +//! iterating over an [`SGTable`].
> +
> +use crate::{
> +    alloc,
> +    alloc::allocator::VmallocPageIter,
> +    bindings,
> +    device::{Bound, Device},
> +    devres::Devres,
> +    dma, error,
> +    io::resource::ResourceSize,
> +    page,
> +    prelude::*,
> +    types::{ARef, Opaque},
> +};
> +use core::{ops::Deref, ptr::NonNull};
> +
> +/// A single entry in a scatter-gather list.
> +///
> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
> +/// for DMA.
> +///
> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
> +/// or own [`SGEntry`] objects directly.
> +#[repr(transparent)]
> +pub struct SGEntry(Opaque<bindings::scatterlist>);
> +
> +// SAFETY: `SGEntry` can be send to any task.
> +unsafe impl Send for SGEntry {}
> +
> +// SAFETY: `SGEntry` can be accessed concurrently.
> +unsafe impl Sync for SGEntry {}
> +
> +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
> +        // to a `struct scatterlist` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct scatterlist *`.
> +    #[inline]
> +    fn as_raw(&self) -> *mut bindings::scatterlist {
> +        self.0.get()
> +    }
> +
> +    /// Returns the DMA address of this SG entry.
> +    ///
> +    /// This is the address that the device should use to access the memory segment.
> +    #[inline]
> +    pub fn dma_address(&self) -> dma::DmaAddress {
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
> +    }
> +
> +    /// Returns the length of this SG entry in bytes.
> +    #[inline]
> +    pub fn dma_len(&self) -> ResourceSize {
> +        #[allow(clippy::useless_conversion)]
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_len(self.as_raw()) }.into()
> +    }
> +}
> +
> +/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
> +#[repr(transparent)]
> +pub struct Borrowed(Opaque<bindings::sg_table>);
> +
> +// SAFETY: `Borrowed` can be send to any task.
> +unsafe impl Send for Borrowed {}
> +
> +// SAFETY: `Borrowed` can be accessed concurrently.
> +unsafe impl Sync for Borrowed {}
> +
> +/// A scatter-gather table.
> +///
> +/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
> +/// memory segments that can be passed to a device for I/O operations.
> +///
> +/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
> +/// tables.
> +///
> +///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
> +///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
> +///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
> +///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
> +///    external lifetime.
> +///
> +/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
> +#[repr(transparent)]
> +#[pin_data]
> +pub struct SGTable<T: private::Sealed = Borrowed> {
> +    #[pin]
> +    inner: T,
> +}
> +
> +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).
> +    ///
> +    /// # 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`].
> +/// - `sgt` is always DMA mapped.
> +struct DmaMapSgt {
> +    sgt: NonNull<bindings::sg_table>,
> +    dev: ARef<Device>,
> +    dir: dma::DataDirection,
> +}
> +
> +// SAFETY: `DmaMapSgt` can be send to any task.
> +unsafe impl Send for DmaMapSgt {}
> +
> +// SAFETY: `DmaMapSgt` can be accessed concurrently.
> +unsafe impl Sync for DmaMapSgt {}
> +
> +impl DmaMapSgt {
> +    /// # Safety
> +    ///
> +    /// - `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
> +    ///   returned [`DmaMapSgt`].
> +    /// - The caller must guarantee that `sgt` remains DMA mapped for the entire lifetime of
> +    ///   [`DmaMapSgt`].
> +    unsafe fn new(
> +        sgt: NonNull<bindings::sg_table>,
> +        dev: &Device<Bound>,
> +        dir: dma::DataDirection,
> +    ) -> Result<Self> {
> +        // SAFETY:
> +        // - `dev.as_raw()` is a valid pointer to a `struct device`, which is guaranteed to be
> +        //   bound to a driver for the duration of this call.
> +        // - `sgt` is a valid pointer to a `struct sg_table`.
> +        error::to_result(unsafe {
> +            bindings::dma_map_sgtable(dev.as_raw(), sgt.as_ptr(), dir.into(), 0)
> +        })?;
> +
> +        // INVARIANT: By the safety requirements of this function it is guaranteed that `sgt` is
> +        // valid for the entire lifetime of this object instance.
> +        Ok(Self {
> +            sgt,
> +            dev: dev.into(),
> +            dir,
> +        })
> +    }
> +}
> +
> +impl Drop for DmaMapSgt {
> +    #[inline]
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.dev.as_raw()` is a pointer to a valid `struct device`.
> +        // - `self.dev` is the same device the mapping has been created for in `Self::new()`.
> +        // - `self.sgt.as_ptr()` is a valid pointer to a `struct sg_table` by the type invariants
> +        //   of `Self`.
> +        // - `self.dir` is the same `dma::DataDirection` the mapping has been created with in
> +        //   `Self::new()`.
> +        unsafe {
> +            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sgt.as_ptr(), self.dir.into(), 0)
> +        };
> +    }
> +}
> +
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {
> +    #[pin]
> +    sgt: Opaque<bindings::sg_table>,
> +}
> +
> +// SAFETY: `RawSGTable` can be send to any task.
> +unsafe impl Send for RawSGTable {}
> +
> +// SAFETY: `RawSGTable` can be accessed concurrently.
> +unsafe impl Sync for RawSGTable {}
> +
> +impl RawSGTable {
> +    fn new(
> +        pages: &mut [*mut bindings::page],

This mutable reference is suspicious, IMO. Does it modify the list? You
don't read the values after calling `sg_alloc_table_from_pages_segment()`.

> +        size: usize,
> +        max_segment: u32,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> + '_ {
> +        try_pin_init!(Self {
> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
> +                // produces a NPE.
> +                if pages.is_empty() {
> +                    return Err(EINVAL);
> +                }
> +
> +                // SAFETY:
> +                // - `slot` is a valid pointer to uninitialized memory.
> +                // - As by the check above, `pages` is not empty.
> +                error::to_result(unsafe {
> +                    bindings::sg_alloc_table_from_pages_segment(
> +                        slot,
> +                        pages.as_mut_ptr(),
> +                        pages.len().try_into()?,
> +                        0,
> +                        size,
> +                        max_segment,
> +                        flags.as_raw(),
> +                    )
> +                })
> +            }),
> +        })
> +    }
> +
> +    #[inline]
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.sgt.get()
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RawSGTable {
> +    #[inline]
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
> +        unsafe { bindings::sg_free_table(self.sgt.get()) };

It's weird that this is called free when the sg_table isn't freed by
this call.

> +    }
> +}
> +
> +/// The [`Owned`] type state of an [`SGTable`].
> +///
> +/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
> +///
> +/// - The backing memory pages.
> +/// - The `struct sg_table` allocation (`sgt`).
> +/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
> +///
> +/// Users interact with this type through the [`SGTable`] handle and do not need to manage
> +/// [`Owned`] directly.
> +#[pin_data]
> +pub struct Owned<P> {
> +    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
> +    // `struct sg_table` and finally free the backing pages.
> +    #[pin]
> +    dma: Devres<DmaMapSgt>,
> +    #[pin]
> +    sgt: RawSGTable,
> +    _pages: P,
> +}
> +
> +// SAFETY: `Owned` can be send to any task if `P` can be send to any task.
> +unsafe impl<P: Send> Send for Owned<P> {}
> +
> +// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
> +unsafe impl<P: Sync> Sync for Owned<P> {}
> +
> +impl<P> Owned<P>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> +{
> +    fn new(
> +        dev: &Device<Bound>,
> +        mut pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> Result<impl PinInit<Self, Error> + '_> {
> +        let page_iter = pages.page_iter();
> +        let size = page_iter.size();

Variable naming here is confusing. There's another variable called size
in an inner scope, and then afterwards in RawSGTable you use *this* size
variable again.

> +        let mut page_vec: KVec<*mut bindings::page> =
> +            KVec::with_capacity(page_iter.page_count(), flags)?;
> +
> +        for page in page_iter {
> +            page_vec.push(page.as_ptr(), flags)?;
> +        }
> +
> +        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
> +        // an `unsigned int`.
> +        let max_segment = {
> +            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
> +            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
> +            if size == 0 {
> +                u32::MAX
> +            } else {
> +                u32::try_from(size).unwrap_or(u32::MAX)
> +            }
> +        };
> +
> +        Ok(try_pin_init!(&this in Self {
> +            sgt <- RawSGTable::new(&mut page_vec, size, max_segment, flags),
> +            dma <- {
> +                // SAFETY: `this` is a valid pointer to uninitialized memory.
> +                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
> +
> +                // SAFETY: `sgt` is guaranteed to be non-null.
> +                let sgt = unsafe { NonNull::new_unchecked(sgt) };
> +
> +                // SAFETY:
> +                // - It is guaranteed that the object returned by `DmaMapSgt::new` won't out-live
> +                //   `sgt`.
> +                // - `sgt` is never DMA unmapped manually.
> +                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
> +            },
> +            _pages: pages,
> +        }))
> +    }
> +}
> +
> +impl<P> SGTable<Owned<P>>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> +{
> +    /// Allocates a new scatter-gather table from the given pages and maps it for DMA.
> +    ///
> +    /// This constructor creates a new [`SGTable<Owned>`] that takes ownership of `P`.
> +    /// It allocates a `struct sg_table`, populates it with entries corresponding to the physical
> +    /// pages of `P`, and maps the table for DMA with the specified [`Device`] and
> +    /// [`dma::DataDirection`].
> +    ///
> +    /// The DMA mapping is managed through [`Devres`], ensuring that the DMA mapping is unmapped
> +    /// once the associated [`Device`] is unbound, or when the [`SGTable<Owned>`] is dropped.
> +    ///
> +    /// # Parameters
> +    ///
> +    /// * `dev`: The [`Device`] that will be performing the DMA.
> +    /// * `pages`: The entity providing the backing pages. It must implement [`page::AsPageIter`].
> +    ///   The ownership of this entity is moved into the new [`SGTable<Owned>`].
> +    /// * `dir`: The [`dma::DataDirection`] of the DMA transfer.
> +    /// * `flags`: Allocation flags for internal allocations (e.g., [`GFP_KERNEL`]).
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::{
> +    ///     device::{Bound, Device},
> +    ///     dma, page,
> +    ///     prelude::*,
> +    ///     scatterlist::*,
> +    /// };
> +    ///
> +    /// fn test(dev: &Device<Bound>) -> Result {
> +    ///     let size = 4 * page::PAGE_SIZE;
> +    ///     let pages = VVec::<u8>::with_capacity(size, GFP_KERNEL)?;
> +    ///
> +    ///     let sgt = KBox::pin_init(SGTable::new(
> +    ///         dev,
> +    ///         pages,
> +    ///         dma::DataDirection::ToDevice,
> +    ///         GFP_KERNEL,
> +    ///     ), GFP_KERNEL)?;
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    pub fn new(
> +        dev: &Device<Bound>,
> +        pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> + '_ {
> +        try_pin_init!(Self {
> +            inner <- Owned::new(dev, pages, dir, flags)?
> +        })
> +    }
> +}
> +
> +impl<P> Deref for SGTable<Owned<P>> {
> +    type Target = SGTable;
> +
> +    #[inline]
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: `self.inner.sgt.as_raw()` is a valid pointer to a `struct sg_table` for the
> +        // entire lifetime of `self`.
> +        unsafe { SGTable::from_raw(self.inner.sgt.as_raw()) }
> +    }
> +}
> +
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Borrowed {}
> +    impl<P> Sealed for super::Owned<P> {}
> +}
> +
> +impl<'a> IntoIterator for &'a SGTable {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    #[inline]
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.as_iter()
> +    }
> +}
> +
> +/// 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) }
> +        });
> +
> +        Some(entry)
> +    }
> +}
> -- 
> 2.50.1
> 

Re: [PATCH v2 0/5] Rust infrastructure for sg_table and scatterlist

[Alice Ryhl]

On Wed, Aug 20, 2025 at 06:52:54PM +0200, Danilo Krummrich wrote:
> Changes in v2:
>   - Switch to an enum impl for DmaDirection utilizing compile time boundary
>     checks.
>   - Add missing Send/ Sync impls.
>   - Rename as_ref() to from_raw().

Thanks, but it's still as_ref() in the cover letter.

Alice

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>> +#[pinned_drop]
>> +impl PinnedDrop for RawSGTable {
>> +    #[inline]
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
>> +        unsafe { bindings::sg_free_table(self.sgt.get()) };
>
> It's weird that this is called free when the sg_table isn't freed by
> this call.

It frees the entries contained in the sg_table.

>> +    }
>> +}
>> +
>> +/// The [`Owned`] type state of an [`SGTable`].
>> +///
>> +/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
>> +///
>> +/// - The backing memory pages.
>> +/// - The `struct sg_table` allocation (`sgt`).
>> +/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
>> +///
>> +/// Users interact with this type through the [`SGTable`] handle and do not need to manage
>> +/// [`Owned`] directly.
>> +#[pin_data]
>> +pub struct Owned<P> {
>> +    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
>> +    // `struct sg_table` and finally free the backing pages.
>> +    #[pin]
>> +    dma: Devres<DmaMapSgt>,
>> +    #[pin]
>> +    sgt: RawSGTable,
>> +    _pages: P,
>> +}
>> +
>> +// SAFETY: `Owned` can be send to any task if `P` can be send to any task.
>> +unsafe impl<P: Send> Send for Owned<P> {}
>> +
>> +// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
>> +unsafe impl<P: Sync> Sync for Owned<P> {}
>> +
>> +impl<P> Owned<P>
>> +where
>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>> +{
>> +    fn new(
>> +        dev: &Device<Bound>,
>> +        mut pages: P,
>> +        dir: dma::DataDirection,
>> +        flags: alloc::Flags,
>> +    ) -> Result<impl PinInit<Self, Error> + '_> {
>> +        let page_iter = pages.page_iter();
>> +        let size = page_iter.size();
>
> Variable naming here is confusing. There's another variable called size
> in an inner scope, and then afterwards in RawSGTable you use *this* size
> variable again.

I can change the size in the assignment block of max_segment to max_size, or do
you have other suggestions?

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alice Ryhl]

On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
> >> +impl<P> Owned<P>
> >> +where
> >> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> >> +{
> >> +    fn new(
> >> +        dev: &Device<Bound>,
> >> +        mut pages: P,
> >> +        dir: dma::DataDirection,
> >> +        flags: alloc::Flags,
> >> +    ) -> Result<impl PinInit<Self, Error> + '_> {
> >> +        let page_iter = pages.page_iter();
> >> +        let size = page_iter.size();
> >
> > Variable naming here is confusing. There's another variable called size
> > in an inner scope, and then afterwards in RawSGTable you use *this* size
> > variable again.
> 
> I can change the size in the assignment block of max_segment to max_size, or do
> you have other suggestions?

How about just this?

let max_segment = {
    // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
    let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
    if max_segment == 0 {
        u32::MAX
    } else {
        u32::try_from(max_segment).unwrap_or(u32::MAX)
    }
};

Alice

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On 8/22/25 1:52 PM, Alice Ryhl wrote:
> On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
>> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>>>> +impl<P> Owned<P>
>>>> +where
>>>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>>>> +{
>>>> +    fn new(
>>>> +        dev: &Device<Bound>,
>>>> +        mut pages: P,
>>>> +        dir: dma::DataDirection,
>>>> +        flags: alloc::Flags,
>>>> +    ) -> Result<impl PinInit<Self, Error> + '_> {
>>>> +        let page_iter = pages.page_iter();
>>>> +        let size = page_iter.size();
>>>
>>> Variable naming here is confusing. There's another variable called size
>>> in an inner scope, and then afterwards in RawSGTable you use *this* size
>>> variable again.
>>
>> I can change the size in the assignment block of max_segment to max_size, or do
>> you have other suggestions?
> 
> How about just this?
> 
> let max_segment = {
>      // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
>      let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
>      if max_segment == 0 {
>          u32::MAX
>      } else {
>          u32::try_from(max_segment).unwrap_or(u32::MAX)
>      }
> };

Looks good, thanks!

Re: [PATCH v2 0/5] Rust infrastructure for sg_table and scatterlist

[Alexandre Courbot]

On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
> This patch series provides abstractions for struct sg_table and struct
> scatterlist.
>
> Abdiel and me agreed for me to take over his previous iterations on this topic.
> I decided to send my patches as a new series rather than as a subsequent version
> of Abdiel's previous iterations, since the changes I made turned out to be much
> closer to a full rewrite.
>
> The most notable differences in design are:
>
>   - SGTable utilizes BorrowedPage, AsPageIter and VmallocPageIter from my patch
>     series in [1].
>
>   -  SGTable is a transparent wrapper over either struct Owned<P> (where P is
>      the provider of the backing pages) or struct Borrowed, which by itself is a
>      transparent wrapper over Opaque<bindings::sg_table>, i.e. either
>      SGTable<Owned<P>> or just SGTable (which is equivalent to
>      SGTable<Borrowed>.
>
>      - `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.
>
>      - As a consequence, a borrowed SG table can be created with
>        SGTable::as_ref(), which returns a &'a SGTable, just like similar
>        existing abstractions.
>
>        An owned SGTable is created with SGTable::new(), which returns an
>        impl PinInit<SGTable<Owned<P>>, Error>, such that it can be initialized
>        directly within existing private data memory allocations while providing
>        the required pin guarantees.
>
>   - SGTable<Owned<P>> uses an inner type Devres<DmaMapSgt> to ensure that the
>     DMA mapping can't out-live device unbind.
>
>   - SGTable<Owned<P>> uses pin-init for initialization.
>
> This patch series depends on [1] (branch containing the patches in [2]). A
> branch containing this series (including dependencies) can be found in [3];
> Abdiel's latest series can be found in [4].
>
> [1] https://lore.kernel.org/rust-for-linux/20250820145434.94745-1-dakr@kernel.org/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=page-iter
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=scatterlist
> [4] https://lore.kernel.org/lkml/20250718103359.1026240-1-abdiel.janulgue@gmail.com/

I am going to do a full review of this revision over the weekend, but I
have already successfully used it in nova-core to boot the GSP. Thus,

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH v2 1/5] rust: dma: implement DataDirection

[Alexandre Courbot]

On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
> Add the `DataDirection` struct, a newtype wrapper around the C
> `enum dma_data_direction`.
>
> This provides a type-safe Rust interface for specifying the direction of
> DMA transfers.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

One very very minor comment below.

> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/dma.rs              | 68 +++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 0e140e07758b..c2cc52ee9945 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -47,6 +47,7 @@
>  #include <linux/cpumask.h>
>  #include <linux/cred.h>
>  #include <linux/device/faux.h>
> +#include <linux/dma-direction.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 2bc8ab51ec28..5daba00ecc78 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -244,6 +244,74 @@ pub mod attrs {
>      pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
>  }
>  
> +/// DMA data direction.
> +///
> +/// Corresponds to the C [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +#[repr(u32)]
> +pub enum DataDirection {
> +    /// The DMA mapping is for bidirectional data transfer.
> +    ///
> +    /// This is used when the buffer can be both read from and written to by the device.
> +    /// The cache for the corresponding memory region is both flushed and invalidated.
> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
> +
> +    /// The DMA mapping is for data transfer from memory to the device (write).
> +    ///
> +    /// The CPU has prepared data in the buffer, and the device will read it.
> +    /// The cache for the corresponding memory region is flushed.

Is flushed when? :) The comment for `FromDevice` specifies the timing
for cache invalidation, so it would be nice to have the same here imho.

Re: [PATCH v2 2/5] rust: dma: add type alias for bindings::dma_addr_t

[Alexandre Courbot]

On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
> Add a type alias for bindings::dma_addr_t (DmaAddress), such that we do
> not have to access bindings directly.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

On Fri Aug 22, 2025 at 8:54 PM JST, Danilo Krummrich wrote:
> On 8/22/25 1:52 PM, Alice Ryhl wrote:
>> On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
>>> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>>>>> +impl<P> Owned<P>
>>>>> +where
>>>>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>>>>> +{
>>>>> +    fn new(
>>>>> +        dev: &Device<Bound>,
>>>>> +        mut pages: P,
>>>>> +        dir: dma::DataDirection,
>>>>> +        flags: alloc::Flags,
>>>>> +    ) -> Result<impl PinInit<Self, Error> + '_> {
>>>>> +        let page_iter = pages.page_iter();
>>>>> +        let size = page_iter.size();
>>>>
>>>> Variable naming here is confusing. There's another variable called size
>>>> in an inner scope, and then afterwards in RawSGTable you use *this* size
>>>> variable again.
>>>
>>> I can change the size in the assignment block of max_segment to max_size, or do
>>> you have other suggestions?
>> 
>> How about just this?
>> 
>> let max_segment = {
>>      // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
>>      let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
>>      if max_segment == 0 {
>>          u32::MAX
>>      } else {
>>          u32::try_from(max_segment).unwrap_or(u32::MAX)
>>      }
>> };
>
> Looks good, thanks!

Would this also work? It's a bit shorter.

// SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
let max_segment = match unsafe { bindings::dma_max_mapping_size(dev.as_raw()) } {
    0 => u32::MAX,
    max_segment => u32::try_from(max_segment).unwrap_or(u32::MAX),
};

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

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@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.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

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

Oops, forgot to mention a couple more things:

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.

Is this actually a typestate? From my understanding, the typestate
pattern implies transitions from one state to the other (such as
Unmapped -> Mapped), but in this version there are no such transitions
(the previous ones had, though). We are just using a generic parameter,
so mentioning typestate sounds a bit misleading to me.

Another random thought, in the owned case, do we want to provide an
accessor to the provider of the backing pages? Or do we expect the
caller to take dispositions to keep such a reference if they need to
access the backing buffer post-mapping?

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Sat Aug 23, 2025 at 3:22 PM CEST, Alexandre Courbot wrote:
> On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
>> +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".

"guarantee" seems correct to me; it's "requirements" not "requirement".

(I think we commonly use the plural, i.e. "requirements" even if we end up
listing a single requirement only.)

> <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".

I'm not a native speaker, but I think "an" is correct, since "sg_table" is
pronounced with a vowel sound, /ɛs/, at the beginning.

>> +    ///
>> +    /// # 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 }SGEntry
>> +    }
>> +}
>> +
>> +/// # Invariants
>> +///
>> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>
> nit: "of the".

This one I don't know for sure, maybe a native speaker can help.

I chose "for", since I think it indicates duration and "of" rather belonging,
but I honestly don't know. :)

>> +/// - `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. :)

I think it makes sense to rename to SGTable::iter() and make it public.

I'm also fine removing the IntoIterator implementation, it seems pretty unlikely
that we'll have another type that provides an Iterator with SGEntry items we
need a generic interface for.

>> +
>> +/// 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.

[...]

> follow the advice given by the documentation of
> `sg_dma_address` and also stop if the DMA length of the next one is
> zero.

Doh! I was even aware of this before sending the initial version and simply
forgot to add this stop condition after having been interrupted.

Thanks a lot for catching this!

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
> Oops, forgot to mention a couple more things:
>
> 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.
>
> Is this actually a typestate? From my understanding, the typestate
> pattern implies transitions from one state to the other (such as
> Unmapped -> Mapped), but in this version there are no such transitions
> (the previous ones had, though). We are just using a generic parameter,
> so mentioning typestate sounds a bit misleading to me.

I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
&SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
uni-directional transition I guess.

> Another random thought, in the owned case, do we want to provide an
> accessor to the provider of the backing pages? Or do we expect the
> caller to take dispositions to keep such a reference if they need to
> access the backing buffer post-mapping?

That's not going to work that easily. Once the backing pages are DMA mapped, the
backing buffer can be accessed safely an more.

See also the safety requirements of dma::CoherentAllocation::as_slice() and
dma::CoherentAllocation::as_slice_mut().

If we want to support that, we have to provide a new type for this and maybe
want to define a common trait for DMA mapped memory accessors, etc.

Not the scope for this series, I believe. :)

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

On Sat Aug 23, 2025 at 10:48 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:22 PM CEST, Alexandre Courbot wrote:
>> On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
>>> +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".
>
> "guarantee" seems correct to me; it's "requirements" not "requirement".
>
> (I think we commonly use the plural, i.e. "requirements" even if we end up
> listing a single requirement only.)

Ah, you are correct! I missed the plural on "requirements".

>
>> <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".
>
> I'm not a native speaker, but I think "an" is correct, since "sg_table" is
> pronounced with a vowel sound, /ɛs/, at the beginning.

TIL. :)

>
>>> +    ///
>>> +    /// # 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 }SGEntry
>>> +    }
>>> +}
>>> +
>>> +/// # Invariants
>>> +///
>>> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>>
>> nit: "of the".
>
> This one I don't know for sure, maybe a native speaker can help.
>
> I chose "for", since I think it indicates duration and "of" rather belonging,
> but I honestly don't know. :)

I didn't give enough context. I meant "of the [`DmaMapSgt`]" (as in, it
cannot be any DmaMapSgt; it has to be this particular one).

>
>>> +/// - `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. :)
>
> I think it makes sense to rename to SGTable::iter() and make it public.
>
> I'm also fine removing the IntoIterator implementation, it seems pretty unlikely
> that we'll have another type that provides an Iterator with SGEntry items we
> need a generic interface for.

I assumed there was some Rust pattern to this `IntoIterator`
implementation on a reference, but I cannot see its usefulness when an
`iter` method also works on a reference anyway. So yeah if there is no
reason against it I think this would be more intuitive.

>
>>> +
>>> +/// 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.
>
> [...]
>
>> follow the advice given by the documentation of
>> `sg_dma_address` and also stop if the DMA length of the next one is
>> zero.
>
> Doh! I was even aware of this before sending the initial version and simply
> forgot to add this stop condition after having been interrupted.
>
> Thanks a lot for catching this!

A detail whose knowledge is typically acquired through considerable
suffering. :)

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>> Oops, forgot to mention a couple more things:
>>
>> 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.
>>
>> Is this actually a typestate? From my understanding, the typestate
>> pattern implies transitions from one state to the other (such as
>> Unmapped -> Mapped), but in this version there are no such transitions
>> (the previous ones had, though). We are just using a generic parameter,
>> so mentioning typestate sounds a bit misleading to me.
>
> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
> uni-directional transition I guess.

That's technically correct, but is also not the intent of the design, at
least compared to something like Unmapped <-> Mapped. Not a big problem
if you prefer to keep the current naming though.

>
>> Another random thought, in the owned case, do we want to provide an
>> accessor to the provider of the backing pages? Or do we expect the
>> caller to take dispositions to keep such a reference if they need to
>> access the backing buffer post-mapping?
>
> That's not going to work that easily. Once the backing pages are DMA mapped, the
> backing buffer can be accessed safely an more.
>
> See also the safety requirements of dma::CoherentAllocation::as_slice() and
> dma::CoherentAllocation::as_slice_mut().

Yup. So couldn't similar accessors (marked unsafe of course) be
convenient?

>
> If we want to support that, we have to provide a new type for this and maybe
> want to define a common trait for DMA mapped memory accessors, etc.
>
> Not the scope for this series, I believe. :)

I've had a few thoughts in that direction as well, but completely agree
we should debate about this *after* this series is merged. :)

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Sat Aug 23, 2025 at 4:16 PM CEST, Alexandre Courbot wrote:
> On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
>> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>>> Oops, forgot to mention a couple more things:
>>>
>>> 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.
>>>
>>> Is this actually a typestate? From my understanding, the typestate
>>> pattern implies transitions from one state to the other (such as
>>> Unmapped -> Mapped), but in this version there are no such transitions
>>> (the previous ones had, though). We are just using a generic parameter,
>>> so mentioning typestate sounds a bit misleading to me.
>>
>> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
>> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
>> uni-directional transition I guess.
>
> That's technically correct, but is also not the intent of the design, at
> least compared to something like Unmapped <-> Mapped. Not a big problem
> if you prefer to keep the current naming though.

I don't mind to name / call it differently, any suggestion?

>>
>>> Another random thought, in the owned case, do we want to provide an
>>> accessor to the provider of the backing pages? Or do we expect the
>>> caller to take dispositions to keep such a reference if they need to
>>> access the backing buffer post-mapping?
>>
>> That's not going to work that easily. Once the backing pages are DMA mapped, the
>> backing buffer can be accessed safely an more.
>>
>> See also the safety requirements of dma::CoherentAllocation::as_slice() and
>> dma::CoherentAllocation::as_slice_mut().
>
> Yup. So couldn't similar accessors (marked unsafe of course) be
> convenient?

Absolutely! But I think we want them represented by a common trait that can be
used by SGTable and dma::CoherentAllocation.

>>
>> If we want to support that, we have to provide a new type for this and maybe
>> want to define a common trait for DMA mapped memory accessors, etc.
>>
>> Not the scope for this series, I believe. :)
>
> I've had a few thoughts in that direction as well, but completely agree
> we should debate about this *after* this series is merged. :)

Yeah, let's add this feature subsequently.

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Alexandre Courbot]

On Sat Aug 23, 2025 at 11:20 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 4:16 PM CEST, Alexandre Courbot wrote:
>> On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
>>> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>>>> Oops, forgot to mention a couple more things:
>>>>
>>>> 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.
>>>>
>>>> Is this actually a typestate? From my understanding, the typestate
>>>> pattern implies transitions from one state to the other (such as
>>>> Unmapped -> Mapped), but in this version there are no such transitions
>>>> (the previous ones had, though). We are just using a generic parameter,
>>>> so mentioning typestate sounds a bit misleading to me.
>>>
>>> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
>>> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
>>> uni-directional transition I guess.
>>
>> That's technically correct, but is also not the intent of the design, at
>> least compared to something like Unmapped <-> Mapped. Not a big problem
>> if you prefer to keep the current naming though.
>
> I don't mind to name / call it differently, any suggestion?

Simply using "generic parameter" would lift the possiblity for
misinterpretation IMHO.

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Jason Gunthorpe]

On Sat, Aug 23, 2025 at 10:22:47PM +0900, Alexandre Courbot wrote:

> 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.")

> 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

Correct, this is misusing the API, and I don't know if the lengths are
even guarenteed to be zero. To iterate the DMA list you must use the
length of the DMA list returned by dma_map_sg() and nothing else as
the stop condition.

To repeat again, the scatterlist data structure is "optimized" and
contains two completely different lists - the CPU list and the DMA
list. The DMA list is always <= the size of the CPU list.

For all purposes they are completely seperate things and we have a
unique set of iterators and accessors for the CPU vs DMA data.

Jason

Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Sat Aug 23, 2025 at 4:32 PM CEST, Jason Gunthorpe wrote:
> Correct, this is misusing the API, and I don't know if the lengths are
> even guarenteed to be zero.

Hopefully it is. Otherwise, it's not only the documentation if sg_dma_address()
being wrong, but also a few drivers and maybe iommu_dma_unmap_sg(). However,
since it's a specific implementation of the API, it might be correct relying on
the sg_dma_len() == 0 check.

In any case, I think we can just use the nents field of struct sg_table, just
like for_each_sgtable_dma_sg() does.