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

riscv: use hwprobe syscall for capability detection #24172

Closed

Conversation

ZenithalHourlyRate
Copy link
Contributor

@ZenithalHourlyRate ZenithalHourlyRate commented Apr 17, 2024

This originates from #24069 (comment)

hwprobe is a linux syscall specific to riscv architecture where key/value pairs can be queried to detect whether an extension exists. This corresponds to the capability defined in openssl.

hwprobe syscall is added in Linux 6.4 and <asm/hwprobe.h> is still updating (check lkml). To workaround the compile time/runtime linux version problem, examples are

  • compile/run in system where kernel<6.4
  • compile/run in system where kernel>=6.4 but <asm/hwprobe.h> is does not corresponds to the hwprobe.h openssl required
  • compile in a new system and run in an old system

As for non-existing syscall, __has_include(<asm/hwprobe.h>) can be used at compile time and -ENOSYS can be used at runtime

As for unknown key/value query, the syscall would clear it, see

If a key is unknown to the kernel, its key field will be cleared to -1, and its value set to 0.
https://docs.kernel.org/arch/riscv/hwprobe.html

The only problem is the possible unknown macros from <asm/hwprobe.h> (undefined macro when compiling in old system) so directly coping value is the workaround.

As the original thread discussed, OPENSSL_riscvcap, when provided, should override hwprobe interface.

Documentation for OPENSSL_riscvcap together with hwprobe would be added in another PR.

Checklist
  • documentation is added or updated

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 17, 2024
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing labels Apr 17, 2024
include/crypto/riscv_arch.h Outdated Show resolved Hide resolved
@ZenithalHourlyRate
Copy link
Contributor Author

ZenithalHourlyRate commented Apr 17, 2024

Cherry-picked #24069 on this branch and ran openssl on Canaan Kendryte K230 where kernel=6.8.0 to test whether hwprobe works, the results matched

$ unset OPENSSL_riscvcap
$ ./openssl speed -evp chacha20-poly1305
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305    33989.34k    52381.21k    75642.11k    79155.54k    90438.59k    90581.67k
$ export OPENSSL_riscvcap="rv64gc"
$ ./openssl speed -evp chacha20-poly1305
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305    34001.63k    53398.65k    59937.11k    62083.66k    63053.82k    63018.33k
$ export OPENSSL_riscvcap="rv64gc_v_zbb"
$ ./openssl speed -evp chacha20-poly1305
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305    33614.25k    52176.50k    76595.11k    79170.56k    90750.98k    90608.98k

@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Apr 17, 2024
@ZenithalHourlyRate
Copy link
Contributor Author

Now cpuinfo can be displayed. Benchmark result using #24069

$ ./openssl info -cpusettings
OPENSSL_riscvcap=ZBA_ZBB_ZBC_ZBS_V
$ ./openssl speed -evp chacha20-poly1305
CPUINFO: OPENSSL_riscvcap=ZBA_ZBB_ZBC_ZBS_V
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305    33657.82k    52458.13k    75558.49k    78885.42k    90753.71k    90570.75k

$ export OPENSSL_riscvcap="rv64gc_zba_zbb_zbc_zbs"
$ ./openssl info -cpusettings
OPENSSL_riscvcap=ZBA_ZBB_ZBC_ZBS env:rv64gc_zba_zbb_zbc_zbs
$ ./openssl speed -evp chacha20-poly1305
CPUINFO: OPENSSL_riscvcap=ZBA_ZBB_ZBC_ZBS env:rv64gc_zba_zbb_zbc_zbs
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305    33741.30k    52294.55k    59968.85k    62311.77k    62792.63k    62941.87k

@ZenithalHourlyRate
Copy link
Contributor Author

Documentation for OPENSSL_riscvcap and its interaction with hwprobe is added. Cc @kroeckx in #24069 (comment)

The limitation of the environment variable parser, as documented, would be fixed in another PR.

@ZenithalHourlyRate ZenithalHourlyRate force-pushed the riscv-hwprobe branch 2 times, most recently from fe7ef98 to b4e7fe6 Compare April 24, 2024 12:44
@t8m t8m closed this Apr 24, 2024
@t8m t8m reopened this Apr 24, 2024
@ZenithalHourlyRate
Copy link
Contributor Author

Wonder if anything should be done for this PR

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

My approval stands

@paulidale
Copy link
Contributor

@openssl/committers

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

A few nits

crypto/riscvcap.c Outdated Show resolved Hide resolved
crypto/riscvcap.c Outdated Show resolved Hide resolved
crypto/info.c Outdated Show resolved Hide resolved
@ZenithalHourlyRate
Copy link
Contributor Author

Requested changes addressed

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

@paulidale please reconfirm

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 8, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 9, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented May 9, 2024

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this May 9, 2024
openssl-machine pushed a commit that referenced this pull request May 9, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24172)
openssl-machine pushed a commit that referenced this pull request May 9, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24172)
openssl-machine pushed a commit that referenced this pull request May 9, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24172)
*/
# define OSSL_RISCV_HWPROBE_PAIR_COUNT 1
# define OSSL_RISCV_HWPROBE_PAIR_CONTENT \
{ 4, 0 },
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to replace 4 here with RISCV_HWPROBE_KEY_IMA_EXT_0 with #define RISCV_HWPROBE_KEY_IMA_EXT_0 4 somewhere.

jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24172)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24172)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24172)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants