linux-kernel - Re: [PATCH] powerpc/features: Remove CPU_FTR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wnzuzb1x.fsf@mpe.ellerman.id.au>
Date:   Tue, 13 Oct 2020 21:15:38 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/features: Remove CPU_FTR_NODSISRALIGN

Christophe Leroy <christophe.leroy@...roup.eu> writes:
> Le 13/10/2020 à 09:23, Aneesh Kumar K.V a écrit :
>> Christophe Leroy <christophe.leroy@...roup.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@...roup.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