From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Mon, 22 Feb 2016 11:18:25 -0700
Subject: [U-Boot] [PATCH 2/9] arm64: Make full va map code more dynamic
In-Reply-To: <1456106232-233210-3-git-send-email-agraf@suse.de>
References: <1456106232-233210-1-git-send-email-agraf@suse.de>
 <1456106232-233210-3-git-send-email-agraf@suse.de>
Message-ID: <56CB50F1.6060508@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

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

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

> +		/* 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).

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

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