Re: [PATCH 05/16] gpu: nova-core: mm: pagetable: Add PdeOps trait

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `PdeOps` trait only provides a `table_vram_address()` getter instead of a generic `table_pfn()`, causing a semantic mismatch and data corruption for system memory PDEs.
--

commit 43d16f339a2596bedc1626661f8f6564c220f1cb
Author: Joel Fernandes <joelagnelf@nvidia.com>

gpu: nova-core: mm: pagetable: Add PdeOps trait
    
Introduce a trait for GPU Page Directory Entries (PDEs). Default
`read()`/`write()` helpers via a `Pramin` are provided. The forthcoming MMU
v2, v3 PDE structs will each implement `PdeOps`, allowing the later page-table
walker and mapper to call PDE operations.

> diff --git a/drivers/gpu/nova-core/mm/pagetable.rs b/drivers/gpu/nova-core/mm/pagetable.rs
> index a23370b4b62d1..14436fa6adf04 100644
> --- a/drivers/gpu/nova-core/mm/pagetable.rs
> +++ b/drivers/gpu/nova-core/mm/pagetable.rs
> @@ -128,6 +128,47 @@ fn write(&self, pramin: &mut pramin::Pramin<'_>, addr: VramAddress) -> Result {
>      }
>  }
>  
> +/// Operations on Page Directory Entries (`PDE`s).
> +pub(super) trait PdeOps: Copy + core::fmt::Debug + Into<u64> {

[ ... ]

> +    /// Get the memory aperture of this `PDE`.
> +    fn aperture(&self) -> AperturePde;
> +
> +    /// Get the VRAM address of the page table.
> +    fn table_vram_address(&self) -> VramAddress;

[Severity: High]
Does this trait need a generic table_pfn() method instead of forcing VRAM
addresses?

When a PDE points to system memory (AperturePde::SystemCoherent), forcing the
implementation to return a VRAM address causes issues. For instance, in
PtWalkInner::walk_pde_levels(), the page table walker calls
dpde.small_vram_address() (which delegates to PdeOps::table_vram_address)
after only checking if the PDE has a small page table:

drivers/gpu/nova-core/mm/pagetable/walk.rs:PtWalkInner::walk_pde_levels() {
    ...
                if dpde.has_small() {
                    cur_table = dpde.small_vram_address();
                    continue;
                }
    ...
}

Because system memory PFNs are often stored in different bitfields than VRAM
PFNs, returning a VRAM address for a system memory PDE can result in
returning a corrupted frame number or panicking in debug builds, which
would cause the walker to perform an out-of-bounds VRAM read via pramin.

> +
> +    /// Read a `PDE` from VRAM.
> +    fn read(pramin: &mut pramin::Pramin<'_>, addr: VramAddress) -> Result<Self> {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-mmrebase-v1-0-8dd5d4225d2e@nvidia.com?part=5