Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Jul 2023 08:27:00 +0100
Message-ID: <875y6s8bwb.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Maria Yu <quic_aiquny@quicinc.com>
Cc:     <will@kernel.org>, <corbet@lwn.net>, <catalin.marinas@arm.com>,
        <quic_pkondeti@quicinc.com>, <quic_kaushalk@quicinc.com>,
        <quic_satyap@quicinc.com>, <quic_shashim@quicinc.com>,
        <quic_songxue@quicinc.com>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] arm64: Add the arm64.nolse_atomics command line option
In-Reply-To: <20230710055955.36551-1-quic_aiquny@quicinc.com>
References: <20230710055955.36551-1-quic_aiquny@quicinc.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 10 Jul 2023 06:59:55 +0100,
Maria Yu <quic_aiquny@quicinc.com> wrote:
> 
> In order to be able to disable lse_atomic even if cpu
> support it, most likely because of memory controller
> cannot deal with the lse atomic instructions, use a
> new idreg override to deal with it.

In general, the idreg overrides are *not* there to paper over HW bugs.
They are there to force the kernel to use or disable a feature for
performance reason or to guide the *enabling* of a feature, but not
because the HW is broken.

The broken status of a HW platform must also be documented so that we
know what to expect when we look at, for example, a bad case of memory
corruption (something I'd expect to see on a system that only
partially implements atomic memory operations).

> 
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  arch/arm64/include/asm/cpufeature.h             |  1 +
>  arch/arm64/kernel/cpufeature.c                  |  4 +++-
>  arch/arm64/kernel/idreg-override.c              | 11 +++++++++++
>  4 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 85fb0fa5d091..6ad754549f1d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -433,6 +433,8 @@
>  	arm64.nomops	[ARM64] Unconditionally disable Memory Copy and Memory
>  			Set instructions support
>  
> +	arm64.nolse_atomic [ARM64] Unconditionally disable LSE Atomic support
> +

'nolse', or 'noatomic' should be enough. In general, the suffix should
be either derived from the FEAT_* name or the idreg field name.

>  	ataflop=	[HW,M68k]
>  
>  	atarimouse=	[HW,MOUSE] Atari Mouse
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 96e50227f940..9d56dea1fe62 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -916,6 +916,7 @@ extern struct arm64_ftr_override id_aa64pfr0_override;
>  extern struct arm64_ftr_override id_aa64pfr1_override;
>  extern struct arm64_ftr_override id_aa64zfr0_override;
>  extern struct arm64_ftr_override id_aa64smfr0_override;
> +extern struct arm64_ftr_override id_aa64isar0_override;
>  extern struct arm64_ftr_override id_aa64isar1_override;
>  extern struct arm64_ftr_override id_aa64isar2_override;
>  
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index f9d456fe132d..9bd766880807 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -669,6 +669,7 @@ struct arm64_ftr_override __ro_after_init id_aa64pfr0_override;
>  struct arm64_ftr_override __ro_after_init id_aa64pfr1_override;
>  struct arm64_ftr_override __ro_after_init id_aa64zfr0_override;
>  struct arm64_ftr_override __ro_after_init id_aa64smfr0_override;
> +struct arm64_ftr_override __ro_after_init id_aa64isar0_override;
>  struct arm64_ftr_override __ro_after_init id_aa64isar1_override;
>  struct arm64_ftr_override __ro_after_init id_aa64isar2_override;
>  
> @@ -721,7 +722,8 @@ static const struct __ftr_reg_entry {
>  	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_raz),
>  
>  	/* Op1 = 0, CRn = 0, CRm = 6 */
> -	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
> +	ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0,
> +			       &id_aa64isar0_override),
>  	ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64ISAR1_EL1, ftr_id_aa64isar1,
>  			       &id_aa64isar1_override),
>  	ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64ISAR2_EL1, ftr_id_aa64isar2,
> diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> index 2fe2491b692c..af41ab4f3d94 100644
> --- a/arch/arm64/kernel/idreg-override.c
> +++ b/arch/arm64/kernel/idreg-override.c
> @@ -105,6 +105,15 @@ static const struct ftr_set_desc pfr1 __initconst = {
>  	},
>  };
>  
> +static const struct ftr_set_desc isar0 __initconst = {
> +	.name		= "id_aa64isar0",
> +	.override	= &id_aa64isar0_override,
> +	.fields		= {
> +	        FIELD("atomic", ID_AA64ISAR0_EL1_ATOMIC_SHIFT, NULL),
> +		{}
> +	},
> +};
> +
>  static const struct ftr_set_desc isar1 __initconst = {
>  	.name		= "id_aa64isar1",
>  	.override	= &id_aa64isar1_override,
> @@ -163,6 +172,7 @@ static const struct ftr_set_desc * const regs[] __initconst = {
>  	&mmfr1,
>  	&pfr0,
>  	&pfr1,
> +	&isar0,
>  	&isar1,
>  	&isar2,
>  	&smfr0,
> @@ -185,6 +195,7 @@ static const struct {
>  	{ "arm64.nomops",		"id_aa64isar2.mops=0" },
>  	{ "arm64.nomte",		"id_aa64pfr1.mte=0" },
>  	{ "nokaslr",			"arm64_sw.nokaslr=1" },
> +	{ "arm64.nolse_atomic",         "id_aa64isar0.atomic=0" },

And what of 32bit?

	M.

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