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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to