Re: [PATCH v4] io: add io_pgtable abstraction

[Alice Ryhl]

On Fri, Dec 19, 2025 at 08:50:06AM -0300, Daniel Almeida wrote:
> 
> 
> > On 19 Dec 2025, at 08:43, Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> > 
> > On Fri, Dec 19, 2025 at 08:04:17AM -0300, Daniel Almeida wrote:
> >> Hi Alice,
> >> 
> >>> On 19 Dec 2025, at 07:50, Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> >>> 
> >>> From: Asahi Lina <lina+kernel@xxxxxxxxxxxxx>
> >>> 
> >>> This will be used by the Tyr driver to create and modify the page table
> >>> of each address space on the GPU. Each time a mapping gets created or
> >>> removed by userspace, Tyr will call into GPUVM, which will figure out
> >>> which calls to map_pages and unmap_pages are required to map the data in
> >>> question in the page table so that the GPU may access those pages when
> >>> using that address space.
> >>> 
> >>> The Rust type wraps the struct using a raw pointer rather than the usual
> >>> Opaque+ARef approach because Opaque+ARef requires the target type to be
> >>> refcounted.
> >>> 
> >>> Signed-off-by: Asahi Lina <lina+kernel@xxxxxxxxxxxxx>
> >>> Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >>> Co-developed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> >>> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > 
> >>> +/// An io page table using a specific format.
> >>> +///
> >>> +/// # Invariants
> >>> +///
> >>> +/// The pointer references a valid io page table.
> >>> +pub struct IoPageTable<F: IoPageTableFmt> {
> >>> +    ptr: NonNull<bindings::io_pgtable_ops>,
> >>> +    _marker: PhantomData<F>,
> >>> +}
> >>> +
> >>> +// SAFETY: `struct io_pgtable_ops` is not restricted to a single thread.
> >>> +unsafe impl<F: IoPageTableFmt> Send for IoPageTable<F> {}
> >>> +// SAFETY: `struct io_pgtable_ops` may be accessed concurrently.
> >>> +unsafe impl<F: IoPageTableFmt> Sync for IoPageTable<F> {}
> >>> +
> >>> +/// The format used by this page table.
> >>> +pub trait IoPageTableFmt: 'static {
> >>> +    /// The value representing this format.
> >>> +    const FORMAT: io_pgtable_fmt;
> >>> +}
> >>> +
> >>> +impl<F: IoPageTableFmt> IoPageTable<F> {
> >> 
> >> I don’t see a reason to keep struct Foo and impl Foo separate.
> >> 
> >> IMHO, these should always be together, as the first thing one wants
> >> to read after a type declaration is its implementation.
> > 
> > I thought it was pretty natural like this. First we describe the page
> > table, then we say it's thread safe, then we describe that a page table
> > must specify a FORMAT, then we describe that it has a constructor,
> > then we say you can map pages, etc. etc.
> 
> Right, this is more a personal preference thing anyways. Fine with me if you
> want to keep it like this.
> 
> > 
> >>> +    /// Create a new `IoPageTable` as a device resource.
> >>> +    #[inline]
> >>> +    pub fn new(
> >>> +        dev: &Device<Bound>,
> >>> +        config: Config,
> >>> +    ) -> impl PinInit<Devres<IoPageTable<F>>, Error> + '_ {
> >>> +        // SAFETY: Devres ensures that the value is dropped during device unbind.
> >>> +        Devres::new(dev, unsafe { Self::new_raw(dev, config) })
> >>> +    }
> >>> +
> >>> +    /// Create a new `IoPageTable`.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// If successful, then the returned value must be dropped before the device is unbound.
> >>> +    #[inline]
> >>> +    pub unsafe fn new_raw(dev: &Device<Bound>, config: Config) -> Result<IoPageTable<F>> {
> >>> +        let mut raw_cfg = bindings::io_pgtable_cfg {
> >>> +            quirks: config.quirks,
> >>> +            pgsize_bitmap: config.pgsize_bitmap,
> >>> +            ias: config.ias,
> >>> +            oas: config.oas,
> >>> +            coherent_walk: config.coherent_walk,
> >>> +            tlb: &raw const NOOP_FLUSH_OPS,
> >>> +            iommu_dev: dev.as_raw(),
> >>> +            // SAFETY: All zeroes is a valid value for `struct io_pgtable_cfg`.
> >>> +            ..unsafe { core::mem::zeroed() }
> >>> +        };
> >>> +
> >>> +        // SAFETY:
> >>> +        // * The raw_cfg pointer is valid for the duration of this call.
> >>> +        // * The provided `FLUSH_OPS` contains valid function pointers that accept a null pointer
> >>> +        //   as cookie.
> >>> +        // * The caller ensures that the io pgtable does not outlive the device.
> >> 
> >> We should probably tailor the sentence above for Devres?
> > 
> > Maybe "does not outlive device unbind" is better worded, but not sure
> > what you're looking for with Devres tailoring.
> 
> What about “Devres ensures that the io potable does not outlive device
> unbind by revoking access”, or something along these lines?

The `new_raw` method does not require the caller to use `Devres` to do
that, so it's not necessarily the case that it is Devres that ensures
this. An end-user could call `new_raw` directly and use some other
mechanism if they wish.

> >>> +        let ops = unsafe {
> >>> +            bindings::alloc_io_pgtable_ops(F::FORMAT, &mut raw_cfg, core::ptr::null_mut())
> >>> +        };
> >> 
> >> I’d add a blank here.
> >> 
> >>> +impl<F: IoPageTableFmt> Drop for IoPageTable<F> {
> >>> +    fn drop(&mut self) {
> >>> +        // SAFETY: The caller of `ttbr` promised that the page table is not live when this
> >>> +        // destructor runs.
> >> 
> >> 
> >> Not sure I understand this sentence. Perhaps we should remove the word “ttbr” from here? ttbr is a register.
> > 
> > ttbr is a method defined below with a safety requirement.
> 
> Can't we link to that then? i.e.: [`ttbr`]: Self::ttbr, or whatever the right
> syntax is. Because it’s more natural to think about ttbr the register vs
> ttbr the method.

This isn't a doc comment. There's no such thing as a link in normal
comments. But I can write:

	The caller of `Self::ttbr()` promised that the ...

Alice