linux-kernel - Re: [PATCH v3] rust: platform: add Io support

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1oI5JwExd1stnT-@pollux.localdomain>
Date: Wed, 11 Dec 2024 22:49:24 +0100
From: Danilo Krummrich <dakr@...nel.org>
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>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Boris Brezillon <boris.brezillon@...labora.com>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rust: platform: add Io support

On Wed, Dec 11, 2024 at 06:00:31PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> > On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@...nel.org> wrote:
> >> +///
> >> +///     // Read and write a 32-bit value at `offset`. Calling `try_access()` on
> >> +///     // the `Devres` makes sure that the resource is still valid.
> >> +///     let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
> >> +///
> >> +///     iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
> >> +///
> >> +///     // Unlike `ioremap_resource_sized`, here the size of the memory region
> >> +///     // is not known at compile time, so only the `try_read*` and `try_write*`
> >> +///     // family of functions are exposed, leading to runtime checks on every
> >> +///     // access.
> >> +///     let iomem = pdev.ioremap_resource(0, None)?;
> >> +///
> >> +///     let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
> >> +///
> >> +///     iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
> >> +///
> >> +///     # Ok::<(), Error>(())
> >> +/// }
> >> +/// ```
> >> +///
> >> +pub struct IoMem<const SIZE: usize = 0> {
> >> +    io: IoRaw<SIZE>,
> >> +    res_start: u64,
> >> +    exclusive: bool,
> >> +}
> > 
> > I think both the `Resource` and `IoMem` implementation do not belong into
> > platform.rs. Neither of those depends on any platform bus structures. They're
> > only used by platform structures.
> > 
> > I think we should move this into files under rust/kernel/io/ and create separate
> > commits out of this one.
> 
> Just to be clear, one commit with the boilerplate to create rust/kernel/io, and another one with
> kernel::io::Resource and kernel::io::IoMem?

I don't think there will be much boilerplate. I was thinking of one for
io/resource.rs and one for io/mem.rs. Does that make sense?

> 
> > 
> >> +
> >> +impl<const SIZE: usize> IoMem<SIZE> {
> >> +    /// Creates a new `IoMem` instance.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// The caller must ensure that `IoMem` does not outlive the device it is
> >> +    /// associated with, usually by wrapping the `IoMem` in a `Devres`.
> > 
> > More precisely, `Devres` revokes when the device is unbound from the matched
> > driver, i.e. the driver should not be able to control the device anymore. This
> > may be much earlier than when the device disappears.
> > 
> >> +    unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> >> +        let size = resource.size();
> >> +        if size == 0 {
> >> +            return Err(ENOMEM);
> >> +        }
> >> +
> >> +        let res_start = resource.start();
> >> +
> >> +        // SAFETY:
> >> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> >> +        // - `size` is known not to be zero at this point.
> >> +        // - `resource.name()` returns a valid C string.
> >> +        let mem_region =
> >> +            unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
> > 
> > This should only be called if exclusive == true, right?
> 
> Yes (oops)
> 
> > 
> > Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
> > partial exclusive mappings?
> 
> Rob pointed out that lots of drivers do not call `request_mem_region` in his review for v2, which
> Is why I added support for non-exclusive access.
> 
> What do you mean by `partial exclusive mappings` ?

I was assuming that the reason for non-exclusive access would be that a single
resource contains multiple hardware interfaces, hence multiple mappings.

If we allow partial mappings of the resource and make them exclusive instead it
would be a better solution IMHO.

But this presumes that we don't need non-exclusive access for different reasons.

If not for the reason of having multiple hardware interfaces in the same
resource, which reasons do we have (in Rust) to have multiple mappings of the
same thing?