[PATCH] arm64: mm: Align PGDs to at least 64 bytes

[Anshuman Khandual]

On 11/24/22 13:12, Ard Biesheuvel wrote:
> On Thu, 24 Nov 2022 at 05:34, Anshuman Khandual
> <anshuman.khandual at arm.com> wrote:
>>
>>
>>
>> On 11/22/22 22:26, Ard Biesheuvel wrote:
>>> My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
>>> minimum alignment of root page tables as being conditional on whether
>>> 52-bit physical addressing is supported and enabled, even though I seem
>>> to remember that this was the case formerly (and our code suggests the
>>> same).
>> ARM ARM (DDI 0487G.a) says,
>>
>> "The minimum alignment of a translation table containing fewer than eight
>> entries is 64 bytes." But that does not seem to be conditional on 52-bit
>> physical address support, unless only the 52 bit physical address space
>> support could produce table configurations, which will have fewer than 8
>> entries in the PGD level.
>>
> 
> The architecture permits you to dimension the top level table freely,
> so this could happen with smaller PA sizes too.

Okay.

> 
>>>
>>> So align pgd_t[] allocations to 64 bytes. Note that this only affects
>>> 16k/4 levels configurations, which are unlikely to be in wide use.
>>
>> Instead of "16k/4 levels configurations", could we mention the in terms
>> of [PG_SIZE/VA_BITS/PA_BITS] format which can be easily related with
>> available config options.
>>
> 
> """
> With 16k granule and 48 bits of VA space, the root level table is only
> 16 bytes in size (two entries), and so aligning to PGD_SIZE is
> insufficient here.
> """

Sounds good.

> 
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>>> ---
>>>  arch/arm64/mm/pgd.c | 13 +++----------
>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
>>> index 4a64089e5771c1e2..8f01a75c35caaa9a 100644
>>> --- a/arch/arm64/mm/pgd.c
>>> +++ b/arch/arm64/mm/pgd.c
>>> @@ -40,17 +40,10 @@ void __init pgtable_cache_init(void)
>>>       if (PGD_SIZE == PAGE_SIZE)
>>>               return;
>>>
>>> -#ifdef CONFIG_ARM64_PA_BITS_52
>>>       /*
>>> -      * With 52-bit physical addresses, the architecture requires the
>>> -      * top-level table to be aligned to at least 64 bytes.
>>> +      * Naturally aligned pgds required by the architecture, with a minimum
>>> +      * alignment of 64 bytes.
>>>        */
>>> -     BUILD_BUG_ON(PGD_SIZE < 64);
>>> -#endif
>>> -
>>> -     /*
>>> -      * Naturally aligned pgds required by the architecture.
>>> -      */
>>> -     pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, PGD_SIZE,
>>> +     pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, max(PGD_SIZE, 64),
>>>                                     SLAB_PANIC, NULL);
>>
>> There is a build warning as follows, which can be fixed with typecasting
>> constant 64 with (size_t). While here, it would also make sense to define
>> a macro for PGD minimum alignment requirement i.e 64 bytes.
>>
> 
> Hmm, I didn't spot this.
> 
> Maybe something like
> 
> #define TTBR_MIN_ALIGN 64UL
> 
> in the pgtable-hwdef header should do the trick?

Right, makes sense. Please add this macro.

> 
>> diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
>> index 8f01a75c35ca..8d4b28d9590f 100644
>> --- a/arch/arm64/mm/pgd.c
>> +++ b/arch/arm64/mm/pgd.c
>> @@ -44,6 +44,6 @@ void __init pgtable_cache_init(void)
>>          * Naturally aligned pgds required by the architecture, with a minimum
>>          * alignment of 64 bytes.
>>          */
>> -       pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, max(PGD_SIZE, 64),
>> +       pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, max(PGD_SIZE, (size_t) 64),
>>                                       SLAB_PANIC, NULL);
>>  }
>>
>> In file included from ./include/linux/kernel.h:26,
>>                  from ./arch/arm64/include/asm/cpufeature.h:22,
>>                  from ./arch/arm64/include/asm/ptrace.h:11,
>>                  from ./arch/arm64/include/asm/irqflags.h:10,
>>                  from ./include/linux/irqflags.h:16,
>>                  from ./include/linux/spinlock.h:59,
>>                  from ./include/linux/mmzone.h:8,
>>                  from ./include/linux/gfp.h:7,
>>                  from ./include/linux/mm.h:7,
>>                  from arch/arm64/mm/pgd.c:9:
>> arch/arm64/mm/pgd.c: In function ‘pgtable_cache_init’:
>> ./include/linux/minmax.h:20:28: warning: comparison of distinct pointer types lacks a cast
>>    20 |  (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>>       |                            ^~
>> ./include/linux/minmax.h:26:4: note: in expansion of macro ‘__typecheck’
>>    26 |   (__typecheck(x, y) && __no_side_effects(x, y))
>>       |    ^~~~~~~~~~~
>> ./include/linux/minmax.h:36:24: note: in expansion of macro ‘__safe_cmp’
>>    36 |  __builtin_choose_expr(__safe_cmp(x, y), \
>>       |                        ^~~~~~~~~~
>> ./include/linux/minmax.h:52:19: note: in expansion of macro ‘__careful_cmp’
>>    52 | #define max(x, y) __careful_cmp(x, y, >)
>>       |                   ^~~~~~~~~~~~~
>> arch/arm64/mm/pgd.c:47:55: note: in expansion of macro ‘max’
>>    47 |  pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, max(PGD_SIZE, 64),
>>
>>>  }
>>
>> Besides, tested on config [16K|48VA|48PA] producing "CONFIG_PGTABLE_LEVELS=4"
>> and with PGD_SIZE != PAGE_SIZE, which booted successfully.
> 
> Thanks!