[PATCH v2 0/2] rust: add initial scatterlist abstraction

[PATCH v2 0/2] rust: add initial scatterlist abstraction

[Abdiel Janulgue]

Hi

Took some time to re-work this after the initial feedback but here is
v2 of the scatterlist abstraction.

Basically what this version accomplishes is attempt to enforce proper
use of the scatterlist api for device drivers in Rust. This comes down
to a couple of things:

(1) Ensure ownership of the backing pages is moved to the sg table.

This is done via the SGTablePages trait. By allowing users to implement
this trait, we allow users flexibility in how the list of pages are
stored and organised. During construction, the sg table takes ownership
of an object implementing the SGTablePages trait to build and allocate
the sg table.

(2) Prevent invalid use of the API such as retrieving the DMA address of
entries of an sg table which is not yet mapped for DMA. Another invalid
use is setting the page entries of an sg table which is already mapped
for DMA.

Safe use is enforced using Rust's newtype pattern which allows us to
ensure that only specific variants of `SGTable` objects are allowed to
safely return an iterator. For example a `DeviceSGTable` (a sg table that
is mapped for DMA operation) is the only object allowed to safely iterate
and retrieve the DMA address for sg entries. A `DeviceSGTable` object
likewise is returned only by the safe variant of the dma_map function.
Also, the set_pages interface is now hidden from users of the API and is
only internally called when building the table.

Lastly, a glue layer provides unsafe interfaces to help in writing
Rust abstractions for other kernel subsystems is provided (currently
targeting Lyude's gem shmem work).

I'd like to acknowledge Alexandre Courbot for providing the feedback and
initial idea for the SGTablePages trait approach.

Changes since v2:
- Drop typestate pattern. Introduce SGTablePages trait to enforce ownership
  of the pages to SGTable.

Link to v1: https://lore.kernel.org/lkml/20250528221525.1705117-1-abdiel.janulgue@gmail.com/

Abdiel Janulgue (2):
  rust: add initial scatterlist bindings
  samples: rust: add sample code for scatterlist bindings

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/scatterlist.c      |  30 +++
 rust/kernel/dma.rs              |  18 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 390 ++++++++++++++++++++++++++++++++
 samples/rust/rust_dma.rs        |  29 ++-
 7 files changed, 469 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs


base-commit: 0303584766b7bdb6564c7e8f13e0b59b6ef44984
-- 
2.43.0

[PATCH v2 1/2] rust: add initial scatterlist bindings

[Abdiel Janulgue]

Add the rust abstraction for scatterlist. This allows use of the C
scatterlist within Rust code which the caller can allocate themselves
or to wrap existing kernel sg_table objects.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/scatterlist.c      |  30 +++
 rust/kernel/dma.rs              |  18 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 390 ++++++++++++++++++++++++++++++++
 6 files changed, 441 insertions(+)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8cbb660e2ec2..e1e289284ce8 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -47,6 +47,7 @@
 #include <linux/cred.h>
 #include <linux/device/faux.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-direction.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/file.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index b15b3cddad4e..199a83ff3583 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -35,6 +35,7 @@
 #include "rbtree.c"
 #include "rcu.c"
 #include "refcount.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..c871de853539
--- /dev/null
+++ b/rust/helpers/scatterlist.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-direction.h>
+
+void rust_helper_sg_set_page(struct scatterlist *sg, struct page *page,
+			     unsigned int len, unsigned int offset)
+{
+	return sg_set_page(sg, page, len, offset);
+}
+
+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/dma.rs b/rust/kernel/dma.rs
index 8e317005decd..b44c84928f1a 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -102,6 +102,24 @@ pub mod attrs {
     pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
 }
 
+/// DMA mapping direction.
+///
+/// Corresponds to the kernel's [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[repr(i32)]
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub enum DmaDataDirection {
+    /// Direction isn't known.
+    DmaBidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
+    /// Data is going from the memory to the device.
+    DmaToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
+    /// Data is coming from the device to the memory.
+    DmaFromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
+    /// No direction (used for debugging).
+    DmaNone = bindings::dma_data_direction_DMA_NONE,
+}
+
 /// 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
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..476c4a3de9c6 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -101,6 +101,7 @@
 pub mod print;
 pub mod rbtree;
 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..2e17a56ea7c7
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Scatterlist
+//!
+//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
+
+use crate::{
+    bindings,
+    device::{Bound, Device},
+    dma::DmaDataDirection,
+    error::{Error, Result},
+    page::Page,
+    types::{ARef, Opaque},
+};
+
+/// A single scatter-gather entry, representing a span of pages in the device's DMA address space.
+///
+/// This interface is accessible only via the `SGTable` iterators. When using the API safely, certain
+/// methods are only available depending on a specific state of operation of the scatter-gather table,
+/// i.e. setting page entries is done internally only during construction while retrieving the DMA address
+/// is only possible when the `SGTable` is already mapped for DMA via a device.
+///
+/// # Invariants
+///
+/// The `scatterlist` pointer is valid for the lifetime of an SGEntry instance.
+#[repr(transparent)]
+pub struct SGEntry(Opaque<bindings::scatterlist>);
+
+impl SGEntry {
+    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the lifetime
+    /// of the returned reference.
+    pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
+        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Convert a raw `struct scatterlist *` to a `&'a mut SGEntry`.
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    ///
+    /// # Safety
+    ///
+    /// See safety requirements of [`SGEntry::as_ref`]. In addition, callers must ensure that only
+    /// a single mutable reference can be taken from the same raw pointer, i.e. for the lifetime of the
+    /// returned reference, no other call to this function on the same `struct scatterlist *` should
+    /// be permitted.
+    pub(crate) unsafe fn as_mut<'a>(ptr: *mut bindings::scatterlist) -> &'a mut Self {
+        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// Obtain the raw `struct scatterlist *`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::scatterlist {
+        self.0.get()
+    }
+
+    /// Returns the DMA address of this SG entry.
+    pub fn dma_address(&self) -> bindings::dma_addr_t {
+        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
+        unsafe { bindings::sg_dma_address(self.0.get()) }
+    }
+
+    /// Returns the length of this SG entry.
+    pub fn dma_len(&self) -> u32 {
+        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
+        unsafe { bindings::sg_dma_len(self.0.get()) }
+    }
+
+    /// Internal constructor helper to set this entry to point at a given page. Not to be used directly.
+    fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
+        let c: *mut bindings::scatterlist = self.0.get();
+        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
+        // `Page` invariant also ensure the pointer is valid.
+        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
+    }
+}
+
+/// A scatter-gather table of DMA address spans.
+///
+/// This structure represents the Rust abstraction for a C `struct sg_table`. This implementation
+/// abstracts the usage of an already existing C `struct sg_table` within Rust code that we get
+/// passed from the C side.
+///
+/// Note that constructing a new scatter-gather object using [`SGTable::alloc_table`] enforces safe
+/// and proper use of the API.
+///
+/// # Invariants
+///
+/// The `sg_table` pointer is valid for the lifetime of an SGTable instance.
+#[repr(transparent)]
+pub struct SGTable(Opaque<bindings::sg_table>);
+
+impl SGTable {
+    /// Convert a raw `struct sg_table *` to a `&'a SGTable`.
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is initialized and valid for
+    /// the lifetime of the returned reference.
+    #[allow(unused)]
+    pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Convert a raw `struct sg_table *` to a `&'a mut SGTable`.
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    ///
+    /// # Safety
+    ///
+    /// See safety requirements of [`SGTable::as_ref`]. In addition, callers must ensure that only
+    /// a single mutable reference can be taken from the same raw pointer, i.e. for the lifetime of the
+    /// returned reference, no other call to this function on the same `struct sg_table *` should
+    /// be permitted.
+    #[allow(unused)]
+    pub(crate) unsafe fn as_mut<'a>(ptr: *mut bindings::sg_table) -> &'a mut Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// Obtain the raw `struct sg_table *`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::sg_table {
+        self.0.get()
+    }
+
+    /// Allocate and build a new scatter-gather table from an existing list of `pages`. This method
+    /// moves the ownership of `pages` to the table.
+    ///
+    /// To build a scatter-gather table, provide the `pages` object which must implement the
+    /// `SGTablePages` trait.
+    ///
+    ///# Examples
+    ///
+    /// ```
+    /// use kernel::{device::Device, scatterlist::*, page::*, prelude::*};
+    ///
+    /// struct PagesArray(KVec<Page>);
+    /// impl SGTablePages for PagesArray {
+    ///     fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
+    ///         self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))
+    ///     }
+    ///
+    ///     fn entries(&self) -> usize {
+    ///         self.0.len()
+    ///     }
+    /// }
+    ///
+    /// let mut pages = KVec::new();
+    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
+    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
+    /// let sgt = SGTable::alloc_table(PagesArray(pages), GFP_KERNEL)?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn alloc_table<P: SGTablePages>(
+        pages: P,
+        flags: kernel::alloc::Flags,
+    ) -> Result<SGTablePageList<P>> {
+        let sgt: Opaque<bindings::sg_table> = Opaque::uninit();
+
+        // SAFETY: The sgt pointer is from the Opaque-wrapped `sg_table` object hence is valid.
+        let ret =
+            unsafe { bindings::sg_alloc_table(sgt.get(), pages.entries() as u32, flags.as_raw()) };
+        if ret != 0 {
+            return Err(Error::from_errno(ret));
+        }
+        let mut sgtable = Self(sgt);
+        let sgentries = sgtable.iter_mut().zip(pages.iter());
+        for entry in sgentries {
+            let page = entry.1;
+            entry.0.set_page(page.0, page.1 as u32, page.2 as u32)
+        }
+
+        // INVARIANT: We just successfully allocated and built the table from the page entries.
+        Ok(SGTablePageList { sg: sgtable, pages })
+    }
+
+    /// Map this scatter-gather table describing a buffer for DMA by the `Device`.
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    /// See also the safe version [`SGTablePageList::dma_map`] which enforces the safety requirements below.
+    ///
+    /// # Safety
+    ///
+    /// * The caller takes responsibility that the sg entries in the scatter-gather table object are
+    ///   already populated beforehand.
+    /// * The caller takes responsibility that the table does not get mapped more than once to prevent UB.
+    pub(crate) unsafe fn dma_map_raw(&self, dev: &Device<Bound>, dir: DmaDataDirection) -> Result {
+        // SAFETY: Invariants on `Device` and `SGTable` ensures that the pointers are valid.
+        let ret = unsafe {
+            bindings::dma_map_sgtable(
+                dev.as_raw(),
+                self.as_raw(),
+                dir as i32,
+                bindings::DMA_ATTR_NO_WARN as usize,
+            )
+        };
+        if ret != 0 {
+            return Err(Error::from_errno(ret));
+        }
+        Ok(())
+    }
+
+    /// Returns an immutable iterator over the scatter-gather table that is mapped for DMA. The iterator
+    /// serves as an interface to retrieve the DMA address of the entries
+    ///
+    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
+    /// See also the safe version [`DeviceSGTable::iter`] which enforces the safety requirement below.
+    ///
+    /// # Safety
+    ///
+    /// Callers take responsibility that `self` is already mapped for DMA by a device.
+    pub(crate) unsafe fn iter_raw(&self) -> SGTableIter<'_> {
+        SGTableIter {
+            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
+            pos: Some(unsafe { SGEntry::as_ref((*self.0.get()).sgl) }),
+        }
+    }
+
+    /// Internal helper to create a mutable iterator for the constructor when building the table. Not
+    /// to be used directly.
+    fn iter_mut(&mut self) -> SGTableIterMut<'_> {
+        SGTableIterMut {
+            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`. This call
+            // is within a private method called only within the `[SGTable::alloc_table]` constructor
+            // ensuring that the mutable reference can only be obtained once for this object.
+            pos: Some(unsafe { SGEntry::as_mut((*self.0.get()).sgl) }),
+        }
+    }
+}
+
+/// Provides a list of pages that can be used to build a `SGTable`.
+pub trait SGTablePages {
+    /// Returns an iterator to the pages providing the backing memory of `self`.
+    ///
+    /// Implementers should return an iterator which provides information regarding each page entry to
+    /// build the `SGTable`. The first element in the tuple is a reference to the Page, the second element
+    /// as the offset into the page, and the third as the length of data. The fields correspond to the
+    /// first three fields of the C `struct scatterlist`.
+    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)>;
+
+    /// Returns the number of pages in the list.
+    fn entries(&self) -> usize;
+}
+
+/// A scatter-gather table that owns the page entries.
+///
+/// # Invariants
+///
+/// The scatter-gather table is valid and initialized with sg entries.
+pub struct SGTablePageList<P: SGTablePages> {
+    sg: SGTable,
+    pages: P,
+}
+
+impl<P: SGTablePages> SGTablePageList<P> {
+    /// Map this scatter-gather table describing a buffer for DMA by the `Device`.
+    ///
+    /// To prevent the table from being mapped more than once, this call consumes `self` and transfers
+    /// ownership of resources to the new `DeviceSGTable` object.
+    pub fn dma_map(self, dev: &Device<Bound>, dir: DmaDataDirection) -> Result<DeviceSGTable<P>> {
+        // SAFETY: By the type invariant, `self.sg` is already built with valid sg entries. Since we are
+        // in a method that consumes self, it also ensures that dma_map_raw is only called once for
+        // this `SGTable`.
+        unsafe {
+            self.sg.dma_map_raw(dev, dir)?;
+        }
+
+        // INVARIANT: We just successfully mapped the table for DMA.
+        Ok(DeviceSGTable {
+            sg: self.sg,
+            dir,
+            dev: dev.into(),
+            _pages: self.pages,
+        })
+    }
+}
+
+/// A scatter-gather table that is mapped for DMA operation.
+///
+/// # Invariants
+///
+/// The scatter-gather table is mapped for DMA operation.
+pub struct DeviceSGTable<P: SGTablePages> {
+    sg: SGTable,
+    dir: DmaDataDirection,
+    dev: ARef<Device>,
+    _pages: P,
+}
+
+impl<P: SGTablePages> DeviceSGTable<P> {
+    /// Returns an immutable iterator over the scather-gather table that is mapped for DMA. The iterator
+    /// serves as an interface to retrieve the DMA address of the entries
+    pub fn iter(&self) -> SGTableIter<'_> {
+        // SAFETY: By the type invariant, `self.sg` is already mapped for DMA.
+        unsafe { self.sg.iter_raw() }
+    }
+}
+
+impl<P: SGTablePages> Drop for DeviceSGTable<P> {
+    fn drop(&mut self) {
+        // SAFETY: Invariants on `Device<Bound>` and `SGTable` ensures that the `self.dev` and `self.sg`
+        // pointers are valid.
+        unsafe {
+            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sg.as_raw(), self.dir as i32, 0)
+        };
+    }
+}
+
+/// SAFETY: A `SGTable<Mapped>` is an immutable interface and should be safe to `Send` across threads.
+unsafe impl Send for SGTable {}
+
+/// A mutable iterator through `SGTable` entries.
+pub struct SGTableIterMut<'a> {
+    pos: Option<&'a mut SGEntry>,
+}
+
+impl<'a> Iterator for SGTableIterMut<'a> {
+    type Item = &'a mut SGEntry;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.pos.take().map(|entry| {
+            let sg = entry.as_raw();
+            // SAFETY: `sg` is guaranteed to be valid and non-NULL while inside this closure.
+            let next = unsafe { bindings::sg_next(sg) };
+            self.pos = (!next.is_null()).then(||
+                                              // SAFETY: `SGEntry::as_mut` is called on `next` only once,
+                                              // which is valid and non-NULL
+                                              // inside the closure.
+                                              unsafe { SGEntry::as_mut(next) });
+            // SAFETY: `SGEntry::as_mut` is called on `sg` only once, which is valid and non-NULL
+            // inside the closure.
+            unsafe { SGEntry::as_mut(sg) }
+        })
+    }
+}
+
+impl<'a> IntoIterator for &'a mut SGTable {
+    type Item = &'a mut SGEntry;
+    type IntoIter = SGTableIterMut<'a>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.iter_mut()
+    }
+}
+
+/// An iterator through `SGTable` entries.
+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: `sg` is an immutable reference and is equivalent to `scatterlist` via its type
+        // invariants, so its safe to use with sg_next.
+        let next = unsafe { bindings::sg_next(self.pos?.as_raw()) };
+
+        // SAFETY: `sg_next` returns either a valid pointer to a `scatterlist`, or null if we
+        // are at the end of the scatterlist.
+        self.pos = (!next.is_null()).then(|| unsafe { SGEntry::as_ref(next) });
+        entry
+    }
+}
+
+impl<'a, P: SGTablePages> IntoIterator for &'a DeviceSGTable<P> {
+    type Item = &'a SGEntry;
+    type IntoIter = SGTableIter<'a>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.iter()
+    }
+}
+
+impl Drop for SGTable {
+    fn drop(&mut self) {
+        // SAFETY: Invariant on `SGTable` ensures that the sg_table is valid.
+        unsafe { bindings::sg_free_table(self.as_raw()) };
+    }
+}
-- 
2.43.0

[PATCH v2 2/2] samples: rust: add sample code for scatterlist bindings

[Abdiel Janulgue]

Add simple excercises to test the scatterlist bindings.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 samples/rust/rust_dma.rs | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 874c2c964afa..c4d44e5a45b8 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -4,11 +4,15 @@
 //!
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
-use kernel::{bindings, device::Core, dma::CoherentAllocation, pci, prelude::*, types::ARef};
+use kernel::{
+    bindings, device::Core, dma::CoherentAllocation, page::*, pci, prelude::*, scatterlist::*,
+    types::ARef,
+};
 
 struct DmaSampleDriver {
     pdev: ARef<pci::Device>,
     ca: CoherentAllocation<MyStruct>,
+    sgt: DeviceSGTable<PagesArray>,
 }
 
 const TEST_VALUES: [(u32, u32); 5] = [
@@ -34,6 +38,18 @@ unsafe impl kernel::transmute::AsBytes for MyStruct {}
 // SAFETY: Instances of `MyStruct` have no uninitialized portions.
 unsafe impl kernel::transmute::FromBytes for MyStruct {}
 
+struct PagesArray(KVec<Page>);
+
+impl SGTablePages for PagesArray {
+    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
+        self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))
+    }
+
+    fn entries(&self) -> usize {
+        self.0.len()
+    }
+}
+
 kernel::pci_device_table!(
     PCI_TABLE,
     MODULE_PCI_TABLE,
@@ -62,10 +78,20 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
             Ok(())
         }()?;
 
+        let mut pages = KVec::new();
+        for _ in TEST_VALUES.into_iter() {
+            let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
+        }
+
+        let sgt = SGTable::alloc_table(PagesArray(pages), GFP_KERNEL)?;
+        let sgt = sgt.dma_map(pdev.as_ref(), kernel::dma::DmaDataDirection::DmaToDevice)?;
+
         let drvdata = KBox::new(
             Self {
                 pdev: pdev.into(),
                 ca,
+                // Save object to excercise the destructor.
+                sgt,
             },
             GFP_KERNEL,
         )?;
@@ -77,6 +103,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
 impl Drop for DmaSampleDriver {
     fn drop(&mut self) {
         dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
+        assert_eq!(self.sgt.iter().count(), TEST_VALUES.len());
 
         let _ = || -> Result {
             for (i, value) in TEST_VALUES.into_iter().enumerate() {
-- 
2.43.0

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Alexandre Courbot]

On Fri Jun 27, 2025 at 5:30 AM JST, Abdiel Janulgue wrote:
> Add the rust abstraction for scatterlist. This allows use of the C
> scatterlist within Rust code which the caller can allocate themselves
> or to wrap existing kernel sg_table objects.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
<snip>
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> new file mode 100644
> index 000000000000..2e17a56ea7c7
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Scatterlist
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +
> +use crate::{
> +    bindings,
> +    device::{Bound, Device},
> +    dma::DmaDataDirection,
> +    error::{Error, Result},
> +    page::Page,
> +    types::{ARef, Opaque},
> +};
> +
> +/// A single scatter-gather entry, representing a span of pages in the device's DMA address space.
> +///
> +/// This interface is accessible only via the `SGTable` iterators. When using the API safely, certain
> +/// methods are only available depending on a specific state of operation of the scatter-gather table,
> +/// i.e. setting page entries is done internally only during construction while retrieving the DMA address
> +/// is only possible when the `SGTable` is already mapped for DMA via a device.
> +///
> +/// # Invariants
> +///
> +/// The `scatterlist` pointer is valid for the lifetime of an SGEntry instance.
> +#[repr(transparent)]
> +pub struct SGEntry(Opaque<bindings::scatterlist>);

I am not sure to understand why you dropped the typestate here. Not all
members of `scatterlist` are valid at all times, and the typestate
allowed us to control which members could be accessed. Dropping it
forces you to define access rules using unsafe methods and invariants,
pushing the responsibility of writing valid code to the user while we
could very well use the compiler for this.

> +
> +impl SGEntry {
> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the lifetime
> +    /// of the returned reference.
> +    pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
> +        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Convert a raw `struct scatterlist *` to a `&'a mut SGEntry`.
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    ///
> +    /// # Safety
> +    ///
> +    /// See safety requirements of [`SGEntry::as_ref`]. In addition, callers must ensure that only
> +    /// a single mutable reference can be taken from the same raw pointer, i.e. for the lifetime of the
> +    /// returned reference, no other call to this function on the same `struct scatterlist *` should
> +    /// be permitted.
> +    pub(crate) unsafe fn as_mut<'a>(ptr: *mut bindings::scatterlist) -> &'a mut Self {
> +        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct scatterlist *`.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::scatterlist {
> +        self.0.get()
> +    }
> +
> +    /// Returns the DMA address of this SG entry.
> +    pub fn dma_address(&self) -> bindings::dma_addr_t {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_address(self.0.get()) }
> +    }
> +
> +    /// Returns the length of this SG entry.
> +    pub fn dma_len(&self) -> u32 {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_len(self.0.get()) }
> +    }
> +
> +    /// Internal constructor helper to set this entry to point at a given page. Not to be used directly.
> +    fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> +        let c: *mut bindings::scatterlist = self.0.get();
> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> +        // `Page` invariant also ensure the pointer is valid.
> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> +    }
> +}
> +
> +/// A scatter-gather table of DMA address spans.
> +///
> +/// This structure represents the Rust abstraction for a C `struct sg_table`. This implementation
> +/// abstracts the usage of an already existing C `struct sg_table` within Rust code that we get
> +/// passed from the C side.
> +///
> +/// Note that constructing a new scatter-gather object using [`SGTable::alloc_table`] enforces safe
> +/// and proper use of the API.
> +///
> +/// # Invariants
> +///
> +/// The `sg_table` pointer is valid for the lifetime of an SGTable instance.
> +#[repr(transparent)]
> +pub struct SGTable(Opaque<bindings::sg_table>);
> +
> +impl SGTable {
> +    /// Convert a raw `struct sg_table *` to a `&'a SGTable`.
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is initialized and valid for
> +    /// the lifetime of the returned reference.
> +    #[allow(unused)]
> +    pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Convert a raw `struct sg_table *` to a `&'a mut SGTable`.
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    ///
> +    /// # Safety
> +    ///
> +    /// See safety requirements of [`SGTable::as_ref`]. In addition, callers must ensure that only
> +    /// a single mutable reference can be taken from the same raw pointer, i.e. for the lifetime of the
> +    /// returned reference, no other call to this function on the same `struct sg_table *` should
> +    /// be permitted.
> +    #[allow(unused)]
> +    pub(crate) unsafe fn as_mut<'a>(ptr: *mut bindings::sg_table) -> &'a mut Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct sg_table *`.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.0.get()
> +    }
> +
> +    /// Allocate and build a new scatter-gather table from an existing list of `pages`. This method
> +    /// moves the ownership of `pages` to the table.
> +    ///
> +    /// To build a scatter-gather table, provide the `pages` object which must implement the
> +    /// `SGTablePages` trait.
> +    ///
> +    ///# Examples
> +    ///
> +    /// ```
> +    /// use kernel::{device::Device, scatterlist::*, page::*, prelude::*};
> +    ///
> +    /// struct PagesArray(KVec<Page>);
> +    /// impl SGTablePages for PagesArray {
> +    ///     fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
> +    ///         self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))
> +    ///     }
> +    ///
> +    ///     fn entries(&self) -> usize {
> +    ///         self.0.len()
> +    ///     }
> +    /// }
> +    ///
> +    /// let mut pages = KVec::new();
> +    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +    /// let sgt = SGTable::alloc_table(PagesArray(pages), GFP_KERNEL)?;
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn alloc_table<P: SGTablePages>(
> +        pages: P,
> +        flags: kernel::alloc::Flags,
> +    ) -> Result<SGTablePageList<P>> {

This returns a `SGTablePageList`, so why is this a method of `SGTable`?

> +        let sgt: Opaque<bindings::sg_table> = Opaque::uninit();
> +
> +        // SAFETY: The sgt pointer is from the Opaque-wrapped `sg_table` object hence is valid.
> +        let ret =
> +            unsafe { bindings::sg_alloc_table(sgt.get(), pages.entries() as u32, flags.as_raw()) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        }
> +        let mut sgtable = Self(sgt);
> +        let sgentries = sgtable.iter_mut().zip(pages.iter());
> +        for entry in sgentries {
> +            let page = entry.1;
> +            entry.0.set_page(page.0, page.1 as u32, page.2 as u32)
> +        }
> +
> +        // INVARIANT: We just successfully allocated and built the table from the page entries.
> +        Ok(SGTablePageList { sg: sgtable, pages })
> +    }
> +
> +    /// Map this scatter-gather table describing a buffer for DMA by the `Device`.
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    /// See also the safe version [`SGTablePageList::dma_map`] which enforces the safety requirements below.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * The caller takes responsibility that the sg entries in the scatter-gather table object are
> +    ///   already populated beforehand.
> +    /// * The caller takes responsibility that the table does not get mapped more than once to prevent UB.

These safety requirements would not be needed if you had a typestate in
`SGTable`. :)

> +    pub(crate) unsafe fn dma_map_raw(&self, dev: &Device<Bound>, dir: DmaDataDirection) -> Result {
> +        // SAFETY: Invariants on `Device` and `SGTable` ensures that the pointers are valid.
> +        let ret = unsafe {
> +            bindings::dma_map_sgtable(
> +                dev.as_raw(),
> +                self.as_raw(),
> +                dir as i32,
> +                bindings::DMA_ATTR_NO_WARN as usize,
> +            )
> +        };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        }
> +        Ok(())
> +    }
> +
> +    /// Returns an immutable iterator over the scatter-gather table that is mapped for DMA. The iterator
> +    /// serves as an interface to retrieve the DMA address of the entries
> +    ///
> +    /// This is meant as a helper for other kernel subsystems and not to be used by device drivers directly.
> +    /// See also the safe version [`DeviceSGTable::iter`] which enforces the safety requirement below.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers take responsibility that `self` is already mapped for DMA by a device.
> +    pub(crate) unsafe fn iter_raw(&self) -> SGTableIter<'_> {
> +        SGTableIter {
> +            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
> +            pos: Some(unsafe { SGEntry::as_ref((*self.0.get()).sgl) }),
> +        }
> +    }
> +
> +    /// Internal helper to create a mutable iterator for the constructor when building the table. Not
> +    /// to be used directly.
> +    fn iter_mut(&mut self) -> SGTableIterMut<'_> {

Just because it is private does not dispense this method from being
unsafe, as `iter_raw` is.

> +        SGTableIterMut {
> +            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`. This call
> +            // is within a private method called only within the `[SGTable::alloc_table]` constructor
> +            // ensuring that the mutable reference can only be obtained once for this object.
> +            pos: Some(unsafe { SGEntry::as_mut((*self.0.get()).sgl) }),
> +        }
> +    }
> +}
> +
> +/// Provides a list of pages that can be used to build a `SGTable`.
> +pub trait SGTablePages {
> +    /// Returns an iterator to the pages providing the backing memory of `self`.
> +    ///
> +    /// Implementers should return an iterator which provides information regarding each page entry to
> +    /// build the `SGTable`. The first element in the tuple is a reference to the Page, the second element
> +    /// as the offset into the page, and the third as the length of data. The fields correspond to the
> +    /// first three fields of the C `struct scatterlist`.
> +    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)>;

How about using a `Range<usize>` instead of an (offset, length) tuple?

> +
> +    /// Returns the number of pages in the list.
> +    fn entries(&self) -> usize;

Maybe `num_entries` or `len`? The current name suggests that the entries
themselves are going to be returned, which is not the case.

> +}
> +
> +/// A scatter-gather table that owns the page entries.
> +///
> +/// # Invariants
> +///
> +/// The scatter-gather table is valid and initialized with sg entries.
> +pub struct SGTablePageList<P: SGTablePages> {
> +    sg: SGTable,
> +    pages: P,
> +}
> +
> +impl<P: SGTablePages> SGTablePageList<P> {
> +    /// Map this scatter-gather table describing a buffer for DMA by the `Device`.
> +    ///
> +    /// To prevent the table from being mapped more than once, this call consumes `self` and transfers
> +    /// ownership of resources to the new `DeviceSGTable` object.
> +    pub fn dma_map(self, dev: &Device<Bound>, dir: DmaDataDirection) -> Result<DeviceSGTable<P>> {
> +        // SAFETY: By the type invariant, `self.sg` is already built with valid sg entries. Since we are
> +        // in a method that consumes self, it also ensures that dma_map_raw is only called once for
> +        // this `SGTable`.
> +        unsafe {
> +            self.sg.dma_map_raw(dev, dir)?;
> +        }
> +
> +        // INVARIANT: We just successfully mapped the table for DMA.
> +        Ok(DeviceSGTable {
> +            sg: self.sg,
> +            dir,
> +            dev: dev.into(),
> +            _pages: self.pages,
> +        })
> +    }
> +}
> +
> +/// A scatter-gather table that is mapped for DMA operation.
> +///
> +/// # Invariants
> +///
> +/// The scatter-gather table is mapped for DMA operation.
> +pub struct DeviceSGTable<P: SGTablePages> {
> +    sg: SGTable,
> +    dir: DmaDataDirection,
> +    dev: ARef<Device>,
> +    _pages: P,
> +}
> +
> +impl<P: SGTablePages> DeviceSGTable<P> {
> +    /// Returns an immutable iterator over the scather-gather table that is mapped for DMA. The iterator
> +    /// serves as an interface to retrieve the DMA address of the entries
> +    pub fn iter(&self) -> SGTableIter<'_> {
> +        // SAFETY: By the type invariant, `self.sg` is already mapped for DMA.
> +        unsafe { self.sg.iter_raw() }
> +    }
> +}
> +
> +impl<P: SGTablePages> Drop for DeviceSGTable<P> {
> +    fn drop(&mut self) {
> +        // SAFETY: Invariants on `Device<Bound>` and `SGTable` ensures that the `self.dev` and `self.sg`
> +        // pointers are valid.
> +        unsafe {
> +            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sg.as_raw(), self.dir as i32, 0)
> +        };
> +    }
> +}
> +
> +/// SAFETY: A `SGTable<Mapped>` is an immutable interface and should be safe to `Send` across threads.
> +unsafe impl Send for SGTable {}
> +
> +/// A mutable iterator through `SGTable` entries.
> +pub struct SGTableIterMut<'a> {
> +    pos: Option<&'a mut SGEntry>,
> +}
> +
> +impl<'a> Iterator for SGTableIterMut<'a> {
> +    type Item = &'a mut SGEntry;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        self.pos.take().map(|entry| {
> +            let sg = entry.as_raw();
> +            // SAFETY: `sg` is guaranteed to be valid and non-NULL while inside this closure.
> +            let next = unsafe { bindings::sg_next(sg) };
> +            self.pos = (!next.is_null()).then(||
> +                                              // SAFETY: `SGEntry::as_mut` is called on `next` only once,
> +                                              // which is valid and non-NULL
> +                                              // inside the closure.
> +                                              unsafe { SGEntry::as_mut(next) });
> +            // SAFETY: `SGEntry::as_mut` is called on `sg` only once, which is valid and non-NULL
> +            // inside the closure.
> +            unsafe { SGEntry::as_mut(sg) }
> +        })
> +    }
> +}
> +
> +impl<'a> IntoIterator for &'a mut SGTable {
> +    type Item = &'a mut SGEntry;
> +    type IntoIter = SGTableIterMut<'a>;
> +
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.iter_mut()
> +    }
> +}

Why is this so far from `SGTable` compared to its other impl blocks?

The fact that this calls the above-mentioned `iter_mut`, which should be
unsafe, is worrying. Basically one can just 

> +
> +/// An iterator through `SGTable` entries.
> +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: `sg` is an immutable reference and is equivalent to `scatterlist` via its type
> +        // invariants, so its safe to use with sg_next.
> +        let next = unsafe { bindings::sg_next(self.pos?.as_raw()) };
> +
> +        // SAFETY: `sg_next` returns either a valid pointer to a `scatterlist`, or null if we
> +        // are at the end of the scatterlist.
> +        self.pos = (!next.is_null()).then(|| unsafe { SGEntry::as_ref(next) });
> +        entry
> +    }
> +}
> +
> +impl<'a, P: SGTablePages> IntoIterator for &'a DeviceSGTable<P> {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.iter()
> +    }
> +}
> +
> +impl Drop for SGTable {
> +    fn drop(&mut self) {
> +        // SAFETY: Invariant on `SGTable` ensures that the sg_table is valid.
> +        unsafe { bindings::sg_free_table(self.as_raw()) };
> +    }
> +}

Ditto.

I actually have some more comments, but I'd like to understasnd first
why you decided to drop the typestate. If there is no good reason, I
think I can provide a more detailed explanation about the design I had
in mind when thinking about Lyude's usecase - basically, a two-stages
typestate SGTable type where the first stage is unsafe, but the second
one leverages SGTablePages and is safe. But going into that explanation
now would be pointless if the typestate cannot be used for some reason.

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Alexandre Courbot]

On Mon Jun 30, 2025 at 5:34 PM JST, Alexandre Courbot wrote:
> I actually have some more comments, but I'd like to understasnd first
> why you decided to drop the typestate. If there is no good reason, I
> think I can provide a more detailed explanation about the design I had
> in mind when thinking about Lyude's usecase - basically, a two-stages
> typestate SGTable type where the first stage is unsafe, but the second
> one leverages SGTablePages and is safe. But going into that explanation
> now would be pointless if the typestate cannot be used for some reason.

After thinking some more about it, I think I can verbalize better my
expectations for this interface (and my problems with this version):

Basically, I believe we should (and can) only need unsafe methods when
using one of the constructors for a SG table. Once the SG table object
is built, nothing in the interface should need to be unsafe, and we
should have access to exactly the features that are safe to use
according to the construction-time invariants. This implies that we have
several constructors, and several types for SG tables - possibly
typestates or completely distinct types as you did in this version.

I wrote a bit of test code trying to match both the requirements of GEM
SHMEM objects (work with an already-existing `sg_table`), and of owned
SG tables ; and I *think* I got something that works by leveraging
`Borrow`. Let me clean up my code after a good night of sleep and I'll
try to elaborate a bit.

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Alexandre Courbot]

On Mon Jun 30, 2025 at 9:56 PM JST, Alexandre Courbot wrote:
> On Mon Jun 30, 2025 at 5:34 PM JST, Alexandre Courbot wrote:
>> I actually have some more comments, but I'd like to understasnd first
>> why you decided to drop the typestate. If there is no good reason, I
>> think I can provide a more detailed explanation about the design I had
>> in mind when thinking about Lyude's usecase - basically, a two-stages
>> typestate SGTable type where the first stage is unsafe, but the second
>> one leverages SGTablePages and is safe. But going into that explanation
>> now would be pointless if the typestate cannot be used for some reason.
>
> After thinking some more about it, I think I can verbalize better my
> expectations for this interface (and my problems with this version):
>
> Basically, I believe we should (and can) only need unsafe methods when
> using one of the constructors for a SG table. Once the SG table object
> is built, nothing in the interface should need to be unsafe, and we
> should have access to exactly the features that are safe to use
> according to the construction-time invariants. This implies that we have
> several constructors, and several types for SG tables - possibly
> typestates or completely distinct types as you did in this version.
>
> I wrote a bit of test code trying to match both the requirements of GEM
> SHMEM objects (work with an already-existing `sg_table`), and of owned
> SG tables ; and I *think* I got something that works by leveraging
> `Borrow`. Let me clean up my code after a good night of sleep and I'll
> try to elaborate a bit.

Alright, that was an interesting rabbit hole, but I think we're closing in.
Apologies, for this is a long post.

Let's first summarize the use-cases we want to support:

- The GEM use-case, where we need to work with `sg_tables` that already exist
  and are owned by some existing entity. We want to "wrap" these into a Rust
  type that provides methods that are always safe to call. In this case the
  Rust `SGTable` does not allocate or free the underlying `sg_table` ; thus
  it is primordial to ensure that it 1) doesn't outlive the `sg_table` and
  2) that the `sg_table` is not modified while we are using it. Let's call it
  the "borrowed" case.

- The Nova use-case, where we want to make an bit of memory (e.g. a `VVec`)
  available to a device. In this case, the Rust `SGTable` DOES own the
  `sg_table` and can even mutate it. However, it shall not outlive the
  backing memory, which must be pinned for as long as the `SGTable` exists.
  Let's call it the "owned" case.

For the borrowed case, there is not much choice but to use an `unsafe`
constructor, with the caller guaranteeing that the expected properties of the
`sg_table` are respected. Conversely, the owned case can be built safely for
any type that provides an implementation of the `SGTablePages` trait (we need
to find a more explicit name for this trait).

So, with this in mind, there are two dimensions around which a `SGTable` can be
built:

1. The ownership or not of the underlying `sg_table`,
2. Whether the `sg_table` is DMA-mapped or not.

For 1., the `Borrow` and `BorrowMut` traits have us covered. If we access the
`sg_table` through them, we can support both the borrowed and owned scenarios.

For 2., a typestate can ensure that only methods that are valid for the given
mapped state of the `SGTable` are visible.

Which makes our `SGTable` look something like this:

  pub struct SGTable<T: Borrow<bindings::sg_table>, M: MappingState> {
      // Declared first so it is dropped first, as we want to unmap before
      // freeing the `sg_table` if we own it.
      mapping: M,
      table: T,
  }

With the mapping typestate being:

  pub trait MappingState {}

  // For sg_tables that are not mapped.
  pub struct Unmapped;
  impl MappingState for Unmapped {}

  // For sg_tables that are mapped, but not managed by us.
  pub struct BorrowedMapping;
  impl MappingState for BorrowedMapping {}

This lets us have constructors to cover the case where we want to wrap an
existing `sg_table`:

  impl<T> SGTable<T, Unmapped>
  where
      T: Borrow<bindings::sg_table>,
  {
      // Safety: 'r' must borrow a sg_table that is unmapped, and which
      // does not change as long as the returned object exists.
      pub unsafe fn new_unmapped(r: T) -> Self {
          Self {
              table: r,
              mapping: Unmapped,
          }
      }
  }

  impl<T> SGTable<T, BorrowedMapping>
  where
      T: Borrow<bindings::sg_table>,
  {
      // Safety: 'r' must borrow a sg_table that is DMA-mapped, and which
      // does not change as long as the returned object exists.
      pub unsafe fn new_mapped(r: T) -> Self {
          Self {
              table: r,
              mapping: BorrowedMapping,
          }
      }
  }

And this should be enough to cover the needs of GEM. Lyude mentioned building a
wrapper around a reference to a `sg_table`, which can be done as follows:

  // Obtain the reference from `gem_shmem_object` with the proper lifetime.
  let sg_table_ref: &bindings::sg_table = ...
  let sg_table = SGTable::new_mapped(sg_table_ref);

Here `SGTable` borrows a reference to `gem_shmem_object`, meaning it cannot
outlive it, and `gem_shmem_object` cannot be mutated for as long as we hold
that reference.

This also works to implement an equivalent of `OwnedSGTable`, if I understood
it correctly:

  struct WrappedAref(ARef<gem::Object>);
  impl Borrow<bindings::sg_table> for WrapperARef ...

  // `self` is a `&gem::Object`.
  let wrapped_aref: WrapperAref = self.into();
  let sg_table = SGTable::new_mapped(wrapped_aref);

Here the fact that we are passing an `ARef` lets the GEM object's lifetime be
managed at runtime, just like `OwnedSGTable` does.

Now on to the Nova use-case. Here we want to allocate, manage, and eventually
free both the `struct sg_table` and its DMA mapping.

For the former, we can define a new struct that implements `Borrow` and takes
care of freeing the `sg_table`:

  pub struct OwnedSgt<P: SGTablePages> {
        sgt: bindings::sg_table,
        pages: P,
  }

  impl<P> Drop for OwnedSgt<P>
    where
        P: SGTablePages,
  {
        fn drop(&mut self) {
          unsafe { bindings::sg_free_table(&mut self.sgt) };
      }
  }

  impl<P> Borrow<bindings::sg_table> for OwnedSgt<P>
    where
        P: SGTablePages,
  {
        fn borrow(&self) -> &bindings::sg_table {
          &self.sgt
      }
  }

This will be our first generic argument for `SGTable`. The mapping can then be
handled similarly:

  pub struct ManagedMapping {
      dev: ARef<Device>,
      dir: DmaDataDirection,
      // This works because the `sgl` member of `struct sg_table` never moves
      // after being allocated.
      sgl: *mut bindings::scatterlist,
      orig_nents: ffi::c_uint,
  }
  impl MappingState for ManagedMapping {}

  impl Drop for ManagedMapping {
      fn drop(&mut self) {
          unsafe {
              bindings::dma_unmap_sg_attrs(
                  self.dev.as_raw(),
                  self.sgl,
                  self.orig_nents as i32,
                  self.dir as i32,
                  0,
              )
          };
      }
  }

With this, and the `SGTablePages` trait, we can now provide a safe constructor
for mapping existing memory into a device:

  impl<P: SGTablePages> SGTable<OwnedSgt<P>, ManagedMapping> {
        pub fn new_owned(pages: P, flags: kernel::alloc::Flags) -> Result<Self> ...

The DMA mapping then remains in effect for as long as the returned object
lives.

You would then have `impl` blocks to provide the raw `sg_table` pointer as well
as DMA or CPU address iterators, made available or not depending on the mapping
typestate. And if the type borrowing the `sg_table` also implements
`BorrowMut`, we can even change the mapping state programmatically. I have
omitted it here for conciseness but have some code for this as well.

Although I remember a mention of the `Unmapped` state being unneeded in
discussion of the previous iteration - and indeed, so far both the GEM and Nova
use-cases do not really need it, so if that's confirmed we could remove the
`Unmapped` state and any kind of transition to simplify the interface.

Thoughts? If Abdiel is comfortable with this I can submit a v3 with this design
for review (putting myself as co-developer), on which Abdiel could then keep
iterating, as I suspect this would be easier to understand than this long email
:).

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Alexandre Courbot]

On Wed Jul 2, 2025 at 11:37 AM JST, Alexandre Courbot wrote:
> Thoughts? If Abdiel is comfortable with this I can submit a v3 with this design
> for review (putting myself as co-developer), on which Abdiel could then keep
> iterating, as I suspect this would be easier to understand than this long email
> :).

Figured I could just as well share the code with you and save both of us
some time. ^_^;

The top commit of this branch contains the proposal discussed:

https://github.com/Gnurou/linux/tree/scatterlists

The sample code has been updated to add dummy examples for the 3
use-cases discussed (reference to an existing `sg_table`, refcounted
reference, and owned data).

There are still things missing, including the typestate on `SGEntry`, as
it wasn't necessary to demonstrate the basic idea.

Note also that if we decide to only support DMA-mapped SG-entries, we
can remove a bunch of code, including the one that maps a `SGTable` if
the backing type implements `BorrowMut`.

For your consideration. :) Please feel free to take and use anything you
find useful.

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Abdiel Janulgue]

Hi,

On 03/07/2025 10:03, Alexandre Courbot wrote:
> On Wed Jul 2, 2025 at 11:37 AM JST, Alexandre Courbot wrote:
>> Thoughts? If Abdiel is comfortable with this I can submit a v3 with this design
>> for review (putting myself as co-developer), on which Abdiel could then keep
>> iterating, as I suspect this would be easier to understand than this long email
>> :).
> 
> Figured I could just as well share the code with you and save both of us
> some time. ^_^;
> 
> The top commit of this branch contains the proposal discussed:
> 
> https://github.com/Gnurou/linux/tree/scatterlists
> 
> The sample code has been updated to add dummy examples for the 3
> use-cases discussed (reference to an existing `sg_table`, refcounted
> reference, and owned data).
> 
> There are still things missing, including the typestate on `SGEntry`, as
> it wasn't necessary to demonstrate the basic idea.
> 
> Note also that if we decide to only support DMA-mapped SG-entries, we
> can remove a bunch of code, including the one that maps a `SGTable` if
> the backing type implements `BorrowMut`.
> 
> For your consideration. :) Please feel free to take and use anything you
> find useful.

Sorry for the delay, just came back from a week vacation.

Regarding your question regarding why I dropped type-state, I thought 
the general consensus here is to drop this approach:

https://lore.kernel.org/lkml/DAC20AXGABW2.X147X4JPMRBS@nvidia.com/

I think I might have misunderstood after re-reading it now. What you 
probably meant was to have no intermediate state from initialized to 
mapped to create the sg_table?

Anyways, thanks for the code above I'll look into this in detail and 
pick bits to integrate into v3 :)

/Abdiel

Re: [PATCH v2 1/2] rust: add initial scatterlist bindings

[Alexandre Courbot]

Hi Abdiel,

On Wed Jul 9, 2025 at 8:59 PM JST, Abdiel Janulgue wrote:
> Hi,
>
> On 03/07/2025 10:03, Alexandre Courbot wrote:
>> On Wed Jul 2, 2025 at 11:37 AM JST, Alexandre Courbot wrote:
>>> Thoughts? If Abdiel is comfortable with this I can submit a v3 with this design
>>> for review (putting myself as co-developer), on which Abdiel could then keep
>>> iterating, as I suspect this would be easier to understand than this long email
>>> :).
>> 
>> Figured I could just as well share the code with you and save both of us
>> some time. ^_^;
>> 
>> The top commit of this branch contains the proposal discussed:
>> 
>> https://github.com/Gnurou/linux/tree/scatterlists
>> 
>> The sample code has been updated to add dummy examples for the 3
>> use-cases discussed (reference to an existing `sg_table`, refcounted
>> reference, and owned data).
>> 
>> There are still things missing, including the typestate on `SGEntry`, as
>> it wasn't necessary to demonstrate the basic idea.
>> 
>> Note also that if we decide to only support DMA-mapped SG-entries, we
>> can remove a bunch of code, including the one that maps a `SGTable` if
>> the backing type implements `BorrowMut`.
>> 
>> For your consideration. :) Please feel free to take and use anything you
>> find useful.
>
> Sorry for the delay, just came back from a week vacation.

Welcome back!

>
> Regarding your question regarding why I dropped type-state, I thought 
> the general consensus here is to drop this approach:
>
> https://lore.kernel.org/lkml/DAC20AXGABW2.X147X4JPMRBS@nvidia.com/
>
> I think I might have misunderstood after re-reading it now. What you 
> probably meant was to have no intermediate state from initialized to 
> mapped to create the sg_table?

Sorry, that was poorly phrased - and yes this is what I meant. Since we
have no use for unmapped SG tables at the moment, let's start with this
simple and working approach. We can always add more states/stuff later
if the need arises.

>
> Anyways, thanks for the code above I'll look into this in detail and 
> pick bits to integrate into v3 :)

Looking forward to it! Please don't hesitate to reach out if anything is
unclear.