Re: [PATCH 1/2] riscv: ptdump: Move pagetable definitions to common header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Uninitialized `bits` and `num` fields in `kernel_pg_levels` completely suppress page table attribute output in debugfs.
- [Low] Folded PMD levels on 32-bit RISC-V (RV32) will incorrectly display as 'PMD' instead of 'PGD'.
--

commit f96cd5eefaf170c680412811c0fe9c407b08f9a0
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: ptdump: Move pagetable definitions to common header

Move the pagetable walking state and level definitions to a new
arch/riscv/include/asm/ptdump.h header to allow reuse of the attribute parsing
logic. The attribute parsing logic in dump_prot() is updated to use attribute
bits defined within the pg_level structure rather than a global array.

> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index f4b4a9fcbbd88..7e55656cbbc92 100644
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c

[ ... ]

> @@ -183,52 +152,48 @@ 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 kernel_pg_levels[] = {
>  	{ /* pgd */
>  		.name = "PGD",
>  	}, { /* p4d */
> -		.name = (CONFIG_PGTABLE_LEVELS > 4) ? "P4D" : "PGD",
> +		.name = "P4D",
>  	}, { /* pud */
> -		.name = (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",
> +		.name = "PUD",
>  	}, { /* pmd */
> -		.name = (CONFIG_PGTABLE_LEVELS > 2) ? "PMD" : "PGD",
> +		.name = "PMD",

[Severity: Low]
For 32-bit RISC-V (RV32) architectures that use a 2-level page table where PMD
is folded into PGD, will this cause the top-level entries to incorrectly
display as PMD instead of PGD?

The original code dynamically renamed the PMD level to PGD based on
CONFIG_PGTABLE_LEVELS, but this appears to be omitted from the new code.

>  	}, { /* pte */
>  		.name = "PTE",
>  	},
>  };
>  
> -static void dump_prot(struct pg_state *st)
> +static void dump_prot(struct ptdump_pg_state *st)
>  {
>  	unsigned int i;
> +	const struct ptdump_pg_level *lvl = &st->pg_level[st->level];
> +	const struct ptdump_prot_bits *bits = lvl->bits;
>  
> -	for (i = 0; i < ARRAY_SIZE(pte_bits); i++) {
> +	for (i = 0; i < lvl->num; i++) {

[Severity: Medium]
Will this loop ever execute?

The kernel_pg_levels array is statically initialized without setting the .bits
or .num fields. Since .num is implicitly 0, it looks like this loop is entirely
skipped, which will silently discard printing page table protection attributes
to debugfs.

[ ... ]

> @@ -446,12 +414,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;

Does this dynamic initialization loop need to populate the .bits and .num fields
for kernel_pg_levels as well?

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