Re: [PATCH v3 21/22] arm64: Delay enabling hardware DBM feature

From: Dave Martin
Date: Fri Feb 09 2018 - 13:58:52 EST


On Fri, Feb 09, 2018 at 05:55:12PM +0000, Suzuki K Poulose wrote:
> We enable hardware DBM bit in a capable CPU, very early in the
> boot via __cpu_setup. This doesn't give us a flexibility of
> optionally disable the feature, as the clearing the bit
> is a bit costly as the TLB can cache the settings. Instead,
> we delay enabling the feature until the CPU is brought up
> into the kernel. We use the feature capability mechanism
> to handle it.
>
> The hardware DBM is a non-conflicting feature. i.e, the kernel
> can safely run with a mix of CPUs with some using the feature
> and the others don't. So, it is safe for a late CPU to have
> this capability and enable it, even if the active CPUs don't.
>
> To get this handled properly by the infrastructure, we
> unconditionally set the capability and only enable it
> on CPUs which really have the feature. Also, we print the
> feature detection from the "matches" call back to make sure
> we don't mislead the user when none of the CPUs could use the
> feature.
>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Dave Martin <dave.martin@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> ---
> Changes since V2
> - Print the feature detection message only when at least one CPU
> is actually using it.
> ---
> arch/arm64/include/asm/cpucaps.h | 3 +-
> arch/arm64/kernel/cpufeature.c | 71 ++++++++++++++++++++++++++++++++++++++++
> arch/arm64/mm/proc.S | 13 ++++----
> 3 files changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index bb263820de13..8df80cc828ac 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -45,7 +45,8 @@
> #define ARM64_HARDEN_BRANCH_PREDICTOR 24
> #define ARM64_HARDEN_BP_POST_GUEST_EXIT 25
> #define ARM64_HAS_RAS_EXTN 26
> +#define ARM64_HW_DBM 27
>
> -#define ARM64_NCAPS 27
> +#define ARM64_NCAPS 28
>
> #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d8663822c604..a96a1f94f427 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -939,6 +939,57 @@ static int __init parse_kpti(char *str)
> __setup("kpti=", parse_kpti);
> #endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
>
> +#ifdef CONFIG_ARM64_HW_AFDBM
> +static inline void __cpu_enable_hw_dbm(void)
> +{
> + u64 tcr = read_sysreg(tcr_el1) | TCR_HD;
> +
> + write_sysreg(tcr, tcr_el1);
> + isb();
> +}
> +
> +static bool cpu_can_use_dbm(const struct arm64_cpu_capabilities *cap)
> +{
> + return has_cpuid_feature(cap, SCOPE_LOCAL_CPU);
> +}
> +
> +static void cpu_enable_hw_dbm(struct arm64_cpu_capabilities const *cap)
> +{
> + if (cpu_can_use_dbm(cap))
> + __cpu_enable_hw_dbm();
> +}
> +
> +static bool has_hw_dbm(const struct arm64_cpu_capabilities *cap,
> + int __unused)
> +{
> + static bool detected = false;
> + /*
> + * DBM is a non-conflicting feature. i.e, the kernel can safely
> + * run a mix of CPUs with and without the feature. So, we
> + * unconditionally enable the capability to allow any late CPU
> + * to use the feature. We only enable the control bits on the
> + * CPU, if it actually supports.
> + *
> + * We have to make sure we print the "feature" detection only
> + * when at least one CPU actually uses it. So check if this CPU
> + * can actually use it and print the message exactly once.
> + *
> + * This is safe as all CPUs (including secondary CPUs - due to the
> + * LOCAL_CPU scope - and the hotplugged CPUs - via verification)
> + * goes through the "matches" check exactly once. Also if a CPU
> + * matches the criteria, it is guaranteed that the CPU will turn
> + * the DBM on, as the capability is unconditionally enabled.
> + */
> + if (!detected && cpu_can_use_dbm(cap)) {
> + detected = true;
> + pr_info("detected feature: Hardware dirty bit management\n");
> + }

Can we just do

if (cpu_can_use_dbm(cap))
pr_info_once(...);

Then we can get rid of "detected".

[...]

Cheers
---Dave