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).
On Mon, Oct 20, 2025 at 8:56 PM Joel Fernandes <[email protected]> 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
