Messages in this thread Patch in this message | | | Date | Wed, 18 Oct 2023 16:33:09 +1000 | Subject | Re: [PATCH] arm64: mm: Validate CONFIG_PGTABLE_LEVELS conditionally | From | Gavin Shan <> |
| |
Hi Mark,
On 10/17/23 21:05, Mark Rutland wrote: > On Tue, Oct 17, 2023 at 10:53:00AM +1000, Gavin Shan wrote: >> It's allowed for the fixmap virtual address space to span multiple >> PMD entries. Instead, the address space isn't allowed to span multiple >> PUD entries. However, PMD entries are folded to PUD and PGD entries >> in the following combination. In this particular case, the validation >> on NR_BM_PMD_TABLES should be avoided. >> >> CONFIG_ARM64_PAGE_SHIFT = 14 >> CONFIG_ARM64_VA_BITS_36 = y >> CONFIG_PGTABLE_LEVELS = 2 > > Is this something you found by inspection, or are you hitting a real issue on a > particular config? > > I built a kernel with: > > defconfig + CONFIG_ARM64_16K_PAGES=y + CONFIG_ARM64_VA_BITS_36=y > > ... which gives the CONFIG_* configuration you list above, and that works just > fine. > > For 2-level 16K pages we'd need to reserve more than 32M of fixmap slots for > the assertion to fire, and we only reserve ~6M of slots in total today, so I > can't see how this would be a problem unless you have 26M+ of local additions > to the fixmap? > > Regardless of that, I don't think it's right to elide the check entirely. >
It's all about code inspection. When CONFIG_PGTABLE_LEVELS == 2, PGD/PUD/PMD are equivalent. The following two macros are interchangeable. The forthcoming static_assert() enforces that the fixmap virtual space can't span multiple PMD entries, meaning the space is limited to 32MB with above configuration.
#define NR_BM_PMD_TABLES \ SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PUD_SHIFT) #define NR_BM_PMD_TABLES \ SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT)
static_assert(NR_BM_PMD_TABLES == 1);
However, multiple PTE tables are allowed. It means the fixmap virtual space can span multiple PMD entries, which is controversial to the above enforcement from the code level. Hopefully, I understood everything correctly.
#define NR_BM_PTE_TABLES \ SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT) static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss;
You're correct that the following edition is needed to trigger the assert. The point is to have the fixmap virtual space larger than 32MB.
enum fixed_addresses { FIX_HOLE, : FIX_PTE, FIX_PMD, FIX_PUD, FIX_PGD, FIX_DUMMY = FIX_PGD + 2048,
__end_of_fixed_addresses };
> The point of the check is to make sure that the fixmap VA range doesn't span > across multiple PMD/PUD/P4D/PGD entries, as the early_fixmap_init() and > fixmap_copy() code don't handle that in general. When using 2-level 16K pages, > we still want to ensure the fixmap is contained within a single PGD, and > checking that it falls within a single folded PMD will check that. > > See the message for commit: > > 414c109bdf496195 ("arm64: mm: always map fixmap at page granularity") > > ... and the bits that deleted from early_fixmap_init(). > > AFAICT this is fine as-is. >
As I can see, multiple PMD entries can be handled well in early_fixmap_init(). However, multiple PMD entries aren't handled in fixmap_copy(), as you said.
early_fixmap_init early_fixmap_init_pud early_fixmap_init_pmd // multiple PMD entries handled in the loop
Thanks, Gavin
>> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> arch/arm64/mm/fixmap.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c >> index c0a3301203bd..5384e5c3aeaa 100644 >> --- a/arch/arm64/mm/fixmap.c >> +++ b/arch/arm64/mm/fixmap.c >> @@ -18,10 +18,11 @@ >> >> #define NR_BM_PTE_TABLES \ >> SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT) >> +#if CONFIG_PGTABLE_LEVELS > 2 >> #define NR_BM_PMD_TABLES \ >> SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PUD_SHIFT) >> - >> static_assert(NR_BM_PMD_TABLES == 1); >> +#endif >> >> #define __BM_TABLE_IDX(addr, shift) \ >> (((addr) >> (shift)) - (FIXADDR_TOT_START >> (shift))) >> -- >> 2.41.0 >> >
| |