Skip to content

Conversation

@vt-alt
Copy link
Contributor

@vt-alt vt-alt commented Jun 25, 2021

As requested in #1019 (comment)
Two commits to support builds on x86 (i686) and ppc64le, all algorithms are supposed to build as generic implementations.

Tested and all tests are passing on ALT Linux. Our build logs (with tests run):

ps. These two commits are just cherry-pick'ed from our downstream repo, with some conflict resolved for the second one.

@baentsch
Copy link
Member

Thanks very much for this PR! From reading through the patch and the ALT Linux test logs, it looks clear enough and I'd approve it -- but you see in the discussions on #1019 that we wonder how we could keep confirming these architectures via a liboqs local CI system... So, could you share how you do CI for ALT Linux on/for x586 & ppc64le so we could possible learn from, maybe mimick this?

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

We are using rpm-based rpmbuild system with .spec files, which can have %check section where tests could be run after %build&%install stages. So, basically, I just run ninja run_tests there. Because we are using native builds these are run on every architecture where the build just happened (we have aarch64, armh, i586, ppc64le, and x86_64) and before it's later committed into the repository.

Tests were passing when these commits applied over 0.6.0, now I found that when applied over the current main branch (like in this PR) tests are failing on i686 architecture.

=========================== short test summary info ============================
FAILED tests/test_cmdline.py::test_kem[BIKE-L1] - AssertionError: Got unexpec...
FAILED tests/test_cmdline.py::test_kem[BIKE-L3] - AssertionError: Got unexpec...
FAILED tests/test_kat.py::test_kem[BIKE-L1] - AssertionError: Got unexpected ...
FAILED tests/test_kat.py::test_kem[BIKE-L3] - AssertionError: Got unexpected ...
FAILED tests/test_mem.py::test_mem_kem[BIKE-L3] - AssertionError: Got unexpec...
FAILED tests/test_mem.py::test_mem_kem[BIKE-L1] - AssertionError: Got unexpec...
============ 6 failed, 506 passed, 258 skipped in 61.45s (0:01:01) =============

I see there is commit 17d3e0b with heavy BIKE update. Maybe it requires additional porting effort.

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

I see there is commit 17d3e0b with heavy BIKE update. Maybe it requires additional porting effort.

Btw, it says * Disable BIKE build on 32-bit ARM and # BIKE is not supported on Windows and 32-bit ARM — is it known why? Perhaps, it's related.

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

Perhaps, it's related.

Yes, when I disable BIKE similar to how it's disabled for 32-bit ARM in 17d3e0b tests are passing again. Patch:

diff --git a/.CMake/alg_support.cmake b/.CMake/alg_support.cmake
index 51094bcf..ae695014 100644
--- a/.CMake/alg_support.cmake
+++ b/.CMake/alg_support.cmake
@@ -28,7 +28,7 @@ endif()
 endif()

 # BIKE is not supported on Windows and 32-bit ARM
-cmake_dependent_option(OQS_ENABLE_KEM_BIKE "Enable BIKE algorithm family" ON "NOT WIN32; NOT ARCH_ARM32v7" OFF)
+cmake_dependent_option(OQS_ENABLE_KEM_BIKE "Enable BIKE algorithm family" ON "NOT WIN32; NOT ARCH_ARM32v7; NOT ARCH_X86" OFF)
 cmake_dependent_option(OQS_ENABLE_KEM_bike1_l1_cpa "" ON "OQS_ENABLE_KEM_BIKE" OFF)
 cmake_dependent_option(OQS_ENABLE_KEM_bike1_l1_fo "" ON "OQS_ENABLE_KEM_BIKE" OFF)
 cmake_dependent_option(OQS_ENABLE_KEM_bike1_l3_cpa "" ON "OQS_ENABLE_KEM_BIKE" OFF)

I will force-push the updated version of the commits.

But, it's pity BIKE is not working (anymore?) on 32-bit arches.

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

Btw, I may try to [experimentally] create Dockerfile to build liboqs on the ALT's docker image for testing purposes. These could be run in Github Actions or directly. The apparent downside is ppc64le's one will be qemu-user emulated (and slower).

ps. I noticed such CI method in ispc project.

@baentsch
Copy link
Member

we have aarch64, armh, i586, ppc64le, and x86_64

OK, thanks for the explanation: So you have such physical machines somewhere and "simply" build/test on them (?)

Btw, I may try to [experimentally] create Dockerfile to build liboqs on the ALT's docker image for testing purposes. These could be run in Github Actions or directly. The apparent downside is ppc64le's one will be qemu-user emulated (and slower).

FWIW, the oqs-demos and profiling subprojects also do that -- but when looking at Github Actions, I'm not seeing a way to run things other than x86, ARM64/32 even using self-hosted runners -- even leaving the security downside of a public project aside. So that indeed would call for emulated ppc64le or s390x -- which arguably is way too slow, no?

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

So you have such physical machines somewhere and "simply" build/test on them (?)

Yes. They are integrated into our build system and considered to be the main architectures.

So that indeed would call for emulated ppc64le or s390x -- which arguably is way too slow, no?

It will be slower than native or KVM virtualization, but faster than full qemu-system emulation (with emulated userspace and kernel, while qemu-user only emulates userspace and pass-through the kernel).

https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/Measuring-QEMU-Emulation-Efficiency/
This page measures ppc64le native to emulation instructions ratio from 1:6 (work with integers) to 1:58 (work with doubles). Our build and run_tests took 923.59user cpu seconds, thus making an optimistic run starting from 46 minutes on emulation (on a single core) or longer. (Github Actions provide 2 core, so it would be >= 23 minutes. And for the manual runs, a multi-core developer box would be even faster.)

The only public ppc64le instances I know are Travis-CI and GCC Compile Farm (perhaps not suitable for automatic testing).

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 27, 2021

FYI. For the sake of curiosity, I tested liboqs on ppc64 (which is the big-endian counterpart of ppc64le) (on the gcc203 box of GCC Compile Farm) and it produces 28 test failures (and 1 build failure). So, I guess the code is not big-endian ready. (Not that we need it.)

@dstebila
Copy link
Member

Code looks good to me for now. But I'd like to get at least an x86 CI build before merging.

@bhess
Copy link
Member

bhess commented Jun 28, 2021

Thanks @vt-alt for preparing this.

I've set up simplistic TravisCI config for ppc64le on top of the PR to build and run the ninja tests (still running the constant-time tests on another machine). See
https://github.com/bhess/liboqs/tree/travis-ppc
https://travis-ci.com/github/bhess/liboqs

We can add the TravisCI build if we want to make exception and slightly expand the CI zoo again. The plus points are that it's free for ppc64le (and s390x). For more advanced workflow like triggering downstream builds it could become more complex however. I think if we limit to build&test it should be manageable.

It might also make sense to push the SIKE changes to upstream.

@baentsch
Copy link
Member

We can add the TravisCI build if we want to make exception and slightly expand the CI zoo again.

I'd be OK with this if it's clear that Travis (and ppc64l4 and s390x) are "second-grade" citizens until corresponding downstream CI checks have been added -- at the very least in those downstream projects for which this work has been initiated: Could you comment which ones those are, @vt-alt ? Was it oqs-openssh? Personally, I'd think oqs-openssl should also get added platform support as it's the basis for many (lib)OQS integrations. In my eyes, automatic downstream triggering then only would be icing on the cake.

@dstebila
Copy link
Member

Thanks @bhess for creating the ppc64le Travis testing. The CI configuration looks sufficiently straightforward that I'm not worried about it expanding the CI zoo much.

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 29, 2021

It might also make sense to push the SIKE changes to upstream.

Thanks. I will create PR there.

Could you comment which ones those are, @vt-alt ? Was it oqs-openssh?

I build liboqs and openquantumsafe-openssh for ALT Linux. There is info for most distributions:
https://repology.org/project/liboqs/versions
https://repology.org/project/openquantumsafe-openssh/versions

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 29, 2021

While porting to PQCrypto-SIDH I realized that I had incorrect assumptions (about the generic implementation of SIKE) and will force push slightly corrected ppc64le commit here. (No data or behavior change, just remove redundant #if arguments where they aren't needed anyway.

@vt-alt
Copy link
Contributor Author

vt-alt commented Jun 29, 2021

FYI. I created a pull request there microsoft/PQCrypto-SIDH#50

Force pushed mentioned above change here:

diff --git a/src/kem/sike/external/config.h b/src/kem/sike/external/config.h
index 97a9b8bf..591f1cd5 100644
--- a/src/kem/sike/external/config.h
+++ b/src/kem/sike/external/config.h
@@ -88,8 +88,6 @@ typedef uint64_t uint128_t[2];
 typedef unsigned uint128_t __attribute__((mode(TI)));
 #elif (TARGET == TARGET_ARM64 && OS_TARGET == OS_NIX)
 typedef unsigned uint128_t __attribute__((mode(TI)));
-#elif (TARGET == TARGET_PPC64LE && OS_TARGET == OS_NIX)
-typedef unsigned uint128_t __attribute__((mode(TI)));
 #elif (TARGET == TARGET_AMD64 && OS_TARGET == OS_WIN)
 typedef uint64_t uint128_t[2];
 #endif
@@ -228,7 +226,7 @@ static __inline unsigned int is_digit_lessthan_ct(digit_t x, digit_t y) { // Is
         ADC128(addend, product, carry, result);                    \
     }

-#elif ((TARGET == TARGET_AMD64 || TARGET == TARGET_ARM64 || TARGET == TARGET_PPC64LE) && OS_TARGET == OS_NIX)
+#elif ((TARGET == TARGET_AMD64 || TARGET == TARGET_ARM64) && OS_TARGET == OS_NIX)

 // Digit multiplication
 #define MUL(multiplier, multiplicand, hi, lo)                                    \

This is because these ifs are on else path of #if defined(GENERIC_IMPLEMENTATION), thus unreachable anyway.

@baentsch
Copy link
Member

baentsch commented Jul 5, 2021

This all looks good -- only issue remaining before merge seems to be

I'd like to get at least an x86 CI build before merging.

@dstebila : Do you still want this -- or is the ppc64le test via Travis enough?

@bhess
Copy link
Member

bhess commented Jul 7, 2021

I've added the TravisCI config for ppc64le to #1048 (needs to be rebased after this merges).

@dstebila dstebila added this to the 0.7.0 milestone Jul 7, 2021
vt-alt added 2 commits July 8, 2021 10:10
No processor extensions support.
Tests are passed.

Signed-off-by: Vitaly Chikunov <[email protected]>
No (AltiVec/VSX) processor extensions support is detected.
Tests are passed.

Note that `secure_cmp32` may require additional treatment for this
architecture.

Signed-off-by: Vitaly Chikunov <[email protected]>
@dstebila
Copy link
Member

dstebila commented Jul 8, 2021

Thanks @bhess. I've rebased onto main and am just waiting for the build to pass then it can be merged.

@dstebila dstebila mentioned this pull request Jul 8, 2021
@dstebila dstebila merged commit 9c2b485 into open-quantum-safe:main Jul 9, 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