Message-ID: <e6820d63-a8da-4ebb-b078-741ab3fcd262@arm.com>
Date: Wed, 29 Jan 2025 16:43:26 +0000
From: James Morse <james.morse@....com>
To: Douglas Anderson <dianders@...omium.org>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Mark Rutland <mark.rutland@....com>
Cc: Roxana Bradescu <roxabee@...gle.com>, Julius Werner
 <jwerner@...omium.org>, bjorn.andersson@....qualcomm.com,
 Trilok Soni <quic_tsoni@...cinc.com>, linux-arm-msm@...r.kernel.org,
 Florian Fainelli <florian.fainelli@...adcom.com>,
 linux-arm-kernel@...ts.infradead.org, Jeffrey Hugo <quic_jhugo@...cinc.com>,
 Scott Bauer <sbauer@...cinc.com>, stable@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/5] arm64: errata: Assume that unknown CPUs _are_
 vulnerable to Spectre BHB

Hi Doug,

On 07/01/2025 20:05, Douglas Anderson wrote:
> The code for detecting CPUs that are vulnerable to Spectre BHB was
> based on a hardcoded list of CPU IDs that were known to be affected.
> Unfortunately, the list mostly only contained the IDs of standard ARM
> cores. The IDs for many cores that are minor variants of the standard
> ARM cores (like many Qualcomm Kyro CPUs) weren't listed. This led the
> code to assume that those variants were not affected.
> 
> Flip the code on its head and instead assume that a core is vulnerable
> if it doesn't have CSV2_3 but is unrecognized as being safe. This
> involves creating a "Spectre BHB safe" list.
> 
> As of right now, the only CPU IDs added to the "Spectre BHB safe" list
> are ARM Cortex A35, A53, A55, A510, and A520. This list was created by
> looking for cores that weren't listed in ARM's list [1] as per review
> feedback on v2 of this patch [2]. Additionally Brahma A53 is added as
> per mailing list feedback [3].
> 
> NOTE: this patch will not actually _mitigate_ anyone, it will simply
> cause them to report themselves as vulnerable. If any cores in the
> system are reported as vulnerable but not mitigated then the whole
> system will be reported as vulnerable though the system will attempt
> to mitigate with the information it has about the known cores.

> arch/arm64/include/asm/spectre.h |   1 -
> arch/arm64/kernel/proton-pack.c  | 203 ++++++++++++++++---------------
> 2 files changed, 102 insertions(+), 102 deletions(-)

This is a pretty hefty diff-stat for adding a list of six CPUs. It looks like there are
multiple things going on here: I think you're adding the 'safe' list of CPUs, then
removing the list of firmware-mitigated list, then removing some indentation to do the
mitigation detection differently. Any chance this can be split up?
(I'm not sure about the last chunk - it breaks automatic backporting)


> diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
> index e149efadff20..17aa836fe46d 100644
> --- a/arch/arm64/kernel/proton-pack.c
> +++ b/arch/arm64/kernel/proton-pack.c


> +static u8 spectre_bhb_loop_affected(void)
>  {
>  	u8 k = 0;
> -	static u8 max_bhb_k;
> -
> -	if (scope == SCOPE_LOCAL_CPU) {
> -		static const struct midr_range spectre_bhb_k32_list[] = {
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A78),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A78AE),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A78C),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_X1),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A710),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_X2),
> -			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
> -			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
> -			{},
> -		};
> -		static const struct midr_range spectre_bhb_k24_list[] = {
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A76),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A77),
> -			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
> -			MIDR_ALL_VERSIONS(MIDR_QCOM_KRYO_4XX_GOLD),
> -			{},
> -		};
> -		static const struct midr_range spectre_bhb_k11_list[] = {
> -			MIDR_ALL_VERSIONS(MIDR_AMPERE1),
> -			{},
> -		};
> -		static const struct midr_range spectre_bhb_k8_list[] = {
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A72),
> -			MIDR_ALL_VERSIONS(MIDR_CORTEX_A57),
> -			{},
> -		};
> -
> -		if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k32_list))
> -			k = 32;
> -		else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k24_list))
> -			k = 24;
> -		else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k11_list))
> -			k = 11;
> -		else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k8_list))
> -			k =  8;
> -
> -		max_bhb_k = max(max_bhb_k, k);
> -	} else {
> -		k = max_bhb_k;
> -	}
> +
> +	static const struct midr_range spectre_bhb_k32_list[] = {
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A78),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A78AE),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A78C),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_X1),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A710),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_X2),
> +		MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
> +		MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
> +		{},
> +	};
> +	static const struct midr_range spectre_bhb_k24_list[] = {
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A76),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A77),
> +		MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
> +		MIDR_ALL_VERSIONS(MIDR_QCOM_KRYO_4XX_GOLD),
> +		{},
> +	};
> +	static const struct midr_range spectre_bhb_k11_list[] = {
> +		MIDR_ALL_VERSIONS(MIDR_AMPERE1),
> +		{},
> +	};
> +	static const struct midr_range spectre_bhb_k8_list[] = {
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A72),
> +		MIDR_ALL_VERSIONS(MIDR_CORTEX_A57),
> +		{},
> +	};
> +
> +	if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k32_list))
> +		k = 32;
> +	else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k24_list))
> +		k = 24;
> +	else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k11_list))
> +		k = 11;
> +	else if (is_midr_in_range_list(read_cpuid_id(), spectre_bhb_k8_list))
> +		k =  8;
>  
>  	return k;
>  }

This previous hunk reduces the indent to remove the static variable from inside the
function. Your patch-1 can be picked up automatically by stable branches, but after this
change, that will have to be done by hand. Arm have recently updated that table of CPUs
with extra entries (thanks for picking those up!) - but now that patch can't be easily
applied to older kernels.
I suspect making the reporting assuming-vulnerable may make other CPUs come out of the
wood work too...

Could we avoid changing this unless we really need to?


As background, the idea is that CPUs that are newer than the kernel will discover the need
for mitigation from firmware. That sucks for performance, but it can be improved once the
kernel is updated to know about the 'local' workaround.


> @@ -955,6 +956,8 @@ static bool supports_ecbhb(int scope)
>  						    ID_AA64MMFR1_EL1_ECBHB_SHIFT);
>  }
>  
> +static u8 max_bhb_k;
> +
>  bool is_spectre_bhb_affected(const struct arm64_cpu_capabilities *entry,
>  			     int scope)
>  {
> @@ -963,16 +966,18 @@ bool is_spectre_bhb_affected(const struct arm64_cpu_capabilities *entry,
>  	if (supports_csv2p3(scope))
>  		return false;
>  
> -	if (supports_clearbhb(scope))
> -		return true;
> -
> -	if (spectre_bhb_loop_affected(scope))
> -		return true;
> +	if (is_spectre_bhb_safe(scope))
> +		return false;
>  
> -	if (is_spectre_bhb_fw_affected(scope))
> -		return true;
> +	/*
> +	 * At this point the core isn't known to be "safe" so we're going to
> +	 * assume it's vulnerable. We still need to update `max_bhb_k` though,
> +	 * but only if we aren't mitigating with clearbhb though.

+	or firmware.


> +	 */
> +	if (scope == SCOPE_LOCAL_CPU && !supports_clearbhb(SCOPE_LOCAL_CPU))
> +		max_bhb_k = max(max_bhb_k, spectre_bhb_loop_affected());

CPUs that need a firmware mitigation will get in here too. Its probably harmless as
they'll report '0' as their k value... but you went to the trouble to weed out the CPUs
that support the clearbhb instruction ...

For the change described in the commit message, isn't it enough to replace the final
'return false' with 'return !is_spectre_bhb_safe(scope)' ?


>  
> -	return false;
> +	return true;
>  }


Thanks,

James

Powered by blists - more mailing lists