Received-SPF: pass (google.com: domain of linux-kernel+bounces-142404-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 12 Apr 2024 10:25:16 +0100
From: Mark Rutland <mark.rutland@arm.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>,
	David Hildenbrand <david@redhat.com>,
	Donald Dutile <ddutile@redhat.com>,
	Eric Chanudet <echanude@redhat.com>,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	Itaru Kitayama <itaru.kitayama@fujitsu.com>
Subject: Re: [PATCH v2 3/4] arm64: mm: Don't remap pgtables for allocate vs
 populate
Message-ID: <Zhj9_HFVVxEZqdnB@FVFF77S0Q05N>
References: <20240404143308.2224141-1-ryan.roberts@arm.com>
 <20240404143308.2224141-4-ryan.roberts@arm.com>
 <ZhffSyrqCQsMV2pG@FVFF77S0Q05N>
 <37336367-f876-4429-a8a6-f887fc7f69ee@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <37336367-f876-4429-a8a6-f887fc7f69ee@arm.com>

On Fri, Apr 12, 2024 at 08:53:18AM +0100, Ryan Roberts wrote:
> Hi Mark,
> 
> [...]
> 
> > Does something like the below look ok to you? The trade-off performance-wise is
> > that late uses will still use the fixmap, and will redundantly zero the tables,
> > but the logic remains fairly simple, and I suspect the overhead for late
> > allocations might not matter since the bulk of late changes are non-allocating.

> > @@ -303,12 +301,18 @@ static void alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
> >  			pudval |= PUD_TABLE_PXN;
> >  		BUG_ON(!pgtable_alloc);
> >  		pmd_phys = pgtable_alloc(PMD_SHIFT);
> > +
> > +		pmdp = pmd_set_fixmap(pmd_phys);
> > +		init_clear_pgtable(pmdp);
> > +
> >  		__pud_populate(pudp, pmd_phys, pudval);
> >  		pud = READ_ONCE(*pudp);
> > +	} else {
> > +		pmdp = pmd_set_fixmap(pud_page_paddr(pud));
> >  	}
> >  	BUG_ON(pud_bad(pud));
> >  
> > -	pmdp = pmd_set_fixmap_offset(pudp, addr);
> > +	pmdp += pmd_index(addr);
> >  	do {
> >  		pgprot_t __prot = prot;
> >  
> > @@ -345,12 +349,18 @@ static void alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
> >  			p4dval |= P4D_TABLE_PXN;
> >  		BUG_ON(!pgtable_alloc);
> >  		pud_phys = pgtable_alloc(PUD_SHIFT);
> > +
> > +		pudp = pud_set_fixmap(pud_phys);
> > +		init_clear_pgtable(pudp);
> > +
> >  		__p4d_populate(p4dp, pud_phys, p4dval);
> >  		p4d = READ_ONCE(*p4dp);
> > +	} else {
> > +		pudp = pud_set_fixmap(p4d_page_paddr(p4d));
> 
> With this change I end up in pgtable folding hell. pXX_set_fixmap() is defined
> as NULL when the level is folded (and pXX_page_paddr() is not defined at all).
> So it all compiles, but doesn't boot.

Sorry about that; I had not thought to check the folding logic when hacking
that up.

> I think the simplest approach is to follow this pattern:
> 
> ----8<----
> @@ -340,12 +338,15 @@ static void alloc_init_pud(p4d_t *p4dp, unsigned long
> addr, unsigned long end,
>                         p4dval |= P4D_TABLE_PXN;
>                 BUG_ON(!pgtable_alloc);
>                 pud_phys = pgtable_alloc(PUD_SHIFT);
> +               pudp = pud_set_fixmap(pud_phys);
> +               init_clear_pgtable(pudp);
> +               pudp += pud_index(addr);
>                 __p4d_populate(p4dp, pud_phys, p4dval);
> -               p4d = READ_ONCE(*p4dp);
> +       } else {
> +               BUG_ON(p4d_bad(p4d));
> +               pudp = pud_set_fixmap_offset(p4dp, addr);
>         }
> -       BUG_ON(p4d_bad(p4d));
> 
> -       pudp = pud_set_fixmap_offset(p4dp, addr);
>         do {
>                 pud_t old_pud = READ_ONCE(*pudp);
> ----8<----
> 
> For the map case, we continue to use pud_set_fixmap_offset() which is always
> defined (and always works correctly).
> 
> Note also that the previously unconditional BUG_ON needs to be prior to the
> fixmap call to be useful, and its really only valuable in the map case because
> for the alloc case we are the ones setting the p4d so we already know its not
> bad. This means we don't need the READ_ONCE() in the alloc case.
> 
> Shout if you disagree.

That looks good, and I agree with the reasoning here.

Thanks for working on this!

Mark.