Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The page table walker treats valid system-memory PDEs as missing, which can lead to silent lookup failures or page table corruption if overwritten. - [High] The page table walker silently truncates system-memory Dual PDEs, bypassing the VRAM location check and causing it to read from an invalid physical address. - [Medium] The page table walker unconditionally traverses only the small page table in Dual PDEs, ignoring big page mappings and incorrectly reporting them as missing or unmapped. --
commit 8ab80bbde0121304a601722ca26dc100aca36061 Author: Joel Fernandes <[email protected]> gpu: nova-core: mm: Add page table walker for MMU v2/v3 This commit adds the page table walker implementation that traverses the page table hierarchy for both MMU v2 (5-level) and MMU v3 (6-level) to resolve virtual addresses to physical addresses or find PTE locations. > diff --git a/drivers/gpu/nova-core/mm/pagetable/walk.rs > b/drivers/gpu/nova-core/mm/pagetable/walk.rs > new file mode 100644 > index 0000000000000..76c1729971f53 > --- /dev/null > +++ b/drivers/gpu/nova-core/mm/pagetable/walk.rs [ ... ] > + pub(super) fn walk_pde_levels( > + &self, > + pramin: &mut pramin::Pramin<'_>, > + vfn: Vfn, > + resolve_prepared: impl Fn(VramAddress) -> Option<VramAddress>, > + ) -> Result<WalkPdeResult> { > + let va = VirtualAddress::from(vfn); > + let mut cur_table = self.pdb_addr; > + > + for &level in M::PDE_LEVELS { > + let idx = M::level_index(va, level.as_index()); > + let install_addr = Self::entry_addr(cur_table, level, idx); > + > + if level == M::DUAL_PDE_LEVEL { > + // 128-bit dual PDE with big+small page table pointers. > + let dpde = M::DualPde::read(pramin, install_addr)?; > + if dpde.has_small() { [Severity: Medium] Are big page mappings intentionally ignored here? The walker unconditionally traverses only the small page table in Dual PDEs. If a virtual address is mapped via a big page, this would either fall through to WalkPdeResult::Missing (if has_small() is false) or incorrectly search the small page table and fail, resulting in lookup failures instead of returning an explicit error or resolving the mapping. > + cur_table = dpde.small_vram_address(); [Severity: High] Does this silently truncate system-memory Dual PDEs? Unlike the regular PDE path which explicitly checks pde.is_valid_vram() to prevent truncating the wider system-memory frame number, this Dual PDE path unconditionally calls dpde.small_vram_address(). If a Dual PDE points to a small page table in system memory, this could read from an invalid physical VRAM address, leading to data corruption or a GPU fault. > + continue; > + } > + } else { > + // Regular 64-bit PDE. Use `is_valid_vram()` because > + // `table_vram_address()` only reads the VRAM frame-number > + // bitfield; system-memory PDEs store the address in a > + // different (wider) field and would be silently truncated. > + let pde = M::Pde::read(pramin, install_addr)?; > + if pde.is_valid_vram() { > + cur_table = pde.table_vram_address(); > + continue; > + } > + } > + > + // PDE missing in HW. Ask caller for resolution. > + if let Some(prepared_addr) = resolve_prepared(install_addr) { [Severity: High] Could this lead to overwriting valid system-memory PDEs? When walk_pde_levels() encounters a regular PDE that points to system memory, pde.is_valid_vram() will return false. The logic then falls through to this "PDE missing" block. If the walker is used to allocate or install a missing PDE, it could instruct the caller to overwrite the valid system-memory PDE with a new VRAM PDE, causing a memory leak of the old page table and loss of its mappings. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
