Re: [PATCH] powerpc/features: Remove CPU_FTR_NODSISRALIGN

[Michael Ellerman <mpe@ellerman.id.au>]

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},


>>>   	{"idle-stop", feat_enable_idle_stop, 0},
>>>   	{"machine-check-power8", feat_enable_mce_power8, 0},
>>>   	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
>>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>>> index c1545f22c077..a5a5acb627fe 100644
>>> --- a/arch/powerpc/kernel/prom.c
>>> +++ b/arch/powerpc/kernel/prom.c
>>> @@ -165,7 +165,7 @@ static struct ibm_pa_feature {
>>>   #ifdef CONFIG_PPC_RADIX_MMU
>>>   	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX | MMU_FTR_GTSE },
>>>   #endif
>>> -	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features = CPU_FTR_NODSISRALIGN },
>>> +	{ .pabyte = 1,  .pabit = 1, .invert = 1, },
>>>   	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
>>>   				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
>> 
>> I didn't follow this change. Should the line be dropped?
>> 
>
> Don't know. I have to look closer, I don't know what it is used for.

All it does is clear the CPU feature if firmware tells us to. So if
we're dropping the CPU feature we can drop the whole entry in the
feature array.

cheers