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

[Barry Song <21cnbao@gmail.com>]

On Thu, Sep 5, 2024 at 3:50 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> 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?

It's not your fault. I just realized that, for some unknown reason, I forgot to
CC you.

>
> >
> > 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.

If this is the case, I'd rather add some comments instead in v2?

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index a3edced29ac1..55107d27d3f8 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -421,6 +421,12 @@ 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);

+               /*
+                * We are not advancing entry because __ptep_set_access_flags()
+                * only consumes access flags from entry. And since we
have checked
+                * for the whole contpte block and returned early, pte_same()
+                * within __ptep_set_access_flags() is likely false.
+                */
                for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
                        __ptep_set_access_flags(vma, addr, ptep, entry, 0);
-- 
2.39.3 (Apple Git-146)

Thanks
Barry