From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 22 Feb 2016 11:45:14 -0700 Subject: [U-Boot] [PATCH 2/9] arm64: Make full va map code more dynamic In-Reply-To: References: <1456106232-233210-1-git-send-email-agraf@suse.de> <1456106232-233210-3-git-send-email-agraf@suse.de> <56CB50F1.6060508@wwwdotorg.org> Message-ID: <56CB573A.2010304@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/22/2016 11:37 AM, Alexander Graf wrote: > > On Feb 22, 2016, at 7:18 PM, Stephen Warren wrote: > >> 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"). > > Oops, yes. I split the pte setting into a separate function and forgot to update the comment. Nice catch. > >> >>> +/* 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. > > Yes, or - if your va_bits < 39 it actually accounts for level 1 because the page table starts@Lv1. > >> >>> + 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). > > I definitely should use the level2shift helper here, yes. > >> >>> + /* 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). > > The reason I didn't account for level 3 is that *usually* we shouldn't have those around. I guess the size estimation really could use some more thought... As you mention, it's unlikely we'd need level 3 in practice. However, we should still either account for it now, or explicitly fail if the code for that isn't written yet. I'd like at least an assert()/panic()/... somewhere (here seems best?) if the mem_map[] entries are not 2MB-aligned.