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...

> 
>> +    }
>> +
>> +    /* 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?
> 

The 4 is a random pick to give room for page tables that we need to split to 
make room for dcache attributes.

>> 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?

I've seen the comment and I think the problem was that MMU_SECTION_SHIFT 
indicated 2MB pages while the PTEs were actually on 512MB pages. That should be 
all solved now FWIW, yes. We now support real 2MB sections.


Alex

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to