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

[Philipp Stanner <pstanner@redhat.com>]

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.

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.



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

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


P.

> 
> thanks,
> 
> greg k-h
>