[PATCH v11 0/3] Add dma coherent allocator abstraction

[PATCH v11 0/3] Add dma coherent allocator abstraction

[Abdiel Janulgue]

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).
- Link to v10: https://lore.kernel.org/all/20250121191432.1178734-1-abdiel.janulgue@gmail.com/

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.

Abdiel Janulgue (3):
  rust: error: Add EOVERFLOW
  rust: add dma coherent allocator abstraction.
  MAINTAINERS: add entry for Rust dma mapping helpers device driver API

 MAINTAINERS                     |  10 ++
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/dma.rs              | 282 ++++++++++++++++++++++++++++++++
 rust/kernel/error.rs            |   1 +
 rust/kernel/lib.rs              |   1 +
 5 files changed, 295 insertions(+)
 create mode 100644 rust/kernel/dma.rs


base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
-- 
2.43.0

[PATCH v11 1/3] 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>
Reviewed-by: Alice Ryhl <aliceryhl@google.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 v11 2/3] 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.

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

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 5c4dfe22f41a..49bf713b9bb6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -11,6 +11,7 @@
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
 #include <linux/cred.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..83afc606d67a
--- /dev/null
+++ b/rust/kernel/dma.rs
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Direct memory access (DMA).
+//!
+//! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
+
+use crate::{
+    bindings, build_assert,
+    device::Device,
+    error::code::*,
+    error::Result,
+    transmute::{AsBytes, FromBytes},
+    types::ARef,
+};
+
+/// Possible attributes associated with a DMA mapping.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`attrs`] module.
+#[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 attrributes.
+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 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,
+    dma_attrs: Attrs,
+}
+
+impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
+    /// Allocates a region of `size_of::<T> * count` of consistent memory.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::device::Device;
+    /// use kernel::dma::{attrs::*, CoherentAllocation};
+    ///
+    /// # fn test(dev: &Device) -> Result {
+    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_attrs(dev.into(), 4, GFP_KERNEL,
+    ///                                                                  DMA_ATTR_NO_WARN)?;
+    /// # Ok::<(), Error>(()) }
+    /// ```
+    pub fn alloc_attrs(
+        dev: ARef<Device>,
+        count: usize,
+        gfp_flags: kernel::alloc::Flags,
+        dma_attrs: Attrs,
+    ) -> Result<CoherentAllocation<T>> {
+        build_assert!(
+            core::mem::size_of::<T>() > 0,
+            "It doesn't make sense for the allocated type to be a ZST"
+        );
+
+        let size = count
+            .checked_mul(core::mem::size_of::<T>())
+            .ok_or(EOVERFLOW)?;
+        let mut dma_handle = 0;
+        // SAFETY: device pointer is guaranteed as valid by 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,
+                gfp_flags.as_raw(),
+                dma_attrs.as_raw(),
+            )
+        };
+        if ret.is_null() {
+            return Err(ENOMEM);
+        }
+        // INVARIANT: We just successfully allocated a coherent region which is accessible for
+        // `count` elements, hence the cpu address is valid. We also hold a refcounted reference
+        // to the device.
+        Ok(Self {
+            dev,
+            dma_handle,
+            count,
+            cpu_addr: ret as *mut T,
+            dma_attrs,
+        })
+    }
+
+    /// Performs the same functionality as `alloc_attrs`, except the `dma_attrs` is 0 by default.
+    pub fn alloc_coherent(
+        dev: ARef<Device>,
+        count: usize,
+        gfp_flags: kernel::alloc::Flags,
+    ) -> Result<CoherentAllocation<T>> {
+        CoherentAllocation::alloc_attrs(dev, count, gfp_flags, Attrs(0))
+    }
+
+    /// Returns the device, base address, dma handle, attributes and the size of the
+    /// allocated region.
+    ///
+    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
+    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
+    /// the returned device exists.
+    pub fn into_parts(
+        self,
+    ) -> (
+        ARef<Device>,
+        *mut T,
+        bindings::dma_addr_t,
+        crate::ffi::c_ulong,
+        usize,
+    ) {
+        let size = self.count * core::mem::size_of::<T>();
+        let ret = (
+            // SAFETY: `&self.dev` is valid for reads.
+            unsafe { core::ptr::read(&self.dev) },
+            self.cpu_addr,
+            self.dma_handle,
+            self.dma_attrs.as_raw(),
+            size,
+        );
+        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
+    }
+
+    /// 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
+    }
+
+    /// Reads 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 data returned should be regarded by the
+    /// caller as a snapshot of the region when this function is called, as 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.
+    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
+        if offset + count >= 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) })
+    }
+
+    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
+    /// number of bytes.
+    pub fn write(&self, src: &[T], offset: usize) -> Result {
+        if offset + src.len() >= 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(())
+    }
+}
+
+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,
+                self.dma_attrs.as_raw(),
+            )
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 545d1170ee63..36ac88fd91e7 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -37,6 +37,7 @@
 pub mod build_assert;
 pub mod cred;
 pub mod device;
+pub mod dma;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
-- 
2.43.0

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

[Petr Tesařík]

On Thu, 23 Jan 2025 12:42:58 +0200
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, which
> was also based on earlier work by Wedson Almeida Filho.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>[...]
> +/// 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 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,
> +    dma_attrs: Attrs,
> +}
> +
> +impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
> +    /// Allocates a region of `size_of::<T> * count` of consistent memory.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::device::Device;
> +    /// use kernel::dma::{attrs::*, CoherentAllocation};
> +    ///
> +    /// # fn test(dev: &Device) -> Result {
> +    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_attrs(dev.into(), 4, GFP_KERNEL,
> +    ///                                                                  DMA_ATTR_NO_WARN)?;
> +    /// # Ok::<(), Error>(()) }
> +    /// ```
> +    pub fn alloc_attrs(
> +        dev: ARef<Device>,
> +        count: usize,
> +        gfp_flags: kernel::alloc::Flags,
> +        dma_attrs: Attrs,
> +    ) -> Result<CoherentAllocation<T>> {
> +        build_assert!(
> +            core::mem::size_of::<T>() > 0,
> +            "It doesn't make sense for the allocated type to be a ZST"
> +        );
> +
> +        let size = count
> +            .checked_mul(core::mem::size_of::<T>())
> +            .ok_or(EOVERFLOW)?;
> +        let mut dma_handle = 0;
> +        // SAFETY: device pointer is guaranteed as valid by 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,
> +                gfp_flags.as_raw(),
> +                dma_attrs.as_raw(),
> +            )
> +        };
> +        if ret.is_null() {
> +            return Err(ENOMEM);
> +        }
> +        // INVARIANT: We just successfully allocated a coherent region which is accessible for
> +        // `count` elements, hence the cpu address is valid. We also hold a refcounted reference
> +        // to the device.
> +        Ok(Self {
> +            dev,
> +            dma_handle,
> +            count,
> +            cpu_addr: ret as *mut T,
> +            dma_attrs,
> +        })
> +    }
> +
> +    /// Performs the same functionality as `alloc_attrs`, except the `dma_attrs` is 0 by default.
> +    pub fn alloc_coherent(
> +        dev: ARef<Device>,
> +        count: usize,
> +        gfp_flags: kernel::alloc::Flags,
> +    ) -> Result<CoherentAllocation<T>> {
> +        CoherentAllocation::alloc_attrs(dev, count, gfp_flags, Attrs(0))
> +    }
> +
> +    /// Returns the device, base address, dma handle, attributes and the size of the
> +    /// allocated region.
> +    ///
> +    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
> +    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
> +    /// the returned device exists.
> +    pub fn into_parts(
> +        self,
> +    ) -> (
> +        ARef<Device>,
> +        *mut T,
> +        bindings::dma_addr_t,
> +        crate::ffi::c_ulong,
> +        usize,
> +    ) {
> +        let size = self.count * core::mem::size_of::<T>();
> +        let ret = (
> +            // SAFETY: `&self.dev` is valid for reads.
> +            unsafe { core::ptr::read(&self.dev) },
> +            self.cpu_addr,
> +            self.dma_handle,
> +            self.dma_attrs.as_raw(),
> +            size,
> +        );
> +        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
> +    }
> +
> +    /// 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
> +    }
> +
> +    /// Reads 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 data returned should be regarded by the
> +    /// caller as a snapshot of the region when this function is called, as 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.
> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> +        if offset + count >= self.count {

I'm probably missing something, but how do you know that this addition
can't overflow? I mean, since this is a public function, users can do
something dumb such as buf.as_slice(usize::MAX, n), can't they?

What about something like:

    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) })
> +    }
> +
> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> +    /// number of bytes.
> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
> +        if offset + src.len() >= self.count {

Same here.

Petr T

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

[Abdiel Janulgue]

On 23/01/2025 14:30, Petr Tesařík wrote:
> On Thu, 23 Jan 2025 12:42:58 +0200
> Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
>> +
>> +    /// Reads 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 data returned should be regarded by the
>> +    /// caller as a snapshot of the region when this function is called, as 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.
>> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
>> +        if offset + count >= self.count {
> 
> I'm probably missing something, but how do you know that this addition
> can't overflow? I mean, since this is a public function, users can do
> something dumb such as buf.as_slice(usize::MAX, n), can't they?
> 
> What about something like:
> 
>      let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>      if end >= self.count { ... }
>

Makes sense. This could also just return EINVAL instead of EOVERFLOW, 
but either way is fine with me.

Miguel, what do you think? Would it be possible just include this change 
and the one below locally if you think this series is ready for merging?

Regards,
Abdiel

>> +            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) })
>> +    }
>> +
>> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
>> +    /// number of bytes.
>> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
>> +        if offset + src.len() >= self.count {
> 
> Same here.
> 
> Petr T

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

[Miguel Ojeda]

On Thu, Jan 23, 2025 at 2:38 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Miguel, what do you think? Would it be possible just include this change
> and the one below locally if you think this series is ready for merging?

Sure -- there is more than a week until the merge window closes and so
on, so we will see if there is more feedback and Acks by then.

Cheers,
Miguel

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

[Daniel Almeida]

> On 23 Jan 2025, at 11:44, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Thu, Jan 23, 2025 at 2:38 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>> 
>> Miguel, what do you think? Would it be possible just include this change
>> and the one below locally if you think this series is ready for merging?
> 
> Sure -- there is more than a week until the merge window closes and so
> on, so we will see if there is more feedback and Acks by then.
> 
> Cheers,
> Miguel
> 

Note: since an unsafe `as_slice()` function was added (on Boqun’s request IIUC) then
an unsafe `as_slice_mut()` should be added too.

I discussed this with Abdiel earlier.

— Daniel

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

[Alice Ryhl]

On Thu, Jan 23, 2025 at 11:43 AM 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, which
> was also based on earlier work by Wedson Almeida Filho.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>

Overall I think it looks pretty good, but a follow-up from our
discussion the other day.

> +    /// Returns the device, base address, dma handle, attributes and the size of the
> +    /// allocated region.
> +    ///
> +    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
> +    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
> +    /// the returned device exists.
> +    pub fn into_parts(
> +        self,
> +    ) -> (
> +        ARef<Device>,
> +        *mut T,
> +        bindings::dma_addr_t,
> +        crate::ffi::c_ulong,
> +        usize,
> +    ) {
> +        let size = self.count * core::mem::size_of::<T>();
> +        let ret = (
> +            // SAFETY: `&self.dev` is valid for reads.
> +            unsafe { core::ptr::read(&self.dev) },

This safety comment should explain why this will not lead to
decrementing the refcount too many times. Which is because this
doesn't actually duplicate the ARef due to the use of mem::forget.

That said, I think wrapping self in ManuallyDrop is slightly easier to
read than using mem::forget. I refer you to this pattern from Tokio
that solves a similar problem:
https://github.com/tokio-rs/tokio/blob/ee19b0ed7371b069112b9c9ef9280b81f3438d26/tokio/src/sync/rwlock/owned_write_guard.rs#L38-L51

> +            self.cpu_addr,
> +            self.dma_handle,
> +            self.dma_attrs.as_raw(),

Why not return the Attrs type instead of an integer?

> +            size,
> +        );
> +        core::mem::forget(self);
> +        ret
> +    }

> +    /// Reads 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 data returned should be regarded by the
> +    /// caller as a snapshot of the region when this function is called, as 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.
> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {

You were asked to rename this function because it returns a slice, but
I wonder if it's better to take an `&mut [T]` argument and to have
this function copy data into that argument. That way, we could make
the function itself safe. Perhaps the actual copy needs to be
volatile?

Well ... I understand that we did this previously and that we want to
avoid it because it causes too much reading if T is a struct and we
just want to read one of its fields. How about an API like this?

dma_read!(my_alloc[7].foo)

which expands to something that reads the value of the foo field of
the 7th element, and

dma_write!(my_alloc[7].foo = 13);

That expands to something that writes 13 to field foo of the 7th element.

Thoughts? I'm proposing this to avoid going in circles between the
same solutions.

Alice

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

[Petr Tesařík]

On Fri, 24 Jan 2025 08:27:36 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Thu, Jan 23, 2025 at 11:43 AM 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, which
> > was also based on earlier work by Wedson Almeida Filho.
> >
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>  
> 
> Overall I think it looks pretty good, but a follow-up from our
> discussion the other day.
> 
> > +    /// Returns the device, base address, dma handle, attributes and the size of the
> > +    /// allocated region.
> > +    ///
> > +    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
> > +    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
> > +    /// the returned device exists.
> > +    pub fn into_parts(
> > +        self,
> > +    ) -> (
> > +        ARef<Device>,
> > +        *mut T,
> > +        bindings::dma_addr_t,
> > +        crate::ffi::c_ulong,
> > +        usize,
> > +    ) {
> > +        let size = self.count * core::mem::size_of::<T>();
> > +        let ret = (
> > +            // SAFETY: `&self.dev` is valid for reads.
> > +            unsafe { core::ptr::read(&self.dev) },  
> 
> This safety comment should explain why this will not lead to
> decrementing the refcount too many times. Which is because this
> doesn't actually duplicate the ARef due to the use of mem::forget.
> 
> That said, I think wrapping self in ManuallyDrop is slightly easier to
> read than using mem::forget. I refer you to this pattern from Tokio
> that solves a similar problem:
> https://github.com/tokio-rs/tokio/blob/ee19b0ed7371b069112b9c9ef9280b81f3438d26/tokio/src/sync/rwlock/owned_write_guard.rs#L38-L51
> 
> > +            self.cpu_addr,
> > +            self.dma_handle,
> > +            self.dma_attrs.as_raw(),  
> 
> Why not return the Attrs type instead of an integer?
> 
> > +            size,
> > +        );
> > +        core::mem::forget(self);
> > +        ret
> > +    }  
> 
> > +    /// Reads 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 data returned should be regarded by the
> > +    /// caller as a snapshot of the region when this function is called, as 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.
> > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {  
> 
> You were asked to rename this function because it returns a slice, but
> I wonder if it's better to take an `&mut [T]` argument and to have
> this function copy data into that argument. That way, we could make
> the function itself safe. Perhaps the actual copy needs to be
> volatile?
> 
> Well ... I understand that we did this previously and that we want to
> avoid it because it causes too much reading if T is a struct and we
> just want to read one of its fields. How about an API like this?
> 
> dma_read!(my_alloc[7].foo)
> 
> which expands to something that reads the value of the foo field of
> the 7th element, and
> 
> dma_write!(my_alloc[7].foo = 13);
> 
> That expands to something that writes 13 to field foo of the 7th element.
> 
> Thoughts? I'm proposing this to avoid going in circles between the
> same solutions.

I don't have a strong opinion, but maybe you could elaborate a bit on
what exactly you dislike about as_slice() / as_slice_mut().

Petr T

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 7:16 AM Petr Tesařík <petr@tesarici.cz> wrote:
>
> On Fri, 24 Jan 2025 08:27:36 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Thu, Jan 23, 2025 at 11:43 AM 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, which
> > > was also based on earlier work by Wedson Almeida Filho.
> > >
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> >
> > Overall I think it looks pretty good, but a follow-up from our
> > discussion the other day.
> >
> > > +    /// Returns the device, base address, dma handle, attributes and the size of the
> > > +    /// allocated region.
> > > +    ///
> > > +    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
> > > +    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
> > > +    /// the returned device exists.
> > > +    pub fn into_parts(
> > > +        self,
> > > +    ) -> (
> > > +        ARef<Device>,
> > > +        *mut T,
> > > +        bindings::dma_addr_t,
> > > +        crate::ffi::c_ulong,
> > > +        usize,
> > > +    ) {
> > > +        let size = self.count * core::mem::size_of::<T>();
> > > +        let ret = (
> > > +            // SAFETY: `&self.dev` is valid for reads.
> > > +            unsafe { core::ptr::read(&self.dev) },
> >
> > This safety comment should explain why this will not lead to
> > decrementing the refcount too many times. Which is because this
> > doesn't actually duplicate the ARef due to the use of mem::forget.
> >
> > That said, I think wrapping self in ManuallyDrop is slightly easier to
> > read than using mem::forget. I refer you to this pattern from Tokio
> > that solves a similar problem:
> > https://github.com/tokio-rs/tokio/blob/ee19b0ed7371b069112b9c9ef9280b81f3438d26/tokio/src/sync/rwlock/owned_write_guard.rs#L38-L51
> >
> > > +            self.cpu_addr,
> > > +            self.dma_handle,
> > > +            self.dma_attrs.as_raw(),
> >
> > Why not return the Attrs type instead of an integer?
> >
> > > +            size,
> > > +        );
> > > +        core::mem::forget(self);
> > > +        ret
> > > +    }
> >
> > > +    /// Reads 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 data returned should be regarded by the
> > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> >
> > You were asked to rename this function because it returns a slice, but
> > I wonder if it's better to take an `&mut [T]` argument and to have
> > this function copy data into that argument. That way, we could make
> > the function itself safe. Perhaps the actual copy needs to be
> > volatile?
> >
> > Well ... I understand that we did this previously and that we want to
> > avoid it because it causes too much reading if T is a struct and we
> > just want to read one of its fields. How about an API like this?
> >
> > dma_read!(my_alloc[7].foo)
> >
> > which expands to something that reads the value of the foo field of
> > the 7th element, and
> >
> > dma_write!(my_alloc[7].foo = 13);
> >
> > That expands to something that writes 13 to field foo of the 7th element.
> >
> > Thoughts? I'm proposing this to avoid going in circles between the
> > same solutions.
>
> I don't have a strong opinion, but maybe you could elaborate a bit on
> what exactly you dislike about as_slice() / as_slice_mut().

The as_slice() method is unsafe as currently defined. The purpose of
this proposal is so we can have a method that is not unsafe.

Alice

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

[Danilo Krummrich]

On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
> > +    /// Reads 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 data returned should be regarded by the
> > +    /// caller as a snapshot of the region when this function is called, as 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.
> > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> 
> You were asked to rename this function because it returns a slice, but
> I wonder if it's better to take an `&mut [T]` argument and to have
> this function copy data into that argument. That way, we could make
> the function itself safe. Perhaps the actual copy needs to be
> volatile?

Why do we consider the existing one unsafe?

Surely, it's not desirable that the contents of the buffer are modified by the
HW unexpectedly, but is this a concern in terms of Rust safety requirements?

And if so, how does this go away with the proposed approach?

> 
> Well ... I understand that we did this previously and that we want to
> avoid it because it causes too much reading if T is a struct and we
> just want to read one of its fields. How about an API like this?
> 
> dma_read!(my_alloc[7].foo)
> 
> which expands to something that reads the value of the foo field of
> the 7th element, and
> 
> dma_write!(my_alloc[7].foo = 13);

I really like how this turns out.

> 
> That expands to something that writes 13 to field foo of the 7th element.
> 
> Thoughts? I'm proposing this to avoid going in circles between the
> same solutions.
> 
> Alice

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > <abdiel.janulgue@gmail.com> wrote:
> > > +    /// Reads 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 data returned should be regarded by the
> > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> >
> > You were asked to rename this function because it returns a slice, but
> > I wonder if it's better to take an `&mut [T]` argument and to have
> > this function copy data into that argument. That way, we could make
> > the function itself safe. Perhaps the actual copy needs to be
> > volatile?
>
> Why do we consider the existing one unsafe?
>
> Surely, it's not desirable that the contents of the buffer are modified by the
> HW unexpectedly, but is this a concern in terms of Rust safety requirements?
>
> And if so, how does this go away with the proposed approach?

In Rust, it is undefined behavior if the value behind an immutable
reference changes (unless the type uses UnsafeCell / Opaque or
similar). That is, any two consecutive reads of the same immutable
reference must return the same byte value no matter what happened in
between those reads.

If we manually perform the read as a volatile read, then it is
arguably allowed for the value to be modified by the hardware while we
read the value.

> > Well ... I understand that we did this previously and that we want to
> > avoid it because it causes too much reading if T is a struct and we
> > just want to read one of its fields. How about an API like this?
> >
> > dma_read!(my_alloc[7].foo)
> >
> > which expands to something that reads the value of the foo field of
> > the 7th element, and
> >
> > dma_write!(my_alloc[7].foo = 13);
>
> I really like how this turns out.

Yes, I think it would be a nice API.

Alice

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

[Danilo Krummrich]

On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > <abdiel.janulgue@gmail.com> wrote:
> > > > +    /// Reads 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 data returned should be regarded by the
> > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > >
> > > You were asked to rename this function because it returns a slice, but
> > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > this function copy data into that argument. That way, we could make
> > > the function itself safe. Perhaps the actual copy needs to be
> > > volatile?
> >
> > Why do we consider the existing one unsafe?
> >
> > Surely, it's not desirable that the contents of the buffer are modified by the
> > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> >
> > And if so, how does this go away with the proposed approach?
> 
> In Rust, it is undefined behavior if the value behind an immutable
> reference changes (unless the type uses UnsafeCell / Opaque or
> similar). That is, any two consecutive reads of the same immutable
> reference must return the same byte value no matter what happened in
> between those reads.

Undefined as in the sense of anything is allowed to happen? I thought undefined
as in you might still see the old value on two consecutive reads.

Do you have a pointer to the corresponding docs?

> 
> If we manually perform the read as a volatile read, then it is
> arguably allowed for the value to be modified by the hardware while we
> read the value.

From read_volatile() [1]: "In particular, a race between a read_volatile and any
write operation to the same location is undefined behavior."

Also, what if the hardware put a value that is invalid for the type?

[1] https://doc.rust-lang.org/std/ptr/fn.read_volatile.html

> 
> > > Well ... I understand that we did this previously and that we want to
> > > avoid it because it causes too much reading if T is a struct and we
> > > just want to read one of its fields. How about an API like this?
> > >
> > > dma_read!(my_alloc[7].foo)
> > >
> > > which expands to something that reads the value of the foo field of
> > > the 7th element, and
> > >
> > > dma_write!(my_alloc[7].foo = 13);
> >
> > I really like how this turns out.
> 
> Yes, I think it would be a nice API.
> 
> Alice

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 1:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > >
> > > > You were asked to rename this function because it returns a slice, but
> > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > this function copy data into that argument. That way, we could make
> > > > the function itself safe. Perhaps the actual copy needs to be
> > > > volatile?
> > >
> > > Why do we consider the existing one unsafe?
> > >
> > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > >
> > > And if so, how does this go away with the proposed approach?
> >
> > In Rust, it is undefined behavior if the value behind an immutable
> > reference changes (unless the type uses UnsafeCell / Opaque or
> > similar). That is, any two consecutive reads of the same immutable
> > reference must return the same byte value no matter what happened in
> > between those reads.
>
> Undefined as in the sense of anything is allowed to happen?

Yes.

> I thought undefined
> as in you might still see the old value on two consecutive reads.

That is the optimization that motivates this being UB, but it's
defined as full UB.

> Do you have a pointer to the corresponding docs?

Sure, it's on the "behavior considered undefined" page:
Moreover, the bytes pointed to by a shared reference, including
transitively through other references (both shared and mutable) and
Boxes, are immutable; transitivity includes those references stored in
fields of compound types.

https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.immutable

> > If we manually perform the read as a volatile read, then it is
> > arguably allowed for the value to be modified by the hardware while we
> > read the value.
>
> From read_volatile() [1]: "In particular, a race between a read_volatile and any
> write operation to the same location is undefined behavior."

I mean, ultimately we are a bit on our own here. In C code you just
use an ordinary read / write, so we could use the ordinary
core::ptr::copy_nonoverlapping method to mirror that. We've been told
from the Rust project that we should just do these kinds of things
like we do in C - technically these things aren't okay in C either,
but because LLVM will try to avoid breaking patterns used in the
kernel, they shouldn't break in Rust either.

But using an immutable reference should be avoided because that gives
LLVM optimization hints that we are not giving to LLVM in C code.

> Also, what if the hardware put a value that is invalid for the type?

The trait bounds require that T is FromBytes, which has a safety
requirement that all bit-patterns must be valid for the type.

Alice

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

[Danilo Krummrich]

On Mon, Jan 27, 2025 at 02:25:03PM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 1:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > >
> > > > > You were asked to rename this function because it returns a slice, but
> > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > this function copy data into that argument. That way, we could make
> > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > volatile?
> > > >
> > > > Why do we consider the existing one unsafe?
> > > >
> > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > >
> > > > And if so, how does this go away with the proposed approach?
> > >
> > > In Rust, it is undefined behavior if the value behind an immutable
> > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > similar). That is, any two consecutive reads of the same immutable
> > > reference must return the same byte value no matter what happened in
> > > between those reads.
> >
> > Undefined as in the sense of anything is allowed to happen?
> 
> Yes.
> 
> > I thought undefined
> > as in you might still see the old value on two consecutive reads.
> 
> That is the optimization that motivates this being UB, but it's
> defined as full UB.
> 
> > Do you have a pointer to the corresponding docs?
> 
> Sure, it's on the "behavior considered undefined" page:
> Moreover, the bytes pointed to by a shared reference, including
> transitively through other references (both shared and mutable) and
> Boxes, are immutable; transitivity includes those references stored in
> fields of compound types.
> 
> https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.immutable
> 
> > > If we manually perform the read as a volatile read, then it is
> > > arguably allowed for the value to be modified by the hardware while we
> > > read the value.
> >
> > From read_volatile() [1]: "In particular, a race between a read_volatile and any
> > write operation to the same location is undefined behavior."
> 
> I mean, ultimately we are a bit on our own here. In C code you just
> use an ordinary read / write, so we could use the ordinary
> core::ptr::copy_nonoverlapping method to mirror that. We've been told
> from the Rust project that we should just do these kinds of things
> like we do in C - technically these things aren't okay in C either,
> but because LLVM will try to avoid breaking patterns used in the
> kernel, they shouldn't break in Rust either.
> 
> But using an immutable reference should be avoided because that gives
> LLVM optimization hints that we are not giving to LLVM in C code.

Thanks for clarifying.

I think we should add this as a note to the corresponding code.

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 2:34 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jan 27, 2025 at 02:25:03PM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 1:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > > >
> > > > > > You were asked to rename this function because it returns a slice, but
> > > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > > this function copy data into that argument. That way, we could make
> > > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > > volatile?
> > > > >
> > > > > Why do we consider the existing one unsafe?
> > > > >
> > > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > > >
> > > > > And if so, how does this go away with the proposed approach?
> > > >
> > > > In Rust, it is undefined behavior if the value behind an immutable
> > > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > > similar). That is, any two consecutive reads of the same immutable
> > > > reference must return the same byte value no matter what happened in
> > > > between those reads.
> > >
> > > Undefined as in the sense of anything is allowed to happen?
> >
> > Yes.
> >
> > > I thought undefined
> > > as in you might still see the old value on two consecutive reads.
> >
> > That is the optimization that motivates this being UB, but it's
> > defined as full UB.
> >
> > > Do you have a pointer to the corresponding docs?
> >
> > Sure, it's on the "behavior considered undefined" page:
> > Moreover, the bytes pointed to by a shared reference, including
> > transitively through other references (both shared and mutable) and
> > Boxes, are immutable; transitivity includes those references stored in
> > fields of compound types.
> >
> > https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.immutable
> >
> > > > If we manually perform the read as a volatile read, then it is
> > > > arguably allowed for the value to be modified by the hardware while we
> > > > read the value.
> > >
> > > From read_volatile() [1]: "In particular, a race between a read_volatile and any
> > > write operation to the same location is undefined behavior."
> >
> > I mean, ultimately we are a bit on our own here. In C code you just
> > use an ordinary read / write, so we could use the ordinary
> > core::ptr::copy_nonoverlapping method to mirror that. We've been told
> > from the Rust project that we should just do these kinds of things
> > like we do in C - technically these things aren't okay in C either,
> > but because LLVM will try to avoid breaking patterns used in the
> > kernel, they shouldn't break in Rust either.
> >
> > But using an immutable reference should be avoided because that gives
> > LLVM optimization hints that we are not giving to LLVM in C code.
>
> Thanks for clarifying.
>
> I think we should add this as a note to the corresponding code.

As for whether it should be volatile or not, we should follow the
kernel's guidance in C code. I notice this sentence:

Pointers to data structures in coherent memory which might be modified
by I/O devices can, sometimes, legitimately be volatile. A ring buffer
used by a network adapter, where that adapter changes pointers to
indicate which descriptors have been processed, is an example of this
type of situation.
https://docs.kernel.org/process/volatile-considered-harmful.html

Alice

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

[Boqun Feng]

On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > <abdiel.janulgue@gmail.com> wrote:
> > > > +    /// Reads 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 data returned should be regarded by the
> > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > >
> > > You were asked to rename this function because it returns a slice, but
> > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > this function copy data into that argument. That way, we could make
> > > the function itself safe. Perhaps the actual copy needs to be
> > > volatile?
> >
> > Why do we consider the existing one unsafe?
> >
> > Surely, it's not desirable that the contents of the buffer are modified by the
> > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> >
> > And if so, how does this go away with the proposed approach?
> 
> In Rust, it is undefined behavior if the value behind an immutable
> reference changes (unless the type uses UnsafeCell / Opaque or
> similar). That is, any two consecutive reads of the same immutable
> reference must return the same byte value no matter what happened in
> between those reads.
> 
> If we manually perform the read as a volatile read, then it is
> arguably allowed for the value to be modified by the hardware while we
> read the value.
> 

Do you also assume that volatile read/write provide some sort of
atomicity? Because otherwise even though the read/write may not be
considered as UB, then results can be load/store teared.

I asked because I think in case that we need atomicity, we should just
use atomic APIs.

Regards,
Boqun

> > > Well ... I understand that we did this previously and that we want to
> > > avoid it because it causes too much reading if T is a struct and we
> > > just want to read one of its fields. How about an API like this?
> > >
> > > dma_read!(my_alloc[7].foo)
> > >
> > > which expands to something that reads the value of the foo field of
> > > the 7th element, and
> > >
> > > dma_write!(my_alloc[7].foo = 13);
> >
> > I really like how this turns out.
> 
> Yes, I think it would be a nice API.
> 
> Alice

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > >
> > > > You were asked to rename this function because it returns a slice, but
> > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > this function copy data into that argument. That way, we could make
> > > > the function itself safe. Perhaps the actual copy needs to be
> > > > volatile?
> > >
> > > Why do we consider the existing one unsafe?
> > >
> > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > >
> > > And if so, how does this go away with the proposed approach?
> >
> > In Rust, it is undefined behavior if the value behind an immutable
> > reference changes (unless the type uses UnsafeCell / Opaque or
> > similar). That is, any two consecutive reads of the same immutable
> > reference must return the same byte value no matter what happened in
> > between those reads.
> >
> > If we manually perform the read as a volatile read, then it is
> > arguably allowed for the value to be modified by the hardware while we
> > read the value.
> >
>
> Do you also assume that volatile read/write provide some sort of
> atomicity? Because otherwise even though the read/write may not be
> considered as UB, then results can be load/store teared.
>
> I asked because I think in case that we need atomicity, we should just
> use atomic APIs.

No, I'm not assuming that. I think it's like uaccess. Under normal
cases, it's not going to be concurrently modified, but it shouldn't
trigger UB if it is.

Alice

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

[Boqun Feng]

On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > >
> > > > > You were asked to rename this function because it returns a slice, but
> > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > this function copy data into that argument. That way, we could make
> > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > volatile?
> > > >
> > > > Why do we consider the existing one unsafe?
> > > >
> > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > >
> > > > And if so, how does this go away with the proposed approach?
> > >
> > > In Rust, it is undefined behavior if the value behind an immutable
> > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > similar). That is, any two consecutive reads of the same immutable
> > > reference must return the same byte value no matter what happened in
> > > between those reads.
> > >
> > > If we manually perform the read as a volatile read, then it is
> > > arguably allowed for the value to be modified by the hardware while we
> > > read the value.
> > >
> >
> > Do you also assume that volatile read/write provide some sort of
> > atomicity? Because otherwise even though the read/write may not be
> > considered as UB, then results can be load/store teared.
> >
> > I asked because I think in case that we need atomicity, we should just
> > use atomic APIs.
> 
> No, I'm not assuming that. I think it's like uaccess. Under normal
> cases, it's not going to be concurrently modified, but it shouldn't
> trigger UB if it is.
> 

Let's say my_alloc[7].foo is a (u64, u64), would

	dma_read!(my_alloc[7].foo)

and 

	dma_write!(my_alloc[7].foo, (1u64, 2u64))

trigger any UB when they are concurrent? (Of course, the example here is
a bit inpropriate because it's DMA buff, but still the question is more
on whatever atomic expectation we want from read_volatile() and
write_volatile()).

Regards,
Boqun

> Alice

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 7:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > > >
> > > > > > You were asked to rename this function because it returns a slice, but
> > > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > > this function copy data into that argument. That way, we could make
> > > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > > volatile?
> > > > >
> > > > > Why do we consider the existing one unsafe?
> > > > >
> > > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > > >
> > > > > And if so, how does this go away with the proposed approach?
> > > >
> > > > In Rust, it is undefined behavior if the value behind an immutable
> > > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > > similar). That is, any two consecutive reads of the same immutable
> > > > reference must return the same byte value no matter what happened in
> > > > between those reads.
> > > >
> > > > If we manually perform the read as a volatile read, then it is
> > > > arguably allowed for the value to be modified by the hardware while we
> > > > read the value.
> > > >
> > >
> > > Do you also assume that volatile read/write provide some sort of
> > > atomicity? Because otherwise even though the read/write may not be
> > > considered as UB, then results can be load/store teared.
> > >
> > > I asked because I think in case that we need atomicity, we should just
> > > use atomic APIs.
> >
> > No, I'm not assuming that. I think it's like uaccess. Under normal
> > cases, it's not going to be concurrently modified, but it shouldn't
> > trigger UB if it is.
> >
>
> Let's say my_alloc[7].foo is a (u64, u64), would
>
>         dma_read!(my_alloc[7].foo)
>
> and
>
>         dma_write!(my_alloc[7].foo, (1u64, 2u64))
>
> trigger any UB when they are concurrent? (Of course, the example here is
> a bit inpropriate because it's DMA buff, but still the question is more
> on whatever atomic expectation we want from read_volatile() and
> write_volatile()).

I imagine that it would be most convenient for it to not be UB, but I
also don't think people would have an expectation for that to not
involve tearing.

Alice

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

[Boqun Feng]

On Mon, Jan 27, 2025 at 07:46:07PM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 7:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> > > On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > > > >
> > > > > > > You were asked to rename this function because it returns a slice, but
> > > > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > > > this function copy data into that argument. That way, we could make
> > > > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > > > volatile?
> > > > > >
> > > > > > Why do we consider the existing one unsafe?
> > > > > >
> > > > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > > > >
> > > > > > And if so, how does this go away with the proposed approach?
> > > > >
> > > > > In Rust, it is undefined behavior if the value behind an immutable
> > > > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > > > similar). That is, any two consecutive reads of the same immutable
> > > > > reference must return the same byte value no matter what happened in
> > > > > between those reads.
> > > > >
> > > > > If we manually perform the read as a volatile read, then it is
> > > > > arguably allowed for the value to be modified by the hardware while we
> > > > > read the value.
> > > > >
> > > >
> > > > Do you also assume that volatile read/write provide some sort of
> > > > atomicity? Because otherwise even though the read/write may not be
> > > > considered as UB, then results can be load/store teared.
> > > >
> > > > I asked because I think in case that we need atomicity, we should just
> > > > use atomic APIs.
> > >
> > > No, I'm not assuming that. I think it's like uaccess. Under normal
> > > cases, it's not going to be concurrently modified, but it shouldn't
> > > trigger UB if it is.
> > >
> >
> > Let's say my_alloc[7].foo is a (u64, u64), would
> >
> >         dma_read!(my_alloc[7].foo)
> >
> > and
> >
> >         dma_write!(my_alloc[7].foo, (1u64, 2u64))
> >
> > trigger any UB when they are concurrent? (Of course, the example here is
> > a bit inpropriate because it's DMA buff, but still the question is more
> > on whatever atomic expectation we want from read_volatile() and
> > write_volatile()).
> 
> I imagine that it would be most convenient for it to not be UB, but I
> also don't think people would have an expectation for that to not
> involve tearing.
> 

Depending on the granularity that tearing can happen, if .foo is an enum
(or any other type that not all bit combinations are valid) and tearing
can happen at byte levels, then a racing dma_read() may read invalid
data.

I think it's fine to expect read_volatile() and write_volatile()
themselves don't trigger UB, but we will need to be careful about the
atomic granularity that we can expect on them. It would be more clear if
we use the atomic API here (and implementation can be read_volatile()
and write_volatile()), and it can avoid coding based on tribal knowledge
such as "in kernel, read_volatile() and write_volatile() imply atomic".

Regards,
Boqun

> Alice

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 8:01 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Jan 27, 2025 at 07:46:07PM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 7:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> > > > On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > > > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > > > > >
> > > > > > > > You were asked to rename this function because it returns a slice, but
> > > > > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > > > > this function copy data into that argument. That way, we could make
> > > > > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > > > > volatile?
> > > > > > >
> > > > > > > Why do we consider the existing one unsafe?
> > > > > > >
> > > > > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > > > > >
> > > > > > > And if so, how does this go away with the proposed approach?
> > > > > >
> > > > > > In Rust, it is undefined behavior if the value behind an immutable
> > > > > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > > > > similar). That is, any two consecutive reads of the same immutable
> > > > > > reference must return the same byte value no matter what happened in
> > > > > > between those reads.
> > > > > >
> > > > > > If we manually perform the read as a volatile read, then it is
> > > > > > arguably allowed for the value to be modified by the hardware while we
> > > > > > read the value.
> > > > > >
> > > > >
> > > > > Do you also assume that volatile read/write provide some sort of
> > > > > atomicity? Because otherwise even though the read/write may not be
> > > > > considered as UB, then results can be load/store teared.
> > > > >
> > > > > I asked because I think in case that we need atomicity, we should just
> > > > > use atomic APIs.
> > > >
> > > > No, I'm not assuming that. I think it's like uaccess. Under normal
> > > > cases, it's not going to be concurrently modified, but it shouldn't
> > > > trigger UB if it is.
> > > >
> > >
> > > Let's say my_alloc[7].foo is a (u64, u64), would
> > >
> > >         dma_read!(my_alloc[7].foo)
> > >
> > > and
> > >
> > >         dma_write!(my_alloc[7].foo, (1u64, 2u64))
> > >
> > > trigger any UB when they are concurrent? (Of course, the example here is
> > > a bit inpropriate because it's DMA buff, but still the question is more
> > > on whatever atomic expectation we want from read_volatile() and
> > > write_volatile()).
> >
> > I imagine that it would be most convenient for it to not be UB, but I
> > also don't think people would have an expectation for that to not
> > involve tearing.
> >
>
> Depending on the granularity that tearing can happen, if .foo is an enum
> (or any other type that not all bit combinations are valid) and tearing
> can happen at byte levels, then a racing dma_read() may read invalid
> data.

T: FromBytes + ToBytes is already required for these types. You can't
use these operations with such an enum.

> I think it's fine to expect read_volatile() and write_volatile()
> themselves don't trigger UB, but we will need to be careful about the
> atomic granularity that we can expect on them. It would be more clear if
> we use the atomic API here (and implementation can be read_volatile()
> and write_volatile()), and it can avoid coding based on tribal knowledge
> such as "in kernel, read_volatile() and write_volatile() imply atomic".

Why should we use atomics for operations that don't need to be atomic?
Most of the time, dma memory is not *actually* changed while you read
it.

Alice

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

[Boqun Feng]

On Mon, Jan 27, 2025 at 08:05:46PM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 8:01 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Jan 27, 2025 at 07:46:07PM +0100, Alice Ryhl wrote:
> > > On Mon, Jan 27, 2025 at 7:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> > > > > On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > > > > > > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > > > > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > > > > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > > > > > > +    /// Reads 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 data returned should be regarded by the
> > > > > > > > > > +    /// caller as a snapshot of the region when this function is called, as 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.
> > > > > > > > > > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > > > > > > > >
> > > > > > > > > You were asked to rename this function because it returns a slice, but
> > > > > > > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > > > > > > this function copy data into that argument. That way, we could make
> > > > > > > > > the function itself safe. Perhaps the actual copy needs to be
> > > > > > > > > volatile?
> > > > > > > >
> > > > > > > > Why do we consider the existing one unsafe?
> > > > > > > >
> > > > > > > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > > > > > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > > > > > > >
> > > > > > > > And if so, how does this go away with the proposed approach?
> > > > > > >
> > > > > > > In Rust, it is undefined behavior if the value behind an immutable
> > > > > > > reference changes (unless the type uses UnsafeCell / Opaque or
> > > > > > > similar). That is, any two consecutive reads of the same immutable
> > > > > > > reference must return the same byte value no matter what happened in
> > > > > > > between those reads.
> > > > > > >
> > > > > > > If we manually perform the read as a volatile read, then it is
> > > > > > > arguably allowed for the value to be modified by the hardware while we
> > > > > > > read the value.
> > > > > > >
> > > > > >
> > > > > > Do you also assume that volatile read/write provide some sort of
> > > > > > atomicity? Because otherwise even though the read/write may not be
> > > > > > considered as UB, then results can be load/store teared.
> > > > > >
> > > > > > I asked because I think in case that we need atomicity, we should just
> > > > > > use atomic APIs.
> > > > >
> > > > > No, I'm not assuming that. I think it's like uaccess. Under normal
> > > > > cases, it's not going to be concurrently modified, but it shouldn't
> > > > > trigger UB if it is.
> > > > >
> > > >
> > > > Let's say my_alloc[7].foo is a (u64, u64), would
> > > >
> > > >         dma_read!(my_alloc[7].foo)
> > > >
> > > > and
> > > >
> > > >         dma_write!(my_alloc[7].foo, (1u64, 2u64))
> > > >
> > > > trigger any UB when they are concurrent? (Of course, the example here is
> > > > a bit inpropriate because it's DMA buff, but still the question is more
> > > > on whatever atomic expectation we want from read_volatile() and
> > > > write_volatile()).
> > >
> > > I imagine that it would be most convenient for it to not be UB, but I
> > > also don't think people would have an expectation for that to not
> > > involve tearing.
> > >
> >
> > Depending on the granularity that tearing can happen, if .foo is an enum
> > (or any other type that not all bit combinations are valid) and tearing
> > can happen at byte levels, then a racing dma_read() may read invalid
> > data.
> 
> T: FromBytes + ToBytes is already required for these types. You can't
> use these operations with such an enum.
> 

I was talking about a wider problem, but fine ;-) So the assumption is
the read_volatile() or copy_nonoverlapping() provide byte-level
atomicity? Although unlikely, but if tearing happens at sub-byte level,
then even if `T: FromBytes + ToBytes` you can still get invalid data.

> > I think it's fine to expect read_volatile() and write_volatile()
> > themselves don't trigger UB, but we will need to be careful about the
> > atomic granularity that we can expect on them. It would be more clear if
> > we use the atomic API here (and implementation can be read_volatile()
> > and write_volatile()), and it can avoid coding based on tribal knowledge
> > such as "in kernel, read_volatile() and write_volatile() imply atomic".
> 
> Why should we use atomics for operations that don't need to be atomic?
> Most of the time, dma memory is not *actually* changed while you read
> it.
> 

Because the requirement here actually needs atomic at byte level, it's
similar to:

	https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1478r8.html

also, notice that for byte level atomic, it's actually free on most of
the architectures (i.e. no extra cost). Again, it's more of a "how do we
express our assumption" question. If indeed we expect byte-level
atomicity, then I see no harm to use atomic API here.

Regards,
Boqun

> Alice

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

[Boqun Feng]

On Mon, Jan 27, 2025 at 11:21:52AM -0800, Boqun Feng wrote:
[...]
> > > Depending on the granularity that tearing can happen, if .foo is an enum
> > > (or any other type that not all bit combinations are valid) and tearing
> > > can happen at byte levels, then a racing dma_read() may read invalid
> > > data.
> > 
> > T: FromBytes + ToBytes is already required for these types. You can't
> > use these operations with such an enum.
> > 
> 
> I was talking about a wider problem, but fine ;-) So the assumption is
> the read_volatile() or copy_nonoverlapping() provide byte-level
> atomicity? Although unlikely, but if tearing happens at sub-byte level,
> then even if `T: FromBytes + ToBytes` you can still get invalid data.
> 

OK, `FromBytes + ToBytes` should tolerate sub-byte level tearing, so I
was wrong on this.

> > > I think it's fine to expect read_volatile() and write_volatile()
> > > themselves don't trigger UB, but we will need to be careful about the
> > > atomic granularity that we can expect on them. It would be more clear if
> > > we use the atomic API here (and implementation can be read_volatile()
> > > and write_volatile()), and it can avoid coding based on tribal knowledge
> > > such as "in kernel, read_volatile() and write_volatile() imply atomic".
> > 
> > Why should we use atomics for operations that don't need to be atomic?

And yes, given the `FromBytes + ToBytes`, it looks like you don't need
any atomicity for the basic DMA read/write operation.

Regards,
Boqun

> > Most of the time, dma memory is not *actually* changed while you read
> > it.
> > 
> 
> Because the requirement here actually needs atomic at byte level, it's
> similar to:
> 
> 	https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1478r8.html
> 
> also, notice that for byte level atomic, it's actually free on most of
> the architectures (i.e. no extra cost). Again, it's more of a "how do we
> express our assumption" question. If indeed we expect byte-level
> atomicity, then I see no harm to use atomic API here.
> 
> Regards,
> Boqun
> 
> > Alice

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

[Daniel Almeida]

> 
> Why do we consider the existing one unsafe?
> 
> Surely, it's not desirable that the contents of the buffer are modified by the
> HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> 
> And if so, how does this go away with the proposed approach?
> 

Alice pointed out that if you hand out a &mut T, you must ensure exclusive access
to that memory location. So if you pass that address to some piece of hardware and
it writes it, that violates that expectation.

The solution was to simply not do that when the &mut T was alive, i.e. to ensure that
no hardware operation involving that memory was taking place while the slice was still
in use.

Some people then commented that they would use the abstraction for ring buffers, which 
would violate this assumption in perpetuity, as both producer and consumer would have
the range mapped at the same time. 

The solution was to revert back to pointers.

I think Boqun asked for the unsafe as_slice() function as a way to impose the safety check
on users, for the cases where it could be proved through other means that the reference
rules were upheld.

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

[Daniel Almeida]

> 
> You were asked to rename this function because it returns a slice, but
> I wonder if it's better to take an `&mut [T]` argument and to have
> this function copy data into that argument. That way, we could make
> the function itself safe. Perhaps the actual copy needs to be
> volatile?
> 
> Well ... I understand that we did this previously and that we want to
> avoid it because it causes too much reading if T is a struct and we
> just want to read one of its fields. How about an API like this?
> 
> dma_read!(my_alloc[7].foo)
> 
> which expands to something that reads the value of the foo field of
> the 7th element, and
> 
> dma_write!(my_alloc[7].foo = 13);
> 
> That expands to something that writes 13 to field foo of the 7th element.
> 
> Thoughts? I'm proposing this to avoid going in circles between the
> same solutions.
> 
> Alice
> 


I think I missed something here. How is this any different from the *safe*
functions we currently have?

Anything that involves a copy is basically what we have already, although
your version seems to reduce the amount of data copied from sizeof(T) to
sizeof(T.field)?

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

[Alice Ryhl]

On Mon, Jan 27, 2025 at 12:00 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>
> >
> > You were asked to rename this function because it returns a slice, but
> > I wonder if it's better to take an `&mut [T]` argument and to have
> > this function copy data into that argument. That way, we could make
> > the function itself safe. Perhaps the actual copy needs to be
> > volatile?
> >
> > Well ... I understand that we did this previously and that we want to
> > avoid it because it causes too much reading if T is a struct and we
> > just want to read one of its fields. How about an API like this?
> >
> > dma_read!(my_alloc[7].foo)
> >
> > which expands to something that reads the value of the foo field of
> > the 7th element, and
> >
> > dma_write!(my_alloc[7].foo = 13);
> >
> > That expands to something that writes 13 to field foo of the 7th element.
> >
> > Thoughts? I'm proposing this to avoid going in circles between the
> > same solutions.
> >
> > Alice
> >
>
>
> I think I missed something here. How is this any different from the *safe*
> functions we currently have?
>
> Anything that involves a copy is basically what we have already, although
> your version seems to reduce the amount of data copied from sizeof(T) to
> sizeof(T.field)?

The difference is exactly that - you reduce the amount of data copied
to just the size of the field.

Alice

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

[Daniel Almeida]

Alice,

> 
> The difference is exactly that - you reduce the amount of data copied
> to just the size of the field.
> 
> Alice
> 


Is your idea to have that *in addition* to the safe methods?

How does it work when you want to read a T at a time?

— Daniel

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

[Alice Ryhl]

On Tue, Jan 28, 2025 at 11:23 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Alice,
>
> >
> > The difference is exactly that - you reduce the amount of data copied
> > to just the size of the field.
> >
> > Alice
> >
>
>
> Is your idea to have that *in addition* to the safe methods?
>
> How does it work when you want to read a T at a time?

I mean, I imagine that you could make the syntax
`dma_read!(my_alloc[7])` read the entire struct. I'm not sure which
safe methods you are referring to, because right now there is only the
unsafe as_slice().

Alice

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

[Daniel Almeida]

> 
> I mean, I imagine that you could make the syntax
> `dma_read!(my_alloc[7])` read the entire struct. I'm not sure which
> safe methods you are referring to, because right now there is only the
> unsafe as_slice().
> 
> Alice

I mean this:

+    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
+    /// number of bytes.
+    pub fn write(&self, src: &[T], offset: usize) -> Result {
+        if offset + src.len() >= 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(())
+    }
+}

…and the similar read() method that was apparently removed, i.e.:

+    /// Reads 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 data returned should be regarded by the
+    /// caller as a snapshot of the region when this function is called, as 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.
+    pub unsafe fn read(&self, offset: usize, count: usize) -> Result<&[T]> {
+        if offset + count >= 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.
+        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.wrapping_add(offset), count) })
+    }

I don’t think there’s anything wrong with these, so maybe your solution can come in as a convenience
when one wants to save on the amount of copying? IMHO the two methods above co-exist with what
you propose.

The unsafe `as_slice()` and `as_slice_mut()` should also stay, in my opinion. They can be used, for example,
in codec drivers.

— Daniel

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

[Alice Ryhl]

On Tue, Jan 28, 2025 at 11:37 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> >
> > I mean, I imagine that you could make the syntax
> > `dma_read!(my_alloc[7])` read the entire struct. I'm not sure which
> > safe methods you are referring to, because right now there is only the
> > unsafe as_slice().
> >
> > Alice
>
> I mean this:
>
> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> +    /// number of bytes.
> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
> +        if offset + src.len() >= 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(())
> +    }
> +}
>
> …and the similar read() method that was apparently removed, i.e.:
>
> +    /// Reads 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 data returned should be regarded by the
> +    /// caller as a snapshot of the region when this function is called, as 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.
> +    pub unsafe fn read(&self, offset: usize, count: usize) -> Result<&[T]> {
> +        if offset + count >= 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.
> +        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.wrapping_add(offset), count) })
> +    }
>
> I don’t think there’s anything wrong with these, so maybe your solution can come in as a convenience
> when one wants to save on the amount of copying? IMHO the two methods above co-exist with what
> you propose.
>
> The unsafe `as_slice()` and `as_slice_mut()` should also stay, in my opinion. They can be used, for example,
> in codec drivers.

Agreed, there is nothing wrong with the safe write that was here
previously, or the current unsafe functions.

Alice

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

[Daniel Almeida]

Hi Abdiel

I noticed that there’s no API to call `dma_set_mask/dma_set_coherent_mask`.

This should probably be included, i.e.:

```
By default, the kernel assumes that your device can address 32-bits of DMA addressing.
For a 64-bit capable device, this needs to be increased, and for a device with limitations,
it needs to be decreased.
```

— Daniel

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

[Robin Murphy]

On 15/02/2025 9:40 pm, Daniel Almeida wrote:
> Hi Abdiel
> 
> I noticed that there’s no API to call `dma_set_mask/dma_set_coherent_mask`.
> 
> This should probably be included, i.e.:
> 
> ```
> By default, the kernel assumes that your device can address 32-bits of DMA addressing.
> For a 64-bit capable device, this needs to be increased, and for a device with limitations,
> it needs to be decreased.
> ```

Oh, good point (and I'm rather ashamed I missed that!)

FWIW we've been wanting to steer away from relying on the default mask 
in new code, so it would be quite neat to actually enforce that 
allocations fail if dma_coherent_mask hasn't been explicitly set 
(assuming it's sufficiently cheap to keep a flag in the Device handle or 
something like that - it's not the end of the world if it isn't practical).

Thanks,
Robin.

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

[Abdiel Janulgue]

On 17/02/2025 15:52, Robin Murphy wrote:
> On 15/02/2025 9:40 pm, Daniel Almeida wrote:
>> Hi Abdiel
>>
>> I noticed that there’s no API to call `dma_set_mask/ 
>> dma_set_coherent_mask`.
>>
>> This should probably be included, i.e.:
>>
>> ```
>> By default, the kernel assumes that your device can address 32-bits of 
>> DMA addressing.
>> For a 64-bit capable device, this needs to be increased, and for a 
>> device with limitations,
>> it needs to be decreased.
>> ```
> 
> Oh, good point (and I'm rather ashamed I missed that!)


Ok. Missed this part while working on the nova stub and hadn't had to 
use this interface yet, but yes will add this for the next revision.

> 
> FWIW we've been wanting to steer away from relying on the default mask 
> in new code, so it would be quite neat to actually enforce that 
> allocations fail if dma_coherent_mask hasn't been explicitly set 
> (assuming it's sufficiently cheap to keep a flag in the Device handle or 
> something like that - it's not the end of the world if it isn't practical).
> 
Thanks for the heads-up on what this interface is expected to look like!


Regards,
Abdiel

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

[Abdiel Janulgue]

Hi Robin,

On 17/02/2025 15:52, Robin Murphy wrote:
> FWIW we've been wanting to steer away from relying on the default mask 
> in new code, so it would be quite neat to actually enforce that 
> allocations fail if dma_coherent_mask hasn't been explicitly set 
> (assuming it's sufficiently cheap to keep a flag in the Device handle or 
> something like that - it's not the end of the world if it isn't practical).

I just had a quick look on how to possible approach this and indeed 
would refactor the Device binding a bit. If it is okay with you this 
could go in a follow-up patch? I was hoping to upstream the initial 
support first - at least with the dma_set_mask/dma_set_coherent_mask put 
in place already.

Thanks!
/Abdiel

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

[Daniel Almeida]

Hi Abdiel,

> On 18 Feb 2025, at 06:58, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Hi Robin,
> 
> On 17/02/2025 15:52, Robin Murphy wrote:
>> FWIW we've been wanting to steer away from relying on the default mask in new code, so it would be quite neat to actually enforce that allocations fail if dma_coherent_mask hasn't been explicitly set (assuming it's sufficiently cheap to keep a flag in the Device handle or something like that - it's not the end of the world if it isn't practical).
> 
> I just had a quick look on how to possible approach this and indeed would refactor the Device binding a bit. If it is okay with you this could go in a follow-up patch? I was hoping to upstream the initial support first - at least with the dma_set_mask/dma_set_coherent_mask put in place already.
> 
> Thanks!
> /Abdiel
> 

FYI: I don’t mind a follow-up patch. 

— Daniel

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

[Robin Murphy]

On 18/02/2025 12:19 pm, Daniel Almeida wrote:
> Hi Abdiel,
> 
>> On 18 Feb 2025, at 06:58, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
>>
>> Hi Robin,
>>
>> On 17/02/2025 15:52, Robin Murphy wrote:
>>> FWIW we've been wanting to steer away from relying on the default mask in new code, so it would be quite neat to actually enforce that allocations fail if dma_coherent_mask hasn't been explicitly set (assuming it's sufficiently cheap to keep a flag in the Device handle or something like that - it's not the end of the world if it isn't practical).
>>
>> I just had a quick look on how to possible approach this and indeed would refactor the Device binding a bit. If it is okay with you this could go in a follow-up patch? I was hoping to upstream the initial support first - at least with the dma_set_mask/dma_set_coherent_mask put in place already.
>>
>> Thanks!
>> /Abdiel
>>
> 
> FYI: I don’t mind a follow-up patch.

Agreed, as I say it's far from critical anyway - being able to make the 
correct dma_set_coherent_mask() call at all is the only thing we need 
for sure. I just see a slightly selfish opportunity here, that if the 
binding layer could make it impossible to call the API incorrectly then 
hopefully I should never have to review any of its users :D

Cheers,
Robin.

[PATCH v11 3/3] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Abdiel Janulgue]

Add an entry for the Rust dma mapping helpers abstractions.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index baf0eeb9a355..4808c9880b3e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6822,6 +6822,16 @@ 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>
+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

Re: [PATCH v11 3/3] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Robin Murphy]

On 2025-01-23 10:42 am, Abdiel Janulgue wrote:
> Add an entry for the Rust dma mapping helpers abstractions.

Can't really speak for the Rust side of things, but I could be an 
additional reviewer to keep an eye on the DMA API angle if that would help.

Thanks,
Robin.

> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>   MAINTAINERS | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index baf0eeb9a355..4808c9880b3e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6822,6 +6822,16 @@ 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>
> +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>

Re: [PATCH v11 3/3] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Danilo Krummrich]

Hi Robin,

On Thu, Jan 30, 2025 at 11:49:12AM +0000, Robin Murphy wrote:
> On 2025-01-23 10:42 am, Abdiel Janulgue wrote:
> > Add an entry for the Rust dma mapping helpers abstractions.
> 
> Can't really speak for the Rust side of things, but I could be an additional
> reviewer to keep an eye on the DMA API angle if that would help.

I think this is a great idea and it is very welcome and appreciated.

- Danilo

> 
> Thanks,
> Robin.
> 
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >   MAINTAINERS | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index baf0eeb9a355..4808c9880b3e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6822,6 +6822,16 @@ 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>
> > +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>
> 

Re: [PATCH v11 3/3] MAINTAINERS: add entry for Rust dma mapping helpers device driver API

[Abdiel Janulgue]

On 30/01/2025 13:49, Robin Murphy wrote:
> On 2025-01-23 10:42 am, Abdiel Janulgue wrote:
>> Add an entry for the Rust dma mapping helpers abstractions.
> 
> Can't really speak for the Rust side of things, but I could be an 
> additional reviewer to keep an eye on the DMA API angle if that would help.
> 

Yes, would really appreciate your help! Thanks!
Abdiel