Re: [PATCH 2/6] arm64: use hw_pte_val for HW PTE atomics

[Muhammad Usama Anjum <usama.anjum@arm.com>]

On 18/09/2026 4:55 pm, Ryan Roberts wrote:
> On 14/09/2026 14:51, Muhammad Usama Anjum wrote:
>> Add hw_pte_val() to preserve an lvalue for the HW PTE bits, so atomic
>> updates can take their address. pte_val() expects a SW PTE value and
>> cannot operate directly on a distinct hw_pte_t.
>>
>> With ARCH_HAS_HW_PTE_T, use pte_val() on the wrapper's __pte member;
>> otherwise, use pte_val() directly.
>>
>> The atomic operations and their ordering are unchanged.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
>> ---
>>  arch/arm64/include/asm/pgtable.h | 10 +++++-----
>>  arch/arm64/mm/fault.c            |  6 +++---
>>  include/linux/pgtable_types.h    |  4 ++++
>>  3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 652ce413be389..4768ec59de555 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -1303,7 +1303,7 @@ static inline bool __ptep_test_and_clear_young(struct vm_area_struct *vma,
>>  	do {
>>  		old_pte = pte;
>>  		pte = pte_mkold(pte);
>> -		pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep),
>> +		pte_val(pte) = cmpxchg_relaxed(&hw_pte_val(*ptep),
>>  					       pte_val(old_pte), pte_val(pte));
>>  	} while (pte_val(pte) != pte_val(old_pte));
>>  
>> @@ -1346,7 +1346,7 @@ static inline pte_t __ptep_get_and_clear_anysz(struct mm_struct *mm,
>>  					       hw_pte_t *ptep,
>>  					       unsigned long pgsize)
>>  {
>> -	pte_t pte = __pte(xchg_relaxed(&pte_val(*ptep), 0));
>> +	pte_t pte = __pte(xchg_relaxed(&hw_pte_val(*ptep), 0));
>>  
>>  	switch (pgsize) {
>>  	case PAGE_SIZE:
>> @@ -1422,7 +1422,7 @@ static inline void ___ptep_set_wrprotect(struct mm_struct *mm,
>>  	do {
>>  		old_pte = pte;
>>  		pte = pte_wrprotect(pte);
>> -		pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep),
>> +		pte_val(pte) = cmpxchg_relaxed(&hw_pte_val(*ptep),
>>  					       pte_val(old_pte), pte_val(pte));
>>  	} while (pte_val(pte) != pte_val(old_pte));
>>  }
>> @@ -1460,7 +1460,7 @@ static inline void __clear_young_dirty_pte(struct vm_area_struct *vma,
>>  		if (flags & CYDP_CLEAR_DIRTY)
>>  			pte = pte_mkclean(pte);
>>  
>> -		pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep),
>> +		pte_val(pte) = cmpxchg_relaxed(&hw_pte_val(*ptep),
>>  					       pte_val(old_pte), pte_val(pte));
>>  	} while (pte_val(pte) != pte_val(old_pte));
>>  }
>> @@ -1830,7 +1830,7 @@ static inline bool ptep_try_set(hw_pte_t *ptep, pte_t new_pte)
>>  {
>>  	pteval_t old = 0;
>>  
>> -	if (!try_cmpxchg(&pte_val(*ptep), &old, pte_val(new_pte)))
>> +	if (!try_cmpxchg(&hw_pte_val(*ptep), &old, pte_val(new_pte)))
>>  		return false;
>>  
>>  	/*
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index b77f4be88e3ea..7d6c30f27214e 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -225,8 +225,8 @@ int __ptep_set_access_flags_anysz(struct vm_area_struct *vma,
>>  	/*
>>  	 * Setting the flags must be done atomically to avoid racing with the
>>  	 * hardware update of the access/dirty state. The PTE_RDONLY bit must
>> -	 * be set to the most permissive (lowest value) of *ptep and entry
>> -	 * (calculated as: a & b == ~(~a | ~b)).
>> +	 * be set to the most permissive (lowest value) of the current PTE and
>> +	 * entry (calculated as: a & b == ~(~a | ~b)).
> 
> This seems like an unrelated and unecessary comment change?

I'm trying to replace *ptep with current PTE for consistency. I'll drop this
change.

> 
>>  	 */
>>  	pte_val(entry) ^= PTE_RDONLY;
>>  	pteval = pte_val(pte);
>> @@ -235,7 +235,7 @@ int __ptep_set_access_flags_anysz(struct vm_area_struct *vma,
>>  		pteval ^= PTE_RDONLY;
>>  		pteval |= pte_val(entry);
>>  		pteval ^= PTE_RDONLY;
>> -		pteval = cmpxchg_relaxed(&pte_val(*ptep), old_pteval, pteval);
>> +		pteval = cmpxchg_relaxed(&hw_pte_val(*ptep), old_pteval, pteval);
>>  	} while (pteval != old_pteval);
>>  
>>  	/*
>> diff --git a/include/linux/pgtable_types.h b/include/linux/pgtable_types.h
>> index d6c5a7548550b..ee4eace5c3e1c 100644
>> --- a/include/linux/pgtable_types.h
>> +++ b/include/linux/pgtable_types.h
>> @@ -9,9 +9,13 @@
>>  #ifdef CONFIG_ARCH_HAS_HW_PTE_T
>>  typedef struct __hw_pte_t { pte_t __pte; } hw_pte_t;
>>  #define __pte_from_hw(pte)	((pte).__pte)
>> +
> 
> nit: why the newline here (and equivalent below)?

I'll drop the newlines.

> 
>> +#define hw_pte_val(x)  pte_val((x).__pte)
> 
> Wouldn't it be better to add these as part of the generic series? I know we
> prefer to add an api along with its first user, but in this case it seems odd,
> because you're effectively requiring that arm64 is the first merged arch to
> support this? You could also use the same argument to say that none of this
> should be merged until the commit where an arch turns on ARCH_HAS_HW_PTE_T.

Agreed. Thanks for explaining. I'll move the definition to the generic series.

> 
> Thanks,
> Ryan
> 
> 
>>  #else
>>  #define hw_pte_t pte_t
>>  #define __pte_from_hw(pte)	(pte)
>> +
>> +#define hw_pte_val(x)  pte_val(x)
>>  #endif
>>  
>>  #endif /* !__ASSEMBLY__ */
>>
> 


-- 
Thanks,
Usama