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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete transition to per-instance and per-level page table attributes in ptdump. The commit introduces a `pg_level` pointer in `struct ptdump_pg_state` to allow different callers (like future KVM gstage code) to use their own attribute arrays. However, while `dump_prot()` uses this pointer, `note_page()` and `dump_addr()` still hardcode the global `kernel_pg_levels` array. Additionally, `ptdump_init()` hardcodes `pte_bits` when calculating masks instead of using the newly added per-level `bits` and `num` fields.
--

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

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

This commit renames ptdump structures and adds a pg_level pointer to
ptdump_pg_state. This allows different callers (like future KVM gstage code)
to define and use their own page table attributes rather than relying on a
global array.

> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index d9a955b8a532d..c67fbbf5ff624 100644
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
[ ... ]
> @@ -221,10 +233,10 @@ static void dump_addr(struct 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 print incorrect level names if a caller provides a custom
pg_level array? If the goal is to allow per-instance attribute arrays,
should this use st->pg_level[st->level].name instead of the global
kernel_pg_levels array?

[ ... ]
> @@ -242,12 +254,12 @@ static void note_prot_wx(struct pg_state *st, unsigned long addr)
>  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;
>  
>  	if (level >= 0)
> -		prot = val & pg_level[level].mask;
> +		prot = val & kernel_pg_levels[level].mask;

[Severity: Medium]
Does this bypass the custom attributes provided by the caller?

Using the hardcoded kernel_pg_levels array here will mask the PTE values
with the global mask, rather than using st->pg_level[level].mask from the
ptdump_pg_state instance.

[ ... ]
> @@ -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;

[Severity: Medium]
Since kernel_pg_levels[i] now contains its own .bits and .num fields,
should this initialization loop iterate over kernel_pg_levels[i].bits
up to kernel_pg_levels[i].num instead of hardcoding the global pte_bits
array?

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