[PATCH] rust: platform: add Io support

[PATCH] rust: platform: add Io support

[Daniel Almeida]

The IoMem is backed by ioremap_resource()

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
The PCI/Platform abstractions are in-flight and can be found at:

https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/io.c               | 11 ++++++
 rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,6 +13,7 @@
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/firmware.h>
+#include <linux/ioport.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
 #include <linux/of_device.h>
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
--- a/rust/helpers/io.c
+++ b/rust/helpers/io.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/io.h>
+#include <linux/ioport.h>
 
 u8 rust_helper_readb(const volatile void __iomem *addr)
 {
@@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
 	writeq_relaxed(value, addr);
 }
 #endif
+
+resource_size_t rust_helper_resource_size(struct resource *res)
+{
+	return resource_size(res);
+}
+
+void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
+{
+	return ioremap(addr, size);
+}
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -4,11 +4,15 @@
 //!
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
+use core::ops::Deref;
+
 use crate::{
     bindings, container_of, device,
     device_id::RawDeviceId,
+    devres::Devres,
     driver,
     error::{to_result, Result},
+    io::Io,
     of,
     prelude::*,
     str::CStr,
@@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
         // embedded in `struct platform_device`.
         unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
     }
+
+    /// Maps a platform resource through ioremap() where the size is known at
+    /// compile time.
+    pub fn ioremap_resource_sized<const SIZE: usize>(
+        &self,
+        resource: u32,
+    ) -> Result<Devres<IoMem<SIZE>>> {
+        let res = self.resource(resource)?;
+        let size = self.resource_len(resource)? as usize;
+
+        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
+        // is given by the kernel as per `self.resource_len()`.
+        let io = unsafe { IoMem::new(res as _, size) }?;
+        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
+
+        Ok(devres)
+    }
+
+    /// Maps a platform resource through ioremap().
+    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
+        self.ioremap_resource_sized::<0>(resource)
+    }
+
+    /// Returns the resource len for `resource`, if it exists.
+    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
+        match self.resource(resource) {
+            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
+            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
+            Err(e) => Err(e),
+        }
+    }
+
+    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
+        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
+        let resource = unsafe {
+            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
+        };
+        if !resource.is_null() {
+            Ok(resource)
+        } else {
+            Err(EINVAL)
+        }
+    }
 }
 
 impl AsRef<device::Device> for Device {
@@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
         &self.0
     }
 }
+
+/// A I/O-mapped memory region for platform devices.
+///
+/// See also [`kernel::pci::Bar`].
+pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
+
+impl<const SIZE: usize> IoMem<SIZE> {
+    /// Creates a new `IoMem` instance.
+    ///
+    /// # Safety
+    ///
+    /// The caller must make sure that `paddr` is a valid MMIO address.
+    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
+        // SAFETY: the safety requirements assert that `paddr` is a valid MMIO address.
+        let addr = unsafe { bindings::ioremap(paddr as _, size as u64) };
+        if addr.is_null() {
+            return Err(ENOMEM);
+        }
+
+        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
+        // size `size`.
+        let io = unsafe { Io::new(addr as _, size)? };
+
+        Ok(IoMem(io))
+    }
+}
+
+impl<const SIZE: usize> Drop for IoMem<SIZE> {
+    fn drop(&mut self) {
+        // SAFETY: Safe as by the invariant of `Io`.
+        unsafe { bindings::iounmap(self.0.base_addr() as _) };
+    }
+}
+
+impl<const SIZE: usize> Deref for IoMem<SIZE> {
+    type Target = Io<SIZE>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}

---
base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH] rust: platform: add Io support

[Alice Ryhl]

On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> The IoMem is backed by ioremap_resource()
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> The PCI/Platform abstractions are in-flight and can be found at:
>
> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers/io.c               | 11 ++++++
>  rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -13,6 +13,7 @@
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
>  #include <linux/firmware.h>
> +#include <linux/ioport.h>
>  #include <linux/jiffies.h>
>  #include <linux/mdio.h>
>  #include <linux/of_device.h>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
> --- a/rust/helpers/io.c
> +++ b/rust/helpers/io.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>
>  #include <linux/io.h>
> +#include <linux/ioport.h>
>
>  u8 rust_helper_readb(const volatile void __iomem *addr)
>  {
> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
>         writeq_relaxed(value, addr);
>  }
>  #endif
> +
> +resource_size_t rust_helper_resource_size(struct resource *res)
> +{
> +       return resource_size(res);
> +}
> +
> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
> +{
> +       return ioremap(addr, size);
> +}
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -4,11 +4,15 @@
>  //!
>  //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>
> +use core::ops::Deref;
> +
>  use crate::{
>      bindings, container_of, device,
>      device_id::RawDeviceId,
> +    devres::Devres,
>      driver,
>      error::{to_result, Result},
> +    io::Io,
>      of,
>      prelude::*,
>      str::CStr,
> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>          // embedded in `struct platform_device`.
>          unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>      }
> +
> +    /// Maps a platform resource through ioremap() where the size is known at
> +    /// compile time.
> +    pub fn ioremap_resource_sized<const SIZE: usize>(
> +        &self,
> +        resource: u32,
> +    ) -> Result<Devres<IoMem<SIZE>>> {
> +        let res = self.resource(resource)?;
> +        let size = self.resource_len(resource)? as usize;

You're calling platform_get_resource twice here? Can you be sure that
it returns the same pointer each time?

> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
> +        // is given by the kernel as per `self.resource_len()`.
> +        let io = unsafe { IoMem::new(res as _, size) }?;

Why do we know that `res` is a valid MMIO address? Is it because
platform_get_resource always does so?

> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +
> +    /// Maps a platform resource through ioremap().
> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
> +        self.ioremap_resource_sized::<0>(resource)
> +    }

I guess size zero is special?

> +    /// Returns the resource len for `resource`, if it exists.
> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {

Should this just return usize? Should we have a type alias for this size type?

> +        match self.resource(resource) {
> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
> +            Err(e) => Err(e),
> +        }
> +    }
> +
> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> +        let resource = unsafe {
> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> +        };
> +        if !resource.is_null() {
> +            Ok(resource)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
>  }
>
>  impl AsRef<device::Device> for Device {
> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
>          &self.0
>      }
>  }
> +
> +/// A I/O-mapped memory region for platform devices.
> +///
> +/// See also [`kernel::pci::Bar`].
> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    /// Creates a new `IoMem` instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must make sure that `paddr` is a valid MMIO address.
> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {

What happens if `size != SIZE`?

> +        // SAFETY: the safety requirements assert that `paddr` is a valid MMIO address.
> +        let addr = unsafe { bindings::ioremap(paddr as _, size as u64) };
> +        if addr.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> +        // size `size`.
> +        let io = unsafe { Io::new(addr as _, size)? };
> +
> +        Ok(IoMem(io))
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.0.base_addr() as _) };
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
>
> ---
> base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
> change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887
>
> Best regards,
> --
> Daniel Almeida <daniel.almeida@collabora.com>
>

Re: [PATCH] rust: platform: add Io support

[Daniel Almeida]

Hi Alice,

> On 28 Oct 2024, at 12:37, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> The IoMem is backed by ioremap_resource()
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> The PCI/Platform abstractions are in-flight and can be found at:
>> 
>> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
>> ---
>> rust/bindings/bindings_helper.h |  1 +
>> rust/helpers/io.c               | 11 ++++++
>> rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 100 insertions(+)
>> 
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -13,6 +13,7 @@
>> #include <linux/errname.h>
>> #include <linux/ethtool.h>
>> #include <linux/firmware.h>
>> +#include <linux/ioport.h>
>> #include <linux/jiffies.h>
>> #include <linux/mdio.h>
>> #include <linux/of_device.h>
>> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
>> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
>> --- a/rust/helpers/io.c
>> +++ b/rust/helpers/io.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0
>> 
>> #include <linux/io.h>
>> +#include <linux/ioport.h>
>> 
>> u8 rust_helper_readb(const volatile void __iomem *addr)
>> {
>> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
>>        writeq_relaxed(value, addr);
>> }
>> #endif
>> +
>> +resource_size_t rust_helper_resource_size(struct resource *res)
>> +{
>> +       return resource_size(res);
>> +}
>> +
>> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
>> +{
>> +       return ioremap(addr, size);
>> +}
>> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
>> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
>> --- a/rust/kernel/platform.rs
>> +++ b/rust/kernel/platform.rs
>> @@ -4,11 +4,15 @@
>> //!
>> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>> 
>> +use core::ops::Deref;
>> +
>> use crate::{
>>     bindings, container_of, device,
>>     device_id::RawDeviceId,
>> +    devres::Devres,
>>     driver,
>>     error::{to_result, Result},
>> +    io::Io,
>>     of,
>>     prelude::*,
>>     str::CStr,
>> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>>         // embedded in `struct platform_device`.
>>         unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>>     }
>> +
>> +    /// Maps a platform resource through ioremap() where the size is known at
>> +    /// compile time.
>> +    pub fn ioremap_resource_sized<const SIZE: usize>(
>> +        &self,
>> +        resource: u32,
>> +    ) -> Result<Devres<IoMem<SIZE>>> {
>> +        let res = self.resource(resource)?;
>> +        let size = self.resource_len(resource)? as usize;
> 
> You're calling platform_get_resource twice here? Can you be sure that
> it returns the same pointer each time?

This comes from the DT. Yes, it will be the same pointer (so long as we
pass the same index).

Although, I did not see where it is being called twice?

> 
>> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
>> +        // is given by the kernel as per `self.resource_len()`.
>> +        let io = unsafe { IoMem::new(res as _, size) }?;
> 
> Why do we know that `res` is a valid MMIO address? Is it because
> platform_get_resource always does so?

Again, if there’s a problem, the DT itself should be fixed. Also the C code would be broken too.

> 
>> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
>> +
>> +        Ok(devres)
>> +    }
>> +
>> +    /// Maps a platform resource through ioremap().
>> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
>> +        self.ioremap_resource_sized::<0>(resource)
>> +    }
> 
> I guess size zero is special?
> 

`ioremap_resource_sized` is used when you know the size at compile time. Setting SIZE == 0
means that you get runtime checks on whether your reads and writes are within bounds,
because you will have to use try_read() and try_write() instead of read() and write() or your build
will fail.

This is not related to this patch in particular, but to a preceding one that introduces struct Io.

We merely expose the same API from Io to users here.

Although I do agree it’s a bit confusing that SIZE 0 is special-cased. It took me a while and
several read-throughs to understand what was going on.

Note that it’s common to have to read the size from the DT, so deferring to runtime checks
seems to be unavoidable in a lot of cases if we want to have a safe API.

Here’s the relevant code from that commit:

```
+macro_rules! define_write {
+ ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+ /// Write 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.
+ $(#[$attr])*
+ #[inline]
+ pub fn $name(&self, value: $type_name, offset: usize) {
+ 	let addr = self.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 _, ) }
+ }
+
+ /// Write IO data from a given offset.
+ ///
+ /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
+ /// out of bounds.
+ $(#[$attr])*
+ pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
+ 	let addr = self.io_addr::<$type_name>(offset)?;
+
+ 	// SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+ 	unsafe { bindings::$name(value, addr as _) }
+ 	Ok(())
+ }
```

Where,

```
+ #[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.base_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.base_addr() + offset
+ }
```

And

```
+ #[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
+ 	}
+ }
```

>> +    /// Returns the resource len for `resource`, if it exists.
>> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
> 
> Should this just return usize? Should we have a type alias for this size type?


I guess usize would indeed be a better fit, if we consider the code below:

```
#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif

typedef phys_addr_t resource_size_t;

```

> 
>> +        match self.resource(resource) {
>> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
>> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
>> +            Err(e) => Err(e),
>> +        }
>> +    }
>> +
>> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
>> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
>> +        let resource = unsafe {
>> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
>> +        };
>> +        if !resource.is_null() {
>> +            Ok(resource)
>> +        } else {
>> +            Err(EINVAL)
>> +        }
>> +    }
>> }
>> 
>> impl AsRef<device::Device> for Device {
>> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
>>         &self.0
>>     }
>> }
>> +
>> +/// A I/O-mapped memory region for platform devices.
>> +///
>> +/// See also [`kernel::pci::Bar`].
>> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
>> +
>> +impl<const SIZE: usize> IoMem<SIZE> {
>> +    /// Creates a new `IoMem` instance.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must make sure that `paddr` is a valid MMIO address.
>> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
> 
> What happens if `size != SIZE`?


```
+impl<const SIZE: usize> Io<SIZE> {
+ ///
+ ///
+ /// # Safety
+ ///
+ /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+ /// `maxsize`.
+ pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
+ 	if maxsize < SIZE {
+ 		return Err(EINVAL);
+	 }
```

As we’ve seen in the other code snippets I pasted, it’s SIZE that is used in the try_read and
try_write checks.

> 
>> +        // SAFETY: the safety requirements assert that `paddr` is a valid MMIO address.
>> +        let addr = unsafe { bindings::ioremap(paddr as _, size as u64) };
>> +        if addr.is_null() {
>> +            return Err(ENOMEM);
>> +        }
>> +
>> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
>> +        // size `size`.
>> +        let io = unsafe { Io::new(addr as _, size)? };
>> +
>> +        Ok(IoMem(io))
>> +    }
>> +}
>> +
>> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: Safe as by the invariant of `Io`.
>> +        unsafe { bindings::iounmap(self.0.base_addr() as _) };
>> +    }
>> +}
>> +
>> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
>> +    type Target = Io<SIZE>;
>> +
>> +    fn deref(&self) -> &Self::Target {
>> +        &self.0
>> +    }
>> +}
>> 
>> ---
>> base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
>> change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887
>> 
>> Best regards,
>> --
>> Daniel Almeida <daniel.almeida@collabora.com>


— Daniel

Re: [PATCH] rust: platform: add Io support

[Danilo Krummrich]

On 10/24/24 4:20 PM, Daniel Almeida wrote:
> The IoMem is backed by ioremap_resource()

ioremap_resource()?

Also, that's a rather short commit message for such a core piece of
infrastructure, can you please expand on this?

> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> The PCI/Platform abstractions are in-flight and can be found at:
> 
> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
> ---
>   rust/bindings/bindings_helper.h |  1 +
>   rust/helpers/io.c               | 11 ++++++
>   rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 100 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -13,6 +13,7 @@
>   #include <linux/errname.h>
>   #include <linux/ethtool.h>
>   #include <linux/firmware.h>
> +#include <linux/ioport.h>
>   #include <linux/jiffies.h>
>   #include <linux/mdio.h>
>   #include <linux/of_device.h>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
> --- a/rust/helpers/io.c
> +++ b/rust/helpers/io.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0
>   
>   #include <linux/io.h>
> +#include <linux/ioport.h>
>   
>   u8 rust_helper_readb(const volatile void __iomem *addr)
>   {
> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
>   	writeq_relaxed(value, addr);
>   }
>   #endif
> +
> +resource_size_t rust_helper_resource_size(struct resource *res)
> +{
> +	return resource_size(res);
> +}
> +
> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
> +{
> +	return ioremap(addr, size);
> +}
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -4,11 +4,15 @@
>   //!
>   //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>   
> +use core::ops::Deref;
> +
>   use crate::{
>       bindings, container_of, device,
>       device_id::RawDeviceId,
> +    devres::Devres,
>       driver,
>       error::{to_result, Result},
> +    io::Io,
>       of,
>       prelude::*,
>       str::CStr,
> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>           // embedded in `struct platform_device`.
>           unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>       }
> +
> +    /// Maps a platform resource through ioremap() where the size is known at
> +    /// compile time.
> +    pub fn ioremap_resource_sized<const SIZE: usize>(
> +        &self,
> +        resource: u32,
> +    ) -> Result<Devres<IoMem<SIZE>>> {
> +        let res = self.resource(resource)?;
> +        let size = self.resource_len(resource)? as usize;
> +
> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
> +        // is given by the kernel as per `self.resource_len()`.
> +        let io = unsafe { IoMem::new(res as _, size) }?;
> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +
> +    /// Maps a platform resource through ioremap().
> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
> +        self.ioremap_resource_sized::<0>(resource)
> +    }
> +
> +    /// Returns the resource len for `resource`, if it exists.
> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
> +        match self.resource(resource) {
> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
> +            Err(e) => Err(e),
> +        }
> +    }
> +
> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> +        let resource = unsafe {
> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> +        };
> +        if !resource.is_null() {
> +            Ok(resource)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
>   }
>   
>   impl AsRef<device::Device> for Device {
> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
>           &self.0
>       }
>   }
> +
> +/// A I/O-mapped memory region for platform devices.
> +///
> +/// See also [`kernel::pci::Bar`].
> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);

Is this meant to be bus specific, such as `pci::Bar` is PCI specific?

If so, I think it'd be better to pass the platform device and resource index to
`platform::IoMem` (or whatever we want to call it) and let it take care of 
everything.

> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    /// Creates a new `IoMem` instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must make sure that `paddr` is a valid MMIO address.
> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
> +        // SAFETY: the safety requirements assert that `paddr` is a valid MMIO address.
> +        let addr = unsafe { bindings::ioremap(paddr as _, size as u64) };

Do we need to consider ioremap_uc(), ioremap_wc(), ioremap_np()?

Also, I think you're missing request_region() here.

> +        if addr.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> +        // size `size`.
> +        let io = unsafe { Io::new(addr as _, size)? };
> +
> +        Ok(IoMem(io))
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.0.base_addr() as _) };
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> 
> ---
> base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
> change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887
> 
> Best regards,

Re: [PATCH] rust: platform: add Io support

[Alice Ryhl]

On Mon, Oct 28, 2024 at 7:23 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> > On 28 Oct 2024, at 12:37, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> The IoMem is backed by ioremap_resource()
> >>
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> ---
> >> The PCI/Platform abstractions are in-flight and can be found at:
> >>
> >> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
> >> ---
> >> rust/bindings/bindings_helper.h |  1 +
> >> rust/helpers/io.c               | 11 ++++++
> >> rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 100 insertions(+)
> >>
> >> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> >> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
> >> --- a/rust/bindings/bindings_helper.h
> >> +++ b/rust/bindings/bindings_helper.h
> >> @@ -13,6 +13,7 @@
> >> #include <linux/errname.h>
> >> #include <linux/ethtool.h>
> >> #include <linux/firmware.h>
> >> +#include <linux/ioport.h>
> >> #include <linux/jiffies.h>
> >> #include <linux/mdio.h>
> >> #include <linux/of_device.h>
> >> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> >> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
> >> --- a/rust/helpers/io.c
> >> +++ b/rust/helpers/io.c
> >> @@ -1,6 +1,7 @@
> >> // SPDX-License-Identifier: GPL-2.0
> >>
> >> #include <linux/io.h>
> >> +#include <linux/ioport.h>
> >>
> >> u8 rust_helper_readb(const volatile void __iomem *addr)
> >> {
> >> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
> >>        writeq_relaxed(value, addr);
> >> }
> >> #endif
> >> +
> >> +resource_size_t rust_helper_resource_size(struct resource *res)
> >> +{
> >> +       return resource_size(res);
> >> +}
> >> +
> >> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
> >> +{
> >> +       return ioremap(addr, size);
> >> +}
> >> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> >> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
> >> --- a/rust/kernel/platform.rs
> >> +++ b/rust/kernel/platform.rs
> >> @@ -4,11 +4,15 @@
> >> //!
> >> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
> >>
> >> +use core::ops::Deref;
> >> +
> >> use crate::{
> >>     bindings, container_of, device,
> >>     device_id::RawDeviceId,
> >> +    devres::Devres,
> >>     driver,
> >>     error::{to_result, Result},
> >> +    io::Io,
> >>     of,
> >>     prelude::*,
> >>     str::CStr,
> >> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
> >>         // embedded in `struct platform_device`.
> >>         unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
> >>     }
> >> +
> >> +    /// Maps a platform resource through ioremap() where the size is known at
> >> +    /// compile time.
> >> +    pub fn ioremap_resource_sized<const SIZE: usize>(
> >> +        &self,
> >> +        resource: u32,
> >> +    ) -> Result<Devres<IoMem<SIZE>>> {
> >> +        let res = self.resource(resource)?;
> >> +        let size = self.resource_len(resource)? as usize;
> >
> > You're calling platform_get_resource twice here? Can you be sure that
> > it returns the same pointer each time?
>
> This comes from the DT. Yes, it will be the same pointer (so long as we
> pass the same index).
>
> Although, I did not see where it is being called twice?

The `resource_len` function calls `resource`, so you call `resource`
twice. Each call to `resource` results in a call to
`platform_get_resource`.

> >> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
> >> +        // is given by the kernel as per `self.resource_len()`.
> >> +        let io = unsafe { IoMem::new(res as _, size) }?;
> >
> > Why do we know that `res` is a valid MMIO address? Is it because
> > platform_get_resource always does so?
>
> Again, if there’s a problem, the DT itself should be fixed. Also the C code would be broken too.

Sorry if I was unclear. I just wanted you to elaborate in the safety comment :)

> >> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> >> +
> >> +        Ok(devres)
> >> +    }
> >> +
> >> +    /// Maps a platform resource through ioremap().
> >> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
> >> +        self.ioremap_resource_sized::<0>(resource)
> >> +    }
> >
> > I guess size zero is special?
> >
>
> `ioremap_resource_sized` is used when you know the size at compile time. Setting SIZE == 0
> means that you get runtime checks on whether your reads and writes are within bounds,
> because you will have to use try_read() and try_write() instead of read() and write() or your build
> will fail.
>
> This is not related to this patch in particular, but to a preceding one that introduces struct Io.
>
> We merely expose the same API from Io to users here.
>
> Although I do agree it’s a bit confusing that SIZE 0 is special-cased. It took me a while and
> several read-throughs to understand what was going on.
>
> Note that it’s common to have to read the size from the DT, so deferring to runtime checks
> seems to be unavoidable in a lot of cases if we want to have a safe API.
>
> Here’s the relevant code from that commit:
>
> ```
> +macro_rules! define_write {
> + ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
> + /// Write 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.
> + $(#[$attr])*
> + #[inline]
> + pub fn $name(&self, value: $type_name, offset: usize) {
> +       let addr = self.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 _, ) }
> + }
> +
> + /// Write IO data from a given offset.
> + ///
> + /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
> + /// out of bounds.
> + $(#[$attr])*
> + pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
> +       let addr = self.io_addr::<$type_name>(offset)?;
> +
> +       // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> +       unsafe { bindings::$name(value, addr as _) }
> +       Ok(())
> + }
> ```
>
> Where,
>
> ```
> + #[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.base_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.base_addr() + offset
> + }
> ```
>
> And
>
> ```
> + #[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
> +       }
> + }
> ```

Acknowledged.

> >> +    /// Returns the resource len for `resource`, if it exists.
> >> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
> >
> > Should this just return usize? Should we have a type alias for this size type?
>
>
> I guess usize would indeed be a better fit, if we consider the code below:
>
> ```
> #ifdef CONFIG_PHYS_ADDR_T_64BIT
> typedef u64 phys_addr_t;
> #else
> typedef u32 phys_addr_t;
> #endif
>
> typedef phys_addr_t resource_size_t;

Hmm. I guess they probably do that because phys_addr_t could differ
from size_t? Sounds like we may want a typedef called phys_addr_t
somewhere on the Rust side?

> >> +        match self.resource(resource) {
> >> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
> >> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
> >> +            Err(e) => Err(e),
> >> +        }
> >> +    }
> >> +
> >> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> >> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> >> +        let resource = unsafe {
> >> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> >> +        };
> >> +        if !resource.is_null() {
> >> +            Ok(resource)
> >> +        } else {
> >> +            Err(EINVAL)
> >> +        }
> >> +    }
> >> }
> >>
> >> impl AsRef<device::Device> for Device {
> >> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
> >>         &self.0
> >>     }
> >> }
> >> +
> >> +/// A I/O-mapped memory region for platform devices.
> >> +///
> >> +/// See also [`kernel::pci::Bar`].
> >> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
> >> +
> >> +impl<const SIZE: usize> IoMem<SIZE> {
> >> +    /// Creates a new `IoMem` instance.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// The caller must make sure that `paddr` is a valid MMIO address.
> >> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
> >
> > What happens if `size != SIZE`?
>
>
> ```
> +impl<const SIZE: usize> Io<SIZE> {
> + ///
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> + /// `maxsize`.
> + pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
> +       if maxsize < SIZE {
> +               return Err(EINVAL);
> +        }
> ```
>
> As we’ve seen in the other code snippets I pasted, it’s SIZE that is used in the try_read and
> try_write checks.

Ok.

Alice

Re: [PATCH] rust: platform: add Io support

[Daniel Almeida]

Hi Danilo, thanks for the review!

> On 29 Oct 2024, at 10:42, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On 10/24/24 4:20 PM, Daniel Almeida wrote:
>> The IoMem is backed by ioremap_resource()
> 
> ioremap_resource()?

Yeah, a small mixup with the similarly named `devm_ioremap_resource`.

Thanks for catching that.

> 
> Also, that's a rather short commit message for such a core piece of
> infrastructure, can you please expand on this?
> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> The PCI/Platform abstractions are in-flight and can be found at:
>> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
>> ---
>>  rust/bindings/bindings_helper.h |  1 +
>>  rust/helpers/io.c               | 11 ++++++
>>  rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 100 insertions(+)
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -13,6 +13,7 @@
>>  #include <linux/errname.h>
>>  #include <linux/ethtool.h>
>>  #include <linux/firmware.h>
>> +#include <linux/ioport.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/mdio.h>
>>  #include <linux/of_device.h>
>> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
>> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
>> --- a/rust/helpers/io.c
>> +++ b/rust/helpers/io.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>    #include <linux/io.h>
>> +#include <linux/ioport.h>
>>    u8 rust_helper_readb(const volatile void __iomem *addr)
>>  {
>> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
>>   writeq_relaxed(value, addr);
>>  }
>>  #endif
>> +
>> +resource_size_t rust_helper_resource_size(struct resource *res)
>> +{
>> + return resource_size(res);
>> +}
>> +
>> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
>> +{
>> + return ioremap(addr, size);
>> +}
>> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
>> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
>> --- a/rust/kernel/platform.rs
>> +++ b/rust/kernel/platform.rs
>> @@ -4,11 +4,15 @@
>>  //!
>>  //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>>  +use core::ops::Deref;
>> +
>>  use crate::{
>>      bindings, container_of, device,
>>      device_id::RawDeviceId,
>> +    devres::Devres,
>>      driver,
>>      error::{to_result, Result},
>> +    io::Io,
>>      of,
>>      prelude::*,
>>      str::CStr,
>> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>>          // embedded in `struct platform_device`.
>>          unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>>      }
>> +
>> +    /// Maps a platform resource through ioremap() where the size is known at
>> +    /// compile time.
>> +    pub fn ioremap_resource_sized<const SIZE: usize>(
>> +        &self,
>> +        resource: u32,
>> +    ) -> Result<Devres<IoMem<SIZE>>> {
>> +        let res = self.resource(resource)?;
>> +        let size = self.resource_len(resource)? as usize;
>> +
>> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
>> +        // is given by the kernel as per `self.resource_len()`.
>> +        let io = unsafe { IoMem::new(res as _, size) }?;
>> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
>> +
>> +        Ok(devres)
>> +    }
>> +
>> +    /// Maps a platform resource through ioremap().
>> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
>> +        self.ioremap_resource_sized::<0>(resource)
>> +    }
>> +
>> +    /// Returns the resource len for `resource`, if it exists.
>> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
>> +        match self.resource(resource) {
>> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
>> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
>> +            Err(e) => Err(e),
>> +        }
>> +    }
>> +
>> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
>> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
>> +        let resource = unsafe {
>> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
>> +        };
>> +        if !resource.is_null() {
>> +            Ok(resource)
>> +        } else {
>> +            Err(EINVAL)
>> +        }
>> +    }
>>  }
>>    impl AsRef<device::Device> for Device {
>> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
>>          &self.0
>>      }
>>  }
>> +
>> +/// A I/O-mapped memory region for platform devices.
>> +///
>> +/// See also [`kernel::pci::Bar`].
>> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
> 
> Is this meant to be bus specific, such as `pci::Bar` is PCI specific?

Yes.

> If so, I think it'd be better to pass the platform device and resource index to
> `platform::IoMem` (or whatever we want to call it) and let it take care of everything.

Ack.

> 
>> +
>> +impl<const SIZE: usize> IoMem<SIZE> {
>> +    /// Creates a new `IoMem` instance.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must make sure that `paddr` is a valid MMIO address.
>> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
>> +        // SAFETY: the safety requirements assert that `paddr` is a valid MMIO address.
>> +        let addr = unsafe { bindings::ioremap(paddr as _, size as u64) };
> 
> Do we need to consider ioremap_uc(), ioremap_wc(), ioremap_np()?

Do you guys need it for Nova? Maybe we can return different types depending on
which ioremap function was called.

> 
> Also, I think you're missing request_region() here.

That’s true

> 
>> +        if addr.is_null() {
>> +            return Err(ENOMEM);
>> +        }
>> +
>> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
>> +        // size `size`.
>> +        let io = unsafe { Io::new(addr as _, size)? };
>> +
>> +        Ok(IoMem(io))
>> +    }
>> +}
>> +
>> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: Safe as by the invariant of `Io`.
>> +        unsafe { bindings::iounmap(self.0.base_addr() as _) };
>> +    }
>> +}
>> +
>> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
>> +    type Target = Io<SIZE>;
>> +
>> +    fn deref(&self) -> &Self::Target {
>> +        &self.0
>> +    }
>> +}
>> ---
>> base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
>> change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887
>> Best regards,
> 
> 

— Daniel

Re: [PATCH] rust: platform: add Io support

[Daniel Almeida]

Hi Alice!

> On 29 Oct 2024, at 10:46, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Mon, Oct 28, 2024 at 7:23 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> Hi Alice,
>> 
>>> On 28 Oct 2024, at 12:37, Alice Ryhl <aliceryhl@google.com> wrote:
>>> 
>>> On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
>>> <daniel.almeida@collabora.com> wrote:
>>>> 
>>>> The IoMem is backed by ioremap_resource()
>>>> 
>>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>>> ---
>>>> The PCI/Platform abstractions are in-flight and can be found at:
>>>> 
>>>> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
>>>> ---
>>>> rust/bindings/bindings_helper.h |  1 +
>>>> rust/helpers/io.c               | 11 ++++++
>>>> rust/kernel/platform.rs         | 88 +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 100 insertions(+)
>>>> 
>>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>>> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
>>>> --- a/rust/bindings/bindings_helper.h
>>>> +++ b/rust/bindings/bindings_helper.h
>>>> @@ -13,6 +13,7 @@
>>>> #include <linux/errname.h>
>>>> #include <linux/ethtool.h>
>>>> #include <linux/firmware.h>
>>>> +#include <linux/ioport.h>
>>>> #include <linux/jiffies.h>
>>>> #include <linux/mdio.h>
>>>> #include <linux/of_device.h>
>>>> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
>>>> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..f708c09c4c87634c56af29ef22ebaa2bf51d82a9 100644
>>>> --- a/rust/helpers/io.c
>>>> +++ b/rust/helpers/io.c
>>>> @@ -1,6 +1,7 @@
>>>> // SPDX-License-Identifier: GPL-2.0
>>>> 
>>>> #include <linux/io.h>
>>>> +#include <linux/ioport.h>
>>>> 
>>>> u8 rust_helper_readb(const volatile void __iomem *addr)
>>>> {
>>>> @@ -89,3 +90,13 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
>>>>       writeq_relaxed(value, addr);
>>>> }
>>>> #endif
>>>> +
>>>> +resource_size_t rust_helper_resource_size(struct resource *res)
>>>> +{
>>>> +       return resource_size(res);
>>>> +}
>>>> +
>>>> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
>>>> +{
>>>> +       return ioremap(addr, size);
>>>> +}
>>>> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
>>>> index addf5356f44f3cf233503aed97f1aa0d32f4f062..d152432c80a4499ead30ddaebe8d87a9679bfa97 100644
>>>> --- a/rust/kernel/platform.rs
>>>> +++ b/rust/kernel/platform.rs
>>>> @@ -4,11 +4,15 @@
>>>> //!
>>>> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>>>> 
>>>> +use core::ops::Deref;
>>>> +
>>>> use crate::{
>>>>    bindings, container_of, device,
>>>>    device_id::RawDeviceId,
>>>> +    devres::Devres,
>>>>    driver,
>>>>    error::{to_result, Result},
>>>> +    io::Io,
>>>>    of,
>>>>    prelude::*,
>>>>    str::CStr,
>>>> @@ -208,6 +212,49 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>>>>        // embedded in `struct platform_device`.
>>>>        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>>>>    }
>>>> +
>>>> +    /// Maps a platform resource through ioremap() where the size is known at
>>>> +    /// compile time.
>>>> +    pub fn ioremap_resource_sized<const SIZE: usize>(
>>>> +        &self,
>>>> +        resource: u32,
>>>> +    ) -> Result<Devres<IoMem<SIZE>>> {
>>>> +        let res = self.resource(resource)?;
>>>> +        let size = self.resource_len(resource)? as usize;
>>> 
>>> You're calling platform_get_resource twice here? Can you be sure that
>>> it returns the same pointer each time?
>> 
>> This comes from the DT. Yes, it will be the same pointer (so long as we
>> pass the same index).
>> 
>> Although, I did not see where it is being called twice?
> 
> The `resource_len` function calls `resource`, so you call `resource`
> twice. Each call to `resource` results in a call to
> `platform_get_resource`.
> 
>>>> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
>>>> +        // is given by the kernel as per `self.resource_len()`.
>>>> +        let io = unsafe { IoMem::new(res as _, size) }?;
>>> 
>>> Why do we know that `res` is a valid MMIO address? Is it because
>>> platform_get_resource always does so?
>> 
>> Again, if there’s a problem, the DT itself should be fixed. Also the C code would be broken too.
> 
> Sorry if I was unclear. I just wanted you to elaborate in the safety comment :)
> 
>>>> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
>>>> +
>>>> +        Ok(devres)
>>>> +    }
>>>> +
>>>> +    /// Maps a platform resource through ioremap().
>>>> +    pub fn ioremap_resource(&self, resource: u32) -> Result<Devres<IoMem>> {
>>>> +        self.ioremap_resource_sized::<0>(resource)
>>>> +    }
>>> 
>>> I guess size zero is special?
>>> 
>> 
>> `ioremap_resource_sized` is used when you know the size at compile time. Setting SIZE == 0
>> means that you get runtime checks on whether your reads and writes are within bounds,
>> because you will have to use try_read() and try_write() instead of read() and write() or your build
>> will fail.
>> 
>> This is not related to this patch in particular, but to a preceding one that introduces struct Io.
>> 
>> We merely expose the same API from Io to users here.
>> 
>> Although I do agree it’s a bit confusing that SIZE 0 is special-cased. It took me a while and
>> several read-throughs to understand what was going on.
>> 
>> Note that it’s common to have to read the size from the DT, so deferring to runtime checks
>> seems to be unavoidable in a lot of cases if we want to have a safe API.
>> 
>> Here’s the relevant code from that commit:
>> 
>> ```
>> +macro_rules! define_write {
>> + ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
>> + /// Write 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.
>> + $(#[$attr])*
>> + #[inline]
>> + pub fn $name(&self, value: $type_name, offset: usize) {
>> +       let addr = self.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 _, ) }
>> + }
>> +
>> + /// Write IO data from a given offset.
>> + ///
>> + /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
>> + /// out of bounds.
>> + $(#[$attr])*
>> + pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
>> +       let addr = self.io_addr::<$type_name>(offset)?;
>> +
>> +       // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>> +       unsafe { bindings::$name(value, addr as _) }
>> +       Ok(())
>> + }
>> ```
>> 
>> Where,
>> 
>> ```
>> + #[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.base_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.base_addr() + offset
>> + }
>> ```
>> 
>> And
>> 
>> ```
>> + #[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
>> +       }
>> + }
>> ```
> 
> Acknowledged.
> 
>>>> +    /// Returns the resource len for `resource`, if it exists.
>>>> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
>>> 
>>> Should this just return usize? Should we have a type alias for this size type?
>> 
>> 
>> I guess usize would indeed be a better fit, if we consider the code below:
>> 
>> ```
>> #ifdef CONFIG_PHYS_ADDR_T_64BIT
>> typedef u64 phys_addr_t;
>> #else
>> typedef u32 phys_addr_t;
>> #endif
>> 
>> typedef phys_addr_t resource_size_t;
> 
> Hmm. I guess they probably do that because phys_addr_t could differ
> from size_t? Sounds like we may want a typedef called phys_addr_t
> somewhere on the Rust side?


By the way, I wonder if that connects with Gary’s work on unifying the bindgen
primitives somehow.


I think that having a #ifdef for `phys_addr_t` is pretty self-explanatory, but I have no
idea why this is not simply `size_t`. My understanding is that `size_t` and `phys_addr_t`
should be basically interchangeable, in the sense that, for example, a 32bit machine can
only address up to 0xffffffff, and, by extension, can only have objects that are 0xffffffff in size
at maximum.

This behavior is identical to usize, unless I missed something.

Maybe more knowledgeable people than me can chime in here?


> 
>>>> +        match self.resource(resource) {
>>>> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
>>>> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
>>>> +            Err(e) => Err(e),
>>>> +        }
>>>> +    }
>>>> +
>>>> +    fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
>>>> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
>>>> +        let resource = unsafe {
>>>> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
>>>> +        };
>>>> +        if !resource.is_null() {
>>>> +            Ok(resource)
>>>> +        } else {
>>>> +            Err(EINVAL)
>>>> +        }
>>>> +    }
>>>> }
>>>> 
>>>> impl AsRef<device::Device> for Device {
>>>> @@ -215,3 +262,44 @@ fn as_ref(&self) -> &device::Device {
>>>>        &self.0
>>>>    }
>>>> }
>>>> +
>>>> +/// A I/O-mapped memory region for platform devices.
>>>> +///
>>>> +/// See also [`kernel::pci::Bar`].
>>>> +pub struct IoMem<const SIZE: usize = 0>(Io<SIZE>);
>>>> +
>>>> +impl<const SIZE: usize> IoMem<SIZE> {
>>>> +    /// Creates a new `IoMem` instance.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// The caller must make sure that `paddr` is a valid MMIO address.
>>>> +    unsafe fn new(paddr: usize, size: usize) -> Result<Self> {
>>> 
>>> What happens if `size != SIZE`?
>> 
>> 
>> ```
>> +impl<const SIZE: usize> Io<SIZE> {
>> + ///
>> + ///
>> + /// # Safety
>> + ///
>> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
>> + /// `maxsize`.
>> + pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
>> +       if maxsize < SIZE {
>> +               return Err(EINVAL);
>> +        }
>> ```
>> 
>> As we’ve seen in the other code snippets I pasted, it’s SIZE that is used in the try_read and
>> try_write checks.
> 
> Ok.
> 
> Alice

— Daniel

Re: [PATCH] rust: platform: add Io support

[Alice Ryhl]

On Mon, Nov 4, 2024 at 10:28 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice!
>
> > On 29 Oct 2024, at 10:46, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Mon, Oct 28, 2024 at 7:23 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> Hi Alice,
> >>
> >>> On 28 Oct 2024, at 12:37, Alice Ryhl <aliceryhl@google.com> wrote:
> >>>
> >>> On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> >>> <daniel.almeida@collabora.com> wrote:
> >>>> +    /// Returns the resource len for `resource`, if it exists.
> >>>> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
> >>>
> >>> Should this just return usize? Should we have a type alias for this size type?
> >>
> >>
> >> I guess usize would indeed be a better fit, if we consider the code below:
> >>
> >> ```
> >> #ifdef CONFIG_PHYS_ADDR_T_64BIT
> >> typedef u64 phys_addr_t;
> >> #else
> >> typedef u32 phys_addr_t;
> >> #endif
> >>
> >> typedef phys_addr_t resource_size_t;
> >
> > Hmm. I guess they probably do that because phys_addr_t could differ
> > from size_t? Sounds like we may want a typedef called phys_addr_t
> > somewhere on the Rust side?
>
>
> By the way, I wonder if that connects with Gary’s work on unifying the bindgen
> primitives somehow.
>
>
> I think that having a #ifdef for `phys_addr_t` is pretty self-explanatory, but I have no
> idea why this is not simply `size_t`. My understanding is that `size_t` and `phys_addr_t`
> should be basically interchangeable, in the sense that, for example, a 32bit machine can
> only address up to 0xffffffff, and, by extension, can only have objects that are 0xffffffff in size
> at maximum.
>
> This behavior is identical to usize, unless I missed something.
>
> Maybe more knowledgeable people than me can chime in here?

It seems PHYS_ADDR_T_64BIT can be set even on some 32-bit machines.
See config HIGHMEM64G for 32-bit processors with more than 4 GB of
physical ram.

Alice

Re: [PATCH] rust: platform: add Io support

[Alice Ryhl]

On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
> +    /// Maps a platform resource through ioremap() where the size is known at
> +    /// compile time.
> +    pub fn ioremap_resource_sized<const SIZE: usize>(
> +        &self,
> +        resource: u32,
> +    ) -> Result<Devres<IoMem<SIZE>>> {
> +        let res = self.resource(resource)?;
> +        let size = self.resource_len(resource)? as usize;
> +
> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
> +        // is given by the kernel as per `self.resource_len()`.
> +        let io = unsafe { IoMem::new(res as _, size) }?;

This cast looks wrong to me. You're taking a pointer to `struct
resource` and casting that to an MMIO address? Shouldn't the address
be (*res).start?

Danilo already mentioned this, but I think you're missing a call to
`request_mem_region` as well.

> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;

What purpose does the Devres serve?

Alice