[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <E999B1A2-B1F6-4625-B784-08016672F4E0@collabora.com>
Date: Tue, 22 Jul 2025 21:44:39 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: acourbot@...dia.com,
dakr@...nel.org,
jgg@...pe.ca,
lyude@...hat.com,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <lossin@...nel.org>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>,
Tamir Duberstein <tamird@...il.com>,
FUJITA Tomonori <fujita.tomonori@...il.com>,
open list <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Randy Dunlap <rdunlap@...radead.org>,
Herbert Xu <herbert@...dor.apana.org.au>,
Caleb Sander Mateos <csander@...estorage.com>,
Petr Tesarik <petr@...arici.cz>,
Sui Jingfeng <sui.jingfeng@...ux.dev>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Robin Murphy <robin.murphy@....com>,
airlied@...hat.com,
"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust: add initial scatterlist abstraction
Hi Abdiel, Alex,
This overall looks good, a few minor comments below.
> On 18 Jul 2025, at 07:33, Abdiel Janulgue <abdiel.janulgue@...il.com> 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.
>
> Co-developed-by: Alexandre Courbot <acourbot@...dia.com>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.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 | 405 ++++++++++++++++++++++++++++++++
> 6 files changed, 456 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 0683fffdbde2..7b18bde78844 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 1f7bae643416..598fa50e878d 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
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +#[repr(i32)]
> +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 f61ac6f81f5d..48391a75bb62 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..0242884bf9fd
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Scatterlist
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +
> +use core::borrow::{Borrow, BorrowMut};
> +
> +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() }
> + }
Did you see this? [0]
> +
> + /// 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) };
> + }
> +}
> +
> +/// Trait implemented by all mapping states.
> +pub trait MappingState {}
> +
> +/// Trait implemented by all mapping states representing the fact that a `struct sg_table` is
> +/// mapped (and thus its DMA addresses are valid).
> +pub trait MappedState: MappingState {}
> +
> +/// Represents the fact that a `struct sg_table` is not DMA-mapped.
> +pub struct Unmapped;
> +impl MappingState for Unmapped {}
> +
> +/// Represents the fact that a `struct sg_table` is DMA-mapped by an external entity.
Perhaps it would be nice to define what an “external entity” means?
> +pub struct BorrowedMapping;
> +impl MappingState for BorrowedMapping {}
> +impl MappedState for BorrowedMapping {}
> +
> +/// A managed DMA mapping of a `struct sg_table` to a given device.
> +///
> +/// The mapping is cleared when this object is dropped.
> +///
> +/// # Invariants
> +///
> +/// - The `scatterlist` pointer is valid for the lifetime of a `ManagedMapping` instance.
> +/// - The `Device` instance is within a [`kernel::device::Bound`] context.
> +pub struct ManagedMapping {
> + dev: ARef<Device>,
> + dir: DmaDataDirection,
> + // This works because the `sgl` member of `struct sg_table` never moves, and the fact we can
> + // build this implies that we have an exclusive reference to the `sg_table`, thus it cannot be
> + // modified by anyone else.
> + sgl: *mut bindings::scatterlist,
> + orig_nents: ffi::c_uint,
> +}
> +
> +/// SAFETY: An `ManagedMapping` object is an immutable interface and should be safe to `Send` across threads.
> +unsafe impl Send for ManagedMapping {}
> +impl MappingState for ManagedMapping {}
> +impl MappedState for ManagedMapping {}
> +
> +impl Drop for ManagedMapping {
> + fn drop(&mut self) {
> + // SAFETY: Invariants on `Device<Bound>` and `Self` ensures that the `self.dev` and `self.sgl`
> + // are valid.
> + unsafe {
> + bindings::dma_unmap_sg_attrs(
> + self.dev.as_raw(),
> + self.sgl,
> + self.orig_nents as i32,
> + self.dir as i32,
> + 0,
> + )
> + };
> + }
> +}
> +
> +/// A scatter-gather table of DMA address spans.
> +///
> +/// This structure represents the Rust abstraction for a C `struct sg_table`. This implementation
> +/// abstracts the usage of an already existing C `struct sg_table` within Rust code that we get
> +/// passed from the C side.
> +pub struct SGTable<T: Borrow<bindings::sg_table>, M: MappingState> {
> + /// Mapping state of the underlying `struct sg_table`.
> + ///
> + /// This defines which methods of `SGTable` are available.
> + ///
> + /// Declared first so it is dropped before `table`, so we remove the mapping before freeing the
> + /// SG table if the latter is owned.
> + _mapping: M,
> +
> + /// Something that can borrow the underlying `struct sg_table`.
> + table: T,
> +}
> +
> +impl<T> SGTable<T, Unmapped>
> +where
> + T: Borrow<bindings::sg_table>,
> +{
> + /// Create a new unmapped `SGTable` from an already-existing `struct sg_table`.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the `struct sg_table` borrowed by `r` is initialized, valid for
> + /// the lifetime of the returned reference, and is not mapped.
> + pub unsafe fn new_unmapped(r: T) -> Self {
> + Self {
> + table: r,
> + _mapping: Unmapped,
> + }
> + }
> +}
> +
> +impl<T> SGTable<T, BorrowedMapping>
> +where
> + T: Borrow<bindings::sg_table>,
> +{
> + /// Create a new mapped `SGTable` from an already-existing `struct sg_table`.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the `struct sg_table` borrowed by `r` is initialized, valid for
> + /// the lifetime of the returned reference, and is DMA-mapped.
> + pub unsafe fn new_mapped(r: T) -> Self {
> + Self {
> + table: r,
> + _mapping: BorrowedMapping,
> + }
> + }
> +}
> +
> +impl<T, M> SGTable<T, M>
> +where
> + T: Borrow<bindings::sg_table>,
> + M: MappedState,
> +{
> + /// Returns an immutable iterator over the scatter-gather table.
> + pub fn iter(&self) -> SGTableIter<'_> {
> + SGTableIter {
> + // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
> + pos: Some(unsafe { SGEntry::as_ref(self.table.borrow().sgl) }),
> + }
> + }
> +}
> +
> +/// Provides a list of pages that can be used to build a `SGTable`.
> +pub trait SGTablePages {
I feel like this could be defined closer to where it is used.
> + /// 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`.
I feel like the above could be replaced by simply defining a struct with the
right field names.
> + fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)>;
> +
> + /// Returns the number of pages in the list.
> + fn entries(&self) -> usize;
> +}
> +
> +/// 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, T, M> IntoIterator for &'a SGTable<T, M>
> +where
> + T: Borrow<bindings::sg_table>,
> + M: MappedState,
> +{
> + type Item = &'a SGEntry;
> + type IntoIter = SGTableIter<'a>;
> +
> + fn into_iter(self) -> Self::IntoIter {
> + self.iter()
> + }
> +}
> +
> +impl<T> SGTable<T, Unmapped>
> +where
> + T: BorrowMut<bindings::sg_table>,
> +{
> + /// 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 `SGTable<_, ManagedMapping>` object.
> + pub fn dma_map(
> + mut self,
> + dev: &Device<Bound>,
> + dir: DmaDataDirection,
> + ) -> Result<SGTable<T, ManagedMapping>> {
> + // SAFETY: Invariants on `Device<Bound>` and `SGTable` ensures that the pointers are valid.
> + let ret = unsafe {
> + bindings::dma_map_sgtable(
> + dev.as_raw(),
> + self.table.borrow_mut(),
> + dir as i32,
> + bindings::DMA_ATTR_NO_WARN as usize,
> + )
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> +
> + let sgl = self.table.borrow_mut().sgl;
> + let orig_nents = self.table.borrow().orig_nents;
> +
> + Ok(SGTable {
> + table: self.table,
> + // INVARIANT:
> + // - `sgl` is valid by the type invariant of `OwnedSgt`.
How is `OwnedSgt` relevant here? I see self, which is SGTable, then table,
which is T: Borrow<bindings::sg_table>, and then borrow(), which is
bindings::sg_table.
> + // - `dev` is a reference to Device<Bound>.
> + _mapping: ManagedMapping {
> + dev: dev.into(),
> + dir,
> + sgl,
> + orig_nents,
> + },
> + })
> + }
> +}
> +
> +/// An owned `struct sg_table`, which lifetime is tied to this object.
s/which/whose ? Although I am not a native English speaker.
> +///
> +/// # Invariants
> +///
> +/// The `sg_table` is valid and initialized for the lifetime of an `OwnedSgt` instance.
> +pub struct OwnedSgt<P: SGTablePages> {
> + sgt: bindings::sg_table,
> + /// Used to keep the memory pointed to by `sgt` alive.
> + _pages: P,
> +}
> +
> +/// SAFETY: An `OwnedSgt` object is constructed internally by `SGTable` and no interface is exposed to
> +/// the user to modify its state after construction, except [`SGTable::dma_map`] which transfers
> +/// ownership of the object, hence should be safe to `Send` across threads.
> +unsafe impl<P: SGTablePages> Send for OwnedSgt<P> {}
> +
> +impl<P> Drop for OwnedSgt<P>
> +where
> + P: SGTablePages,
> +{
> + fn drop(&mut self) {
> + // SAFETY: Invariant on `OwnedSgt` ensures that the sg_table is valid.
> + 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
> + }
> +}
> +
> +// To allow mapping the state!
Can you expand this comment a bit?
> +impl<P> BorrowMut<bindings::sg_table> for OwnedSgt<P>
> +where
> + P: SGTablePages,
> +{
> + fn borrow_mut(&mut self) -> &mut bindings::sg_table {
> + &mut self.sgt
> + }
> +}
> +
> +impl<P: SGTablePages> SGTable<OwnedSgt<P>, Unmapped> {
Oh, I now see that you can have SGTable<OwnedSgt<..>, …>. In any case, when
I said in my previous comment that "I don't see how OwnedSGTable is relevant
here”, T was unconstrained.
I am mentioning this on the off-chance that it's a mistaken assumption instead
of a typo.
> + /// Allocate and build a new `SGTable` 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>);
A blank line would be welcome here, IMHO.
> + /// impl SGTablePages for PagesArray {
> + /// fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
See, it's hard to figure out what is going on here. I had to go back up to the
comment that explains what each field means in the tuple. This could be fixed
if this thing was its own struct with named fields.
> + /// self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))
By the way, ironically, the order seems to be inverted here :)
> + /// }
> + ///
> + /// 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::new_owned(PagesArray(pages), GFP_KERNEL)?;
> + /// # Ok::<(), Error>(())
> + /// ```
> + pub fn new_owned(pages: P, flags: kernel::alloc::Flags) -> Result<Self> {
> + // SAFETY: `sgt` is not a reference.
> + let mut sgt: bindings::sg_table = unsafe { core::mem::zeroed() };
> +
> + // SAFETY: The sgt pointer is from the Opaque-wrapped `sg_table` object hence is valid.
IMHO the sentence above does not read very well.
> + let ret =
> + unsafe { bindings::sg_alloc_table(&mut sgt, pages.entries() as u32, flags.as_raw()) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> + // SAFETY: We just successfully allocated `sgt`, hence the pointer is valid and have sole access to
> + // it at this point.
> + let sgentries = unsafe { core::slice::from_raw_parts_mut(sgt.sgl, pages.entries()) };
> + for (entry, page) in sgentries
> + .iter_mut()
> + .map(|e|
> + // SAFETY: `SGEntry::as_mut` is called on the pointer only once, which is valid and non-NULL
> + // while inside the closure.
> + unsafe { SGEntry::as_mut(e) })
> + .zip(pages.iter())
> + {
> + entry.set_page(page.0, page.1 as u32, page.2 as u32)
> + }
> +
> + Ok(Self {
> + // INVARIANT: We just successfully allocated and built the table from the page entries.
> + table: OwnedSgt { sgt, _pages: pages },
> + _mapping: Unmapped,
> + })
> + }
> +}
> --
> 2.43.0
>
>
If you give me a couple of days I can test this on Tyr and see if the MCU still boots :)
— Daniel
[0] https://lore.kernel.org/rust-for-linux/20250711-device-as-ref-v2-0-1b16ab6402d7@google.com/
Powered by blists - more mailing lists