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@xxxxxxxxxx> 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@xxxxxxxxxx> > > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx> > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx> > > [...] > > > 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 >