Message-Id: <DEOMBKIRDXH6.2CF2MR2RB2W2C@nvidia.com>
Date: Wed, 03 Dec 2025 22:32:58 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alice Ryhl" <aliceryhl@...gle.com>, "Alexandre Courbot"
 <acourbot@...dia.com>
Cc: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <dakr@...nel.org>, <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>,
 <a.hindborg@...nel.org>, <tmgross@...ch.edu>, <markus.probst@...teo.de>,
 <helgaas@...nel.org>, <cjia@...dia.com>, <smitra@...dia.com>,
 <ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
 <targupta@...dia.com>, <joelagnelf@...dia.com>, <jhubbard@...dia.com>,
 <zhiwang@...nel.org>
Subject: Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io
 trait

On Mon Dec 1, 2025 at 9:23 PM JST, Alice Ryhl wrote:
> On Mon, Dec 01, 2025 at 08:57:09PM +0900, Alexandre Courbot wrote:
>> On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
>> > On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> >> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>> >>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>> >>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> >>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> >>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, Zhi Wang wrote:
>> >>> >> >> The previous Io<SIZE> type combined both the generic I/O access helpers
>> >>> >> >> and MMIO implementation details in a single struct.
>> >>> >> >> 
>> >>> >> >> To establish a cleaner layering between the I/O interface and its concrete
>> >>> >> >> backends, paving the way for supporting additional I/O mechanisms in the
>> >>> >> >> future, Io<SIZE> need to be factored.
>> >>> >> >> 
>> >>> >> >> Factor the common helpers into new {Io, Io64} traits, and move the
>> >>> >> >> MMIO-specific logic into a dedicated Mmio<SIZE> type implementing that
>> >>> >> >> trait. Rename the IoRaw to MmioRaw and update the bus MMIO implementations
>> >>> >> >> to use MmioRaw.
>> >>> >> >> 
>> >>> >> >> No functional change intended.
>> >>> >> >> 
>> >>> >> >> Cc: Alexandre Courbot <acourbot@...dia.com>
>> >>> >> >> Cc: Alice Ryhl <aliceryhl@...gle.com>
>> >>> >> >> Cc: Bjorn Helgaas <helgaas@...nel.org>
>> >>> >> >> Cc: Danilo Krummrich <dakr@...nel.org>
>> >>> >> >> Cc: John Hubbard <jhubbard@...dia.com>
>> >>> >> >> Signed-off-by: Zhi Wang <zhiw@...dia.com>
>> >>> >> >
>> >>> >> > I said this on a previous version, but I still don't buy the split
>> >>> >> > into IoFallible and IoInfallible.
>> >>> >> >
>> >>> >> > For one, we're never going to have a method that can accept any Io - we
>> >>> >> > will always want to accept either IoInfallible or IoFallible, so the
>> >>> >> > base Io trait serves no purpose.
>> >>> >> >
>> >>> >> > For another, the docs explain that the distinction between them is
>> >>> >> > whether the bounds check is done at compile-time or runtime. That is not
>> >>> >> > the kind of capability one normally uses different traits to distinguish
>> >>> >> > between. It makes sense to have additional traits to distinguish
>> >>> >> > between e.g.:
>> >>> >> >
>> >>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> >>> >> > * Whether 64-bit IO ops are possible.
>> >>> >> >
>> >>> >> > Well ... I guess one could distinguish between whether it's possible to
>> >>> >> > check bounds at compile-time at all. But if you can check them at
>> >>> >> > compile-time, it should always be possible to check at runtime too, so
>> >>> >> > one should be a sub-trait of the other if you want to distinguish
>> >>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >>> >> >
>> >>> >> > And I'm not really convinced that the current compile-time checked
>> >>> >> > traits are a good idea at all. See:
>> >>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >>> >> >
>> >>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>> >>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>> >>> >> > to only contain integers <= the size. For example, the Bounded integer
>> >>> >> > being added elsewhere.
>> >>> >> 
>> >>> >> Would that be so different from using an associated const value though?
>> >>> >> IIUC the bounded integer type would play the same role, only slightly
>> >>> >> differently - by that I mean that if the offset is expressed by an
>> >>> >> expression that is not const (such as an indexed access), then the
>> >>> >> bounded integer still needs to rely on `build_assert` to be built.
>> >>> >
>> >>> > I mean something like this:
>> >>> >
>> >>> > trait Io {
>> >>> >     const SIZE: usize;
>> >>> >     fn write(&mut self, i: Bounded<Self::SIZE>);
>> >>> > }
>> >>> 
>> >>> I have experimented a bit with this idea, and unfortunately expressing
>> >>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>> >>> not doable as of today.
>> >>> 
>> >>> Bounding an integer with an upper/lower bound also proves to be more
>> >>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>> >>> constants must be of the same type as the wrapped `T` type, which again
>> >>> makes rustc unhappy ("the type of const parameters must not depend on
>> >>> other generic parameters"). A workaround would be to use a macro to
>> >>> define individual types for each integer type we want to support - or to
>> >>> just limit this to `usize`.
>> >>> 
>> >>> But the requirement for generic_const_exprs makes this a non-starter I'm
>> >>> afraid. :/
>> >>
>> >> Can you try this?
>> >>
>> >> trait Io {
>> >>     type IdxInt: Int;
>> >>     fn write(&mut self, i: Self::IdxInt);
>> >> }
>> >>
>> >> then implementers would write:
>> >>
>> >> impl Io for MyIo {
>> >>     type IdxInt = Bounded<17>;
>> >> }
>> >>
>> >> instead of:
>> >> impl Io for MyIo {
>> >>     const SIZE = 17;
>> >> }
>> >
>> > The following builds (using the existing `Bounded` type for
>> > demonstration purposes):
>> >
>> >     trait Io {
>> >         // Type containing an index guaranteed to be valid for this IO.
>> >         type IdxInt: Into<usize>;
>> >
>> >         fn write(&mut self, i: Self::IdxInt);
>> >     }
>> >
>> >     struct FooIo;
>> >
>> >     impl Io for FooIo {
>> >         type IdxInt = Bounded<usize, 8>;
>> >
>> >         fn write(&mut self, i: Self::IdxInt) {
>> >             let idx: usize = i.into();
>> >
>> >             // Now do the IO knowing that `idx` is a valid index.
>> >         }
>> >     }
>> >
>> > That looks promising, and I like how we can effectively use a wider set
>> > of index types - even, say, a `u16` if a particular I/O happens to have
>> > a guaranteed size of 65536!
>> >
>> > I suspect it also changes how we would design the Io interfaces, but I
>> > am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
>> > either unwrapping the result of its fallible methods or using some
>> > `unchecked` accessors?
>> 
>> Mmm, one important point I have neglected is that the index type will
>> have to validate not only the range, but also the alignment of the
>> index! And the valid alignment is dependent on the access width. So
>> getting this right is going to take some time and some experimenting I'm
>> afraid.
>> 
>> Meanwhile, it would be great if we could agree (and proceed) with the
>> split of the I/O interface into a trait, as other work depends on it.
>> Changing the index type of compile-time checked bounds is I think an
>> improvement that is orthogonal to this task.
>
>
>
>> I have been thinking a bit (too much? ^_^;) about the general design for
>> this interface, how it would work with the `register!` macro, and how we
>> could maybe limit the boilerplate. Sorry in advance for this is going to
>> be a long post.
>> 
>> IIUC there are several aspects we need to tackle with the I/O interface:
>> 
>> - Raw IO access. Given an address, perform the IO operation without any
>>   check. Depending on the bus, this might return the data directly (e.g.
>>   `Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
>>   implementation ignores the bus error, which we probably shouldn't.
>>   Also the raw access is reimplemented twice, in both the build-time and
>>   runtime accessors, a fact that is mostly hidden by the use of macros.
>> - Access with dynamic bounds check. This can return `EINVAL` if the
>>   provided index is invalid (out-of-bounds or not aligned), on top of
>>   the bus errors, if any.
>> - Access with build-time index check. Same as above, but the error
>>   occurs at build time if the index is invalid. Otherwise the return
>>   type of the raw IO accessor is returned.
>> 
>> At the moment we have two traits for build-time and runtime index
>> checks. What bothers me a bit about them is that they basically
>> re-implement the same raw I/O accessors. This strongly hints that we
>> should implement the raw accessors as a base trait, which the
>> user-facing API would call into:
>> 
>>   pub trait Io {
>>       /// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
>>       type Error: Into<Error>;
>> 
>>       /// Returns the base address of this mapping.
>>       fn addr(&self) -> usize;
>> 
>>       /// Returns the maximum size of this mapping.
>>       fn maxsize(&self) -> usize;
>> 
>>       unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
>>       unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
>>       // etc. for 16/32 bits accessors.
>>   }
>> 
>> Then we could build the current `IoFallible` trait on top of it:
>> 
>>   pub trait IoFallible: Io {
>>       fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>>           if !offset_valid::<U>(offset, self.maxsize()) {
>>               return Err(EINVAL);
>>           }
>> 
>>           self.addr().checked_add(offset).ok_or(EINVAL)
>>       }
>> 
>>       /// 8-bit read with runtime bounds check.
>>       fn try_read8_checked(&self, offset: usize) -> Result<u8> {
>>           let addr = self.io_addr::<u8>(offset)?;
>> 
>>           unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
>>       }
>> 
>>       /// 8-bit write with runtime bounds check.
>>       fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
>>           let addr = self.io_addr::<u8>(offset)?;
>> 
>>           unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
>>       }
>>   }
>> 
>> Note how this trait is now auto-implemented. Making it available for all
>> implementers of `Io` is as simple as:
>> 
>>   impl<IO: Io> IoFallible for IO {}
>> 
>> (... which hints that maybe it should simply be folded into `Io`, as
>> Alice previously suggested)
>
> Yes, it probably should. At the very least, it should be an extension
> trait, which means that it should never be used in trait bounds, since
> T: IoFallible is equivalent to T: Io. But in this case, probably just
> fold it into Io.

Agreed, I don't see any benefit in keeping these separate although I
remember Danilo prefering to keep them parted.

<snip>
>> How does this sound? I can share a branch with a basic implementation
>> of this idea if that helps.
>
> My main thoughts are:
>
> First, we need to think some more about the naming. Currently you have
> several methods with the same name. For example, you have a read8 method
> implemented in terms of a different read8 method. It'd be nice with a
> summary of the meaning of:
>
> * try_ prefix
> * _unchecked suffix
> * _checked suffix (not the same as try_ prefix?)

I guess it shows that I quickly tried to adopt a naming pattern as I was
writing my email. :)

The logic was roughly:

- `_unchecked` suffix for unsafe ops, like the standard library,
- `try_` prefix if the accessor returns an bus error,
- `_checked` suffix if the bounds are checked at run-time by the
  accessor.

So `try_read8_checked` checks the bounds at runtime and returns a
`Result`, `try_read8` checks the bounds at build time and returns a
`Result` which only errors are bus ones, and `read8` is for the case
where the bus' error type is infallible, and we can know at build time
that no error will ever be returned.

... this naming pattern can probably be improved.

> Second, I think we need to think a bit more about the error types.
> Perhaps the trait could define:
>
> /// Error type used by `*_unchecked` methods.
> type Error;
>
> /// Error type that can be either `Self::Error` or a failed bounds
> /// check.
> type TryError: From<Self::Error> + From<BoundsError>;
>
> where BoundsError is a new zero-sized error type we can define
> somewhere. This way, Mmio can use these errors:
>
> type Error = Infallible;
> type TryError = BoundsError;
>
> wheres cases that can fail with an IO error can use kernel::error::Error
> for both cases.

I thought we could roughly achieve this by using the regular kernel
`Error` as the type returned by all accessors - does this extra
associated type bring something more?

> Third, if we're going to postpone the custom integer type for
> IoKnownSize, then I think we should get rid of build-checked IO ops
> entirely for now.

That would be a pretty disruptive change. I dread the Nova patch that
would be required. ^_^;

I'd also say it looks out-of-scope for this change? Zhi's goal is just
to extract the Io API into a trait, and this design doesn't
fundamentally change it, nor does it adds the build-checked IO ops -
they are already here, and this doesn't make the situation worse. I
agree this front should be improved, but that's a different effort.

Should we drop these ops, every single register access in Nova would
become fallible, and we would need to add error handling code for errors
we know for a fact cannot happen. I'd really like to avoid that.

> Fourth, I didn't know about the alignment requirement. I would like to
> know how that fits in with the rest of this. Is it treated like a bounds
> check? That could make sense, and we could also have a custom integer
> type that both has a max value and alignment invariant. But what about
> the *_unchecked and runtime bounds-checked methods?

Basically we would need the custom integer type to hold as invariant
that the current `offset_valid` is always true:

  const fn offset_valid<U>(offset: usize, size: usize) -> bool {
      let type_size = core::mem::size_of::<U>();
      if let Some(end) = offset.checked_add(type_size) {
          end <= size && offset % type_size == 0
      } else {
          false
      }
  }

I initially thought that we could maybe turn this into a class of
primitive types for which a custom invariant to hold could be passed as
e.g. a closure, but of course it's not that simple. I'm still hopeful we
can reach a breakthrough somehow though.

Powered by blists - more mailing lists