On 02/21/2016 06:57 PM, Alexander Graf wrote:
The idea to generate our pages tables from an array of memory ranges is very sound. However, instead of hard coding the code to create up to 2 levels of 64k granule page tables, we really should just create normal 4k page tables that allow us to set caching attributes on 2M or 4k level later on.
So this patch moves the full_va mapping code to 4k page size and makes it fully flexible to dynamically create as many levels as necessary for a map (including dynamic 1G/2M pages). It also adds support to dynamically split a large map into smaller ones when some code wants to set dcache attributes.
With all this in place, there is very little reason to create your own page tables in board specific files.
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
+/* Creates a new full table (512 entries) and sets *pte to refer to it */ +static u64 *create_table(void)
I think that comment is stale (there's no *pte; I assume it should say "returns").
+/* Returns the estimated required size of all page tables */ +u64 get_page_table_size(void) +{
- int i;
- u64 one_pt = MAX_PTE_ENTRIES * sizeof(u64);
- u64 size = 0;
- /* root page table */
- size += one_pt;
Isn't the root page table level 0? So, that accounts for the page table that's indexed by VA bits 47:39.
- for (i = 0; i < ARRAY_SIZE(mem_map); i++) {
struct mm_region *map = &mem_map[i];/* Account for Lv0 page tables */size += one_pt * ((map->size >> 39) + 1);
So here, isn't the code accounting for level 1 tables, not level 0 tables? That is, the tables indexed by VA bits 38:30.
(As an aside for myself when I come back and read this later, the shift is 39, since we need to allocate as many tables as there are values for bits 39 and above).
/* 1GB aligned pages fit already, so count the others */if (map->size & 0x3fffffffULL)size += one_pt;if (map->base & 0x3fffffffULL)size += one_pt;
Here, I believe we're accounting for any required level 2 tables (which are indexed by VA bits 29:21).
We seem to be missing code inside the loop that accounts for any required level 3 tables (indexed by VA bits 20:12).
- }
- /* Assume we may have to split up to 4 more page tables off */
- size += one_pt * 4;
Is this meant to be accounting for level 3 tables? If so, I'm not sure where the value "4" comes from. I would have expected "2" instead (for misaligned start/end) *and* for this calculation to be inside the loop rather than outside. Doesn't putting the calculation outside the loop make assumptions about how many mem_map[] entries are mis-aligned relative to 2MB sections?
diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
#define VA_BITS (42) /* 42 bits virtual address */ #else #define VA_BITS CONFIG_SYS_VA_BITS -#define PTL2_BITS CONFIG_SYS_PTL2_BITS +#define PTE_BLOCK_BITS CONFIG_SYS_PTL2_BITS #endif
When I "wrote" the Tegra ARMv8 MMU code (i.e. cut/pasted it from other ARMv8 MMU code), I recall finding some inconsistencies between the value of VA_BITS and *SECTION_SHIFT between different header files. I think that was e.g.:
arch/arm/include/asm/armv8/mmu.h:26:#define VA_BITS (42) /* 42 bits virtual address */ arch/arm/mach-tegra/arm64-mmu.c:25:#define TEGRA_VA_BITS 40 arch/arm/cpu/armv8/zynqmp/cpu.c:59:#define ZYNQMO_VA_BITS 40
asm/armv8/mmu.h's value for SECTION_SHIFT and asm/system.h's value for MMU_SECTION_SHIFT.
(Also see the related parts of the description of git commit 376cb1a45315 "ARM: tegra: add custom MMU setup on ARMv8")
Does this patch entirely address those discrepancies?