[PATCH v8 33/43] arm64: Enable LPA2 at boot if supported by the system

Tue Aug 27 02:03:03 PDT 2024

On Wed, 7 Aug 2024 at 23:41, Ryan Roberts <ryan.roberts at arm.com> wrote:
>
> On 07/08/2024 09:46, Ryan Roberts wrote:
> > On 06/08/2024 17:16, Ryan Roberts wrote:
> >> Hi Ard,
> >>
> >> Sorry to drag up this old thread; I'm doing some work in this space and am
> >> having a tough time convincing myself of the safety - see below...
> >>
> >> On 14/02/2024 12:29, Ard Biesheuvel wrote:
> >>> From: Ard Biesheuvel <ardb at kernel.org>
> >>>
> >>> Update the early kernel mapping code to take 52-bit virtual addressing
> >>> into account based on the LPA2 feature. This is a bit more involved than
> >>> LVA (which is supported with 64k pages only), given that some page table
> >>> descriptor bits change meaning in this case.
> >>>
> >>> To keep the handling in asm to a minimum, the initial ID map is still
> >>> created with 48-bit virtual addressing, which implies that the kernel
> >>> image must be loaded into 48-bit addressable physical memory. This is
> >>> currently required by the boot protocol, even though we happen to
> >>> support placement outside of that for LVA/64k based configurations.
> >>>
> >>> Enabling LPA2 involves more than setting TCR.T1SZ to a lower value,
> >>> there is also a DS bit in TCR that needs to be set, and which changes
> >>> the meaning of bits [9:8] in all page table descriptors. Since we cannot
> >>> enable DS and every live page table descriptor at the same time, let's
> >>> pivot through another temporary mapping. This avoids the need to
> >>> reintroduce manipulations of the page tables with the MMU and caches
> >>> disabled.
> >>>
> >>> To permit the LPA2 feature to be overridden on the kernel command line,
> >>> which may be necessary to work around silicon errata, or to deal with
> >>> mismatched features on heterogeneous SoC designs, test for CPU feature
> >>> overrides first, and only then enable LPA2.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> >>
> >> [...]
> >>
> >>> +static void __init remap_idmap_for_lpa2(void)
> >>> +{
> >>> +   /* clear the bits that change meaning once LPA2 is turned on */
> >>> +   pteval_t mask = PTE_SHARED;
> >>> +
> >>> +   /*
> >>> +    * We have to clear bits [9:8] in all block or page descriptors in the
> >>> +    * initial ID map, as otherwise they will be (mis)interpreted as
> >>> +    * physical address bits once we flick the LPA2 switch (TCR.DS). Since
> >>> +    * we cannot manipulate live descriptors in that way without creating
> >>> +    * potential TLB conflicts, let's create another temporary ID map in a
> >>> +    * LPA2 compatible fashion, and update the initial ID map while running
> >>> +    * from that.
> >>> +    */
> >>> +   create_init_idmap(init_pg_dir, mask);
> >>
> >> Given the init_idmap always uses 48 bit VA, and the swapper VA size is
> >> determined through Kconfig and may be smaller than 48 bit, how can you be
> >> certain that init_pg_dir is big enough to hold the init idmap? Surely swapper
> >> may use fewer levels and therefore be sized for fewer pages?
> >>
> >> I wonder if its possible that we end up running into the early_init_stack then
> >> off the end of the BSS?
> >
> > I'm bad at maths so decided to test this impirically by compiling the macros up
> > into a test program and spitting out the values for all supported combinations
> > of page size and va bits:
> >
> > PAGE_SHIFT=12:
> >   VA_BITS=52:
> >     INIT_DIR_SIZE=53248
> >     INIT_IDMAP_DIR_SIZE=32768
> >     INIT_IDMAP_FDT_SIZE=24576
> >   VA_BITS=48:
> >     INIT_DIR_SIZE=45056
> >     INIT_IDMAP_DIR_SIZE=32768
> >     INIT_IDMAP_FDT_SIZE=24576
> >   VA_BITS=39:
> >     INIT_DIR_SIZE=36864
> >     INIT_IDMAP_DIR_SIZE=32768
> >     INIT_IDMAP_FDT_SIZE=24576
> > PAGE_SHIFT=14:
> >   VA_BITS=52:
> >     INIT_DIR_SIZE=131072
> >     INIT_IDMAP_DIR_SIZE=131072
> >     INIT_IDMAP_FDT_SIZE=98304
> >   VA_BITS=48:
> >     INIT_DIR_SIZE=131072
> >     INIT_IDMAP_DIR_SIZE=131072
> >     INIT_IDMAP_FDT_SIZE=98304
> >   VA_BITS=47:
> >     INIT_DIR_SIZE=98304      <<< TOO SMALL!
> >     INIT_IDMAP_DIR_SIZE=131072
> >     INIT_IDMAP_FDT_SIZE=98304
> >   VA_BITS=36:
> >     INIT_DIR_SIZE=65536      <<< TOO SMALL!
> >     INIT_IDMAP_DIR_SIZE=131072
> >     INIT_IDMAP_FDT_SIZE=98304
> > PAGE_SHIFT=16:
> >   VA_BITS=52:
> >     INIT_DIR_SIZE=327680
> >     INIT_IDMAP_DIR_SIZE=327680
> >     INIT_IDMAP_FDT_SIZE=262144
> >   VA_BITS=48:
> >     INIT_DIR_SIZE=327680
> >     INIT_IDMAP_DIR_SIZE=327680
> >     INIT_IDMAP_FDT_SIZE=262144
> >   VA_BITS=42:
> >     INIT_DIR_SIZE=196608     <<< TOO SMALL!
> >     INIT_IDMAP_DIR_SIZE=327680
> >     INIT_IDMAP_FDT_SIZE=262144
> >
> > There are 3 configurations where the space allocated in BSS for the init_pg_dir
> > is smaller than the space required for the init_idmap_pg_dir. So I think there
> > is definitely a problem here?
> >
> > As I said, I'm doing work in this area at the moment, so propose to send some
> > patches to fix this by ensuring the space allocated for init_pg_dir is
> > MAX(INIT_DIR_SIZE, INIT_IDMAP_DIR_SIZE). I'm also going to track the limit of
> > the buffer that is being allocated from so we can runtime check we don't
> > overflow. Shout if you disagree.
>
> There's no bug here; We only take this path if LPA2 is enabled, and LPA2 is only
> enabled if 52-bit VA is configured. So in that case, init_pg_dir must be at
> least as big as init_idmap_pg_dir and can definitely hold the 48 bit VA init idmap.
>
> Sorry for the noise.
>

Not at all - thanks for going down this rabbit hole and confirming
that the allocation is guaranteed to be of sufficient size. A
1/2-liner summary of this added as a comment in the appropriate place
would be highly appreciated as a follow-up patch to this.

Thanks,
Ard.