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

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

[Ard Biesheuvel]

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).

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.

Signed-off-by: Ard Biesheuvel <ardb@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);
 }
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anshuman Khandual]

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.

> 
> 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.

> 
> Signed-off-by: Ard Biesheuvel <ardb@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.

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Thu, 24 Nov 2022 at 05:34, Anshuman Khandual
<anshuman.khandual@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.

> >
> > 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.
"""

> >
> > Signed-off-by: Ard Biesheuvel <ardb@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?

> 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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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@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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Catalin Marinas]

On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte

G.a is nearly two years old. You may want to upgrade to H.a ;).

> 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).

The wording in the ARM ARM implies that it's only needed if we go beyond
48 bits for the base address:

  A translation table must be aligned to the size of the table, except
  that when using a translation table base address larger than 48 bits
  the minimum alignment of a table containing fewer than eight entries
  is 64 bytes.

But I'm fine with the patch, always forcing the 64 byte alignment. With
the 'max_t' instead of 'max' (or whatever solves Anshuman's error):

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Mon, 28 Nov 2022 at 18:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> > My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
>
> G.a is nearly two years old. You may want to upgrade to H.a ;).
>
> > 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).
>
> The wording in the ARM ARM implies that it's only needed if we go beyond
> 48 bits for the base address:
>
>   A translation table must be aligned to the size of the table, except
>   that when using a translation table base address larger than 48 bits
>   the minimum alignment of a table containing fewer than eight entries
>   is 64 bytes.
>
> But I'm fine with the patch, always forcing the 64 byte alignment. With
> the 'max_t' instead of 'max' (or whatever solves Anshuman's error):
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Right, so it appears they backpedaled on that.

The distinction is kind of important depending on whether we want to
fall back to 47 or 48 bits of VA space on on 16k granule configs with
LPA2 running on systems that lack LPA2 support.

If we want to fall back to 48 bits, TTBR1 must be incremented to point
to the entries describing the 48-bit VA space inside a table
dimensioned for 52 bits, and in this case, those entries can simply
not appear on a 64 byte aligned boundary.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

On Mon, Nov 28, 2022 at 05:50:48PM +0000, Catalin Marinas wrote:
> On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> > My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
> 
> G.a is nearly two years old. You may want to upgrade to H.a ;).

H.a is over eight months old. You may want to upgrade to I.a :p

(Actually, don't bother -- it's written using these unreadable rule things.
H.a, all is forgiven).

> > 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).
> 
> The wording in the ARM ARM implies that it's only needed if we go beyond
> 48 bits for the base address:
> 
>   A translation table must be aligned to the size of the table, except
>   that when using a translation table base address larger than 48 bits
>   the minimum alignment of a table containing fewer than eight entries
>   is 64 bytes.

FWIW, this wording is the same in I.a.

> But I'm fine with the patch, always forcing the 64 byte alignment. With
> the 'max_t' instead of 'max' (or whatever solves Anshuman's error):
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Happy to take a v2 or add the max_t() to this version.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Tue, 29 Nov 2022 at 10:51, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Nov 28, 2022 at 05:50:48PM +0000, Catalin Marinas wrote:
> > On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> > > My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
> >
> > G.a is nearly two years old. You may want to upgrade to H.a ;).
>
> H.a is over eight months old. You may want to upgrade to I.a :p
>
> (Actually, don't bother -- it's written using these unreadable rule things.
> H.a, all is forgiven).
>
> > > 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).
> >
> > The wording in the ARM ARM implies that it's only needed if we go beyond
> > 48 bits for the base address:
> >
> >   A translation table must be aligned to the size of the table, except
> >   that when using a translation table base address larger than 48 bits
> >   the minimum alignment of a table containing fewer than eight entries
> >   is 64 bytes.
>
> FWIW, this wording is the same in I.a.
>
> > But I'm fine with the patch, always forcing the 64 byte alignment. With
> > the 'max_t' instead of 'max' (or whatever solves Anshuman's error):
> >
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Happy to take a v2 or add the max_t() to this version.
>

In spite of the off-list discussion we just had where we concluded
that this patch is not necessary, I think we still do:
in revision I.a, I still see the following wording

D17.2.147 TTBR1_EL1, Translation Table Base Register 1 (EL1)

------- Note --------
 A translation table is required to be aligned to the size of the
table. If a table contains fewer than
eight entries, it must be aligned on a 64 byte address boundary.


with no mention whatsoever regarding this requirement being
conditional on the configured PA range.

I'll respin and resend.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

On Tue, Nov 29, 2022 at 12:18:20PM +0100, Ard Biesheuvel wrote:
> On Tue, 29 Nov 2022 at 10:51, Will Deacon <will@kernel.org> wrote:
> >
> > On Mon, Nov 28, 2022 at 05:50:48PM +0000, Catalin Marinas wrote:
> > > On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> > > > My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
> > >
> > > G.a is nearly two years old. You may want to upgrade to H.a ;).
> >
> > H.a is over eight months old. You may want to upgrade to I.a :p
> >
> > (Actually, don't bother -- it's written using these unreadable rule things.
> > H.a, all is forgiven).
> >
> > > > 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).
> > >
> > > The wording in the ARM ARM implies that it's only needed if we go beyond
> > > 48 bits for the base address:
> > >
> > >   A translation table must be aligned to the size of the table, except
> > >   that when using a translation table base address larger than 48 bits
> > >   the minimum alignment of a table containing fewer than eight entries
> > >   is 64 bytes.
> >
> > FWIW, this wording is the same in I.a.
> >
> > > But I'm fine with the patch, always forcing the 64 byte alignment. With
> > > the 'max_t' instead of 'max' (or whatever solves Anshuman's error):
> > >
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >
> > Happy to take a v2 or add the max_t() to this version.
> >
> 
> In spite of the off-list discussion we just had where we concluded
> that this patch is not necessary, I think we still do:
> in revision I.a, I still see the following wording
> 
> D17.2.147 TTBR1_EL1, Translation Table Base Register 1 (EL1)
> 
> ------- Note --------
>  A translation table is required to be aligned to the size of the
> table. If a table contains fewer than
> eight entries, it must be aligned on a 64 byte address boundary.
> 
> 
> with no mention whatsoever regarding this requirement being
> conditional on the configured PA range.

Ha, so the text is different between stage-1 (e.g. TTBRx_EL1) and stage-2
(e.g. VTTBR_EL2)! I wonder if that's deliberate? Maybe something to do with
coalescing? :/

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On Tue, 29 Nov 2022 12:23:00 +0000,
Will Deacon <will@kernel.org> wrote:
> 
> On Tue, Nov 29, 2022 at 12:18:20PM +0100, Ard Biesheuvel wrote:
> > On Tue, 29 Nov 2022 at 10:51, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Mon, Nov 28, 2022 at 05:50:48PM +0000, Catalin Marinas wrote:
> > > > On Tue, Nov 22, 2022 at 05:56:18PM +0100, Ard Biesheuvel wrote:
> > > > > My copy of the ARM ARM (DDI 0487G.a) no longer describes the 64 byte
> > > >
> > > > G.a is nearly two years old. You may want to upgrade to H.a ;).
> > >
> > > H.a is over eight months old. You may want to upgrade to I.a :p
> > >
> > > (Actually, don't bother -- it's written using these unreadable rule things.
> > > H.a, all is forgiven).
> > >
> > > > > 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).
> > > >
> > > > The wording in the ARM ARM implies that it's only needed if we go beyond
> > > > 48 bits for the base address:
> > > >
> > > >   A translation table must be aligned to the size of the table, except
> > > >   that when using a translation table base address larger than 48 bits
> > > >   the minimum alignment of a table containing fewer than eight entries
> > > >   is 64 bytes.
> > >
> > > FWIW, this wording is the same in I.a.
> > >
> > > > But I'm fine with the patch, always forcing the 64 byte alignment. With
> > > > the 'max_t' instead of 'max' (or whatever solves Anshuman's error):
> > > >
> > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > >
> > > Happy to take a v2 or add the max_t() to this version.
> > >
> > 
> > In spite of the off-list discussion we just had where we concluded
> > that this patch is not necessary, I think we still do:
> > in revision I.a, I still see the following wording
> > 
> > D17.2.147 TTBR1_EL1, Translation Table Base Register 1 (EL1)
> > 
> > ------- Note --------
> >  A translation table is required to be aligned to the size of the
> > table. If a table contains fewer than
> > eight entries, it must be aligned on a 64 byte address boundary.
> > 
> > 
> > with no mention whatsoever regarding this requirement being
> > conditional on the configured PA range.
> 
> Ha, so the text is different between stage-1 (e.g. TTBRx_EL1) and stage-2
> (e.g. VTTBR_EL2)! I wonder if that's deliberate? Maybe something to do with
> coalescing? :/

I think the whole VTTBR_EL2.BADDR section is full of crap, and has
been for a long time (since 0487B.a). It talks about S1 translation
all over the shop, and feels like a copy-paste gone horribly wrong...

Coalescing actually forces a stronger alignment, as you have to align
on the full size of the top-level table.

	M. /puzzled

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel