Re: [PATCH v3 09/16] rust: add `io::Io` base type

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Oct 28, 2024 at 04:43:02PM +0100, Alice Ryhl wrote:
> On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > I/O memory is typically either mapped through direct calls to ioremap()
> > or subsystem / bus specific ones such as pci_iomap().
> >
> > Even though subsystem / bus specific functions to map I/O memory are
> > based on ioremap() / iounmap() it is not desirable to re-implement them
> > in Rust.
> >
> > Instead, implement a base type for I/O mapped memory, which generically
> > provides the corresponding accessors, such as `Io::readb` or
> > `Io:try_readb`.
> >
> > `Io` supports an optional const generic, such that a driver can indicate
> > the minimal expected and required size of the mapping at compile time.
> > Correspondingly, calls to the 'non-try' accessors, support compile time
> > checks of the I/O memory offset to read / write, while the 'try'
> > accessors, provide boundary checks on runtime.
> 
> And using zero works because the user then statically knows that zero
> bytes are available ... ?

Zero would mean that the (minimum) resource size is unknown at compile time.
Correspondingly, any call to `read` and `write` would not compile, since the
compile time check requires that `offset + type_size <= SIZE`.

(Hope this answers the questions, I'm not sure I got it correctly.)

> 
> > `Io` is meant to be embedded into a structure (e.g. pci::Bar or
> > io::IoMem) which creates the actual I/O memory mapping and initializes
> > `Io` accordingly.
> >
> > To ensure that I/O mapped memory can't out-live the device it may be
> > bound to, subsystems should embedd the corresponding I/O memory type
> > (e.g. pci::Bar) into a `Devres` container, such that it gets revoked
> > once the device is unbound.
> 
> I wonder if `Io` should be a reference type instead. That is:
> 
> struct Io<'a, const SIZE: usize> {
>     addr: usize,
>     maxsize: usize,
>     _lifetime: PhantomData<&'a ()>,
> }
> 
> and then the constructor requires "addr must be valid I/O mapped
> memory for maxsize bytes for the duration of 'a". And instead of
> embedding it in another struct, the other struct creates an `Io` on
> each access?

So, we'd create the `Io` instance in `deref` of the parent structure, right?
What would be the advantage?

> 
> > Co-developed-by: Philipp Stanner <pstanner@redhat.com>
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> [...]
> 
> > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> > new file mode 100644
> > index 000000000000..750af938f83e
> > --- /dev/null
> > +++ b/rust/kernel/io.rs
> > @@ -0,0 +1,234 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Memory-mapped IO.
> > +//!
> > +//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
> > +
> > +use crate::error::{code::EINVAL, Result};
> > +use crate::{bindings, build_assert};
> > +
> > +/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
> > +///
> > +/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
> > +/// mapping, performing an additional region request etc.
> > +///
> > +/// # Invariant
> > +///
> > +/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
> > +/// `maxsize`.
> 
> Do you not also need an invariant that `SIZE <= maxsize`?

I guess so, yes. It's enforced by `Io::new`, which fails if `SIZE > maxsize`.

> 
> Alice
>