rust/bindings/bindings_helper.h | 1 + rust/helpers/io.c | 11 ++++++ rust/kernel/platform.rs | 88 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+)
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>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
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,
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
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>
>
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
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
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
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
© 2016 - 2024 Red Hat, Inc.