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 <[email protected]> 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/[email protected]?part=5
