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

[Ryan Roberts]

On 21/03/2023 14:18, Mark Rutland wrote:
> 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).

So perhaps one approach would be to have 2 sets of macros:

#define ARCH_FIXADDR_START
#define ARCH_FIXADDR_TOP
#define FIXADDR_START
#define FIXADDR_TOP

static_assert(ARCH_FIXADDR_START <= FIXADDR_START);
static_assert(ARCH_FIXADDR_TOP >= FIXADDR_TOP);

ARCH_FIXADDR_* covers the entire region exactly. And FIXADDR_* describes the
sub-region that the generic code cares about. Then all the fixmap internals can
be done against ARCH_FIXADDR_* and the generic code can do its checks against
FIXADDR_* ?

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