Add a simple dma coherent allocator rust abstraction which was based on
Andreas Hindborg's dma abstractions from the rnvme driver.

This version:
- Does not introduce the unused dma pool functionality for now.
- Represents the internal CPU buffer as a slice instead of using raw
  pointer reads and writes.
- Ensures both 32 and 64-bit DMA addressing works.
- Make use of Result error-handling instead of Some.

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/kernel/dma.rs | 153 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 2 files changed, 154 insertions(+)
 create mode 100644 rust/kernel/dma.rs

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
new file mode 100644
index 000000000000..8390b3a4e8aa
--- /dev/null
+++ b/rust/kernel/dma.rs
@@ -0,0 +1,153 @@
+// 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,
+};
+
+/// Abstraction of dma_alloc_coherent
+///
+/// # Invariants
+///
+/// For the lifetime of an instance of CoherentAllocation:
+/// 1. The cpu address pointer is valid and is accessed with an index bounded within count.
+/// 2. We hold a reference to the device.
+pub struct CoherentAllocation<T: 'static> {
+    dev: ARef<Device>,
+    dma_handle: bindings::dma_addr_t,
+    count: usize,
+    cpu_addr: &'static mut [T],
+}
+
+impl<T> 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>();
+        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,
+            // SAFETY: The raw buffer and size is valid from the checks we've made above.
+            cpu_addr: unsafe { core::slice::from_raw_parts_mut(ret as _, size) },
+        })
+    }
+
+    /// 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_addr.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_addr.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_addr.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_mut_ptr() 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_ptr() 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.as_mut_ptr() as _
+    }
+
+    /// 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
+    }
+}
+
+impl<T> 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_mut_ptr() 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

Hi Abdiel,

> On 23 Oct 2024, at 08:32, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Add a simple dma coherent allocator rust abstraction which was based on
> Andreas Hindborg's dma abstractions from the rnvme driver.
> 
> This version:
> - Does not introduce the unused dma pool functionality for now.
> - Represents the internal CPU buffer as a slice instead of using raw
>  pointer reads and writes.

This patch is not a v2, so was anybody against using a raw pointer at some time?

> - Ensures both 32 and 64-bit DMA addressing works.
> - Make use of Result error-handling instead of Some.
> 
> 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/kernel/dma.rs | 153 +++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs |   1 +
> 2 files changed, 154 insertions(+)
> create mode 100644 rust/kernel/dma.rs
> 
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> new file mode 100644
> index 000000000000..8390b3a4e8aa
> --- /dev/null
> +++ b/rust/kernel/dma.rs
> @@ -0,0 +1,153 @@
> +// 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,
> +};
> +
> +/// Abstraction of dma_alloc_coherent
> +///
> +/// # Invariants
> +///
> +/// For the lifetime of an instance of CoherentAllocation:
> +/// 1. The cpu address pointer is valid and is accessed with an index bounded within count.
> +/// 2. We hold a reference to the device.
> +pub struct CoherentAllocation<T: 'static> {
> +    dev: ARef<Device>,
> +    dma_handle: bindings::dma_addr_t,
> +    count: usize,
> +    cpu_addr: &'static mut [T],
> +}

Not sure why there’s ’static here. The lifetime of `cpu_addr` is the lifetime of the object.

This is why keeping a pointer and building the slice as needed is actually a better approach, IMHO.
That will correctly express the lifetime we want to enforce, i.e.:

```
pub fn cpu(&’a self) -> &’a mut [T]; 
```

Where ‘a is automatically filled in, of course.

Also, naming a slice as `cpu_addr` doesn’t sound very good, to be honest.

> +
> +impl<T> 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)?;

Have you considered ZSTs? What happens if someone writes down:

```
let c = CoherentAllocation<()> = …
```

This doesn’t really make sense and should be forbidden.


> +    /// # Ok(()) }
> +    /// ```
> +    pub fn alloc_coherent(
> +        dev: &Device,
> +        count: usize,
> +        flags: kernel::alloc::Flags,
> +    ) -> Result<CoherentAllocation<T>> {
> +        let t_size = core::mem::size_of::<T>();
> +        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 assume that ZSTs will simply return ENOMEM as per above, but that’s not quite
right either. The API should prevent this per-design instead of returning `Error`.

> +
> +        Ok(Self {
> +            dev: dev.into(),
> +            dma_handle,
> +            count,
> +            // SAFETY: The raw buffer and size is valid from the checks we've made above.
> +            cpu_addr: unsafe { core::slice::from_raw_parts_mut(ret as _, size) },
> +        })
> +    }
> +
> +    /// 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_addr.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_addr.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_addr.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_mut_ptr() 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_ptr() 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.as_mut_ptr() as _
> +    }
> +
> +    /// 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
> +    }
> +}
> +
> +impl<T> 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_mut_ptr() 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
> 
> 

Everything else looks good to me!

— Daniel

Hi,

On 28/10/2024 17:38, Daniel Almeida wrote:
> 
> This patch is not a v2, so was anybody against using a raw pointer at some time?

The original idea behind this was to improve a little bit from raw 
pointer manipulation when indexing the data. But yeah, for the proper v2 
I now reintroduced the raw pointer but dynamically created slice from it 
as you suggested below. :)

> 
> Not sure why there’s ’static here. The lifetime of `cpu_addr` is the lifetime of the object.
> 
> This is why keeping a pointer and building the slice as needed is actually a better approach, IMHO.
> That will correctly express the lifetime we want to enforce, i.e.:
> 
> ```
> pub fn cpu(&’a self) -> &’a mut [T];
> ```
> 
> Where ‘a is automatically filled in, of course.
> 

>> +    /// # 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)?;
> 
> Have you considered ZSTs? What happens if someone writes down:
> 
> ```
> let c = CoherentAllocation<()> = …
> ```
> 
> This doesn’t really make sense and should be forbidden.

Would restricting it to a primitive type make sense?

e.g:

pub struct CoherentAllocation<T: BitAnd + BitOr>

I'm not sure, but are there other ways to enforce that restriction?

> 
>> 2.43.0
>>
>>
> 
> Everything else looks good to me!

Thanks,
Abdiel

> 
> — Daniel
> 

On 23/10/2024 14:32, Abdiel Janulgue wrote:
> Add a simple dma coherent allocator rust abstraction which was based on
> Andreas Hindborg's dma abstractions from the rnvme driver.
> 
> This version:
> - Does not introduce the unused dma pool functionality for now.
> - Represents the internal CPU buffer as a slice instead of using raw
>    pointer reads and writes.
> - Ensures both 32 and 64-bit DMA addressing works.
> - Make use of Result error-handling instead of Some.


Ping! Does this approach make sense? :)

> 
> 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/kernel/dma.rs | 153 +++++++++++++++++++++++++++++++++++++++++++++
>   rust/kernel/lib.rs |   1 +
>   2 files changed, 154 insertions(+)
>   create mode 100644 rust/kernel/dma.rs
>