Message-ID: <AC732BF8-8806-483B-AC14-66306D763ACF@kloenk.dev>
Date: Tue, 28 Jan 2025 12:11:29 +0100
From: Fiona Behrens <me@...enk.dev>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: io: move offset_valid and io_addr(_assert) to IoRaw



On 22 Jan 2025, at 15:22, Danilo Krummrich wrote:

> On Wed, Jan 22, 2025 at 01:38:09PM +0100, Fiona Behrens wrote:
>> Move the helper functions `offset_valid`, `io_addr` and
>> `io_addr_asset` from `Io` to `IoRaw`. This allows `IoRaw` to be reused
>> if other abstractions with different write/read functions are
>> needed (e.g. `writeb` vs `iowrite` vs `outb`).
>>
>> Make this functions public as well so they can be used from other
>> modules if you aquire a `IoRaw`.
>
> I don't think they should be public. Instead the abstraction for I/O ports
> should be in this file, just like `Io` is.

Will make it private, but would keep the documentation I wrote to make it easier to read the code.

>
> Another option could also be to just extend the existing `Io` abstraction for
> I/O ports.

Was thinking about that on zulip[0] already as well, but unsure if we should then remove the current write functions and use iowrite as those are the generic new interface as far as I understand, so it works with both iomem and portmem.

Thanks,
Fiona

[0]: https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/.60Io.60.20type.20and.20usage.20of.20.60iowrite*.60.2F.60ioread*.60

>
>>
>> Signed-off-by: Fiona Behrens <me@...enk.dev>
>> ---
>>  rust/kernel/io.rs | 98 +++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 63 insertions(+), 35 deletions(-)
>>
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index d4a73e52e3ee68f7b558749ed0108acde92ae5fe..a6d026f458608626113fd194ee5a8616b4ef76fe 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -15,6 +15,11 @@
>>  /// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
>>  /// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
>>  /// any guarantees are given.
>> +///
>> +/// # Invariant
>
> You phrased this invariant as if it would be a requirement, but it's more like a
> something that's always uphold. I'd phrase it as a fact that can be relied on.
>
>> +///
>> +/// `addr` plus `maxsize` has to fit in memory (smaller than [`usize::MAX`])
>
> "fit in memory" sounds a bit misleading. I think you want to say they have to be
> in the range of some address space (e.g. PIO).
>
> Besides that, why do we need this at all in this patch? I think it's fine to
> add, but then it should be separate patch I think.
>
>> +/// and `maxsize` has to be smaller or equal to `SIZE`.
>
> That's wrong, it's the other way around.
>
>>  pub struct IoRaw<const SIZE: usize = 0> {
>>      addr: usize,
>>      maxsize: usize,
>> @@ -23,7 +28,7 @@ pub struct IoRaw<const SIZE: usize = 0> {
>>  impl<const SIZE: usize> IoRaw<SIZE> {
>>      /// Returns a new `IoRaw` instance on success, an error otherwise.
>>      pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
>> -        if maxsize < SIZE {
>> +        if maxsize < SIZE || addr.checked_add(maxsize).is_none() {
>>              return Err(EINVAL);
>>          }
>>
>> @@ -32,15 +37,66 @@ pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
>>
>>      /// Returns the base address of the MMIO region.
>>      #[inline]
>> -    pub fn addr(&self) -> usize {
>> +    pub const fn addr(&self) -> usize {
>>          self.addr
>>      }
>>
>>      /// Returns the maximum size of the MMIO region.
>>      #[inline]
>> -    pub fn maxsize(&self) -> usize {
>> +    pub const fn maxsize(&self) -> usize {
>>          self.maxsize
>>      }
>> +
>> +    /// Check if the offset plus the size of the type `U` fits in the bounds of `size`.
>> +    /// Also checks if the offset is aligned with the type size.
>> +    #[inline]
>> +    pub const fn offset_valid<U>(offset: usize, size: usize) -> bool {
>> +        let type_size = core::mem::size_of::<U>();
>> +        if let Some(end) = offset.checked_add(type_size) {
>> +            end <= size && offset % type_size == 0
>> +        } else {
>> +            false
>> +        }
>> +    }
>> +
>> +    /// Check if the offset (plus the type size) is out of bounds.
>> +    ///
>> +    /// Runtime checked version of [`io_addr_assert`].
>> +    ///
>> +    /// See [`offset_valid`] for the performed offset check.
>> +    ///
>> +    /// # Errors
>> +    ///
>> +    /// Returns [`EINVAL`] if the type does not fit into [`IoRaw`] at the given offset.
>> +    ///
>> +    /// [`offset_valid`]: Self::offset_valid
>> +    /// [`io_addr_assert`]: Self::io_addr_assert
>> +    #[inline]
>> +    pub fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> +        if !Self::offset_valid::<U>(offset, self.maxsize()) {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Probably no need to check, since the safety requirements of `Self::new` guarantee that
>> +        // this can't overflow.
>> +        self.addr().checked_add(offset).ok_or(EINVAL)
>> +    }
>> +
>> +    /// Check at build time if the offset (plus the type size) is out of bounds.
>> +    ///
>> +    /// Compiletime checked version of [`io_addr`].
>> +    ///
>> +    /// See [`offset_valid`] for the performed offset check.
>> +    ///
>> +    ///
>> +    /// [`offset_valid`]: Self::offset_valid
>> +    /// [`io_addr`]: Self::io_addr
>> +    #[inline]
>> +    pub const fn io_addr_assert<U>(&self, offset: usize) -> usize {
>> +        build_assert!(Self::offset_valid::<U>(offset, SIZE));
>> +
>> +        self.addr() + offset
>> +    }
>>  }
>>
>>  /// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
>> @@ -116,7 +172,7 @@ macro_rules! define_read {
>>          $(#[$attr])*
>>          #[inline]
>>          pub fn $name(&self, offset: usize) -> $type_name {
>> -            let addr = self.io_addr_assert::<$type_name>(offset);
>> +            let addr = self.0.io_addr_assert::<$type_name>(offset);
>>
>>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>>              unsafe { bindings::$name(addr as _) }
>> @@ -128,7 +184,7 @@ pub fn $name(&self, offset: usize) -> $type_name {
>>          /// out of bounds.
>>          $(#[$attr])*
>>          pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
>> -            let addr = self.io_addr::<$type_name>(offset)?;
>> +            let addr = self.0.io_addr::<$type_name>(offset)?;
>>
>>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>>              Ok(unsafe { bindings::$name(addr as _) })
>> @@ -145,7 +201,7 @@ macro_rules! define_write {
>>          $(#[$attr])*
>>          #[inline]
>>          pub fn $name(&self, value: $type_name, offset: usize) {
>> -            let addr = self.io_addr_assert::<$type_name>(offset);
>> +            let addr = self.0.io_addr_assert::<$type_name>(offset);
>>
>>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>>              unsafe { bindings::$name(value, addr as _, ) }
>> @@ -157,7 +213,7 @@ pub fn $name(&self, value: $type_name, offset: usize) {
>>          /// out of bounds.
>>          $(#[$attr])*
>>          pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
>> -            let addr = self.io_addr::<$type_name>(offset)?;
>> +            let addr = self.0.io_addr::<$type_name>(offset)?;
>>
>>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>>              unsafe { bindings::$name(value, addr as _) }
>> @@ -190,34 +246,6 @@ pub fn maxsize(&self) -> usize {
>>          self.0.maxsize()
>>      }
>>
>> -    #[inline]
>> -    const fn offset_valid<U>(offset: usize, size: usize) -> bool {
>> -        let type_size = core::mem::size_of::<U>();
>> -        if let Some(end) = offset.checked_add(type_size) {
>> -            end <= size && offset % type_size == 0
>> -        } else {
>> -            false
>> -        }
>> -    }
>> -
>> -    #[inline]
>> -    fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> -        if !Self::offset_valid::<U>(offset, self.maxsize()) {
>> -            return Err(EINVAL);
>> -        }
>> -
>> -        // Probably no need to check, since the safety requirements of `Self::new` guarantee that
>> -        // this can't overflow.
>> -        self.addr().checked_add(offset).ok_or(EINVAL)
>> -    }
>> -
>> -    #[inline]
>> -    fn io_addr_assert<U>(&self, offset: usize) -> usize {
>> -        build_assert!(Self::offset_valid::<U>(offset, SIZE));
>> -
>> -        self.addr() + offset
>> -    }
>> -
>>      define_read!(readb, try_readb, u8);
>>      define_read!(readw, try_readw, u16);
>>      define_read!(readl, try_readl, u32);
>>
>> ---
>> base-commit: 01b3cb620815fc3feb90ee117d9445a5b608a9f7
>> change-id: 20250122-rust-io-offset-7b39b11e84ac
>>
>> Best regards,
>> -- 
>> Fiona Behrens <me@...enk.dev>
>>

Powered by blists - more mailing lists