Message-ID: <c8229530-ab87-4b51-8f2b-6199a01095b3@arm.com>
Date: Fri, 28 Nov 2025 16:47:52 +0000
From: Robin Murphy <robin.murphy@....com>
To: Alice Ryhl <aliceryhl@...gle.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 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.

>>> +    /// * 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).

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.

>>> +    }
>>> +
>>> +    /// 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.

Thanks,
Robin.

Powered by blists - more mailing lists