Message-Id: <935A78FF-0851-4A72-BCCB-C33E3F4BF61C@collabora.com>
Date: Mon, 4 Nov 2024 18:28:14 -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 29 Oct 2024, at 10:46, Alice Ryhl <aliceryhl@...gle.com> wrote:
> 
> On Mon, Oct 28, 2024 at 7:23 PM Daniel Almeida
> <daniel.almeida@...labora.com> wrote:
>> 
>> 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?
> 
> 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

Powered by blists - more mailing lists