From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 29 Feb 2016 09:52:21 -0700 Subject: [U-Boot] [PATCH 02/10] arm64: Make full va map code more dynamic In-Reply-To: <56D191FE.80801@suse.de> References: <1456315904-113924-1-git-send-email-agraf@suse.de> <1456315904-113924-3-git-send-email-agraf@suse.de> <56CDF305.1020302@wwwdotorg.org> <56CF2D7B.9090101@suse.de> <56D0A6C0.6040404@wwwdotorg.org> <56D191FE.80801@suse.de> Message-ID: <56D47745.50905@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/27/2016 05:09 AM, Alexander Graf wrote: > > > On 26.02.16 20:25, Stephen Warren wrote: >> On 02/25/2016 09:36 AM, Alexander Graf wrote: >>> >>> >>> On 24.02.16 19:14, Stephen Warren wrote: >>>> 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 >>>> >>>>> +/* >>>>> + * 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. >>> >>> We're basically asking the function whether a PTE for address at >>> level would be an inval/block/level PTE. is_level marks it as a >>> level pte. >>> >>> I could maybe rename this into pte_type and create an enum that is >>> either PTE_INVAL, PTE_BLOCK or PTE_LEVEL. >>> >>>> >>>>> + >>>>> + 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. >>> >>> I think the easiest way to visualize what's going on is if we start with >>> level -1. >>> >>> We basically ask the function at level -1 whether a PTE at level -1 (48 >>> bits) would fit into a block PTE at level -1 (so the PTE spans all 48 >>> bits) or whether we need to create a new level entry plus page table >>> for it. >> >> Hmm, I had overlooked that the call to count_required_pts() passed >> "start_level - 1" not "start_level". Now that I see that, your >> explanation makes more sense to me. >> >> It'd be nice if some/all of this explanation were comments in the code >> to aid readers. >> >> That said, I would have expected a slightly more direct calculation; why >> not: >> >> int start_level = 0; // or 1 if small VA space >> total_pts = count_required_pts(start_level); > > We need to account for the count twice, to have an (immutable) copy of > our page tables around when we split them. I think that's just a hard-coded "* 2" there, unless I'm missing your point? >> total_pts += 4; // "random" number to account for later splits >> >> int count_required_pts(int level, ...) { > > ... includes the address, as I have it today, I guess? > >> int npts = 1; // the table at this level >> for each pte slot at this level: >> if a mem_map entry starts/stops within the pte slot: >> npts += count_required_pts(level + 1, ...); > > This means we would count some levels multiple times. Imagine two > entries from > > 1G - 1.25G > 1.25G - 1.5G > > With your pseudo-code, we would count for both cases while checking for > 1G page table entries. Hence we need to jump out of the loop here and do > the counting outside. I don't think so; "if a mem_map entry starts/stops within the pte slot" is a single decision for that PT entry at the current level, not a loop that recurses once per memory block. Hence, you'd only ever recurse once, so there's no double-counting. > > At which point this is basically my code, right? :) > > > Alex > >> else: >> // nothing; pte is either a block or inval at this level >> return npts; >> } >> >> Still, the current code appears to work find, and can be ammended later >> if we want. >