Re: [PATCH] arm64: mm: Validate CONFIG_PGTABLE_LEVELS conditionally
From: Gavin Shan
Date: Wed Oct 18 2023 - 02:34:15 EST
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@xxxxxxxxxx>
---
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