Improve SSE4.1/AES-NI support #644

tbvdm · 2024-03-31T07:29:12Z

The CryptX Perl module contains a vendored copy of libtomcrypt. It uses -msse4.1 -maes to enable AES-NI support. The problem is that these flags are used for all source files. This results in SIGILL crashes on CPUs without SSE4.1.

I think the solution is for CryptX to use -msse4.1 -maes only for aesni.c. But this is not possible without several changes to libtomcrypt. This pull request is a proposal for those changes.

The primary change is in how SSE4.1 is detected. Currently libtomcrypt checks __SSE4_1__. But this requires that -msse4.1 is used for multiple source files. Moreover, it's a compile-time check, not a run-time one. I think it would be better to detect SSE4.1 with cpuid.

Then there is the build procedure. I came up with the following approach. By default, AES-NI support is disabled. To enable it, add -DLTC_AES_NI to CFLAGS and set the required compiler flags in CFLAGS_AES_NI (typically -maes -msse4.1). For example:

make -f makefile.unix CFLAGS=-DLTC_AES_NI CFLAGS_AES_NI="-maes -msse4.1"

Checklist

documentation is added or updated
tests are added or updated

Closes #641

tbvdm · 2024-03-31T08:04:28Z

Maybe I should cc @karel-m

sjaeckel · 2024-04-02T22:35:28Z

Did you try the proposed change of #641 and whether this maybe fixes it already?

tbvdm · 2024-04-03T19:15:49Z

#641 doesn't help, unfortunately. Here is a crash from the fix-perl-cryptx-99 branch on a machine without SSE4.1 (running OpenBSD):

$ git clone -b fix-perl-cryptx-99 https://github.com/libtom/libtomcrypt.git
$ cd libtomcrypt
$ make -f makefile.unix CFLAGS="-maes -msse4.1"
$ cd ..
$ cat test.c
#include <tomcrypt.h>

int main(void)
{
	register_cipher(&aes_desc);
	gcm_test();
}
$ cc -I libtomcrypt/src/headers libtomcrypt/libtomcrypt.a test.c
$ ./a.out
Illegal instruction (core dumped)
$ egdb -q a.out a.out.core
Reading symbols from a.out...
(No debugging symbols found in a.out)
[New process 186382]
Core was generated by `a.out'.
Program terminated with signal SIGILL, Illegal instruction.
#0  0x00000a534708a0f0 in gcm_test ()
(gdb) x/i $pc
=> 0xa534708a0f0 <gcm_test+1696>:	ptest  %xmm0,%xmm0

sjaeckel · 2024-06-12T08:41:46Z

Sorry for the long delay.

#641 doesn't help, unfortunately. Here is a crash from the fix-perl-cryptx-99 branch on a machine without SSE4.1 (running OpenBSD):

$ git clone -b fix-perl-cryptx-99 https://github.com/libtom/libtomcrypt.git
$ cd libtomcrypt
$ make -f makefile.unix CFLAGS="-maes -msse4.1"
$ cd ..
$ cat test.c
#include <tomcrypt.h>

int main(void)
{
	register_cipher(&aes_desc);
	gcm_test();
}
$ cc -I libtomcrypt/src/headers libtomcrypt/libtomcrypt.a test.c
$ ./a.out
Illegal instruction (core dumped)
$ egdb -q a.out a.out.core
Reading symbols from a.out...
(No debugging symbols found in a.out)
[New process 186382]
Core was generated by `a.out'.
Program terminated with signal SIGILL, Illegal instruction.
#0  0x00000a534708a0f0 in gcm_test ()
(gdb) x/i $pc
=> 0xa534708a0f0 <gcm_test+1696>:	ptest  %xmm0,%xmm0

FMU that's a different problem. You can also see that it fails in gcm_test() and not in AES related code.

A valid test-case for AES-NI would be to first configure&build the library as you did and then execute this:

#include <tomcrypt.h>

int main(void)
{
	aes_test();
}

Regarding what you're seeing: You're explicitly enabling SSE4.1, which allows the compiler to generate SSE-specific instructions.

WARNING: I checked the disassembly in the past and was too lazy to re-check now, so I hope the following statement is still correct :) Please correct me if I'm wrong!

The following code comes from an era when compilers were not yet that smart:

libtomcrypt/src/encauth/gcm/gcm_mult_h.c

Lines 19 to 41 in f7e6519

    
           #ifdef LTC_GCM_TABLES 
        
              int x; 
        
           #ifdef LTC_GCM_TABLES_SSE2 
        
              asm("movdqa (%0),%%xmm0"::"r"(&gcm->PC[0][I[0]][0])); 
        
              for (x = 1; x < 16; x++) { 
        
                 asm("pxor (%0),%%xmm0"::"r"(&gcm->PC[x][I[x]][0])); 
        
              } 
        
              asm("movdqa %%xmm0,(%0)"::"r"(&T)); 
        
           #else 
        
              int y; 
        
              XMEMCPY(T, &gcm->PC[0][I[0]][0], 16); 
        
              for (x = 1; x < 16; x++) { 
        
           #ifdef LTC_FAST 
        
                  for (y = 0; y < 16; y += sizeof(LTC_FAST_TYPE)) { 
        
                      *(LTC_FAST_TYPE_PTR_CAST(T + y)) ^= *(LTC_FAST_TYPE_PTR_CAST(&gcm->PC[x][I[x]][y])); 
        
                  } 
        
           #else 
        
                  for (y = 0; y < 16; y++) { 
        
                      T[y] ^= gcm->PC[x][I[x]][y]; 
        
                  } 
        
           #endif /* LTC_FAST */ 
        
              } 
        
           #endif /* LTC_GCM_TABLES_SSE2 */

By enabling -msse4.1 at compile time you did not enable that "assembly optimized path" in the source code (via the define etc.), but the compilers got so smart that they generate exactly those assembly instructions from the "default code path" (even when LTC_GCM_TABLES_SSE2 is not defined).

You should also be able to reproduce this failure with the latest release (master branch) that has no AES-NI support yet.

Maybe also try to add debug information when building the library to see where exactly it breaks? make -f makefile.unix CFLAGS="-maes -msse4.1 -g3"

Yes, these details are only slightly related to the error you see, but should show you the problem.

sjaeckel · 2024-06-12T08:54:42Z

And also you're right with this PR... now I understand the purpose :)

With this we have now a "proper" runtime check for AES-NI. It allows to build the rest of the library for all amd64 (also non SSE) and only requires the AES-NI related parts to be built with SSE4.1 support.

sjaeckel

LGTM

README.md

tbvdm · 2024-06-24T19:25:56Z

Sorry for the long delay.

No worries. Thanks for looking at this.

FMU that's a different problem. You can also see that it fails in gcm_test() and not in AES related code.

[...]

Regarding what you're seeing: You're explicitly enabling SSE4.1, which allows the compiler to generate SSE-specific instructions.

That's precisely the point: currently, if you want to enable AES-NI support, you need to enable SSE4.1 unconditionally for the whole source tree.

This is especially relevant when building binary packages. The same binary package should (ideally) be able to run different CPUs with different capabilities.

And also you're right with this PR... now I understand the purpose :)

With this we have now a "proper" runtime check for AES-NI. It allows to build the rest of the library for all amd64 (also non SSE) and only requires the AES-NI related parts to be built with SSE4.1 support.

Yes, exactly. :)

tbvdm · 2024-06-24T19:29:23Z

I've force-pushed a new commit that includes the change you requested.

sjaeckel

Uhm ... I've just had a third look at this and wouldn't it even make sense to compile aesni.c always with those flags? ... but then we'd need some kind of configure step which determines the target architecture ... What do you think?

Also I've only now realized that there's no CMake support for this feature, that's why I haven't merged it yet.

tbvdm · 2024-06-29T18:09:24Z

Why don't we use target attributes? See the new commit. This avoids the need for flags altogether. You only have to make sure that LTC_AES_NI is defined. For example:

make -f makefile.unix CFLAGS=-DLTC_AES_NI

I defined __attribute__ away for MSVC. Also, MSVC does not need flags or attributes to enable SSE4.1 and AES intrinsics.

(However, I don't think MSVC will work anyway, because apparently it does not support inline assembly on x64. So the cpuid call will fail. I think the solution is to call cpuid through an intrinsic.)

levitte

ltc's source checker in helper.pl doesn't like it when you redefine compiler specific symbols. It may be better to make an LTC macro for what you want, like this.

src/ciphers/aes/aesni.c

tbvdm · 2024-08-31T12:54:30Z

I've pushed a new commit that is rebased on current develop, squashes the two commits in this PR, and includes @levitte's suggestions.

sjaeckel · 2024-08-31T13:14:26Z

src/ciphers/aes/aesni.c

+#if defined(__GNUC__) || defined(__clang__)
+#define LTC_ATTRIBUTE(x) __attribute__(x)
+#else
+#define LTC_ATTRIBUTE(x)
+#endif


Could you please directly put this in tomcrypt_cfg.h?

sjaeckel

Thanks a lot for the improvements on this and thanks for the PR. Looks very good now.

(and please rebase on top of current develop :) )

tbvdm · 2024-09-01T17:11:09Z

Done!

sjaeckel · 2024-09-01T17:43:22Z

This also supersedes #641 right?

tbvdm · 2024-09-01T19:39:39Z

Yes, I believe it does.

sjaeckel · 2024-09-02T06:52:23Z

Before merging this I have one more question:

Wouldn't it now make sense to enable this by default on x86/amd64?
If yes, we should change the compile-time-switch to disable if not desired.

Btw. thanks (and sorry) @tbvdm for doing this lengthy exercise with us ;)

karel-m · 2024-09-02T08:51:57Z

@tbvdm could you please rebase this PR's branch on top of current develop? I will try to make a dev release of CryptX perl bindings with this patch to check what will happen on cpantesters & co.

sjaeckel · 2024-09-02T09:20:32Z

Wouldn't it now make sense to enable this by default on x86/amd64?

@karel-m @levitte I'd also like your opinion on this

karel-m · 2024-09-02T10:31:52Z

Wouldn't it now make sense to enable this by default on x86/amd64?

I do not have a strong opinion on this but I would rather keep it off by default,

According to my experiance you basically need:

to found out (or guess) whether your compiler suupports AESNI (in my CryptX I turn it on only for gcc/clang/llvm v5+)
than the compiler needs a special options e.g. -msse4.1 -maes for gcc/clang/llvm (OK, perhaps using target attributes might help here, but I'm not sure how it will work on older or less common compilers)
on top of that you will simply pass also -DLTC_AES_NI

levitte · 2024-09-02T10:34:47Z

I think I'm leaning on the cautious side re the default. @karel-m said it a lot better

sjaeckel · 2024-09-02T11:57:10Z

Alright, then we'll stay away from the danger zone and leave it off by default ;)

@tbvdm I took the liberty to rebase and force-push your branch, I hope that's OK

sjaeckel · 2024-09-02T13:11:37Z

I guess we forgot to enable this in the CI build ... ;)

libtomcrypt/.github/workflows/main.yml

Line 41 in ce904c8

    
           - { BUILDNAME: 'STOCK+AESNI',             BUILDOPTIONS: '-msse4.1 -maes',                                                       BUILDSCRIPT: '.ci/run.sh' }

tbvdm · 2024-09-02T20:02:11Z

Thanks for merging this. :)

Yes, it seems I missed main.yml when I grepped the tree for relevant files.

sjaeckel self-requested a review April 2, 2024 22:34

sjaeckel self-assigned this Apr 2, 2024

This comment was marked as spam.

Sign in to view

sjaeckel requested changes Jun 12, 2024

View reviewed changes

README.md Show resolved Hide resolved

tbvdm force-pushed the aesni branch from 69d6df3 to 3617882 Compare June 24, 2024 19:27

sjaeckel reviewed Jun 26, 2024

View reviewed changes

karel-m mentioned this pull request Jul 6, 2024

AESNI tracking ticket DCIT/perl-CryptX#106

Closed

levitte requested changes Aug 29, 2024

View reviewed changes

src/ciphers/aes/aesni.c Outdated Show resolved Hide resolved

src/ciphers/aes/aesni.c Outdated Show resolved Hide resolved

src/ciphers/aes/aesni.c Outdated Show resolved Hide resolved

src/ciphers/aes/aesni.c Outdated Show resolved Hide resolved

tbvdm force-pushed the aesni branch from 4b0c860 to e9fabc3 Compare August 31, 2024 12:48

sjaeckel reviewed Aug 31, 2024

View reviewed changes

sjaeckel requested changes Aug 31, 2024

View reviewed changes

tbvdm force-pushed the aesni branch from e9fabc3 to 0c6a551 Compare September 1, 2024 17:09

sjaeckel approved these changes Sep 1, 2024

View reviewed changes

sjaeckel requested review from karel-m and levitte September 2, 2024 06:52

Improve SSE4.1/AES-NI support

33d1c81

sjaeckel force-pushed the aesni branch from 0c6a551 to 33d1c81 Compare September 2, 2024 11:55

sjaeckel merged commit ce904c8 into libtom:develop Sep 2, 2024
75 checks passed

sjaeckel mentioned this pull request Sep 3, 2024

Fix AESNI build job #666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve SSE4.1/AES-NI support #644

Improve SSE4.1/AES-NI support #644

tbvdm commented Mar 31, 2024 •

edited by sjaeckel

Loading

tbvdm commented Mar 31, 2024

sjaeckel commented Apr 2, 2024

tbvdm commented Apr 3, 2024

This comment was marked as spam.

sjaeckel commented Jun 12, 2024 •

edited

Loading

sjaeckel commented Jun 12, 2024 •

edited

Loading

sjaeckel left a comment

tbvdm commented Jun 24, 2024

tbvdm commented Jun 24, 2024

sjaeckel left a comment

tbvdm commented Jun 29, 2024

levitte left a comment

tbvdm commented Aug 31, 2024

sjaeckel Aug 31, 2024 •

edited

Loading

sjaeckel left a comment •

edited

Loading

tbvdm commented Sep 1, 2024

sjaeckel commented Sep 1, 2024

tbvdm commented Sep 1, 2024

sjaeckel commented Sep 2, 2024

karel-m commented Sep 2, 2024

sjaeckel commented Sep 2, 2024 •

edited

Loading

karel-m commented Sep 2, 2024

levitte commented Sep 2, 2024

sjaeckel commented Sep 2, 2024

sjaeckel commented Sep 2, 2024

tbvdm commented Sep 2, 2024

Improve SSE4.1/AES-NI support #644

Improve SSE4.1/AES-NI support #644

Conversation

tbvdm commented Mar 31, 2024 • edited by sjaeckel Loading

Checklist

tbvdm commented Mar 31, 2024

sjaeckel commented Apr 2, 2024

tbvdm commented Apr 3, 2024

This comment was marked as spam.

sjaeckel commented Jun 12, 2024 • edited Loading

sjaeckel commented Jun 12, 2024 • edited Loading

sjaeckel left a comment

Choose a reason for hiding this comment

tbvdm commented Jun 24, 2024

tbvdm commented Jun 24, 2024

sjaeckel left a comment

Choose a reason for hiding this comment

tbvdm commented Jun 29, 2024

levitte left a comment

Choose a reason for hiding this comment

tbvdm commented Aug 31, 2024

sjaeckel Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

sjaeckel left a comment • edited Loading

Choose a reason for hiding this comment

tbvdm commented Sep 1, 2024

sjaeckel commented Sep 1, 2024

tbvdm commented Sep 1, 2024

sjaeckel commented Sep 2, 2024

karel-m commented Sep 2, 2024

sjaeckel commented Sep 2, 2024 • edited Loading

karel-m commented Sep 2, 2024

levitte commented Sep 2, 2024

sjaeckel commented Sep 2, 2024

sjaeckel commented Sep 2, 2024

tbvdm commented Sep 2, 2024

tbvdm commented Mar 31, 2024 •

edited by sjaeckel

Loading

sjaeckel commented Jun 12, 2024 •

edited

Loading

sjaeckel commented Jun 12, 2024 •

edited

Loading

sjaeckel Aug 31, 2024 •

edited

Loading

sjaeckel left a comment •

edited

Loading

sjaeckel commented Sep 2, 2024 •

edited

Loading