Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Danilo Krummrich <dakr@redhat.com>]

On Fri, Jun 21, 2024 at 11:43:34AM +0200, Philipp Stanner wrote:

Please find a few additions below.

But as mentioned, please let us sort out [1] first.

[1] https://lore.kernel.org/lkml/ZnSeAZu3IMA4fR8P@cassiopeiae/

> On Thu, 2024-06-20 at 16:53 +0200, Greg KH wrote:
> > On Wed, Jun 19, 2024 at 01:39:53AM +0200, Danilo Krummrich 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.
> > 
> > Why not?
> 
> Because you'd then up reimplementing all that logic that the C code
> already provides. In the worst case that could lead to you effectively
> reimplemting the subsystem instead of wrapping it. And that's obviously
> uncool because you'd then have two of them (besides, the community in
> general rightfully pushes back against reimplementing stuff; see the
> attempts to provide redundant Rust drivers in the past).
> 
> The C code already takes care of figuring out region ranges and all
> that, and it's battle hardened.

To add an example, instead of reimplementing things like pci_iomap() we use
`Io` as base type providing the accrssors like readl() and let the resource
implement the mapping parts, such as `pci::Bar`.

> 
> The main point of Rust is to make things safer; so if that can be
> achieved without rewrite, as is the case with the presented container
> solution, that's the way to go.
> 
> > 
> > > Instead, implement a base type for I/O mapped memory, which
> > > generically
> > > provides the corresponding accessors, such as `Io::readb` or
> > > `Io:try_readb`.
> > 
> > It provides a subset of the existing accessors, one you might want to
> > trim down for now, see below...
> > 
> > > +/* io.h */
> > > +u8 rust_helper_readb(const volatile void __iomem *addr)
> > > +{
> > > +       return readb(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readb);
> > 
> > <snip>
> > 
> > You provide wrappers for a subset of what io.h provides, why that
> > specific subset?
> > 
> > Why not just add what you need, when you need it?  I doubt you need
> > all
> > of these, and odds are you will need more.
> > 
> 
> That was written by me as a first play set to test. Nova itself
> currently reads only 8 byte from a PCI BAR, so we could indeed drop
> everything but readq() for now and add things subsequently later, as
> you suggest.

I think it is reasonable to start with the most common accessors
{read,write}{b,w,l,q and maybe their relaxed variants.

We generate them through the `define_read!` and `define_write!` macros anyways
and the only difference between all the variants is only the size type (u8, u16,
etc.) we pass to the macro.

> 
> 
> 
> > > +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> > > +{
> > > +       return readl_relaxed(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);
> > 
> > I know everyone complains about wrapper functions around inline
> > functions, so I'll just say it again, this is horrid.  And it's going
> > to
> > hurt performance, so any rust code people write is not on a level
> > playing field here.
> > 
> > Your call, but ick...
> 
> Well, can anyone think of another way to do it?
> 
> > 
> > > +#ifdef CONFIG_64BIT
> > > +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> > > +{
> > > +       return readq_relaxed(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
> > > +#endif
> > 
> > Rust works on 32bit targets in the kernel now?
> 
> Ahm, afaik not. That's some relic. Let's address that with your subset
> comment from above.

I think we should keep this guard; readq() implementations in the arch code have
this guard as well.

Should we ever add a 32bit target for Rust we also don't want this to break.

> 
> > 
> > > +macro_rules! define_read {
> > > +    ($(#[$attr:meta])* $name:ident, $try_name:ident,
> > > $type_name:ty) => {
> > > +        /// Read IO data from a given offset known at compile
> > > time.
> > > +        ///
> > > +        /// Bound checks are performed on compile time, hence if
> > > the offset is not known at compile
> > > +        /// time, the build will fail.
> > 
> > offsets aren't know at compile time for many implementations, as it
> > could be a dynamically allocated memory range.  How is this going to
> > work for that?  Heck, how does this work for DT-defined memory ranges
> > today?
> 
> The macro below will take care of those where it's only knowable at
> runtime I think.
> 
> Rust has this feature (called "const generic") that can be used for
> APIs where ranges which are known at compile time, so the compiler can
> check all the parameters at that point. That has been judged to be
> positive because errors with the range handling become visible before
> the kernel runs and because it gives some performance advantages.

Let's add an exammple based on `pci::Bar` here.

As a driver you can optionally map a `pci::Bar` with an additional `SIZE`
constant, e.g.

```
let bar = pdev.iomap_region_sized::<0x1000>(0)?;
```

This call only succeeds of the actual bar size is *at least* 4k. Subsequent
calls to, let's say, `bar.readl(0x10)` can boundary check things on compile
time, such that `bar.readl(0x1000)` would fail on compile time.

This is useful when a driver knows the minum required / expected size of this
memory region.

Alternatively, a driver cann always fall back to a runtime check, e.g.

```
let bar = pdev.iomap_region(0)?;
let val = bar.try_readl(0x1000)?;
```

- Danilo

> 
> 
> P.
> 
> > 
> > thanks,
> > 
> > greg k-h
> > 
>