Re: [PATCH 16/16] debug_vm_pgtable/ppc64: Add a variant of pfn_pte/pmd

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

On 08/13/2020 12:07 PM, Aneesh Kumar K.V wrote:
> On 8/13/20 11:00 AM, Anshuman Khandual wrote:
>>
>> On 08/12/2020 12:03 PM, Aneesh Kumar K.V wrote:
>>> The tests do expect _PAGE_PTE bit set by different page table accessors.
>>> This is not true for the kernel. Within the kernel, _PAGE_PTE bits are
>>> usually set by set_pte_at(). To make the below tests work correctly add test
>>> specific pfn_pte/pmd helpers that set _PAGE_PTE bit.
>>>
>>> pte_t pte = pfn_pte(pfn, prot);
>>> WARN_ON(!pte_devmap(pte_mkdevmap(pte)));
>>> WARN_ON(!pte_savedwrite(pte_mk_savedwrite(pte_clear_savedwrite(pte))));
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 65 +++++++++++++++++++++++++++----------------
>>>   1 file changed, 41 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index eea62d5e503b..153c925b5273 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -31,6 +31,23 @@
>>>   #include <asm/pgalloc.h>
>>>   #include <asm/tlbflush.h>
>>>   +#ifdef CONFIG_PPC_BOOK3S_64
>>> +static inline pte_t debug_vm_pfn_pte(unsigned long pfn, pgprot_t pgprot)
>>> +{
>>> +    pte_t pte = pfn_pte(pfn, pgprot);
>>> +    return __pte(pte_val(pte) | _PAGE_PTE);
>>> +
>>> +}
>>> +static inline pmd_t debug_vm_pfn_pmd(unsigned long pfn, pgprot_t pgprot)
>>> +{
>>> +    pmd_t pmd = pfn_pmd(pfn, pgprot);
>>> +    return __pmd(pmd_val(pmd) | _PAGE_PTE);
>>> +}
>>> +#else
>>> +#define debug_vm_pfn_pte(pfn, pgprot) pfn_pte(pfn, pgprot)
>>> +#define debug_vm_pfn_pmd(pfn, pgprot) pfn_pmd(pfn, pgprot)
>>> +#endif
>>
>> Again, no platform specific constructs please. This defeats the whole purpose of
>> this test. If __PAGE_PTE is required for the helpers, then pfn_pmd/pte() could
>> be modified to accommodate that. We dont see similar issues on other platforms,
>> hence could you please explain why ppc64 is different here.
>>
> 
> It is not platform specific. set_pte_at is the one that set the _PAGE_PTE bit. We don't call that in the test.  The test seems to make the assumption that pfn_pte returns a proper pte which is not true.

'#ifdef CONFIG_PPC_BOOK3S_64' definitely makes it platform specific. Here is how
set_pte_at() updates an entry on other platforms without changing the pte value.
_PAGE_PTE bit update during set_pte_at() on ppc64 seems to be a deviation.

1. set_pte_at() on arm64 directly update the entry after TLB, cache maintenance
2. set_pte_at() on s390 directly updates the entry for !CONFIG_PGSTE
3. set_pte_at() on arc directly updates the entry via set_pte()
4. set_pte_at() on x86 directly update the entry via native_set_pte()

set_pte_at() does take a pte created with pfn_pte().

As an example do_anonymous_page() does the same.

......
entry = mk_pte(page, vma->vm_page_prot);	/* Call pfn_pte() */
entry = pte_sw_mkyoung(entry);
if (vma->vm_flags & VM_WRITE)
	entry = pte_mkwrite(pte_mkdirty(entry));
.....
set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
.....