[PATCH 0/2] Add dma coherent allocator abstraction

[PATCH 0/2] Add dma coherent allocator abstraction

[Abdiel Janulgue]

This series adds support for the dma coherent allocator. This is based
on code developed by Andreas Hindborg for the rnvme driver. We adapted
this for basic use in the Nova driver to access the GSP via DMA [0].

[0] https://gitlab.freedesktop.org/abj/nova-drm

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

 rust/kernel/dma.rs   | 153 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/error.rs |   1 +
 rust/kernel/lib.rs   |   1 +
 3 files changed, 155 insertions(+)
 create mode 100644 rust/kernel/dma.rs


base-commit: 15541c9263ce34ff95a06bc68f45d9bc5c990bcd
-- 
2.43.0

[PATCH 1/2] rust: error: Add EOVERFLOW

[Abdiel Janulgue]

Trivial addition for missing EOVERFLOW error.

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

Re: [PATCH 1/2] rust: error: Add EOVERFLOW

[Alice Ryhl]

On Wed, Oct 23, 2024 at 1:34 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Trivial addition for missing EOVERFLOW error.
>
> 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.");

The commit message should explain why you're adding it. What will you
use it for?

Alice

Re: [PATCH 1/2] rust: error: Add EOVERFLOW

[Abdiel Janulgue]

On 23/10/2024 14:39, Alice Ryhl wrote:
> On Wed, Oct 23, 2024 at 1:34 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> Trivial addition for missing EOVERFLOW error.
>>
>> 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.");
> 
> The commit message should explain why you're adding it. What will you
> use it for?

Yup I forgot to include the intent of the change, which is needed in the 
next patch. But will fix in next update.

/Abdiel

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

[Abdiel Janulgue]

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

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

[Abdiel Janulgue]

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
> 

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

[Daniel Almeida]

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

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

[Abdiel Janulgue]

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
> 

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

[Andreas Hindborg]

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

> This series adds support for the dma coherent allocator. This is based
> on code developed by Andreas Hindborg for the rnvme driver.

I was not the original author, I just rebased the patches for a few years. I
believe Wedson was the original author. I also did not add my signed-off-by to
this.


Best regards,
Andreas Hindborg

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

[Abdiel Janulgue]

On 11/11/2024 14:14, Andreas Hindborg wrote:
> "Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes:
> 
>> This series adds support for the dma coherent allocator. This is based
>> on code developed by Andreas Hindborg for the rnvme driver.
> 
> I was not the original author, I just rebased the patches for a few years. I
> believe Wedson was the original author. I also did not add my signed-off-by to
> this.
> 

My apologies for the mix-up, but thanks for the clarification. Will fix 
this in next iteration.

Kind regards,
Abdiel