On 23.02.16 14:17, Simon Glass wrote:
> Hi Alex,
>
> On 21 February 2016 at 18:57, Alexander Graf <ag...@suse.de> 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.
>>
>> Signed-off-by: Alexander Graf <ag...@suse.de>
>> ---
>> arch/arm/cpu/armv8/cache_v8.c | 346
>> +++++++++++++++++++++++++++++++------
>> arch/arm/include/asm/armv8/mmu.h | 68 ++++----
>> arch/arm/include/asm/global_data.h | 4 +-
>> arch/arm/include/asm/system.h | 3 +-
>> include/configs/thunderx_88xx.h | 14 +-
>> 5 files changed, 332 insertions(+), 103 deletions(-)
>>
>
> Should the change to the thunderx file go in a separate patch?
We're changing semantics for some defines from "this define acts in
L1/L2 page table entries" to "this define is for level/block type PTEs".
So it's tied to this patch :).
>
>> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
>> index 9229532..4369a83 100644
>> --- a/arch/arm/cpu/armv8/cache_v8.c
>> +++ b/arch/arm/cpu/armv8/cache_v8.c
>> @@ -2,6 +2,9 @@
>> * (C) Copyright 2013
>> * David Feng <feng...@phytium.com.cn>
>> *
>> + * (C) Copyright 2016
>> + * Alexander Graf <ag...@suse.de>
>> + *
>> * SPDX-License-Identifier: GPL-2.0+
>> */
>>
>> @@ -9,35 +12,40 @@
>> #include <asm/system.h>
>> #include <asm/armv8/mmu.h>
>>
>> -DECLARE_GLOBAL_DATA_PTR;
>> -
>> -#ifndef CONFIG_SYS_DCACHE_OFF
>> +/* #define DEBUG_MMU */
>>
>> -#ifdef CONFIG_SYS_FULL_VA
>> -static void set_ptl1_entry(u64 index, u64 ptl2_entry)
>> -{
>> - u64 *pgd = (u64 *)gd->arch.tlb_addr;
>> - u64 value;
>> +#ifdef DEBUG_MMU
>> +#define DPRINTF(a, ...) printf("%s:%d: " a, __func__, __LINE__, __VA_ARGS__)
>> +#else
>> +#define DPRINTF(a, ...) do { } while(0)
>> +#endif
>
> Can you use the normal DEBUG and debug()?
Uh, I guess so, yeah.
>
>>
>> - value = ptl2_entry | PTL1_TYPE_TABLE;
>> - pgd[index] = value;
>> -}
>> +DECLARE_GLOBAL_DATA_PTR;
>>
>> -static void set_ptl2_block(u64 ptl1, u64 bfn, u64 address, u64 memory_attrs)
>> -{
>> - u64 *pmd = (u64 *)ptl1;
>> - u64 value;
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>>
>> - value = address | PTL2_TYPE_BLOCK | PTL2_BLOCK_AF;
>> - value |= memory_attrs;
>> - pmd[bfn] = value;
>> -}
>> +/*
>> + * With 4k page granule, a virtual address is split into 4 lookup parts
>> + * spanning 9 bits each:
>> + *
>> + * _______________________________________________
>> + * | | | | | | |
>> + * | 0 | Lv0 | Lv1 | Lv2 | Lv3 | off |
>> + * |_______|_______|_______|_______|_______|_______|
>> + * 63-48 47-39 38-30 29-21 20-12 11-00
>> + *
>> + * mask page size
>> + *
>> + * Lv0: FF8000000000 --
>> + * Lv1: 7FC0000000 1G
>> + * Lv2: 3FE00000 2M
>> + * Lv3: 1FF000 4K
>> + * off: FFF
>> + */
>>
>> +#ifdef CONFIG_SYS_FULL_VA
>> static struct mm_region mem_map[] = CONFIG_SYS_MEM_MAP;
>
> I am not ken on the idea of using a big #define table on these boards.
> Is there not a device-tree binding for this that we can use? It is
> just a data table, We are moving to Kconfig and eventually want to
> drop the config files.
I'll move this into board files which then can do whatever they like
with it - take if from a #define in a header, populate it dynamically
from device tree, do something halfway in between, whatever fits your
needs best :).
Btw, if you want to use dt for it, you don't need to add any new
bindings at all. Simply take all of your device regs/ranges and memory
ranges and gobble them into the memory map.
>
>>
>> -#define PTL1_ENTRIES CONFIG_SYS_PTL1_ENTRIES
>> -#define PTL2_ENTRIES CONFIG_SYS_PTL2_ENTRIES
>> -
>> static u64 get_tcr(int el, u64 *pips, u64 *pva_bits)
>> {
>> u64 max_addr = 0;
>> @@ -79,8 +87,8 @@ static u64 get_tcr(int el, u64 *pips, u64 *pva_bits)
>> }
>>
>> /* PTWs cacheable, inner/outer WBWA and inner shareable */
>> - tcr |= TCR_TG0_64K | TCR_SHARED_INNER | TCR_ORGN_WBWA |
>> TCR_IRGN_WBWA;
>> - tcr |= TCR_T0SZ(VA_BITS);
>> + tcr |= TCR_TG0_4K | TCR_SHARED_INNER | TCR_ORGN_WBWA | TCR_IRGN_WBWA;
>> + tcr |= TCR_T0SZ(va_bits);
>>
>> if (pips)
>> *pips = ips;
>> @@ -90,39 +98,196 @@ static u64 get_tcr(int el, u64 *pips, u64 *pva_bits)
>> return tcr;
>> }
>>
>> -static void setup_pgtables(void)
>> +#define MAX_PTE_ENTRIES 512
>> +
>> +static int pte_type(u64 *pte)
>> +{
>> + return *pte & PTE_TYPE_MASK;
>> +}
>> +
>> +/* Returns the LSB number for a PTE on level <level> */
>> +static int level2shift(int level)
>> {
>> - int l1_e, l2_e;
>> - unsigned long pmd = 0;
>> - unsigned long address;
>> -
>> - /* Setup the PMD pointers */
>> - for (l1_e = 0; l1_e < CONFIG_SYS_MEM_MAP_SIZE; l1_e++) {
>> - gd->arch.pmd_addr[l1_e] = gd->arch.tlb_addr +
>> - PTL1_ENTRIES * sizeof(u64);
>> - gd->arch.pmd_addr[l1_e] += PTL2_ENTRIES * sizeof(u64) * l1_e;
>> - gd->arch.pmd_addr[l1_e] = ALIGN(gd->arch.pmd_addr[l1_e],
>> - 0x10000UL);
>> + /* Page is 12 bits wide, every level translates 9 bits */
>> + return (12 + 9 * (3 - level));
>> +}
>> +
>> +static u64 *find_pte(u64 addr, int level)
>> +{
>> + int start_level = 0;
>> + u64 *pte;
>> + u64 idx;
>> + u64 va_bits;
>> + int i;
>> +
>> + DPRINTF("addr=%llx level=%d\n", addr, level);
>> +
>> + get_tcr(0, NULL, &va_bits);
>> + if (va_bits < 39)
>> + start_level = 1;
>> +
>> + if (level < start_level)
>> + return NULL;
>> +
>> + /* Walk through all page table levels to find our PTE */
>> + pte = (u64*)gd->arch.tlb_addr;
>> + for (i = start_level; i < 4; i++) {
>> + idx = (addr >> level2shift(i)) & 0x1FF;
>> + pte += idx;
>> + DPRINTF("idx=%llx PTE %p at level %d: %llx\n", idx, pte, i,
>> *pte);
>> +
>> + /* Found it */
>> + if (i == level)
>> + return pte;
>> + /* PTE is no table (either invalid or block), can't traverse
>> */
>> + if (pte_type(pte) != PTE_TYPE_TABLE)
>> + return NULL;
>> + /* Off to the next level */
>> + pte = (u64*)(*pte & 0x0000fffffffff000ULL);
>> }
>>
>> - /* Setup the page tables */
>> - for (l1_e = 0; l1_e < PTL1_ENTRIES; l1_e++) {
>> - if (mem_map[pmd].base ==
>> - (uintptr_t)l1_e << PTL2_BITS) {
>> - set_ptl1_entry(l1_e, gd->arch.pmd_addr[pmd]);
>> -
>> - for (l2_e = 0; l2_e < PTL2_ENTRIES; l2_e++) {
>> - address = mem_map[pmd].base
>> - + (uintptr_t)l2_e * BLOCK_SIZE;
>> - set_ptl2_block(gd->arch.pmd_addr[pmd], l2_e,
>> - address, mem_map[pmd].attrs);
>> - }
>> + /* Should never reach here */
>> + return NULL;
>> +}
>> +
>> +/* Creates a new full table (512 entries) and sets *pte to refer to it */
>> +static u64 *create_table(void)
>> +{
>> + u64 *new_table = (u64*)gd->arch.tlb_fillptr;
>> + u64 pt_len = MAX_PTE_ENTRIES * sizeof(u64);
>> +
>> + /* Allocate MAX_PTE_ENTRIES pte entries */
>> + gd->arch.tlb_fillptr += pt_len;
>> +
>> + if (gd->arch.tlb_fillptr - gd->arch.tlb_addr > gd->arch.tlb_size)
>> + panic("Insufficient RAM for page table: 0x%lx > 0x%lx",
>> + gd->arch.tlb_fillptr - gd->arch.tlb_addr,
>> + gd->arch.tlb_size);
>
> For each of these panic() calls can you please add a comment as to
> what the user should do? It needs to be very clear what action should
> be taken to resolve the problem.
Good idea. The only case where I can't think of a good text is at
if (pte_type(pte) != PTE_TYPE_TABLE)
panic("PTE %p (%llx) for addr=%llx should be a table",
pte, *pte, start);
because this really is more of an assert(). We should never reach it.
Thanks a bunch for the reviews,
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
