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

Gary Guo
2026-04-28T14:08:48Z

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, 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