Re: [PATCH 7/7] nova-core: mm: Add data structures for page table management

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Miguel,

On 10/20/2025 5:30 PM, Miguel Ojeda wrote:
> Hi Joel,
> 
> A few nits below (I do sometimes this kind of docs review to try to
> keep a consistent style across all Rust code).

Thanks a lot for these, I studied all of the suggestions and agree with them.
May I also suggest to add some of these suggestions to the kernel rust coding
guidelines document, that way others new to sending rust kernel patches don't
miss it (example not adding a period at the end of a markdown doc header.). But
I will make it a point to check all my patches to make sure it confirms to the
suggestions.

Also a lot of your suggestions are related to how it looks it rustdoc, so I will
try to build rustdoc and see what it looks like as well, to get an idea of when
things in my patches could be improved.

thanks,

 - Joel


> 
> On Mon, Oct 20, 2025 at 8:56 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> +//!     .set_table_frame_number(new_table.frame_number());
>> +//! // Call a function to write PDE to VRAM address
> 
> Newline between these. Period ad the end.
> 
>> +//! ## Given a PTE, Get or allocate a PFN (page frame number).
> 
> In headers, no period at the end. Also, is "Get" intended to be capitalized?
> 
>> +//!     // Call a function to read 64-bit PTE value from VRAM address
> 
> Period at the end too (more of these elsewhere).
> 
>> +//!     if pte.valid() {
>> +//!         // Return physical frame number from existing mapping
>> +//!         Ok(Pfn::new(pte.frame_number()))
> 
> Early returns where possible, like in C, i.e. to avoid indentation on
> big `else` branches.
> 
>> +/// Memory size constants
>> +pub(crate) const KB: usize = 1024;
>> +pub(crate) const MB: usize = KB * 1024;
> 
> The docs will only apply to the first item, so this probably was meant
> to be a `//` instead of a `///`.
> 
> Or you could use a module to contain these (and then possibly `use`
> them outside), and then you can have docs in the module itself, but
> that is heavier.
> 
>> +/// Page size: 4 KiB
>> +pub(crate) const PAGE_SIZE: usize = 4 * KB;
> 
> `rustdoc` would eventually render the value and the non-evaluated
> expression, and in the source code it already says `4 * KB`, so I
> think repeating the value isn't needed, unless you mean to show it is
> really a multiple of 2.
> 
>> +pub(crate) enum PageTableLevel {
>> +    Pdb, // Level 0 - Page Directory Base
>> +    L1,  // Level 1
>> +    L2,  // Level 2
>> +    L3,  // Level 3 - Dual PDE (128-bit entries)
>> +    L4,  // Level 4 - PTEs
> 
> In this case, I think you meant the other way around, i.e. actual
> docs: `///` instead of `//`.
> 
> (Also, unless there is a particular reason (e.g. it is a big table),
> please generally put comments on top of things, not at the side, which
> matches closer to what is needed for docs.)
> 
>> +    /// Convert an Address to a frame number.
> 
> These should eventually be intra-doc links, but at least please use
> for the moment backticks when referring to Rust items like types etc.
> 
>> +    /// # Example
> 
> We always use the plural for these section headers, even if there is a
> single item (e.g. single example).
> 
>> +    /// ```no_run
>> +    /// let va = VirtualAddress::default();
>> +    /// let pte_idx = va.level_index(PageTableLevel::L4);
>> +    /// ```
> 
> This will need some `use` lines -- not needed now, but just as a
> reminder that these will get actually built eventually.
> 
>> +    /// Get raw u64 value.
> 
> Intra-doc link or at least backticks.
> 
>> +    /// The valid bit is inverted so add an accessor to flip it.
>> +    pub(crate) fn set_valid(&self, value: bool) -> Pde {
> 
> This docs string sounds like a commit message.
> 
>> +/// Dual PDE at Level 3 - 128-bit entry containing both LPT and SPT pointers.
>> +/// Lower 64 bits = big/large page, upper 64 bits = small page.
> 
> It sounds like a few of these details should be on its own paragraph
> to avoid having them in the short description (title).
> 
>> +/// // Call a function to read dual PDE from VRAM address
>> +/// let mut dual_pde: DualPde = read_dual_pde(dpde_addr)?;
>> +/// // Call a function to allocate a page table and return its VRAM address
>> +/// let spt_addr = allocate_page_table()?;
>> +/// dual_pde.set_spt(Pfn::from(spt_addr), AperturePde::VideoMemory);
>> +/// // Call a function to write dual PDE to VRAM address
>> +/// write_dual_pde(dpde_addr, dual_pde)?;
> 
> Newlines before the comments, i.e. between "conceptual blocks".
> 
>> +    pub lpt: Pde, // Large/Big Page Table pointer (2MB pages) - bits 63:0 (lower)
>> +    pub spt: Pde, // Small Page Table pointer (4KB pages) - bits 127:64 (upper)
> 
> Docs instead of comments.
> 
>> +    /// Check if has valid Small Page Table.
> 
> Missing word?
> 
> Thanks!
> 
> Cheers,
> Miguel