-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
CI: cross-compile: riscv: enable more tests on extensions #24403
base: master
Are you sure you want to change the base?
Conversation
.github/workflows/cross-compiles.yml
Outdated
@@ -150,7 +167,7 @@ jobs: | |||
tests: none | |||
} | |||
] | |||
runs-on: ${{ github.server_url == 'https://github.com' && 'ubuntu-latest' || 'ubuntu-22.04-self-hosted' }} | |||
runs-on: ${{ github.server_url == 'https://github.com' && 'ubuntu-24.04' || 'ubuntu-22.04-self-hosted' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ubuntu-latest
not pointing to ubuntu-24.04
?
Also, the fallback to Ubuntu 22.04 is pointless because it will always trigger a failing build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
ubuntu-latest
not pointing toubuntu-24.04
?
The README of https://github.com/actions/runner-images shows ubuntu-latest points to ubuntu-22.04 and ubuntu-24.04 is marked as "beta"
Also, the fallback to Ubuntu 22.04 is pointless because it will always trigger a failing build.
Do not know the details of these self hosted CI. Do not know if it is safe to remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. it would be bad to remove them. But you should skip the build for these two newly added targets if running on these self-hosted runners.
You probably mean Ubuntu 24.04. In general, I think it would be great if we could finally have proper CI pipelines for RISC-V. What I am missing is covering all relevant combinations. For example, we have different Zvkg-optimizations with ( |
Could the fips issue be another instance of #23979 ? We really need to solve it. |
e3e6b12
to
a3f505f
Compare
https://github.com/openssl/openssl/actions/runs/9097624311?pr=24403 Interesting results... First #23973 does not fix fips install, check https://github.com/openssl/openssl/actions/runs/9097624311/job/25005869697?pr=24403 Second some cross-compile would not dump Third hwprobe interface for RISC-V exists and qemu-riscv64 implements this syscall, the default case is no longer RV64GC and riscvcap should be set to empty now. Check https://github.com/openssl/openssl/actions/runs/9097624311/job/25005868354?pr=24403
|
Turned out those are
Now for RV64GC case, fips install would fail as OPENSSL_riscvcap is now manually set; set |
Reproduced
Seems like OPENSSL_cpuid_setup is called twice and the second call from fips caused the problem. There are two definitions of
openssl/include/crypto/riscv_arch.h Lines 16 to 20 in a6afe2b
openssl/include/crypto/riscv_arch.h Lines 44 to 52 in a6afe2b
The Lines 19 to 26 in a6afe2b
And assumed fips module should not make syscall. The different struct definition is a side effect. For #23979, I assume there are two definition of |
Now with the same structure it still fails. After debugging I found it is not accessing outside of RISCV_capabilities as changing that line to |
openssl/providers/fips/fipsprov.c Lines 932 to 941 in a6afe2b
|
In
In my opinion, attribute constructor should be removed, and cpuid_setup should be called explicitly inside fips init (as libcrypto has done), instead of the implicit .init_array. Like #23978 (reply in thread) pointed out, this magic is confusing. |
There has to be a separate And yes, given the providers have an explicit init function, the cpuid setup routines should be called from the FIPS provider's init function after the core callbacks are set but before the self tests are run. |
This PR is now self-contained enough and ready for review. The fips init issue is addressed in another PR #24419 and it is does not block this PR.
Interestingly, two alternative code path for crypto/modes/gcm128.c failed, they are Lines 529 to 530 in a6afe2b
Lines 533 to 536 in a6afe2b
I recently do not get much spare for debugging into assemblies so might the original authors take care of these paths? There are several ways to address this (as we want to see CI green on master branch otherwise it is annoying):
I prefer the first way. If you accept I'll add a commit commenting out them. I'll also add a tmp commit now for --Update1 https://github.com/openssl/openssl/actions/runs/9111809271/job/25049744210 Done. Seems like the gcm problem. The exit code is 132 which is illegal instruction |
Ignore this, it was incorrect.
|
@paulidale no, this is not true. Please look at the current DEP code in the FIPS provider. The DEP just indicates the state - i.e. that the selftest must be run before the module is being operational. But they are actually run from the provider init function. |
They are related to a QEMU bug where the Check https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg02231.html I'll ping upstream to backport this fix Now all code path (that I found, that controlled by riscvcap) for riscv64 pass the CI. The lefting paths are those enabled by -march CFLAGS; would be done in another PR. |
__attribute__((constructor)) makes cpuid_setup called when loading dynamic libraries like libcrypto.so and fips.so, typically in .init_array. However, this is not wanted as some required function, like BIO_snprintf is not ready. Instead, cpuid_setup should be explicitly called inside the init function of these libraries.
Related to actions/runner-images#9848 Related to openssl#20139
This reverts commit 4597200.
Otherwise riscv hwprobe interface would set cap
Most paths are covered by these two cases 1. bitmanip and scalar crypto extension 2. vector bitmanip/crypto extension
zvkb would enable/disable zvkg due to typo in qemu src Check https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg02231.html
Ping for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is scalable. This adds 9 new CI jobs on every pull request and that is something we probably want to avoid.
At least all the jobs with the same target could be somehow iteratively run on the same build.
I fully agree. We don't need to build all relevant combinations (given all optimizations are based on run-time properties, a single build might be enough). But all distinct code paths should be run. So we also don't need to run all tests for all combinations. E.g. all AES-related tests should be run with all AES-relevant extension combinations. When I was working on the Zvk* tests, I used the following:
Maybe this serves as an inspiration to test only relevant tests. |
Github Actions recently supported Ubuntu 24.04, which ships with QEMU 8.2.2 and all currently OpenSSL-supported extensions are there.
This PR picks up an old attempt to add CI on Zb/Zk and will try to add other CIs covering other extensions like #20149, #24069 (enabled by OPENSSL_riscvcap) and #16710 (enabled by CFLAGS)
Cc @cmuellner
Though
fips: no
might be needed for those CI as they failed to pass the test on installing fips module on my machine. I do not fully understand the relationship between fips support and RISC-V extension support, please point out if fips support is a requirement when some extension is enabled and these should be addressed in other PRsChecklist