[PATCH v5 1/3] arm64: mm: Support Common Not Private translations

[catalin.marinas@arm.com (Catalin Marinas)]

On Mon, Jul 30, 2018 at 05:29:35PM +0100, Robin Murphy wrote:
> On 30/07/18 16:42, Catalin Marinas wrote:
> > On Mon, Jul 30, 2018 at 11:08:27AM +0100, Vladimir Murzin wrote:
> > > On 27/07/18 12:35, Catalin Marinas wrote:
> > > > On Tue, Jun 19, 2018 at 11:18:20AM +0100, Vladimir Murzin wrote:
> > > > > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> > > > > index 39ec0b8..c506fb7 100644
> > > > > --- a/arch/arm64/include/asm/mmu_context.h
> > > > > +++ b/arch/arm64/include/asm/mmu_context.h
> > > > > @@ -149,6 +149,18 @@ static inline void cpu_replace_ttbr1(pgd_t *pgdp)
> > > > >   	phys_addr_t pgd_phys = virt_to_phys(pgdp);
> > > > > +	if (system_supports_cnp() && !WARN_ON(pgdp != lm_alias(swapper_pg_dir))) {
> > > > > +		/*
> > > > > +		 * cpu_replace_ttbr1() is used when there's a boot CPU
> > > > > +		 * up (i.e. cpufeature framework is not up yet) and
> > > > > +		 * latter only when we enable CNP via cpufeature's
> > > > > +		 * enable() callback.
> > > > > +		 * Also we rely on the cpu_hwcap bit being set before
> > > > > +		 * calling the enable() function.
> > > > > +		 */
> > > > > +		pgd_phys |= TTBR_CNP_BIT;
> > > > > +	}
> > > > > +
> > > > >   	replace_phys = (void *)__pa_symbol(idmap_cpu_replace_ttbr1);
> > > > 
> > > > So the above code sets the TTBR_CNP_BIT (bit 0) in pgd_phys and calls
> > > > the idmap_cpu_replace_ttbr1() with this value. Looking at the latter, it
> > > > performs a phys_to_ttbr transformation of pgd_phys which masks out the
> > > > bottom 2 bits when CONFIG_ARM64_PA_BITS_52 is enabled. I think we need
> > > > to tweak TTBR_BADDR_MASK_52 to start from bit 0.
> > > 
> > > Something like bellow?
> > > 
> > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> > > index 0bcc98d..e0b4b2f 100644
> > > --- a/arch/arm64/include/asm/assembler.h
> > > +++ b/arch/arm64/include/asm/assembler.h
> > > @@ -524,11 +524,10 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
> > >    * 	ttbr:	returns the TTBR value
> > >    */
> > >   	.macro	phys_to_ttbr, ttbr, phys
> > > -#ifdef CONFIG_ARM64_PA_BITS_52
> > > -	orr	\ttbr, \phys, \phys, lsr #46
> > > -	and	\ttbr, \ttbr, #TTBR_BADDR_MASK_52
> > > -#else
> > >   	mov	\ttbr, \phys
> > > +#ifdef CONFIG_ARM64_PA_BITS_52
> > > +	ubfx	\ttbr, \ttbr, #48, #4
> > > +	orr	\ttbr, \phys, \ttbr, lsl #2
> > >   #endif
> > >   	.endm
> > 
> > This would do, I don't have a better idea on how to write it. But I'd
> > like a comment to say that this is moving bits 51:48 of the address to
> > bits 5:2 of TTBR_ELx.
> 
> The diff above is *copying* 51:48 into 5:2; it doesn't *move* it like the
> existing code does. That's pretty crucial, because without the BADDR masking
> operation it's going to leave the upper address bits in the ASID/VMID field,
> which looks like a recipe for disaster if the reserved TTBR1 happens to be
> at a high enough address (at least cpu_switch_mm might just be OK by virtue
> of using BFI rather than ORR for the ASID).

Oh, very good point.

> > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > > index 1bdeca8..1b9d0e9 100644
> > > --- a/arch/arm64/include/asm/pgtable.h
> > > +++ b/arch/arm64/include/asm/pgtable.h
> > > @@ -770,7 +770,7 @@ static inline void update_mmu_cache(struct vm_area_struct *vma,
> > >   #define kc_offset_to_vaddr(o)	((o) | VA_START)
> > >   #ifdef CONFIG_ARM64_PA_BITS_52
> > > -#define phys_to_ttbr(addr)	(((addr) | ((addr) >> 46)) & TTBR_BADDR_MASK_52)
> > > +#define phys_to_ttbr(addr)	((addr) | (((addr) >> 46) & TTBR_BADDR_MASK_52))
> 
> Ditto - by the look of things, this definitely stands to corrupt the VMID in
> update_vttbr(). If we really have no choice but to smuggle the CnP value in
> something which is logically an address, then I think we'd need to handle it
> more like this:
> 
> #define TTBR_CNP (1)
> #define phys_to_ttbr(addr) \
> ((((addr) | ((addr) >> 46)) & TTBR_BADDR_MASK_52) | (addr & TTBR_CNP))
> 
> Although in patch #2 we're only applying CnP *after* the BADDR conversion,
> so is changing this one even necessary? If I've understood the intent
> correctly, all that might be needed is something like the below (untested,
> of course).

Or we could keep phys_to_ttbr() as it is for both the asm and C and
apply the CNP bit afterwards (as you've already noticed in patch 2). For
cpu_replace_ttbr1(), we'd also need to change idmap_cpu_replace_ttbr1 to
actually take the TTBR1_EL1 value directly so that it doesn't have to
invoke phys_to_ttbr().

-- 
Catalin