Re: [RFC V1 07/16] arm64/mm: Convert READ_ONCE() as p4dp_get() while accessing P4D

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 08/04/26 5:47 PM, David Hildenbrand (Arm) wrote:
> On 2/24/26 06:11, Anshuman Khandual wrote:
>> Convert all READ_ONCE() based P4D accesses as p4dp_get() instead which will
>> support both D64 and D128 translation regime going forward.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: kasan-dev@googlegroups.com
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
> 
> 
> [...]
> 
>>  static void __init kasan_pgd_populate(unsigned long addr, unsigned long end,
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index a80d06db4de6..16ae11b29f66 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -354,7 +354,7 @@ static int alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
>>  {
>>  	int ret = 0;
>>  	unsigned long next;
>> -	p4d_t p4d = READ_ONCE(*p4dp);
>> +	p4d_t p4d = p4dp_get(p4dp);
>>  	pud_t *pudp;
>>  
>>  	if (p4d_none(p4d)) {
>> @@ -443,7 +443,7 @@ static int alloc_init_p4d(pgd_t *pgdp, unsigned long addr, unsigned long end,
>>  	}
>>  
>>  	do {
>> -		p4d_t old_p4d = READ_ONCE(*p4dp);
>> +		p4d_t old_p4d = p4dp_get(p4dp);
>>  
>>  		next = p4d_addr_end(addr, end);
>>  
>> @@ -453,7 +453,7 @@ static int alloc_init_p4d(pgd_t *pgdp, unsigned long addr, unsigned long end,
>>  			goto out;
>>  
>>  		BUG_ON(p4d_val(old_p4d) != 0 &&
>> -		       p4d_val(old_p4d) != READ_ONCE(p4d_val(*p4dp)));
>> +		       p4d_val(old_p4d) != (p4d_val(p4dp_get(p4dp))));
> 
> Same here, while at it remove the BUG_ON. (see below)
> 
>>  
>>  		phys += next - addr;
>>  	} while (p4dp++, addr = next, addr != end);
>> @@ -1541,7 +1541,7 @@ static void unmap_hotplug_p4d_range(pgd_t *pgdp, unsigned long addr,
>>  	do {
>>  		next = p4d_addr_end(addr, end);
>>  		p4dp = p4d_offset(pgdp, addr);
>> -		p4d = READ_ONCE(*p4dp);
>> +		p4d = p4dp_get(p4dp);
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> @@ -1703,7 +1703,7 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
>>  	do {
>>  		next = p4d_addr_end(addr, end);
>>  		p4dp = p4d_offset(pgdp, addr);
>> -		p4d = READ_ONCE(*p4dp);
>> +		p4d = p4dp_get(p4dp);
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> @@ -1724,7 +1724,7 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
>>  	 */
>>  	p4dp = p4d_offset(pgdp, 0UL);
>>  	for (i = 0; i < PTRS_PER_P4D; i++) {
>> -		if (!p4d_none(READ_ONCE(p4dp[i])))
>> +		if (!p4d_none(p4dp_get(p4dp + i)))
>>  			return;
>>  	}
>>  
>> @@ -2258,4 +2258,21 @@ int pmdp_test_and_clear_young(struct vm_area_struct *vma,
>>  }
>>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG */
>>  
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> +phys_addr_t pud_offset_phys(p4d_t *p4dp, unsigned long addr)
>> +{
>> +	p4d_t p4d = p4dp_get(p4dp);
>> +
>> +	BUG_ON(!pgtable_l4_enabled());
> 
> Heh, while at it, convert that to a VM_WARN_ON_ONCE() or anything else
> that is not a BUG.
> 
> I strongly assume CONFIG_DEBUG_VM checks are sufficient.

There are multiple similar BUG_ON() instances

arch/arm64/include/asm/pgtable.h:       BUG_ON(!pgtable_l4_enabled());
arch/arm64/include/asm/pgtable.h:       BUG_ON(!pgtable_l5_enabled());

arch/arm64/mm/mmu.c:                    BUG_ON(pmd_val(old_pmd) != 0 &&
arch/arm64/mm/mmu.c:                    BUG_ON(pud_val(old_pud) != 0 &&
arch/arm64/mm/mmu.c:            BUG_ON(p4d_val(old_p4d) != 0 &&

Shall we convert all of them as VM_WARN_ON_ONCE() in a separate patch
as a pre-requisite ?

> 
>> +
>> +	return p4d_page_paddr(p4d) + pud_index(addr) * sizeof(pud_t);
>> +}
>> +
>