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

Driver-only ECC: all three top-level modules #7272

Closed
mpg opened this issue Mar 13, 2023 · 2 comments · Fixed by #7321
Closed

Driver-only ECC: all three top-level modules #7272

mpg opened this issue Mar 13, 2023 · 2 comments · Fixed by #7321
Labels
enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Mar 13, 2023

Context: see #6839

Once TL.ecdsa, TL.ecdh and TL.ecjpake have been achieved though previous tasks, it's time to turn to the full TL: all three modules accelerated at once. This will materialize in a new all.sh component component_test_psa_crypto_config_accel_ecc_use_psa (plus its reference companion). Add it and fix any issue that may arise (labeled size-m for the uncertainty, but it might turn out that there are no issues and this is really an S).

Once this component is added, the previous components accel_{ecdsa,ecdh,ecjapke}_use_psa can probably be removed, or at least changed to all use the same reference component. We should strive for a balance between test coverage and use of CI resources which tend to be limited.

Edit: also address remaining review comments from #7192 while at it.

@valeriosetti
Copy link
Contributor

Once this component is added, the previous components accel_{ecdsa,ecdh,ecjapke}_use_psa can probably be removed, or at least changed to all use the same reference component. We should strive for a balance between test coverage and use of CI resources which tend to be limited.

In the current implementation I'm working on, I removed all the previous ECDSA/ECDH/ECJPAKE single cases, because it seemed to me that trying to address all possible combinations of the 3 in all.sh could end up in a quite messy thing.
However, if we still want to keep the possibility to accelerate any combination of those 3, what about adding something in depends.py? @mpg Wdyt?

@mpg
Copy link
Contributor Author

mpg commented Mar 21, 2023

I don't think we need to test all possible combinations either in all.sh or in depends.py. I'm inclined to keep only 3 components (plus one reference component):

  • accel_ecc_use_psa accelerating all three with USE_PSA, running ssl-opt.sh and a reference component for parity testing
  • accel_ecdh, accel_ecdsa and accel_ecjpake starting with the default config, no USE_PSA, no ssl-opt.sh and no parity testing (basically the three component that existed before we started this line of work).

In particular, I think we can remove the component I added recently in #7192 as it's superseded by the new accel_ecc_use_psa.

I'm not 100% sure about this plan, but IIUC that's roughly what you've done so far? The thing is we want to find a balance, test enough combinations without using too many CI resources. My idea is for the most costly tests (USE_PSA, ssl-opt.sh and reference component) we should have only one component with everything accelerated, and then we can test only-one accelerated in a less costly way (just make test). Though perhaps even that is redundant.

Wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants