[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <BC47085B-4160-4D9B-89F4-06EDE706CD5A@collabora.com>
Date: Mon, 28 Oct 2024 15:22:31 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Trevor Gross <tmgross@...ch.edu>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: platform: add Io support
Hi Alice,
> On 28 Oct 2024, at 12:37, Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> <daniel.almeida@...labora.com> wrote:
>>
>> The IoMem is backed by ioremap_resource()
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.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@...labora.com>
— Daniel
Powered by blists - more mailing lists