Re: [PATCH] mm: fix double page fault on arm64 if PTE_AF is cleared

[Anshuman Khandual]

On 09/04/2019 10:27 AM, Justin He (Arm Technology China) wrote:
> Hi Anshuman, thanks for the comments, see below please
> 
>> -----Original Message-----
>> From: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> Sent: 2019å9æ4æ 12:38
>> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; Andrew
>> Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; Matthew Wilcox
>> <willy@xxxxxxxxxxxxx>; JÃrÃme Glisse <jglisse@xxxxxxxxxx>; Ralph
>> Campbell <rcampbell@xxxxxxxxxx>; Jason Gunthorpe <jgg@xxxxxxxx>;
>> Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Dave Airlie <airlied@xxxxxxxxxx>;
>> Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>; Thomas Hellstrom
>> <thellstrom@xxxxxxxxxx>; Souptick Joarder <jrdr.linux@xxxxxxxxx>;
>> linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH] mm: fix double page fault on arm64 if PTE_AF is
>> cleared
>>
>>
>>
>> On 09/04/2019 08:49 AM, Anshuman Khandual wrote:
>>>  		/*
>>>  		 * This really shouldn't fail, because the page is there
>>>  		 * in the page tables. But it might just be unreadable,
>>>  		 * in which case we just give up and fill the result with
>>> -		 * zeroes.
>>> +		 * zeroes. If PTE_AF is cleared on arm64, it might
>>> +		 * cause double page fault here. so makes pte young here
>>>  		 */
>>> +		if (!pte_young(vmf->orig_pte)) {
>>> +			entry = pte_mkyoung(vmf->orig_pte);
>>> +			if (ptep_set_access_flags(vmf->vma, vmf->address,
>>> +				vmf->pte, entry, vmf->flags &
>> FAULT_FLAG_WRITE))
>>> +				update_mmu_cache(vmf->vma, vmf-
>>> address,
>>> +						vmf->pte);
>>> +		}
>>
>> This looks correct where it updates the pte entry with PTE_AF which
>> will prevent a subsequent page fault. But I think what we really need
>> here is to make sure 'uaddr' is mapped correctly at vma->pte. Probably
>> a generic function arch_map_pte() when defined for arm64 should check
>> CPU version and ensure continuance of PTE_AF if required. The comment
>> above also need to be updated saying not only the page should be there
>> in the page table, it needs to mapped appropriately as well.
> 
> I agree that a generic interface here is needed but not the arch_map_pte().
> In this case, I thought all the pgd/pud/pmd/pte had been set correctly except
> for the PTE_AF bit.
> How about arch_hw_access_flag()?

Sure, go ahead. I just meant 'map' to include not only the PFN but also
appropriate HW attributes not cause a page fault.

> If non-arm64, arch_hw_access_flag() == true

The function does not need to return anything. Dummy default definition
in generic MM will do nothing when arch does not override.

> If arm64 with hardware-managed access flag supported, == true
> else == false?

On arm64 with hardware-managed access flag supported, it will do nothing.
But in case its not supported the above mentioned pte update as in the
current proposal needs to be executed. The details should hide in arch
specific override.