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

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 at
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

On Mon, Nov 3, 2025 at 8:21?PM Joel Fernandes <joelagnelf at nvidia.com>
wrote:
>
> 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
You're welcome!

I don't think everyone reads the documentation, and one issue is that
the longer it is, the less people may read it. For instance, the note
about using "Examples" as the section name is already explicitly there
and other bits can be inferred from the examples' style.

Now, in 2025, thanks to AI, you actually have a point, in the sense
that I assume people may be able to point a patch to an AI to ask it
to apply the guidelines from such a document.

So a good way forward may be best to have a list of "short
rules/examples" in a separate section or document, where I can easily
add entries with a simple example without too much explanation. Yeah,
I think I will do that.
> 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.
Definitely, please do!

We all should be doing it, especially so when the changes aren't
trivial (e.g. adding an entire new feature/API).

I have it in the "Subsystem Profile" document from `MAINTAINERS`:

    https://rust-for-linux.com/contributing#submit-checklist-addendum

    "When submitting changes to Rust code documentation, please render
them using the `rustdoc` target and ensure the result looks as
expected."

Cheers,
Miguel

On Tue Nov 4, 2025 at 6:54 PM CET, Miguel Ojeda wrote:
> On Mon, Nov 3, 2025 at 8:21?PM Joel Fernandes <joelagnelf at
nvidia.com> wrote:
>> 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.
For the drm-rust tree we also have a summary [1] of things committers are
expected to check (including a link to the generic kernel and Rust checklist).
> Definitely, please do!
@Joel: Just be aware that for leaf crates no documentation is built yet, only
the kernel crate is built.

[1]
https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-rust.html#submit-checklist