From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Wed, 24 Feb 2016 11:14:29 -0700
Subject: [U-Boot] [PATCH 02/10] arm64: Make full va map code more dynamic
In-Reply-To: <1456315904-113924-3-git-send-email-agraf@suse.de>
References: <1456315904-113924-1-git-send-email-agraf@suse.de>
 <1456315904-113924-3-git-send-email-agraf@suse.de>
Message-ID: <56CDF305.1020302@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/24/2016 05:11 AM, 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.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

> +/*
> + * This is a recursively called function to count the number of
> + * page tables we need to cover a particular PTE range. If you
> + * call this with level = -1 you basically get the full 48 bit
> + * coverage.
> + */
> +static int count_required_pts(u64 addr, int level, u64 maxaddr)

I think this looks correct now. Nits below if a respin is needed for 
other reasons.

> +{
> +	int levelshift = level2shift(level);
> +	u64 levelsize = 1ULL << levelshift;
> +	u64 levelmask = levelsize - 1;
> +	u64 levelend = addr + levelsize;
> +	int r = 0;
> +	int i;
> +	bool is_level = false;

I might suggest renaming that is_level_needed. It's not obvious to me 
exactly what the name "is_level" is intended to represent; the name 
seems to represent whether something (unspecified) is a level or not.

> +
> +	for (i = 0; i < ARRAY_SIZE(mem_map); i++) {
> +		struct mm_region *map = &mem_map[i];
> +		u64 start = map->base;
> +		u64 end = start + map->size;
> +
> +		/* Check if the PTE would overlap with the map */
> +		if (max(addr, start) <= min(levelend, end)) {
> +			start = max(addr, start);
> +			end = min(levelend, end);
> +
> +			/* We need a sub-pt for this level */
> +			if ((start & levelmask) || (end & levelmask)) {
> +				is_level = true;
> +				break;
> +			}
> +
> +			/* Lv0 can not do block PTEs, so do levels here too */
> +			if (level <= 0) {
> +				is_level = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * Block PTEs at this level are already covered by the parent page
> +	 * table, so we only need to count sub page tables.
> +	 */
> +	if (is_level) {
> +		int sublevel = level + 1;
> +		u64 sublevelsize = 1ULL << level2shift(sublevel);
> +
> +		/* Account for the new sub page table ... */
> +		r = 1;

"Account for the page table at /this/ level"? This may represent the 
top-level (0/1) page table, not just sub page tables. The sub-level 
accounting is via the recursive call to count_required_pts() I believe.

> +
> +		/* ... and for all child page tables that one might have */
> +		for (i = 0; i < MAX_PTE_ENTRIES; i++) {
> +			r += count_required_pts(addr, sublevel, maxaddr);
> +			addr += sublevelsize;