[PATCH v4 0/2] Add dma coherent allocator abstraction

[PATCH v4 0/2] Add dma coherent allocator abstraction

[Abdiel Janulgue]

This series adds support for the dma coherent allocator.

Changs 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.

Abdiel Janulgue (2):
  rust: error: Add EOVERFLOW
  rust: add dma coherent allocator abstraction.

 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/dma.rs              | 169 ++++++++++++++++++++++++++++++++
 rust/kernel/error.rs            |   1 +
 rust/kernel/lib.rs              |   1 +
 4 files changed, 172 insertions(+)
 create mode 100644 rust/kernel/dma.rs


base-commit: ae7851c29747fa3765ecb722fe722117a346f988
-- 
2.43.0

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

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 7cd3bbab52f2..92bfdaaedb02 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -63,6 +63,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 v4 2/2] 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.

Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/dma.rs              | 169 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 3 files changed, 171 insertions(+)
 create mode 100644 rust/kernel/dma.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index a80783fcbe04..3ff2abbfaef6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/dma-mapping.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/firmware.h>
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
new file mode 100644
index 000000000000..3311fc99e5df
--- /dev/null
+++ b/rust/kernel/dma.rs
@@ -0,0 +1,169 @@
+// 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,
+    device::Device,
+    error::code::*,
+    error::Result,
+    types::ARef,
+    transmute::{AsBytes, FromBytes},
+};
+
+/// Abstraction of dma_alloc_coherent
+///
+/// # Invariants
+///
+/// For the lifetime of an instance of CoherentAllocation, the cpu address is a valid pointer
+/// to an allocated region of consistent memory and we hold a reference to the device.
+pub struct CoherentAllocation<T: AsBytes + FromBytes> {
+    dev: ARef<Device>,
+    dma_handle: bindings::dma_addr_t,
+    count: usize,
+    cpu_addr: *mut T,
+}
+
+impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
+    /// Allocates a region of `size_of::<T> * count` of consistent memory.
+    ///
+    /// Returns a CoherentAllocation object which 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.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::device::Device;
+    /// use kernel::dma::CoherentAllocation;
+    ///
+    /// # fn dox(dev: &Device) -> Result<()> {
+    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_coherent(dev, 4, GFP_KERNEL)?;
+    /// # Ok(()) }
+    /// ```
+    pub fn alloc_coherent(
+        dev: &Device,
+        count: usize,
+        flags: kernel::alloc::Flags,
+    ) -> Result<CoherentAllocation<T>> {
+        let t_size = core::mem::size_of::<T>();
+        if t_size == 0 {
+            return Err(EINVAL)
+        }
+
+        let size = count.checked_mul(t_size).ok_or(EOVERFLOW)?;
+        let mut dma_handle = 0;
+        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
+        // We ensure that we catch the failure on this function and throw an ENOMEM
+        let ret = unsafe {
+            bindings::dma_alloc_attrs(
+                dev.as_raw(),
+                size,
+                &mut dma_handle, flags.as_raw(),
+                0,
+            )
+        };
+        if ret.is_null() {
+            return Err(ENOMEM)
+        }
+
+        Ok(Self {
+            dev: dev.into(),
+            dma_handle,
+            count,
+            cpu_addr: ret as _,
+        })
+    }
+
+    /// Reads a value on a location specified by index.
+    pub fn read(&self, index: usize) -> Result<T>
+    where
+        T: Copy
+    {
+        if let Some(val) = self.cpu_buf().get(index) {
+            Ok(*val)
+        } else {
+            Err(EINVAL)
+        }
+    }
+
+    /// Write a value on the memory location specified by index.
+    pub fn write(&mut self, index: usize, value: &T) -> Result
+    where
+        T: Copy,
+    {
+        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
+            *elem = *value;
+            Ok(())
+        } else {
+            Err(EINVAL)
+        }
+    }
+
+    /// Performs a read and then a write of a value on a location specified by index.
+    pub fn read_write(&mut self, index: usize, value: &T) -> Result<T>
+    where
+        T: Copy,
+    {
+        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
+            let val = *elem;
+            *elem = *value;
+            Ok(val)
+        } else {
+            Err(EINVAL)
+        }
+    }
+
+    /// Returns the base address to the allocated region and the dma handle.
+    /// Caller takes ownership of returned resources.
+    pub fn into_parts(self) -> (usize, bindings::dma_addr_t) {
+        let ret = (self.cpu_addr as _, self.dma_handle);
+        core::mem::forget(self);
+        ret
+    }
+
+    /// 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 as _
+    }
+
+    /// 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
+    }
+
+    fn cpu_buf(&self) -> &[T]
+    {
+        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
+        // is valid for reads for `self.count * size_of::<T>` bytes.
+        unsafe { core::slice::from_raw_parts(self.cpu_addr, self.count) }
+    }
+
+    fn cpu_buf_mut(&mut self) -> &mut [T]
+    {
+        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
+        // is valid for reads for `self.count * size_of::<T>` bytes.
+        unsafe { core::slice::from_raw_parts_mut(self.cpu_addr, self.count) }
+    }
+}
+
+impl<T: AsBytes + FromBytes> Drop for CoherentAllocation<T> {
+    fn drop(&mut self) {
+        let size = self.count * core::mem::size_of::<T>();
+        // SAFETY: the device, cpu address, and the dma handle is 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, 0) }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b62451f64f6e..b713c92eb1ef 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -32,6 +32,7 @@
 pub mod block;
 mod build_assert;
 pub mod device;
+pub mod dma;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
-- 
2.43.0

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

[Miguel Ojeda]

Hi Abdiel,

Some quick comments on the docs I noticed.

On Tue, Nov 5, 2024 at 11:47 AM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> +/// Abstraction of dma_alloc_coherent
> +///
> +/// # Invariants
> +///
> +/// For the lifetime of an instance of CoherentAllocation, the cpu address is a valid pointer
> +/// to an allocated region of consistent memory and we hold a reference to the device.

Please use Markdown formatting everywhere (including documentation
like here, as well as comments). For documentation, also please use
intra-doc links where applicable (i.e. the square brackets, e.g.
[`Foo`]).

Also, ideally, please define/explain the abstraction, rather than just
saying it is an abstraction of something else.

Finally, if you want to have a link to a C header to refer to it, you
can use a `srctree/...` link (please refer to others to see how it is
usually done).

> +    /// # fn dox(dev: &Device) -> Result<()> {

Can we avoid this, i.e. use the `# Ok::<(), ...>(())` approach instead?

I see we got another `dox()` function in `page.rs`, which I guess is
where you got it from, but I think those should be cleaned up too,
unless I am missing something.

> +    /// Returns the base address to the allocated region and the dma handle.
> +    /// Caller takes ownership of returned resources.

Note that this will make it a single paragraph -- you may want to have
an empty line here.

Thanks!

Cheers,
Miguel

Re: [PATCH v4 0/2] Add dma coherent allocator abstraction

[Miguel Ojeda]

On Tue, Nov 5, 2024 at 11:47 AM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> This series adds support for the dma coherent allocator.

Shouldn't this Cc the DMA mapping helpers subsystem/maintainers? (also
please Cc the rest of the Rust subsystem maintainers/reviewers too).

Thanks!

Cheers,
Miguel

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

[Abdiel Janulgue]

On 05/11/2024 13:23, Miguel Ojeda wrote:
> Hi Abdiel,
> 
> Some quick comments on the docs I noticed.
> 
> On Tue, Nov 5, 2024 at 11:47 AM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> +/// Abstraction of dma_alloc_coherent
>> +///
>> +/// # Invariants
>> +///
>> +/// For the lifetime of an instance of CoherentAllocation, the cpu address is a valid pointer
>> +/// to an allocated region of consistent memory and we hold a reference to the device.
> 
> Please use Markdown formatting everywhere (including documentation
> like here, as well as comments). For documentation, also please use
> intra-doc links where applicable (i.e. the square brackets, e.g.
> [`Foo`]).
> 
> Also, ideally, please define/explain the abstraction, rather than just
> saying it is an abstraction of something else.
> 
> Finally, if you want to have a link to a C header to refer to it, you
> can use a `srctree/...` link (please refer to others to see how it is
> usually done).
> 
>> +    /// # fn dox(dev: &Device) -> Result<()> {
> 
> Can we avoid this, i.e. use the `# Ok::<(), ...>(())` approach instead?
> 
> I see we got another `dox()` function in `page.rs`, which I guess is
> where you got it from, 

Yep, just learning it from existing code. But will update it.

Regards,
Abdiel

Re: [PATCH v4 0/2] Add dma coherent allocator abstraction

[Abdiel Janulgue]

On 05/11/2024 13:23, Miguel Ojeda wrote:
> On Tue, Nov 5, 2024 at 11:47 AM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> This series adds support for the dma coherent allocator.
> 
> Shouldn't this Cc the DMA mapping helpers subsystem/maintainers? (also
> please Cc the rest of the Rust subsystem maintainers/reviewers too).
> 

Thanks for the reminder with this guideline. Will do so in next update.

/Abdiel

> Thanks!
> 
> Cheers,
> Miguel
> 

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

[Dirk Behme]

Hi Miguel,

On 05.11.2024 12:23, Miguel Ojeda wrote:
> Hi Abdiel,
> 
> Some quick comments on the docs I noticed.
....
>> +    /// # fn dox(dev: &Device) -> Result<()> {
> 
> Can we avoid this, i.e. use the `# Ok::<(), ...>(())` approach instead?
> 
> I see we got another `dox()` function in `page.rs`, which I guess is
> where you got it from, but I think those should be cleaned up too,
> unless I am missing something.

Are we talking about something like [1]?

While reading this I was looking at something similar to get rid of some 
.unwrap(). Would something like [2] be acceptable as well?

Thanks

Dirk


[1]

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdac6c375fe46..5f0347f9cd548 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -57,9 +57,8 @@ impl Page {
      /// ```
      /// use kernel::page::Page;
      ///
-    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
      /// let page = Page::alloc_page(GFP_KERNEL)?;
-    /// # Ok(()) }
+    /// # Ok::<(), Error>(())
      /// ```
      ///
      /// Allocate memory for a page and zero its contents.
@@ -67,9 +66,8 @@ impl Page {
      /// ```
      /// use kernel::page::Page;
      ///
-    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
      /// let page = Page::alloc_page(GFP_KERNEL | __GFP_ZERO)?;
-    /// # Ok(()) }
+    /// # Ok::<(), Error>(())
      /// ```
      pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
          // SAFETY: Depending on the value of `gfp_flags`, this call 
may sleep. Other than that, it


[2]

index c9919ba0b6836..ff249c98b53c3 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -390,7 +390,8 @@ macro_rules! stack_try_pin_init {
  ///     },
  /// });
  /// # initializer }
-/// # KBox::pin_init(demo(), GFP_KERNEL).unwrap();
+/// # KBox::pin_init(demo(), GFP_KERNEL)?;
+/// # Ok::<(), Error>(())
  /// ```
  ///
  /// Arbitrary Rust expressions can be used to set the value of a variable.
@@ -1076,8 +1077,9 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, 
E> {
  /// ```rust
  /// use kernel::{alloc::KBox, error::Error, init::init_array_from_fn};
  /// let array: KBox<[usize; 1_000]> =
-///     KBox::init::<Error>(init_array_from_fn(|i| i), 
GFP_KERNEL).unwrap();
+///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL)?;
  /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
  /// ```
  pub fn init_array_from_fn<I, const N: usize, T, E>(
      mut make_init: impl FnMut(usize) -> I,
@@ -1120,8 +1122,9 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
  /// ```rust
  /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, 
new_mutex};
  /// let array: Arc<[Mutex<usize>; 1_000]> =
-///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), 
GFP_KERNEL).unwrap();
+///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), 
GFP_KERNEL)?;
  /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
  /// ```
  pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
      mut make_init: impl FnMut(usize) -> I,

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

[Miguel Ojeda]

On Wed, Nov 6, 2024 at 11:21 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> Are we talking about something like [1]?

Yeah.

> While reading this I was looking at something similar to get rid of some
> .unwrap(). Would something like [2] be acceptable as well?

Yeah -- sometimes the `unwrap()` in examples may be used for
simplicity, but in "real code" we should use `?`.

Personally, I think that we should strive to give "real code" in
examples too, especially since they are part of the test suite, so
ideally we don't panic.

Thanks!

Cheers,
Miguel

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

[Daniel Almeida]

Hi Abdiel,


> On 5 Nov 2024, at 07:46, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Add a simple dma coherent allocator rust abstraction. Based on
> Andreas Hindborg's dma abstractions from the rnvme driver.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
> rust/bindings/bindings_helper.h |   1 +
> rust/kernel/dma.rs              | 169 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs              |   1 +
> 3 files changed, 171 insertions(+)
> create mode 100644 rust/kernel/dma.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a80783fcbe04..3ff2abbfaef6 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
> #include <linux/blk-mq.h>
> #include <linux/blk_types.h>
> #include <linux/blkdev.h>
> +#include <linux/dma-mapping.h>
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> #include <linux/firmware.h>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> new file mode 100644
> index 000000000000..3311fc99e5df
> --- /dev/null
> +++ b/rust/kernel/dma.rs
> @@ -0,0 +1,169 @@
> +// 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,
> +    device::Device,
> +    error::code::*,
> +    error::Result,
> +    types::ARef,
> +    transmute::{AsBytes, FromBytes},
> +};
> +
> +/// Abstraction of dma_alloc_coherent
> +///
> +/// # Invariants
> +///
> +/// For the lifetime of an instance of CoherentAllocation, the cpu address is a valid pointer
> +/// to an allocated region of consistent memory and we hold a reference to the device.
> +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> +    dev: ARef<Device>,
> +    dma_handle: bindings::dma_addr_t,
> +    count: usize,
> +    cpu_addr: *mut T,
> +}
> +
> +impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
> +    /// Allocates a region of `size_of::<T> * count` of consistent memory.
> +    ///
> +    /// Returns a CoherentAllocation object which 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.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::device::Device;
> +    /// use kernel::dma::CoherentAllocation;
> +    ///
> +    /// # fn dox(dev: &Device) -> Result<()> {
> +    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_coherent(dev, 4, GFP_KERNEL)?;
> +    /// # Ok(()) }
> +    /// ```
> +    pub fn alloc_coherent(
> +        dev: &Device,
> +        count: usize,
> +        flags: kernel::alloc::Flags,
> +    ) -> Result<CoherentAllocation<T>> {
> +        let t_size = core::mem::size_of::<T>();
> +        if t_size == 0 {
> +            return Err(EINVAL)
> +        }
> +
> +        let size = count.checked_mul(t_size).ok_or(EOVERFLOW)?;
> +        let mut dma_handle = 0;
> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
> +        // We ensure that we catch the failure on this function and throw an ENOMEM
> +        let ret = unsafe {
> +            bindings::dma_alloc_attrs(
> +                dev.as_raw(),
> +                size,
> +                &mut dma_handle, flags.as_raw(),
> +                0,
> +            )
> +        };
> +        if ret.is_null() {
> +            return Err(ENOMEM)
> +        }
> +
> +        Ok(Self {
> +            dev: dev.into(),
> +            dma_handle,
> +            count,
> +            cpu_addr: ret as _,
> +        })
> +    }
> +
> +    /// Reads a value on a location specified by index.
> +    pub fn read(&self, index: usize) -> Result<T>
> +    where
> +        T: Copy
> +    {
> +        if let Some(val) = self.cpu_buf().get(index) {
> +            Ok(*val)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +
> +    /// Write a value on the memory location specified by index.
> +    pub fn write(&mut self, index: usize, value: &T) -> Result
> +    where
> +        T: Copy,
> +    {
> +        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
> +            *elem = *value;
> +            Ok(())
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }


Calling `get/get_mut()` for each T seems a bit of too much overhead.

If T is u32, for example, I expect a driver to want to read or write
a significant number of them at once. This should be common
if you want to read or write raw bytes.

You should probably make the CPU-addressable region available
to users after validating if the range asked is within bounds.

Luckily, cpu_buf() will already tie the lifetime of the slice to &self,
so they cannot touch that memory if the mapping is gone.

The fact that reads/writes will copy the memory is also a bit problematic
given that you can offer access to the underlying buffer without copying
anything. Also, the bound on `FromBytes/AsBytes` is, for now, a de facto
bound on `Copy`, since it is only implemented for types which themselves
implement Copy, i.e.:

```
// SAFETY: All bit patterns are acceptable values of the types below.
unsafe impl FromBytes for u8 {}
unsafe impl FromBytes for u16 {}
unsafe impl FromBytes for u32 {}
unsafe impl FromBytes for u64 {}
unsafe impl FromBytes for usize {}
unsafe impl FromBytes for i8 {}
unsafe impl FromBytes for i16 {}
unsafe impl FromBytes for i32 {}
unsafe impl FromBytes for i64 {}
unsafe impl FromBytes for isize {}
// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
// patterns are also acceptable for arrays of that type.
unsafe impl<T: FromBytes> FromBytes for [T] {}
unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {}
```

Also, I think FromBytes is a bit restrictive here. In a lot of places, drivers
expect to be able to cast the mapping to a given struct whose definition
was sourced from some technical document or vendor code somewhere.

For example, here is some code for the H.264 decoder in the `rkvdec`
driver:

```
	struct rkvdec_h264_priv_tbl *priv_tbl;

<cut>

	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
```

Where `struct rkvdec_h264_priv_tbl` is:

```
struct rkvdec_h264_scaling_list {
	u8 scaling_list_4x4[6][16];
	u8 scaling_list_8x8[6][64];
	u8 padding[128];
};

struct rkvdec_sps_pps_packet {
	u32 info[8];
};

/* Data structure describing auxiliary buffer format. */
struct rkvdec_h264_priv_tbl {
	s8 cabac_table[4][464][2];
	struct rkvdec_h264_scaling_list scaling_list;
	u32 rps[RKV_RPS_SIZE];
	struct rkvdec_sps_pps_packet param_set[256];
	u8 err_info[RKV_ERROR_INFO_SIZE];
};
```	
 
IMHO we should somehow support that, assuming that all members
implement `FromBytes/AsBytes`, and that the structure is #[repr(C)].

Not sure how right now, but we should.

The alternative would be extremely tedious and itself error-prone.
 
> +
> +    /// Performs a read and then a write of a value on a location specified by index.
> +    pub fn read_write(&mut self, index: usize, value: &T) -> Result<T>
> +    where
> +        T: Copy,
> +    {
> +        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
> +            let val = *elem;
> +            *elem = *value;
> +            Ok(val)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +
> +    /// Returns the base address to the allocated region and the dma handle.
> +    /// Caller takes ownership of returned resources.
> +    pub fn into_parts(self) -> (usize, bindings::dma_addr_t) {
> +        let ret = (self.cpu_addr as _, self.dma_handle);
> +        core::mem::forget(self);
> +        ret
> +    }
> +
> +    /// 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 as _
> +    }
> +
> +    /// 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
> +    }
> +
> +    fn cpu_buf(&self) -> &[T]
> +    {
> +        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> +        // is valid for reads for `self.count * size_of::<T>` bytes.
> +        unsafe { core::slice::from_raw_parts(self.cpu_addr, self.count) }
> +    }
> +
> +    fn cpu_buf_mut(&mut self) -> &mut [T]
> +    {
> +        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> +        // is valid for reads for `self.count * size_of::<T>` bytes.
> +        unsafe { core::slice::from_raw_parts_mut(self.cpu_addr, self.count) }
> +    }
> +}
> +
> +impl<T: AsBytes + FromBytes> Drop for CoherentAllocation<T> {
> +    fn drop(&mut self) {
> +        let size = self.count * core::mem::size_of::<T>();
> +        // SAFETY: the device, cpu address, and the dma handle is 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, 0) }
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index b62451f64f6e..b713c92eb1ef 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -32,6 +32,7 @@
> pub mod block;
> mod build_assert;
> pub mod device;
> +pub mod dma;
> pub mod error;
> #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> pub mod firmware;
> -- 
> 2.43.0
> 
> 


— Daniel

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

[Abdiel Janulgue]

Hi Daniel,

Thanks for the feedback!

On 06/11/2024 18:40, Daniel Almeida wrote:
> Calling `get/get_mut()` for each T seems a bit of too much overhead.
> 
> If T is u32, for example, I expect a driver to want to read or write
> a significant number of them at once. This should be common
> if you want to read or write raw bytes.
> 
> You should probably make the CPU-addressable region available
> to users after validating if the range asked is within bounds.
> 
> Luckily, cpu_buf() will already tie the lifetime of the slice to &self,
> so they cannot touch that memory if the mapping is gone.

You mean the read/write helpers could be removed and we expose the 
cpu_buf() helper instead that takes a valid range, returns a checked 
slice and let the user have the flexibility in implementing read/writes 
themselves. Is this what you had in mind?

> 
> The fact that reads/writes will copy the memory is also a bit problematic
> given that you can offer access to the underlying buffer without copying
> anything. Also, the bound on `FromBytes/AsBytes` is, for now, a de facto
> bound on `Copy`, since it is only implemented for types which themselves
> implement Copy, i.e.:
> 
> ```
> // SAFETY: All bit patterns are acceptable values of the types below.
> unsafe impl FromBytes for u8 {}
> unsafe impl FromBytes for u16 {}
> unsafe impl FromBytes for u32 {}
> unsafe impl FromBytes for u64 {}
> unsafe impl FromBytes for usize {}
> unsafe impl FromBytes for i8 {}
> unsafe impl FromBytes for i16 {}
> unsafe impl FromBytes for i32 {}
> unsafe impl FromBytes for i64 {}
> unsafe impl FromBytes for isize {}
> // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
> // patterns are also acceptable for arrays of that type.
> unsafe impl<T: FromBytes> FromBytes for [T] {}
> unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {}
> ```
> 
> Also, I think FromBytes is a bit restrictive here. In a lot of places, drivers
> expect to be able to cast the mapping to a given struct whose definition
> was sourced from some technical document or vendor code somewhere.
> 
> For example, here is some code for the H.264 decoder in the `rkvdec`
> driver:
> 
> ```
> 	struct rkvdec_h264_priv_tbl *priv_tbl;
> 
> <cut>
> 
> 	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
> 				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
> ```

We could just remove the FromBytes/AsBytes bound and let
the constructor handle the ZST restriction there (like in the current 
submission)? If that is the case, there would be no need to wrestle with 
this issue then?

/Abdiel

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

[Daniel Almeida]

Hi Abdiel,

> On 7 Nov 2024, at 10:01, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Hi Daniel,
> 
> Thanks for the feedback!
> 
> On 06/11/2024 18:40, Daniel Almeida wrote:
>> Calling `get/get_mut()` for each T seems a bit of too much overhead.
>> If T is u32, for example, I expect a driver to want to read or write
>> a significant number of them at once. This should be common
>> if you want to read or write raw bytes.
>> You should probably make the CPU-addressable region available
>> to users after validating if the range asked is within bounds.
>> Luckily, cpu_buf() will already tie the lifetime of the slice to &self,
>> so they cannot touch that memory if the mapping is gone.
> 
> You mean the read/write helpers could be removed and we expose the cpu_buf() helper instead that takes a valid range, returns a checked slice and let the user have the flexibility in implementing read/writes themselves. Is this what you had in mind?

Yes, that’s what I have in mind.

> 
>> The fact that reads/writes will copy the memory is also a bit problematic
>> given that you can offer access to the underlying buffer without copying
>> anything. Also, the bound on `FromBytes/AsBytes` is, for now, a de facto
>> bound on `Copy`, since it is only implemented for types which themselves
>> implement Copy, i.e.:
>> ```
>> // SAFETY: All bit patterns are acceptable values of the types below.
>> unsafe impl FromBytes for u8 {}
>> unsafe impl FromBytes for u16 {}
>> unsafe impl FromBytes for u32 {}
>> unsafe impl FromBytes for u64 {}
>> unsafe impl FromBytes for usize {}
>> unsafe impl FromBytes for i8 {}
>> unsafe impl FromBytes for i16 {}
>> unsafe impl FromBytes for i32 {}
>> unsafe impl FromBytes for i64 {}
>> unsafe impl FromBytes for isize {}
>> // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
>> // patterns are also acceptable for arrays of that type.
>> unsafe impl<T: FromBytes> FromBytes for [T] {}
>> unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {}
>> ```
>> Also, I think FromBytes is a bit restrictive here. In a lot of places, drivers
>> expect to be able to cast the mapping to a given struct whose definition
>> was sourced from some technical document or vendor code somewhere.
>> For example, here is some code for the H.264 decoder in the `rkvdec`
>> driver:
>> ```
>> struct rkvdec_h264_priv_tbl *priv_tbl;
>> <cut>
>> priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>>      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>> ```
> 
> We could just remove the FromBytes/AsBytes bound and let
> the constructor handle the ZST restriction there (like in the current submission)? If that is the case, there would be no need to wrestle with this issue then?
> 
> /Abdiel
> 

Alice pointed out to me later that the FromBytes/AsBytes traits can be implemented
by driver-defined types. I had missed that.

Given this, the FromBytes/AsBytes bound seems fine. Drivers would just need to
implement the (unsafe) traits for their types and it would be up to them to make
sure that the safety requirements hold.

Given the above, I am fine with FromBytes/AsBytes, you don’t need to change
that IMHO.

With read() and write() gone, so is the bound on Copy, which also satisfies
my other comment.

— Daniel.

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

[Abdiel Janulgue]

On 10/11/2024 01:42, Daniel Almeida wrote:
>> You mean the read/write helpers could be removed and we expose the cpu_buf() helper instead that takes a valid range, returns a checked slice and let the user have the flexibility in implementing read/writes themselves. Is this what you had in mind?
> 
> Yes, that’s what I have in mind.
> 
> 
> Alice pointed out to me later that the FromBytes/AsBytes traits can be implemented
> by driver-defined types. I had missed that.
> 
> Given this, the FromBytes/AsBytes bound seems fine. Drivers would just need to
> implement the (unsafe) traits for their types and it would be up to them to make
> sure that the safety requirements hold.
> 
> Given the above, I am fine with FromBytes/AsBytes, you don’t need to change
> that IMHO.
> 
> With read() and write() gone, so is the bound on Copy, which also satisfies
> my other comment.
> 

Thanks! Will incorporate this feedback on the next re-spin.

/abdiel

Re: [PATCH v4 2/2] 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.
>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/dma.rs              | 169 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  3 files changed, 171 insertions(+)
>  create mode 100644 rust/kernel/dma.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a80783fcbe04..3ff2abbfaef6 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
>  #include <linux/firmware.h>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> new file mode 100644
> index 000000000000..3311fc99e5df
> --- /dev/null
> +++ b/rust/kernel/dma.rs
> @@ -0,0 +1,169 @@
> +// 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,
> +    device::Device,
> +    error::code::*,
> +    error::Result,
> +    types::ARef,
> +    transmute::{AsBytes, FromBytes},
> +};
> +
> +/// Abstraction of dma_alloc_coherent
> +///
> +/// # Invariants
> +///
> +/// For the lifetime of an instance of CoherentAllocation, the cpu address is a valid pointer
> +/// to an allocated region of consistent memory and we hold a reference to the device.
> +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> +    dev: ARef<Device>,
> +    dma_handle: bindings::dma_addr_t,
> +    count: usize,
> +    cpu_addr: *mut T,
> +}
> +
> +impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
> +    /// Allocates a region of `size_of::<T> * count` of consistent memory.
> +    ///
> +    /// Returns a CoherentAllocation object which 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.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::device::Device;
> +    /// use kernel::dma::CoherentAllocation;
> +    ///
> +    /// # fn dox(dev: &Device) -> Result<()> {
> +    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_coherent(dev, 4, GFP_KERNEL)?;
> +    /// # Ok(()) }
> +    /// ```
> +    pub fn alloc_coherent(
> +        dev: &Device,
> +        count: usize,
> +        flags: kernel::alloc::Flags,
> +    ) -> Result<CoherentAllocation<T>> {
> +        let t_size = core::mem::size_of::<T>();
> +        if t_size == 0 {
> +            return Err(EINVAL)
> +        }

Can this be a `build_assert!()`?

> +
> +        let size = count.checked_mul(t_size).ok_or(EOVERFLOW)?;
> +        let mut dma_handle = 0;
> +        // SAFETY: device pointer is guaranteed as valid by invariant on `Device`.
> +        // We ensure that we catch the failure on this function and throw an ENOMEM
> +        let ret = unsafe {
> +            bindings::dma_alloc_attrs(
> +                dev.as_raw(),
> +                size,
> +                &mut dma_handle, flags.as_raw(),
> +                0,
> +            )
> +        };
> +        if ret.is_null() {
> +            return Err(ENOMEM)
> +        }
> +

I think we need an `// Invariant:` block here. See [1] for an example.

> +        Ok(Self {
> +            dev: dev.into(),
> +            dma_handle,
> +            count,
> +            cpu_addr: ret as _,

Please use an explicit cast here.

> +        })
> +    }
> +
> +    /// Reads a value on a location specified by index.
> +    pub fn read(&self, index: usize) -> Result<T>
> +    where
> +        T: Copy
> +    {
> +        if let Some(val) = self.cpu_buf().get(index) {
> +            Ok(*val)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +
> +    /// Write a value on the memory location specified by index.
> +    pub fn write(&mut self, index: usize, value: &T) -> Result
> +    where
> +        T: Copy,
> +    {
> +        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
> +            *elem = *value;
> +            Ok(())
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +
> +    /// Performs a read and then a write of a value on a location specified by index.
> +    pub fn read_write(&mut self, index: usize, value: &T) -> Result<T>
> +    where
> +        T: Copy,
> +    {
> +        if let Some(elem) = self.cpu_buf_mut().get_mut(index) {
> +            let val = *elem;
> +            *elem = *value;
> +            Ok(val)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +
> +    /// Returns the base address to the allocated region and the dma handle.
> +    /// Caller takes ownership of returned resources.
> +    pub fn into_parts(self) -> (usize, bindings::dma_addr_t) {
> +        let ret = (self.cpu_addr as _, self.dma_handle);
> +        core::mem::forget(self);
> +        ret
> +    }
> +
> +    /// 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 as _

Please use an explicit cast (ptr::cast, ptr::cast_mut, ptr::cast_const).

> +    }
> +
> +    /// 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
> +    }
> +
> +    fn cpu_buf(&self) -> &[T]

I think someone suggested elsewhere to make the slice publicly
available. Is there any reason not to make this and the `mut` version
public?

> +    {
> +        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> +        // is valid for reads for `self.count * size_of::<T>` bytes.
> +        unsafe { core::slice::from_raw_parts(self.cpu_addr, self.count) }
> +    }
> +
> +    fn cpu_buf_mut(&mut self) -> &mut [T]
> +    {
> +        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> +        // is valid for reads for `self.count * size_of::<T>` bytes.

But you are handing out an exclusive reference, so valid for reads is
not enough.


Best regards,
Andreas Hindborg



[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/sync/arc.rs?h=v6.12-rc7#n200

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

[Abdiel Janulgue]

On 11/11/2024 14:55, Andreas Hindborg wrote:

> 
> I think someone suggested elsewhere to make the slice publicly
> available. Is there any reason not to make this and the `mut` version
> public?

Yes, this is in the pipeline.

Regards,
Abdiel