From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Mon, 30 Jul 2018 17:29:35 +0100 Subject: [PATCH v5 1/3] arm64: mm: Support Common Not Private translations In-Reply-To: <20180730154209.2tawriqxgxub7paa@armageddon.cambridge.arm.com> References: <1529403502-2843-1-git-send-email-vladimir.murzin@arm.com> <1529403502-2843-2-git-send-email-vladimir.murzin@arm.com> <20180727113502.3qdbyffuvysva6bz@armageddon.cambridge.arm.com> <20180730154209.2tawriqxgxub7paa@armageddon.cambridge.arm.com> Message-ID: <16344e26-6cd2-5e55-d34e-fd2a48bef733@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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). >> 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). Robin. ----->8----- diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 0bcc98dbba56..5b0f8f80238f 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -527,6 +527,7 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU #ifdef CONFIG_ARM64_PA_BITS_52 orr \ttbr, \phys, \phys, lsr #46 and \ttbr, \ttbr, #TTBR_BADDR_MASK_52 + bfi \ttbr, \addr, #0, #1 // CnP #else mov \ttbr, \phys #endif