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

Signed-off-by: Fiona Behrens <me@kloenk.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
+///
+/// `addr` plus `maxsize` has to fit in memory (smaller than [`usize::MAX`])
+/// and `maxsize` has to be smaller or equal to `SIZE`.
 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@kloenk.dev>

On Wed, 22 Jan 2025 13:38:09 +0100
Fiona Behrens <me@kloenk.dev> 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`.
> 
> Signed-off-by: Fiona Behrens <me@kloenk.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
> +///
> +/// `addr` plus `maxsize` has to fit in memory (smaller than [`usize::MAX`])
> +/// and `maxsize` has to be smaller or equal to `SIZE`.
>  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);
>          }

By creating an invariant, you'll need to add `// INVARIANT` for the
construction of `IoRaw` below (the untouched lines, so not visible in
the patch).

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

I know this is moved over, but I think if you added an invariant you
should use it. Given now this is given by the invariant, you
should be able to use `unchecked_add` (or just add and leave the
behaviour of overflow to user's .config).

> +    }
> +
> +    /// 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
> +    }
>  }

Best,
Gary

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.

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

> 
> Signed-off-by: Fiona Behrens <me@kloenk.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@kloenk.dev>
>

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@kloenk.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@kloenk.dev>
>>

On Wed, 22 Jan 2025 15:22:27 +0100
Danilo Krummrich <dakr@kernel.org> 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.
> 
> Another option could also be to just extend the existing `Io` abstraction for
> I/O ports.
> 
> > 
> > Signed-off-by: Fiona Behrens <me@kloenk.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.

I thinkt the use of `Invariant` here is correct, as this needs to be
uphold by the constructors (and only then it can be relied on). However
the patch doesn't clearly indicate that.

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

Yeah, this is wrong.

> 
> >  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);
> >          }

Best,
Gary

On Wed, Jan 22, 2025 at 02:56:53PM +0000, Gary Guo wrote:
> On Wed, 22 Jan 2025 15:22:27 +0100
> Danilo Krummrich <dakr@kernel.org> 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.
> > 
> > Another option could also be to just extend the existing `Io` abstraction for
> > I/O ports.
> > 
> > > 
> > > Signed-off-by: Fiona Behrens <me@kloenk.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.
> 
> I thinkt the use of `Invariant` here is correct, as this needs to be

I think so too -- `Invariant` is the correct thing to use here.

But everywhere else in the kernel we phrase it differently. For instace, in
`Box` we say:

"`self.0` is always properly aligned and either points to memory allocated with
`A` or, for zero-sized types, is a dangling, well aligned pointer."

because this is ensured by the (safety requirements of the) constructor.

We don't say:

"`self.0` must be always properly aligned and either point to memory allocated
with `A` or, for zero-sized types, must be a dangling, well aligned pointer."

> uphold by the constructors (and only then it can be relied on). However
> the patch doesn't clearly indicate that.