| Message ID | 20221122165618.2453224-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: mm: Align PGDs to at least 64 bytes | expand |
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.
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!
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!
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>
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.
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
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.
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
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
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); }
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(-)