linux-kernel - Re: [PATCH v3 1/5] rust: io: factor common I/O helpers into Io trait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251031144836.110ac310.zhiw@nvidia.com>
Date: Fri, 31 Oct 2025 14:48:36 +0200
From: Zhi Wang <zhiw@...dia.com>
To: Alice Ryhl <aliceryhl@...gle.com>
CC: <rust-for-linux@...r.kernel.org>, <dakr@...nel.org>,
	<bhelgaas@...gle.com>, <kwilczynski@...nel.org>, <ojeda@...nel.org>,
	<alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
	<bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
	<tmgross@...ch.edu>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <cjia@...dia.com>, <smitra@...dia.com>,
	<ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
	<targupta@...dia.com>, <zhiwang@...nel.org>, <acourbot@...dia.com>,
	<joelagnelf@...dia.com>, <jhubbard@...dia.com>, <markus.probst@...teo.de>,
	Bjorn Helgaas <helgaas@...nel.org>
Subject: Re: [PATCH v3 1/5] rust: io: factor common I/O helpers into Io
 trait

On Fri, 31 Oct 2025 09:07:04 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> On Thu, Oct 30, 2025 at 03:48:38PM +0000, Zhi Wang wrote:
> > The previous Io<SIZE> type combined both the generic I/O access
> > helpers and MMIO implementation details in a single struct.
> > 
> > To establish a cleaner layering between the I/O interface and its
> > concrete backends, paving the way for supporting additional I/O
> > mechanisms in the future, Io<SIZE> need to be factored.
> > 
> > Factor the common helpers into a new Io trait, and move the
> > MMIO-specific logic into a dedicated Mmio<SIZE> type implementing
> > that trait. Rename the IoRaw to MmioRaw and update the bus MMIO
> > implementations to use MmioRaw.
> > 
> > No functional change intended.
> > 
> > Cc: Alexandre Courbot <acourbot@...dia.com>
> > Cc: Bjorn Helgaas <helgaas@...nel.org>
> > Cc: Danilo Krummrich <dakr@...nel.org>
> > Cc: John Hubbard <jhubbard@...dia.com>
> > Signed-off-by: Zhi Wang <zhiw@...dia.com>
> 
> > +/// Represents a region of I/O space of a fixed size.
> > +///
> > +/// Provides common helpers for offset validation and address
> > +/// calculation on top of a base address and maximum size.
> > +///
> > +/// Types implementing this trait (e.g. MMIO BARs or PCI config
> > +/// regions) can share the same accessors.
> > +pub trait Io<const SIZE: usize> {
> 
> I would consider moving SIZE to an associated constant.
> 
> 	pub trait Io {
> 	    const MIN_SIZE: usize;
> 	
> 	    ...
> 	}
> 
> If it's a generic parameter, then the same type can implement both
> Io<5> and Io<7> at the same time, but I don't think it makes sense
> for a single type to implement Io with different minimum sizes.
> 

I see your point. It also makes the code look cleaner.
From my understanding, this is essentially a choice between performing
static boundary checks through the type system using const generics, or
using build_assert!() with a trait or struct-associated constant.

Let me take a closer look and experiment a bit with it. :)

> >      /// Returns the base address of this mapping.
> > -    #[inline]
...ditto
> > +    /// Infallible 64-bit write with compile-time bounds check
> > (64-bit only).
> > +    #[cfg(CONFIG_64BIT)]
> > +    fn write64(&self, _value: u64, _offset: usize) {
> > +        ()
> > +    }
> > +
> > +    /// Fallible 8-bit write with runtime bounds check.
> > +    fn try_write8(&self, value: u8, offset: usize) -> Result;
> > +
> > +    /// Fallible 16-bit write with runtime bounds check.
> > +    fn try_write16(&self, value: u16, offset: usize) -> Result;
> > +
> > +    /// Fallible 32-bit write with runtime bounds check.
> > +    fn try_write32(&self, value: u32, offset: usize) -> Result;
> > +
> > +    /// Fallible 64-bit write with runtime bounds check (64-bit
> > only).
> > +    #[cfg(CONFIG_64BIT)]
> > +    fn try_write64(&self, _value: u64, _offset: usize) -> Result {
> > +        Err(ENOTSUPP)
> > +    }
> 
> Why are there default implementations for all of these trait methods?
> I would suggest not providing any default implementations at all.
> 

Yeah, I actually tried that in an earlier version.
I noticed that each backend is a bit different — for example, the PCI
config space routines don’t have read64()/write64() either. By
design, we don’t provide infallible versions for the PCI config space
backend (unlike the MMIO one). Other backends might have similar cases
as well.

So I ended up keeping the trait’s default implementation "minimal", only
including the methods every backend really has to implement. The default
impls are mainly there to catch situations where a driver calls something
it shouldn’t.

I should probably make the compiler complain when an infallible op isn’t
supported by a given backend. And if you have any ideas on making this
more elegant, I’m all ears. :)

Z.

> Alice