Re: [PATCH v2 1/6] rust: io: turn IoCapable into a functional trait

[lyude@redhat.com]

On Fri, 2026-02-06 at 15:00 +0900, Alexandre Courbot wrote:
> `IoCapable<T>` is currently used as a marker trait to signal that the
> methods of the `Io` trait corresponding to `T` have been overridden
> by
> the implementor (the default implementations triggering a build-time
> error).
> 
> This goes against the DRY principle and separates the signaling of
> the
> capability from its implementation, making it possible to forget a
> step
> while implementing a new `Io`.

I realized another thing that might be missing from this series while
porting the iosys_map code over: it seems like IoCapable is fine being
a safe trait, but Io and IoKnownSize both seem like they should
actually be unsafe traits. The main reason being that IoCapable doesn't
actually provide any methods that provide a guarantee of being able to
read/write from the IO space, but Io does - and IoKnownSize is making
the guarantee the IO space is at least of size IoKnownSize::MIN_SIZE.

> 
> Another undesirable side-effect is that it makes the implementation
> of
> I/O backends boilerplate-y and convoluted: currently this is done
> using
> two levels of imbricated macros that generate unsafe code.
> 
> Fix these issues by turning `IoCapable` into a functional trait that
> includes the raw implementation of the I/O access for `T` using
> unsafe methods that work with an arbitrary address.
> 
> This allows us to turn the default methods of `Io` into regular
> methods
> that check the passed offset, turn it into an address, and call into
> the
> corresponding `IoCapable` functions, removing the need to overload
> them
> at all.
> 
> `IoCapable` must still be implemented for all supported primitive
> types,
> which is still done more concisely using a macro, but this macro
> becomes
> much simpler and does not require calling into another one.
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Acked-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  rust/kernel/io.rs     | 169 ++++++++++++++++++++++++++++++++++++++--
> ----------
>  rust/kernel/pci/io.rs |  37 ++++++++++-
>  2 files changed, 163 insertions(+), 43 deletions(-)
> 
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index c1cca7b438c3..dc894a45bbcc 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -273,14 +273,29 @@ const fn offset_valid<U>(offset: usize, size:
> usize) -> bool {
>      }
>  }
>  
> -/// Marker trait indicating that an I/O backend supports operations
> of a certain type.
> +/// Trait indicating that an I/O backend supports operations of a
> certain type and providing an
> +/// implementation for these operations.
>  ///
>  /// Different I/O backends can implement this trait to expose only
> the operations they support.
>  ///
>  /// For example, a PCI configuration space may implement
> `IoCapable<u8>`, `IoCapable<u16>`,
>  /// and `IoCapable<u32>`, but not `IoCapable<u64>`, while an MMIO
> region on a 64-bit
>  /// system might implement all four.
> -pub trait IoCapable<T> {}
> +pub trait IoCapable<T> {
> +    /// Performs an I/O read of type `T` at `address` and returns
> the result.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The range `[address..address + size_of::<T>()]` must be
> within the bounds of `Self`.
> +    unsafe fn io_read(&self, address: usize) -> T;
> +
> +    /// Performs an I/O write of `value` at `address`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The range `[address..address + size_of::<T>()]` must be
> within the bounds of `Self`.
> +    unsafe fn io_write(&self, value: T, address: usize);
> +}
>  
>  /// Types implementing this trait (e.g. MMIO BARs or PCI config
> regions)
>  /// can perform I/O operations on regions of memory.
> @@ -322,146 +337,198 @@ fn io_addr<U>(&self, offset: usize) ->
> Result<usize> {
>  
>      /// Fallible 8-bit read with runtime bounds check.
>      #[inline(always)]
> -    fn try_read8(&self, _offset: usize) -> Result<u8>
> +    fn try_read8(&self, offset: usize) -> Result<u8>
>      where
>          Self: IoCapable<u8>,
>      {
> -        build_error!("Backend does not support fallible 8-bit read")
> +        let address = self.io_addr::<u8>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        Ok(unsafe { self.io_read(address) })
>      }
>  
>      /// Fallible 16-bit read with runtime bounds check.
>      #[inline(always)]
> -    fn try_read16(&self, _offset: usize) -> Result<u16>
> +    fn try_read16(&self, offset: usize) -> Result<u16>
>      where
>          Self: IoCapable<u16>,
>      {
> -        build_error!("Backend does not support fallible 16-bit
> read")
> +        let address = self.io_addr::<u16>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        Ok(unsafe { self.io_read(address) })
>      }
>  
>      /// Fallible 32-bit read with runtime bounds check.
>      #[inline(always)]
> -    fn try_read32(&self, _offset: usize) -> Result<u32>
> +    fn try_read32(&self, offset: usize) -> Result<u32>
>      where
>          Self: IoCapable<u32>,
>      {
> -        build_error!("Backend does not support fallible 32-bit
> read")
> +        let address = self.io_addr::<u32>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        Ok(unsafe { self.io_read(address) })
>      }
>  
>      /// Fallible 64-bit read with runtime bounds check.
>      #[inline(always)]
> -    fn try_read64(&self, _offset: usize) -> Result<u64>
> +    fn try_read64(&self, offset: usize) -> Result<u64>
>      where
>          Self: IoCapable<u64>,
>      {
> -        build_error!("Backend does not support fallible 64-bit
> read")
> +        let address = self.io_addr::<u64>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        Ok(unsafe { self.io_read(address) })
>      }
>  
>      /// Fallible 8-bit write with runtime bounds check.
>      #[inline(always)]
> -    fn try_write8(&self, _value: u8, _offset: usize) -> Result
> +    fn try_write8(&self, value: u8, offset: usize) -> Result
>      where
>          Self: IoCapable<u8>,
>      {
> -        build_error!("Backend does not support fallible 8-bit
> write")
> +        let address = self.io_addr::<u8>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        unsafe { self.io_write(value, address) };
> +        Ok(())
>      }
>  
>      /// Fallible 16-bit write with runtime bounds check.
>      #[inline(always)]
> -    fn try_write16(&self, _value: u16, _offset: usize) -> Result
> +    fn try_write16(&self, value: u16, offset: usize) -> Result
>      where
>          Self: IoCapable<u16>,
>      {
> -        build_error!("Backend does not support fallible 16-bit
> write")
> +        let address = self.io_addr::<u16>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        unsafe { self.io_write(value, address) };
> +        Ok(())
>      }
>  
>      /// Fallible 32-bit write with runtime bounds check.
>      #[inline(always)]
> -    fn try_write32(&self, _value: u32, _offset: usize) -> Result
> +    fn try_write32(&self, value: u32, offset: usize) -> Result
>      where
>          Self: IoCapable<u32>,
>      {
> -        build_error!("Backend does not support fallible 32-bit
> write")
> +        let address = self.io_addr::<u32>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        unsafe { self.io_write(value, address) };
> +        Ok(())
>      }
>  
>      /// Fallible 64-bit write with runtime bounds check.
>      #[inline(always)]
> -    fn try_write64(&self, _value: u64, _offset: usize) -> Result
> +    fn try_write64(&self, value: u64, offset: usize) -> Result
>      where
>          Self: IoCapable<u64>,
>      {
> -        build_error!("Backend does not support fallible 64-bit
> write")
> +        let address = self.io_addr::<u64>(offset)?;
> +
> +        // SAFETY: `address` has been validated by `io_addr`.
> +        unsafe { self.io_write(value, address) };
> +        Ok(())
>      }
>  
>      /// Infallible 8-bit read with compile-time bounds check.
>      #[inline(always)]
> -    fn read8(&self, _offset: usize) -> u8
> +    fn read8(&self, offset: usize) -> u8
>      where
>          Self: IoKnownSize + IoCapable<u8>,
>      {
> -        build_error!("Backend does not support infallible 8-bit
> read")
> +        let address = self.io_addr_assert::<u8>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_read(address) }
>      }
>  
>      /// Infallible 16-bit read with compile-time bounds check.
>      #[inline(always)]
> -    fn read16(&self, _offset: usize) -> u16
> +    fn read16(&self, offset: usize) -> u16
>      where
>          Self: IoKnownSize + IoCapable<u16>,
>      {
> -        build_error!("Backend does not support infallible 16-bit
> read")
> +        let address = self.io_addr_assert::<u16>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_read(address) }
>      }
>  
>      /// Infallible 32-bit read with compile-time bounds check.
>      #[inline(always)]
> -    fn read32(&self, _offset: usize) -> u32
> +    fn read32(&self, offset: usize) -> u32
>      where
>          Self: IoKnownSize + IoCapable<u32>,
>      {
> -        build_error!("Backend does not support infallible 32-bit
> read")
> +        let address = self.io_addr_assert::<u32>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_read(address) }
>      }
>  
>      /// Infallible 64-bit read with compile-time bounds check.
>      #[inline(always)]
> -    fn read64(&self, _offset: usize) -> u64
> +    fn read64(&self, offset: usize) -> u64
>      where
>          Self: IoKnownSize + IoCapable<u64>,
>      {
> -        build_error!("Backend does not support infallible 64-bit
> read")
> +        let address = self.io_addr_assert::<u64>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_read(address) }
>      }
>  
>      /// Infallible 8-bit write with compile-time bounds check.
>      #[inline(always)]
> -    fn write8(&self, _value: u8, _offset: usize)
> +    fn write8(&self, value: u8, offset: usize)
>      where
>          Self: IoKnownSize + IoCapable<u8>,
>      {
> -        build_error!("Backend does not support infallible 8-bit
> write")
> +        let address = self.io_addr_assert::<u8>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_write(value, address) }
>      }
>  
>      /// Infallible 16-bit write with compile-time bounds check.
>      #[inline(always)]
> -    fn write16(&self, _value: u16, _offset: usize)
> +    fn write16(&self, value: u16, offset: usize)
>      where
>          Self: IoKnownSize + IoCapable<u16>,
>      {
> -        build_error!("Backend does not support infallible 16-bit
> write")
> +        let address = self.io_addr_assert::<u16>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_write(value, address) }
>      }
>  
>      /// Infallible 32-bit write with compile-time bounds check.
>      #[inline(always)]
> -    fn write32(&self, _value: u32, _offset: usize)
> +    fn write32(&self, value: u32, offset: usize)
>      where
>          Self: IoKnownSize + IoCapable<u32>,
>      {
> -        build_error!("Backend does not support infallible 32-bit
> write")
> +        let address = self.io_addr_assert::<u32>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_write(value, address) }
>      }
>  
>      /// Infallible 64-bit write with compile-time bounds check.
>      #[inline(always)]
> -    fn write64(&self, _value: u64, _offset: usize)
> +    fn write64(&self, value: u64, offset: usize)
>      where
>          Self: IoKnownSize + IoCapable<u64>,
>      {
> -        build_error!("Backend does not support infallible 64-bit
> write")
> +        let address = self.io_addr_assert::<u64>(offset);
> +
> +        // SAFETY: `address` has been validated by `io_addr_assert`.
> +        unsafe { self.io_write(value, address) }
>      }
>  }
>  
> @@ -487,14 +554,36 @@ fn io_addr_assert<U>(&self, offset: usize) ->
> usize {
>      }
>  }
>  
> -// MMIO regions support 8, 16, and 32-bit accesses.
> -impl<const SIZE: usize> IoCapable<u8> for Mmio<SIZE> {}
> -impl<const SIZE: usize> IoCapable<u16> for Mmio<SIZE> {}
> -impl<const SIZE: usize> IoCapable<u32> for Mmio<SIZE> {}
> +/// Implements [`IoCapable`] on `$mmio` for `$ty` using `$read_fn`
> and `$write_fn`.
> +macro_rules! impl_mmio_io_capable {
> +    ($mmio:ident, $(#[$attr:meta])* $ty:ty, $read_fn:ident,
> $write_fn:ident) => {
> +        $(#[$attr])*
> +        impl<const SIZE: usize> IoCapable<$ty> for $mmio<SIZE> {
> +            unsafe fn io_read(&self, address: usize) -> $ty {
> +                // SAFETY: By the trait invariant `address` is a
> valid address for MMIO operations.
> +                unsafe { bindings::$read_fn(address as *const
> c_void) }
> +            }
>  
> +            unsafe fn io_write(&self, value: $ty, address: usize) {
> +                // SAFETY: By the trait invariant `address` is a
> valid address for MMIO operations.
> +                unsafe { bindings::$write_fn(value, address as *mut
> c_void) }
> +            }
> +        }
> +    };
> +}
> +
> +// MMIO regions support 8, 16, and 32-bit accesses.
> +impl_mmio_io_capable!(Mmio, u8, readb, writeb);
> +impl_mmio_io_capable!(Mmio, u16, readw, writew);
> +impl_mmio_io_capable!(Mmio, u32, readl, writel);
>  // MMIO regions on 64-bit systems also support 64-bit accesses.
> -#[cfg(CONFIG_64BIT)]
> -impl<const SIZE: usize> IoCapable<u64> for Mmio<SIZE> {}
> +impl_mmio_io_capable!(
> +    Mmio,
> +    #[cfg(CONFIG_64BIT)]
> +    u64,
> +    readq,
> +    writeq
> +);
>  
>  impl<const SIZE: usize> Io for Mmio<SIZE> {
>      /// Returns the base address of this mapping.
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 6ca4cf75594c..8c8aab2e3f22 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -142,10 +142,41 @@ macro_rules! call_config_write {
>      };
>  }
>  
> +/// Implements [`IoCapable`] on [`ConfigSpace`] for `$ty` using
> `$read_fn` and `$write_fn`.
> +macro_rules! impl_config_space_io_capable {
> +    ($ty:ty, $read_fn:ident, $write_fn:ident) => {
> +        impl<'a, S: ConfigSpaceKind> IoCapable<$ty> for
> ConfigSpace<'a, S> {
> +            unsafe fn io_read(&self, address: usize) -> $ty {
> +                let mut val: $ty = 0;
> +
> +                // Return value from C function is ignored in
> infallible accessors.
> +                let _ret =
> +                    // SAFETY: By the type invariant `self.pdev` is
> a valid address.
> +                    // CAST: The offset is cast to `i32` because the
> C functions expect a 32-bit
> +                    // signed offset parameter. PCI configuration
> space size is at most 4096 bytes,
> +                    // so the value always fits within `i32` without
> truncation or sign change.
> +                    unsafe { bindings::$read_fn(self.pdev.as_raw(),
> address as i32, &mut val) };
> +
> +                val
> +            }
> +
> +            unsafe fn io_write(&self, value: $ty, address: usize) {
> +                // Return value from C function is ignored in
> infallible accessors.
> +                let _ret =
> +                    // SAFETY: By the type invariant `self.pdev` is
> a valid address.
> +                    // CAST: The offset is cast to `i32` because the
> C functions expect a 32-bit
> +                    // signed offset parameter. PCI configuration
> space size is at most 4096 bytes,
> +                    // so the value always fits within `i32` without
> truncation or sign change.
> +                    unsafe { bindings::$write_fn(self.pdev.as_raw(),
> address as i32, value) };
> +            }
> +        }
> +    };
> +}
> +
>  // PCI configuration space supports 8, 16, and 32-bit accesses.
> -impl<'a, S: ConfigSpaceKind> IoCapable<u8> for ConfigSpace<'a, S> {}
> -impl<'a, S: ConfigSpaceKind> IoCapable<u16> for ConfigSpace<'a, S>
> {}
> -impl<'a, S: ConfigSpaceKind> IoCapable<u32> for ConfigSpace<'a, S>
> {}
> +impl_config_space_io_capable!(u8, pci_read_config_byte,
> pci_write_config_byte);
> +impl_config_space_io_capable!(u16, pci_read_config_word,
> pci_write_config_word);
> +impl_config_space_io_capable!(u32, pci_read_config_dword,
> pci_write_config_dword);
>  
>  impl<'a, S: ConfigSpaceKind> Io for ConfigSpace<'a, S> {
>      /// Returns the base address of the I/O region. It is always 0
> for configuration space.