Message-ID: <CAH5fLgjC5Rcq5VJbEFSVP_rE0xjj8CGdqxZexhPVsGcTZ+85HA@mail.gmail.com>
Date: Tue, 29 Oct 2024 11:18:58 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, 
	tmgross@...ch.edu, a.hindborg@...sung.com, airlied@...il.com, 
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com, 
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org, 
	daniel.almeida@...labora.com, saravanak@...gle.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 09/16] rust: add `io::Io` base type

On Tue, Oct 29, 2024 at 10:21 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> 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@...nel.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.)

Yeah, thanks! I got it now.

> > > `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?

What you're doing now is a bit awkward to use. You have to make sure
that it never escapes the struct it's created for, so e.g. you can't
give out a mutable reference as the user could otherwise `mem::swap`
it with another Io. Similarly, the Io can never be in a public field.
Your safety comment on Io::new really needs to say something like
"while this struct exists, the `addr` must be a valid I/O region",
since I assume such regions are not valid forever? Similarly if we
look at [1], the I/O region actually gets unmapped *before* the Io is
destroyed since IoMem::drop runs before the fields of IoMem are
destroyed, so you really need something like "until the last use of
this Io" and not "until this Io is destroyed" in the safety comment.

If you compare similar cases in Rust, then they also do what I
suggested. For example, Vec<T> holds a raw pointer, and it uses unsafe
to assert that it's valid on each use of the raw pointer - it does not
create e.g. an `&'static mut [T]` to hold in a field of the Vec<T>.
Having an IoRaw<S> and an Io<'a, S> corresponds to what Vec<T> does
with IoRaw being like NonNull<T> and Io<'a, S> being like &'a T.

[1]: https://lore.kernel.org/all/20241024-topic-panthor-rs-platform_io_support-v1-1-3d1addd96e30@collabora.com/

> > > 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`.

Sure. It's still an invariant.

Alice

Powered by blists - more mailing lists