[PATCH v2] arm64: mm: increase VA range of identity map

[catalin.marinas@arm.com (Catalin Marinas)]

On Wed, Feb 25, 2015 at 01:55:04PM +0000, Ard Biesheuvel wrote:
> On 25 February 2015 at 13:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 25 February 2015 at 12:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
> >> On Tue, Feb 24, 2015 at 05:08:23PM +0000, Ard Biesheuvel wrote:
> >>> --- a/arch/arm64/kernel/head.S
> >>> +++ b/arch/arm64/kernel/head.S
> >>> @@ -387,6 +387,28 @@ __create_page_tables:
> >>>       mov     x0, x25                         // idmap_pg_dir
> >>>       ldr     x3, =KERNEL_START
> >>>       add     x3, x3, x28                     // __pa(KERNEL_START)
> >>> +
> >>> +#ifndef CONFIG_ARM64_VA_BITS_48
> >>> +#define EXTRA_SHIFT  (PGDIR_SHIFT + PAGE_SHIFT - 3)
> >>> +     /*
> >>> +      * If VA_BITS < 48, it may be too small to allow for an ID mapping to be
> >>> +      * created that covers system RAM if that is located sufficiently high
> >>> +      * in the physical address space. So for the ID map, use an extended
> >>> +      * virtual range in that case, by configuring an additional translation
> >>> +      * level.
> >>> +      */
> >>> +     adrp    x5, KERNEL_END
> >>> +     clz     x5, x5                  // # of leading 0's in __pa(KERNEL_END)
> >>> +     cmp     x5, #TCR_T0SZ(VA_BITS)  // VA_BITS sufficiently large?
> >>
> >> I think we need some better comment here for people looking at this code
> >> again in the future (including myself). So the VA bits needed to cover
> >> KERNEL_END are calculated as (64 - clz(__pa(KERNEL_END))). T0SZ is
> >> calculated as (64 - VA_BITS) which means that T0SZ is
> >> clz(__pa(KERNEL_END)).
> >>
> >
> > OK.
> >
> >>> +     b.ge    1f                      // .. then skip additional level
> >>> +
> >>> +     adrp    x6, idmap_t0sz
> >>> +     str     x5, [x6, #:lo12:idmap_t0sz]
> >>> +
> >>> +     create_table_entry x0, x3, EXTRA_SHIFT, PTRS_PER_PGD, x5, x6
> >>
> >> EXTRA_SHIFT is correctly calculated (one level more than PGDIR_SHIFT)
> >> but I don't think PTRS_PER_PGD is appropriate here. For example, we had
> >> in the past 39-bit VA with 64KB pages which made PTRS_PER_PGD pretty
> >> small (well, it may cover the 40-bit case you are trying to fix but not
> >> up to 48). So maybe define something like:
> >>
> >> #define EXTRA_PTRS      (1 << (48 - EXTRA_SHIFT))
> >>
> >
> > My assumption was that, if you are running with fewer translation
> > levels than required to map all 48 bits, it makes little sense to use
> > only half a page or less at the top level, and PTRS_PER_PGD will
> > always cover a full page (which is what we reserve at the root level
> > anyway). But perhaps this is not universally true.
> 
> Actually, this assumption is encoded into the patch in another way as
> well: decreasing T0SZ is always assumed to result in an additional
> level of translation to be configured. However, your VA_BITS==39 on
> 64k pages case would break this assumption too. So perhaps using
> MAX_VA_BITS instead of the actual size of the PA is the right thing to
> do here after all.

We have two options:

a) enforce that for VA_BITS != 48, the PTRS_PER_PGD is always (1 <<
   (PAGE_SHIFT - 3)). This way we now that anything bigger than VA_BITS
   requires more levels (currently only one more)

b) decouple the idmap_t0sz calculation from the additional page table
   level so we can have different idmap_t0sz but not necessarily an
   extra level

I think to keep things simple, we should go for (a) with your original
MAX_VA_BITS when KERNEL_END cannot be covered by VA_BITS:

	64 - clz(__pa(KERNEL_END) > VA_BITS

You can still use PTRS_PER_PGD since we know it covers a full page but
we need an #error somewhere in case we forget about this restriction,
something like:

#if !defined(CONFIG_ARM64_VA_BITS_48) && \
	(8 * PTRS_PER_PGD != PAGE_SIZE)
#error "PGD not a full page size"
#endif

-- 
Catalin