Message-ID: <aS1m4DawouC1utSj@google.com>
Date: Mon, 1 Dec 2025 09:58:56 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Robin Murphy <robin.murphy@....com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Will Deacon <will@...nel.org>, 
	Daniel Almeida <daniel.almeida@...labora.com>, 
	Boris Brezillon <boris.brezillon@...labora.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, 
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Joerg Roedel <joro@...tes.org>, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	Asahi Lina <lina+kernel@...hilina.net>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, iommu@...ts.linux.dev, linux-mm@...ck.org
Subject: Re: [PATCH v3] io: add io_pgtable abstraction

On Fri, Nov 28, 2025 at 04:47:52PM +0000, Robin Murphy wrote:
> On 2025-11-28 12:27 pm, Alice Ryhl wrote:
> [...]
> > > > +    /// Map a physically contiguous range of pages of the same size.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// * This page table must not contain any mapping that overlaps with the mapping created by
> > > > +    ///   this call.
> > > 
> > > As mentioned this isn't necessarily true of io-pgtable itself, but since
> > > you've not included QUIRK_NO_WARN in the abstraction then it's fair if this
> > > layer wants to be a little stricter toward Rust users.
> > 
> > Assuming that we don't allow QUICK_NO_WARN, would you say that it's
> > precise as-is?
> 
> As an assumption of use for the Rust API, like I say it's fine - it's still
> not really "unsafe" if a caller did try an overlapping mapping; the call
> will still fail gracefully and accurately, it's just it will also fire a
> WARN_ON() since ARM_64_LPAE_S1 without IO_PGTABLE_QUIRK_NO_WARN considers
> this indicative of a usage error or race in the caller.
> 
> If we do end up wanting to support more opportunistic and/or
> userspace-controlled mappings by Rust drivers in future then we can relax
> this expectation as appropriate.

Yeah, let's just say that it's an unsupported use-case. These bindings
can be expanded in the future if anyone needs QUICK_NO_WARN.

> > > > +    /// * If this page table is live, then the caller must ensure that it's okay to access the
> > > > +    ///   physical address being mapped for the duration in which it is mapped.
> > > > +    #[inline]
> > > > +    pub unsafe fn map_pages(
> > > > +        &self,
> > > > +        iova: usize,
> > > > +        paddr: PhysAddr,
> > > > +        pgsize: usize,
> > > > +        pgcount: usize,
> > > > +        prot: u32,
> > > > +        flags: alloc::Flags,
> > > > +    ) -> Result<usize> {
> > > > +        let mut mapped: usize = 0;
> > > > +
> > > > +        // SAFETY: The `map_pages` function in `io_pgtable_ops` is never null.
> > > > +        let map_pages = unsafe { (*self.raw_ops()).map_pages.unwrap_unchecked() };
> > > > +
> > > > +        // SAFETY: The safety requirements of this method are sufficient to call `map_pages`.
> > > > +        to_result(unsafe {
> > > > +            (map_pages)(
> > > > +                self.raw_ops(),
> > > > +                iova,
> > > > +                paddr,
> > > > +                pgsize,
> > > > +                pgcount,
> > > > +                prot as i32,
> > > > +                flags.as_raw(),
> > > > +                &mut mapped,
> > > > +            )
> > > > +        })?;
> > > > +
> > > > +        Ok(mapped)
> > > 
> > > Just to double-check since I'm a bit unclear on the Rust semantics, this can
> > > correctly reflect all 4 outcomes back to the caller, right? I.e.:
> > > 
> > > - no error, mapped == pgcount * pgsize (success)
> > > - no error, mapped < pgcount * pgsize (call again with the remainder)
> > > - error, mapped > 0 (probably unmap that bit, unless clever trickery where
> > > an error was expected)
> > > - error, mapped == 0 (nothing was done, straightforward failure)
> > > 
> > > (the only case not permitted is "no error, mapped == 0" - failure to make
> > > any progress must always be an error)
> > > 
> > > Alternatively you might want to consider encapsulating the partial-mapping
> > > handling in this layer as well - in the C code that's done at the level of
> > > the IOMMU API calls that io-pgtable-using IOMMU drivers are merely passing
> > > through, hence why panfrost/panthor have to open-code their own equivalents,
> > > but there's no particular reason to follow the *exact* same pattern here.
> > 
> > Ah, no this signature does not reflect all of those cases. The return
> > type is Result<usize>, which corresponds to:
> > 
> > struct my_return_type {
> >      bool success;
> >      union {
> >          size_t ok;
> >          int err; // an errno
> >      }
> > };
> > 
> > We need a different signature if it's possible to have mapped != 0 when
> > returning an error.
> 
> Aha, thanks for clarifying - indeed this is not the common "value or error"
> case, it is two (almost) orthogonal return values. However if we're not
> permitting callers to try to do anything clever with -EEXIST then it might
> make sense to just embed the inevitable cleanup-on-failure boilerplate here
> anyway (even if we still leave retry-on-partial-success to the caller).

Is the only possible error -EEXIST? I could encode that in the API if
that is the case.

> Note that it does appear to be the case that io-pgtable-arm in its current
> state won't actually do this, since it happens to handle all its error
> return cases before any leaf PTEs are touched and "mapped" is updated, but
> the abstraction layer shouldn't assume that in general since other
> implementations like io-pgtable-arm-v7s definitely *can* fail with a partial
> mapping.

Agreed, I will update the API accordingly.

> > > > +    }
> > > > +
> > > > +    /// Unmap a range of virtually contiguous pages of the same size.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// This page table must contain a mapping at `iova` that consists of exactly `pgcount` pages
> > > > +    /// of size `pgsize`.
> > > 
> > > Again, the underlying requirement here is only that pgsize * pgcount
> > > represents the IOVA range of one or more consecutive ranges previously
> > > mapped, i.e.:
> > > 
> > > map(0, 4KB * 256);
> > > map(1MB, 4KB * 256);
> > > unmap(0, 2MB * 1);
> > > 
> > > is legal, since it's generally impractical for callers to know and keep
> > > track of the *exact* structure of a given pagetable. In this case there
> > > isn't really any good reason to try to be stricter.
> > 
> > How about this wording?
> > 
> > This page table must contain one or more consecutive mappings starting
> > at `iova` whose total size is `pgcount*pgsize`.
> 
> Yes, that's a nice way to put it.

Perfect thanks.

Alice

Powered by blists - more mailing lists