Re: [RFC V2 07/14] arm64/mm: Route all pgtable reads via pxxval_get()

[Anshuman Khandual]

On 27/05/26 7:41 PM, Ryan Roberts wrote:
> On 13/05/2026 05:45, Anshuman Khandual wrote:
>> Define arm64 platform specific implementations for new pXdp_get() helpers.
>> These resolve into READ_ONCE(), thus ensuring required single copy atomic
>> semantics for the page table entry reads.
>>
>> In future this infrastructure can be used for D128 to maintain single copy
>> atomicity semantics with inline asm blocks.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> Changes in RFC V2:
>>
>> - Renamed all ptdesc_ instances as pxxval_ instead
>> - Moved arm64 pgtable header READ_ONCE() replacements here in this patch
>>
>>  arch/arm64/include/asm/pgtable.h | 38 +++++++++++++++++++++++++++-----
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index cefe8ab86acd..72da582e8d12 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -84,6 +84,32 @@ static inline void arch_leave_lazy_mmu_mode(void)
>>  	arch_flush_lazy_mmu_mode();
>>  }
>>  
>> +#define pxxval_get(x)		READ_ONCE(x)
> 
> I think this would be better as an inline (or __force_inline) function, which
> takes a pointer, and dereferences it. READ_ONCE() is special - it's modifying
> how the dereference should be done, so it makes sense that the dereference is
> done in the caller.
> 
>> +
>> +#define pmdp_get pmdp_get
>> +static inline pmd_t pmdp_get(pmd_t *pmdp)
>> +{
>> +	return pxxval_get(*pmdp);
> 
> This is weird to read. It would be clearer as:
> 
> 	return pxxval_get(pmdp);

But being a macro enables pxxval_get() to be used for all page table
levels as the dereference is done in the callers. Otherwise we would
need separate inline functions which take the corresponding pointers
for each pgtable level. Just wondering - would that be any better ?

Although pxxval_get(*pmdp) format does look bit weird to read.
> 
> Thanks,
> Ryan
> 
>> +}
>> +
>> +#define pudp_get pudp_get
>> +static inline pud_t pudp_get(pud_t *pudp)
>> +{
>> +	return pxxval_get(*pudp);
>> +}
>> +
>> +#define p4dp_get p4dp_get
>> +static inline p4d_t p4dp_get(p4d_t *p4dp)
>> +{
>> +	return pxxval_get(*p4dp);
>> +}
>> +
>> +#define pgdp_get pgdp_get
>> +static inline pgd_t pgdp_get(pgd_t *pgdp)
>> +{
>> +	return pxxval_get(*pgdp);
>> +}
>> +
>>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>  #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE
>>  
>> @@ -380,7 +406,7 @@ static inline void __set_pte(pte_t *ptep, pte_t pte)
>>  
>>  static inline pte_t __ptep_get(pte_t *ptep)
>>  {
>> -	return READ_ONCE(*ptep);
>> +	return pxxval_get(*ptep);
>>  }
>>  
>>  extern void __sync_icache_dcache(pte_t pteval);
>> @@ -1011,7 +1037,7 @@ static inline phys_addr_t pud_offset_phys(p4d_t *p4dp, unsigned long addr)
>>  {
>>  	BUG_ON(!pgtable_l4_enabled());
>>  
>> -	return p4d_page_paddr(READ_ONCE(*p4dp)) + pud_index(addr) * sizeof(pud_t);
>> +	return p4d_page_paddr(p4dp_get(p4dp)) + pud_index(addr) * sizeof(pud_t);
>>  }
>>  
>>  static inline
>> @@ -1025,7 +1051,7 @@ pud_t *pud_offset_lockless(p4d_t *p4dp, p4d_t p4d, unsigned long addr)
>>  
>>  static inline pud_t *pud_offset(p4d_t *p4dp, unsigned long addr)
>>  {
>> -	return pud_offset_lockless(p4dp, READ_ONCE(*p4dp), addr);
>> +	return pud_offset_lockless(p4dp, p4dp_get(p4dp), addr);
>>  }
>>  #define pud_offset	pud_offset
>>  
>> @@ -1134,7 +1160,7 @@ static inline phys_addr_t p4d_offset_phys(pgd_t *pgdp, unsigned long addr)
>>  {
>>  	BUG_ON(!pgtable_l5_enabled());
>>  
>> -	return pgd_page_paddr(READ_ONCE(*pgdp)) + p4d_index(addr) * sizeof(p4d_t);
>> +	return pgd_page_paddr(pgdp_get(pgdp)) + p4d_index(addr) * sizeof(p4d_t);
>>  }
>>  
>>  static inline
>> @@ -1148,7 +1174,7 @@ p4d_t *p4d_offset_lockless(pgd_t *pgdp, pgd_t pgd, unsigned long addr)
>>  
>>  static inline p4d_t *p4d_offset(pgd_t *pgdp, unsigned long addr)
>>  {
>> -	return p4d_offset_lockless(pgdp, READ_ONCE(*pgdp), addr);
>> +	return p4d_offset_lockless(pgdp, pgdp_get(pgdp), addr);
>>  }
>>  
>>  static inline p4d_t *p4d_set_fixmap(unsigned long addr)
>> @@ -1346,7 +1372,7 @@ static inline bool pmdp_test_and_clear_young(struct vm_area_struct *vma,
>>  		unsigned long address, pmd_t *pmdp)
>>  {
>>  	/* Operation applies to PMD table entry only if FEAT_HAFT is enabled */
>> -	VM_WARN_ON(pmd_table(READ_ONCE(*pmdp)) && !system_supports_haft());
>> +	VM_WARN_ON(pmd_table(pmdp_get(pmdp)) && !system_supports_haft());
>>  	return __ptep_test_and_clear_young(vma, address, (pte_t *)pmdp);
>>  }
>>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG */
>