Re: [PATCH v4 09/20] rust: io: use view types instead of addresses for `Io`

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

On Tue Jun 16, 2026 at 3:05 PM BST, Alexandre Courbot wrote:
> On Fri Jun 12, 2026 at 1:28 AM JST, Gary Guo wrote:
>> Currently, `io_read` and `io_write` methods require the exact type of `Io`
>> plus an address. This means that they need to be monomorphized for each
>> different `Io` instance. This also means that multiple I/O implementors for
>> the same I/O kind needs to duplicate implementation (e.g. `Mmio` and
>> `MmioOwned`).
>>
>> Create a new `IoBackend` trait and define these operations on it instead.
>> The operations are just going to receive a view type and operate on them.
>> This has the additional advantage that the invariants can be moved from the
>> trait (and guaranteed via `unsafe`) to type invariants on the canonical
>> view types of the backends, so `io_read` and `io_write` can be safe.
>>
>> Note that view type is needed; addresses are insufficient in this
>> designk, as they do not carry sufficient information. For example,
>
> typo: design
>
>> `ConfigSpace` needs `&pci::Device` in addition to the address.
>>
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>> ---
>>  rust/kernel/io.rs     | 345 ++++++++++++++++++++++++++------------------------
>>  rust/kernel/pci/io.rs |  70 ++++++----
>>  2 files changed, 224 insertions(+), 191 deletions(-)
>>
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index 3ac8b396f5a7..e422a5ff2a5e 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -244,6 +244,38 @@ const fn offset_valid<U>(base: usize, offset: usize, size: usize) -> bool {
>>      }
>>  }
>>  
>> +/// I/O backends.
>> +///
>> +/// This is an abstract representation to be implemented by arbitrary I/O
>> +/// backends (e.g. MMIO, PCI config space, etc.).
>> +///
>> +/// The base trait only defines the projection operations; which I/O methods are available depends
>> +/// on which [`IoCapable<T>`] traits are implemented for the type. For example, for MMIO regions,
>> +/// all widths (u8, u16, u32, and u64 on 64-bit systems) are typically supported. For PCI
>> +/// configuration space, u8, u16, and u32 are supported but u64 is not.
>> +///
>> +/// This trait is separate from the `Io` trait as multiple different I/O types may share the same
>> +/// operation.
>> +pub trait IoBackend {
>> +    /// View type for this I/O backend.
>> +    type View<'a, T: ?Sized + KnownSize>: Io<'a, Backend = Self, Target = T>;
>> +
>> +    /// Convert a `view` to a raw pointer for projection.
>> +    fn as_ptr<'a, T: ?Sized + KnownSize>(view: Self::View<'a, T>) -> *mut T;
>
> Same as the previous patch, this pointer is not necessarily
> dereferencable (e.g. for `pci::ConfigSpace`). This should probably be
> mentioned, or maybe we can use a newtype to prevent dereferencing?

This needs to be a pointer for pointer projection to work. I think the fact
they're raw pointers is a very good indication that they can realisticly be
anything; but I can add a note like

    /// Convert a `view` to a raw pointer for projection.
    ///
    /// The returned pointer is private implementation detail of the backend;
    /// it is likely not valid. It should be used for projection only.

>> +pub trait Io<'a>: Copy {
>> +    /// Type that defines all I/O operations.
>> +    type Backend: IoBackend;
>> +
>>      /// Type of this I/O region. For untyped regions, [`Region`] can be used.
>>      type Target: ?Sized + KnownSize;
>>  
>> -    /// Returns the base address of this mapping.
>> -    fn addr(self) -> usize;
>> -
>> -    /// Returns the maximum size of this mapping.
>> -    fn maxsize(self) -> usize;
>> +    /// Return a view that covers the full region.
>> +    fn as_view(self) -> <Self::Backend as IoBackend>::View<'a, Self::Target>;
>>  
>> -    /// Returns the absolute I/O address for a given `offset`,
>> -    /// performing compile-time bound checks.
>> +    /// Returns a view for a given `offset`, performing compile-time bound checks.
>>      // Always inline to optimize out error path of `build_assert`.
>>      #[inline(always)]
>> -    fn io_addr_assert<U>(self, offset: usize) -> usize {
>> -        // We cannot check alignment with `offset_valid` using `self.addr()`. So set 0 for it and
>> +    fn io_addr_assert<U>(self, offset: usize) -> <Self::Backend as IoBackend>::View<'a, U> {
>
> Since this doesn't return an address anymore, should it be renamed?
>
>> +        // We cannot check alignment with `offset_valid` using `ptr.addr()`. So set 0 for it and
>>          // ensure alignment by checking that the alignment of `U` is smaller or equal to the
>>          // alignment of `Self::Target`.
>>          const_assert!(Alignment::of::<U>().as_usize() <= Self::Target::MIN_ALIGN.as_usize());
>>          build_assert!(offset_valid::<U>(0, offset, Self::Target::MIN_SIZE));
>>  
>> -        self.addr() + offset
>> +        let view = self.as_view();
>> +        let ptr = Self::Backend::as_ptr(view);
>> +        let projected_ptr = ptr.cast::<U>().wrapping_byte_add(offset);
>> +        // SAFETY: `offset_valid` checks for size and alignment and therefore `projected_ptr` is a
>> +        // valid projection.
>> +        unsafe { Self::Backend::project_view(view, projected_ptr) }
>>      }
>>  
>> -    /// Returns the absolute I/O address for a given `offset`,
>> -    /// performing runtime bound checks.
>> +    /// Returns a view for a given `offset`, performing runtime bound checks.
>>      #[inline]
>> -    fn io_addr<U>(self, offset: usize) -> Result<usize> {
>> -        if !offset_valid::<U>(self.addr(), offset, self.maxsize()) {
>> +    fn io_addr<U>(self, offset: usize) -> Result<<Self::Backend as IoBackend>::View<'a, U>> {
>
> Same here.
>
> And potentially, a more serious issue: `io_addr_assert` and `io_addr`
> remain part of `Io`, which is a public trait. They only verify size and
> alignment for `U`, not whether a projection of `U` at `offset` is
> structurally valid. AFAICT this remains that way by the end of the
> series, so users are able to call `io_addr*` to create and use invalid
> projections.

I think this could be solved by adding `U: FromBytes + IntoBytes`? We're only
using it form primitives anyway.

Technically, even without the bound, nothing can cause any breakage anyway,
given even after the full series to do things with the view you'd need to use
`copy_read`/`copy_write` which themselves carry the `FromBytes`/`IntoBytes`
bound.

Best,
Gary

>
> Moving `io_addr*` out of the trait and into local helpers should be
> enough to close that loophole.
>
> Also, (and not entirely sure of it because I haven't completely wrapped
> my head around the issue yet), we might need to seal or otherwise
> restrict `IoLoc` so external code cannot create arbitrary
> implementations that allow invalid projections.