Re: [PATCH v4] io: add io_pgtable abstraction

[Daniel Almeida <daniel.almeida@collabora.com>]

> On 19 Dec 2025, at 08:43, Alice Ryhl <aliceryhl@google.com> 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@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.

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?

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

> 
> Alice