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

[Ryan Roberts]

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.

Thanks,
Ryan


> 
> Thanks,
> Ryan
> 
>> +	dsb(ishst);
>> +	set_ttbr0_for_lpa2((u64)init_pg_dir);
>> +
>> +	/*
>> +	 * Recreate the initial ID map with the same granularity as before.
>> +	 * Don't bother with the FDT, we no longer need it after this.
>> +	 */
>> +	memset(init_idmap_pg_dir, 0,
>> +	       (u64)init_idmap_pg_dir - (u64)init_idmap_pg_end);
>> +
>> +	create_init_idmap(init_idmap_pg_dir, mask);
>> +	dsb(ishst);
>> +
>> +	/* switch back to the updated initial ID map */
>> +	set_ttbr0_for_lpa2((u64)init_idmap_pg_dir);
>> +
>> +	/* wipe the temporary ID map from memory */
>> +	memset(init_pg_dir, 0, (u64)init_pg_end - (u64)init_pg_dir);
>> +}
>