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;
Message-ID: <32f442e3-3d5c-4cec-9791-0da039f88287@quicinc.com>
Date:   Mon, 10 Jul 2023 16:19:54 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH] arm64: Add the arm64.nolse_atomics command line option
To:     Marc Zyngier <maz@kernel.org>
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>
References: <20230710055955.36551-1-quic_aiquny@quicinc.com>
 <875y6s8bwb.wl-maz@kernel.org>
From:   "Aiqun(Maria) Yu" <quic_aiquny@quicinc.com>
In-Reply-To: <875y6s8bwb.wl-maz@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/10/2023 3:27 PM, Marc Zyngier wrote:
> 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).
> 

good idea. A noc error would be happened if the lse atomic instruction 
happened during a memory controller doesn't support lse atomic instructions.
I can put the information in next patchset comment message. Pls feel 
free to let know if there is other place to have this kind of 
information with.
>>
>> 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.

noatomic can be used in next patchset.
ID_AA64ISAR0_EL1_ATOMIC_SHIFT
> 
>>   	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.
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu