Re: [PATCH 11/21] riscv: mm: use proper set_pXd() for generic compile-time folded patable in vmalloc_fault()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/21/26 12:55, Yeoreum Yun wrote:
> The behaviour of pXdp_get() will change with generic compile-time folded
> page tables, making it invalid to use set_pXd() with the result of
> pXdp_get(). Instead, the appropriate first-level set_pXd() must be paired
> with the corresponding pXdp_get().
> 
> Therefore, call the appropriate set_pXd() for generic compile-time folded
> page tables so that vmalloc_fault() continues to handle folded entries
> correctly when checking pXd_present().
> 
> There should be no functional change.
> 
> Since most architectures use a similar pattern to handle vmalloc fault
> by syncing the faulted entry, it would be worth cleaning this up by
> introducing a common helper in the near future.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---

@ Riscv maintainers, is this good enough until we get some more generic cleanups
in this area?


>  arch/riscv/mm/fault.c | 52 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
> index 04ed6f8acae4f..b2bcaf10d13f1 100644
> --- a/arch/riscv/mm/fault.c
> +++ b/arch/riscv/mm/fault.c
> @@ -167,14 +167,16 @@ bad_area(struct pt_regs *regs, struct mm_struct *mm, int code,
>  
>  static inline void vmalloc_fault(struct pt_regs *regs, int code, unsigned long addr)
>  {
> -	pgd_t *pgd, *pgd_k;
> -	pud_t *pud_k;
> -	p4d_t *p4d_k;
> -	pmd_t *pmd_k;
> -	pte_t *pte_k;
> +	pgd_t *pgdp_k, *pgdp, pgd_k;
> +	p4d_t *p4dp_k, *p4dp;
> +	pud_t *pudp_k, *pudp, pud_k;
> +	pmd_t *pmdp_k, *pmdp, pmd_k;
> +	pte_t *ptep_k;
>  	int index;
>  	unsigned long pfn;
>  
> +	BUILD_BUG_ON(CONFIG_PGTABLE_LEVELS != 5 && CONFIG_PGTABLE_LEVELS != 2);
> +
>  	/* User mode accesses just cause a SIGSEGV */
>  	if (user_mode(regs))
>  		return do_trap(regs, SIGSEGV, code, addr);
> @@ -189,39 +191,51 @@ static inline void vmalloc_fault(struct pt_regs *regs, int code, unsigned long a
>  	 */
>  	index = pgd_index(addr);
>  	pfn = csr_read(CSR_SATP) & SATP_PPN;
> -	pgd = (pgd_t *)pfn_to_virt(pfn) + index;
> -	pgd_k = init_mm.pgd + index;
>  
> -	if (!pgd_present(pgdp_get(pgd_k))) {
> +	pgdp = (pgd_t *)pfn_to_virt(pfn) + index;
> +	pgdp_k = init_mm.pgd + index;
> +
> +	pgd_k = pgdp_get(pgdp_k);
> +	if (!pgd_present(pgd_k)) {
>  		no_context(regs, addr);
>  		return;
>  	}
> -	set_pgd(pgd, pgdp_get(pgd_k));
> +	if (CONFIG_PGTABLE_LEVELS == 5)
> +		set_pgd(pgdp, pgd_k);
>  
> -	p4d_k = p4d_offset(pgd_k, addr);
> -	if (!p4d_present(p4dp_get(p4d_k))) {
> +	p4dp = p4d_offset(pgdp, addr);
> +	p4dp_k = p4d_offset(pgdp_k, addr);
> +	if (!p4d_present(p4dp_get(p4dp_k))) {
>  		no_context(regs, addr);
>  		return;
>  	}
>  
> -	pud_k = pud_offset(p4d_k, addr);
> -	if (!pud_present(pudp_get(pud_k))) {
> +	pudp = pud_offset(p4dp, addr);
> +	pudp_k = pud_offset(p4dp_k, addr);
> +
> +	pud_k = pudp_get(pudp_k);
> +	if (!pud_present(pud_k)) {
>  		no_context(regs, addr);
>  		return;
>  	}
> -	if (pud_leaf(pudp_get(pud_k)))
> +	if (pud_leaf(pud_k))
>  		goto flush_tlb;
>  
>  	/*
>  	 * Since the vmalloc area is global, it is unnecessary
>  	 * to copy individual PTEs
>  	 */
> -	pmd_k = pmd_offset(pud_k, addr);
> -	if (!pmd_present(pmdp_get(pmd_k))) {
> +	pmdp = pmd_offset(pudp, addr);
> +	pmdp_k = pmd_offset(pudp_k, addr);
> +
> +	pmd_k = pmdp_get(pmdp_k);
> +	if (!pmd_present(pmd_k)) {
>  		no_context(regs, addr);
>  		return;
>  	}
> -	if (pmd_leaf(pmdp_get(pmd_k)))
> +	if (CONFIG_PGTABLE_LEVELS == 2)
> +		set_pmd(pmdp, pmd_k);
> +	if (pmd_leaf(pmd_k))
>  		goto flush_tlb;
>  
>  	/*
> @@ -230,8 +244,8 @@ static inline void vmalloc_fault(struct pt_regs *regs, int code, unsigned long a
>  	 * addresses. If we don't do this, this will just
>  	 * silently loop forever.
>  	 */
> -	pte_k = pte_offset_kernel(pmd_k, addr);
> -	if (!pte_present(ptep_get(pte_k))) {
> +	ptep_k = pte_offset_kernel(pmdp_k, addr);
> +	if (!pte_present(ptep_get(ptep_k))) {
>  		no_context(regs, addr);
>  		return;
>  	}
> 


-- 
Cheers,

David