lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Feb 2020 11:30:44 +0000 From: Suzuki Kuruppassery Poulose <suzuki.poulose@....com> To: Ionela Voinescu <ionela.voinescu@....com>, catalin.marinas@....com, will@...nel.org, mark.rutland@....com, maz@...nel.org, sudeep.holla@....com, lukasz.luba@....com, valentin.schneider@....com, rjw@...ysocki.net Cc: peterz@...radead.org, mingo@...hat.com, vincent.guittot@...aro.org, viresh.kumar@...aro.org, linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org Subject: Re: [PATCH v3 1/7] arm64: add support for the AMU extension v1 Hi Ionela, On 11/02/2020 18:45, Ionela Voinescu wrote: > The activity monitors extension is an optional extension introduced > by the ARMv8.4 CPU architecture. This implements basic support for > version 1 of the activity monitors architecture, AMUv1. > > This support includes: > - Extension detection on each CPU (boot, secondary, hotplugged) > - Register interface for AMU aarch64 registers > - disable_amu kernel parameter to disable detection/counter access > at runtime > > Signed-off-by: Ionela Voinescu <ionela.voinescu@....com> > Cc: Catalin Marinas <catalin.marinas@....com> > Cc: Will Deacon <will@...nel.org> > Cc: Suzuki K Poulose <suzuki.poulose@....com> > Cc: Marc Zyngier <maz@...nel.org> > Cc: Mark Rutland <mark.rutland@....com> > --- > .../admin-guide/kernel-parameters.txt | 10 ++ > arch/arm64/Kconfig | 31 ++++++ > arch/arm64/include/asm/cpucaps.h | 3 +- > arch/arm64/include/asm/cpufeature.h | 5 + > arch/arm64/include/asm/sysreg.h | 38 ++++++++ > arch/arm64/kernel/cpufeature.c | 97 +++++++++++++++++++ > 6 files changed, 183 insertions(+), 1 deletion(-) > ... > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 04cf64e9f0c9..029a473ad273 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -156,6 +156,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV3_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV2_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_DIT_SHIFT, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_AMU_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_SVE), > FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_SVE_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_RAS_SHIFT, 4, 0), > @@ -1150,6 +1151,84 @@ static bool has_hw_dbm(const struct arm64_cpu_capabilities *cap, > > #endif > > +#ifdef CONFIG_ARM64_AMU_EXTN > + > +/* > + * The "amu_cpus" cpumask only signals that the CPU implementation for the > + * flagged CPUs supports the Activity Monitors Unit (AMU) but does not provide > + * information regarding all the events that it supports. When a CPU bit is > + * set in the cpumask, the user of this feature can only rely on the presence > + * of the 4 fixed counters for that CPU. But this does not guarantee that the > + * counters are enabled or access to these counters is enabled by code > + * executed at higher exception levels (firmware). > + */ > +static cpumask_var_t amu_cpus; > + > +bool cpu_has_amu_feat(int cpu) > +{ > + if (cpumask_available(amu_cpus)) > + return cpumask_test_cpu(cpu, amu_cpus); > + > + return false; > +} > + > +static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap) > +{ > + if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) { > + pr_info("detected CPU%d: Activity Monitors Unit (AMU)\n", > + smp_processor_id()); > + cpumask_set_cpu(smp_processor_id(), amu_cpus); > + } > +} > + > +/* > + * For known broken firmware, a kernel parameter ("disable_amu") is provided > + * to ensure access to AMU counter registers is not attempted. By default, > + * the feature is enabled, but disable_amu can both be used to disable or > + * enable the capability at runtime in case the default changes in the future. > + * > + * To be noted that for security considerations, this does not bypass the > + * setting of AMUSERENR_EL0 to trap accesses from EL0 (userspace) to EL1 > + * (kernel). Therefore, firmware should still ensure accesses to AMU registers > + * are not trapped in EL2/EL3. > + */ > +static bool disable_amu; > + > +static int __init set_disable_amu(char *str) > +{ > + int value = 0; > + > + disable_amu = get_option(&str, &value) ? !!value : true; minor nit: You could simply use strtobool(str) here, which accepts: disable_amu= [0/1/on/off/y/n] > + > + return 0; > +} > +early_param("disable_amu", set_disable_amu); > + > +static bool has_amu(const struct arm64_cpu_capabilities *cap, > + int __unused) > +{ > + /* > + * The AMU extension is a non-conflicting feature: the kernel can > + * safely run a mix of CPUs with and without support for the > + * activity monitors extension. Therefore, if not disabled through > + * the kernel command line early parameter, enable the capability > + * to allow any late CPU to use the feature. > + * > + * With this feature enabled, the cpu_enable function will be called > + * for all CPUs that match the criteria, including secondary and > + * hotplugged, marking this feature as present on that respective CPU. > + * The enable function will also print a detection message. > + */ > + > + if (!disable_amu && !zalloc_cpumask_var(&amu_cpus, GFP_KERNEL)) { This looks problematic. Don't we end up in allocating the memory during "each CPU" check and thus leaking memory ? Do we really need to allocate this dynamically ? > + pr_err("Activity Monitors Unit (AMU): fail to allocate memory"); > + disable_amu = true; > + } > + > + return !disable_amu; > +} > +#endif > + > #ifdef CONFIG_ARM64_VHE > static bool runs_at_el2(const struct arm64_cpu_capabilities *entry, int __unused) > { > @@ -1419,6 +1498,24 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .cpu_enable = cpu_clear_disr, > }, > #endif /* CONFIG_ARM64_RAS_EXTN */ > +#ifdef CONFIG_ARM64_AMU_EXTN > + { > + /* > + * The feature is enabled by default if CONFIG_ARM64_AMU_EXTN=y. > + * Therefore, don't provide .desc as we don't want the detection > + * message to be shown until at least one CPU is detected to > + * support the feature. > + */ > + .capability = ARM64_HAS_AMU_EXTN, > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > + .matches = has_amu, > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .sign = FTR_UNSIGNED, > + .field_pos = ID_AA64PFR0_AMU_SHIFT, > + .min_field_value = ID_AA64PFR0_AMU, > + .cpu_enable = cpu_amu_enable, > + }, > +#endif /* CONFIG_ARM64_AMU_EXTN */ > { > .desc = "Data cache clean to the PoU not required for I/D coherence", > .capability = ARM64_HAS_CACHE_IDC, > Thanks Suzuki
Powered by blists - more mailing lists