* [PATCH 0/3] Support Common Not Private translations
@ 2017-10-09 12:55 Vladimir Murzin
2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Vladimir Murzin @ 2017-10-09 12:55 UTC (permalink / raw)
To: linux-arm-kernel
Common Not Private (CNP) translations is a feature of ARMv8.2
extension which allows translation table entries to be shared between
different PEs in the same inner shareable domain, so the hardware can
use this fact to optimise the caching of such entries in the TLB.
This patch set is an attempt to bring CNP support into Linux. It was
tested on a v8.2 Fast Model with exploring traces and checking that
TTBRx_ELy and VTTBR_EL2 have CnP bit set where appropriate.
Changelog:
RFC -> v1
- dropped RFC tag
- rebased on 4.14-rc4
Thanks!
Vladimir Murzin (3):
arm64: mm: Support Common Not Private translations
arm64: KVM: Support Common Not Private translations
arm64: Introduce command line parameter to disable CNP
Documentation/admin-guide/kernel-parameters.txt | 4 +++
arch/arm64/Kconfig | 13 +++++++++
arch/arm64/include/asm/asm-uaccess.h | 2 ++
arch/arm64/include/asm/cpucaps.h | 3 +-
arch/arm64/include/asm/cpufeature.h | 6 ++++
arch/arm64/include/asm/memory.h | 1 +
arch/arm64/include/asm/mmu_context.h | 14 +++++++++
arch/arm64/include/asm/uaccess.h | 10 +++++--
arch/arm64/kernel/cpufeature.c | 39 +++++++++++++++++++++++++
arch/arm64/kernel/hibernate.c | 2 +-
arch/arm64/kvm/hyp-init.S | 4 +++
arch/arm64/kvm/hyp/switch.c | 6 +++-
arch/arm64/mm/proc.S | 5 ++++
13 files changed, 104 insertions(+), 5 deletions(-)
--
2.0.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 12:55 [PATCH 0/3] Support Common Not Private translations Vladimir Murzin @ 2017-10-09 12:55 ` Vladimir Murzin 2017-10-09 15:23 ` Catalin Marinas 2017-10-10 15:19 ` James Morse 2017-10-09 12:55 ` [PATCH 2/3] arm64: KVM: " Vladimir Murzin 2017-10-09 12:55 ` [PATCH 3/3] arm64: Introduce command line parameter to disable CNP Vladimir Murzin 2 siblings, 2 replies; 13+ messages in thread From: Vladimir Murzin @ 2017-10-09 12:55 UTC (permalink / raw) To: linux-arm-kernel Common Not Private (CNP) is a feature of ARMv8.2 extension which allows translation table entries to be shared between different PEs in the same inner shareable domain, so the hardware can use this fact to optimise the caching of such entries in the TLB. CNP occupies one bit in TTBRx_ELy and VTTBR_EL2, which advertises to the hardware that the translation table entries pointed to by this TTBR are the same as every PE in the same inner shareable domain for which the equivalent TTBR also has CNP bit set. In case CNP bit is set but TTBR does not point at the same translation table entries, then the system is mis-configured, so the results of translations are UNPREDICTABLE. This patch adds support for Common Not Private translations on different exceptions levels: (1) For EL0 there are a few cases we need to care of changes in TTBR0_EL1: - a switch to idmap - software emulated PAN in these cases we make sure that CNP is set for non-zero ASIDs only. (2) For EL1 we postpone setting CNP till all cpus are up and rely on cpufeature framework to 1) patch the code which is sensitive to CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is changed to save raw TTBR1_EL1 and blindly restore it on resume. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm64/Kconfig | 13 +++++++++++++ arch/arm64/include/asm/asm-uaccess.h | 2 ++ arch/arm64/include/asm/cpucaps.h | 3 ++- arch/arm64/include/asm/cpufeature.h | 6 ++++++ arch/arm64/include/asm/memory.h | 1 + arch/arm64/include/asm/mmu_context.h | 14 ++++++++++++++ arch/arm64/include/asm/uaccess.h | 10 ++++++++-- arch/arm64/kernel/cpufeature.c | 21 +++++++++++++++++++++ arch/arm64/kernel/hibernate.c | 2 +- arch/arm64/mm/proc.S | 5 +++++ 10 files changed, 73 insertions(+), 4 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 0df64a6..351382f 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -973,6 +973,19 @@ config ARM64_PMEM operations if DC CVAP is not supported (following the behaviour of DC CVAP itself if the system does not define a point of persistence). +config ARM64_CNP + bool "Enable support for Common Not Private (CNP) translations" + default y + help + Common Not Private (CNP) allows translation table entries to + be shared between different PEs in the same inner shareable + domain, so the hardware can use this fact to optimise the + caching of such entries in the TLB. + + Selecting this option allows the CNP feature to be detected + at runtime, and does not affect PEs that do not implement + this feature. + endmenu config ARM64_MODULE_CMODEL_LARGE diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h index ecd9788..a3e452e 100644 --- a/arch/arm64/include/asm/asm-uaccess.h +++ b/arch/arm64/include/asm/asm-uaccess.h @@ -12,6 +12,8 @@ #ifdef CONFIG_ARM64_SW_TTBR0_PAN .macro __uaccess_ttbr0_disable, tmp1 mrs \tmp1, ttbr1_el1 // swapper_pg_dir + bic \tmp1, \tmp1, #TTBR_CNP_BIT // unconditionally clear CNP bit to avoid alternatives + // inside alternatives add \tmp1, \tmp1, #SWAPPER_DIR_SIZE // reserved_ttbr0 at the end of swapper_pg_dir msr ttbr0_el1, \tmp1 // set reserved TTBR0_EL1 isb diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index 8da6216..9b7d94c 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -40,7 +40,8 @@ #define ARM64_WORKAROUND_858921 19 #define ARM64_WORKAROUND_CAVIUM_30115 20 #define ARM64_HAS_DCPOP 21 +#define ARM64_HAS_CNP 22 -#define ARM64_NCAPS 22 +#define ARM64_NCAPS 23 #endif /* __ASM_CPUCAPS_H */ diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 428ee1f..f7c75ac 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -262,6 +262,12 @@ static inline bool system_uses_ttbr0_pan(void) !cpus_have_const_cap(ARM64_HAS_PAN); } +static inline bool system_supports_cnp(void) +{ + return IS_ENABLED(CONFIG_ARM64_CNP) && + cpus_have_const_cap(ARM64_HAS_CNP); +} + #endif /* __ASSEMBLY__ */ #endif diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index f7c4d21..9640abc 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -78,6 +78,7 @@ #define PCI_IO_START (PCI_IO_END - PCI_IO_SIZE) #define FIXADDR_TOP (PCI_IO_START - SZ_2M) #define TASK_SIZE_64 (UL(1) << VA_BITS) +#define TTBR_CNP_BIT (UL(1) << 0) #ifdef CONFIG_COMPAT #define TASK_SIZE_32 UL(0x100000000) diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index 3257895a..c8adce2 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -135,6 +135,17 @@ static inline void cpu_replace_ttbr1(pgd_t *pgd) phys_addr_t pgd_phys = virt_to_phys(pgd); + if (system_supports_cnp()) { + /* + * cpu_replace_ttbr1() is used when there's a boot CPU up + * (i.e. cpufeture framework is not up yet) and latter only + * when we enable CNP via cpufeature's enable() callback. + */ + BUG_ON(pgd != swapper_pg_dir); + + pgd_phys |= TTBR_CNP_BIT; + } + replace_phys = (void *)__pa_symbol(idmap_cpu_replace_ttbr1); cpu_install_idmap(); @@ -178,6 +189,9 @@ static inline void update_saved_ttbr0(struct task_struct *tsk, BUG_ON(mm->pgd == swapper_pg_dir); task_thread_info(tsk)->ttbr0 = virt_to_phys(mm->pgd) | ASID(mm) << 48; + + if (system_supports_cnp() && ASID(mm)) + task_thread_info(tsk)->ttbr0 |= TTBR_CNP_BIT; } } #else diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index fc0f9eb..14b1799 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -107,8 +107,14 @@ static inline void __uaccess_ttbr0_disable(void) { unsigned long ttbr; - /* reserved_ttbr0 placed at the end of swapper_pg_dir */ - ttbr = read_sysreg(ttbr1_el1) + SWAPPER_DIR_SIZE; + /* + * reserved_ttbr0 is placed at the end of swapper_pg_dir. + * When CNP is in use, TTBR1 may have the CNP bit set, but the + * reserved_ttbr should only be used without CNP. + */ + ttbr = read_sysreg(ttbr1_el1); + ttbr &= ~TTBR_CNP_BIT; + ttbr += SWAPPER_DIR_SIZE; write_sysreg(ttbr, ttbr0_el1); isb(); } diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 21e2c95..8d098a1 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -101,6 +101,7 @@ EXPORT_SYMBOL(cpu_hwcap_keys); static bool __maybe_unused cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused); +static int cpu_enable_cnp(void *__unused);; /* * NOTE: Any changes to the visibility of features should be kept in @@ -898,6 +899,18 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .sys_reg = SYS_ID_AA64ISAR1_EL1, .field_pos = ID_AA64ISAR1_DPB_SHIFT, .min_field_value = 1, +#endif +#ifdef CONFIG_ARM64_CNP + { + .desc = "Common not Private translations", + .capability = ARM64_HAS_CNP, + .def_scope = SCOPE_SYSTEM, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64MMFR2_EL1, + .sign = FTR_UNSIGNED, + .field_pos = ID_AA64MMFR2_CNP_SHIFT, + .min_field_value = 1, + .enable = cpu_enable_cnp, }, #endif {}, @@ -1211,6 +1224,14 @@ cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) return (cpus_have_const_cap(ARM64_HAS_PAN) && !cpus_have_const_cap(ARM64_HAS_UAO)); } +#ifdef CONFIG_ARM64_CNP +static int cpu_enable_cnp(void *__unused) +{ + cpu_replace_ttbr1(swapper_pg_dir); + return 0; +} +#endif /* CONFIG_ARM64_CNP */ + /* * We emulate only the following system register space. * Op0 = 0x3, CRn = 0x0, Op1 = 0x0, CRm = [0, 4 - 7] diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index 095d3c1..1d056f3 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size) return -EOVERFLOW; arch_hdr_invariants(&hdr->invariants); - hdr->ttbr1_el1 = __pa_symbol(swapper_pg_dir); + hdr->ttbr1_el1 = read_sysreg(ttbr1_el1); hdr->reenter_kernel = _cpu_resume; /* We can't use __hyp_get_vectors() because kvm may still be loaded */ diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 877d42f..1c94030 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -141,6 +141,11 @@ ENTRY(cpu_do_switch_mm) pre_ttbr0_update_workaround x0, x2, x3 mmid x1, x1 // get mm->context.id bfi x0, x1, #48, #16 // set the ASID +alternative_if ARM64_HAS_CNP + cbz x1, 1f + orr x0, x0, #TTBR_CNP_BIT +1: +alternative_else_nop_endif msr ttbr0_el1, x0 // set TTBR0 isb post_ttbr0_update_workaround -- 2.0.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin @ 2017-10-09 15:23 ` Catalin Marinas 2017-10-09 16:48 ` James Morse 2017-10-10 12:50 ` Vladimir Murzin 2017-10-10 15:19 ` James Morse 1 sibling, 2 replies; 13+ messages in thread From: Catalin Marinas @ 2017-10-09 15:23 UTC (permalink / raw) To: linux-arm-kernel Hi Vladimir, On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote: > Common Not Private (CNP) is a feature of ARMv8.2 extension which > allows translation table entries to be shared between different PEs in > the same inner shareable domain, so the hardware can use this fact to > optimise the caching of such entries in the TLB. > > CNP occupies one bit in TTBRx_ELy and VTTBR_EL2, which advertises to > the hardware that the translation table entries pointed to by this > TTBR are the same as every PE in the same inner shareable domain for > which the equivalent TTBR also has CNP bit set. In case CNP bit is set > but TTBR does not point at the same translation table entries, I would add something like "for a given ASID and VMID". > then > the system is mis-configured, so the results of translations are > UNPREDICTABLE. > > This patch adds support for Common Not Private translations on > different exceptions levels: > > (1) For EL0 there are a few cases we need to care of changes in > TTBR0_EL1: > - a switch to idmap > - software emulated PAN > in these cases we make sure that CNP is set for non-zero ASIDs > only. > > (2) For EL1 we postpone setting CNP till all cpus are up and rely on > cpufeature framework to 1) patch the code which is sensitive to > CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where > TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is > changed to save raw TTBR1_EL1 and blindly restore it on resume. Even if you do this when all the CPUs are up, that's not always true. Starting with maxcpus=1 allows something like systemd to bring up new CPUs once user space starts. The problem we have is that we don't know what the firmware is doing, whether it's setting CnP or not. Maybe we should add some statement in Documentation/arm64/booting.txt that firmware must not use CnP at all. > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index f7c4d21..9640abc 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -78,6 +78,7 @@ > #define PCI_IO_START (PCI_IO_END - PCI_IO_SIZE) > #define FIXADDR_TOP (PCI_IO_START - SZ_2M) > #define TASK_SIZE_64 (UL(1) << VA_BITS) > +#define TTBR_CNP_BIT (UL(1) << 0) Please move this to arch/arm64/include/asm/pgtable-hwdef.h. That's where we keep the TCR_* bits as well. > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > index 3257895a..c8adce2 100644 > --- a/arch/arm64/include/asm/mmu_context.h > +++ b/arch/arm64/include/asm/mmu_context.h > @@ -135,6 +135,17 @@ static inline void cpu_replace_ttbr1(pgd_t *pgd) > > phys_addr_t pgd_phys = virt_to_phys(pgd); > > + if (system_supports_cnp()) { > + /* > + * cpu_replace_ttbr1() is used when there's a boot CPU up > + * (i.e. cpufeture framework is not up yet) and latter only s/cpufeture/cpufeature/ > + * when we enable CNP via cpufeature's enable() callback. > + */ > + BUG_ON(pgd != swapper_pg_dir); > + > + pgd_phys |= TTBR_CNP_BIT; > + } Rather than BUG_ON, can we have: if (system_supports_cnp() && pgd == swapper_pg_dir) or, if you want to keep the warning: if (system_supports_cnp() && !WARN_ON(pgd != swapper_pg_dir)) We also seem to rely on the cpu_hwcap bit being set before calling the enable() function. We need to be careful not to change this, otherwise the above will break. > @@ -178,6 +189,9 @@ static inline void update_saved_ttbr0(struct task_struct *tsk, > BUG_ON(mm->pgd == swapper_pg_dir); > task_thread_info(tsk)->ttbr0 = > virt_to_phys(mm->pgd) | ASID(mm) << 48; > + > + if (system_supports_cnp() && ASID(mm)) > + task_thread_info(tsk)->ttbr0 |= TTBR_CNP_BIT; > } > } > #else > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index fc0f9eb..14b1799 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -107,8 +107,14 @@ static inline void __uaccess_ttbr0_disable(void) > { > unsigned long ttbr; > > - /* reserved_ttbr0 placed at the end of swapper_pg_dir */ > - ttbr = read_sysreg(ttbr1_el1) + SWAPPER_DIR_SIZE; > + /* > + * reserved_ttbr0 is placed at the end of swapper_pg_dir. > + * When CNP is in use, TTBR1 may have the CNP bit set, but the > + * reserved_ttbr should only be used without CNP. > + */ > + ttbr = read_sysreg(ttbr1_el1); > + ttbr &= ~TTBR_CNP_BIT; > + ttbr += SWAPPER_DIR_SIZE; > write_sysreg(ttbr, ttbr0_el1); > isb(); > } As for the asm __uaccess_ttbr0_disable, we probably don't care as hw PAN is available since ARMv8.1 and CnP is an ARMv8.2 feature. Sow we always end up with unnecessary code for sw PAN that's only executed where it doesn't matter. We could check for this at feature detection time (with a .matches function) or just add a Kconfig line: depends on ARM64_PAN || !ARM64_SW_TTBR0_PAN > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > index 095d3c1..1d056f3 100644 > --- a/arch/arm64/kernel/hibernate.c > +++ b/arch/arm64/kernel/hibernate.c > @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size) > return -EOVERFLOW; > > arch_hdr_invariants(&hdr->invariants); > - hdr->ttbr1_el1 = __pa_symbol(swapper_pg_dir); > + hdr->ttbr1_el1 = read_sysreg(ttbr1_el1); > hdr->reenter_kernel = _cpu_resume; > > /* We can't use __hyp_get_vectors() because kvm may still be loaded */ Are all the CPUs up when coming out of hibernation and restoring ttbr1_el1? > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index 877d42f..1c94030 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -141,6 +141,11 @@ ENTRY(cpu_do_switch_mm) > pre_ttbr0_update_workaround x0, x2, x3 > mmid x1, x1 // get mm->context.id > bfi x0, x1, #48, #16 // set the ASID > +alternative_if ARM64_HAS_CNP > + cbz x1, 1f > + orr x0, x0, #TTBR_CNP_BIT > +1: > +alternative_else_nop_endif Some comments here would be useful for future readers (e.g. "do not set the CnP bit if ASID == 0). -- Catalin ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 15:23 ` Catalin Marinas @ 2017-10-09 16:48 ` James Morse 2017-10-10 12:50 ` Vladimir Murzin 2017-10-10 12:50 ` Vladimir Murzin 1 sibling, 1 reply; 13+ messages in thread From: James Morse @ 2017-10-09 16:48 UTC (permalink / raw) To: linux-arm-kernel Hi Catalin, Vladimir, On 09/10/17 16:23, Catalin Marinas wrote: > On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote: >> This patch adds support for Common Not Private translations on >> different exceptions levels: >> (2) For EL1 we postpone setting CNP till all cpus are up and rely on >> cpufeature framework to 1) patch the code which is sensitive to >> CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where >> TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is >> changed to save raw TTBR1_EL1 and blindly restore it on resume. > Even if you do this when all the CPUs are up, that's not always true. > Starting with maxcpus=1 allows something like systemd to bring up new > CPUs once user space starts. For secondary CPUs cpu_enable_cnp() will be called to set CNP on TTBR1_EL1. But what about cpuidle? This also resets the TTBR1_EL1 value. > The problem we have is that we don't know > what the firmware is doing, whether it's setting CnP or not. Maybe we > should add some statement in Documentation/arm64/booting.txt that > firmware must not use CnP at all. >> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c >> index 095d3c1..1d056f3 100644 >> --- a/arch/arm64/kernel/hibernate.c >> +++ b/arch/arm64/kernel/hibernate.c >> @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size) >> return -EOVERFLOW; >> >> arch_hdr_invariants(&hdr->invariants); >> - hdr->ttbr1_el1 = __pa_symbol(swapper_pg_dir); >> + hdr->ttbr1_el1 = read_sysreg(ttbr1_el1); >> hdr->reenter_kernel = _cpu_resume; >> >> /* We can't use __hyp_get_vectors() because kvm may still be loaded */ > Are all the CPUs up when coming out of hibernation and restoring > ttbr1_el1? 'nonboot' CPUs are powered off around hibernate, so this only runs on one CPU. Restoring with the CNP set like this will share all the TTBR1 mappings using the reserved ASID out of TTBR0. Hibernate then calls cpu_uninstall_idmap() via __cpu_suspend_exit(), which will restore the original ttbr0 value and CNP bit. Thanks, James ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 16:48 ` James Morse @ 2017-10-10 12:50 ` Vladimir Murzin 2017-10-10 15:19 ` James Morse 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2017-10-10 12:50 UTC (permalink / raw) To: linux-arm-kernel Hi James, On 09/10/17 17:48, James Morse wrote: > Hi Catalin, Vladimir, > > On 09/10/17 16:23, Catalin Marinas wrote: >> On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote: >>> This patch adds support for Common Not Private translations on >>> different exceptions levels: > >>> (2) For EL1 we postpone setting CNP till all cpus are up and rely on >>> cpufeature framework to 1) patch the code which is sensitive to >>> CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where >>> TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is >>> changed to save raw TTBR1_EL1 and blindly restore it on resume. > >> Even if you do this when all the CPUs are up, that's not always true. >> Starting with maxcpus=1 allows something like systemd to bring up new >> CPUs once user space starts. > > For secondary CPUs cpu_enable_cnp() will be called to set CNP on TTBR1_EL1. But > what about cpuidle? This also resets the TTBR1_EL1 value. Good point! I've missed it because reset happens via __enable_mmu, which has no idea about CnP. Would something like below be sufficient? diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c index 1e3be90..03a02c4 100644 --- a/arch/arm64/kernel/suspend.c +++ b/arch/arm64/kernel/suspend.c @@ -46,6 +46,10 @@ void notrace __cpu_suspend_exit(void) */ cpu_uninstall_idmap(); +#ifdef CONFIG_ARM64_CNP + /* Restore CnP bit in TTBR1_EL1 */ + cpu_replace_ttbr1(swapper_pg_dir); +#endif /* * PSTATE was not saved over suspend/resume, re-enable any detected * features that might not have been set correctly. > > >> The problem we have is that we don't know >> what the firmware is doing, whether it's setting CnP or not. Maybe we >> should add some statement in Documentation/arm64/booting.txt that >> firmware must not use CnP at all. > > >>> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c >>> index 095d3c1..1d056f3 100644 >>> --- a/arch/arm64/kernel/hibernate.c >>> +++ b/arch/arm64/kernel/hibernate.c >>> @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size) >>> return -EOVERFLOW; >>> >>> arch_hdr_invariants(&hdr->invariants); >>> - hdr->ttbr1_el1 = __pa_symbol(swapper_pg_dir); >>> + hdr->ttbr1_el1 = read_sysreg(ttbr1_el1); >>> hdr->reenter_kernel = _cpu_resume; >>> >>> /* We can't use __hyp_get_vectors() because kvm may still be loaded */ > >> Are all the CPUs up when coming out of hibernation and restoring >> ttbr1_el1? > > 'nonboot' CPUs are powered off around hibernate, so this only runs on one CPU. > > Restoring with the CNP set like this will share all the TTBR1 mappings using the > reserved ASID out of TTBR0. Hibernate then calls cpu_uninstall_idmap() via > __cpu_suspend_exit(), which will restore the original ttbr0 value and CNP bit. > Thanks for explanation! Vladimir > > Thanks, > > James > ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-10 12:50 ` Vladimir Murzin @ 2017-10-10 15:19 ` James Morse 0 siblings, 0 replies; 13+ messages in thread From: James Morse @ 2017-10-10 15:19 UTC (permalink / raw) To: linux-arm-kernel Hi Vladimir, On 10/10/17 13:50, Vladimir Murzin wrote: > On 09/10/17 17:48, James Morse wrote: >> On 09/10/17 16:23, Catalin Marinas wrote: >>> On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote: >>>> This patch adds support for Common Not Private translations on >>>> different exceptions levels: >> >>>> (2) For EL1 we postpone setting CNP till all cpus are up and rely on >>>> cpufeature framework to 1) patch the code which is sensitive to >>>> CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where >>>> TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is >>>> changed to save raw TTBR1_EL1 and blindly restore it on resume. >> >>> Even if you do this when all the CPUs are up, that's not always true. >>> Starting with maxcpus=1 allows something like systemd to bring up new >>> CPUs once user space starts. >> >> For secondary CPUs cpu_enable_cnp() will be called to set CNP on TTBR1_EL1. But >> what about cpuidle? This also resets the TTBR1_EL1 value. > > Good point! I've missed it because reset happens via __enable_mmu, which has > no idea about CnP. > > Would something like below be sufficient? > > > diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c > index 1e3be90..03a02c4 100644 > --- a/arch/arm64/kernel/suspend.c > +++ b/arch/arm64/kernel/suspend.c > @@ -46,6 +46,10 @@ void notrace __cpu_suspend_exit(void) > */ > cpu_uninstall_idmap(); > > +#ifdef CONFIG_ARM64_CNP > + /* Restore CnP bit in TTBR1_EL1 */ > + cpu_replace_ttbr1(swapper_pg_dir); > +#endif > /* > * PSTATE was not saved over suspend/resume, re-enable any detected > * features that might not have been set correctly. > This re-install -> uninstalls the idmap, and if we don't actually have CNP support, it wouldn't have changed anything. How about: > if (cpus_have_const_cap(ARM64_HAS_CNP) > cpu_replace_ttbr1(lm_alias(swapper_pg_dir)); We could look at having a combined helper that is called with the idmap installed and does the uninstall. hibernate uses these cpu_suspend helpers to save/restore the CPU registers, so if we fix cpu-idle, you don't need the hibernate hunk anymore. Thanks, James ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 15:23 ` Catalin Marinas 2017-10-09 16:48 ` James Morse @ 2017-10-10 12:50 ` Vladimir Murzin 1 sibling, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2017-10-10 12:50 UTC (permalink / raw) To: linux-arm-kernel Hi Catalin, On 09/10/17 16:23, Catalin Marinas wrote: > Hi Vladimir, > > On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote: >> Common Not Private (CNP) is a feature of ARMv8.2 extension which >> allows translation table entries to be shared between different PEs in >> the same inner shareable domain, so the hardware can use this fact to >> optimise the caching of such entries in the TLB. >> >> CNP occupies one bit in TTBRx_ELy and VTTBR_EL2, which advertises to >> the hardware that the translation table entries pointed to by this >> TTBR are the same as every PE in the same inner shareable domain for >> which the equivalent TTBR also has CNP bit set. In case CNP bit is set >> but TTBR does not point at the same translation table entries, > > I would add something like "for a given ASID and VMID". Done. > >> then >> the system is mis-configured, so the results of translations are >> UNPREDICTABLE. >> >> This patch adds support for Common Not Private translations on >> different exceptions levels: >> >> (1) For EL0 there are a few cases we need to care of changes in >> TTBR0_EL1: >> - a switch to idmap >> - software emulated PAN >> in these cases we make sure that CNP is set for non-zero ASIDs >> only. >> >> (2) For EL1 we postpone setting CNP till all cpus are up and rely on >> cpufeature framework to 1) patch the code which is sensitive to >> CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where >> TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is >> changed to save raw TTBR1_EL1 and blindly restore it on resume. > > Even if you do this when all the CPUs are up, that's not always true. > Starting with maxcpus=1 allows something like systemd to bring up new > CPUs once user space starts. The problem we have is that we don't know > what the firmware is doing, whether it's setting CnP or not. Maybe we > should add some statement in Documentation/arm64/booting.txt that > firmware must not use CnP at all. I think it should be fine if firmware uses CnP in secure world and it is quite unusual to see firmware executing at the same exception level as Linux. However, Mark told me in private that kexec can be such sort of "firmware" it is why PATCH 3/3 exists. Anyway, I do agree that CnP should be mentioned in booting.txt, but I'm not sure about wording... > >> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h >> index f7c4d21..9640abc 100644 >> --- a/arch/arm64/include/asm/memory.h >> +++ b/arch/arm64/include/asm/memory.h >> @@ -78,6 +78,7 @@ >> #define PCI_IO_START (PCI_IO_END - PCI_IO_SIZE) >> #define FIXADDR_TOP (PCI_IO_START - SZ_2M) >> #define TASK_SIZE_64 (UL(1) << VA_BITS) >> +#define TTBR_CNP_BIT (UL(1) << 0) > > Please move this to arch/arm64/include/asm/pgtable-hwdef.h. That's where > we keep the TCR_* bits as well. > Done. >> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h >> index 3257895a..c8adce2 100644 >> --- a/arch/arm64/include/asm/mmu_context.h >> +++ b/arch/arm64/include/asm/mmu_context.h >> @@ -135,6 +135,17 @@ static inline void cpu_replace_ttbr1(pgd_t *pgd) >> >> phys_addr_t pgd_phys = virt_to_phys(pgd); >> >> + if (system_supports_cnp()) { >> + /* >> + * cpu_replace_ttbr1() is used when there's a boot CPU up >> + * (i.e. cpufeture framework is not up yet) and latter only > > s/cpufeture/cpufeature/ Done. > >> + * when we enable CNP via cpufeature's enable() callback. >> + */ >> + BUG_ON(pgd != swapper_pg_dir); >> + >> + pgd_phys |= TTBR_CNP_BIT; >> + } > > Rather than BUG_ON, can we have: > > if (system_supports_cnp() && pgd == swapper_pg_dir) > > or, if you want to keep the warning: > > if (system_supports_cnp() && !WARN_ON(pgd != swapper_pg_dir)) > > We also seem to rely on the cpu_hwcap bit being set before calling the > enable() function. We need to be careful not to change this, otherwise > the above will break. I kept warning and updated comment to mention cpu_hwcap. > >> @@ -178,6 +189,9 @@ static inline void update_saved_ttbr0(struct task_struct *tsk, >> BUG_ON(mm->pgd == swapper_pg_dir); >> task_thread_info(tsk)->ttbr0 = >> virt_to_phys(mm->pgd) | ASID(mm) << 48; >> + >> + if (system_supports_cnp() && ASID(mm)) >> + task_thread_info(tsk)->ttbr0 |= TTBR_CNP_BIT; >> } >> } >> #else >> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >> index fc0f9eb..14b1799 100644 >> --- a/arch/arm64/include/asm/uaccess.h >> +++ b/arch/arm64/include/asm/uaccess.h >> @@ -107,8 +107,14 @@ static inline void __uaccess_ttbr0_disable(void) >> { >> unsigned long ttbr; >> >> - /* reserved_ttbr0 placed at the end of swapper_pg_dir */ >> - ttbr = read_sysreg(ttbr1_el1) + SWAPPER_DIR_SIZE; >> + /* >> + * reserved_ttbr0 is placed at the end of swapper_pg_dir. >> + * When CNP is in use, TTBR1 may have the CNP bit set, but the >> + * reserved_ttbr should only be used without CNP. >> + */ >> + ttbr = read_sysreg(ttbr1_el1); >> + ttbr &= ~TTBR_CNP_BIT; >> + ttbr += SWAPPER_DIR_SIZE; >> write_sysreg(ttbr, ttbr0_el1); >> isb(); >> } > > As for the asm __uaccess_ttbr0_disable, we probably don't care as hw PAN > is available since ARMv8.1 and CnP is an ARMv8.2 feature. Sow we always > end up with unnecessary code for sw PAN that's only executed where it > doesn't matter. We could check for this at feature detection time (with > a .matches function) or just add a Kconfig line: > > depends on ARM64_PAN || !ARM64_SW_TTBR0_PAN Kconfig updated and unnecessary code gone. > >> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c >> index 095d3c1..1d056f3 100644 >> --- a/arch/arm64/kernel/hibernate.c >> +++ b/arch/arm64/kernel/hibernate.c >> @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size) >> return -EOVERFLOW; >> >> arch_hdr_invariants(&hdr->invariants); >> - hdr->ttbr1_el1 = __pa_symbol(swapper_pg_dir); >> + hdr->ttbr1_el1 = read_sysreg(ttbr1_el1); >> hdr->reenter_kernel = _cpu_resume; >> >> /* We can't use __hyp_get_vectors() because kvm may still be loaded */ > > Are all the CPUs up when coming out of hibernation and restoring > ttbr1_el1? > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S >> index 877d42f..1c94030 100644 >> --- a/arch/arm64/mm/proc.S >> +++ b/arch/arm64/mm/proc.S >> @@ -141,6 +141,11 @@ ENTRY(cpu_do_switch_mm) >> pre_ttbr0_update_workaround x0, x2, x3 >> mmid x1, x1 // get mm->context.id >> bfi x0, x1, #48, #16 // set the ASID >> +alternative_if ARM64_HAS_CNP >> + cbz x1, 1f >> + orr x0, x0, #TTBR_CNP_BIT >> +1: >> +alternative_else_nop_endif > > Some comments here would be useful for future readers (e.g. "do not set > the CnP bit if ASID == 0). > Done Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin 2017-10-09 15:23 ` Catalin Marinas @ 2017-10-10 15:19 ` James Morse 2017-10-11 8:49 ` Vladimir Murzin 1 sibling, 1 reply; 13+ messages in thread From: James Morse @ 2017-10-10 15:19 UTC (permalink / raw) To: linux-arm-kernel Hi Vladimir, On 09/10/17 13:55, Vladimir Murzin wrote: > Common Not Private (CNP) is a feature of ARMv8.2 extension which > allows translation table entries to be shared between different PEs in > the same inner shareable domain, so the hardware can use this fact to > optimise the caching of such entries in the TLB. > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 21e2c95..8d098a1 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1211,6 +1224,14 @@ cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) > return (cpus_have_const_cap(ARM64_HAS_PAN) && !cpus_have_const_cap(ARM64_HAS_UAO)); > } > > +#ifdef CONFIG_ARM64_CNP > +static int cpu_enable_cnp(void *__unused) > +{ > + cpu_replace_ttbr1(swapper_pg_dir); All the other callers of cpu_replace_ttbr1() wrap swapper_pg_dir in lm_alias(). I'm pretty sure this is so that virt_to_phys() works when swapper_pg_dir's address is in the vmalloc range. CONFIG_DEBUG_VIRTUAL should catch problems like this. (Nit: you shouldn't need the #ifdeffery, the only caller of this function is in the same file, so the compiler should do the right thing) > + return 0; > +} > +#endif /* CONFIG_ARM64_CNP */ > + > /* > * We emulate only the following system register space. > * Op0 = 0x3, CRn = 0x0, Op1 = 0x0, CRm = [0, 4 - 7] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm64: mm: Support Common Not Private translations 2017-10-10 15:19 ` James Morse @ 2017-10-11 8:49 ` Vladimir Murzin 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2017-10-11 8:49 UTC (permalink / raw) To: linux-arm-kernel On 10/10/17 16:19, James Morse wrote: > Hi Vladimir, > > On 09/10/17 13:55, Vladimir Murzin wrote: >> Common Not Private (CNP) is a feature of ARMv8.2 extension which >> allows translation table entries to be shared between different PEs in >> the same inner shareable domain, so the hardware can use this fact to >> optimise the caching of such entries in the TLB. > >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index 21e2c95..8d098a1 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -1211,6 +1224,14 @@ cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) >> return (cpus_have_const_cap(ARM64_HAS_PAN) && !cpus_have_const_cap(ARM64_HAS_UAO)); >> } >> >> +#ifdef CONFIG_ARM64_CNP >> +static int cpu_enable_cnp(void *__unused) >> +{ >> + cpu_replace_ttbr1(swapper_pg_dir); > > All the other callers of cpu_replace_ttbr1() wrap swapper_pg_dir in lm_alias(). > I'm pretty sure this is so that virt_to_phys() works when swapper_pg_dir's > address is in the vmalloc range. > > CONFIG_DEBUG_VIRTUAL should catch problems like this. Indeed it did! > > > (Nit: you shouldn't need the #ifdeffery, the only caller of this function is in > the same file, so the compiler should do the right thing) > I'll look into it. Thanks Vladimir > >> + return 0; >> +} >> +#endif /* CONFIG_ARM64_CNP */ >> + >> /* >> * We emulate only the following system register space. >> * Op0 = 0x3, CRn = 0x0, Op1 = 0x0, CRm = [0, 4 - 7] > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] arm64: KVM: Support Common Not Private translations 2017-10-09 12:55 [PATCH 0/3] Support Common Not Private translations Vladimir Murzin 2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin @ 2017-10-09 12:55 ` Vladimir Murzin 2017-10-09 12:55 ` [PATCH 3/3] arm64: Introduce command line parameter to disable CNP Vladimir Murzin 2 siblings, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2017-10-09 12:55 UTC (permalink / raw) To: linux-arm-kernel We rely on cpufeature framework to detect and enable CNP so for KVM we need to patch hyp to set CNP bit just before TTBR0_EL2 gets written. For the guest it is enough to update VTTBR_EL2 with CNP bit just before it gets scheduled. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm64/kvm/hyp-init.S | 4 ++++ arch/arm64/kvm/hyp/switch.c | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S index 3f96155..4fd31b3 100644 --- a/arch/arm64/kvm/hyp-init.S +++ b/arch/arm64/kvm/hyp-init.S @@ -63,6 +63,10 @@ __do_hyp_init: cmp x0, #HVC_STUB_HCALL_NR b.lo __kvm_handle_stub_hvc +alternative_if ARM64_HAS_CNP + orr x0, x0, #TTBR_CNP_BIT +alternative_else_nop_endif + msr ttbr0_el2, x0 mrs x4, tcr_el1 diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 945e79c..a1f3bf2 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -150,7 +150,11 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) static void __hyp_text __activate_vm(struct kvm_vcpu *vcpu) { struct kvm *kvm = kern_hyp_va(vcpu->kvm); - write_sysreg(kvm->arch.vttbr, vttbr_el2); + u64 val = kvm->arch.vttbr; + + if (system_supports_cnp()) + val |= TTBR_CNP_BIT; + write_sysreg(val, vttbr_el2); } static void __hyp_text __deactivate_vm(struct kvm_vcpu *vcpu) -- 2.0.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] arm64: Introduce command line parameter to disable CNP 2017-10-09 12:55 [PATCH 0/3] Support Common Not Private translations Vladimir Murzin 2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin 2017-10-09 12:55 ` [PATCH 2/3] arm64: KVM: " Vladimir Murzin @ 2017-10-09 12:55 ` Vladimir Murzin 2017-10-10 14:36 ` Julien Thierry 2 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2017-10-09 12:55 UTC (permalink / raw) To: linux-arm-kernel There are cases when activating of Common Not Private (CNP) feature might not be desirable; this patch allows to forcefully disable CNP even it is supported by hardware. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- Documentation/admin-guide/kernel-parameters.txt | 4 ++++ arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 0549662..3c1e45d 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -2560,6 +2560,10 @@ noclflush [BUGS=X86] Don't use the CLFLUSH instruction + nocnp [ARM64] + Disable CNP (Common not Private translations) + even if it is supported by processor. + nodelayacct [KNL] Disable per-task delay accounting nodsp [SH] Disable hardware DSP at boot time. diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 8d098a1..724fd93 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -771,6 +771,24 @@ static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry, int _ MIDR_CPU_VAR_REV(1, MIDR_REVISION_MASK)); } +static bool nocnp; + +static int __init early_nocnp(char *p) +{ + nocnp = true; + return 0; +} +early_param("nocnp", early_nocnp); + +static bool has_useable_cnp(const struct arm64_cpu_capabilities *entry, int scope) +{ + if (!has_cpuid_feature(entry, scope)) + return false; + + return nocnp ? false : true; +} + + static bool runs_at_el2(const struct arm64_cpu_capabilities *entry, int __unused) { return is_kernel_in_hyp_mode(); @@ -905,7 +923,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .desc = "Common not Private translations", .capability = ARM64_HAS_CNP, .def_scope = SCOPE_SYSTEM, - .matches = has_cpuid_feature, + .matches = has_useable_cnp, .sys_reg = SYS_ID_AA64MMFR2_EL1, .sign = FTR_UNSIGNED, .field_pos = ID_AA64MMFR2_CNP_SHIFT, -- 2.0.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] arm64: Introduce command line parameter to disable CNP 2017-10-09 12:55 ` [PATCH 3/3] arm64: Introduce command line parameter to disable CNP Vladimir Murzin @ 2017-10-10 14:36 ` Julien Thierry 2017-10-11 8:47 ` Vladimir Murzin 0 siblings, 1 reply; 13+ messages in thread From: Julien Thierry @ 2017-10-10 14:36 UTC (permalink / raw) To: linux-arm-kernel Hi Vladimir, On 09/10/17 13:55, Vladimir Murzin wrote: > There are cases when activating of Common Not Private (CNP) feature > might not be desirable; this patch allows to forcefully disable CNP > even it is supported by hardware. > > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 0549662..3c1e45d 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2560,6 +2560,10 @@ > > noclflush [BUGS=X86] Don't use the CLFLUSH instruction > > + nocnp [ARM64] > + Disable CNP (Common not Private translations) > + even if it is supported by processor. > + > nodelayacct [KNL] Disable per-task delay accounting > > nodsp [SH] Disable hardware DSP at boot time. > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 8d098a1..724fd93 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -771,6 +771,24 @@ static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry, int _ > MIDR_CPU_VAR_REV(1, MIDR_REVISION_MASK)); > } > > +static bool nocnp; > + > +static int __init early_nocnp(char *p) > +{ > + nocnp = true; > + return 0; > +} > +early_param("nocnp", early_nocnp); > + > +static bool has_useable_cnp(const struct arm64_cpu_capabilities *entry, int scope) > +{ > + if (!has_cpuid_feature(entry, scope)) > + return false; > + > + return nocnp ? false : true; This feels a bit odd. Wouldn't the following be better? return !nocnp; Or simply the whole function as: return has_cpuid_feature(entry, scope) && !nocnp; Thanks, > +} > + > + > static bool runs_at_el2(const struct arm64_cpu_capabilities *entry, int __unused) > { > return is_kernel_in_hyp_mode(); > @@ -905,7 +923,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .desc = "Common not Private translations", > .capability = ARM64_HAS_CNP, > .def_scope = SCOPE_SYSTEM, > - .matches = has_cpuid_feature, > + .matches = has_useable_cnp, > .sys_reg = SYS_ID_AA64MMFR2_EL1, > .sign = FTR_UNSIGNED, > .field_pos = ID_AA64MMFR2_CNP_SHIFT, > -- Julien Thierry ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] arm64: Introduce command line parameter to disable CNP 2017-10-10 14:36 ` Julien Thierry @ 2017-10-11 8:47 ` Vladimir Murzin 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2017-10-11 8:47 UTC (permalink / raw) To: linux-arm-kernel Hi Julien, On 10/10/17 15:36, Julien Thierry wrote: > Hi Vladimir, > > On 09/10/17 13:55, Vladimir Murzin wrote: >> There are cases when activating of Common Not Private (CNP) feature >> might not be desirable; this patch allows to forcefully disable CNP >> even it is supported by hardware. >> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> >> --- >> Documentation/admin-guide/kernel-parameters.txt | 4 ++++ >> arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- >> 2 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 0549662..3c1e45d 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -2560,6 +2560,10 @@ >> noclflush [BUGS=X86] Don't use the CLFLUSH instruction >> + nocnp [ARM64] >> + Disable CNP (Common not Private translations) >> + even if it is supported by processor. >> + >> nodelayacct [KNL] Disable per-task delay accounting >> nodsp [SH] Disable hardware DSP at boot time. >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index 8d098a1..724fd93 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -771,6 +771,24 @@ static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry, int _ >> MIDR_CPU_VAR_REV(1, MIDR_REVISION_MASK)); >> } >> +static bool nocnp; >> + >> +static int __init early_nocnp(char *p) >> +{ >> + nocnp = true; >> + return 0; >> +} >> +early_param("nocnp", early_nocnp); >> + >> +static bool has_useable_cnp(const struct arm64_cpu_capabilities *entry, int scope) >> +{ >> + if (!has_cpuid_feature(entry, scope)) >> + return false; >> + >> + return nocnp ? false : true; > > This feels a bit odd. > > Wouldn't the following be better? > return !nocnp; > > Or simply the whole function as: > return has_cpuid_feature(entry, scope) && !nocnp; > I have no strong opinion on that, so I'll change per your suggestion. Cheers Vladimir > Thanks, > >> +} >> + >> + >> static bool runs_at_el2(const struct arm64_cpu_capabilities *entry, int __unused) >> { >> return is_kernel_in_hyp_mode(); >> @@ -905,7 +923,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { >> .desc = "Common not Private translations", >> .capability = ARM64_HAS_CNP, >> .def_scope = SCOPE_SYSTEM, >> - .matches = has_cpuid_feature, >> + .matches = has_useable_cnp, >> .sys_reg = SYS_ID_AA64MMFR2_EL1, >> .sign = FTR_UNSIGNED, >> .field_pos = ID_AA64MMFR2_CNP_SHIFT, >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-10-11 8:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-09 12:55 [PATCH 0/3] Support Common Not Private translations Vladimir Murzin 2017-10-09 12:55 ` [PATCH 1/3] arm64: mm: " Vladimir Murzin 2017-10-09 15:23 ` Catalin Marinas 2017-10-09 16:48 ` James Morse 2017-10-10 12:50 ` Vladimir Murzin 2017-10-10 15:19 ` James Morse 2017-10-10 12:50 ` Vladimir Murzin 2017-10-10 15:19 ` James Morse 2017-10-11 8:49 ` Vladimir Murzin 2017-10-09 12:55 ` [PATCH 2/3] arm64: KVM: " Vladimir Murzin 2017-10-09 12:55 ` [PATCH 3/3] arm64: Introduce command line parameter to disable CNP Vladimir Murzin 2017-10-10 14:36 ` Julien Thierry 2017-10-11 8:47 ` Vladimir Murzin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.