[PATCH v14 00/11] rust: add dma coherent allocator abstraction

[PATCH v14 00/11] rust: add dma coherent allocator abstraction

[Abdiel Janulgue]

This series adds the rust bindings for the dma coherent allocator which
is needed for nova-core[0].

For v14, mark for TODO the solution for revoking the device resources of a
`CoherentAllocation`, but not the entire `CoherentAllocation` including
the allocated memory itself.

This is tested on a Nvidia GA104 GPU device using PoC code which parses
and loads the GSP firmware via DMA.

Changes since v13:
- Remove Devres wrapper, document correct solution for proper resource
  deallocation as TODO, add patches from Danilo to properly add DMA
  addressing capabilities for the device which requires a new trait
  that defines the DMA specific methods of devices. (Danilo)
- Link to v13: https://lore.kernel.org/lkml/20250307110821.1703422-1-abdiel.janulgue@gmail.com/

Changes since v12:
- Move out the contentious functions: as_slice, as_slice_mut, and write
  for later discussion as a separate patch. Make write unsafe as well.
  Remove skip_drop, use volatile r/w for field_read and field_write
  (Alice Ryhl, et al.).
- Documentation improvements, use markdown for intra-doc links
  (Miguel Ojeda).
- Move dma addressing capabilities to a separate patch within device.rs
  (Andreas Hindborg).
- Add a simple driver to excercise the basics of the api (Danilo).

Changes since v11:
- Ensure robust handling for potential pointer arithmetic overflows
  in bounds checking (Petr Tesařík).
- Add dma mask helpers (Daniel Almeida).
- Clarification in the safety aspects of the as_slice/as_slice_mut API,
  Use ManuallyDrop trait as a replacement for into_parts(),
  Add dma_read!/dma_write! helper macros (Alice Ryhl).

Changes since v10:
- rename read() to as_slice() (Boqun Feng)
- Do a bitwise copy of ARef<Device> in into_parts() when returning the
  device reference (Alice Ryhl).

Changes since v9:
- Use ARef<Device> in the constructor arguments, docs clarification avoid
  manually dropping the refcount for the device in into_parts(), use
  add() instead of wrapping_add() in the pointer arithmetic for performance
  (Alice Ryhl).

Changes since v8:
- Add MAINTAINERS entry
- Fix build issues due to switch from core::ffi to crate:ffi in bindgen.
- Ensure the wrapped attribute is non-pub in struct Attrs, declare it 
  #[repr(transparent)] as well (Daniel Sedlak)

Changes since v7:
- Remove cpu_buf() and cpu_buf_mut() as exporting a r/w interface via
  a slice is undefined behaviour due to slice's requirement that the
  underlying pointer should not be modified (Alice Ryhl, Robin Murphy).
- Reintroduce r/w helpers instead which includes proper safety
  invariants (Daniel Almeida).

Changes since v6:
- Include the dma_attrs in the constructor, use alloc::Flags as inpiration

Changes since v5:
- Remove unnecessary lifetime annotation when returning the CPU buffer.

Changes since v4:
- Documentation and example fixes, use Markdown formatting (Miguel Ojeda).
- Discard read()/write() helpers to remove bound on Copy and fix overhead
  (Daniel Almeida).
- Improve error-handling in the constructor block (Andreas Hindborg).

Changes since v3:
- Reject ZST types by checking the type size in the constructor in
  addition to requiring FromBytes/AsBytes traits for the type (Alice Ryhl).

Changes since v2:
- Fixed missing header for generating the bindings.

Changes since v1:
- Fix missing info in commit log where EOVERFLOW is used.
- Restrict the dma coherent allocator to numeric types for now for valid
  behaviour (Daniel Almeida).
- Build slice dynamically.

[0] https://lore.kernel.org/lkml/20250131220432.17717-1-dakr@kernel.org/
[1] https://lore.kernel.org/lkml/20250305174118.GA351188@nvidia.com/

Abdiel Janulgue (5):
  rust: error: Add EOVERFLOW
  rust: add dma coherent allocator abstraction.
  samples: rust: add Rust dma test sample driver
  MAINTAINERS: add entry for Rust dma mapping helpers device driver API
  rust: dma: add as_slice/write functions for CoherentAllocation

Danilo Krummrich (6):
  rust: dma: implement `dma::Device` trait
  rust: dma: add dma addressing capabilities
  rust: pci: implement the `dma::Device` trait
  rust: platform: implement the `dma::Device` trait
  rust: dma: use `dma::Device` in `CoherentAllocation`
  rust: samples: dma: set DMA mask

 MAINTAINERS                     |  13 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/dma.c              |   8 +
 rust/helpers/helpers.c          |   1 +
 rust/kernel/dma.rs              | 503 ++++++++++++++++++++++++++++++++
 rust/kernel/error.rs            |   1 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/pci.rs              |   2 +
 rust/kernel/platform.rs         |   2 +
 samples/rust/Kconfig            |  11 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_dma.rs        | 104 +++++++
 12 files changed, 648 insertions(+)
 create mode 100644 rust/helpers/dma.c
 create mode 100644 rust/kernel/dma.rs
 create mode 100644 samples/rust/rust_dma.rs


base-commit: ab2ebb7bc9d9af2f50b0ad54deb65e1d0b01bc70
-- 
2.43.0

[PATCH v14 01/11] rust: error: Add EOVERFLOW

[Abdiel Janulgue]

Trivial addition for missing EOVERFLOW error. This is used by a
subsequent patch that might require returning EOVERFLOW as a result
of `checked_mul`.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/error.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index f6ecf09cb65f..1e510181432c 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -64,6 +64,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(EOVERFLOW, "Value too large for defined data type.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
-- 
2.43.0

[PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Abdiel Janulgue]

Add a simple dma coherent allocator rust abstraction. Based on
Andreas Hindborg's dma abstractions from the rnvme driver, which
was also based on earlier work by Wedson Almeida Filho.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/dma.rs              | 369 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 3 files changed, 371 insertions(+)
 create mode 100644 rust/kernel/dma.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ae39fc18a8bf..ccb988340df6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -12,6 +12,7 @@
 #include <linux/blkdev.h>
 #include <linux/cred.h>
 #include <linux/device/faux.h>
+#include <linux/dma-mapping.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/file.h>
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
new file mode 100644
index 000000000000..325b00fe7871
--- /dev/null
+++ b/rust/kernel/dma.rs
@@ -0,0 +1,369 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Direct memory access (DMA).
+//!
+//! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
+
+use crate::{
+    bindings, build_assert,
+    device::Device,
+    error::code::*,
+    error::Result,
+    transmute::{AsBytes, FromBytes},
+    types::ARef,
+};
+
+/// Possible attributes associated with a DMA mapping.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`attrs`] module.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::device::Device;
+/// use kernel::dma::{attrs::*, CoherentAllocation};
+///
+/// # fn test(dev: &Device) -> Result {
+/// let attribs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_WARN;
+/// let c: CoherentAllocation<u64> =
+///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, attribs)?;
+/// # Ok::<(), Error>(()) }
+/// ```
+#[derive(Clone, Copy, PartialEq)]
+#[repr(transparent)]
+pub struct Attrs(u32);
+
+impl Attrs {
+    /// Get the raw representation of this attribute.
+    pub(crate) fn as_raw(self) -> crate::ffi::c_ulong {
+        self.0 as _
+    }
+
+    /// Check whether `flags` is contained in `self`.
+    pub fn contains(self, flags: Attrs) -> bool {
+        (self & flags) == flags
+    }
+}
+
+impl core::ops::BitOr for Attrs {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl core::ops::BitAnd for Attrs {
+    type Output = Self;
+    fn bitand(self, rhs: Self) -> Self::Output {
+        Self(self.0 & rhs.0)
+    }
+}
+
+impl core::ops::Not for Attrs {
+    type Output = Self;
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}
+
+/// DMA mapping attributes.
+pub mod attrs {
+    use super::Attrs;
+
+    /// Specifies that reads and writes to the mapping may be weakly ordered, that is that reads
+    /// and writes may pass each other.
+    pub const DMA_ATTR_WEAK_ORDERING: Attrs = Attrs(bindings::DMA_ATTR_WEAK_ORDERING);
+
+    /// Specifies that writes to the mapping may be buffered to improve performance.
+    pub const DMA_ATTR_WRITE_COMBINE: Attrs = Attrs(bindings::DMA_ATTR_WRITE_COMBINE);
+
+    /// Lets the platform to avoid creating a kernel virtual mapping for the allocated buffer.
+    pub const DMA_ATTR_NO_KERNEL_MAPPING: Attrs = Attrs(bindings::DMA_ATTR_NO_KERNEL_MAPPING);
+
+    /// Allows platform code to skip synchronization of the CPU cache for the given buffer assuming
+    /// that it has been already transferred to 'device' domain.
+    pub const DMA_ATTR_SKIP_CPU_SYNC: Attrs = Attrs(bindings::DMA_ATTR_SKIP_CPU_SYNC);
+
+    /// Forces contiguous allocation of the buffer in physical memory.
+    pub const DMA_ATTR_FORCE_CONTIGUOUS: Attrs = Attrs(bindings::DMA_ATTR_FORCE_CONTIGUOUS);
+
+    /// This is a hint to the DMA-mapping subsystem that it's probably not worth the time to try
+    /// to allocate memory to in a way that gives better TLB efficiency.
+    pub const DMA_ATTR_ALLOC_SINGLE_PAGES: Attrs = Attrs(bindings::DMA_ATTR_ALLOC_SINGLE_PAGES);
+
+    /// This tells the DMA-mapping subsystem to suppress allocation failure reports (similarly to
+    /// __GFP_NOWARN).
+    pub const DMA_ATTR_NO_WARN: Attrs = Attrs(bindings::DMA_ATTR_NO_WARN);
+
+    /// Used to indicate that the buffer is fully accessible at an elevated privilege level (and
+    /// ideally inaccessible or at least read-only at lesser-privileged levels).
+    pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
+}
+
+/// 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
+/// large consistent DMA regions.
+///
+/// A [`CoherentAllocation`] instance contains a pointer to the allocated region (in the
+/// processor's virtual address space) and the device address which can be given to the device
+/// as the DMA address base of the region. The region is released once [`CoherentAllocation`]
+/// is dropped.
+///
+/// # Invariants
+///
+/// For the lifetime of an instance of [`CoherentAllocation`], the `cpu_addr` is a valid pointer
+/// to an allocated region of consistent memory and `dma_handle` is the DMA address base of
+/// the region.
+// TODO
+//
+// DMA allocations potentially carry device resources (e.g.IOMMU mappings), hence for soundness
+// reasons DMA allocation would need to be embedded in a `Devres` container, in order to ensure
+// that device resources can never survive device unbind.
+//
+// However, it is neither desirable nor necessary to protect the allocated memory of the DMA
+// allocation from surviving device unbind; it would require RCU read side critical sections to
+// access the memory, which may require subsequent unnecessary copies.
+//
+// Hence, find a way to revoke the device resources of a `CoherentAllocation`, but not the
+// entire `CoherentAllocation` including the allocated memory itself.
+pub struct CoherentAllocation<T: AsBytes + FromBytes> {
+    dev: ARef<Device>,
+    dma_handle: bindings::dma_addr_t,
+    count: usize,
+    cpu_addr: *mut T,
+    dma_attrs: Attrs,
+}
+
+impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
+    /// Allocates a region of `size_of::<T> * count` of consistent memory.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::device::Device;
+    /// use kernel::dma::{attrs::*, CoherentAllocation};
+    ///
+    /// # fn test(dev: &Device) -> Result {
+    /// let c: CoherentAllocation<u64> =
+    ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
+    /// # Ok::<(), Error>(()) }
+    /// ```
+    pub fn alloc_attrs(
+        dev: &Device,
+        count: usize,
+        gfp_flags: kernel::alloc::Flags,
+        dma_attrs: Attrs,
+    ) -> Result<CoherentAllocation<T>> {
+        build_assert!(
+            core::mem::size_of::<T>() > 0,
+            "It doesn't make sense for the allocated type to be a ZST"
+        );
+
+        let size = count
+            .checked_mul(core::mem::size_of::<T>())
+            .ok_or(EOVERFLOW)?;
+        let mut dma_handle = 0;
+        // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
+        let ret = unsafe {
+            bindings::dma_alloc_attrs(
+                dev.as_raw(),
+                size,
+                &mut dma_handle,
+                gfp_flags.as_raw(),
+                dma_attrs.as_raw(),
+            )
+        };
+        if ret.is_null() {
+            return Err(ENOMEM);
+        }
+        // INVARIANT: We just successfully allocated a coherent region which is accessible for
+        // `count` elements, hence the cpu address is valid. We also hold a refcounted reference
+        // to the device.
+        Ok(Self {
+            dev: dev.into(),
+            dma_handle,
+            count,
+            cpu_addr: ret as *mut T,
+            dma_attrs,
+        })
+    }
+
+    /// Performs the same functionality as [`CoherentAllocation::alloc_attrs`], except the
+    /// `dma_attrs` is 0 by default.
+    pub fn alloc_coherent(
+        dev: &Device,
+        count: usize,
+        gfp_flags: kernel::alloc::Flags,
+    ) -> Result<CoherentAllocation<T>> {
+        CoherentAllocation::alloc_attrs(dev, count, gfp_flags, Attrs(0))
+    }
+
+    /// Returns the base address to the allocated region in the CPU's virtual address space.
+    pub fn start_ptr(&self) -> *const T {
+        self.cpu_addr
+    }
+
+    /// Returns the base address to the allocated region in the CPU's virtual address space as
+    /// a mutable pointer.
+    pub fn start_ptr_mut(&mut self) -> *mut T {
+        self.cpu_addr
+    }
+
+    /// Returns a DMA handle which may given to the device as the DMA address base of
+    /// the region.
+    pub fn dma_handle(&self) -> bindings::dma_addr_t {
+        self.dma_handle
+    }
+
+    /// Returns a pointer to an element from the region with bounds checking. `offset` is in
+    /// units of `T`, not the number of bytes.
+    ///
+    /// Public but hidden since it should only be used from [`dma_read`] and [`dma_write`] macros.
+    #[doc(hidden)]
+    pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
+        if offset >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`
+        // and we've just checked that the range and index is within bounds.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        // that `self.count` won't overflow early in the constructor.
+        Ok(unsafe { self.cpu_addr.add(offset) })
+    }
+
+    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
+    ///
+    /// # Safety
+    ///
+    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
+    /// validated beforehand.
+    ///
+    /// Public but hidden since it should only be used from [`dma_read`] macro.
+    #[doc(hidden)]
+    pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
+        // SAFETY: By the safety requirements field is valid.
+        unsafe { field.read_volatile() }
+    }
+
+    /// Writes a value to `field` and ensures that its type is [`AsBytes`].
+    ///
+    /// # Safety
+    ///
+    /// This must be called from the [`dma_write`] macro which ensures that the `field` pointer is
+    /// validated beforehand.
+    ///
+    /// Public but hidden since it should only be used from [`dma_write`] macro.
+    #[doc(hidden)]
+    pub unsafe fn field_write<F: AsBytes>(&self, field: *mut F, val: F) {
+        // SAFETY: By the safety requirements field is valid.
+        unsafe { field.write_volatile(val) }
+    }
+}
+
+/// Note that the device configured to do DMA must be halted before this object is dropped.
+impl<T: AsBytes + FromBytes> Drop for CoherentAllocation<T> {
+    fn drop(&mut self) {
+        let size = self.count * core::mem::size_of::<T>();
+        // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
+        // The cpu address, and the dma handle are valid due to the type invariants on
+        // `CoherentAllocation`.
+        unsafe {
+            bindings::dma_free_attrs(
+                self.dev.as_raw(),
+                size,
+                self.cpu_addr as _,
+                self.dma_handle,
+                self.dma_attrs.as_raw(),
+            )
+        }
+    }
+}
+
+/// Reads a field of an item from an allocated region of structs.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::device::Device;
+/// use kernel::dma::{attrs::*, CoherentAllocation};
+///
+/// struct MyStruct { field: u32, }
+///
+/// // SAFETY: All bit patterns are acceptable values for `MyStruct`.
+/// unsafe impl kernel::transmute::FromBytes for MyStruct{};
+/// // SAFETY: Instances of `MyStruct` have no uninitialized portions.
+/// unsafe impl kernel::transmute::AsBytes for MyStruct{};
+///
+/// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
+/// let whole = kernel::dma_read!(alloc[2]);
+/// let field = kernel::dma_read!(alloc[1].field);
+/// # Ok::<(), Error>(()) }
+/// ```
+#[macro_export]
+macro_rules! dma_read {
+    ($dma:expr, $idx: expr, $($field:tt)*) => {{
+        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+        // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+        // dereferenced. The compiler also further validates the expression on whether `field`
+        // is a member of `item` when expanded by the macro.
+        unsafe {
+            let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
+            $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
+        }
+    }};
+    ($dma:ident [ $idx:expr ] $($field:tt)* ) => {
+        $crate::dma_read!($dma, $idx, $($field)*);
+    };
+    ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
+        $crate::dma_read!($($dma).*, $idx, $($field)*);
+    };
+}
+
+/// Writes to a field of an item from an allocated region of structs.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::device::Device;
+/// use kernel::dma::{attrs::*, CoherentAllocation};
+///
+/// struct MyStruct { member: u32, }
+///
+/// // SAFETY: All bit patterns are acceptable values for `MyStruct`.
+/// unsafe impl kernel::transmute::FromBytes for MyStruct{};
+/// // SAFETY: Instances of `MyStruct` have no uninitialized portions.
+/// unsafe impl kernel::transmute::AsBytes for MyStruct{};
+///
+/// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
+/// kernel::dma_write!(alloc[2].member = 0xf);
+/// kernel::dma_write!(alloc[1] = MyStruct { member: 0xf });
+/// # Ok::<(), Error>(()) }
+/// ```
+#[macro_export]
+macro_rules! dma_write {
+    ($dma:ident [ $idx:expr ] $($field:tt)*) => {{
+        $crate::dma_write!($dma, $idx, $($field)*);
+    }};
+    ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
+        $crate::dma_write!($($dma).*, $idx, $($field)*);
+    }};
+    ($dma:expr, $idx: expr, = $val:expr) => {
+        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+        // SAFETY: `item_from_index` ensures that `item` is always a valid item.
+        unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+    };
+    ($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
+        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+        // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+        // dereferenced. The compiler also further validates the expression on whether `field`
+        // is a member of `item` when expanded by the macro.
+        unsafe {
+            let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
+            $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
+        }
+    };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 398242f92a96..8e76ef9b4346 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -44,6 +44,7 @@
 pub mod device;
 pub mod device_id;
 pub mod devres;
+pub mod dma;
 pub mod driver;
 pub mod error;
 pub mod faux;
-- 
2.43.0

[PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Abdiel Janulgue]

Add a simple driver to excercise the basics of the Rust DMA
coherent allocator bindings.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 samples/rust/Kconfig     | 11 +++++
 samples/rust/Makefile    |  1 +
 samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 samples/rust/rust_dma.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 3b6eae84b297..e2d14aa6beec 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_DMA
+	tristate "DMA Test Driver"
+	depends on PCI
+	help
+	  This option builds the Rust dma test driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called dma.
+
+	  If unsure, say N.
+
 endif # SAMPLES_RUST
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 0dbc6d90f1ef..1a9aff6e8d6a 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA)		+= rust_dma.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
new file mode 100644
index 000000000000..1740140faba6
--- /dev/null
+++ b/samples/rust/rust_dma.rs
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust DMA api test (based on QEMU's `pci-testdev`).
+//!
+//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
+
+use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
+
+struct DmaSampleDriver {
+    pdev: pci::Device,
+    ca: CoherentAllocation<MyStruct>,
+}
+
+const TEST_VALUES: [(u32, u32); 5] = [
+    (0xa, 0xb),
+    (0xc, 0xd),
+    (0xe, 0xf),
+    (0xab, 0xba),
+    (0xcd, 0xef),
+];
+
+struct MyStruct {
+    h: u32,
+    b: u32,
+}
+
+impl MyStruct {
+    fn new(h: u32, b: u32) -> Self {
+        Self { h, b }
+    }
+}
+// SAFETY: All bit patterns are acceptable values for `MyStruct`.
+unsafe impl kernel::transmute::AsBytes for MyStruct {}
+// SAFETY: Instances of `MyStruct` have no uninitialized portions.
+unsafe impl kernel::transmute::FromBytes for MyStruct {}
+
+kernel::pci_device_table!(
+    PCI_TABLE,
+    MODULE_PCI_TABLE,
+    <DmaSampleDriver as pci::Driver>::IdInfo,
+    [(
+        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
+        ()
+    )]
+);
+
+impl pci::Driver for DmaSampleDriver {
+    type IdInfo = ();
+    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+
+    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+        dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
+
+        let ca: CoherentAllocation<MyStruct> =
+            CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
+
+        || -> Result {
+            for (i, value) in TEST_VALUES.into_iter().enumerate() {
+                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
+            }
+
+            Ok(())
+        }()?;
+
+        let drvdata = KBox::new(
+            Self {
+                pdev: pdev.clone(),
+                ca,
+            },
+            GFP_KERNEL,
+        )?;
+
+        Ok(drvdata.into())
+    }
+}
+
+impl Drop for DmaSampleDriver {
+    fn drop(&mut self) {
+        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
+
+        let _ = || -> Result {
+            for (i, value) in TEST_VALUES.into_iter().enumerate() {
+                assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
+                assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
+            }
+            Ok(())
+        }();
+    }
+}
+
+kernel::module_pci_driver! {
+    type: DmaSampleDriver,
+    name: "rust_dma",
+    author: "Abdiel Janulgue",
+    description: "Rust DMA test",
+    license: "GPL v2",
+}
-- 
2.43.0

[PATCH v14 04/11] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Abdiel Janulgue]

Add an entry for the Rust dma mapping helpers abstractions.

Nacked-by: Christoph Hellwig <hch@lst.de>
Acked-by: Danilo Krummrich <dakr@kernel.org>
Acked-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 MAINTAINERS | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 007ca67cc830..aa49c551fbec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6902,6 +6902,19 @@ F:	include/linux/dma-mapping.h
 F:	include/linux/swiotlb.h
 F:	kernel/dma/
 
+DMA MAPPING HELPERS DEVICE DRIVER API [RUST]
+M:	Abdiel Janulgue <abdiel.janulgue@gmail.com>
+M:	Danilo Krummrich <dakr@kernel.org>
+R:	Daniel Almeida <daniel.almeida@collabora.com>
+R:	Robin Murphy <robin.murphy@arm.com>
+R:	Andreas Hindborg <a.hindborg@kernel.org>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+W:	https://rust-for-linux.com
+T:	git https://github.com/Rust-for-Linux/linux.git alloc-next
+F:	rust/kernel/dma.rs
+F:	samples/rust/rust_dma.rs
+
 DMA-BUF HEAPS FRAMEWORK
 M:	Sumit Semwal <sumit.semwal@linaro.org>
 R:	Benjamin Gaignard <benjamin.gaignard@collabora.com>
-- 
2.43.0

[PATCH v14 05/11] rust: dma: implement `dma::Device` trait

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

Add a trait that defines the DMA specific methods of devices.

The `dma::Device` trait should be implemented by (bus) device
representations, where the underlying bus potentially supports DMA, such
as `pci::Device` or `platform::Device`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 325b00fe7871..7ff797a7ad18 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -6,13 +6,20 @@
 
 use crate::{
     bindings, build_assert,
-    device::Device,
+    device,
     error::code::*,
     error::Result,
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };
 
+/// Trait to be implemented by bus specific devices.
+///
+/// The [`Device`] trait should be implemented by bus specific device representations, where the
+/// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
+/// [crate::platform::Device].
+pub trait Device: AsRef<device::Device> {}
+
 /// Possible attributes associated with a DMA mapping.
 ///
 /// They can be combined with the operators `|`, `&`, and `!`.
@@ -130,7 +137,7 @@ pub mod attrs {
 // Hence, find a way to revoke the device resources of a `CoherentAllocation`, but not the
 // entire `CoherentAllocation` including the allocated memory itself.
 pub struct CoherentAllocation<T: AsBytes + FromBytes> {
-    dev: ARef<Device>,
+    dev: ARef<device::Device>,
     dma_handle: bindings::dma_addr_t,
     count: usize,
     cpu_addr: *mut T,
@@ -152,7 +159,7 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
     /// # Ok::<(), Error>(()) }
     /// ```
     pub fn alloc_attrs(
-        dev: &Device,
+        dev: &device::Device,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
         dma_attrs: Attrs,
@@ -194,7 +201,7 @@ pub fn alloc_attrs(
     /// Performs the same functionality as [`CoherentAllocation::alloc_attrs`], except the
     /// `dma_attrs` is 0 by default.
     pub fn alloc_coherent(
-        dev: &Device,
+        dev: &device::Device,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
     ) -> Result<CoherentAllocation<T>> {
-- 
2.43.0

[PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

Implement `dma_set_mask()` and `dma_set_mask_and_coherent()` in the
`dma::Device` trait.

Those methods are used to inform the kernel about the device's DMA
addressing capabilities.

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/dma.c     |  8 ++++++++
 rust/helpers/helpers.c |  1 +
 rust/kernel/dma.rs     | 46 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 3 deletions(-)
 create mode 100644 rust/helpers/dma.c

diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
new file mode 100644
index 000000000000..8eb482386f93
--- /dev/null
+++ b/rust/helpers/dma.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-mapping.h>
+
+int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
+{
+	return dma_set_mask_and_coherent(dev, mask);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..8f3808c8b7fe 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -13,6 +13,7 @@
 #include "build_bug.c"
 #include "cred.c"
 #include "device.c"
+#include "dma.c"
 #include "err.c"
 #include "fs.c"
 #include "io.c"
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 7ff797a7ad18..ac3ec0042327 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -5,10 +5,10 @@
 //! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
 
 use crate::{
-    bindings, build_assert,
-    device,
+    bindings, build_assert, device,
     error::code::*,
     error::Result,
+    prelude::*,
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };
@@ -18,7 +18,35 @@
 /// The [`Device`] trait should be implemented by bus specific device representations, where the
 /// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
 /// [crate::platform::Device].
-pub trait Device: AsRef<device::Device> {}
+pub trait Device: AsRef<device::Device> {
+    /// Inform the kernel about the device's DMA addressing capabilities.
+    ///
+    /// Set both the DMA mask and the coherent DMA mask to the same value.
+    ///
+    /// Note that we don't check the return value from the C `dma_set_coherent_mask` as the DMA API
+    /// guarantees that the coherent DMA mask can be set to the same or smaller than the streaming
+    /// DMA mask.
+    fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
+        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Same as [`Self::dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
+    fn dma_set_mask(&mut self, mask: u64) -> Result {
+        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        let ret = unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+}
 
 /// Possible attributes associated with a DMA mapping.
 ///
@@ -374,3 +402,15 @@ macro_rules! dma_write {
         }
     };
 }
+
+/// Helper function to set the bit mask for DMA addressing.
+pub const fn dma_bit_mask(n: usize) -> u64 {
+    if n > 64 {
+        return 0;
+    }
+    if n == 64 {
+        !0
+    } else {
+        (1 << (n)) - 1
+    }
+}
-- 
2.43.0

[PATCH v14 07/11] rust: pci: implement the `dma::Device` trait

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

The PCI bus is potentially capable of performing DMA, hence implement
the `dma:Device` trait for `pci::Device`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/pci.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index f7b2743828ae..5839aa5d4098 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -432,3 +432,5 @@ fn as_ref(&self) -> &device::Device {
         &self.0
     }
 }
+
+impl crate::dma::Device for Device {}
-- 
2.43.0

[PATCH v14 08/11] rust: platform: implement the `dma::Device` trait

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

The platform bus is potentially capable of performing DMA, hence implement
the `dma:Device` trait for `platform::Device`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/platform.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 1297f5292ba9..a7bf3525313e 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -198,3 +198,5 @@ fn as_ref(&self) -> &device::Device {
         &self.0
     }
 }
+
+impl crate::dma::Device for Device {}
-- 
2.43.0

[PATCH v14 09/11] rust: dma: use `dma::Device` in `CoherentAllocation`

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

Require a `&dyn dma::Device` in `CoherentAllocation`, such that DMA
memory can only be allocated with devices on potentially DMA capable
busses.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs       | 16 ++++++++--------
 samples/rust/rust_dma.rs |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index ac3ec0042327..d0c6569cfc70 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -57,10 +57,9 @@ fn dma_set_mask(&mut self, mask: u64) -> Result {
 /// # Examples
 ///
 /// ```
-/// use kernel::device::Device;
-/// use kernel::dma::{attrs::*, CoherentAllocation};
+/// use kernel::dma::{attrs::*, Device, CoherentAllocation};
 ///
-/// # fn test(dev: &Device) -> Result {
+/// # fn test(dev: &dyn Device) -> Result {
 /// let attribs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_WARN;
 /// let c: CoherentAllocation<u64> =
 ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, attribs)?;
@@ -178,16 +177,15 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
     /// # Examples
     ///
     /// ```
-    /// use kernel::device::Device;
-    /// use kernel::dma::{attrs::*, CoherentAllocation};
+    /// use kernel::dma::{attrs::*, Device, CoherentAllocation};
     ///
-    /// # fn test(dev: &Device) -> Result {
+    /// # fn test(dev: &dyn Device) -> Result {
     /// let c: CoherentAllocation<u64> =
     ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
     /// # Ok::<(), Error>(()) }
     /// ```
     pub fn alloc_attrs(
-        dev: &device::Device,
+        dev: &dyn Device,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
         dma_attrs: Attrs,
@@ -197,6 +195,8 @@ pub fn alloc_attrs(
             "It doesn't make sense for the allocated type to be a ZST"
         );
 
+        let dev = dev.as_ref();
+
         let size = count
             .checked_mul(core::mem::size_of::<T>())
             .ok_or(EOVERFLOW)?;
@@ -229,7 +229,7 @@ pub fn alloc_attrs(
     /// Performs the same functionality as [`CoherentAllocation::alloc_attrs`], except the
     /// `dma_attrs` is 0 by default.
     pub fn alloc_coherent(
-        dev: &device::Device,
+        dev: &dyn Device,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
     ) -> Result<CoherentAllocation<T>> {
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 1740140faba6..4c7ebf66647a 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -52,7 +52,7 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>
         dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
 
         let ca: CoherentAllocation<MyStruct> =
-            CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
+            CoherentAllocation::alloc_coherent(pdev, TEST_VALUES.len(), GFP_KERNEL)?;
 
         || -> Result {
             for (i, value) in TEST_VALUES.into_iter().enumerate() {
-- 
2.43.0

[PATCH v14 10/11] rust: samples: dma: set DMA mask

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

Set a DMA mask for the `pci::Device` in the Rust DMA sample driver.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 samples/rust/rust_dma.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 4c7ebf66647a..39b6050aa3b6 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -4,7 +4,12 @@
 //!
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
-use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
+use kernel::{
+    bindings,
+    dma::{CoherentAllocation, Device},
+    pci,
+    prelude::*,
+};
 
 struct DmaSampleDriver {
     pdev: pci::Device,
@@ -51,6 +56,8 @@ impl pci::Driver for DmaSampleDriver {
     fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
         dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
 
+        pdev.dma_set_mask_and_coherent(kernel::dma::dma_bit_mask(64))?;
+
         let ca: CoherentAllocation<MyStruct> =
             CoherentAllocation::alloc_coherent(pdev, TEST_VALUES.len(), GFP_KERNEL)?;
 
-- 
2.43.0

[PATCH v14 11/11] rust: dma: add as_slice/write functions for CoherentAllocation

[Abdiel Janulgue]

Add unsafe accessors for the region for reading or writing large
blocks of data.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs | 87 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index d0c6569cfc70..514fbb27c607 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -253,6 +253,93 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
         self.dma_handle
     }
 
+    /// Returns the data from the region starting from `offset` as a slice.
+    /// `offset` and `count` are in units of `T`, not the number of bytes.
+    ///
+    /// Due to the safety requirements of slice, the caller should consider that the region could
+    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where
+    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
+    /// used instead.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that no hardware operations that involve the buffer are currently
+    ///   taking place while the returned slice is live.
+    /// * Callers must ensure that this call does not race with a write to the same region while
+    ///   while the returned slice is live.
+    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
+        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`,
+        // we've just checked that the range and index is within bounds. The immutability of the
+        // of data is also guaranteed by the safety requirements of the function.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        // that `self.count` won't overflow early in the constructor.
+        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.add(offset), count) })
+    }
+
+    /// Performs the same functionality as [`CoherentAllocation::as_slice`], except that a mutable
+    /// slice is returned.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that no hardware operations that involve the buffer are currently
+    ///   taking place while the returned slice is live.
+    /// * Callers must ensure that this call does not race with a read or write to the same region
+    ///   while the returned slice is live.
+    pub unsafe fn as_slice_mut(&self, offset: usize, count: usize) -> Result<&mut [T]> {
+        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`,
+        // we've just checked that the range and index is within bounds. The immutability of the
+        // of data is also guaranteed by the safety requirements of the function.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        // that `self.count` won't overflow early in the constructor.
+        Ok(unsafe { core::slice::from_raw_parts_mut(self.cpu_addr.add(offset), count) })
+    }
+
+    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
+    /// number of bytes.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that no hardware operations that involve the buffer overlaps with
+    ///   this write.
+    /// * Callers must ensure that this call does not race with a read or write to the same region
+    ///   that overlaps with this write.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # fn test(alloc: &mut kernel::dma::CoherentAllocation<u8>) -> Result {
+    /// let somedata: [u8; 4] = [0xf; 4];
+    /// let buf: &[u8] = &somedata;
+    /// // SAFETY: No hw operation on the device and no other r/w access to the region at this point.
+    /// unsafe { alloc.write(buf, 0)?; }
+    /// # Ok::<(), Error>(()) }
+    /// ```
+    pub unsafe fn write(&self, src: &[T], offset: usize) -> Result {
+        let end = offset.checked_add(src.len()).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`
+        // and we've just checked that the range and index is within bounds.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        // that `self.count` won't overflow early in the constructor.
+        unsafe {
+            core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
+        };
+        Ok(())
+    }
+
     /// Returns a pointer to an element from the region with bounds checking. `offset` is in
     /// units of `T`, not the number of bytes.
     ///
-- 
2.43.0

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Boqun Feng]

On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
[...]
> +    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
> +    /// validated beforehand.
> +    ///
> +    /// Public but hidden since it should only be used from [`dma_read`] macro.
> +    #[doc(hidden)]
> +    pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
> +        // SAFETY: By the safety requirements field is valid.
> +        unsafe { field.read_volatile() }

I agree with Andreas that we should document the exception of usage on
{read,write}_volatile() here. How about:

When dealing with a potential race from a hardware or code outside
kernel (e.g. user-space program), we need that read and write on a valid
memory are not UBs. Currently {read,write}_volatile() are used for this,
and the rationale behind is that they should generate the same code as
READ_ONCE() and WRITE_ONCE() which kernel already relies on to avoid UBs
on data races. Note that the usage of {read,write}_volatile() is limited
to this particular case, they cannot be used to emit the UBs caused by
racing between two kernel functions nor do they provide atomicity.

Thoughts? One problem is that I don't know where to put this document
:-( Any suggestion?

Regards,
Boqun

> +    }
> +
> +    /// Writes a value to `field` and ensures that its type is [`AsBytes`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// This must be called from the [`dma_write`] macro which ensures that the `field` pointer is
> +    /// validated beforehand.
> +    ///
> +    /// Public but hidden since it should only be used from [`dma_write`] macro.
> +    #[doc(hidden)]
> +    pub unsafe fn field_write<F: AsBytes>(&self, field: *mut F, val: F) {
> +        // SAFETY: By the safety requirements field is valid.
> +        unsafe { field.write_volatile(val) }
> +    }
> +}
[...]

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Benno Lossin]

On Tue Mar 11, 2025 at 7:12 PM CET, Boqun Feng wrote:
> On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> [...]
>> +    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
>> +    /// validated beforehand.
>> +    ///
>> +    /// Public but hidden since it should only be used from [`dma_read`] macro.
>> +    #[doc(hidden)]
>> +    pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
>> +        // SAFETY: By the safety requirements field is valid.
>> +        unsafe { field.read_volatile() }
>
> I agree with Andreas that we should document the exception of usage on
> {read,write}_volatile() here. How about:
>
> When dealing with a potential race from a hardware or code outside
> kernel (e.g. user-space program), we need that read and write on a valid
> memory are not UBs. Currently {read,write}_volatile() are used for this,

I would use the singular `UB` here and below.

> and the rationale behind is that they should generate the same code as
> READ_ONCE() and WRITE_ONCE() which kernel already relies on to avoid UBs

s/kernel/the kernel/

> on data races. Note that the usage of {read,write}_volatile() is limited
> to this particular case, they cannot be used to emit the UBs caused by

s/emit/prevent/

> racing between two kernel functions nor do they provide atomicity.
>
> Thoughts? One problem is that I don't know where to put this document
> :-( Any suggestion?

I am a bit out of the loop on this one, but why not put into the safety
comment? I.e. explicitly state that this is *not* sound as per the usual
rules and it is a special exception?

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Boqun Feng]

On Tue, Mar 11, 2025 at 09:34:19PM +0000, Benno Lossin wrote:
> On Tue Mar 11, 2025 at 7:12 PM CET, Boqun Feng wrote:
> > On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> > [...]
> >> +    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
> >> +    /// validated beforehand.
> >> +    ///
> >> +    /// Public but hidden since it should only be used from [`dma_read`] macro.
> >> +    #[doc(hidden)]
> >> +    pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
> >> +        // SAFETY: By the safety requirements field is valid.
> >> +        unsafe { field.read_volatile() }
> >
> > I agree with Andreas that we should document the exception of usage on
> > {read,write}_volatile() here. How about:
> >
> > When dealing with a potential race from a hardware or code outside
> > kernel (e.g. user-space program), we need that read and write on a valid
> > memory are not UBs. Currently {read,write}_volatile() are used for this,
> 
> I would use the singular `UB` here and below.
> 
> > and the rationale behind is that they should generate the same code as
> > READ_ONCE() and WRITE_ONCE() which kernel already relies on to avoid UBs
> 
> s/kernel/the kernel/
> 
> > on data races. Note that the usage of {read,write}_volatile() is limited
> > to this particular case, they cannot be used to emit the UBs caused by
> 
> s/emit/prevent/
> 

These above all looks reasonable to me.

> > racing between two kernel functions nor do they provide atomicity.
> >
> > Thoughts? One problem is that I don't know where to put this document
> > :-( Any suggestion?
> 
> I am a bit out of the loop on this one, but why not put into the safety
> comment? I.e. explicitly state that this is *not* sound as per the usual
> rules and it is a special exception?
> 

We may end up with multiple uses of {read,write}_volatile(), and IIUC,
Andreas wanted [1] some clear documentation on this. Also if we have
some document it'll be easier to sync with Rust language people on the
"rules" we following in the kernel.

[1]: https://lore.kernel.org/lkml/87mse2hrd8.fsf@kernel.org/

Regards,
Boqun

Re: [PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Alexandre Courbot]

Hi Abdiel,

On Wed Mar 12, 2025 at 2:48 AM JST, Abdiel Janulgue wrote:
> From: Danilo Krummrich <dakr@kernel.org>
>
> Implement `dma_set_mask()` and `dma_set_mask_and_coherent()` in the
> `dma::Device` trait.
>
> Those methods are used to inform the kernel about the device's DMA
> addressing capabilities.
>
> 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/dma.c     |  8 ++++++++
>  rust/helpers/helpers.c |  1 +
>  rust/kernel/dma.rs     | 46 +++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 52 insertions(+), 3 deletions(-)
>  create mode 100644 rust/helpers/dma.c
>
> diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
> new file mode 100644
> index 000000000000..8eb482386f93
> --- /dev/null
> +++ b/rust/helpers/dma.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-mapping.h>
> +
> +int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
> +{
> +	return dma_set_mask_and_coherent(dev, mask);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e115be..8f3808c8b7fe 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "device.c"
> +#include "dma.c"
>  #include "err.c"
>  #include "fs.c"
>  #include "io.c"
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 7ff797a7ad18..ac3ec0042327 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -5,10 +5,10 @@
>  //! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
>  
>  use crate::{
> -    bindings, build_assert,
> -    device,
> +    bindings, build_assert, device,
>      error::code::*,
>      error::Result,
> +    prelude::*,
>      transmute::{AsBytes, FromBytes},
>      types::ARef,
>  };
> @@ -18,7 +18,35 @@
>  /// The [`Device`] trait should be implemented by bus specific device representations, where the
>  /// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
>  /// [crate::platform::Device].
> -pub trait Device: AsRef<device::Device> {}
> +pub trait Device: AsRef<device::Device> {
> +    /// Inform the kernel about the device's DMA addressing capabilities.
> +    ///
> +    /// Set both the DMA mask and the coherent DMA mask to the same value.
> +    ///
> +    /// Note that we don't check the return value from the C `dma_set_coherent_mask` as the DMA API
> +    /// guarantees that the coherent DMA mask can be set to the same or smaller than the streaming
> +    /// DMA mask.
> +    fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
> +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Same as [`Self::dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
> +    fn dma_set_mask(&mut self, mask: u64) -> Result {
> +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        let ret = unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +}

I'm not quite sure why this trait is needed.

The default implementations of the methods are not overridden in this
patchset and I don't see any scenario where they would need to be. Why
not do it the simple way by just adding an implementation block for
device::Device?

This also introduces a `&dyn Device` trait object as parameter to the
coherent allocation methods - not a big deal, but still inelegant IMHO.

Re: [PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Danilo Krummrich]

On Wed, Mar 12, 2025 at 12:37:45PM +0900, Alexandre Courbot wrote:
> On Wed Mar 12, 2025 at 2:48 AM JST, Abdiel Janulgue wrote:
> > @@ -18,7 +18,35 @@
> >  /// The [`Device`] trait should be implemented by bus specific device representations, where the
> >  /// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
> >  /// [crate::platform::Device].
> > -pub trait Device: AsRef<device::Device> {}
> > +pub trait Device: AsRef<device::Device> {
> > +    /// Inform the kernel about the device's DMA addressing capabilities.
> > +    ///
> > +    /// Set both the DMA mask and the coherent DMA mask to the same value.
> > +    ///
> > +    /// Note that we don't check the return value from the C `dma_set_coherent_mask` as the DMA API
> > +    /// guarantees that the coherent DMA mask can be set to the same or smaller than the streaming
> > +    /// DMA mask.
> > +    fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
> > +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> > +        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> > +    }
> > +
> > +    /// Same as [`Self::dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
> > +    fn dma_set_mask(&mut self, mask: u64) -> Result {
> > +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> > +        let ret = unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> > +    }
> > +}
> 
> I'm not quite sure why this trait is needed.
> 
> The default implementations of the methods are not overridden in this
> patchset and I don't see any scenario where they would need to be. Why
> not do it the simple way by just adding an implementation block for
> device::Device?
> 
> This also introduces a `&dyn Device` trait object as parameter to the
> coherent allocation methods - not a big deal, but still inelegant IMHO.

There are mainly two reasons:

(1) The dma_set_mask() function famility modifies the underlying struct device
    without a lock. Therefore, we'd need a mutable reference to the
    device::Device. However, we can't give out a mutable reference to a
    device::Device, since it'd be prone to mem::swap().

    Besides that - and I think that's even more important - we can't claim for a
    generic device::Device in general, that we own it to an extend that we can
    modify unprotected fields.

    However, we can claim this for bus specific devices in probe()[1]. Hence the
    trait, such that the DMA mask functions can be easily derived by bus
    specific devices.

(2) Setting a DMA mask only makes sense for devices that sit on a (potentially)
    DMA capable bus. Having this trait implemented for only the applicable
    device types prevents setting the DMA mask and allocating DMA memory for the
    wrong device.

    This isn't necessary for safety reasons, but I think it's still nice to
    catch.

---

[1] Gotcha with bus specific devices.

Currently bus specific devices are implemented as Device<ARef<device::Device>>.

This was a bad idea and is on my list to fix. As convinient as it is, it is
unsound, since this means that driver can always derive a mutable reference to a
bus specific device.

I think that the upcoming Ownable stuff would be a good solution, but maybe I
should wait any longer and fix it right away without Ownable.

Re: [PATCH v14 04/11] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Marek Szyprowski]

On 11.03.2025 18:48, Abdiel Janulgue wrote:
> Add an entry for the Rust dma mapping helpers abstractions.
>
> Nacked-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Acked-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   MAINTAINERS | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 007ca67cc830..aa49c551fbec 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6902,6 +6902,19 @@ F:	include/linux/dma-mapping.h
>   F:	include/linux/swiotlb.h
>   F:	kernel/dma/
>   
> +DMA MAPPING HELPERS DEVICE DRIVER API [RUST]
> +M:	Abdiel Janulgue <abdiel.janulgue@gmail.com>
> +M:	Danilo Krummrich <dakr@kernel.org>
> +R:	Daniel Almeida <daniel.almeida@collabora.com>
> +R:	Robin Murphy <robin.murphy@arm.com>
> +R:	Andreas Hindborg <a.hindborg@kernel.org>
> +L:	rust-for-linux@vger.kernel.org
> +S:	Supported
> +W:	https://protect2.fireeye.com/v1/url?k=1b4a7c0e-7a31d687-1b4bf741-74fe48600034-417eb4ae60eba6df&q=1&e=37329b17-9a96-47f7-91f4-e1d88bf7cd47&u=https%3A%2F%2Frust-for-linux.com%2F
> +T:	git https://protect2.fireeye.com/v1/url?k=243e8bac-45452125-243f00e3-74fe48600034-05e697d1fbece59e&q=1&e=37329b17-9a96-47f7-91f4-e1d88bf7cd47&u=https%3A%2F%2Fgithub.com%2FRust-for-Linux%2Flinux.git alloc-next
> +F:	rust/kernel/dma.rs
> +F:	samples/rust/rust_dma.rs
> +
>   DMA-BUF HEAPS FRAMEWORK
>   M:	Sumit Semwal <sumit.semwal@linaro.org>
>   R:	Benjamin Gaignard <benjamin.gaignard@collabora.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Abdiel Janulgue]

On 11/03/2025 23:34, Benno Lossin wrote:
> On Tue Mar 11, 2025 at 7:12 PM CET, Boqun Feng wrote:
>> On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
>> [...]
>>> +    /// Reads the value of `field` and ensures that its type is [`FromBytes`].
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
>>> +    /// validated beforehand.
>>> +    ///
>>> +    /// Public but hidden since it should only be used from [`dma_read`] macro.
>>> +    #[doc(hidden)]
>>> +    pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
>>> +        // SAFETY: By the safety requirements field is valid.
>>> +        unsafe { field.read_volatile() }
>>
>> I agree with Andreas that we should document the exception of usage on
>> {read,write}_volatile() here. How about:
>>
>> When dealing with a potential race from a hardware or code outside
>> kernel (e.g. user-space program), we need that read and write on a valid
>> memory are not UBs. Currently {read,write}_volatile() are used for this,
> 
> I would use the singular `UB` here and below.
> 
>> and the rationale behind is that they should generate the same code as
>> READ_ONCE() and WRITE_ONCE() which kernel already relies on to avoid UBs
> 
> s/kernel/the kernel/
> 
>> on data races. Note that the usage of {read,write}_volatile() is limited
>> to this particular case, they cannot be used to emit the UBs caused by
> 
> s/emit/prevent/
> 
>> racing between two kernel functions nor do they provide atomicity.
>>
>> Thoughts? One problem is that I don't know where to put this document
>> :-( Any suggestion?
> 
> I am a bit out of the loop on this one, but why not put into the safety
> comment? I.e. explicitly state that this is *not* sound as per the usual
> rules and it is a special exception?
> 

Thanks for this! I've incorporated the comments in v15

/Abdiel

Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Andreas Hindborg]

Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:

> Add a simple driver to excercise the basics of the Rust DMA
> coherent allocator bindings.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  samples/rust/Kconfig     | 11 +++++
>  samples/rust/Makefile    |  1 +
>  samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 samples/rust/rust_dma.rs
>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 3b6eae84b297..e2d14aa6beec 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_DRIVER_DMA
> +	tristate "DMA Test Driver"
> +	depends on PCI
> +	help
> +	  This option builds the Rust dma test driver sample.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called dma.
> +
> +	  If unsure, say N.
> +
>  endif # SAMPLES_RUST
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index 0dbc6d90f1ef..1a9aff6e8d6a 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA)		+= rust_dma.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
>  
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> new file mode 100644
> index 000000000000..1740140faba6
> --- /dev/null
> +++ b/samples/rust/rust_dma.rs
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust DMA api test (based on QEMU's `pci-testdev`).
> +//!
> +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
> +
> +use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
> +
> +struct DmaSampleDriver {
> +    pdev: pci::Device,
> +    ca: CoherentAllocation<MyStruct>,
> +}
> +
> +const TEST_VALUES: [(u32, u32); 5] = [
> +    (0xa, 0xb),
> +    (0xc, 0xd),
> +    (0xe, 0xf),
> +    (0xab, 0xba),
> +    (0xcd, 0xef),
> +];
> +
> +struct MyStruct {
> +    h: u32,
> +    b: u32,
> +}
> +
> +impl MyStruct {
> +    fn new(h: u32, b: u32) -> Self {
> +        Self { h, b }
> +    }
> +}
> +// SAFETY: All bit patterns are acceptable values for `MyStruct`.
> +unsafe impl kernel::transmute::AsBytes for MyStruct {}
> +// SAFETY: Instances of `MyStruct` have no uninitialized portions.
> +unsafe impl kernel::transmute::FromBytes for MyStruct {}
> +
> +kernel::pci_device_table!(
> +    PCI_TABLE,
> +    MODULE_PCI_TABLE,
> +    <DmaSampleDriver as pci::Driver>::IdInfo,
> +    [(
> +        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
> +        ()
> +    )]
> +);
> +
> +impl pci::Driver for DmaSampleDriver {
> +    type IdInfo = ();
> +    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
> +
> +    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
> +        dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
> +
> +        let ca: CoherentAllocation<MyStruct> =
> +            CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
> +
> +        || -> Result {
> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
> +                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
> +            }
> +
> +            Ok(())
> +        }()?;

Why is this placed in a closure? Left over from deferred error for pin-init?

> +
> +        let drvdata = KBox::new(
> +            Self {
> +                pdev: pdev.clone(),
> +                ca,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        Ok(drvdata.into())
> +    }
> +}
> +
> +impl Drop for DmaSampleDriver {
> +    fn drop(&mut self) {
> +        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
> +
> +        let _ = || -> Result {
> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
> +                assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
> +                assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);

We should probably change `dma_read!`/`dma_write!` to return `Result`,
so that we don't have to wrap these calls in a closure for obscure reasons.


Best regards,
Andreas Hindborg

Re: [PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Andreas Hindborg]

Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:

> From: Danilo Krummrich <dakr@kernel.org>
>
> Implement `dma_set_mask()` and `dma_set_mask_and_coherent()` in the
> `dma::Device` trait.
>
> Those methods are used to inform the kernel about the device's DMA
> addressing capabilities.
>
> 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/dma.c     |  8 ++++++++
>  rust/helpers/helpers.c |  1 +
>  rust/kernel/dma.rs     | 46 +++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 52 insertions(+), 3 deletions(-)
>  create mode 100644 rust/helpers/dma.c
>
> diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
> new file mode 100644
> index 000000000000..8eb482386f93
> --- /dev/null
> +++ b/rust/helpers/dma.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-mapping.h>
> +
> +int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
> +{
> +	return dma_set_mask_and_coherent(dev, mask);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e115be..8f3808c8b7fe 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "device.c"
> +#include "dma.c"
>  #include "err.c"
>  #include "fs.c"
>  #include "io.c"
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 7ff797a7ad18..ac3ec0042327 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -5,10 +5,10 @@
>  //! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
>  
>  use crate::{
> -    bindings, build_assert,
> -    device,
> +    bindings, build_assert, device,
>      error::code::*,
>      error::Result,
> +    prelude::*,
>      transmute::{AsBytes, FromBytes},
>      types::ARef,
>  };
> @@ -18,7 +18,35 @@
>  /// The [`Device`] trait should be implemented by bus specific device representations, where the
>  /// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
>  /// [crate::platform::Device].
> -pub trait Device: AsRef<device::Device> {}
> +pub trait Device: AsRef<device::Device> {
> +    /// Inform the kernel about the device's DMA addressing capabilities.
> +    ///
> +    /// Set both the DMA mask and the coherent DMA mask to the same value.
> +    ///
> +    /// Note that we don't check the return value from the C `dma_set_coherent_mask` as the DMA API
> +    /// guarantees that the coherent DMA mask can be set to the same or smaller than the streaming
> +    /// DMA mask.
> +    fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
> +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }

As mentioned earlier [1], I think we should use `to_result` here.


Best regards,
Andreas Hindborg



[1] https://lore.kernel.org/all/871pv3l3vi.fsf@kernel.org/

Re: [PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Andreas Hindborg]

Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:

> From: Danilo Krummrich <dakr@kernel.org>
>
> Implement `dma_set_mask()` and `dma_set_mask_and_coherent()` in the
> `dma::Device` trait.
>
> Those methods are used to inform the kernel about the device's DMA
> addressing capabilities.
>
> 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>

[...]

> +
> +/// Helper function to set the bit mask for DMA addressing.
> +pub const fn dma_bit_mask(n: usize) -> u64 {
> +    if n > 64 {
> +        return 0;
> +    }
> +    if n == 64 {
> +        !0
> +    } else {
> +        (1 << (n)) - 1
> +    }
> +}

I think this is dead code?


Best regards,
Andreas Hindborg

Re: [PATCH v14 09/11] rust: dma: use `dma::Device` in `CoherentAllocation`

[Andreas Hindborg]

Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:

> From: Danilo Krummrich <dakr@kernel.org>
>
> Require a `&dyn dma::Device` in `CoherentAllocation`, such that DMA
> memory can only be allocated with devices on potentially DMA capable
> busses.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/dma.rs       | 16 ++++++++--------
>  samples/rust/rust_dma.rs |  2 +-
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index ac3ec0042327..d0c6569cfc70 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -57,10 +57,9 @@ fn dma_set_mask(&mut self, mask: u64) -> Result {
>  /// # Examples
>  ///
>  /// ```
> -/// use kernel::device::Device;
> -/// use kernel::dma::{attrs::*, CoherentAllocation};
> +/// use kernel::dma::{attrs::*, Device, CoherentAllocation};
>  ///
> -/// # fn test(dev: &Device) -> Result {
> +/// # fn test(dev: &dyn Device) -> Result {
>  /// let attribs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_WARN;
>  /// let c: CoherentAllocation<u64> =
>  ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, attribs)?;
> @@ -178,16 +177,15 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
>      /// # Examples
>      ///
>      /// ```
> -    /// use kernel::device::Device;
> -    /// use kernel::dma::{attrs::*, CoherentAllocation};
> +    /// use kernel::dma::{attrs::*, Device, CoherentAllocation};
>      ///
> -    /// # fn test(dev: &Device) -> Result {
> +    /// # fn test(dev: &dyn Device) -> Result {
>      /// let c: CoherentAllocation<u64> =
>      ///     CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
>      /// # Ok::<(), Error>(()) }
>      /// ```
>      pub fn alloc_attrs(
> -        dev: &device::Device,
> +        dev: &dyn Device,
>          count: usize,
>          gfp_flags: kernel::alloc::Flags,
>          dma_attrs: Attrs,
> @@ -197,6 +195,8 @@ pub fn alloc_attrs(
>              "It doesn't make sense for the allocated type to be a ZST"
>          );
>  
> +        let dev = dev.as_ref();
> +
>          let size = count
>              .checked_mul(core::mem::size_of::<T>())
>              .ok_or(EOVERFLOW)?;
> @@ -229,7 +229,7 @@ pub fn alloc_attrs(
>      /// Performs the same functionality as [`CoherentAllocation::alloc_attrs`], except the
>      /// `dma_attrs` is 0 by default.
>      pub fn alloc_coherent(
> -        dev: &device::Device,
> +        dev: &dyn Device,

Is it possible (and would it make sense?) to make these methods, or even
`CoherentAllocation` generic over the `D: Device` to get rid of the
dynamic dispatch:

    pub fn alloc_coherent(
        dev: &impl Device,
        count: usize,
        gfp_flags: kernel::alloc::Flags,
    ) -> Result<CoherentAllocation<T>> {


Best regards,
Andreas Hindborg

Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Abdiel Janulgue]

Hi,

On 18/03/2025 15:26, Andreas Hindborg wrote:
> Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:
> 
>> Add a simple driver to excercise the basics of the Rust DMA
>> coherent allocator bindings.
>>
>> Suggested-by: Danilo Krummrich <dakr@kernel.org>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> ---
>>   samples/rust/Kconfig     | 11 +++++
>>   samples/rust/Makefile    |  1 +
>>   samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 109 insertions(+)
>>   create mode 100644 samples/rust/rust_dma.rs
>>
>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> index 3b6eae84b297..e2d14aa6beec 100644
>> --- a/samples/rust/Kconfig
>> +++ b/samples/rust/Kconfig
>> @@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
>>   
>>   	  If unsure, say N.
>>   
>> +config SAMPLE_RUST_DRIVER_DMA
>> +	tristate "DMA Test Driver"
>> +	depends on PCI
>> +	help
>> +	  This option builds the Rust dma test driver sample.
>> +
>> +	  To compile this as a module, choose M here:
>> +	  the module will be called dma.
>> +
>> +	  If unsure, say N.
>> +
>>   endif # SAMPLES_RUST
>> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>> index 0dbc6d90f1ef..1a9aff6e8d6a 100644
>> --- a/samples/rust/Makefile
>> +++ b/samples/rust/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
>> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA)		+= rust_dma.o
>>   
>>   rust_print-y := rust_print_main.o rust_print_events.o
>>   
>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>> new file mode 100644
>> index 000000000000..1740140faba6
>> --- /dev/null
>> +++ b/samples/rust/rust_dma.rs
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust DMA api test (based on QEMU's `pci-testdev`).
>> +//!
>> +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
>> +
>> +use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
>> +
>> +struct DmaSampleDriver {
>> +    pdev: pci::Device,
>> +    ca: CoherentAllocation<MyStruct>,
>> +}
>> +
>> +const TEST_VALUES: [(u32, u32); 5] = [
>> +    (0xa, 0xb),
>> +    (0xc, 0xd),
>> +    (0xe, 0xf),
>> +    (0xab, 0xba),
>> +    (0xcd, 0xef),
>> +];
>> +
>> +struct MyStruct {
>> +    h: u32,
>> +    b: u32,
>> +}
>> +
>> +impl MyStruct {
>> +    fn new(h: u32, b: u32) -> Self {
>> +        Self { h, b }
>> +    }
>> +}
>> +// SAFETY: All bit patterns are acceptable values for `MyStruct`.
>> +unsafe impl kernel::transmute::AsBytes for MyStruct {}
>> +// SAFETY: Instances of `MyStruct` have no uninitialized portions.
>> +unsafe impl kernel::transmute::FromBytes for MyStruct {}
>> +
>> +kernel::pci_device_table!(
>> +    PCI_TABLE,
>> +    MODULE_PCI_TABLE,
>> +    <DmaSampleDriver as pci::Driver>::IdInfo,
>> +    [(
>> +        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
>> +        ()
>> +    )]
>> +);
>> +
>> +impl pci::Driver for DmaSampleDriver {
>> +    type IdInfo = ();
>> +    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
>> +
>> +    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
>> +        dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
>> +
>> +        let ca: CoherentAllocation<MyStruct> =
>> +            CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>> +
>> +        || -> Result {
>> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> +                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
>> +            }
>> +
>> +            Ok(())
>> +        }()?;
> 
> Why is this placed in a closure? Left over from deferred error for pin-init?
> 

The macro expands into a block which needs to validate the function 
item_from_index()?

>> +
>> +        let drvdata = KBox::new(
>> +            Self {
>> +                pdev: pdev.clone(),
>> +                ca,
>> +            },
>> +            GFP_KERNEL,
>> +        )?;
>> +
>> +        Ok(drvdata.into())
>> +    }
>> +}
>> +
>> +impl Drop for DmaSampleDriver {
>> +    fn drop(&mut self) {
>> +        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>> +
>> +        let _ = || -> Result {
>> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> +                assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
>> +                assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
> 
> We should probably change `dma_read!`/`dma_write!` to return `Result`,
> so that we don't have to wrap these calls in a closure for obscure reasons.
> 

Changing `dma_read!`/`dma_write!` to return `Result` is probably not a 
trivial change, would it be okay to have this fixed in a subsequent patch?

Thanks!

/Abdiel

Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Miguel Ojeda]

On Tue, Mar 18, 2025 at 7:42 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Changing `dma_read!`/`dma_write!` to return `Result` is probably not a
> trivial change, would it be okay to have this fixed in a subsequent patch?

Yeah, if you feel the need for a v16, I would suggest focusing on
changing whatever is essential only, since I will be applying the
first part of the series soon.

Thanks!

Cheers,
Miguel

Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Andreas Hindborg]

"Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes:

> Hi,
>
> On 18/03/2025 15:26, Andreas Hindborg wrote:
>> Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:
>>
>>> Add a simple driver to excercise the basics of the Rust DMA
>>> coherent allocator bindings.
>>>
>>> Suggested-by: Danilo Krummrich <dakr@kernel.org>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>>> ---
>>>   samples/rust/Kconfig     | 11 +++++
>>>   samples/rust/Makefile    |  1 +
>>>   samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 109 insertions(+)
>>>   create mode 100644 samples/rust/rust_dma.rs
>>>
>>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>>> index 3b6eae84b297..e2d14aa6beec 100644
>>> --- a/samples/rust/Kconfig
>>> +++ b/samples/rust/Kconfig
>>> @@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
>>>
>>>   	  If unsure, say N.
>>>
>>> +config SAMPLE_RUST_DRIVER_DMA
>>> +	tristate "DMA Test Driver"
>>> +	depends on PCI
>>> +	help
>>> +	  This option builds the Rust dma test driver sample.
>>> +
>>> +	  To compile this as a module, choose M here:
>>> +	  the module will be called dma.
>>> +
>>> +	  If unsure, say N.
>>> +
>>>   endif # SAMPLES_RUST
>>> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>>> index 0dbc6d90f1ef..1a9aff6e8d6a 100644
>>> --- a/samples/rust/Makefile
>>> +++ b/samples/rust/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>>>   obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
>>> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA)		+= rust_dma.o
>>>
>>>   rust_print-y := rust_print_main.o rust_print_events.o
>>>
>>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>>> new file mode 100644
>>> index 000000000000..1740140faba6
>>> --- /dev/null
>>> +++ b/samples/rust/rust_dma.rs
>>> @@ -0,0 +1,97 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Rust DMA api test (based on QEMU's `pci-testdev`).
>>> +//!
>>> +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
>>> +
>>> +use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
>>> +
>>> +struct DmaSampleDriver {
>>> +    pdev: pci::Device,
>>> +    ca: CoherentAllocation<MyStruct>,
>>> +}
>>> +
>>> +const TEST_VALUES: [(u32, u32); 5] = [
>>> +    (0xa, 0xb),
>>> +    (0xc, 0xd),
>>> +    (0xe, 0xf),
>>> +    (0xab, 0xba),
>>> +    (0xcd, 0xef),
>>> +];
>>> +
>>> +struct MyStruct {
>>> +    h: u32,
>>> +    b: u32,
>>> +}
>>> +
>>> +impl MyStruct {
>>> +    fn new(h: u32, b: u32) -> Self {
>>> +        Self { h, b }
>>> +    }
>>> +}
>>> +// SAFETY: All bit patterns are acceptable values for `MyStruct`.
>>> +unsafe impl kernel::transmute::AsBytes for MyStruct {}
>>> +// SAFETY: Instances of `MyStruct` have no uninitialized portions.
>>> +unsafe impl kernel::transmute::FromBytes for MyStruct {}
>>> +
>>> +kernel::pci_device_table!(
>>> +    PCI_TABLE,
>>> +    MODULE_PCI_TABLE,
>>> +    <DmaSampleDriver as pci::Driver>::IdInfo,
>>> +    [(
>>> +        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
>>> +        ()
>>> +    )]
>>> +);
>>> +
>>> +impl pci::Driver for DmaSampleDriver {
>>> +    type IdInfo = ();
>>> +    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
>>> +
>>> +    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
>>> +        dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
>>> +
>>> +        let ca: CoherentAllocation<MyStruct> =
>>> +            CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>>> +
>>> +        || -> Result {
>>> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
>>> +                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
>>> +            }
>>> +
>>> +            Ok(())
>>> +        }()?;
>>
>> Why is this placed in a closure? Left over from deferred error for pin-init?
>>
>
> The macro expands into a block which needs to validate the function
> item_from_index()?

It is not required here, since we are already in a function that returns
result.

>
>>> +
>>> +        let drvdata = KBox::new(
>>> +            Self {
>>> +                pdev: pdev.clone(),
>>> +                ca,
>>> +            },
>>> +            GFP_KERNEL,
>>> +        )?;
>>> +
>>> +        Ok(drvdata.into())
>>> +    }
>>> +}
>>> +
>>> +impl Drop for DmaSampleDriver {
>>> +    fn drop(&mut self) {
>>> +        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>>> +
>>> +        let _ = || -> Result {
>>> +            for (i, value) in TEST_VALUES.into_iter().enumerate() {
>>> +                assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
>>> +                assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
>>
>> We should probably change `dma_read!`/`dma_write!` to return `Result`,
>> so that we don't have to wrap these calls in a closure for obscure reasons.
>>
>
> Changing `dma_read!`/`dma_write!` to return `Result` is probably not a
> trivial change, would it be okay to have this fixed in a subsequent patch?

It is actually trivial:

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 027ef75a461a..52d235f61886 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -446,20 +446,22 @@ fn drop(&mut self) {
 #[macro_export]
 macro_rules! dma_read {
     ($dma:expr, $idx: expr, $($field:tt)*) => {{
-        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-        // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
-        // dereferenced. The compiler also further validates the expression on whether `field`
-        // is a member of `item` when expanded by the macro.
-        unsafe {
-            let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
-            $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
-        }
+        (|| -> Result<_> {
+            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+            // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+            // dereferenced. The compiler also further validates the expression on whether `field`
+            // is a member of `item` when expanded by the macro.
+            unsafe {
+                let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
+                ::core::result::Result::Ok($crate::dma::CoherentAllocation::field_read(&$dma, ptr_field))
+            }
+        })()
     }};
     ($dma:ident [ $idx:expr ] $($field:tt)* ) => {
-        $crate::dma_read!($dma, $idx, $($field)*);
+        $crate::dma_read!($dma, $idx, $($field)*)
     };
     ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
-        $crate::dma_read!($($dma).*, $idx, $($field)*);
+        $crate::dma_read!($($dma).*, $idx, $($field)*)
     };
 }
 
@@ -486,25 +488,31 @@ macro_rules! dma_read {
 #[macro_export]
 macro_rules! dma_write {
     ($dma:ident [ $idx:expr ] $($field:tt)*) => {{
-        $crate::dma_write!($dma, $idx, $($field)*);
+        $crate::dma_write!($dma, $idx, $($field)*)
     }};
     ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
-        $crate::dma_write!($($dma).*, $idx, $($field)*);
+        $crate::dma_write!($($dma).*, $idx, $($field)*)
     }};
     ($dma:expr, $idx: expr, = $val:expr) => {
-        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-        // SAFETY: `item_from_index` ensures that `item` is always a valid item.
-        unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+        (|| -> Result {
+            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+            // SAFETY: `item_from_index` ensures that `item` is always a valid item.
+            unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+            ::core::result::Result::Ok(())
+        })()
     };
     ($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
-        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-        // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
-        // dereferenced. The compiler also further validates the expression on whether `field`
-        // is a member of `item` when expanded by the macro.
-        unsafe {
-            let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
-            $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
-        }
+        (|| -> Result {
+            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+            // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+            // dereferenced. The compiler also further validates the expression on whether `field`
+            // is a member of `item` when expanded by the macro.
+            unsafe {
+                let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
+                $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
+            }
+            ::core::result::Result::Ok(())
+        })()
     };
 }
 
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 39b6050aa3b6..962e65322893 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -61,13 +61,10 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>
         let ca: CoherentAllocation<MyStruct> =
             CoherentAllocation::alloc_coherent(pdev, TEST_VALUES.len(), GFP_KERNEL)?;
 
-        || -> Result {
-            for (i, value) in TEST_VALUES.into_iter().enumerate() {
-                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
-            }
+        for (i, value) in TEST_VALUES.into_iter().enumerate() {
+            kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
+        }
 
-            Ok(())
-        }()?;
 
         let drvdata = KBox::new(
             Self {
@@ -85,13 +82,10 @@ impl Drop for DmaSampleDriver {
     fn drop(&mut self) {
         dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
 
-        let _ = || -> Result {
-            for (i, value) in TEST_VALUES.into_iter().enumerate() {
-                assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
-                assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
-            }
-            Ok(())
-        }();
+        for (i, value) in TEST_VALUES.into_iter().enumerate() {
+            assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
+            assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
+        }
     }
 }
 



Best regards,
Andreas Hindborg

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Jason Gunthorpe]

On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> +    dev: ARef<Device>,
> +    dma_handle: bindings::dma_addr_t,
> +    count: usize,
> +    cpu_addr: *mut T,
> +    dma_attrs: Attrs,
> +}

I'd like to point out how memory wasteful this is from what real
drivers are doing today when they use the coherent API. Let's compare
against SMMUv3's use for the CD table..

This would be the code in arm_smmu_alloc_cd_ptr()

It is making a 2 level radix tree.

The cpu_addr is stored in a linear array of pointers:

			struct arm_smmu_cdtab_l2 **l2ptrs;

The dma_addr is encoded into the HW data structure itself:

		arm_smmu_write_cd_l1_desc(&cd_table->l2.l1tab[idx],
						  l2ptr_dma);

The size of the allocation is fixed size:
			*l2ptr = dma_alloc_coherent(smmu->dev, sizeof(**l2ptr),
                                                                ^^^^^^^^^^^^
						    &l2ptr_dma, GFP_KERNEL);

It doesn't need a struct device pointer or reference because this uses
the usual kernel 'fence' reasoning for destruction.

It doesn't even use dma_attrs. (why is this in a long term struct?)

So, smmu manages to do this with a single array of 8 bytes/entry to shadow
the CPU pointer, and recovers the dma_addr from the HW data structure:

			dma_free_coherent(smmu->dev,
					  sizeof(*cd_table->l2.l2ptrs[i]),
					  cd_table->l2.l2ptrs[i],
					  arm_smmu_cd_l1_get_desc(&cd_table->l2.l1tab[i]));

Basically, it was designed to be very memory efficient.

If we imagine driving the same HW in rust the array storing the CPU
pointer would have to expand to 40 bytes/entry to hold every
CoherentAllocation. This means rust would need a new high order memory
allocation to hold the CoherentAllocation memory array!

Jason

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 03:25:39PM -0300, Jason Gunthorpe wrote:
> On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> > +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> > +    dev: ARef<Device>,
> > +    dma_handle: bindings::dma_addr_t,
> > +    count: usize,
> > +    cpu_addr: *mut T,
> > +    dma_attrs: Attrs,
> > +}
> 
> I'd like to point out how memory wasteful this is from what real
> drivers are doing today when they use the coherent API. Let's compare
> against SMMUv3's use for the CD table..
> 
> This would be the code in arm_smmu_alloc_cd_ptr()
> 
> It is making a 2 level radix tree.
> 
> The cpu_addr is stored in a linear array of pointers:
> 
> 			struct arm_smmu_cdtab_l2 **l2ptrs;
> 
> The dma_addr is encoded into the HW data structure itself:
> 
> 		arm_smmu_write_cd_l1_desc(&cd_table->l2.l1tab[idx],
> 						  l2ptr_dma);
> 
> The size of the allocation is fixed size:
> 			*l2ptr = dma_alloc_coherent(smmu->dev, sizeof(**l2ptr),
>                                                                 ^^^^^^^^^^^^
> 						    &l2ptr_dma, GFP_KERNEL);
> 
> It doesn't need a struct device pointer or reference because this uses
> the usual kernel 'fence' reasoning for destruction.
> 
> It doesn't even use dma_attrs. (why is this in a long term struct?)
> 
> So, smmu manages to do this with a single array of 8 bytes/entry to shadow
> the CPU pointer, and recovers the dma_addr from the HW data structure:
> 
> 			dma_free_coherent(smmu->dev,
> 					  sizeof(*cd_table->l2.l2ptrs[i]),
> 					  cd_table->l2.l2ptrs[i],
> 					  arm_smmu_cd_l1_get_desc(&cd_table->l2.l1tab[i]));
> 
> Basically, it was designed to be very memory efficient.
> 
> If we imagine driving the same HW in rust the array storing the CPU
> pointer would have to expand to 40 bytes/entry to hold every
> CoherentAllocation. This means rust would need a new high order memory
> allocation to hold the CoherentAllocation memory array!

One solution that comes immediately to my mind is to have something like a
CoherentAllocationPool that holds all the repeating metadata and provides an API
to give you continuous allocations of the same kind without any bloat.

One thing I'd like to note though is that we can't have a perfect solutions for
everything from the get-go. We have to start somewhere and work it out
continuously and keep improving things as required.

So, don't get me wrong, I really think you have a valid point with that. But we
also can't consider every use-case from the get-go, push a huge amount of code
and then get questioned on why there's no user for all this. :) IMHO, it's about
finding a good balance.

Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Boqun Feng]

On Fri, Mar 21, 2025 at 03:25:39PM -0300, Jason Gunthorpe wrote:
> On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> > +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> > +    dev: ARef<Device>,
> > +    dma_handle: bindings::dma_addr_t,
> > +    count: usize,
> > +    cpu_addr: *mut T,
> > +    dma_attrs: Attrs,
> > +}
> 
> I'd like to point out how memory wasteful this is from what real
> drivers are doing today when they use the coherent API. Let's compare
> against SMMUv3's use for the CD table..
> 
> This would be the code in arm_smmu_alloc_cd_ptr()
> 
> It is making a 2 level radix tree.
> 
> The cpu_addr is stored in a linear array of pointers:
> 
> 			struct arm_smmu_cdtab_l2 **l2ptrs;
> 
> The dma_addr is encoded into the HW data structure itself:
> 
> 		arm_smmu_write_cd_l1_desc(&cd_table->l2.l1tab[idx],
> 						  l2ptr_dma);
> 
> The size of the allocation is fixed size:
> 			*l2ptr = dma_alloc_coherent(smmu->dev, sizeof(**l2ptr),
>                                                                 ^^^^^^^^^^^^
> 						    &l2ptr_dma, GFP_KERNEL);
> 
> It doesn't need a struct device pointer or reference because this uses
> the usual kernel 'fence' reasoning for destruction.
> 
> It doesn't even use dma_attrs. (why is this in a long term struct?)
> 
> So, smmu manages to do this with a single array of 8 bytes/entry to shadow
> the CPU pointer, and recovers the dma_addr from the HW data structure:
> 
> 			dma_free_coherent(smmu->dev,
> 					  sizeof(*cd_table->l2.l2ptrs[i]),
> 					  cd_table->l2.l2ptrs[i],
> 					  arm_smmu_cd_l1_get_desc(&cd_table->l2.l1tab[i]));
> 
> Basically, it was designed to be very memory efficient.
> 
> If we imagine driving the same HW in rust the array storing the CPU
> pointer would have to expand to 40 bytes/entry to hold every
> CoherentAllocation. This means rust would need a new high order memory
> allocation to hold the CoherentAllocation memory array!
> 

Thanks for the example, it seems to me that your case needs a

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes> {
        dev: ARef<Device>,
        cpu_addrs: KVec<(*mut T, bindings::dma_addr_t)>,
        dma_attrs: Attrs,
    }

of course, we can get rid of `bindings::dma_addr_t` if there is a
method:

    impl<T: ...> CoherentAllocationVec<T> {
        pub fn get_dma_handle(&self, idx: usize) -> bindings::dma_addr_t { 
	    ...
	    // probably only availabe for a particular T or Vec.
	}
    }

    // and drop of `CoherentAllocationVec` will be:

    impl<T: ...> Drop for CoherentAllocationVec<T> {
        fn drop(&mut self) {
            for (i, cpu_addr) in self.cpu_addrs.as_slice().iter().enumerate() {
	    	dma_free_coherent_attr(self.dev.as_raw(),
		                       core::mem::size_of::<T>(),
				       cpu_addr,
				       self.get_dma_handle(i),
				       self.attrs);
	    }
	    ...
	}
    }

Then we have:

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes> {
        dev: ARef<Device>,
        cpu_addrs: KVec<*mut T>,
        dma_attrs: Attrs,
    }

And we can make `dma_attrs` a const of the type:

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes, const ATTRS: Attrs = Attrs(0)> {
        dev: ARef<Device>,
        cpu_addr: KVec<*mut T>,
    }

As for getting rid of the `dev` pointer, the struct arm_smmu_device has
a pointer to struct device as well, so it's all about how to organize
the fields, at very least, you could do:

    pub struct ArmSmmuDevice {
        // avoid using an ARef<Device> here since we already has it in
	// cdtable.

        cdtable: CoherentAllocationVec<arm_smmu_cdtab_l2>,
	...,
    }

and whenever you need to get a pointer/reference to the device, you can
get it from:

    .cdtable.dev

it may not be the best organization of the fields, but we will see the
real Rust use for a better design.

Regards,
Boqun

> Jason