Message-Id: <D9VQQAY6G20X.RVU8H169KQL2@nvidia.com>
Date: Wed, 14 May 2025 17:29:04 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Abdiel Janulgue" <abdiel.janulgue@...il.com>, <dakr@...nel.org>,
 <lyude@...hat.com>
Cc: "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" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Valentin Obst"
 <kernel@...entinobst.de>, "open list" <linux-kernel@...r.kernel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>, "Robin Murphy"
 <robin.murphy@....com>, <airlied@...hat.com>,
 <rust-for-linux@...r.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>, "Petr Tesarik" <petr@...arici.cz>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Herbert Xu" <herbert@...dor.apana.org.au>,
 "Sui Jingfeng" <sui.jingfeng@...ux.dev>, "Randy Dunlap"
 <rdunlap@...radead.org>, "Michael Kelley" <mhklinux@...look.com>
Subject: Re: [RFC PATCH 1/2] rust: add initial scatterlist bindings

Hi Adbiel,

On Mon May 12, 2025 at 6:53 PM 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@...il.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/scatterlist.c      |  25 +++
>  rust/kernel/lib.rs              |   1 +
>  rust/kernel/scatterlist.rs      | 275 ++++++++++++++++++++++++++++++++
>  5 files changed, 303 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 ab37e1d35c70..a7d3b97cd4e0 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -14,6 +14,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 1e7c84df7252..f45a15f88e25 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -28,6 +28,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..5ab0826f7c0b
> --- /dev/null
> +++ b/rust/helpers/scatterlist.c
> @@ -0,0 +1,25 @@
> +// 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_address(sg);

I believe you mean sg_dma_len()?

> +}
> 
> +
> +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 fa852886d4d1..a8d5fcb55388 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -87,6 +87,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..122a6f94bf56
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Scatterlist
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{Error, Result},
> +    page::Page,
> +    types::{ARef, Opaque},
> +};
> +use core::ops::{Deref, DerefMut};
> +
> +/// A single scatter-gather entry, representing a span of pages in the device's DMA address space.
> +///
> +/// # 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`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the lifetime
> +    /// of the returned reference.
> +    pub 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`.
> +    ///
> +    /// # 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.
> +    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() }
> +    }
> +
> +    /// 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()) }
> +    }
> +
> +    /// Set this entry to point at a given page.
> +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> +        let c: *mut bindings::scatterlist = self.0.get();

You don't need to specify the type here, and no reason to use a
temporary variable, so might as well roll `self.0.get()` into the next
line and make this a one-liner like the other methods.

> +        // 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) };
> +    }
> +
> +    /// Obtain the raw `struct scatterlist *`.
> +    pub fn as_raw(&self) -> *mut bindings::scatterlist {
> +        self.0.get()
> +    }
> +}
> +
> +/// 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(u32)]

The bindings were `i32` on my system and this caused a build error...

> +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,
> +}
> +
> +/// The base interface for 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.
> +///
> +/// # 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`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is initialized and valid for
> +    /// the lifetime of the returned reference.
> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct sg_table *`.
> +    pub fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.0.get()
> +    }
> +
> +    /// Returns a mutable iterator over the scather-gather table.
> +    pub fn iter_mut(&mut self) -> SGTableIterMut<'_> {
> +        SGTableIterMut {
> +            // SAFETY: dereferenced pointer is valid due to the type invariants on `SGTable`.
> +            pos: Some(unsafe { SGEntry::as_mut((*self.0.get()).sgl) }),
> +        }
> +    }
> +
> +    /// Returns an iterator over the scather-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.0.get()).sgl) }),
> +        }
> +    }

I think Jason mentioned this already, but you should really have two
iterators, one for the CPU side and one for the device side. The two
lists are not even guaranteed to be the same size IIUC, so having both
lists in the same iterator is a receipe for confusion and bugs.

I have an (absolutely awful) implementation of that if you want to take
a look:

https://github.com/Gnurou/linux/blob/nova-gsp/drivers/gpu/nova-core/firmware/radix3.rs#L200

It's probably wrong in many places, and I just wrote it as a temporary
alternative until this series lands, but please steal any idea that you
think is reusable.

There is also the fact that SG tables are not always necessarily mapped
on the device side, so we would have to handle that as well, e.g.
through a typestate or maybe by just returning a dedicated error in that
case.

Powered by blists - more mailing lists