[PATCH v2 0/3] riscv: ptdump: Refactor for KVM gstage ptdump support

[PATCH v2 0/3] riscv: ptdump: Refactor for KVM gstage ptdump support

[Dylan.Wu]

This is v2 of the series to add KVM gstage ptdump support for RISC-V.

Changes from v1 (https://lore.kernel.org/linux-riscv/20260701085030.124579-1-fredwudi0305@gmail.com/):
  - Split patch 1 into two patches as requested by Paul Walmsley:
    * Patch 1/3: Create ptdump.h header, move declarations there,
                 and export note_page()
    * Patch 2/3: Make attribute parsing use per-level bits configuration,
                 and rename local pg_level[] to kernel_pg_levels[]
  - Added CONFIG_PTDUMP_GSTAGE_DEBUGFS Kconfig option
  - Replaced custom walk logic with ptdump_walk_pgd() reuse
  - Removed incorrect copyright headers from arch/riscv/kvm/ptdump.c
  - Fixed Makefile ordering (alphabetical, under conditional Kconfig)

Dylan.Wu (3):
  riscv: ptdump: Create ptdump.h and move declarations
  riscv: ptdump: Use per-level attribute bits for parsing
  KVM: riscv: Register ptdump with debugfs on guest creation

 arch/riscv/include/asm/ptdump.h |  42 +++++
 arch/riscv/kvm/Kconfig          |  15 ++
 arch/riscv/kvm/Makefile         |   1 +
 arch/riscv/kvm/ptdump.c         | 276 ++++++++++++++++++++++++++++++++
 arch/riscv/mm/ptdump.c          | 106 +++++-------
 5 files changed, 376 insertions(+), 64 deletions(-)
 create mode 100644 arch/riscv/include/asm/ptdump.h
 create mode 100644 arch/riscv/kvm/ptdump.c

-- 
2.34.1

[PATCH v2 1/3] riscv: ptdump: Create ptdump.h and move declarations

[Dylan.Wu]

Create a new arch/riscv/include/asm/ptdump.h header file and move the
pagetable walking state structures and level definitions there. This
allows other parts of the kernel (like KVM) to reuse the ptdump data
structures.

Also export the note_page() symbol so it can be used by other kernel
components.

Assisted-by: YuanSheng: deepseek-v4-pro
Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Dylan.Wu <fredwudi0305@gmail.com>
---
 arch/riscv/include/asm/ptdump.h | 41 ++++++++++++++++++++++
 arch/riscv/mm/ptdump.c          | 60 +++++++--------------------------
 2 files changed, 53 insertions(+), 48 deletions(-)
 create mode 100644 arch/riscv/include/asm/ptdump.h

diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
new file mode 100644
index 000000000..eb9d11dab
--- /dev/null
+++ b/arch/riscv/include/asm/ptdump.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_RISCV_PTDUMP_H
+#define _ASM_RISCV_PTDUMP_H
+
+#include <linux/ptdump.h>
+#include <linux/seq_file.h>
+
+struct addr_marker {
+	unsigned long start_address;
+	const char *name;
+};
+
+struct ptdump_prot_bits {
+	u64 mask;
+	const char *set;
+	const char *clear;
+};
+
+struct ptdump_pg_level {
+	const struct ptdump_prot_bits *bits;
+	const char *name;
+	u64 mask;
+	int num;
+};
+
+struct ptdump_pg_state {
+	struct ptdump_state ptdump;
+	struct seq_file *seq;
+	const struct addr_marker *marker;
+	unsigned long start_address;
+	unsigned long start_pa;
+	unsigned long last_pa;
+	int level;
+	u64 current_prot;
+	bool check_wx;
+	unsigned long wx_pages;
+};
+
+void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, u64 val);
+
+#endif /* _ASM_RISCV_PTDUMP_H */
diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index f4b4a9fcb..63655a9c8 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -11,6 +11,7 @@
 #include <linux/ptdump.h>
 
 #include <linux/pgtable.h>
+#include <asm/ptdump.h>
 #include <asm/kasan.h>
 
 #define pt_dump_seq_printf(m, fmt, args...)	\
@@ -25,31 +26,6 @@
 		seq_puts(m, fmt);	\
 })
 
-/*
- * The page dumper groups page table entries of the same type into a single
- * description. It uses pg_state to track the range information while
- * iterating over the pte entries. When the continuity is broken it then
- * dumps out a description of the range.
- */
-struct pg_state {
-	struct ptdump_state ptdump;
-	struct seq_file *seq;
-	const struct addr_marker *marker;
-	unsigned long start_address;
-	unsigned long start_pa;
-	unsigned long last_pa;
-	int level;
-	u64 current_prot;
-	bool check_wx;
-	unsigned long wx_pages;
-};
-
-/* Address marker */
-struct addr_marker {
-	unsigned long start_address;
-	const char *name;
-};
-
 /* Private information for debugfs */
 struct ptd_mm_info {
 	struct mm_struct		*mm;
@@ -126,14 +102,7 @@ static struct ptd_mm_info efi_ptd_info = {
 };
 #endif
 
-/* Page Table Entry */
-struct prot_bits {
-	u64 mask;
-	const char *set;
-	const char *clear;
-};
-
-static const struct prot_bits pte_bits[] = {
+static const struct ptdump_prot_bits pte_bits[] = {
 	{
 #ifdef CONFIG_64BIT
 		.mask = _PAGE_NAPOT,
@@ -183,13 +152,7 @@ static const struct prot_bits pte_bits[] = {
 	}
 };
 
-/* Page Level */
-struct pg_level {
-	const char *name;
-	u64 mask;
-};
-
-static struct pg_level pg_level[] = {
+static struct ptdump_pg_level pg_level[] = {
 	{ /* pgd */
 		.name = "PGD",
 	}, { /* p4d */
@@ -203,7 +166,7 @@ static struct pg_level pg_level[] = {
 	},
 };
 
-static void dump_prot(struct pg_state *st)
+static void dump_prot(struct ptdump_pg_state *st)
 {
 	unsigned int i;
 
@@ -240,7 +203,7 @@ static void dump_prot(struct pg_state *st)
 #else
 #define ADDR_FORMAT	"0x%08lx"
 #endif
-static void dump_addr(struct pg_state *st, unsigned long addr)
+static void dump_addr(struct ptdump_pg_state *st, unsigned long addr)
 {
 	static const char units[] = "KMGTPE";
 	const char *unit = units;
@@ -261,7 +224,7 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
 			   pg_level[st->level].name);
 }
 
-static void note_prot_wx(struct pg_state *st, unsigned long addr)
+static void note_prot_wx(struct ptdump_pg_state *st, unsigned long addr)
 {
 	if (!st->check_wx)
 		return;
@@ -276,10 +239,10 @@ static void note_prot_wx(struct pg_state *st, unsigned long addr)
 	st->wx_pages += (addr - st->start_address) / PAGE_SIZE;
 }
 
-static void note_page(struct ptdump_state *pt_st, unsigned long addr,
-		      int level, u64 val)
+void note_page(struct ptdump_state *pt_st, unsigned long addr,
+	       int level, u64 val)
 {
-	struct pg_state *st = container_of(pt_st, struct pg_state, ptdump);
+	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
 	u64 pa = PFN_PHYS(pte_pfn(__pte(val)));
 	u64 prot = 0;
 
@@ -317,6 +280,7 @@ static void note_page(struct ptdump_state *pt_st, unsigned long addr,
 		st->last_pa = pa;
 	}
 }
+EXPORT_SYMBOL_GPL(note_page);
 
 static void note_page_pte(struct ptdump_state *pt_st, unsigned long addr, pte_t pte)
 {
@@ -352,7 +316,7 @@ static void note_page_flush(struct ptdump_state *pt_st)
 
 static void ptdump_walk(struct seq_file *s, struct ptd_mm_info *pinfo)
 {
-	struct pg_state st = {
+	struct ptdump_pg_state st = {
 		.seq = s,
 		.marker = pinfo->markers,
 		.level = -1,
@@ -375,7 +339,7 @@ static void ptdump_walk(struct seq_file *s, struct ptd_mm_info *pinfo)
 
 bool ptdump_check_wx(void)
 {
-	struct pg_state st = {
+	struct ptdump_pg_state st = {
 		.seq = NULL,
 		.marker = (struct addr_marker[]) {
 			{0, NULL},
-- 
2.34.1

Re: [PATCH v2 1/3] riscv: ptdump: Create ptdump.h and move declarations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The refactoring to make ptdump reusable by other components (like KVM) is incomplete. The newly added fields `bits` and `num` in `struct ptdump_pg_level` are dead code, and `note_page()` hardcodes access to file-local static arrays.
- [Low] The function `note_page` is exported globally with a highly generic name, polluting the global namespace.
--

--- Patch [1]: [PATCH v2 1/3] riscv: ptdump: Create ptdump.h and move declarations ---
commit 7ec3e13f790f5980858ebc9e23a4405735f7e6cf
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: ptdump: Create ptdump.h and move declarations

Create a new arch/riscv/include/asm/ptdump.h header file and move the
pagetable walking state structures and level definitions there. This
allows other parts of the kernel (like KVM) to reuse the ptdump data
structures.

Also export the note_page() symbol so it can be used by other kernel
components.

> diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
> new file mode 100644
> index 0000000000000..eb9d11dab6098
> --- /dev/null
> +++ b/arch/riscv/include/asm/ptdump.h
> @@ -0,0 +1,41 @@
[ ... ]
> +struct ptdump_pg_level {
> +	const struct ptdump_prot_bits *bits;
> +	const char *name;
> +	u64 mask;
> +	int num;
> +};

[Severity: Medium]
Are the newly added fields bits and num essentially dead code?

The commit message indicates this refactoring allows other parts of the kernel
(like KVM) to reuse the ptdump data structures. However, these new fields
appear to be unused.

For instance, dump_prot() in arch/riscv/mm/ptdump.c still unconditionally
iterates over the file-local pte_bits array rather than utilizing the
caller-provided bits. Additionally, note_page() continues to hardcode access
to the file-local pg_level array.

Does this incomplete refactoring prevent external components from dumping
page tables with custom layouts?

[ ... ]
> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index f4b4a9fcbbd88..63655a9c8df3e 100644
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
[ ... ]
> -static void note_page(struct ptdump_state *pt_st, unsigned long addr,
> -		      int level, u64 val)
> +void note_page(struct ptdump_state *pt_st, unsigned long addr,
> +	       int level, u64 val)
>  {
[ ... ]
> @@ -317,6 +280,7 @@ static void note_page(struct ptdump_state *pt_st, unsigned long addr,
>  		st->last_pa = pa;
>  	}
>  }
> +EXPORT_SYMBOL_GPL(note_page);

[Severity: Low]
Does exporting this generically named function pollute the global kernel symbol
table?

Using a generic name like note_page() (instead of something specific like
riscv_ptdump_note_page) could increase the risk of linker collisions if another
subsystem uses the same name.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727123013.118984-1-fredwudi0305@gmail.com?part=1

[PATCH v2 2/3] riscv: ptdump: Use per-level attribute bits for parsing

[Dylan.Wu]

Instead of using the global pte_bits array in dump_prot(), use the
per-level bits and num from the associated pg_level entry passed via
ptdump_pg_state. This makes the attribute parsing logic fully driven by
the per-level configuration, which allows KVM (and other future users)
to plug in their own set of protection bits for their pagetable levels.

Rename the local pg_level[] array to kernel_pg_levels[] to avoid
shadowing the pg_level pointer in struct ptdump_pg_state.

Assisted-by: YuanSheng: deepseek-v4-pro
Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Dylan.Wu <fredwudi0305@gmail.com>
---
 arch/riscv/include/asm/ptdump.h |  1 +
 arch/riscv/mm/ptdump.c          | 48 +++++++++++++++++++++------------
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
index eb9d11dab..14a262bd0 100644
--- a/arch/riscv/include/asm/ptdump.h
+++ b/arch/riscv/include/asm/ptdump.h
@@ -28,6 +28,7 @@ struct ptdump_pg_state {
 	struct seq_file *seq;
 	const struct addr_marker *marker;
 	unsigned long start_address;
+	const struct ptdump_pg_level *pg_level;
 	unsigned long start_pa;
 	unsigned long last_pa;
 	int level;
diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index 63655a9c8..06c0217f0 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -152,46 +152,58 @@ static const struct ptdump_prot_bits pte_bits[] = {
 	}
 };
 
-static struct ptdump_pg_level pg_level[] = {
+static struct ptdump_pg_level kernel_pg_levels[] = {
 	{ /* pgd */
+		.bits = pte_bits,
+		.num = ARRAY_SIZE(pte_bits),
 		.name = "PGD",
 	}, { /* p4d */
+		.bits = pte_bits,
+		.num = ARRAY_SIZE(pte_bits),
 		.name = (CONFIG_PGTABLE_LEVELS > 4) ? "P4D" : "PGD",
 	}, { /* pud */
+		.bits = pte_bits,
+		.num = ARRAY_SIZE(pte_bits),
 		.name = (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",
 	}, { /* pmd */
+		.bits = pte_bits,
+		.num = ARRAY_SIZE(pte_bits),
 		.name = (CONFIG_PGTABLE_LEVELS > 2) ? "PMD" : "PGD",
 	}, { /* pte */
+		.bits = pte_bits,
+		.num = ARRAY_SIZE(pte_bits),
 		.name = "PTE",
 	},
 };
 
 static void dump_prot(struct ptdump_pg_state *st)
 {
+	const struct ptdump_pg_level *lvl = &st->pg_level[st->level];
+	const struct ptdump_prot_bits *bits = lvl->bits;
 	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(pte_bits); i++) {
+	for (i = 0; i < lvl->num; i++) {
 		char s[7];
 		unsigned long val;
 
-		val = st->current_prot & pte_bits[i].mask;
+		val = st->current_prot & bits[i].mask;
 		if (val) {
-			if (pte_bits[i].mask == _PAGE_SOFT)
-				snprintf(s, sizeof(s), pte_bits[i].set, val >> 8);
+			if (bits[i].mask == _PAGE_SOFT)
+				snprintf(s, sizeof(s), bits[i].set, val >> 8);
 #ifdef CONFIG_64BIT
-			else if (pte_bits[i].mask == _PAGE_MTMASK_SVPBMT) {
+			else if (bits[i].mask == _PAGE_MTMASK_SVPBMT) {
 				if (val == _PAGE_NOCACHE_SVPBMT)
-					snprintf(s, sizeof(s), pte_bits[i].set, "NC");
+					snprintf(s, sizeof(s), bits[i].set, "NC");
 				else if (val == _PAGE_IO_SVPBMT)
-					snprintf(s, sizeof(s), pte_bits[i].set, "IO");
+					snprintf(s, sizeof(s), bits[i].set, "IO");
 				else
-					snprintf(s, sizeof(s), pte_bits[i].set, "??");
+					snprintf(s, sizeof(s), bits[i].set, "??");
 			}
 #endif
 			else
-				strscpy(s, pte_bits[i].set);
+				strscpy(s, bits[i].set);
 		} else {
-			strscpy(s, pte_bits[i].clear);
+			strscpy(s, bits[i].clear);
 		}
 
 		pt_dump_seq_printf(st->seq, " %s", s);
@@ -221,7 +233,7 @@ static void dump_addr(struct ptdump_pg_state *st, unsigned long addr)
 	}
 
 	pt_dump_seq_printf(st->seq, "%9lu%c %s", delta, *unit,
-			   pg_level[st->level].name);
+			   kernel_pg_levels[st->level].name);
 }
 
 static void note_prot_wx(struct ptdump_pg_state *st, unsigned long addr)
@@ -247,7 +259,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr,
 	u64 prot = 0;
 
 	if (level >= 0)
-		prot = val & pg_level[level].mask;
+		prot = val & kernel_pg_levels[level].mask;
 
 	if (st->level == -1) {
 		st->level = level;
@@ -320,6 +332,7 @@ static void ptdump_walk(struct seq_file *s, struct ptd_mm_info *pinfo)
 		.seq = s,
 		.marker = pinfo->markers,
 		.level = -1,
+		.pg_level = kernel_pg_levels,
 		.ptdump = {
 			.note_page_pte = note_page_pte,
 			.note_page_pmd = note_page_pmd,
@@ -346,6 +359,7 @@ bool ptdump_check_wx(void)
 			{-1, NULL},
 		},
 		.level = -1,
+		.pg_level = kernel_pg_levels,
 		.check_wx = true,
 		.ptdump = {
 			.note_page_pte = note_page_pte,
@@ -410,12 +424,12 @@ static int __init ptdump_init(void)
 
 	kernel_ptd_info.base_addr = KERN_VIRT_START;
 
-	pg_level[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
-	pg_level[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
+	kernel_pg_levels[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
+	kernel_pg_levels[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
 
-	for (i = 0; i < ARRAY_SIZE(pg_level); i++)
+	for (i = 0; i < ARRAY_SIZE(kernel_pg_levels); i++)
 		for (j = 0; j < ARRAY_SIZE(pte_bits); j++)
-			pg_level[i].mask |= pte_bits[j].mask;
+			kernel_pg_levels[i].mask |= pte_bits[j].mask;
 
 	debugfs_create_file("kernel_page_tables", 0400, NULL, &kernel_ptd_info,
 			    &ptdump_fops);
-- 
2.34.1

Re: [PATCH v2 2/3] riscv: ptdump: Use per-level attribute bits for parsing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The refactoring is incomplete because `note_page()` and `dump_addr()` still hardcode `kernel_pg_levels` instead of using the caller-provided `st->pg_level` array.
- [Medium] The `dump_prot()` function retains hardcoded, architecture-specific logic for specific bitmasks, which is unsafe when processing generic, caller-provided bit arrays.
--

--- Patch [2]: [PATCH v2 2/3] riscv: ptdump: Use per-level attribute bits for parsing ---
commit 1ffa1d459c45da98aa9b8ff61631b9ce3dc228c8
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: ptdump: Use per-level attribute bits for parsing

This commit updates the RISC-V page table dumper to use per-level attribute
bits and names passed via the ptdump_pg_state structure. This refactoring is
intended to allow future users, such as KVM, to provide their own protection
bits for parsing.

> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
> @@ -176,21 +188,21 @@ static void dump_prot(struct ptdump_pg_state *st)
>  
>  		val = st->current_prot & bits[i].mask;
>  		if (val) {
> -			if (pte_bits[i].mask == _PAGE_SOFT)
> -				snprintf(s, sizeof(s), pte_bits[i].set, val >> 8);
> +			if (bits[i].mask == _PAGE_SOFT)
> +				snprintf(s, sizeof(s), bits[i].set, val >> 8);

[Severity: Medium]
Since the goal of this patch is to support custom caller-provided bit arrays,
is it safe to retain these hardcoded checks for _PAGE_SOFT (and
_PAGE_MTMASK_SVPBMT below)?

If a future caller (like KVM) provides a custom bitmask that happens to match
the value of _PAGE_SOFT, but provides a format string in bits[i].set that
doesn't expect an integer, could this snprintf() cause a crash or output
garbage?

[ ... ]

> @@ -221,7 +233,7 @@ static void dump_addr(struct ptdump_pg_state *st, unsigned long addr)
>  	}
>  
>  	pt_dump_seq_printf(st->seq, "%9lu%c %s", delta, *unit,
> -			   pg_level[st->level].name);
> +			   kernel_pg_levels[st->level].name);
>  }

[Severity: Medium]
Will this ignore the caller's custom level names?

It looks like dump_addr() was updated to hardcode kernel_pg_levels instead
of using the caller-provided st->pg_level[st->level].name.

[ ... ]

> @@ -247,7 +259,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr,
>  	u64 prot = 0;
>  
>  	if (level >= 0)
> -		prot = val & pg_level[level].mask;
> +		prot = val & kernel_pg_levels[level].mask;

[Severity: Medium]
Similar to the issue in dump_addr(), will this ignore the custom bitmasks
provided by the caller?

By hardcoding kernel_pg_levels[level].mask instead of using
st->pg_level[level].mask, it seems this would cause incorrect page table
attribute masking for custom callers.

Additionally, could this cause an out-of-bounds array read if a custom caller
uses more levels than the 5 defined in kernel_pg_levels?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727123013.118984-1-fredwudi0305@gmail.com?part=2

[PATCH v2 3/3] KVM: riscv: Register ptdump with debugfs on guest creation

[Dylan.Wu]

Add a new RISC-V KVM specific ptdump implementation that dumps the
guest stage-2 (gstage) pagetables via debugfs. The code reuses the
common ptdump data structures and helpers from asm/ptdump.h, and
exposes a "gstage_page_tables" file in each guest VM's debugfs
directory (kvm->debugfs_dentry).

The gstage uses its own set of pg_level and prot_bits arrays (a
simplified set of V/R/W/X/D/A/G/U bits), and plugs these into the
generic ptdump_walk_pgd() walker just like the host kernel ptdump.

The kvm_arch_create_vm_debugfs() hook (overriding the weak default)
is used to register the debugfs file automatically on each VM
creation.

Assisted-by: YuanSheng: deepseek-v4-pro
Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Dylan.Wu <fredwudi0305@gmail.com>
---
 arch/riscv/kvm/Kconfig  |  15 +++
 arch/riscv/kvm/Makefile |   1 +
 arch/riscv/kvm/ptdump.c | 276 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+)
 create mode 100644 arch/riscv/kvm/ptdump.c

diff --git a/arch/riscv/kvm/Kconfig b/arch/riscv/kvm/Kconfig
index ec2cee0a3..9413110a4 100644
--- a/arch/riscv/kvm/Kconfig
+++ b/arch/riscv/kvm/Kconfig
@@ -37,4 +37,19 @@ config KVM
 
 	  If unsure, say N.
 
+config PTDUMP_GSTAGE_DEBUGFS
+	bool "Present the gstage pagetables to debugfs"
+	depends on KVM
+	depends on DEBUG_KERNEL
+	depends on DEBUG_FS
+	depends on PTDUMP_DEBUGFS
+	default n
+	help
+	  Say Y here if you want to show the RISC-V KVM gstage guest page tables
+	  layout in a debugfs file. This information is primarily useful for
+	  architecture-specific kernel developers and KVM maintainers to
+	  investigate memory mapping and permission issues. It is probably
+	  not a good idea to enable this feature in a production kernel.
+	  If in doubt, say N.
+
 endif # VIRTUALIZATION
diff --git a/arch/riscv/kvm/Makefile b/arch/riscv/kvm/Makefile
index 296c2ba05..905593d6f 100644
--- a/arch/riscv/kvm/Makefile
+++ b/arch/riscv/kvm/Makefile
@@ -19,6 +19,7 @@ kvm-y += isa.o
 kvm-y += main.o
 kvm-y += mmu.o
 kvm-y += nacl.o
+kvm-$(CONFIG_PTDUMP_GSTAGE_DEBUGFS) += ptdump.o
 kvm-y += tlb.o
 kvm-y += vcpu.o
 kvm-y += vcpu_config.o
diff --git a/arch/riscv/kvm/ptdump.c b/arch/riscv/kvm/ptdump.c
new file mode 100644
index 000000000..83a92969b
--- /dev/null
+++ b/arch/riscv/kvm/ptdump.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 SiFive
+ * Debug helper to dump the KVM gstage page tables.
+ */
+
+#include <linux/init.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/string.h>
+#include <linux/ptdump.h>
+#include <linux/kvm_host.h>
+
+#include <linux/pgtable.h>
+#include <asm/ptdump.h>
+#include <asm/kvm_gstage.h>
+#include <asm/kvm_mmu.h>
+
+#define pt_dump_seq_printf(m, fmt, args...)	\
+({						\
+	if (m)					\
+		seq_printf(m, fmt, ##args);	\
+})
+
+#define pt_dump_seq_puts(m, fmt)	\
+({					\
+	if (m)				\
+		seq_puts(m, fmt);	\
+})
+
+/* Guest physical address markers (simplified for gstage) */
+enum gstage_address_markers_idx {
+	GSTAGE_GPA_START_NR,
+	GSTAGE_GPA_END_NR,
+	END_OF_GSTAGE_SPACE_NR
+};
+
+/* G-stage PTE bits */
+static const struct ptdump_prot_bits gstage_pte_bits[] = {
+	{
+		.mask = _PAGE_DIRTY,
+		.set = "D",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_ACCESSED,
+		.set = "A",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_GLOBAL,
+		.set = "G",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_USER,
+		.set = "U",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_EXEC,
+		.set = "X",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_WRITE,
+		.set = "W",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_READ,
+		.set = "R",
+		.clear = ".",
+	}, {
+		.mask = _PAGE_PRESENT,
+		.set = "V",
+		.clear = ".",
+	}
+};
+
+static struct ptdump_pg_level gstage_pg_levels[] = {
+	{ /* pgd */
+		.bits = gstage_pte_bits,
+		.num = ARRAY_SIZE(gstage_pte_bits),
+		.name = "PGD",
+	}, { /* p4d */
+		.bits = gstage_pte_bits,
+		.num = ARRAY_SIZE(gstage_pte_bits),
+		.name = (CONFIG_PGTABLE_LEVELS > 4) ? "P4D" : "PGD",
+	}, { /* pud */
+		.bits = gstage_pte_bits,
+		.num = ARRAY_SIZE(gstage_pte_bits),
+		.name = (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",
+	}, { /* pmd */
+		.bits = gstage_pte_bits,
+		.num = ARRAY_SIZE(gstage_pte_bits),
+		.name = (CONFIG_PGTABLE_LEVELS > 2) ? "PMD" : "PGD",
+	}, { /* pte */
+		.bits = gstage_pte_bits,
+		.num = ARRAY_SIZE(gstage_pte_bits),
+		.name = "PTE",
+	},
+};
+
+static void gstage_dump_prot(struct ptdump_pg_state *st)
+{
+	const struct ptdump_pg_level *lvl = &st->pg_level[st->level];
+	const struct ptdump_prot_bits *bits = lvl->bits;
+	unsigned int i;
+
+	for (i = 0; i < lvl->num; i++) {
+		char s[7];
+		unsigned long val;
+
+		val = st->current_prot & bits[i].mask;
+		if (val)
+			strscpy(s, bits[i].set);
+		else
+			strscpy(s, bits[i].clear);
+
+		pt_dump_seq_printf(st->seq, " %s", s);
+	}
+}
+
+#ifdef CONFIG_64BIT
+#define GSTAGE_ADDR_FORMAT	"0x%016lx"
+#else
+#define GSTAGE_ADDR_FORMAT	"0x%08lx"
+#endif
+static void gstage_dump_addr(struct ptdump_pg_state *st, unsigned long addr)
+{
+	static const char units[] = "KMGTPE";
+	const char *unit = units;
+	unsigned long delta;
+
+	pt_dump_seq_printf(st->seq, GSTAGE_ADDR_FORMAT "-" GSTAGE_ADDR_FORMAT "   ",
+			   st->start_address, addr);
+
+	pt_dump_seq_printf(st->seq, " " GSTAGE_ADDR_FORMAT " ", st->start_pa);
+	delta = (addr - st->start_address) >> 10;
+
+	while (!(delta & 1023) && unit[1]) {
+		delta >>= 10;
+		unit++;
+	}
+
+	pt_dump_seq_printf(st->seq, "%9lu%c %s", delta, *unit,
+			   st->pg_level[st->level].name);
+}
+
+static void gstage_note_page_gpa_prot(struct ptdump_pg_state *st, unsigned long addr)
+{
+	if (st->current_prot) {
+		gstage_dump_addr(st, addr);
+		gstage_dump_prot(st);
+		pt_dump_seq_puts(st->seq, "\n");
+	}
+}
+
+static void gstage_note_page(struct ptdump_state *pt_st, unsigned long addr,
+			     int level, u64 val)
+{
+	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
+	u64 pa = PFN_PHYS(pte_pfn(__pte(val)));
+	u64 prot = 0;
+
+	if (level >= 0)
+		prot = val & st->pg_level[level].mask;
+
+	if (st->level == -1) {
+		st->level = level;
+		st->current_prot = prot;
+		st->start_address = addr;
+		st->start_pa = pa;
+		st->last_pa = pa;
+		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+	} else if (prot != st->current_prot ||
+		   level != st->level || addr >= st->marker[1].start_address) {
+		gstage_note_page_gpa_prot(st, addr);
+
+		while (addr >= st->marker[1].start_address) {
+			st->marker++;
+			pt_dump_seq_printf(st->seq, "---[ %s ]---\n",
+					   st->marker->name);
+		}
+
+		st->start_address = addr;
+		st->start_pa = pa;
+		st->last_pa = pa;
+		st->current_prot = prot;
+		st->level = level;
+	} else {
+		st->last_pa = pa;
+	}
+}
+
+static void gstage_note_page_pte(struct ptdump_state *pt_st, unsigned long addr, pte_t pte)
+{
+	gstage_note_page(pt_st, addr, 4, pte_val(pte));
+}
+
+static void gstage_note_page_pmd(struct ptdump_state *pt_st, unsigned long addr, pmd_t pmd)
+{
+	gstage_note_page(pt_st, addr, 3, pmd_val(pmd));
+}
+
+static void gstage_note_page_pud(struct ptdump_state *pt_st, unsigned long addr, pud_t pud)
+{
+	gstage_note_page(pt_st, addr, 2, pud_val(pud));
+}
+
+static void gstage_note_page_p4d(struct ptdump_state *pt_st, unsigned long addr, p4d_t p4d)
+{
+	gstage_note_page(pt_st, addr, 1, p4d_val(p4d));
+}
+
+static void gstage_note_page_pgd(struct ptdump_state *pt_st, unsigned long addr, pgd_t pgd)
+{
+	gstage_note_page(pt_st, addr, 0, pgd_val(pgd));
+}
+
+static void gstage_note_page_flush(struct ptdump_state *pt_st)
+{
+	pte_t pte_zero = {0};
+
+	gstage_note_page(pt_st, 0, -1, pte_val(pte_zero));
+}
+
+static int gstage_ptdump_show(struct seq_file *m, void *v)
+{
+	struct kvm *kvm = m->private;
+	gpa_t gpa_size = kvm_riscv_gstage_gpa_size(kvm->arch.pgd_levels);
+	struct addr_marker gpa_markers[] = {
+		[GSTAGE_GPA_START_NR] = { 0, "Guest Physical Address Start" },
+		[GSTAGE_GPA_END_NR]   = { gpa_size - 1, "Guest Physical Address End" },
+		[END_OF_GSTAGE_SPACE_NR] = { -1, NULL },
+	};
+	struct ptdump_pg_state st = {
+		.seq = m,
+		.marker = gpa_markers,
+		.level = -1,
+		.pg_level = gstage_pg_levels,
+		.ptdump = {
+			.note_page_pte = gstage_note_page_pte,
+			.note_page_pmd = gstage_note_page_pmd,
+			.note_page_pud = gstage_note_page_pud,
+			.note_page_p4d = gstage_note_page_p4d,
+			.note_page_pgd = gstage_note_page_pgd,
+			.note_page_flush = gstage_note_page_flush,
+			.range = (struct ptdump_range[]) {
+				{0, gpa_size - 1},
+				{0, 0}
+			}
+		}
+	};
+	unsigned int i, j;
+
+	for (i = 0; i < ARRAY_SIZE(gstage_pg_levels); i++) {
+		gstage_pg_levels[i].mask = 0;
+		for (j = 0; j < ARRAY_SIZE(gstage_pte_bits); j++)
+			gstage_pg_levels[i].mask |= gstage_pte_bits[j].mask;
+	}
+
+	if (kvm->arch.pgd_levels < 5)
+		gstage_pg_levels[1].name = "PGD";
+	if (kvm->arch.pgd_levels < 4)
+		gstage_pg_levels[2].name = "PGD";
+	if (kvm->arch.pgd_levels < 3)
+		gstage_pg_levels[3].name = "PGD";
+
+	ptdump_walk_pgd(&st.ptdump, kvm->mm, kvm->arch.pgd);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(gstage_ptdump);
+
+void kvm_arch_create_vm_debugfs(struct kvm *kvm)
+{
+	debugfs_create_file("gstage_page_tables", 0400,
+			    kvm->debugfs_dentry, kvm, &gstage_ptdump_fops);
+}
-- 
2.34.1

Re: [PATCH v2 3/3] KVM: riscv: Register ptdump with debugfs on guest creation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] gstage_ptdump_show traverses KVM guest stage-2 page tables without holding kvm->mmu_lock, creating a severe use-after-free race condition.
- [High] Using the host's generic page table walker for KVM guest stage-2 page tables causes incorrect parsing and potential kernel panics due to mismatched root page table geometries.
- [Medium] Concurrent accesses to the debugfs file cause a data race and persistent state corruption due to mutation of the static `gstage_pg_levels` array.
- [Medium] If `kvm->arch.pgd` is NULL, the walker silently falls back to walking the host process's page tables, leaking the host virtual memory layout.
- [Medium] gstage_note_page_flush triggers an integer underflow, printing an astronomically huge size and an incorrect range boundary.
--

--- Patch [3]: [PATCH v2 3/3] KVM: riscv: Register ptdump with debugfs on guest creation ---
commit d69a647519dc99ece115797b207b3ba4248c4915
Author: Dylan.Wu <fredwudi0305@gmail.com>

KVM: riscv: Register ptdump with debugfs on guest creation
    
Add a new RISC-V KVM specific ptdump implementation that dumps the
guest stage-2 (gstage) pagetables via debugfs. The code reuses the
common ptdump data structures and helpers from asm/ptdump.h, and
exposes a "gstage_page_tables" file in each guest VM's debugfs
directory (kvm->debugfs_dentry).

[ ... ]

> diff --git a/arch/riscv/kvm/ptdump.c b/arch/riscv/kvm/ptdump.c
> new file mode 100644
> index 0000000000000..83a92969bd419
> --- /dev/null
> +++ b/arch/riscv/kvm/ptdump.c

[ ... ]

> +static void gstage_note_page_flush(struct ptdump_state *pt_st)
> +{
> +	pte_t pte_zero = {0};
> +
> +	gstage_note_page(pt_st, 0, -1, pte_val(pte_zero));
> +}

[Severity: Medium]
Will this trigger an integer underflow when calculating the block size?

The generic host code uses 0 to denote ULONG_MAX + 1 for address spaces
ending at the top of the 64-bit space, where 0 - start_address gracefully
yields the correct distance.

However, the KVM guest physical address space ends at gpa_size (e.g., 2TB).
Passing 0 here means addr >= st->marker[1].start_address evaluates to
false, skipping the 'Guest Physical Address End' marker. It then calculates
the size as (0 - start_address) >> 10, which underflows into a massive
unsigned integer.

Should this pass the actual end of the guest physical address space instead
of 0?

> +
> +static int gstage_ptdump_show(struct seq_file *m, void *v)
> +{
> +	struct kvm *kvm = m->private;
> +	gpa_t gpa_size = kvm_riscv_gstage_gpa_size(kvm->arch.pgd_levels);

[ ... ]

> +	unsigned int i, j;
> +
> +	for (i = 0; i < ARRAY_SIZE(gstage_pg_levels); i++) {
> +		gstage_pg_levels[i].mask = 0;
> +		for (j = 0; j < ARRAY_SIZE(gstage_pte_bits); j++)
> +			gstage_pg_levels[i].mask |= gstage_pte_bits[j].mask;
> +	}
> +
> +	if (kvm->arch.pgd_levels < 5)
> +		gstage_pg_levels[1].name = "PGD";
> +	if (kvm->arch.pgd_levels < 4)
> +		gstage_pg_levels[2].name = "PGD";
> +	if (kvm->arch.pgd_levels < 3)
> +		gstage_pg_levels[3].name = "PGD";

[Severity: Medium]
Is it safe to mutate the static global array gstage_pg_levels here without
any synchronization?

Since gstage_pg_levels is a global static array, concurrent reads of the
debugfs file for different VMs will race here, which could lead to omitted
protection bits.

Additionally, downgrading a level's name to "PGD" permanently modifies the
array for subsequent reads of VMs that might have deeper page tables. Could
this state be maintained per-VM instead of globally?

> +
> +	ptdump_walk_pgd(&st.ptdump, kvm->mm, kvm->arch.pgd);

[Severity: High]
Does this traversal require kvm->mmu_lock to safely walk the guest page
tables?

If a user reads the debugfs file while the VM is concurrently modifying or
tearing down its page tables, the walker will traverse these structures
locklessly. ptdump_walk_pgd() acquires the host userspace mmap_write_lock
for kvm->mm, but does not acquire kvm->mmu_lock, which could lead to
dereferencing freed intermediate page table pages (use-after-free).


[Severity: High]
Can the host's generic page table walker safely handle KVM guest stage-2
page tables with different geometries?

KVM RISC-V guest page tables have dynamically configured depth through
kvm->arch.pgd_levels, which can differ from the host's compile-time depth
in CONFIG_PGTABLE_LEVELS.

If the host uses 4 levels and the guest uses 3 levels, the generic walker
will treat the guest's 3-level root as a 4-level root. It will descend one
level too deep, extracting a PFN from leaf PTEs, converting it to a kernel
virtual address, and dereferencing it. Could this trigger an out-of-bounds
direct map access and panic the host kernel?


[Severity: Medium]
What happens if kvm->arch.pgd is NULL when this is called?

If the debugfs file is read while kvm_arch_flush_shadow_all() is executing
during process exit, kvm->arch.pgd could be NULL.

Inside the generic walker, it detects a NULL pgd and falls back to
pgd_offset(walk->mm, addr). Since walk->mm is the host QEMU process's mm,
the walker will format and leak the host process's page table layout as if
it were the guest's stage-2 memory. Should we check for a NULL pgd before
initiating the walk?

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727123013.118984-1-fredwudi0305@gmail.com?part=3