Re: [PATCH] arm64: Add the arm64.nolse_atomics command line option

Marc Zyngier
2023-07-10T07:27:40Z

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.