Re: [PATCH v3 6/7] arm64: mm: Implement 4 levels of translation tables

[Steve Capper]

On Sun, Apr 27, 2014 at 12:37:35PM +0900, Jungseok Lee wrote:
> On Thursday, April 24, 2014 1:02 AM, Steve Capper wrote:
> > On Fri, Apr 18, 2014 at 04:59:20PM +0900, Jungseok Lee wrote:

[ ... ]

> > 
> > This is overly complicated. For <4 levels we set x0 to be:
> > ttbr1 + 2*PAGE_SIZE. For 4-levels, we set x0 to be ttbr1 + PAGE_SIZE, then inside the create_pgd_entry
> > macro, we check the VA for FIXADDR_TOP then add another PAGE_SIZE. This is presumably done so the same
> > PUD is used for the swapper block map and the FIXADDR map.
> 
> Is it too complicated to understand the logic?
> 
> 1) For <4 levels:
> PAGE_SIZE is added for FIXADDR map and x0 is passed to create pgd entry.
> 2) For =4 levels:
> PAGE_SIZE is added in the create_pgd_entry macro since FIXADDR map info
> is needed to create pud entry.
> 
> However, I agree that the code should be revised if other people feel
> like it is a labyrinthine logic.
> 
> > If you assume that the PUD always follows the PGD for 4-levels, then you can remove this #ifdef and
> > the conditional VA logic in set_pgd_entry. To make the logic simpler for <4 levels, you could call
> > create_pud_entry in the middle of create_pgd_entry, then put down the actual pgd after.
> 
> create_pud_entry should distinguish block map from FIXADDR map although
> PUD always follows the PGD in case of 4 levels. If we would like to avoid
> unnecessary #ifdef, the conditional logic should be introduced in create_
> pgd_entry macro.
> 
> I cannot find the "best" way even though I've tried to figure it out.
> I would like to find out the most clear and self-descriptive expression.
> 
> Could you give an idea on how to remove both conditional VA logic and #ifdef?


Hello Jungseok,
I had the following logic in my head:
It compiles and runs on the model, but I've not tried it in anger.

=========================================================================

	.macro create_pud_entry, pgd, tbl, virt, pud, tmp1, tmp2
#ifdef CONFIG_ARM64_4_LEVELS
	add	\tbl, \tbl, #PAGE_SIZE		// bump tbl 1 page up.
						// to make room for pud
	add	\pud, \pgd, #PAGE_SIZE		// pgd points to pud which
						// follows pgd
	lsr	\tmp1, \virt, #PUD_SHIFT
	and	\tmp1, \tmp1, #PTRS_PER_PUD - 1	// PUD index
	orr	\tmp2, \tbl, #3			// PUD entry table type
	str	\tmp2, [\pud, \tmp1, lsl #3]
#else
	mov	\pud, \tbl			// pgd points to section table
						// directly for < 4 levels
#endif
	.endm

/*
 * Macro to populate the PGD for the corresponding block entry in the next
 * level (tbl) for the given virtual address.
 *
 * Preserves:	pgd, virt
 * Corrupts:	tmp1, tmp2, tmp3
 * Returns:	tbl -> page where block mappings can be placed
 *	(changed to make room for pud with 4levels, preserved otherwise)
 */
	.macro	create_pgd_entry, pgd, tbl, virt, tmp1, tmp2, tmp3
	create_pud_entry \pgd, \tbl, \virt, \tmp3, \tmp1, \tmp2
	lsr	\tmp1, \virt, #PGDIR_SHIFT
	and	\tmp1, \tmp1, #PTRS_PER_PGD - 1	// PGD index
	orr	\tmp2, \tmp3, #3		// PGD entry table type
	str	\tmp2, [\pgd, \tmp1, lsl #3]
	.endm

=========================================================================

[Note I've changed the extra argument to create_pgd_entry to be at the end
to make it easier to diff callers]

So essentially, we bump up tbl if we are running with 4 levels of page
table. We put the pgd down after the pud, and this allows us to sneak a
pud in after the pgd if we need to, otherwise it points to the target
for the section mapping.

Does this work for you? (I go cross-eyed with too much assembler, so
could have easily missed something).

> 
> > > +	create_pgd_entry x26, x0, x1, x5, x6, x7
> > >
> > 
> > So before this patch we have the following created by
> > __create_page_tables:
> > 
> > +========================+ <--- TEXT_OFFSET + PHYS_OFFSET
> > | FIXADDR (pmd or pte)   |
> > +------------------------+
> > | block map (pmd or pte) |
> > +------------------------+
> > | PGDs for swapper       |
> > +========================+ <--- TTBR1 swapper_pg_dir
> > | block map for idmap    |
> > +------------------------+
> > | PGDs for idmap         |
> > +------------------------+ <--- TTBR0 idmap_pg_dir
> > 
> > 
> > After the patch, for 4 levels activated we have:
> > +========================+ <--- TEXT_OFFSET + PHYS_OFFSET
> > | FIXADDR (ptes)         |
> > +------------------------+
> > | block map (ptes)       |
> > +------------------------+
> > | PUDs for swapper       |
> > +------------------------+
> > | PGDs for swapper       |
> > +========================+ <--- TTBR1 swapper_pg_dir
> > | block map for idmap    |
> > +------------------------+
> > | PUDs for idmap         |
> > +------------------------+
> > | PGDs for idmap         |
> > +------------------------+ <--- TTBR0 idmap_pg_dir
> > 
> > and without 4 levels activated we have:
> > +========================+ <--- TEXT_OFFSET + PHYS_OFFSET
> > | ZERO BYTES             |
> > +------------------------+
> > | FIXADDR (pmd or pte)   |
> > +------------------------+
> > | block map (pmd or pte) |
> > +------------------------+
> > | PGDs for swapper       |
> > +========================+ <--- TTBR1 swapper_pg_dir
> > | ZERO BYTES             |
> > +------------------------+
> > | block map for idmap    |
> > +------------------------+
> > | PGDs for idmap         |
> > +------------------------+ <--- TTBR0 idmap_pg_dir
> 
> This is definitely helpful to understand head.S.
> It would be good to add these figures to Documentation or comments.

Please feel free to grab it if you want... :-).
Otherwise, I can put a patch in for this, as I am working on some logic
to remap the physical memory a PUD blocks for 4K granule.

Cheers,
-- 
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/