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

[Will Deacon <will@kernel.org>]

(Adding Ryan, since you're asking him a question!)

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?
> 
> right.
> 
> >
> >
> > OTOH, there is the initial:
> >
> >
> > if (pte_same(pte, entry))
> >         return 0;
> >
> > check that might accelerate things.
> >
> > 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?
> 
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
> 
> Thanks
> Barry