linux-kernel - Re: [PATCH] x86/crc32: use builtins to improve code generation

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: <68F134A3-5371-4E74-9942-2BC66984C18A@zytor.com>
Date: Thu, 27 Feb 2025 02:52:44 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: Eric Biggers <ebiggers@...nel.org>, Bill Wendling <morbo@...gle.com>
CC: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
        Justin Stitt <justinstitt@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>, linux-crypto@...r.kernel.org,
        clang-built-linux <llvm@...ts.linux.dev>
Subject: Re: [PATCH] x86/crc32: use builtins to improve code generation

On February 26, 2025 10:28:59 PM PST, Eric Biggers <ebiggers@...nel.org> wrote:
>On Wed, Feb 26, 2025 at 10:12:47PM -0800, Bill Wendling wrote:
>> For both gcc and clang, crc32 builtins generate better code than the
>> inline asm. GCC improves, removing unneeded "mov" instructions. Clang
>> does the same and unrolls the loops. GCC has no changes on i386, but
>> Clang's code generation is vastly improved, due to Clang's "rm"
>> constraint issue.
>> 
>> The number of cycles improved by ~0.1% for GCC and ~1% for Clang, which
>> is expected because of the "rm" issue. However, Clang's performance is
>> better than GCC's by ~1.5%, most likely due to loop unrolling.
>> 
>> Link: https://github.com/llvm/llvm-project/issues/20571#issuecomment-2649330009
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Borislav Petkov <bp@...en8.de>
>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>> Cc: x86@...nel.org
>> Cc: "H. Peter Anvin" <hpa@...or.com>
>> Cc: Eric Biggers <ebiggers@...nel.org>
>> Cc: Ard Biesheuvel <ardb@...nel.org>
>> Cc: Nathan Chancellor <nathan@...nel.org>
>> Cc: Nick Desaulniers <nick.desaulniers+lkml@...il.com>
>> Cc: Justin Stitt <justinstitt@...gle.com>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: linux-crypto@...r.kernel.org
>> Cc: llvm@...ts.linux.dev
>> Signed-off-by: Bill Wendling <morbo@...gle.com>
>> ---
>>  arch/x86/Makefile         | 3 +++
>>  arch/x86/lib/crc32-glue.c | 8 ++++----
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>
>Thanks!  A couple concerns, though:
>
>> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
>> index 5b773b34768d..241436da1473 100644
>> --- a/arch/x86/Makefile
>> +++ b/arch/x86/Makefile
>> @@ -114,6 +114,9 @@ else
>>  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none)
>>  endif
>> 
>> +# Enables the use of CRC32 builtins.
>> +KBUILD_CFLAGS += -mcrc32
>
>Doesn't this technically allow the compiler to insert CRC32 instructions
>anywhere in arch/x86/ without the needed runtime CPU feature check?  Normally
>when using intrinsics it's necessary to limit the scope of the feature
>enablement to match the runtime CPU feature check that is done, e.g. by using
>the target function attribute.
>
>> diff --git a/arch/x86/lib/crc32-glue.c b/arch/x86/lib/crc32-glue.c
>> index 2dd18a886ded..fdb94bff25f4 100644
>> --- a/arch/x86/lib/crc32-glue.c
>> +++ b/arch/x86/lib/crc32-glue.c
>> @@ -48,9 +48,9 @@ u32 crc32_le_arch(u32 crc, const u8 *p, size_t len)
>>  EXPORT_SYMBOL(crc32_le_arch);
>> 
>>  #ifdef CONFIG_X86_64
>> -#define CRC32_INST "crc32q %1, %q0"
>> +#define CRC32_INST __builtin_ia32_crc32di
>>  #else
>> -#define CRC32_INST "crc32l %1, %0"
>> +#define CRC32_INST __builtin_ia32_crc32si
>>  #endif
>
>Do both gcc and clang consider these builtins to be a stable API, or do they
>only guarantee the stability of _mm_crc32_*() from immintrin.h?  At least for
>the rest of the SSE and AVX stuff, I thought that only the immintrin.h functions
>are actually considered stable.
>
>- Eric

There is that... also are there compiler versions that we support that do not have -mcrc32 support?