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

[Mark Rutland]

On Tue, Mar 21, 2023 at 02:34:15PM +0000, Ryan Roberts wrote:
> 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/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_* ?

FWIW, x86 has:

| #define FIXADDR_SIZE            (__end_of_permanent_fixed_addresses << PAGE_SHIFT)
| #define FIXADDR_START           (FIXADDR_TOP - FIXADDR_SIZE)
| #define FIXADDR_TOT_SIZE        (__end_of_fixed_addresses << PAGE_SHIFT)
| #define FIXADDR_TOT_START       (FIXADDR_TOP - FIXADDR_TOT_SIZE)

... so I suspect we should do the same, in the hope that can (eventually) be
unified.

Thanks,
Mark.