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

[Ryan Roberts]

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.


> 
> 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);
>>> +}
>>
>