Re: [PATCH v1 2/3] arm64: mm: Handle invalid large leaf mappings correctly

[Ryan Roberts]

On 23/03/2026 16:52, Kevin Brodsky wrote:
> On 23/03/2026 14:03, Ryan Roberts wrote:
>> [...]
>>
>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>> index 358d1dc9a576f..87dfe4c82fa92 100644
>> --- a/arch/arm64/mm/pageattr.c
>> +++ b/arch/arm64/mm/pageattr.c
>> @@ -25,6 +25,11 @@ static ptdesc_t set_pageattr_masks(ptdesc_t val, struct mm_walk *walk)
>>  {
>>  	struct page_change_data *masks = walk->private;
>>  
>> +	/*
>> +	 * Some users clear and set bits which alias eachother (e.g. PTE_NG and
> 
> Nit: "each other"
> 
>> +	 * PTE_PRESENT_INVALID). It is therefore important that we always clear
>> +	 * first then set.
>> +	 */
>>  	val &= ~(pgprot_val(masks->clear_mask));
>>  	val |= (pgprot_val(masks->set_mask));
>>  
>> @@ -36,7 +41,7 @@ static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
>>  {
>>  	pud_t val = pudp_get(pud);
>>  
>> -	if (pud_sect(val)) {
>> +	if (pud_leaf(val)) {
>>  		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
>>  			return -EINVAL;
>>  		val = __pud(set_pageattr_masks(pud_val(val), walk));
>> @@ -52,7 +57,7 @@ static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
>>  {
>>  	pmd_t val = pmdp_get(pmd);
>>  
>> -	if (pmd_sect(val)) {
>> +	if (pmd_leaf(val)) {
>>  		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
>>  			return -EINVAL;
>>  		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
>> @@ -132,11 +137,12 @@ static int __change_memory_common(unsigned long start, unsigned long size,
>>  	ret = update_range_prot(start, size, set_mask, clear_mask);
>>  
>>  	/*
>> -	 * If the memory is being made valid without changing any other bits
>> -	 * then a TLBI isn't required as a non-valid entry cannot be cached in
>> -	 * the TLB.
>> +	 * If the memory is being switched from present-invalid to valid without
>> +	 * changing any other bits then a TLBI isn't required as a non-valid
>> +	 * entry cannot be cached in the TLB.
>>  	 */
>> -	if (pgprot_val(set_mask) != PTE_VALID || pgprot_val(clear_mask))
>> +	if (pgprot_val(set_mask) != (PTE_MAYBE_NG | PTE_VALID) ||
> 
> It isn't obvious to understand where all those PTE_MAYBE_NG come from if
> one hasn't realised that PTE_PRESENT_INVALID overlays PTE_NG.
> 
> Since for this purpose we always set/clear both PTE_VALID and
> PTE_MAYBE_NG, maybe we could define some macro as PTE_VALID |
> PTE_MAYBE_NG, as a counterpart to PTE_PRESENT_INVALID?

How about:

#define PTE_PRESENT_VALID_KERNEL	(PTE_VALID | PTE_MAYBE_NG)

The user space equivalent has NG clear, so important to clarify that this is the
kernel value, I think.

Thanks,
Ryan

> 
> - Kevin
> 
>> [...]