Re: [PATCH v4] io: add io_pgtable abstraction

[Alice Ryhl <aliceryhl@google.com>]

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@google.com> wrote:
> > 
> > From: Asahi Lina <lina+kernel@asahilina.net>
> > 
> > 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@asahilina.net>
> > Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>

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

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

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

Alice