| 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
| ||
|
Date: Fri, 3 Mar 2023 12:30:36 +0800
From: WANG Xuerui <kernel@...0n.name>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Huacai Chen <chenhuacai@...ngson.cn>,
Arnd Bergmann <arnd@...db.de>, loongarch@...ts.linux.dev,
linux-arch@...r.kernel.org, Xuefeng Li <lixuefeng@...ngson.cn>,
Guo Ren <guoren@...nel.org>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
linux-kernel@...r.kernel.org, loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH] LoongArch: Fix the CRC32 feature probing
On 2023/3/3 11:55, Huacai Chen wrote:
> Hi, Xuerui,
>
> On Fri, Mar 3, 2023 at 11:15 AM WANG Xuerui <kernel@...0n.name> wrote:
>>
>> On 2023/3/3 08:25, Huacai Chen wrote:
>>> Not all LoongArch processors support CRC32 instructions, and this feature
>>> is indicated by CPUCFG1.CRC32 (Bit25). This bit is wrongly defined in loongarch.h
>>
>> The ISA manual suggests it's IOCSR_BRD (likely "IOCSR Branding"). You
>> have to somehow reconcile the two, either by fixing the manuals, or
>> mention here explicitly that the manual is wrong. (Actually thinking
>> about it harder now, you may be in fact re-purposing the IOCSR_BRD field
>> for CRC32 capability, because all LoongArch cores in existence are
>> designed by Loongson, and you may very well know that all cores
>> supporting CRC32 have this bit set, and those not having CRC32 haven't.
>> If that's the case, please explicitly document this reasoning too.)
> The ISA manual has been modified and will be released soon.
I'm wary of Loongson official statements like this but let's see...
meanwhile the commit message and/or comments could be improved
nevertheless, to provide more background.
Also I've just noticed the patch title sounds unnatural too. "Fix
probing of the CRC32 feature" or dropping "the" from the current wording
sounds better IMO.
>>> and CRC32 is set unconditionally now, so fix it.
>>>
>>> BTW, expose the CRC32 feature in /proc/cpuinfo.
>>>
>>> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
>>> ---
>>> arch/loongarch/include/asm/cpu-features.h | 1 +
>>> arch/loongarch/include/asm/cpu.h | 40 ++++++++++++-----------
>>> arch/loongarch/include/asm/loongarch.h | 2 +-
>>> arch/loongarch/kernel/cpu-probe.c | 7 +++-
>>> arch/loongarch/kernel/proc.c | 1 +
>>> 5 files changed, 30 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/loongarch/include/asm/cpu-features.h b/arch/loongarch/include/asm/cpu-features.h
>>> index b07974218393..f6177f133477 100644
>>> --- a/arch/loongarch/include/asm/cpu-features.h
>>> +++ b/arch/loongarch/include/asm/cpu-features.h
>>> @@ -42,6 +42,7 @@
>>> #define cpu_has_fpu cpu_opt(LOONGARCH_CPU_FPU)
>>> #define cpu_has_lsx cpu_opt(LOONGARCH_CPU_LSX)
>>> #define cpu_has_lasx cpu_opt(LOONGARCH_CPU_LASX)
>>> +#define cpu_has_crc32 cpu_opt(LOONGARCH_CPU_CRC32)
>>> #define cpu_has_complex cpu_opt(LOONGARCH_CPU_COMPLEX)
>>> #define cpu_has_crypto cpu_opt(LOONGARCH_CPU_CRYPTO)
>>> #define cpu_has_lvz cpu_opt(LOONGARCH_CPU_LVZ)
>>> diff --git a/arch/loongarch/include/asm/cpu.h b/arch/loongarch/include/asm/cpu.h
>>> index c3da91759472..ca9e2be571ec 100644
>>> --- a/arch/loongarch/include/asm/cpu.h
>>> +++ b/arch/loongarch/include/asm/cpu.h
>>> @@ -78,25 +78,26 @@ enum cpu_type_enum {
>>> #define CPU_FEATURE_FPU 3 /* CPU has FPU */
>>> #define CPU_FEATURE_LSX 4 /* CPU has LSX (128-bit SIMD) */
>>> #define CPU_FEATURE_LASX 5 /* CPU has LASX (256-bit SIMD) */
>>> -#define CPU_FEATURE_COMPLEX 6 /* CPU has Complex instructions */
>>> -#define CPU_FEATURE_CRYPTO 7 /* CPU has Crypto instructions */
>>> -#define CPU_FEATURE_LVZ 8 /* CPU has Virtualization extension */
>>> -#define CPU_FEATURE_LBT_X86 9 /* CPU has X86 Binary Translation */
>>> -#define CPU_FEATURE_LBT_ARM 10 /* CPU has ARM Binary Translation */
>>> -#define CPU_FEATURE_LBT_MIPS 11 /* CPU has MIPS Binary Translation */
>>> -#define CPU_FEATURE_TLB 12 /* CPU has TLB */
>>> -#define CPU_FEATURE_CSR 13 /* CPU has CSR */
>>> -#define CPU_FEATURE_WATCH 14 /* CPU has watchpoint registers */
>>> -#define CPU_FEATURE_VINT 15 /* CPU has vectored interrupts */
>>> -#define CPU_FEATURE_CSRIPI 16 /* CPU has CSR-IPI */
>>> -#define CPU_FEATURE_EXTIOI 17 /* CPU has EXT-IOI */
>>> -#define CPU_FEATURE_PREFETCH 18 /* CPU has prefetch instructions */
>>> -#define CPU_FEATURE_PMP 19 /* CPU has perfermance counter */
>>> -#define CPU_FEATURE_SCALEFREQ 20 /* CPU supports cpufreq scaling */
>>> -#define CPU_FEATURE_FLATMODE 21 /* CPU has flat mode */
>>> -#define CPU_FEATURE_EIODECODE 22 /* CPU has EXTIOI interrupt pin decode mode */
>>> -#define CPU_FEATURE_GUESTID 23 /* CPU has GuestID feature */
>>> -#define CPU_FEATURE_HYPERVISOR 24 /* CPU has hypervisor (running in VM) */
>>> +#define CPU_FEATURE_CRC32 6 /* CPU has Complex instructions */
>>
>> "CPU has CRC32 instructions".
>>
>> Also, the diff damage is real, is there any reason this must come here
>> and not last? To me "aesthetics" is not enough to justify such a diff
>> damage.
> To keep CPU_FEATURE and elf_hwcap in the same order.
IMO they don't have to. You could split into two commits so the mass
reordering could happen separately, making each change nicely focused.
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
Powered by blists - more mailing lists