Add data structures and helpers for page table management. Uses
bitfield for cleanly representing and accessing the bitfields in the
structures.
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
drivers/gpu/nova-core/mm/mod.rs | 1 +
drivers/gpu/nova-core/mm/types.rs | 405 ++++++++++++++++++++++++++++++
2 files changed, 406 insertions(+)
create mode 100644 drivers/gpu/nova-core/mm/types.rs
diff --git a/drivers/gpu/nova-core/mm/mod.rs b/drivers/gpu/nova-core/mm/mod.rs
index 54c7cd9416a9..f4985780a8a1 100644
--- a/drivers/gpu/nova-core/mm/mod.rs
+++ b/drivers/gpu/nova-core/mm/mod.rs
@@ -1,3 +1,4 @@
// SPDX-License-Identifier: GPL-2.0
pub(crate) mod pramin;
+pub(crate) mod types;
diff --git a/drivers/gpu/nova-core/mm/types.rs b/drivers/gpu/nova-core/mm/types.rs
new file mode 100644
index 000000000000..0a2dec6b9145
--- /dev/null
+++ b/drivers/gpu/nova-core/mm/types.rs
@@ -0,0 +1,405 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Page table data management for NVIDIA GPUs.
+//!
+//! This module provides data structures for GPU page table management, including
+//! address types, page table entries (PTEs), page directory entries (PDEs), and
+//! the page table hierarchy levels.
+//!
+//! # Examples
+//!
+//! ## Creating and writing a PDE
+//!
+//! ```no_run
+//! let new_pde = Pde::default()
+//! .set_valid(true)
+//! .set_aperture(AperturePde::VideoMemory)
+//! .set_table_frame_number(new_table.frame_number());
+//! // Call a function to write PDE to VRAM address
+//! write_pde(pde_addr, new_pde)?;
+//! ```
+//!
+//! ## Given a PTE, Get or allocate a PFN (page frame number).
+//!
+//! ```no_run
+//! fn get_frame_number(pte_addr: VramAddress) -> Result<Pfn> {
+//! // Call a function to read 64-bit PTE value from VRAM address
+//! let pte = Pte(read_u64_from_vram(pte_addr)?);
+//! if pte.valid() {
+//! // Return physical frame number from existing mapping
+//! Ok(Pfn::new(pte.frame_number()))
+//! } else {
+//! // Create new PTE mapping
+//! // Call a function to allocate a physical page, returning a Pfn
+//! let phys_pfn = allocate_page()?;
+//! let new_pte = Pte::default()
+//! .set_valid(true)
+//! .set_frame_number(phys_pfn.raw())
+//! .set_aperture(AperturePte::VideoMemory)
+//! .set_privilege(false) // User-accessible
+//! .set_read_only(false); // Writable
+//!
+//! // Call a function to write 64-bit PTE value to VRAM address
+//! write_u64_to_vram(pte_addr, new_pte.raw())?;
+//! Ok(phys_pfn)
+//! }
+//! }
+//! ```
+
+#![expect(dead_code)]
+
+/// Memory size constants
+pub(crate) const KB: usize = 1024;
+pub(crate) const MB: usize = KB * 1024;
+
+/// Page size: 4 KiB
+pub(crate) const PAGE_SIZE: usize = 4 * KB;
+
+/// Page Table Level hierarchy
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+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
+}
+
+impl PageTableLevel {
+ /// Get the entry size for this level.
+ pub(crate) fn entry_size(&self) -> usize {
+ match self {
+ Self::L3 => 16, // 128-bit dual PDE
+ _ => 8, // 64-bit PDE/PTE
+ }
+ }
+
+ /// PDE levels constant array for iteration.
+ const PDE_LEVELS: [PageTableLevel; 4] = [
+ PageTableLevel::Pdb,
+ PageTableLevel::L1,
+ PageTableLevel::L2,
+ PageTableLevel::L3,
+ ];
+
+ /// Get iterator over PDE levels.
+ pub(crate) fn pde_levels() -> impl Iterator<Item = PageTableLevel> {
+ Self::PDE_LEVELS.into_iter()
+ }
+}
+
+/// Memory aperture for Page Directory Entries (PDEs)
+#[repr(u8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+pub(crate) enum AperturePde {
+ #[default]
+ Invalid = 0,
+ VideoMemory = 1,
+ SystemCoherent = 2,
+ SystemNonCoherent = 3,
+}
+
+impl From<u8> for AperturePde {
+ fn from(val: u8) -> Self {
+ match val {
+ 1 => Self::VideoMemory,
+ 2 => Self::SystemCoherent,
+ 3 => Self::SystemNonCoherent,
+ _ => Self::Invalid,
+ }
+ }
+}
+
+impl From<AperturePde> for u8 {
+ fn from(val: AperturePde) -> Self {
+ val as u8
+ }
+}
+
+/// Memory aperture for Page Table Entries (PTEs)
+#[repr(u8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+pub(crate) enum AperturePte {
+ #[default]
+ VideoMemory = 0,
+ PeerVideoMemory = 1,
+ SystemCoherent = 2,
+ SystemNonCoherent = 3,
+}
+
+impl From<u8> for AperturePte {
+ fn from(val: u8) -> Self {
+ match val {
+ 0 => Self::VideoMemory,
+ 1 => Self::PeerVideoMemory,
+ 2 => Self::SystemCoherent,
+ 3 => Self::SystemNonCoherent,
+ _ => Self::VideoMemory,
+ }
+ }
+}
+
+impl From<AperturePte> for u8 {
+ fn from(val: AperturePte) -> Self {
+ val as u8
+ }
+}
+
+/// Common trait for address types
+pub(crate) trait Address {
+ /// Get raw u64 value.
+ fn raw(&self) -> u64;
+
+ /// Convert an Address to a frame number.
+ fn frame_number(&self) -> u64 {
+ self.raw() >> 12
+ }
+
+ /// Get the frame offset within an Address.
+ fn frame_offset(&self) -> u16 {
+ (self.raw() & 0xFFF) as u16
+ }
+}
+
+bitfield! {
+ pub(crate) struct VramAddress(u64), "Physical VRAM address representation." {
+ 11:0 offset as u16; // Offset within 4KB page
+ 63:12 frame_number as u64; // Frame number
+ }
+}
+
+impl Address for VramAddress {
+ fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+impl From<Pfn> for VramAddress {
+ fn from(pfn: Pfn) -> VramAddress {
+ VramAddress::default().set_frame_number(pfn.raw())
+ }
+}
+
+bitfield! {
+ pub(crate) struct VirtualAddress(u64), "Virtual address representation for GPU." {
+ 11:0 offset as u16; // Offset within 4KB page
+ 20:12 l4_index as u16; // Level 4 index (PTE)
+ 29:21 l3_index as u16; // Level 3 index
+ 38:30 l2_index as u16; // Level 2 index
+ 47:39 l1_index as u16; // Level 1 index
+ 56:48 l0_index as u16; // Level 0 index (PDB)
+
+ 63:12 frame_number as u64; // Frame number (combination of levels).
+ }
+}
+
+impl VirtualAddress {
+ /// Get index for a specific page table level.
+ ///
+ /// # Example
+ ///
+ /// ```no_run
+ /// let va = VirtualAddress::default();
+ /// let pte_idx = va.level_index(PageTableLevel::L4);
+ /// ```
+ pub(crate) fn level_index(&self, level: PageTableLevel) -> u16 {
+ match level {
+ PageTableLevel::Pdb => self.l0_index(),
+ PageTableLevel::L1 => self.l1_index(),
+ PageTableLevel::L2 => self.l2_index(),
+ PageTableLevel::L3 => self.l3_index(),
+ PageTableLevel::L4 => self.l4_index(),
+ }
+ }
+}
+
+impl Address for VirtualAddress {
+ fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+impl From<Vfn> for VirtualAddress {
+ fn from(vfn: Vfn) -> VirtualAddress {
+ VirtualAddress::default().set_frame_number(vfn.raw())
+ }
+}
+
+bitfield! {
+ pub(crate) struct Pte(u64), "Page Table Entry (PTE) to map virtual pages to physical frames." {
+ 0:0 valid as bool; // (1 = valid for PTEs)
+ 1:1 privilege as bool; // P - Privileged/kernel-only access
+ 2:2 read_only as bool; // RO - Write protection
+ 3:3 atomic_disable as bool; // AD - Disable atomic ops
+ 4:4 encrypted as bool; // E - Encryption enabled
+ 39:8 frame_number as u64; // PA[39:8] - Physical frame number (32 bits)
+ 41:40 aperture as u8 => AperturePte; // Memory aperture type.
+ 42:42 volatile as bool; // VOL - Volatile flag
+ 50:43 kind as u8; // K[7:0] - Compression/tiling kind
+ 63:51 comptag_line as u16; // CTL[12:0] - Compression tag line
+ }
+}
+
+impl Pte {
+ /// Set the physical address mapped by this PTE.
+ pub(crate) fn set_address(&mut self, addr: VramAddress) {
+ self.set_frame_number(addr.frame_number());
+ }
+
+ /// Get the physical address mapped by this PTE.
+ pub(crate) fn address(&self) -> VramAddress {
+ VramAddress::default().set_frame_number(self.frame_number())
+ }
+
+ /// Get raw u64 value.
+ pub(crate) fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+bitfield! {
+ pub(crate) struct Pde(u64), "Page Directory Entry (PDE) pointing to next-level page tables." {
+ 0:0 valid_inverted as bool; // V - Valid bit (0=valid for PDEs)
+ 2:1 aperture as u8 => AperturePde; // Memory aperture type
+ 3:3 volatile as bool; // VOL - Volatile flag
+ 39:8 table_frame_number as u64; // PA[39:8] - Table frame number (32 bits)
+ }
+}
+
+impl Pde {
+ /// Check if PDE is valid.
+ pub(crate) fn is_valid(&self) -> bool {
+ !self.valid_inverted() && self.aperture() != AperturePde::Invalid
+ }
+
+ /// The valid bit is inverted so add an accessor to flip it.
+ pub(crate) fn set_valid(&self, value: bool) -> Pde {
+ self.set_valid_inverted(!value)
+ }
+
+ /// Set the physical table address mapped by this PDE.
+ pub(crate) fn set_table_address(&mut self, addr: VramAddress) {
+ self.set_table_frame_number(addr.frame_number());
+ }
+
+ /// Get the physical table address mapped by this PDE.
+ pub(crate) fn table_address(&self) -> VramAddress {
+ VramAddress::default().set_frame_number(self.table_frame_number())
+ }
+
+ /// Get raw u64 value.
+ pub(crate) fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+/// 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.
+///
+/// # Example
+///
+/// ## Set the SPT (small page table) address in a Dual PDE
+///
+/// ```no_run
+/// // 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)?;
+/// ```
+#[repr(C)]
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct DualPde {
+ 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)
+}
+
+impl DualPde {
+ /// Create a new empty dual PDE.
+ pub(crate) fn new() -> Self {
+ Self {
+ spt: Pde::default(),
+ lpt: Pde::default(),
+ }
+ }
+
+ /// Set the Small Page Table address with aperture.
+ pub(crate) fn set_small_pt_address(&mut self, addr: VramAddress, aperture: AperturePde) {
+ self.spt = Pde::default()
+ .set_valid(true)
+ .set_table_frame_number(addr.frame_number())
+ .set_aperture(aperture);
+ }
+
+ /// Set the Large Page Table address with aperture.
+ pub(crate) fn set_large_pt_address(&mut self, addr: VramAddress, aperture: AperturePde) {
+ self.lpt = Pde::default()
+ .set_valid(true)
+ .set_table_frame_number(addr.frame_number())
+ .set_aperture(aperture);
+ }
+
+ /// Check if has valid Small Page Table.
+ pub(crate) fn has_small_pt_address(&self) -> bool {
+ self.spt.is_valid()
+ }
+
+ /// Check if has valid Large Page Table.
+ pub(crate) fn has_large_pt_address(&self) -> bool {
+ self.lpt.is_valid()
+ }
+
+ /// Set SPT (Small Page Table) using Pfn.
+ pub(crate) fn set_spt(&mut self, pfn: Pfn, aperture: AperturePde) {
+ self.spt = Pde::default()
+ .set_valid(true)
+ .set_aperture(aperture)
+ .set_table_frame_number(pfn.raw());
+ }
+}
+
+/// Virtual Frame Number - virtual address divided by 4KB.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) struct Vfn(u64);
+
+impl Vfn {
+ /// Create a new VFN from a frame number.
+ pub(crate) const fn new(frame_number: u64) -> Self {
+ Self(frame_number)
+ }
+
+ /// Get raw frame number.
+ pub(crate) const fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+impl From<VirtualAddress> for Vfn {
+ fn from(vaddr: VirtualAddress) -> Self {
+ Self(vaddr.frame_number())
+ }
+}
+
+/// Physical Frame Number - physical address divided by 4KB.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) struct Pfn(u64);
+
+impl Pfn {
+ /// Create a new PFN from a frame number.
+ pub(crate) const fn new(frame_number: u64) -> Self {
+ Self(frame_number)
+ }
+
+ /// Get raw frame number.
+ pub(crate) const fn raw(&self) -> u64 {
+ self.0
+ }
+}
+
+impl From<VramAddress> for Pfn {
+ fn from(addr: VramAddress) -> Self {
+ Self(addr.frame_number())
+ }
+}
--
2.34.1On Tue Oct 21, 2025 at 3:55 AM JST, Joel Fernandes wrote: <snip> > +#![expect(dead_code)] > + > +/// Memory size constants > +pub(crate) const KB: usize = 1024; > +pub(crate) const MB: usize = KB * 1024; You can use `kernel::types::SZ_1K` and `SZ_1M` instead. > + > +/// Page size: 4 KiB > +pub(crate) const PAGE_SIZE: usize = 4 * KB; SZ_4K exists as well. :)
> On Oct 22, 2025, at 7:22 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: > > On Tue Oct 21, 2025 at 3:55 AM JST, Joel Fernandes wrote: > <snip> >> +#![expect(dead_code)] >> + >> +/// Memory size constants >> +pub(crate) const KB: usize = 1024; >> +pub(crate) const MB: usize = KB * 1024; > > You can use `kernel::types::SZ_1K` and `SZ_1M` instead. > >> + >> +/// Page size: 4 KiB >> +pub(crate) const PAGE_SIZE: usize = 4 * KB; > > SZ_4K exists as well. :) Thanks a lot, will do. - Joel >
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 <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
On 10/20/25 2: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). > > 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. > Hi Miguel, As Joel also was hinting at, is there any easy way to get this sort of thing automatically checked? This is what scripts/checkpatch.pl helps us out with on the C side, and maybe it is also the right tool for Rust...? It's sad to have a patchset pass both checkpatch.pl, and CLIPPY=1 builds, and yet still be full of typography and convention violations. thanks, -- John Hubbard
On Mon, Nov 3, 2025 at 8:29 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> As Joel also was hinting at, is there any easy way to get this sort
> of thing automatically checked? This is what scripts/checkpatch.pl
> helps us out with on the C side, and maybe it is also the right
> tool for Rust...?
We have a few patches for that script (including for at least one of
the things above), but lately I have been thinking we may want to have
a different script or tools, ideally written in Rust, to encourage
contributions and reviews and tests and so on.
Moreover, for some of the cases above it is better to put it into
other tooling like `rustdoc`, Clippy, `rustfmt` or even klint,
depending on what it is -- over time I have opened quite a few
suggestions and some were implemented and work great, see e.g.
https://github.com/Rust-for-Linux/linux/issues/349
If someone wants to help with some of that, of course, please ping me!
I also had a bot I wrote back then when we used GitHub, with quite a
few checks (especially around development process for newcomers to the
kernel, e.g. using the right SoB and tags etc.) which people seemed to
appreciate (to the point of someone mentioning it in a talk... :).
A long time ago I asked about making the bot send messages to the
mailing list when we migrated, but it wasn't OK'd back then. I can try
again, or perhaps it would make sense to make it send messages in
private.
Finally, nowadays, I imagine an LLM could do a reasonable job for some
of these as well, if there is available AI time somewhere (please see
my reply to Joel on that too).
Cheers,
Miguel
On 11/4/25 9:56 AM, Miguel Ojeda wrote: > On Mon, Nov 3, 2025 at 8:29 PM John Hubbard <jhubbard@nvidia.com> wrote: >> >> As Joel also was hinting at, is there any easy way to get this sort >> of thing automatically checked? This is what scripts/checkpatch.pl >> helps us out with on the C side, and maybe it is also the right >> tool for Rust...? > > We have a few patches for that script (including for at least one of > the things above), but lately I have been thinking we may want to have > a different script or tools, ideally written in Rust, to encourage > contributions and reviews and tests and so on. > > Moreover, for some of the cases above it is better to put it into > other tooling like `rustdoc`, Clippy, `rustfmt` or even klint, rustfmt sounds like a nice user experience: fixing things up upon file save then becomes possible. > depending on what it is -- over time I have opened quite a few > suggestions and some were implemented and work great, see e.g. > > https://github.com/Rust-for-Linux/linux/issues/349 > > If someone wants to help with some of that, of course, please ping me! > > I also had a bot I wrote back then when we used GitHub, with quite a > few checks (especially around development process for newcomers to the > kernel, e.g. using the right SoB and tags etc.) which people seemed to > appreciate (to the point of someone mentioning it in a talk... :). > > A long time ago I asked about making the bot send messages to the > mailing list when we migrated, but it wasn't OK'd back then. I can try I'm grateful for that. I think tooling provides a much happier work environment: you can run the tools locally (and we can put than in a submitters-checklist.rst), as opposed to getting an email after posting. > again, or perhaps it would make sense to make it send messages in > private. > > Finally, nowadays, I imagine an LLM could do a reasonable job for some > of these as well, if there is available AI time somewhere (please see > my reply to Joel on that too). Very true. I saw that. Yes, once we know what the AI should be reading for instructions, could help spot issues. thanks, -- John Hubbard
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
On Mon, Nov 3, 2025 at 8:21 PM Joel Fernandes <joelagnelf@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@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
On 10/20/25 11:55 AM, Joel Fernandes wrote:
> Add data structures and helpers for page table management. Uses
> bitfield for cleanly representing and accessing the bitfields in the
> structures.
>
...
> +bitfield! {
> + pub(crate) struct Pte(u64), "Page Table Entry (PTE) to map virtual pages to physical frames." {
> + 0:0 valid as bool; // (1 = valid for PTEs)
> + 1:1 privilege as bool; // P - Privileged/kernel-only access
> + 2:2 read_only as bool; // RO - Write protection
> + 3:3 atomic_disable as bool; // AD - Disable atomic ops
> + 4:4 encrypted as bool; // E - Encryption enabled
> + 39:8 frame_number as u64; // PA[39:8] - Physical frame number (32 bits)
> + 41:40 aperture as u8 => AperturePte; // Memory aperture type.
> + 42:42 volatile as bool; // VOL - Volatile flag
> + 50:43 kind as u8; // K[7:0] - Compression/tiling kind
> + 63:51 comptag_line as u16; // CTL[12:0] - Compression tag line
> + }
> +}
Hi Joel,
What GPUs is this good for? I ask because I seem to recall that
the format has changed over the years, on a per-GPU-architecture
basis...
thanks,
--
John Hubbard
Hi John,
On 10/20/2025 4:59 PM, John Hubbard wrote:
> On 10/20/25 11:55 AM, Joel Fernandes wrote:
>> Add data structures and helpers for page table management. Uses
>> bitfield for cleanly representing and accessing the bitfields in the
>> structures.
>>
> ...
>> +bitfield! {
>> + pub(crate) struct Pte(u64), "Page Table Entry (PTE) to map virtual pages to physical frames." {
>> + 0:0 valid as bool; // (1 = valid for PTEs)
>> + 1:1 privilege as bool; // P - Privileged/kernel-only access
>> + 2:2 read_only as bool; // RO - Write protection
>> + 3:3 atomic_disable as bool; // AD - Disable atomic ops
>> + 4:4 encrypted as bool; // E - Encryption enabled
>> + 39:8 frame_number as u64; // PA[39:8] - Physical frame number (32 bits)
>> + 41:40 aperture as u8 => AperturePte; // Memory aperture type.
>> + 42:42 volatile as bool; // VOL - Volatile flag
>> + 50:43 kind as u8; // K[7:0] - Compression/tiling kind
>> + 63:51 comptag_line as u16; // CTL[12:0] - Compression tag line
>> + }
>> +}
>
> Hi Joel,
>
> What GPUs is this good for? I ask because I seem to recall that
> the format has changed over the years, on a per-GPU-architecture
> basis...
Yes, there's different format versions.
This patch supports all Turing and later GPUs because all GPUs including Hopper+
are backward compatible with version 1. However they wont be able to use the
version 2 and 3 features with only this patch.
I kind of intentionally did this for a first cut. But yes, I could split it into
versions. The 3 MMU structures (PTE, PDE and Dual PDE) are versioned. Version 2
is Turing and later. Hopper+ is when Version 3 got introduced and it is also
backward compatible with Version 2.
We could eventually support versions 2 and 3 (instead of just version 1 as I am
doing), however my working MMU translation prototype is based on version 1 (I
did not have to configure anything in the MMU to switch versions, this was default).
There are a couple of options:
1. For starters, support only version 1. Drawback is, when/if we want to use
version 2 and 3 features, it may require some rework.
2. Have the following hierarchy:
mm/types.rs - all common structures (stuff that is generic like Pfn).
mm/types/ver1.rs - Version 1 specific types.
mm/types/ver2.rs - Version 2 specific types.
mm/types/ver3.rs - Version 3 specific types.
The advantage of this is it keeps the structs namespaced. So it'd be
nova_core::mm:types::ver2::Pte or nova_core::mm:types::ver3::Pte. And the
nova-core MMU code can pick the correct version.
3. Keep the single file types.rs and just suffix the structs with version
numbers. This is attractive because there are only 3 types that have version
flavors (pte, pde and dual pde). So instead of Pte, we would have PteVersion1,
PteVersion2 etc, and a helper abstraction can pick the correct struct.
4. Any of the options 1-3, but dropping version 1 since Turing+ supports version
2 and later. I do have to figure out how to configure the MMU to use a specific
version (which is reasonable).
5. Your options here.
Btw, I used Nouveau as a reference as well, so likely Nouveau doesn't support
version 2 and 3 features. Not that that matters (we should support newer
features in nova-core), but just thought I'd mention it.
Other thoughts?
thanks,
- Joel
On 10/21/25 11:26 AM, Joel Fernandes wrote: > On 10/20/2025 4:59 PM, John Hubbard wrote: >> On 10/20/25 11:55 AM, Joel Fernandes wrote: >> ... > Yes, there's different format versions. > > This patch supports all Turing and later GPUs because all GPUs including Hopper+ > are backward compatible with version 1. However they wont be able to use the > version 2 and 3 features with only this patch. > > I kind of intentionally did this for a first cut. But yes, I could split it into > versions. The 3 MMU structures (PTE, PDE and Dual PDE) are versioned. Version 2 > is Turing and later. Hopper+ is when Version 3 got introduced and it is also Ah, then we shouldn't even do version 1. We should take full advantage of the fact that Nova's earliest GPU is Turing. > backward compatible with Version 2. > > We could eventually support versions 2 and 3 (instead of just version 1 as I am > doing), however my working MMU translation prototype is based on version 1 (I > did not have to configure anything in the MMU to switch versions, this was default). > > There are a couple of options: > > 1. For starters, support only version 1. Drawback is, when/if we want to use > version 2 and 3 features, it may require some rework. > > 2. Have the following hierarchy: > mm/types.rs - all common structures (stuff that is generic like Pfn). > mm/types/ver1.rs - Version 1 specific types. > mm/types/ver2.rs - Version 2 specific types. > mm/types/ver3.rs - Version 3 specific types. Maybe a file/directory structure that more directly indicates page table formats. "mm/types" could be quite a few things. > The advantage of this is it keeps the structs namespaced. So it'd be > nova_core::mm:types::ver2::Pte or nova_core::mm:types::ver3::Pte. And the > nova-core MMU code can pick the correct version. > > 3. Keep the single file types.rs and just suffix the structs with version > numbers. This is attractive because there are only 3 types that have version > flavors (pte, pde and dual pde). So instead of Pte, we would have PteVersion1, > PteVersion2 etc, and a helper abstraction can pick the correct struct. > > 4. Any of the options 1-3, but dropping version 1 since Turing+ supports version > 2 and later. I do have to figure out how to configure the MMU to use a specific Right, I see you already noticed that we can start with Turing. Good. > version (which is reasonable). > > 5. Your options here. > > Btw, I used Nouveau as a reference as well, so likely Nouveau doesn't support > version 2 and 3 features. Not that that matters (we should support newer > features in nova-core), but just thought I'd mention it. > > Other thoughts? Two things: 1) Danilo is working on writing down locking requirements for Nova page tables, based on earlier experience with Nouveau page table locking problems, which were apparently very serious. 2) Maybe it would be good to start with versions 2 and 3, so that we can see how to do >1 version? thanks, -- John Hubbard
> On Oct 21, 2025, at 4:30 PM, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 10/21/25 11:26 AM, Joel Fernandes wrote:
>>> On 10/20/2025 4:59 PM, John Hubbard wrote:
>>>> On 10/20/25 11:55 AM, Joel Fernandes wrote:
>>> ...
>> Yes, there's different format versions.
>>
>> This patch supports all Turing and later GPUs because all GPUs including Hopper+
>> are backward compatible with version 1. However they wont be able to use the
>> version 2 and 3 features with only this patch.
>>
>> I kind of intentionally did this for a first cut. But yes, I could split it into
>> versions. The 3 MMU structures (PTE, PDE and Dual PDE) are versioned. Version 2
>> is Turing and later. Hopper+ is when Version 3 got introduced and it is also
>
> Ah, then we shouldn't even do version 1. We should take full advantage of
> the fact that Nova's earliest GPU is Turing.
Makes sense, though one advantage of just defining version 1 too is it may
be a good reference or a fallback should we ever need it (Especially since it
took me 2 months to be get my nova-core MMU code working and it may be
nice to have a fallback. Let me think about it, I believe it will be just few more
lines of code, so should not be that bad.)
>
>> backward compatible with Version 2.
>>
>> We could eventually support versions 2 and 3 (instead of just version 1 as I am
>> doing), however my working MMU translation prototype is based on version 1 (I
>> did not have to configure anything in the MMU to switch versions, this was default).
>>
>> There are a couple of options:
>>
>> 1. For starters, support only version 1. Drawback is, when/if we want to use
>> version 2 and 3 features, it may require some rework.
>>
>> 2. Have the following hierarchy:
>> mm/types.rs - all common structures (stuff that is generic like Pfn).
>> mm/types/ver1.rs - Version 1 specific types.
>> mm/types/ver2.rs - Version 2 specific types.
>> mm/types/ver3.rs - Version 3 specific types.
>
> Maybe a file/directory structure that more directly indicates page table
> formats. "mm/types" could be quite a few things.
>
Agreed, though mm/types also contains other non-pagetable structures.
I could prefix ver1/2/3 with page_tables_, so it would be like:
mm/types/page_tables_ver{1,2,3}.rs. That is much more clear.
>
>> version (which is reasonable).
>>
>> 5. Your options here.
>>
>> Btw, I used Nouveau as a reference as well, so likely Nouveau doesn't support
>> version 2 and 3 features. Not that that matters (we should support newer
>> features in nova-core), but just thought I'd mention it.
>>
>> Other thoughts?
>
> Two things:
>
> 1) Danilo is working on writing down locking requirements for Nova page
> tables, based on earlier experience with Nouveau page table locking
> problems, which were apparently very serious.
Sure, though I believe it should not block these patches because we are
not yet designing higher level users. These patches just expose the structures
and things like locking should be built at a higher level. I believe Danilo
is referring to page table look ups and updates, that will a different module
outside of types.
> 2) Maybe it would be good to start with versions 2 and 3, so that we
> can see how to do >1 version?
Yes I will start with version 2, 3. If version 1 is simple, I may include that
in too for the benefit of my working prototype if that is Ok :) Worst case,
we have to just delete an extra file should be not need it :)
thanks,
- Joel
>
> thanks,
> --
> John Hubbard
>
© 2016 - 2026 Red Hat, Inc.