Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding armv8 crypto extensions to SHA256/224 #1052

Merged
merged 14 commits into from
Jul 30, 2021

Conversation

Martyrshot
Copy link
Member

This PR adds the armv8 crypto extensions accelerated implementation of sha256 hashblocks from supercop. This was tested on a ROCK64, a RASPI 3B+, and on an Intel mac (to ensure I didn't break x86 builds by mistake).

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.) - NO
  • Does this PR change the the list of algorithms available -- either adding, removing, or renaming? (If so, PRs in OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also be required by the time this is merged.) - NO

@dstebila
Copy link
Member

Stylecheck failed because you were using an old version of the Doxygen configuration file. I've done a rebase on main and force pushed.

@Martyrshot
Copy link
Member Author

Ah shoot, I thought I forgot something. Sorry about that!

@Martyrshot Martyrshot marked this pull request as ready for review July 23, 2021 00:11
src/common/CMakeLists.txt Outdated Show resolved Hide resolved
src/common/sha2/sha2.c Outdated Show resolved Hide resolved
src/common/sha2/sha2.c Show resolved Hide resolved
src/common/sha2/sha2_c.c Outdated Show resolved Hide resolved
@Martyrshot
Copy link
Member Author

Do we want to consider changing the sha2 context structures to contain a uint8_t pointer rather than a void pointer? Not exactly related to adding armv8 crypto extensions, but since the code is getting updated anyway it could be a simple change to do while we are here?

@Martyrshot Martyrshot requested a review from dstebila July 27, 2021 22:57
src/common/CMakeLists.txt Outdated Show resolved Hide resolved
@baentsch
Copy link
Member

Do we want to consider changing the sha2 context structures to contain a uint8_t pointer rather than a void pointer?

Conceptually and personally I'd be all for accuracy wrt data structure types -- however, the void * in my eyes is an indication that it's not always a uint8_t: In the OSSL case, it's effectively a EVP_MD_CTX*. Thus, I'd keep it as is, but defer to @dstebila who put it like it is right now.

@dstebila
Copy link
Member

Do we want to consider changing the sha2 context structures to contain a uint8_t pointer rather than a void pointer?

Conceptually and personally I'd be all for accuracy wrt data structure types -- however, the void * in my eyes is an indication that it's not always a uint8_t: In the OSSL case, it's effectively a EVP_MD_CTX*. Thus, I'd keep it as is, but defer to @dstebila who put it like it is right now.

Indeed, the point of containing a void * pointer was to permit multiple instantiations which use different types of context structures.

@Martyrshot
Copy link
Member Author

Makes sense to me!

set(SHA2_IMPL sha2/sha2.c sha2/sha2_c.c)
if (OQS_DIST_ARM64v8_BUILD)
set(SHA2_IMPL ${SHA2_IMPL} sha2/sha2_ni.c)
set_source_files_properties(sha2/sha2_ni.c PROPERTIES COMPILE_FLAGS -mcpu=cortex-a53+crypto)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually just realizing that this line might be problematic. It's setting -mcpu=cortex-a53+crypto but is guarded only by if (OQS_DIST_ARM64v8_BUILD); but there will be certainly many other ARM64v8 CPU's other than Cortex-A53, and even some which don't have crypto extensions (e.g., our Raspberry Pi 3B's which don't versus our ROCK64's which do). Have you tried compiling and running this on other ARM64v8 processors? We have the rasp3b's, the rock64's, as well as an Apple Silicon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried it on the roc, raspis and aws instance. I'd be happy to test on apple silicon, but I would need access to a machine since I only have an intel mac. The reason I picked that flag, in particular, is because I have to specify a core in order to "turn on" crypto extensions when compiling on a system that doesn't have them (the +crypto portion of -mcpu), and a53 is the generic core used by liboqs. Without that compile flag the compiler will error saying that the extension isn't supported, but since we do a runtime check when OQS_DIST_ARM64v8_BUILD is specified, so we need to have those intrinsics compiled into the library.

Copy link
Member Author

@Martyrshot Martyrshot Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is an unrelated compiling issue when using OQS_DIST_BUILD I get the following error:

/Users/jgoertzen/stock-oqs/liboqs/src/common/common.c:74:10: fatal error: 'sys/auxv.h' file not found
#include <sys/auxv.h>
         ^~~~~~~~~~~~
1 error generated.

If I don't use OQS_DIST_BUILD my branch builds and all tests pass. (I have tried this on both the liboqs main branch and my branch with the same results). If this is intentional then the above concern isn't an issue, otherwise, I can take a look into finding a solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do want to be able to compile in that configuration, so can you look into it? I think what might be going on there is that the lines 74-92 of common.c are protected by OQS_DIST_ARM64v8_BUILD which in principle would include both Linux ARM (e.g., Raspberry Pi's) and Apple ARM, but the code in that section is solely Linux-focused; for example I think neither <sys/auxv.h> nor getauxval is available on macOS. This site shows one example of reading the CPU features on macOS. Although for Apple chips it seems like the features we want are always available (at least for now), so maybe we can simplify and just set them automatically without testing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll see what I can figure out. Would you like me to create a new PR or issue specifically for detecting apple M1 cpu features?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've done it. It runs for me our M1, but I haven't tested to make sure it doesn't break things on our other ARMs. Can you give a try?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, for sure! Thanks for doing that! I'll try it out right away.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some small changes and tested on the roc, raspi, was, and apple silicon machine and all tests passed. I also verified that on the apple silicon machine that the sha256 instructions were being used rather than the c implementation. So it should be good to go!

@dstebila dstebila added this to the 0.7.0 milestone Jul 29, 2021
…s, and updated some uint32_t to const uint32_t to fix warnings on macOS on Apple silicon
@dstebila dstebila merged commit 636d972 into open-quantum-safe:main Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants