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