[PATCH v13 0/7] rust: add dma coherent allocator abstraction

[PATCH v13 0/7] 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 v13, the biggest change is to move out
the as_slice and write helpers for later discussion. The object is also
now wrapped in Devres to explicitly tie it to the lifetime of the
device[1]. This is tested on a Nvidia GA104 GPU device using PoC code
which parses and loads the GSP firmware via DMA.

Changes sinve 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).
- Link to v12: https://lore.kernel.org/lkml/20250224115007.2072043-1-abdiel.janulgue@gmail.com/

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 (6):
  rust: error: Add EOVERFLOW
  rust: add dma coherent allocator abstraction.
  rust: device: add dma addressing capabilities
  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 (1):
  rust: pci: impl AsMut<Device> for pci::Device

 MAINTAINERS                     |  12 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/dma.c              |   8 +
 rust/helpers/helpers.c          |   1 +
 rust/kernel/device.rs           |  29 ++
 rust/kernel/dma.rs              | 464 ++++++++++++++++++++++++++++++++
 rust/kernel/error.rs            |   1 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/pci.rs              |  11 +
 samples/rust/Kconfig            |  11 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_dma.rs        | 104 +++++++
 12 files changed, 644 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: 6ad64bf91728502fe8a4d1419c0a3e4fd323f503
-- 
2.43.0

[PATCH v13 1/7] 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 v13 2/7] 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.

A CoherentAllocation is wrapped in Devres which basically guarantees
that a driver can't make a CoherentAllocation out-live driver unbind.
This is needed, since DMA allocations potentially also result in
programming of the IOMMU. IOMMU mappings are device resources and
hence the device / driver lifecycle needs to be enforced.

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

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index f46cf3bb7069..bf1110590c19 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..8a250242641c
--- /dev/null
+++ b/rust/kernel/dma.rs
@@ -0,0 +1,378 @@
+// 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,
+    devres::Devres,
+    error::code::*,
+    error::Result,
+    transmute::{AsBytes, FromBytes},
+    types::ARef,
+};
+use kernel::prelude::*;
+
+/// 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, devres::Devres,};
+/// use kernel::dma::{attrs::*, CoherentAllocation};
+///
+/// # fn test(dev: &Device) -> Result {
+/// let attribs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_WARN;
+/// let c: Devres<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.
+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, devres::Devres,};
+    /// use kernel::dma::{attrs::*, CoherentAllocation};
+    ///
+    /// # fn test(dev: &Device) -> Result {
+    /// let c: Devres<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<Devres<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.
+        let devres = Devres::new(
+            dev,
+            Self {
+                dev: dev.into(),
+                dma_handle,
+                count,
+                cpu_addr: ret as *mut T,
+                dma_attrs,
+            },
+            GFP_KERNEL,
+        )?;
+
+        Ok(devres)
+    }
+
+    /// Performs the same functionality as [`alloc_attrs`], except the `dma_attrs` is 0 by default.
+    pub fn alloc_coherent(
+        dev: &Device,
+        count: usize,
+        gfp_flags: kernel::alloc::Flags,
+    ) -> Result<Devres<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, devres::Devres,};
+/// 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(reg: &Devres<CoherentAllocation<MyStruct>>) -> Result {
+/// let alloc = reg.try_access().ok_or(ENXIO)?;
+/// 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, devres::Devres,};
+/// 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(reg: &Devres<CoherentAllocation<MyStruct>>) -> Result {
+/// let alloc = reg.try_access().ok_or(ENXIO)?;
+/// 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)
+        }
+    };
+}
+
+/// 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
+    }
+}
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 v13 3/7] rust: pci: impl AsMut<Device> for pci::Device

[Abdiel Janulgue]

From: Danilo Krummrich <dakr@kernel.org>

Some device methods require mutable references, since they change the
underlying struct device without lock protection.

Hence, make it possible to retrieve a mutable reference to a Device from
a mutable pci::Device.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..141430dac2d5 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -432,3 +432,14 @@ fn as_ref(&self) -> &device::Device {
         &self.0
     }
 }
+
+impl AsMut<device::Device> for Device {
+    fn as_mut(&mut self) -> &mut device::Device {
+        // SAFETY:
+        // - `self.0.as_raw()` is valid by the type invariant of `device::Device`,
+        // - `struct device` is embedded in `struct pci_dev`, hence it is safe to give out a
+        //    mutable reference for `device::Device` if we have a mutable reference to the
+        //    corresponding `pci::Device`.
+        unsafe { &mut *self.0.as_raw().cast() }
+    }
+}
-- 
2.43.0

[PATCH v13 4/7] rust: device: add dma addressing capabilities

[Abdiel Janulgue]

Add functions to set the DMA mask to inform the kernel about the
device's DMA addressing capabilities.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/helpers/dma.c     |  8 ++++++++
 rust/helpers/helpers.c |  1 +
 rust/kernel/device.rs  | 29 +++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)
 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/device.rs b/rust/kernel/device.rs
index db2d9658ba47..f9d3d4f60ddb 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,10 +6,12 @@
 
 use crate::{
     bindings,
+    error::Result,
     str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
+use kernel::prelude::*;
 
 #[cfg(CONFIG_PRINTK)]
 use crate::c_str;
@@ -187,6 +189,33 @@ pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
         unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
     }
+
+    /// Inform the kernel about the device's DMA addressing capabilities.
+    ///
+    /// Set both the DMA mask and the coherent DMA mask to the same thing.
+    /// 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.
+    pub fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
+        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
+        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_raw(), mask) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Same as [`dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
+    pub fn dma_set_mask(&mut self, mask: u64) -> Result {
+        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
+        let ret = unsafe { bindings::dma_set_mask(self.as_raw(), mask) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
 }
 
 // SAFETY: Instances of `Device` are always reference-counted.
-- 
2.43.0

[PATCH v13 5/7] 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 | 104 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 samples/rust/rust_dma.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 3b6eae84b297..04bbba870bd1 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..aed55ae4dcfe
--- /dev/null
+++ b/samples/rust/rust_dma.rs
@@ -0,0 +1,104 @@
+// 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: kernel::devres::Devres<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>>> {
+        let dev = pdev.as_mut();
+
+        dev.dma_set_mask_and_coherent(kernel::dma::dma_bit_mask(64))?;
+
+        dev_info!(dev, "Probe DMA test driver.\n");
+
+        let ca: kernel::devres::Devres<CoherentAllocation<MyStruct>> =
+            CoherentAllocation::alloc_coherent(dev, TEST_VALUES.len(), GFP_KERNEL)?;
+
+        || -> Result {
+            let reg = ca.try_access().ok_or(ENXIO)?;
+
+            for (i, value) in TEST_VALUES.into_iter().enumerate() {
+                kernel::dma_write!(reg[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 {
+            let reg = self.ca.try_access().ok_or(ENXIO)?;
+            for (i, value) in TEST_VALUES.into_iter().enumerate() {
+                assert_eq!(kernel::dma_read!(reg[i].h), value.0);
+                assert_eq!(kernel::dma_read!(reg[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 v13 6/7] 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 12f2d79ce174..72daf5db0242 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6902,6 +6902,18 @@ 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 rust-next
+F:	rust/kernel/dma.rs
+
 DMA-BUF HEAPS FRAMEWORK
 M:	Sumit Semwal <sumit.semwal@linaro.org>
 R:	Benjamin Gaignard <benjamin.gaignard@collabora.com>
-- 
2.43.0

[PATCH v13 7/7] 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 | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 8a250242641c..cee7adc2fc22 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -213,6 +213,92 @@ 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 [`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 v13 0/7] rust: add dma coherent allocator abstraction

[Danilo Krummrich]

On Fri, Mar 07, 2025 at 01:06:17PM +0200, Abdiel Janulgue wrote:
> This series adds the rust bindings for the dma coherent allocator which
> is needed for nova-core[0]. For v13, the biggest change is to move out
> the as_slice and write helpers for later discussion. The object is also
> now wrapped in Devres to explicitly tie it to the lifetime of the
> device[1].

Just to clarify:

What is meant here is that a CoherentAllocation can't out-live driver unbind.
This is not equivalent to the lifetime of the corresponding struct device.

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Alice Ryhl]

On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:
> 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.
> 
> A CoherentAllocation is wrapped in Devres which basically guarantees
> that a driver can't make a CoherentAllocation out-live driver unbind.
> This is needed, since DMA allocations potentially also result in
> programming of the IOMMU. IOMMU mappings are device resources and
> hence the device / driver lifecycle needs to be enforced.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>

You might want to add #[inline] annotations to the various methods, but
otherwise LGTM.

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

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

You don't use this method?

Alice

Re: [PATCH v13 3/7] rust: pci: impl AsMut<Device> for pci::Device

[Alice Ryhl]

On Fri, Mar 07, 2025 at 01:06:20PM +0200, Abdiel Janulgue wrote:
> From: Danilo Krummrich <dakr@kernel.org>
> 
> Some device methods require mutable references, since they change the
> underlying struct device without lock protection.
> 
> Hence, make it possible to retrieve a mutable reference to a Device from
> a mutable pci::Device.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/pci.rs | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..141430dac2d5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -432,3 +432,14 @@ fn as_ref(&self) -> &device::Device {
>          &self.0
>      }
>  }
> +
> +impl AsMut<device::Device> for Device {
> +    fn as_mut(&mut self) -> &mut device::Device {
> +        // SAFETY:
> +        // - `self.0.as_raw()` is valid by the type invariant of `device::Device`,
> +        // - `struct device` is embedded in `struct pci_dev`, hence it is safe to give out a
> +        //    mutable reference for `device::Device` if we have a mutable reference to the
> +        //    corresponding `pci::Device`.
> +        unsafe { &mut *self.0.as_raw().cast() }
> +    }
> +}

This makes it possible to call `mem::swap` on two devices, which I doubt
is allowed.

Alice

Re: [PATCH v13 3/7] rust: pci: impl AsMut<Device> for pci::Device

[Danilo Krummrich]

On Fri, Mar 07, 2025 at 11:18:03AM +0000, Alice Ryhl wrote:
> On Fri, Mar 07, 2025 at 01:06:20PM +0200, Abdiel Janulgue wrote:
> > From: Danilo Krummrich <dakr@kernel.org>
> > 
> > Some device methods require mutable references, since they change the
> > underlying struct device without lock protection.
> > 
> > Hence, make it possible to retrieve a mutable reference to a Device from
> > a mutable pci::Device.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/pci.rs | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 4c98b5b9aa1e..141430dac2d5 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -432,3 +432,14 @@ fn as_ref(&self) -> &device::Device {
> >          &self.0
> >      }
> >  }
> > +
> > +impl AsMut<device::Device> for Device {
> > +    fn as_mut(&mut self) -> &mut device::Device {
> > +        // SAFETY:
> > +        // - `self.0.as_raw()` is valid by the type invariant of `device::Device`,
> > +        // - `struct device` is embedded in `struct pci_dev`, hence it is safe to give out a
> > +        //    mutable reference for `device::Device` if we have a mutable reference to the
> > +        //    corresponding `pci::Device`.
> > +        unsafe { &mut *self.0.as_raw().cast() }
> > +    }
> > +}
> 
> This makes it possible to call `mem::swap` on two devices, which I doubt
> is allowed.

Good catch, that's of course not allowed.

Obviously, this is for Device::dma_set_mask(), introduced in a subsequent patch.

Thinking about it, maybe it should be implemented by bus devices instead (e.g.
pci::Device) -- dma_set_mask() doesn't make sense for every device.

Re: [PATCH v13 3/7] rust: pci: impl AsMut<Device> for pci::Device

[Greg KH]

On Fri, Mar 07, 2025 at 01:06:20PM +0200, Abdiel Janulgue wrote:
> From: Danilo Krummrich <dakr@kernel.org>
> 
> Some device methods require mutable references, since they change the
> underlying struct device without lock protection.
> 
> Hence, make it possible to retrieve a mutable reference to a Device from
> a mutable pci::Device.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

Note, you can't forward on a patch from someone else without also
signing off on it.  Please read the DCO for what this means and why.

thanks,

greg k-h

Re: [PATCH v13 3/7] rust: pci: impl AsMut<Device> for pci::Device

[Abdiel Janulgue]

On 07/03/2025 16:18, Greg KH wrote:
> On Fri, Mar 07, 2025 at 01:06:20PM +0200, Abdiel Janulgue wrote:
>> From: Danilo Krummrich <dakr@kernel.org>
>>
>> Some device methods require mutable references, since they change the
>> underlying struct device without lock protection.
>>
>> Hence, make it possible to retrieve a mutable reference to a Device from
>> a mutable pci::Device.
>>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
> 
> Note, you can't forward on a patch from someone else without also
> signing off on it.  Please read the DCO for what this means and why.
> 
Thanks for the reminder! Will do so in next revision.

Regards,
Abdiel

Re: [PATCH v13 4/7] rust: device: add dma addressing capabilities

[Andreas Hindborg]

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

> Add functions to set the DMA mask to inform the kernel about the
> device's DMA addressing capabilities.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/helpers/dma.c     |  8 ++++++++
>  rust/helpers/helpers.c |  1 +
>  rust/kernel/device.rs  | 29 +++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+)
>  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/device.rs b/rust/kernel/device.rs
> index db2d9658ba47..f9d3d4f60ddb 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,10 +6,12 @@
>
>  use crate::{
>      bindings,
> +    error::Result,
>      str::CStr,
>      types::{ARef, Opaque},
>  };
>  use core::{fmt, ptr};
> +use kernel::prelude::*;
>
>  #[cfg(CONFIG_PRINTK)]
>  use crate::c_str;
> @@ -187,6 +189,33 @@ pub fn property_present(&self, name: &CStr) -> bool {
>          // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
>          unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
>      }
> +
> +    /// Inform the kernel about the device's DMA addressing capabilities.
> +    ///
> +    /// Set both the DMA mask and the coherent DMA mask to the same thing.
> +    /// 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.
> +    pub fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
> +        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_raw(), mask) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }

I think we can use `Error::from_errno` here (and below). As far as I can
tell, these C functions return negative on error.

> +
> +    /// Same as [`dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
> +    pub fn dma_set_mask(&mut self, mask: u64) -> Result {
> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
> +        let ret = unsafe { bindings::dma_set_mask(self.as_raw(), mask) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
>  }
>
>  // SAFETY: Instances of `Device` are always reference-counted.


Best regards,
Andreas Hindborg

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Andreas Hindborg]

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

> 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.
>
> A CoherentAllocation is wrapped in Devres which basically guarantees
> that a driver can't make a CoherentAllocation out-live driver unbind.
> This is needed, since DMA allocations potentially also result in
> programming of the IOMMU. IOMMU mappings are device resources and
> hence the device / driver lifecycle needs to be enforced.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
> +
> +    /// 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() }

From the docs of `read_volatile`:

   Just like in C, whether an operation is volatile has no bearing
   whatsoever on questions involving concurrent access from multiple
   threads. Volatile accesses behave exactly like non-atomic accesses in
   that regard. In particular, a race between a read_volatile and any
   write operation to the same location is undefined behavior.

So the safety requirement is not sufficient. Alice responded to my
question on v12:

   > Volatile reads/writes that race are OK?

   I will not give a blanket yes to that. If you read their docs, you
   will find that they claim to not allow it. But they are the correct
   choice for DMA memory, and there's no way in practice to get
   miscompilations on memory locations that are only accessed with
   volatile operations, and never have references to them created.

   In general, this will fall into the exception that we've been given
   from the Rust people. In cases such as this where the Rust language
   does not give us the operation we want, do it like you do in C. Since
   Rust uses LLVM which does not miscompile the C part of the kernel, it
   should not miscompile the Rust part either.

Since we have an exception for the safety requirements in the
documentation, we should make that clear in the safety comment at the
call site.


Best regards,
Andreas Hindborg

Re: [PATCH v13 4/7] rust: device: add dma addressing capabilities

[Abdiel Janulgue]

Hi,

On 07/03/2025 22:12, Andreas Hindborg wrote:
> "Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes:
> 
>> Add functions to set the DMA mask to inform the kernel about the
>> device's DMA addressing capabilities.
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> ---
>>   rust/helpers/dma.c     |  8 ++++++++
>>   rust/helpers/helpers.c |  1 +
>>   rust/kernel/device.rs  | 29 +++++++++++++++++++++++++++++
>>   3 files changed, 38 insertions(+)
>>   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/device.rs b/rust/kernel/device.rs
>> index db2d9658ba47..f9d3d4f60ddb 100644
>> --- a/rust/kernel/device.rs
>> +++ b/rust/kernel/device.rs
>> @@ -6,10 +6,12 @@
>>
>>   use crate::{
>>       bindings,
>> +    error::Result,
>>       str::CStr,
>>       types::{ARef, Opaque},
>>   };
>>   use core::{fmt, ptr};
>> +use kernel::prelude::*;
>>
>>   #[cfg(CONFIG_PRINTK)]
>>   use crate::c_str;
>> @@ -187,6 +189,33 @@ pub fn property_present(&self, name: &CStr) -> bool {
>>           // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
>>           unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
>>       }
>> +
>> +    /// Inform the kernel about the device's DMA addressing capabilities.
>> +    ///
>> +    /// Set both the DMA mask and the coherent DMA mask to the same thing.
>> +    /// 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.
>> +    pub fn dma_set_mask_and_coherent(&mut self, mask: u64) -> Result {
>> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
>> +        let ret = unsafe { bindings::dma_set_mask_and_coherent(self.as_raw(), mask) };
>> +        if ret != 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(())
>> +        }
>> +    }
> 
> I think we can use `Error::from_errno` here (and below). As far as I can
> tell, these C functions return negative on error.

This already uses `Error::from_errno`?

Thanks!

> 
>> +
>> +    /// Same as [`dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
>> +    pub fn dma_set_mask(&mut self, mask: u64) -> Result {
>> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
>> +        let ret = unsafe { bindings::dma_set_mask(self.as_raw(), mask) };
>> +        if ret != 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(())
>> +        }
>> +    }
>>   }
>>
>>   // SAFETY: Instances of `Device` are always reference-counted.
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 

Re: [PATCH v13 4/7] rust: device: add dma addressing capabilities

[Miguel Ojeda]

On Tue, Mar 11, 2025 at 6:45 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> This already uses `Error::from_errno`?

Maybe Andreas meant `to_result`?

Cheers,
Miguel

Re: [PATCH v13 4/7] rust: device: add dma addressing capabilities

[Andreas Hindborg]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Mar 11, 2025 at 6:45 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> This already uses `Error::from_errno`?
>
> Maybe Andreas meant `to_result`?

Yes, sorry. It includes the if/else, but it tests for ret < 0. But I
think that is fine here.


Best regards,
Andreas Hindborg

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Jason Gunthorpe]

On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:

> +        // 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(),
> +            )

This is not the correct safety statement, the device must have a driver
bound to call this function, a struct device reference is not
sufficient.

I belive Danilo was suggesting to ignore this unsafety for now, but if
so it should be documented correctly.

Also think the use of devres here is going to be very problematic for
drivers to use as I said in other emails. :(

Jason

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 02:23:53PM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:
> 
> > +        // 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(),
> > +            )
> 
> This is not the correct safety statement, the device must have a driver
> bound to call this function, a struct device reference is not
> sufficient.
> 
> I belive Danilo was suggesting to ignore this unsafety for now, but if
> so it should be documented correctly.

If just landed patches [1], which are the foundation of addressing this issue.

With the next cycle, this will be ensured by the type system.

> 
> Also think the use of devres here is going to be very problematic for
> drivers to use as I said in other emails. :(

In an earlier reply today in a different thread already gave you the link [2] of
what we landed, which, besides explaining the situation, also makes clear that
there is *no* Devres wrapper around a CoherentAllocation and why.

[1] https://lore.kernel.org/lkml/20250314160932.100165-1-dakr@kernel.org/
[2] https://github.com/Rust-for-Linux/linux/blob/rust-next/rust/kernel/dma.rs#L120

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Jason Gunthorpe]

On Fri, Mar 21, 2025 at 06:34:53PM +0100, Danilo Krummrich wrote:
> On Fri, Mar 21, 2025 at 02:23:53PM -0300, Jason Gunthorpe wrote:
> > On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:
> > 
> > > +        // 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(),
> > > +            )
> > 
> > This is not the correct safety statement, the device must have a driver
> > bound to call this function, a struct device reference is not
> > sufficient.
> > 
> > I belive Danilo was suggesting to ignore this unsafety for now, but if
> > so it should be documented correctly.
> 
> If just landed patches [1], which are the foundation of addressing this issue.

Those patches say:

 The context types can be extended as required, e.g. to limit availability  of
 certain (bus) device functions to probe().

Which is not an appropriate limitation for dma_alloc_coherent, we
expect it to be called outside probe in real drivers. Is there more to
that story?

Regardless, the safety comment should not be merged with incorrect
information. :\

> > Also think the use of devres here is going to be very problematic for
> > drivers to use as I said in other emails. :(
> 
> In an earlier reply today in a different thread already gave you the link [2] of
> what we landed, which, besides explaining the situation, also makes clear that
> there is *no* Devres wrapper around a CoherentAllocation and why.

Yes, I see, I have so much email right now it is hard find all the
different versions of everything.

Jason

Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 03:29:01PM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 21, 2025 at 06:34:53PM +0100, Danilo Krummrich wrote:
> > On Fri, Mar 21, 2025 at 02:23:53PM -0300, Jason Gunthorpe wrote:
> > > On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:
> > > 
> > > > +        // 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(),
> > > > +            )
> > > 
> > > This is not the correct safety statement, the device must have a driver
> > > bound to call this function, a struct device reference is not
> > > sufficient.
> > > 
> > > I belive Danilo was suggesting to ignore this unsafety for now, but if
> > > so it should be documented correctly.
> > 
> > If just landed patches [1], which are the foundation of addressing this issue.
> 
> Those patches say:
> 
>  The context types can be extended as required, e.g. to limit availability  of
>  certain (bus) device functions to probe().
> 
> Which is not an appropriate limitation for dma_alloc_coherent, we
> expect it to be called outside probe in real drivers. Is there more to
> that story?

Yeah, we can also use them to derive specifically typed Device instances from
other entry points of the driver where we know for sure that at this point the
device must (still) be bound to the driver.

For instance, bus callbacks, subsystem callbacks, certain (but not all) IOCTLs,
IRQ handlers, etc.

All those cases can be covered by only the type system, without additional
locks. We could even use this as an optimization to bypass Devres'
try_access() calls when holding a corresponding device instance in those places.

> 
> Regardless, the safety comment should not be merged with incorrect
> information. :\

v15 did land in rust-next, so unfortunately this was overlooked. Since you
caught, mind sending a patch improving the comment?