On 22.02.16 19:45, Stephen Warren wrote: > On 02/22/2016 11:37 AM, Alexander Graf wrote: >> >> On Feb 22, 2016, at 7:18 PM, Stephen Warren <swar...@wwwdotorg.org> >> 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 at 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.
I've reworked the code to properly account for 4k pages. I guess that also solves it? :) Alex _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot