[U-Boot] [PATCH 2/9] arm64: Make full va map code more dynamic

[Alexander Graf <agraf@suse.de>]

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