Hi Alex,

On 21 February 2016 at 18:57, Alexander Graf agraf@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 agraf@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?

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 fenghua@phytium.com.cn

• • (C) Copyright 2016
• • Alexander Graf agraf@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()?

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

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

•   /* Mark all entries as invalid */
  

•   memset(new_table, 0, pt_len);
  
  

•                   pmd++;
  

•           } else {
  

•                   set_ptl1_entry(l1_e, 0);
  

•   return new_table;
  

+}

+static void set_pte_table(u64 *pte, u64 *table) +{

•   /* Point *pte to the new table */
  

•   DPRINTF("Setting %p to addr=%p\n", pte, table);
  

•   *pte = PTE_TYPE_TABLE | (ulong)table;
  

+}

+/* Add one mm_region map entry to the page tables */ +static void add_map(struct mm_region *map) +{

•   u64 *pte;
  

•   u64 addr = map->base;
  

•   u64 size = map->size;
  

•   u64 attrs = map->attrs | PTE_TYPE_BLOCK | PTE_BLOCK_AF;
  

•   u64 blocksize;
  

•   int level;
  

•   u64 *new_table;
  

•   while (size) {
  

•           pte = find_pte(addr, 0);
  

•           if (pte && (pte_type(pte) == PTE_TYPE_FAULT)) {
  

•                   DPRINTF("Creating table for addr 0x%llx\n", addr);
  

•                   new_table = create_table();
  

•                   set_pte_table(pte, new_table);
            }
  

•           for (level = 1; level < 4; level++) {
  

•                   pte = find_pte(addr, level);
  

•                   blocksize = 1ULL << level2shift(level);
  

•                   DPRINTF("Checking if pte fits for addr=%llx size=%llx "
  

•                           "blocksize=%llx\n", addr, size, blocksize);
  

•                   if (size >= blocksize && !(addr & (blocksize - 1))) {
  

•                           /* Page fits, create block PTE */
  

•                           DPRINTF("Setting PTE %p to block addr=%llx\n",
  

•                                   pte, addr);
  

•                           *pte = addr | attrs;
  

•                           addr += blocksize;
  

•                           size -= blocksize;
  

•                           break;
  

•                   } else if ((pte_type(pte) == PTE_TYPE_FAULT)) {
  

•                           /* Page doesn't fit, create subpages */
  

•                           DPRINTF("Creating subtable for addr 0x%llx "
  

•                                   "blksize=%llx\n", addr, blocksize);
  

•                           new_table = create_table();
  

•                           set_pte_table(pte, new_table);
  

•                   }
  

•           }
  

•   }
  

+}

+/* Splits a block PTE into table with subpages spanning the old block */ +static void split_block(u64 *pte, int level) +{

•   u64 old_pte = *pte;
  

•   u64 *new_table;
  

•   u64 i = 0;
  

•   /* level describes the parent level, we need the child ones */
  

•   int levelshift = level2shift(level + 1);
  

•   if (pte_type(pte) != PTE_TYPE_BLOCK)
  

•           panic("PTE %p (%llx) is not a block", pte, old_pte);
  

•   new_table = create_table();
  

•   DPRINTF("Splitting pte %p (%llx) into %p\n", pte, old_pte, new_table);
  

•   for (i = 0; i < MAX_PTE_ENTRIES; i++) {
  

•           new_table[i] = old_pte | (i << levelshift);
  

•           DPRINTF("Setting new_table[%lld] = %llx\n", i, new_table[i]);
    }
  

•   /* Set the new table into effect */
  

•   set_pte_table(pte, new_table);
  

+}

+/* 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;
  

•   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);
  

•           /* 1GB aligned pages fit already, so count the others */
  

•           if (map->size & 0x3fffffffULL)
  

•                   size += one_pt;
  

•           if (map->base & 0x3fffffffULL)
  

•                   size += one_pt;
  

•   }
  

•   /* Assume we may have to split up to 4 more page tables off */
  

•   size += one_pt * 4;
  

I suspect this is a better idea than just allocating a fixed size for the whole table (like 1MB). But the error you get when this fails should point to here so people know how to fix it.

•   return size;
  

+}

+static void setup_pgtables(void) +{

•   int i;
  

•   /*
  

•    * Allocate the first level we're on with invalidate entries.
  

•    * If the starting level is 0 (va_bits >= 39), then this is our
  

•    * Lv0 page table, otherwise it's the entry Lv1 page table.
  

•    */
  

•   gd->arch.tlb_fillptr = gd->arch.tlb_addr;
  

•   create_table();
  

•   /* Now add all MMU table entries one after another to the table */
  

•   for (i = 0; i < ARRAY_SIZE(mem_map); i++)
  

•           add_map(&mem_map[i]);
  

}

#else @@ -157,10 +322,8 @@ __weak void mmu_setup(void) int el;

#ifdef CONFIG_SYS_FULL_VA

•   unsigned long coreid = read_mpidr() & CONFIG_COREID_MASK;
  

•   /* Set up page tables only on BSP */
  

•   if (coreid == BSP_COREID)
  

•   /* Set up page tables only once */
  

•   if (!gd->arch.tlb_fillptr)
            setup_pgtables();
  
    el = current_el();
  

@@ -311,6 +474,79 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, flush_dcache_range(start, end); asm volatile("dsb sy"); } +#else +static bool is_aligned(u64 addr, u64 size, u64 align) +{

•   return !(addr & (align - 1)) && !(size & (align - 1));
  

+}

+static u64 set_one_region(u64 start, u64 size, u64 attrs, int level) +{

•   int levelshift = level2shift(level);
  

•   u64 levelsize = 1ULL << levelshift;
  

•   u64 *pte = find_pte(start, level);
  

•   /* Can we can just modify the current level block PTE? */
  

•   if (is_aligned(start, size, levelsize)) {
  

•           *pte &= ~PMD_ATTRINDX_MASK;
  

•           *pte |= attrs;
  

•           DPRINTF("Set attrs=%llx pte=%p level=%d\n", attrs, pte, level);
  

•           return levelsize;
  

•   }
  

•   /* Unaligned or doesn't fit, maybe split block into table */
  

•   DPRINTF("addr=%llx level=%d pte=%p (%llx)\n", start, level, pte, *pte);
  

•   /* Maybe we need to split the block into a table */
  

•   if (pte_type(pte) == PTE_TYPE_BLOCK)
  

•           split_block(pte, level);
  

•   /* And then double-check it became a table or already is one */
  

•   if (pte_type(pte) != PTE_TYPE_TABLE)
  

•           panic("PTE %p (%llx) for addr=%llx should be a table",
  

•                 pte, *pte, start);
  

•   /* Roll on to the next page table level */
  

•   return 0;
  

+}

+void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,

•                                enum dcache_option option)
  

+{

•   u64 attrs = PMD_ATTRINDX(option);
  

•   u64 real_start = start;
  

•   u64 real_size = size;
  

•   DPRINTF("start=%lx size=%lx\n", (ulong)start, (ulong)size);
  

•   /*
  

•    * Loop through the address range until we find a page granule that fits
  

•    * our alignment constraints, then set it to the new cache attributes
  

•    */
  

•   while (size > 0) {
  

•           int level;
  

•           u64 r;
  

•           for (level = 1; level < 4; level++) {
  

•                   r = set_one_region(start, size, attrs, level);
  

•                   if (r) {
  

•                           /* PTE successfully replaced */
  

•                           size -= r;
  

•                           start += r;
  

•                           break;
  

•                   }
  

•           }
  

•   }
  

•   asm volatile("dsb sy");
  

•   __asm_invalidate_tlb_all();
  

•   asm volatile("dsb sy");
  

•   asm volatile("isb");
  

•   flush_dcache_range(real_start, real_start + real_size);
  

•   asm volatile("dsb sy");
  

+} #endif

#else /* CONFIG_SYS_DCACHE_OFF */

[snip]

Regards, Simon

On 23.02.16 14:17, Simon Glass wrote:

Hi Alex,

On 21 February 2016 at 18:57, Alexander Graf agraf@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 agraf@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 fenghua@phytium.com.cn

• • (C) Copyright 2016
• • Alexander Graf agraf@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

On 02/24/2016 06:01 PM, Stephen Warren wrote:

On 02/24/2016 03:55 AM, Alexander Graf wrote:

...

On 23.02.16 14:17, Simon Glass wrote:

...

Hi Alex,

On 21 February 2016 at 18:57, Alexander Graf agraf@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 agraf@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 fenghua@phytium.com.cn

• • (C) Copyright 2016
• • Alexander Graf agraf@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.

That's only true if every single MMIO region that the code accesses is already represented in DT already, or any DT conversion patches also add any missing devices. It's quite unlikely that even on heavily DT-dependant platforms that /all/ in-use MMIO regions are already represented in DT. Such a conversion would need quite significant testing.

Well, it's something I'd leave to the SoC / board maintainer really. With v2 you can now fiddle with the map as much as you like inside your code. The important bit to me is that we have a standardized path on how we map the mem_map table into the MMU. I don't care how we fetch the table.

Alex

On 22.2.2016 02:57, Alexander Graf wrote:

Now that we have nice table driven page table creating code that gives us everything we need, move to that.

Signed-off-by: Alexander Graf agraf@suse.de

arch/arm/cpu/armv8/zynqmp/cpu.c | 169 ---------------------------------------- include/configs/xilinx_zynqmp.h | 44 +++++++++++ 2 files changed, 44 insertions(+), 169 deletions(-)

diff --git a/arch/arm/cpu/armv8/zynqmp/cpu.c b/arch/arm/cpu/armv8/zynqmp/cpu.c index c71f291..3f661a9 100644 --- a/arch/arm/cpu/armv8/zynqmp/cpu.c +++ b/arch/arm/cpu/armv8/zynqmp/cpu.c @@ -44,172 +44,3 @@ unsigned int zynqmp_get_silicon_version(void)

return ZYNQMP_CSU_VERSION_SILICON; }

-#ifndef CONFIG_SYS_DCACHE_OFF -#include <asm/armv8/mmu.h>

-#define SECTION_SHIFT_L1 30UL -#define SECTION_SHIFT_L2 21UL -#define BLOCK_SIZE_L0 0x8000000000UL -#define BLOCK_SIZE_L1 (1 << SECTION_SHIFT_L1) -#define BLOCK_SIZE_L2 (1 << SECTION_SHIFT_L2)

-#define TCR_TG1_4K (1 << 31) -#define TCR_EPD1_DISABLE (1 << 23) -#define ZYNQMO_VA_BITS 40 -#define ZYNQMP_TCR TCR_TG1_4K | \

• 		TCR_EPD1_DISABLE | \
  

• 		TCR_SHARED_OUTER | \
  

• 		TCR_SHARED_INNER | \
  

• 		TCR_IRGN_WBWA | \
  

• 		TCR_ORGN_WBWA | \
  

• 		TCR_T0SZ(ZYNQMO_VA_BITS)
  

-#define MEMORY_ATTR PMD_SECT_AF | PMD_SECT_INNER_SHARE | \

• 	PMD_ATTRINDX(MT_NORMAL) |	\
  

• 	PMD_TYPE_SECT
  

-#define DEVICE_ATTR PMD_SECT_AF | PMD_SECT_PXN | \

• 	PMD_SECT_UXN | PMD_ATTRINDX(MT_DEVICE_NGNRNE) |	\
  

• 	PMD_TYPE_SECT
  

-/* 4K size is required to place 512 entries in each level */ -#define TLB_TABLE_SIZE 0x1000

-struct attr_tbl {

• u32 num;
• u64 attr;

-};

-static struct attr_tbl attr_tbll1t0[4] = { {16, 0x0},

• 			   {8, DEVICE_ATTR},
  

• 			   {32, MEMORY_ATTR},
  

• 			   {456, DEVICE_ATTR}
  

• 			 };
  

-static struct attr_tbl attr_tbll2t3[4] = { {0x180, DEVICE_ATTR},

• 			   {0x40, 0x0},
  

• 			   {0x3F, DEVICE_ATTR},
  

• 			   {0x1, MEMORY_ATTR}
  

• 			 };
  

-/*

• • This mmu table looks as below
• • Level 0 table contains two entries to 512GB sizes. One is Level1 Table 0
• • and other Level1 Table1.
• • Level1 Table0 contains entries for each 1GB from 0 to 511GB.
• • Level1 Table1 contains entries for each 1GB from 512GB to 1TB.
• • Level2 Table0, Level2 Table1, Level2 Table2 and Level2 Table3 contains
• • entries for each 2MB starting from 0GB, 1GB, 2GB and 3GB respectively.
• */

-static void zynqmp_mmu_setup(void) -{

• int el;
• u32 index_attr;
• u64 i, section_l1t0, section_l1t1;
• u64 section_l2t0, section_l2t1, section_l2t2, section_l2t3;
• u64 *level0_table = (u64 *)gd->arch.tlb_addr;
• u64 *level1_table_0 = (u64 *)(gd->arch.tlb_addr + TLB_TABLE_SIZE);
• u64 *level1_table_1 = (u64 *)(gd->arch.tlb_addr + (2 * TLB_TABLE_SIZE));
• u64 *level2_table_0 = (u64 *)(gd->arch.tlb_addr + (3 * TLB_TABLE_SIZE));
• u64 *level2_table_1 = (u64 *)(gd->arch.tlb_addr + (4 * TLB_TABLE_SIZE));
• u64 *level2_table_2 = (u64 *)(gd->arch.tlb_addr + (5 * TLB_TABLE_SIZE));
• u64 *level2_table_3 = (u64 *)(gd->arch.tlb_addr + (6 * TLB_TABLE_SIZE));
• level0_table[0] =

• (u64)level1_table_0 | PMD_TYPE_TABLE;
  

• level0_table[1] =

• (u64)level1_table_1 | PMD_TYPE_TABLE;
  

• /*

• * set level 1 table 0, covering 0 to 512GB
  

• * set level 1 table 1, covering 512GB to 1TB
  

• */
  

• section_l1t0 = 0;
• section_l1t1 = BLOCK_SIZE_L0;
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level1_table_0[i] = section_l1t0;
  

• level1_table_0[i] |= attr_tbll1t0[index_attr].attr;
  

• attr_tbll1t0[index_attr].num--;
  

• if (attr_tbll1t0[index_attr].num == 0)
  

• 	index_attr++;
  

• level1_table_1[i] = section_l1t1;
  

• level1_table_1[i] |= DEVICE_ATTR;
  

• section_l1t0 += BLOCK_SIZE_L1;
  

• section_l1t1 += BLOCK_SIZE_L1;
  

• }
• level1_table_0[0] =

• (u64)level2_table_0 | PMD_TYPE_TABLE;
  

• level1_table_0[1] =

• (u64)level2_table_1 | PMD_TYPE_TABLE;
  

• level1_table_0[2] =

• (u64)level2_table_2 | PMD_TYPE_TABLE;
  

• level1_table_0[3] =

• (u64)level2_table_3 | PMD_TYPE_TABLE;
  

• section_l2t0 = 0;
• section_l2t1 = section_l2t0 + BLOCK_SIZE_L1; /* 1GB */
• section_l2t2 = section_l2t1 + BLOCK_SIZE_L1; /* 2GB */
• section_l2t3 = section_l2t2 + BLOCK_SIZE_L1; /* 3GB */
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level2_table_0[i] = section_l2t0 | MEMORY_ATTR;
  

• level2_table_1[i] = section_l2t1 | MEMORY_ATTR;
  

• level2_table_2[i] = section_l2t2 | DEVICE_ATTR;
  

• level2_table_3[i] = section_l2t3 |
  

• 		    attr_tbll2t3[index_attr].attr;
  

• attr_tbll2t3[index_attr].num--;
  

• if (attr_tbll2t3[index_attr].num == 0)
  

• 	index_attr++;
  

• section_l2t0 += BLOCK_SIZE_L2;
  

• section_l2t1 += BLOCK_SIZE_L2;
  

• section_l2t2 += BLOCK_SIZE_L2;
  

• section_l2t3 += BLOCK_SIZE_L2;
  

• }
• /* flush new MMU table */
• flush_dcache_range(gd->arch.tlb_addr,

• 	   gd->arch.tlb_addr + gd->arch.tlb_size);
  

• /* point TTBR to the new table */
• el = current_el();
• set_ttbr_tcr_mair(el, gd->arch.tlb_addr,

• 	  ZYNQMP_TCR, MEMORY_ATTRIBUTES);
  

• set_sctlr(get_sctlr() | CR_M);

-}

-int arch_cpu_init(void) -{

• icache_enable();
• __asm_invalidate_dcache_all();
• __asm_invalidate_tlb_all();
• return 0;

-}

-/*

• • This function is called from lib/board.c.
• • It recreates MMU table in main memory. MMU and d-cache are enabled earlier.
• • There is no need to disable d-cache for this operation.
• */

-void enable_caches(void) -{

• /* The data cache is not active unless the mmu is enabled */
• if (!(get_sctlr() & CR_M)) {

• invalidate_dcache_all();
  

• __asm_invalidate_tlb_all();
  

• zynqmp_mmu_setup();
  

• }
• puts("Enabling Caches...\n");
• set_sctlr(get_sctlr() | CR_C);

-}

-u64 *arch_get_page_table(void) -{

• return (u64 *)(gd->arch.tlb_addr + 0x3000);

-} -#endif diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h index 28622de..439f063 100644 --- a/include/configs/xilinx_zynqmp.h +++ b/include/configs/xilinx_zynqmp.h @@ -29,6 +29,50 @@ #define CONFIG_SYS_MEMTEST_START CONFIG_SYS_SDRAM_BASE #define CONFIG_SYS_MEMTEST_END CONFIG_SYS_SDRAM_SIZE

+#define CONFIG_SYS_FULL_VA +#define CONFIG_SYS_MEM_MAP { \

• { \

• .base = 0x0UL,						\
  

• .size = 0x80000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x80000000UL,					\
  

• .size = 0x70000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xf8000000UL,					\
  

• .size = 0x07e00000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xffe00000UL,					\
  

• .size = 0x00200000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x400000000UL,					\
  

• .size = 0x200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0x600000000UL,					\
  

• .size = 0x800000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0xe00000000UL,					\
  

• .size = 0xf200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, \
• }

/* Have release address at the end of 256MB for now */ #define CPU_RELEASE_ADDR 0xFFFFFF0

No problem with this default map. I didn't check every entry. Siva will look at it. My problem is that this is static. You should enable option option to generate this table dynamically. The reason is that systems can run just with PL DDR not PS and you can't map memory which is not there which can end up in system lock. We want to change current code to setup MMU table for memories at run time based on DT.

Thanks, Michal

Hi,

before I comment the rest. You need to also fix gem driver because it is using 1MB mapping.

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index b3821c31a91d..cf1376ce1bd7 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -150,10 +150,10 @@ struct emac_bd { };

#define RX_BUF 32 -/* Page table entries are set to 1MB, or multiples of 1MB - * (not < 1MB). driver uses less bd's so use 1MB bdspace. +/* Page table entries are set to 2MB, or multiples of 2MB + * (not < 2MB). driver uses less bd's so use 2MB bdspace. */ -#define BD_SPACE 0x100000 +#define BD_SPACE 0x200000 /* BD separation space */ #define BD_SEPRN_SPACE (RX_BUF * sizeof(struct emac_bd))

On 23.2.2016 12:33, Alexander Graf wrote:

On 23.02.16 12:04, Michal Simek wrote:

...

On 22.2.2016 02:57, Alexander Graf wrote:

...

Now that we have nice table driven page table creating code that gives us everything we need, move to that.

Signed-off-by: Alexander Graf agraf@suse.de

arch/arm/cpu/armv8/zynqmp/cpu.c | 169 ---------------------------------------- include/configs/xilinx_zynqmp.h | 44 +++++++++++ 2 files changed, 44 insertions(+), 169 deletions(-)

diff --git a/arch/arm/cpu/armv8/zynqmp/cpu.c b/arch/arm/cpu/armv8/zynqmp/cpu.c index c71f291..3f661a9 100644 --- a/arch/arm/cpu/armv8/zynqmp/cpu.c +++ b/arch/arm/cpu/armv8/zynqmp/cpu.c @@ -44,172 +44,3 @@ unsigned int zynqmp_get_silicon_version(void)

return ZYNQMP_CSU_VERSION_SILICON; }

-#ifndef CONFIG_SYS_DCACHE_OFF -#include <asm/armv8/mmu.h>

-#define SECTION_SHIFT_L1 30UL -#define SECTION_SHIFT_L2 21UL -#define BLOCK_SIZE_L0 0x8000000000UL -#define BLOCK_SIZE_L1 (1 << SECTION_SHIFT_L1) -#define BLOCK_SIZE_L2 (1 << SECTION_SHIFT_L2)

-#define TCR_TG1_4K (1 << 31) -#define TCR_EPD1_DISABLE (1 << 23) -#define ZYNQMO_VA_BITS 40 -#define ZYNQMP_TCR TCR_TG1_4K | \

• 		TCR_EPD1_DISABLE | \
  

• 		TCR_SHARED_OUTER | \
  

• 		TCR_SHARED_INNER | \
  

• 		TCR_IRGN_WBWA | \
  

• 		TCR_ORGN_WBWA | \
  

• 		TCR_T0SZ(ZYNQMO_VA_BITS)
  

-#define MEMORY_ATTR PMD_SECT_AF | PMD_SECT_INNER_SHARE | \

• 	PMD_ATTRINDX(MT_NORMAL) |	\
  

• 	PMD_TYPE_SECT
  

-#define DEVICE_ATTR PMD_SECT_AF | PMD_SECT_PXN | \

• 	PMD_SECT_UXN | PMD_ATTRINDX(MT_DEVICE_NGNRNE) |	\
  

• 	PMD_TYPE_SECT
  

-/* 4K size is required to place 512 entries in each level */ -#define TLB_TABLE_SIZE 0x1000

-struct attr_tbl {

• u32 num;
• u64 attr;

-};

-static struct attr_tbl attr_tbll1t0[4] = { {16, 0x0},

• 			   {8, DEVICE_ATTR},
  

• 			   {32, MEMORY_ATTR},
  

• 			   {456, DEVICE_ATTR}
  

• 			 };
  

-static struct attr_tbl attr_tbll2t3[4] = { {0x180, DEVICE_ATTR},

• 			   {0x40, 0x0},
  

• 			   {0x3F, DEVICE_ATTR},
  

• 			   {0x1, MEMORY_ATTR}
  

• 			 };
  

-/*

• • This mmu table looks as below
• • Level 0 table contains two entries to 512GB sizes. One is Level1 Table 0
• • and other Level1 Table1.
• • Level1 Table0 contains entries for each 1GB from 0 to 511GB.
• • Level1 Table1 contains entries for each 1GB from 512GB to 1TB.
• • Level2 Table0, Level2 Table1, Level2 Table2 and Level2 Table3 contains
• • entries for each 2MB starting from 0GB, 1GB, 2GB and 3GB respectively.
• */

-static void zynqmp_mmu_setup(void) -{

• int el;
• u32 index_attr;
• u64 i, section_l1t0, section_l1t1;
• u64 section_l2t0, section_l2t1, section_l2t2, section_l2t3;
• u64 *level0_table = (u64 *)gd->arch.tlb_addr;
• u64 *level1_table_0 = (u64 *)(gd->arch.tlb_addr + TLB_TABLE_SIZE);
• u64 *level1_table_1 = (u64 *)(gd->arch.tlb_addr + (2 * TLB_TABLE_SIZE));
• u64 *level2_table_0 = (u64 *)(gd->arch.tlb_addr + (3 * TLB_TABLE_SIZE));
• u64 *level2_table_1 = (u64 *)(gd->arch.tlb_addr + (4 * TLB_TABLE_SIZE));
• u64 *level2_table_2 = (u64 *)(gd->arch.tlb_addr + (5 * TLB_TABLE_SIZE));
• u64 *level2_table_3 = (u64 *)(gd->arch.tlb_addr + (6 * TLB_TABLE_SIZE));
• level0_table[0] =

• (u64)level1_table_0 | PMD_TYPE_TABLE;
  

• level0_table[1] =

• (u64)level1_table_1 | PMD_TYPE_TABLE;
  

• /*

• * set level 1 table 0, covering 0 to 512GB
  

• * set level 1 table 1, covering 512GB to 1TB
  

• */
  

• section_l1t0 = 0;
• section_l1t1 = BLOCK_SIZE_L0;
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level1_table_0[i] = section_l1t0;
  

• level1_table_0[i] |= attr_tbll1t0[index_attr].attr;
  

• attr_tbll1t0[index_attr].num--;
  

• if (attr_tbll1t0[index_attr].num == 0)
  

• 	index_attr++;
  

• level1_table_1[i] = section_l1t1;
  

• level1_table_1[i] |= DEVICE_ATTR;
  

• section_l1t0 += BLOCK_SIZE_L1;
  

• section_l1t1 += BLOCK_SIZE_L1;
  

• }
• level1_table_0[0] =

• (u64)level2_table_0 | PMD_TYPE_TABLE;
  

• level1_table_0[1] =

• (u64)level2_table_1 | PMD_TYPE_TABLE;
  

• level1_table_0[2] =

• (u64)level2_table_2 | PMD_TYPE_TABLE;
  

• level1_table_0[3] =

• (u64)level2_table_3 | PMD_TYPE_TABLE;
  

• section_l2t0 = 0;
• section_l2t1 = section_l2t0 + BLOCK_SIZE_L1; /* 1GB */
• section_l2t2 = section_l2t1 + BLOCK_SIZE_L1; /* 2GB */
• section_l2t3 = section_l2t2 + BLOCK_SIZE_L1; /* 3GB */
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level2_table_0[i] = section_l2t0 | MEMORY_ATTR;
  

• level2_table_1[i] = section_l2t1 | MEMORY_ATTR;
  

• level2_table_2[i] = section_l2t2 | DEVICE_ATTR;
  

• level2_table_3[i] = section_l2t3 |
  

• 		    attr_tbll2t3[index_attr].attr;
  

• attr_tbll2t3[index_attr].num--;
  

• if (attr_tbll2t3[index_attr].num == 0)
  

• 	index_attr++;
  

• section_l2t0 += BLOCK_SIZE_L2;
  

• section_l2t1 += BLOCK_SIZE_L2;
  

• section_l2t2 += BLOCK_SIZE_L2;
  

• section_l2t3 += BLOCK_SIZE_L2;
  

• }
• /* flush new MMU table */
• flush_dcache_range(gd->arch.tlb_addr,

• 	   gd->arch.tlb_addr + gd->arch.tlb_size);
  

• /* point TTBR to the new table */
• el = current_el();
• set_ttbr_tcr_mair(el, gd->arch.tlb_addr,

• 	  ZYNQMP_TCR, MEMORY_ATTRIBUTES);
  

• set_sctlr(get_sctlr() | CR_M);

-}

-int arch_cpu_init(void) -{

• icache_enable();
• __asm_invalidate_dcache_all();
• __asm_invalidate_tlb_all();
• return 0;

-}

-/*

• • This function is called from lib/board.c.
• • It recreates MMU table in main memory. MMU and d-cache are enabled earlier.
• • There is no need to disable d-cache for this operation.
• */

-void enable_caches(void) -{

• /* The data cache is not active unless the mmu is enabled */
• if (!(get_sctlr() & CR_M)) {

• invalidate_dcache_all();
  

• __asm_invalidate_tlb_all();
  

• zynqmp_mmu_setup();
  

• }
• puts("Enabling Caches...\n");
• set_sctlr(get_sctlr() | CR_C);

-}

-u64 *arch_get_page_table(void) -{

• return (u64 *)(gd->arch.tlb_addr + 0x3000);

-} -#endif diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h index 28622de..439f063 100644 --- a/include/configs/xilinx_zynqmp.h +++ b/include/configs/xilinx_zynqmp.h @@ -29,6 +29,50 @@ #define CONFIG_SYS_MEMTEST_START CONFIG_SYS_SDRAM_BASE #define CONFIG_SYS_MEMTEST_END CONFIG_SYS_SDRAM_SIZE

+#define CONFIG_SYS_FULL_VA +#define CONFIG_SYS_MEM_MAP { \

• { \

• .base = 0x0UL,						\
  

• .size = 0x80000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x80000000UL,					\
  

• .size = 0x70000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xf8000000UL,					\
  

• .size = 0x07e00000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xffe00000UL,					\
  

• .size = 0x00200000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x400000000UL,					\
  

• .size = 0x200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0x600000000UL,					\
  

• .size = 0x800000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0xe00000000UL,					\
  

• .size = 0xf200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, \
• }

/* Have release address at the end of 256MB for now */ #define CPU_RELEASE_ADDR 0xFFFFFF0

No problem with this default map. I didn't check every entry. Siva will look at it. My problem is that this is static. You should enable option option to generate this table dynamically. The reason is that systems can run just

The way the code works today, I could easily just use a 0-size element as array terminator and use an external pointer rather than the local memory map array.

However, that contradicts with a runtime memory contrained approach to determine the page table size. The only good path I could come up with here is to generate the page table during build time and then save how big it was. We couldn't do that with runtime changing tables.

0-size element is not enough. The problem are base addresses too. ZynqMP memory map is just this

1. 0-2GB PS memory up to 2GB 2. 2GB-3GB PL part 3. 3GB-4GB PCIE + device + small memories OCM/TCM 4. 4GB-16GB reserved 5. 16-24GB PL part 6. 24-32GB PCIe 7. 32-64GB DDR 8. 64-512GB PL 9. 512-768 PCIe 10. 768-1TB DDR 11. 1TB-16TB PL

PL regions 2, 5, 11 can also contain memories but you are able to find them via DT for particular HW design. And start addresses can be in this range. In Xilinx tree I have patches for reading memory configuration from DT and saving them to gd->bd->bi_dram based on CONFIG_NR_DRAM_BANKS setting and I think this should be wired. It means you should setup just memory attributes for memories which u-boot is aware of. For the rest of address space it is not that sensitive.

...

with PL DDR not PS and you can't map memory which is not there which can end up in system lock. We want to change current code to setup MMU table for memories at run time based on DT.

So in your use case, the regions would still be there, just their size changes. That means we could still have static tables that then get modified by runtime code later on to just span less?

Not entirely. Definitely the part of table can be static but I tend to have especially memory mapping more dynamic.

Of course, with a full device tree, we could just generate the mmu tables completely from dt. But then we'd need a good answer to how we determine the page table pool size...

I don't think it is required but at least memory mapping should be more dynamic. As is visible we are moving to u-boot which is fully configured from DT that's why none will want to changes in MMU table that's why this should be more flexible.

Thanks, Michal

On 26.2.2016 01:49, Alexander Graf wrote:

On 23.02.16 14:07, Michal Simek wrote:

...

Hi,

before I comment the rest. You need to also fix gem driver because it is using 1MB mapping.

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index b3821c31a91d..cf1376ce1bd7 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -150,10 +150,10 @@ struct emac_bd { };

#define RX_BUF 32 -/* Page table entries are set to 1MB, or multiples of 1MB

• • (not < 1MB). driver uses less bd's so use 1MB bdspace.

+/* Page table entries are set to 2MB, or multiples of 2MB

• • (not < 2MB). driver uses less bd's so use 2MB bdspace.
  */

-#define BD_SPACE 0x100000 +#define BD_SPACE 0x200000 /* BD separation space */ #define BD_SEPRN_SPACE (RX_BUF * sizeof(struct emac_bd))

Looks like I didn't reply to this before. The change above makes a lot of space, but is not required for this patch set. We simply break the page table down to 4k maps.

Unfortunately 1MB can be divided by 4k pages but with the v1 series I was testing this driver stopped to work.

So to keep the bits I touch in this patch set as small as possible, I think we should (if we really want to) move this into a separate, follow-up patch.

It means this has to be solved before this series is applied because none wants to break any driver.

...

On 23.2.2016 12:33, Alexander Graf wrote:

...

On 23.02.16 12:04, Michal Simek wrote:

...

On 22.2.2016 02:57, Alexander Graf wrote:

...

Now that we have nice table driven page table creating code that gives us everything we need, move to that.

Signed-off-by: Alexander Graf agraf@suse.de

arch/arm/cpu/armv8/zynqmp/cpu.c | 169 ---------------------------------------- include/configs/xilinx_zynqmp.h | 44 +++++++++++ 2 files changed, 44 insertions(+), 169 deletions(-)

diff --git a/arch/arm/cpu/armv8/zynqmp/cpu.c b/arch/arm/cpu/armv8/zynqmp/cpu.c index c71f291..3f661a9 100644 --- a/arch/arm/cpu/armv8/zynqmp/cpu.c +++ b/arch/arm/cpu/armv8/zynqmp/cpu.c @@ -44,172 +44,3 @@ unsigned int zynqmp_get_silicon_version(void)

return ZYNQMP_CSU_VERSION_SILICON; }

-#ifndef CONFIG_SYS_DCACHE_OFF -#include <asm/armv8/mmu.h>

-#define SECTION_SHIFT_L1 30UL -#define SECTION_SHIFT_L2 21UL -#define BLOCK_SIZE_L0 0x8000000000UL -#define BLOCK_SIZE_L1 (1 << SECTION_SHIFT_L1) -#define BLOCK_SIZE_L2 (1 << SECTION_SHIFT_L2)

-#define TCR_TG1_4K (1 << 31) -#define TCR_EPD1_DISABLE (1 << 23) -#define ZYNQMO_VA_BITS 40 -#define ZYNQMP_TCR TCR_TG1_4K | \

• 		TCR_EPD1_DISABLE | \
  

• 		TCR_SHARED_OUTER | \
  

• 		TCR_SHARED_INNER | \
  

• 		TCR_IRGN_WBWA | \
  

• 		TCR_ORGN_WBWA | \
  

• 		TCR_T0SZ(ZYNQMO_VA_BITS)
  

-#define MEMORY_ATTR PMD_SECT_AF | PMD_SECT_INNER_SHARE | \

• 	PMD_ATTRINDX(MT_NORMAL) |	\
  

• 	PMD_TYPE_SECT
  

-#define DEVICE_ATTR PMD_SECT_AF | PMD_SECT_PXN | \

• 	PMD_SECT_UXN | PMD_ATTRINDX(MT_DEVICE_NGNRNE) |	\
  

• 	PMD_TYPE_SECT
  

-/* 4K size is required to place 512 entries in each level */ -#define TLB_TABLE_SIZE 0x1000

-struct attr_tbl {

• u32 num;
• u64 attr;

-};

-static struct attr_tbl attr_tbll1t0[4] = { {16, 0x0},

• 			   {8, DEVICE_ATTR},
  

• 			   {32, MEMORY_ATTR},
  

• 			   {456, DEVICE_ATTR}
  

• 			 };
  

-static struct attr_tbl attr_tbll2t3[4] = { {0x180, DEVICE_ATTR},

• 			   {0x40, 0x0},
  

• 			   {0x3F, DEVICE_ATTR},
  

• 			   {0x1, MEMORY_ATTR}
  

• 			 };
  

-/*

• • This mmu table looks as below
• • Level 0 table contains two entries to 512GB sizes. One is Level1 Table 0
• • and other Level1 Table1.
• • Level1 Table0 contains entries for each 1GB from 0 to 511GB.
• • Level1 Table1 contains entries for each 1GB from 512GB to 1TB.
• • Level2 Table0, Level2 Table1, Level2 Table2 and Level2 Table3 contains
• • entries for each 2MB starting from 0GB, 1GB, 2GB and 3GB respectively.
• */

-static void zynqmp_mmu_setup(void) -{

• int el;
• u32 index_attr;
• u64 i, section_l1t0, section_l1t1;
• u64 section_l2t0, section_l2t1, section_l2t2, section_l2t3;
• u64 *level0_table = (u64 *)gd->arch.tlb_addr;
• u64 *level1_table_0 = (u64 *)(gd->arch.tlb_addr + TLB_TABLE_SIZE);
• u64 *level1_table_1 = (u64 *)(gd->arch.tlb_addr + (2 * TLB_TABLE_SIZE));
• u64 *level2_table_0 = (u64 *)(gd->arch.tlb_addr + (3 * TLB_TABLE_SIZE));
• u64 *level2_table_1 = (u64 *)(gd->arch.tlb_addr + (4 * TLB_TABLE_SIZE));
• u64 *level2_table_2 = (u64 *)(gd->arch.tlb_addr + (5 * TLB_TABLE_SIZE));
• u64 *level2_table_3 = (u64 *)(gd->arch.tlb_addr + (6 * TLB_TABLE_SIZE));
• level0_table[0] =

• (u64)level1_table_0 | PMD_TYPE_TABLE;
  

• level0_table[1] =

• (u64)level1_table_1 | PMD_TYPE_TABLE;
  

• /*

• * set level 1 table 0, covering 0 to 512GB
  

• * set level 1 table 1, covering 512GB to 1TB
  

• */
  

• section_l1t0 = 0;
• section_l1t1 = BLOCK_SIZE_L0;
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level1_table_0[i] = section_l1t0;
  

• level1_table_0[i] |= attr_tbll1t0[index_attr].attr;
  

• attr_tbll1t0[index_attr].num--;
  

• if (attr_tbll1t0[index_attr].num == 0)
  

• 	index_attr++;
  

• level1_table_1[i] = section_l1t1;
  

• level1_table_1[i] |= DEVICE_ATTR;
  

• section_l1t0 += BLOCK_SIZE_L1;
  

• section_l1t1 += BLOCK_SIZE_L1;
  

• }
• level1_table_0[0] =

• (u64)level2_table_0 | PMD_TYPE_TABLE;
  

• level1_table_0[1] =

• (u64)level2_table_1 | PMD_TYPE_TABLE;
  

• level1_table_0[2] =

• (u64)level2_table_2 | PMD_TYPE_TABLE;
  

• level1_table_0[3] =

• (u64)level2_table_3 | PMD_TYPE_TABLE;
  

• section_l2t0 = 0;
• section_l2t1 = section_l2t0 + BLOCK_SIZE_L1; /* 1GB */
• section_l2t2 = section_l2t1 + BLOCK_SIZE_L1; /* 2GB */
• section_l2t3 = section_l2t2 + BLOCK_SIZE_L1; /* 3GB */
• index_attr = 0;
• for (i = 0; i < 512; i++) {

• level2_table_0[i] = section_l2t0 | MEMORY_ATTR;
  

• level2_table_1[i] = section_l2t1 | MEMORY_ATTR;
  

• level2_table_2[i] = section_l2t2 | DEVICE_ATTR;
  

• level2_table_3[i] = section_l2t3 |
  

• 		    attr_tbll2t3[index_attr].attr;
  

• attr_tbll2t3[index_attr].num--;
  

• if (attr_tbll2t3[index_attr].num == 0)
  

• 	index_attr++;
  

• section_l2t0 += BLOCK_SIZE_L2;
  

• section_l2t1 += BLOCK_SIZE_L2;
  

• section_l2t2 += BLOCK_SIZE_L2;
  

• section_l2t3 += BLOCK_SIZE_L2;
  

• }
• /* flush new MMU table */
• flush_dcache_range(gd->arch.tlb_addr,

• 	   gd->arch.tlb_addr + gd->arch.tlb_size);
  

• /* point TTBR to the new table */
• el = current_el();
• set_ttbr_tcr_mair(el, gd->arch.tlb_addr,

• 	  ZYNQMP_TCR, MEMORY_ATTRIBUTES);
  

• set_sctlr(get_sctlr() | CR_M);

-}

-int arch_cpu_init(void) -{

• icache_enable();
• __asm_invalidate_dcache_all();
• __asm_invalidate_tlb_all();
• return 0;

-}

-/*

• • This function is called from lib/board.c.
• • It recreates MMU table in main memory. MMU and d-cache are enabled earlier.
• • There is no need to disable d-cache for this operation.
• */

-void enable_caches(void) -{

• /* The data cache is not active unless the mmu is enabled */
• if (!(get_sctlr() & CR_M)) {

• invalidate_dcache_all();
  

• __asm_invalidate_tlb_all();
  

• zynqmp_mmu_setup();
  

• }
• puts("Enabling Caches...\n");
• set_sctlr(get_sctlr() | CR_C);

-}

-u64 *arch_get_page_table(void) -{

• return (u64 *)(gd->arch.tlb_addr + 0x3000);

-} -#endif diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h index 28622de..439f063 100644 --- a/include/configs/xilinx_zynqmp.h +++ b/include/configs/xilinx_zynqmp.h @@ -29,6 +29,50 @@ #define CONFIG_SYS_MEMTEST_START CONFIG_SYS_SDRAM_BASE #define CONFIG_SYS_MEMTEST_END CONFIG_SYS_SDRAM_SIZE

+#define CONFIG_SYS_FULL_VA +#define CONFIG_SYS_MEM_MAP { \

• { \

• .base = 0x0UL,						\
  

• .size = 0x80000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x80000000UL,					\
  

• .size = 0x70000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xf8000000UL,					\
  

• .size = 0x07e00000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0xffe00000UL,					\
  

• .size = 0x00200000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0x400000000UL,					\
  

• .size = 0x200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, { \

• .base = 0x600000000UL,					\
  

• .size = 0x800000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |			\
  

• 	 PTE_BLOCK_INNER_SHARE				\
  

• }, { \

• .base = 0xe00000000UL,					\
  

• .size = 0xf200000000UL,					\
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |		\
  

• 	 PTE_BLOCK_NON_SHARE |				\
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN			\
  

• }, \
• }

/* Have release address at the end of 256MB for now */ #define CPU_RELEASE_ADDR 0xFFFFFF0

No problem with this default map. I didn't check every entry. Siva will look at it. My problem is that this is static. You should enable option option to generate this table dynamically. The reason is that systems can run just

The way the code works today, I could easily just use a 0-size element as array terminator and use an external pointer rather than the local memory map array.

However, that contradicts with a runtime memory contrained approach to determine the page table size. The only good path I could come up with here is to generate the page table during build time and then save how big it was. We couldn't do that with runtime changing tables.

0-size element is not enough. The problem are base addresses too. ZynqMP memory map is just this

1. 0-2GB PS memory up to 2GB
2. 2GB-3GB PL part
3. 3GB-4GB PCIE + device + small memories OCM/TCM
4. 4GB-16GB reserved
5. 16-24GB PL part
6. 24-32GB PCIe
7. 32-64GB DDR
8. 64-512GB PL
9. 512-768 PCIe
10. 768-1TB DDR
11. 1TB-16TB PL

PL regions 2, 5, 11 can also contain memories but you are able to find them via DT for particular HW design. And start addresses can be in this range. In Xilinx tree I have patches for reading memory configuration from DT and saving them to gd->bd->bi_dram based on CONFIG_NR_DRAM_BANKS setting and I think this should be wired. It means you should setup just memory attributes for memories which u-boot is aware of. For the rest of address space it is not that sensitive.

I think that makes a lot of sense. The current patch set simply moves the logic as it stands today into tables rather than open coded code. To modify the tables dynamically as next step sounds like a good idea to me.

ok

...

...

...

with PL DDR not PS and you can't map memory which is not there which can end up in system lock. We want to change current code to setup MMU table for memories at run time based on DT.

So in your use case, the regions would still be there, just their size changes. That means we could still have static tables that then get modified by runtime code later on to just span less?

Not entirely. Definitely the part of table can be static but I tend to have especially memory mapping more dynamic.

I guess the way the table is now a struct in a .c file works for you? It means you can modify it to whatever extent you like, maybe even generate it completely dynamically if you wanted to.

I have seen that and will look at it again when I start to work with HW.

Thanks, Michal