CI: cross-compile: riscv: enable more tests on extensions #24403
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.
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.
Is
ubuntu-latestnot 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.
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. |
t8m
added
branch: master
ZenithalHourlyRate
changed the title
ZenithalHourlyRate
force-pushed
the
ci-rv
branch
2 times, most recently
from
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. |
github-actions
bot
added
the
severity: fips change
|
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 |
t8m
added
approval: review pending
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: nomight 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