lkml.org 
[lkml]   [2020]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 1/7] arm64: add support for the AMU extension v1
    From
    Date
    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@arm.com>
    > Cc: Catalin Marinas <catalin.marinas@arm.com>
    > Cc: Will Deacon <will@kernel.org>
    > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
    > Cc: Marc Zyngier <maz@kernel.org>
    > Cc: Mark Rutland <mark.rutland@arm.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

    \
     
     \ /
      Last update: 2020-02-12 12:31    [W:3.680 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site