Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Ryan Roberts <ryan.roberts@arm.com>]

On 04/09/2024 16:13, Will Deacon wrote:
> (Adding Ryan, since you're asking him a question!)

Thanks, Will!

Afraid I don't do a good job of monitoring the list; I'm guessing there are
automated ways to filter for mentions of my name so I catch this sort of thing
in future?

> 
> On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
>> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 31.08.24 10:35, Barry Song wrote:
>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>
>>>> Hi Ryan, David,
>>>> it seems contpte_ptep_set_access_flags() has never advanced
>>>> pte pfn, and it is setting all entries' pfn to the first
>>>> subpage. But I feel quite strange we never have a bug reported.
>>>> Am I missing something?
>>>>
>>>> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>>> ---
>>>>   arch/arm64/mm/contpte.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>>> index a3edced29ac1..10dcd2641184 100644
>>>> --- a/arch/arm64/mm/contpte.c
>>>> +++ b/arch/arm64/mm/contpte.c
>>>> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>>>>               ptep = contpte_align_down(ptep);
>>>>               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>>>>
>>>> -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>>>> +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
>>>>                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
>>>> +                     entry = pte_advance_pfn(entry, 1);
>>>> +             }
>>>>
>>>>               if (dirty)
>>>>                       __flush_tlb_range(vma, start_addr, addr,
>>>
>>> Taking a closer look at __ptep_set_access_flags(), there is:
>>>
>>> /* only preserve the access flags and write permission *
>>> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
>>>
>>> So it looks like it doesn't need the PFN?

Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
consumes the access flags from entry.

>>
>> right.
>>
>>>
>>>
>>> OTOH, there is the initial:
>>>
>>>
>>> if (pte_same(pte, entry))
>>>         return 0;
>>>
>>> check that might accelerate things.

There is an equivalent check in contpte_ptep_set_access_flags() which is
checking for the whole contpte block and returning early if so. So I don't think
there is a problem here either.

>>>
>>> So unless I am missing something, this works as expected? (and if the
>>> pte_same() would frequently be taken with your change would be worthwile
>>> to optimize)
>>
>>
>> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
>> for pte_same().
>> This seems quite strange. I think we might need to "fix" it, at least
>> for the sake of code
>> semantics. on the other hand, if pte_same() is not important, it
>> should be dropped.
>>
>> Hi Ryan,
>> what is your take on this?

The code is correct and working as intended, AFAICT. But I accept that this is
not exactly obvious. I'd be happy to Rb your proposed change if you feel it
clarifies things.

Thanks,
Ryan

>>
>>>
>>> --
>>> Cheers,
>>>
>>> David / dhildenb
>>>
>>
>> Thanks
>> Barry