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

valeriosetti · 2023-03-20T16:11:52Z

This is the closing PR of the accelerated EC based algs: all 3 algorithms (ECDSA, ECDH, ECJPAKE) are accelerated at the same time.
Resolves #7272
Depends on #7312

Gatekeeper checklist

changelog not required: changelog for every single accelerated component was already added in previous PRs
backport not required: enhancement
tests provided

valeriosetti · 2023-03-20T16:14:46Z

@mpg As requested, I fixed also remaining comments of #7912, a part from this one, since it is not clear to me what we want to do here

mpg

Just a first pass, making myself familiar with things.

tests/scripts/all.sh

library/psa_crypto.c

library/ssl_misc.h

tests/scripts/analyze_outcomes.py

tests/scripts/all.sh

library/ssl_misc.h

tests/scripts/all.sh

Actually this adds both the accelerated test as well as the reference. Both of them are used to evaluate the driver's coverage with analyze_outcomes.py script. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

…ge analysis All these EC based algs are now tested all at once in test_psa_crypto_config_[accel/reference]_all_ec_algs_use_psa() functions. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

We keep tests without USE_PSA for single accel components (i.e. ECDH, ECDSA, ECJPAKE), but when testing for all 3 accelerated at the same time we use USE_PSA for better test coverage. However for this purpose there is already the: component_test_psa_crypto_config_[reference/accel]_all_ec_algs_use_psa() so we can delete this extra component. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

…e_psa() Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

AndrzejKurek

Looks good, minor improvements (probably) for a separate PR suggested, thanks!

AndrzejKurek · 2023-03-29T07:16:54Z

tests/scripts/all.sh

-# Used by tests/scripts/analyze_outcomes.py for comparison purposes.
-component_test_psa_crypto_config_reference_ecdh_use_psa () {
-    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with reference ECDH + USE_PSA"
+    # Make build-in fallback not available


Very minor:

Suggested change

# Make build-in fallback not available

# Make built-in fallback not available

AndrzejKurek · 2023-03-29T07:19:35Z

tests/scripts/all.sh

-    # Algorithms and key types to accelerate
-    loc_accel_list="ALG_ECDH KEY_TYPE_ECC_KEY_PAIR KEY_TYPE_ECC_PUBLIC_KEY"
+component_test_psa_crypto_config_accel_pake() {
+    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated PAKE"


Minor, and pre-existing in other new test components related to acceleration:
The first message should rather be build, or make, not test, which is reserved for make test or ssl-opt.sh :)

AndrzejKurek · 2023-03-29T07:20:58Z

tests/scripts/all.sh

+# - component_test_psa_crypto_config_accel_all_ec_algs_use_psa;
+# - component_test_psa_crypto_config_reference_all_ec_algs_use_psa.
+# This supports comparing their test coverage with analyze_outcomes.py.
+config_psa_crypto_config_all_ec_algs_use_psa () {


Minor: There's no mention of the input argument.

valeriosetti · 2023-03-29T07:27:18Z

@AndrzejKurek thanks for the review and feedback!

minor improvements (probably) for a separate PR suggested

There is #7103 which is waiting for this one to be merged, so in case there is no other change required for this PR, I will apply your suggestions in that PR

mpg

Approving the rebase. I've double-checked that the 3 commits that have been removed had all landed development in the meantime.

valeriosetti added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Mar 20, 2023

valeriosetti requested a review from mpg March 20, 2023 16:11

valeriosetti self-assigned this Mar 20, 2023

valeriosetti force-pushed the issue7272 branch 4 times, most recently from 283946c to a420b29 Compare March 21, 2023 11:44

AndrzejKurek self-requested a review March 22, 2023 07:20

mpg removed the needs-ci Needs to pass CI tests label Mar 22, 2023

mpg reviewed Mar 22, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/scripts/all.sh Show resolved Hide resolved

tests/scripts/all.sh Show resolved Hide resolved

library/psa_crypto.c Show resolved Hide resolved

valeriosetti force-pushed the issue7272 branch from a420b29 to e2736c2 Compare March 22, 2023 13:03

AndrzejKurek removed the needs-reviewer This PR needs someone to pick it up for review label Mar 22, 2023

valeriosetti force-pushed the issue7272 branch 2 times, most recently from 1f2104e to 9979fe9 Compare March 23, 2023 10:43

mpg reviewed Mar 23, 2023

View reviewed changes

library/ssl_misc.h Outdated Show resolved Hide resolved

AndrzejKurek reviewed Mar 23, 2023

View reviewed changes

tests/scripts/analyze_outcomes.py Show resolved Hide resolved

AndrzejKurek reviewed Mar 23, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

AndrzejKurek reviewed Mar 23, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

valeriosetti mentioned this pull request Mar 24, 2023

Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

Merged

3 tasks

valeriosetti requested review from AndrzejKurek and mpg March 24, 2023 06:30

valeriosetti force-pushed the issue7272 branch from 2032a1d to 6345978 Compare March 24, 2023 07:37

mpg reviewed Mar 24, 2023

View reviewed changes

library/ssl_misc.h Show resolved Hide resolved

valeriosetti force-pushed the issue7272 branch from 6345978 to 22c1b87 Compare March 24, 2023 10:49

mpg reviewed Mar 24, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

mpg added the needs-work label Mar 28, 2023

valeriosetti added 14 commits March 28, 2023 16:26

test: add a test with all EC based algs accelerated

42d5f19

Actually this adds both the accelerated test as well as the reference. Both of them are used to evaluate the driver's coverage with analyze_outcomes.py script. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ssl_tls: fix guard symbols for EC accelerated tests

1fa5c56

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: remove previous tests for accelerated ECDSA/ECDH/ECJPAKE covera…

1c3de61

…ge analysis All these EC based algs are now tested all at once in test_psa_crypto_config_[accel/reference]_all_ec_algs_use_psa() functions. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: fix comments in test_psa_crypto_config_accel_ecc()

6f820cc

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psa_crypto: fix guard for mbedtls_ecc_group_of_psa()

9affb73

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: fix erroneous changes in all.sh

44b178c

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: remove unused tasks in analyze_outcomes.py

2495cdb

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: improve comment in the added test

77bdff1

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ssl: remove useless guard

77a904c

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: moving accel ECJPAKE test close to accel ECDH and ECDSA ones

4fa6d0b

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: simplify comment in test_psa_crypto_config_accel_all_ec_algs_us…

6d687b9

…e_psa() Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: fix wrong accelerated SHA1 symbol

b76672d

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

tls12_client: remove unnecessary parentheses

4642316

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed stale reviews from mpg and AndrzejKurek via 4642316 March 28, 2023 14:27

valeriosetti force-pushed the issue7272 branch from 4ce0d88 to 4642316 Compare March 28, 2023 14:27

valeriosetti requested review from AndrzejKurek and mpg March 28, 2023 14:28

valeriosetti added priority-high High priority - will be reviewed soon needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Mar 28, 2023

AndrzejKurek approved these changes Mar 29, 2023

View reviewed changes

mpg approved these changes Mar 29, 2023

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 29, 2023

mpg merged commit 77902df into Mbed-TLS:development Mar 29, 2023

valeriosetti deleted the issue7272 branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valeriosetti commented Mar 20, 2023 •

edited by mpg

Loading

valeriosetti commented Mar 20, 2023

mpg left a comment

AndrzejKurek left a comment

AndrzejKurek Mar 29, 2023

AndrzejKurek Mar 29, 2023

AndrzejKurek Mar 29, 2023

valeriosetti commented Mar 29, 2023

mpg left a comment

	# Make build-in fallback not available
	# Make built-in fallback not available

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

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

Conversation

valeriosetti commented Mar 20, 2023 • edited by mpg Loading

Gatekeeper checklist

valeriosetti commented Mar 20, 2023

mpg left a comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

AndrzejKurek Mar 29, 2023

Choose a reason for hiding this comment

AndrzejKurek Mar 29, 2023

Choose a reason for hiding this comment

AndrzejKurek Mar 29, 2023

Choose a reason for hiding this comment

valeriosetti commented Mar 29, 2023

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Mar 20, 2023 •

edited by mpg

Loading