Re: [PATCH v2 11/11] rust: io: add copying methods

["Gary Guo" <gary@garyguo.net>]

On Tue Apr 28, 2026 at 2:22 PM BST, Andreas Hindborg wrote:
> Gary Guo <gary@garyguo.net> writes:
>
>> One feature that was lost from the old `dma_read!()` and `dma_write!()`
>> when moving to `io_read!()` and `io_write!()` was the ability to read/write
>> a large structs. However, the semantics was unclear to begin with, as there
>> was no guarantee about their atomicity even for structs that were small
>> enough to fit in u32. Re-introduces the capability in the form of copying
>> methods.
>>
>>     dma_read!(foo, bar) -> io_project!(foo, bar).copy_read()
>>     dma_write!(foo, bar, baz) -> io_project!(foo, bar).copy_write(baz)
>>
>> The semantics for these are modelled after memcpy so user has clear
>> expectation of lack of atomicity. As an additional benefit of this change,
>> this now works for MMIO as well, which maps to `memcpy_{from,to}io`.
>>
>> For slices, which is unsized so the API above can't work, `copy_from_slice`
>> and `copy_to_slice` were added to copy from/to normal memory, and
>> `copy_from_io_slice` and `copy_to_io_slice` were added to copy from/to
>> other `Io` regions. They're optimized if at least one end is mapped to
>> system memory; if none are, the copy occurs with an intermediate stack
>> buffer.
>>
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>> ---
>>  rust/kernel/dma.rs |   8 +-
>>  rust/kernel/io.rs  | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 238 insertions(+), 1 deletion(-)
>>
>> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
>> index bbdeb117c145..307f5769ca0a 100644
>> --- a/rust/kernel/dma.rs
>> +++ b/rust/kernel/dma.rs
>> @@ -16,7 +16,8 @@
>>      fs::file,
>>      io::{
>>          Io,
>> -        IoCapable, //
>> +        IoCapable,
>> +        IoCopyable, //
>>      },
>>      prelude::*,
>>      ptr::KnownSize,
>> @@ -997,6 +998,11 @@ unsafe fn io_write(&self, value: $ty, address: *mut $ty) {
>>      u64
>>  );
>>  
>> +// SAFETY: `Coherent` is mapped to CPU address space.
>> +unsafe impl<T: ?Sized + KnownSize> IoCopyable for Coherent<T> {
>> +    const IS_MAPPED: bool = true;
>> +}
>> +
>>  impl<'a, B: ?Sized + KnownSize, T: ?Sized> crate::io::View<'a, Coherent<B>, T> {
>>      /// Returns a DMA handle which may be given to the device as the DMA address base of
>>      /// the region.
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index efcd7e6741d7..0b1ed68c0f9b 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -4,6 +4,8 @@
>>  //!
>>  //! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
>>  
>> +use core::mem::MaybeUninit;
>> +
>>  use crate::{
>>      bindings,
>>      prelude::*,
>> @@ -233,6 +235,55 @@ pub trait IoCapable<T> {
>>      unsafe fn io_write(&self, value: T, address: *mut T);
>>  }
>>  
>> +/// Trait indicating that an I/O backend supports memory copy operations.
>> +///
>> +/// # Safety
>> +///
>> +/// If [`IS_MAPPED`] is overridden to true, it must be correct per documentation.
>> +pub unsafe trait IoCopyable {
>> +    /// Whether the pointers for this I/O backend are in the CPU address space, and are coherently
>> +    /// mapped.
>> +    ///
>> +    /// When this is true, it means that memory can be accessed with byte-wise atomic memory copy.
>> +    const IS_MAPPED: bool = false;
>> +
>> +    /// Copy `size` bytes from `address` to `buffer`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - The range `[address..address + size]` must be within the bounds of `Self`.
>
> We should probably specify what "bounds of `Self`" means here. It's not
> the bounds of `Self`, it is the bounds of the memory region `Self`
> represents.

I just copied the wording for `IoCapable` methods. That said, I think it's worth
improving.

>
>> +    /// - `buffer` is valid for write for `size` bytes.
>> +    #[inline]
>> +    unsafe fn copy_from_io(&self, address: *mut u8, buffer: *mut u8, size: usize) {
>> +        const_assert!(Self::IS_MAPPED);
>> +
>> +        // Use `bindings::memcpy` instead of copy_nonoverlapping for volatile.
>> +        // SAFETY:
>> +        // - `buffer` is valid for write for `size` bytes.
>> +        // - `IS_MAPPED` guarantees `address` is in CPU address space, with safety requirements
>> +        //   `address` is valid for read for `size` bytes.
>> +        unsafe { bindings::memcpy(buffer.cast(), address.cast(), size) };
>> +    }
>
> You could just leave out the default impl and implement each case for
> `Coherent<_>` and `Mmio<_>`. Do you expect more implementers that can
> share the default impl?

`copy_from_io_slice` and `copy_to_io_slice` needs `IS_MAPPED`, and also the
optimization of `copy_read` / `copy_write` too.

Given that the semantics of `IS_MAPPED` ensures memcpy is always okay, so I
think it's better to just define it.

The implementation of sys mem would also just use memcpy.

>
>> +
>> +    /// Copy `size` bytes from `buffer` to `address`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - The range `[address..address + size]` must be within the bounds of `Self`.
>> +    /// - `buffer` is valid for read for `size` bytes.
>> +    #[inline]
>> +    unsafe fn copy_to_io(&self, address: *mut u8, buffer: *const u8, size: usize) {
>> +        const_assert!(Self::IS_MAPPED);
>> +
>> +        // Use `bindings::memcpy` instead of copy_nonoverlapping for volatile.
>> +        // SAFETY:
>> +        // - `IS_MAPPED` guarantees `address` is in CPU address space, with safety requirements
>> +        //   `address` is valid for write for `size` bytes.
>> +        // - `buffer` is valid for read for `size` bytes.
>> +        unsafe { bindings::memcpy(address.cast(), buffer.cast(), size) };
>> +    }
>> +}
>> +
> [snip]
>> +
>> +impl<IO: ?Sized + Io + IoCopyable> View<'_, IO, [u8]> {
>> +    /// Copy bytes from slice to I/O memory.
>> +    #[inline]
>> +    pub fn copy_from_slice(self, data: &[u8]) {
>> +        assert_eq!(self.len(), data.len());
>
> Do you really want a panic here?

It's the same for core's slice::copy_from_slice.

Best,
Gary