Re: [PATCH] powerpc/features: Remove CPU_FTR_NODSISRALIGN

From: Michael Ellerman <mpe@ellerman.id.au>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powerpc/features: Remove CPU_FTR_NODSISRALIGN
Date: Wed, 14 Oct 2020 22:00:29 +1100	[thread overview]
Message-ID: <87ft6hysvm.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <7b1f7fdd-0256-3370-13ab-d808b9fe9b02@linux.ibm.com>

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 10/13/20 3:45 PM, Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 13/10/2020 à 09:23, Aneesh Kumar K.V a écrit :
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>
>>>>> CPU_FTR_NODSISRALIGN has not been used since
>>>>> commit 31bfdb036f12 ("powerpc: Use instruction emulation
>>>>> infrastructure to handle alignment faults")
>>>>>
>>>>> Remove it.
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>> ---
>>>>>    arch/powerpc/include/asm/cputable.h | 22 ++++++++++------------
>>>>>    arch/powerpc/kernel/dt_cpu_ftrs.c   |  8 --------
>>>>>    arch/powerpc/kernel/prom.c          |  2 +-
>>>>>    3 files changed, 11 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>>>>> index 1098863e17ee..c598961d9f15 100644
>>>>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>>>>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>>>>> @@ -273,13 +273,6 @@ static int __init feat_enable_idle_nap(struct dt_cpu_feature *f)
>>>>>    	return 1;
>>>>>    }
>>>>>    
>>>>> -static int __init feat_enable_align_dsisr(struct dt_cpu_feature *f)
>>>>> -{
>>>>> -	cur_cpu_spec->cpu_features &= ~CPU_FTR_NODSISRALIGN;
>>>>> -
>>>>> -	return 1;
>>>>> -}
>>>>> -
>>>>>    static int __init feat_enable_idle_stop(struct dt_cpu_feature *f)
>>>>>    {
>>>>>    	u64 lpcr;
>>>>> @@ -641,7 +634,6 @@ static struct dt_cpu_feature_match __initdata
>>>>>    	{"tm-suspend-hypervisor-assist", feat_enable, CPU_FTR_P9_TM_HV_ASSIST},
>>>>>    	{"tm-suspend-xer-so-bug", feat_enable, CPU_FTR_P9_TM_XER_SO_BUG},
>>>>>    	{"idle-nap", feat_enable_idle_nap, 0},
>>>>> -	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
>> 
>> Rather than removing it entirely, I'd rather we left a comment, so that
>> it's obvious that we are ignoring that feature on purpose, not because
>> we forget about it.
>> 
>> eg:
>> 
>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> index f204ad79b6b5..45cb7e59bd13 100644
>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> @@ -640,7 +640,7 @@ static struct dt_cpu_feature_match __initdata
>>   	{"tm-suspend-hypervisor-assist", feat_enable, CPU_FTR_P9_TM_HV_ASSIST},
>>   	{"tm-suspend-xer-so-bug", feat_enable, CPU_FTR_P9_TM_XER_SO_BUG},
>>   	{"idle-nap", feat_enable_idle_nap, 0},
>> -	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
>> +	// "alignment-interrupt-dsisr" ignored
>>   	{"idle-stop", feat_enable_idle_stop, 0},
>>   	{"machine-check-power8", feat_enable_mce_power8, 0},
>>   	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
>> 
>
>
> why not do it as
> static int __init feat_enable_align_dsisr(struct dt_cpu_feature *f)
> {
> 	/* This feature should not be enabled */
> #ifdef DEBUG
> 	WARN(1);
> #endif
>
> 	return 1;
> }

No one will ever turn that #define on.
No one will ever turn the feature on either.
Even if they did, it's not a bug because the kernel doesn't use the
DSISR for alignment interrupts any more.

All I want is to be able to compare the list of features defined in
skiboot vs the ones in Linux and see that none are missing in Linux.

cheers