lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH5fLgjdDm3nNvR8g-a6Z8UsSnEDygLJ8i3u63aCrpG5ambQ3A@mail.gmail.com>
Date: Mon, 28 Oct 2024 16:37:34 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.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

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?

> +        // 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@...labora.com>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ