Re: [PATCH 2/2] arm64: mm: always map fixmap at page granularity

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Mar 15, 2023 at 10:23:31AM +0000, Ryan Roberts wrote:
> On 14/03/2023 14:21, Mark Rutland wrote:

> > diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> > index c153f069e9c9..c59d433c1801 100644
> > --- a/arch/arm64/include/asm/fixmap.h
> > +++ b/arch/arm64/include/asm/fixmap.h
> > @@ -36,17 +36,13 @@ enum fixed_addresses {
> >  	FIX_HOLE,
> >  
> >  	/*
> > -	 * Reserve a virtual window for the FDT that is 2 MB larger than the
> > -	 * maximum supported size, and put it at the top of the fixmap region.
> > -	 * The additional space ensures that any FDT that does not exceed
> > -	 * MAX_FDT_SIZE can be mapped regardless of whether it crosses any
> > -	 * 2 MB alignment boundaries.
> > -	 *
> > -	 * Keep this at the top so it remains 2 MB aligned.
> > +	 * Reserve a virtual window for the FDT that is a page bigger than the
> > +	 * maximum supported size. The additional space ensures that any FDT
> > +	 * that does not exceed MAX_FDT_SIZE can be mapped regardless of
> > +	 * whether it crosses any page boundary.
> >  	 */
> > -#define FIX_FDT_SIZE		(MAX_FDT_SIZE + SZ_2M)
> >  	FIX_FDT_END,
> > -	FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
> > +	FIX_FDT = FIX_FDT_END + MAX_FDT_SIZE / PAGE_SIZE - 1,
> 
> I don't think this is consistent with your comment - needs a +1 for the extra
> page? On a 4K system, FIX_FDT will be calculated as 512, but we need it to be
> 513 to deal with the case where the FDT is 2MB and not aligned to a page boundary.

Indeed; the intent was to have 513 in that case, but I clearly either forgot to
fix the calculation or messed that up when rebasing. I've fixed that to:

	FIX_FDT_END + DIV_ROUND_UP(MAX_FDT_SIZE, PAGE_SIZE) + 1,

... which'll work even if MAX_FDT_SIZE weren't a multiple of PAGE_SIZE.

[...]

> > diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c
> > index 54e50552bfe3..d7a6a485f361 100644
> > --- a/arch/arm64/mm/fixmap.c
> > +++ b/arch/arm64/mm/fixmap.c
> > @@ -16,34 +16,77 @@
> >  #include <asm/pgalloc.h>
> >  #include <asm/tlbflush.h>
> >  
> > -static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss;
> > +#define NR_BM_PTE_TABLES \
> > +	SPAN_NR_ENTRIES(FIXADDR_START, FIXADDR_TOP, PMD_SHIFT)
> > +#define NR_BM_PMD_TABLES \
> > +	SPAN_NR_ENTRIES(FIXADDR_START, FIXADDR_TOP, PUD_SHIFT)
> 
> One of the bear traps I stepped in last week when I was wrestling with this is
> that FIXADDR_START isn't actually the address of the first slot! It is
> __end_of_permanent_fixed_addresses. But you then have all the FIX_BTMAP slots,
> FIX_PTE/PMD/PUD/PGD before FIXADDR_START. So I think some refactoring is in
> order to properly cover all the slots.

Ugh, yes. This is a total mess.

> Honestly, the fact that the fixed_addresses enum is backwards is mind-bending
> too. 

That has been a long standing bugbear of mine, too.

> Any chance we can just define FIXADDR_START and FIXADDR_TOP to cover the
> whole lot, then calculate the slot address as (FIXADDR_START + (<ENUM> <<
> PAGE_SHIFT))?

Unfortunately, some code is (implicitly) relying on FIXADDR_START..FIXADDR_TOP
*not* including the FIX_BTMAP slots (e.g. virt_to_fix() and fix_to_virt() use
that to implicitly catch dodgy usage). Other code really wants FIXADDR_START to
be the base of the whole fixmap region (e.g. ptdump).

Cleaning that up is going to be a bit churny/painful. I'll take a look.

> (plus reorder of start/end enum labels for FDT and BTMAP). Or if
> we really care about reclaiming the virtual address space between
> __end_of_permanent_fixed_addresses and __end_of_fixed_addresses, create a new
> FIXADDR_BOOT_START or whatever.

I don't think we care about reclaiming the address space; it's a few pages at
best. I'll go take another look at how churny it'd be clean this up...

> > +
> > +static_assert(NR_BM_PMD_TABLES == 1);
> > +
> > +#define __BM_TABLE_IDX(addr, shift) \
> > +	(((addr) >> (shift)) - (FIXADDR_START >> (shift)))
> 
> Doesn't this run the risk of being negative for one of the slots below
> FIXADDR_START?

Yes; I had erroneously thought FIXADDR_START was below all of those, and the
intent was to use the base of the entire fixmap. So as above we'll need to do
some more preparatory cleanup...

> > +void __init early_fixmap_init_pte(pmd_t *pmdp, unsigned long addr)
> 
> static? - same comment for pmd/pud below.

Yup; done.

> > +{
> > +	pmd_t pmd = READ_ONCE(*pmdp);
> > +	pte_t *ptep;
> >  
> > -	return pud_offset_kimg(p4dp, addr);
> > +	if (pmd_none(pmd)) {
> > +		ptep = bm_pte[BM_PTE_TABLE_IDX(addr)];;
> 
> nit: remove one of the semi-colons.

Whoops; rebase error; done.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel