Re: [PATCH] arm64/mm: Avoid direct referencing page table enties in map_range()

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

On 7/25/24 16:06, Ryan Roberts wrote:
> On 25/07/2024 10:10, Anshuman Khandual wrote:
>> Like else where in arm64 platform, use WRITE_ONCE() in map_range() while
>> creating page table entries. This avoids referencing page table entries
>> directly.
> 
> I could be wrong, but I don't think this code is ever operating on live

map_range() is called on these page tables but sequentially during boot.

primary_entry()
	create_init_idmap()
		map_range(...init_idmap_pg_dir...)

primary_switch()
	early_map_kernel()
		map_fdt()
			map_range(...init_idmap_pg_dir...)

		remap_idmap_for_lpa2()
			create_init_idmap()
				map_range(...init_pg_dir...)
			create_init_idmap()
				map_range(...init_idmap_pg_dir...)

		map_kernel()
			map_segment()
				map_range(...init_pg_dir...)
paging_init()
	create_idmap()
		__pi_map_range(...idmap_pg_dir...)


> pgtables? So there is never a potential to race with the HW walker and therefore
> no need to guarrantee copy atomicity? As long as the correct barriers are placed

Unless there is possibility of concurrent HW walk through these page
tables, WRITE_ONCE() based atomic is not required here ?

I thought arm64 platform decided some time earlier (but don't remember
when) to use READ_ONCE()-WRITE_ONCE() for all page table entry, direct
references for read or write accesses - possibly for some increased
safety ?

> at the point where you load the pgdir into the TTBRx there should be no problem?

Those barriers are already placed as required.

> 
> If my assertion is correct, I don't think there is any need for this change.
> 
> Thanks,
> Ryan
> 
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kernel/pi/map_range.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pi/map_range.c b/arch/arm64/kernel/pi/map_range.c
>> index 5410b2cac590..b93b70cdfb62 100644
>> --- a/arch/arm64/kernel/pi/map_range.c
>> +++ b/arch/arm64/kernel/pi/map_range.c
>> @@ -56,8 +56,8 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,
>>  			 * table mapping if necessary and recurse.
>>  			 */
>>  			if (pte_none(*tbl)) {
>> -				*tbl = __pte(__phys_to_pte_val(*pte) |
>> -					     PMD_TYPE_TABLE | PMD_TABLE_UXN);
>> +				WRITE_ONCE(*tbl, __pte(__phys_to_pte_val(*pte) |
>> +					   PMD_TYPE_TABLE | PMD_TABLE_UXN));
>>  				*pte += PTRS_PER_PTE * sizeof(pte_t);
>>  			}
>>  			map_range(pte, start, next, pa, prot, level + 1,
>> @@ -79,7 +79,7 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,
>>  				protval &= ~PTE_CONT;
>>  
>>  			/* Put down a block or page mapping */
>> -			*tbl = __pte(__phys_to_pte_val(pa) | protval);
>> +			WRITE_ONCE(*tbl, __pte(__phys_to_pte_val(pa) | protval));
>>  		}
>>  		pa += next - start;
>>  		start = next;
>