Re: [PATCH v2 2/2] arm64: mm: implement vmemmap_check_pmd for arm64

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Dec 27, 2024 at 08:27:18AM +0530, Anshuman Khandual wrote:
> On 12/21/24 00:05, Catalin Marinas wrote:
> > On Mon, Dec 09, 2024 at 05:42:27PM +0800, Zhenhua Huang wrote:
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index fd59ee44960e..41c7978a92be 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -1169,7 +1169,8 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
> >>  				unsigned long addr, unsigned long next)
> >>  {
> >>  	vmemmap_verify((pte_t *)pmdp, node, addr, next);
> >> -	return 1;
> >> +
> >> +	return pmd_sect(*pmdp);
> 
> Please change this as pmd_sect(READ_ONCE(*pmdp)) instead.
> 
> >>  }
> >>  
> >>  int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> > 
> > Don't we need this patch only if we implement the first one? Please fold
> > it into the other patch.
> 
> Seems like these patches might not be related.
> 
> While creating huge page based vmemmap mapping during vmemmap_populate_hugepages(),
> vmemmap_check_pmd() validates if a populated (i.e pmd_none) PMD already represents
> a huge mapping and can be skipped there after.
> 
> Current implementation for vmemmap_check_pmd() on arm64, unconditionally returns 1
> thus asserting that the given populated PMD entry is a huge one indeed, which will
> be the case unless something is wrong. vmemmap_verify() only ensures that the node
> where the pfn is allocated from is local.
> 
> int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
>                                 unsigned long addr, unsigned long next)
> {
>         vmemmap_verify((pte_t *)pmdp, node, addr, next);
>         return 1;
> }
> 
> However it does not really check the entry to be a section mapping which it should.
> Returning pmd_sect(READ_ONCE(*pmdp)) is the right thing, which should have been the
> case from the beginning when vmemmap_check_pmd() was added. I guess because arm64's
> original vmemmap_populate() checked only for vmemmap_verify() as well. So probably
> this does not need a "Fixes: " tag.

I did not say the patch is wrong, only that it wouldn't be needed unless
we have the other patch in this series. However, if we do apply the
other patch, we definitely need this change, so keeping them together
would make it easier to backport.

-- 
Catalin