Re: [RFC PATCH v1 3/5] arm64: Add errata and workarounds for systems with broken BBML2

[Marc Zyngier <maz@kernel.org>]

On Wed, 11 Dec 2024 15:45:04 +0000,
Mikołaj Lenczewski <miko.lenczewski@arm.com> wrote:
> 
> There are systems which claim support for BBML2, but whose
> implementation of this support is broken. Add a Kconfig erratum for each
> of these systems, and a cpufeature workaround that forces the supported
> BBM level on these systems to 0.
> 
> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |  32 ++++
>  arch/arm64/Kconfig                          | 164 ++++++++++++++++++++
>  arch/arm64/kernel/cpufeature.c              |  32 +++-
>  3 files changed, 227 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 100570a048c5..9ef8418e8410 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1127,6 +1127,170 @@ config ARM64_ERRATUM_3194386
>  
>  	  If unsure, say Y.
>  
> +config ARM64_WORKAROUND_BROKEN_BBML2_SUPPORT
> +	bool
> +
> +config ARM64_ERRATUM_3696250
> +	bool "Neoverse-N2: workaround for broken BBM level 2 support"
> +	default y
> +	select ARM64_WORKAROUND_BROKEN_BBML2_SUPPORT
> +	help
> +	  Affected Neoverse-N2 cores (r0p0, r0p1, r0p2, r0p3) declare

So you list a number of affected revisions...

[...]

> +static bool has_bbml2(const struct arm64_cpu_capabilities *entry,
> +		      int scope)
> +{
> +	if (IS_ENABLED(CONFIG_ARM64_WORKAROUND_BROKEN_BBML2_SUPPORT)) {
> +		static const struct midr_range broken_bbml2_list[] = {
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_A76),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_A77),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_A78),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_A78C),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_A710),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_X1),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_X2),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_X3),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_X4),
> +			MIDR_ALL_VERSIONS(MIDR_CORTEX_X925),
> +			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
> +			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
> +			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
> +			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V2),
> +			MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V3),
> +			{}

... and yet you flag all versions as broken? So which one is it? If it
is really the case that all versions are broken, then the text should
be simplified. Otherwise, this should really list the broken versions.

The other thing is that I find it incredibly dangerous to rely on
some config option to disable a feature that will absolutely eat your
data if it is broken. I'd rather see the whole BBM-L2 being behind an
option, and unconditionally check for b0rken CPUs.

Specially when it looks like there isn't a single CPU on the planet
that implemented the feature correctly... :-/

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.