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

xtest: add asymmetric cipher perf test #714

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Conversation

yuzexiyzx
Copy link
Contributor

Add perf test for DH, RSA, ECDH, ECDSA algorithm

@yuzexiyzx
Copy link
Contributor Author

image
@jforissier I have fixed the problems according to this.

ta/crypto_perf/include/ta_crypto_perf.h Outdated Show resolved Hide resolved
host/xtest/xtest_main.c Outdated Show resolved Hide resolved
ta/crypto_perf/include/ta_crypto_perf.h Outdated Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Outdated Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
@clementfaure
Copy link
Contributor

That's a funny coincidence, we were working on a similar application benchmark, and we were about to submit it here. We will review the pull-request then.

@jforissier
Copy link
Contributor

That's a funny coincidence, we were working on a similar application benchmark, and we were about to submit it here. We will review the pull-request then.

Much appreciated 👍

Copy link
Contributor

@omasse-linaro omasse-linaro left a comment

Choose a reason for hiding this comment

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

is key derivation also plan to be part of asymmetric benchmark ?

host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
host/xtest/asym_cipher_perf.c Outdated Show resolved Hide resolved
@yuzexiyzx
Copy link
Contributor Author

TEE_ALG_SM2_DSA_SM3 will be added soon. ED25519 and keypair derivation maybe later.

@yuzexiyzx yuzexiyzx force-pushed the master branch 2 times, most recently from d5e4aad to 1bb8323 Compare December 1, 2023 06:32
Copy link
Contributor

@clementfaure clementfaure left a comment

Choose a reason for hiding this comment

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

Split your commit into two commits (CA et TA).
Could you please use fixup commits? That would help a lot on tracking changes during the review. Thanks!

host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
ta/crypto_perf/ta_entry.c Show resolved Hide resolved
@clementfaure
Copy link
Contributor

Hi @yuzexiyzx, please use fixup commits and answer/resolve comments so we can keep track of your modifications. Thanks!

@yuzexiyzx
Copy link
Contributor Author

Hi @yuzexiyzx, please use fixup commits and answer/resolve comments so we can keep track of your modifications. Thanks!
Can you give me an example of fixup commits?Thanks!

@clementfaure
Copy link
Contributor

Hi @yuzexiyzx, please use fixup commits and answer/resolve comments so we can keep track of your modifications. Thanks!
Can you give me an example of fixup commits?Thanks!

Sure!
Please check one of my current pull-request : https://github.com/OP-TEE/optee_os/pull/5990/commits

Instead of amending your previous commit and force-pushing every time, you create a fixup commit that “fixes” one of your previous commits like so : git add ... && git commit --fixup=reword|amend:<COMMIT_ID>
More info : https://git-scm.com/docs/git-commit/2.32.0

It will automatically generate a commit message based on the commit you want to fix. Please note that OPTEE CI does not like commits with empty message body. You can reword it to specify the changes you made. When the review is done, you can squash all commits automatically with git rebase --autosquash

host/xtest/xtest_main.c Outdated Show resolved Hide resolved
@clementfaure
Copy link
Contributor

@yuzexiyzx The latest commit you pushed is empty. You need to add the changes you make during the review to the fixup commit.

@yuzexiyzx yuzexiyzx force-pushed the master branch 3 times, most recently from ed4be4b to cb515d5 Compare December 5, 2023 06:23
@clementfaure
Copy link
Contributor

@yuzexiyzx Could you also answer and eventually resolve comments ? So we know if the comment has been addressed or not. Thanks

host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
ta/crypto_perf/ta_crypto_perf.c Outdated Show resolved Hide resolved
@yuzexiyzx
Copy link
Contributor Author

@jforissier @clementfaure @etienne-lms @omasse-linaro All comments have been solved.

host/xtest/acipher_perf.c Outdated Show resolved Hide resolved
Android.mk Outdated Show resolved Hide resolved
host/xtest/xtest_main.c Outdated Show resolved Hide resolved
@yuzexiyzx
Copy link
Contributor Author

image
only changed this,but CI failed

@jforissier
Copy link
Contributor

image only changed this,but CI failed

Hi @yuzexiyzx, sorry about that. The error is unrelated to your change. It should go away as soon as OP-TEE/build#725 is merged. I will restart the job afterwards.

@yuzexiyzx
Copy link
Contributor Author

@jforissier Thanks!
@clementfaure @omasse-linaro @etienne-lms All comments have been solved.

@yuzexiyzx
Copy link
Contributor Author

@clementfaure @omasse-linaro @etienne-lms All comments have been solved.Anything else need to be modified?

@clementfaure
Copy link
Contributor

Hi @yuzexiyzx
Please check
#714 (comment)
#714 (comment)

@yuzexiyzx
Copy link
Contributor Author

Hi @clementfaure
I replied this two comments.
image
#714 (comment) is modified according to your comments.
image
I think this one is needed to align with others.

@clementfaure
Copy link
Contributor

clementfaure commented Jan 29, 2024

Your comments are Pending. We cannot see it. You need to submit your review.

@yuzexiyzx
Copy link
Contributor Author

@clementfaure Really sorry about that.Could you tell me how to submit my review? I cannot find this option.
image

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Commit "xtest: add asymmetric cipher perf test" looks good to me (with monir comment):
Acked-by: Etienne Carriere <[email protected]>.

@jforissier, there seems to be an issue with the Rust setup build. Any idea?

2024-01-29T11:32:27.2529700Z info: downloading component 'rust-std' for 'aarch64-unknown-linux-gnu'
2024-01-29T11:32:27.2547290Z error: component download failed for rust-docs-x86_64-unknown-linux-gnu: could not rename downloaded file from '/github/home/.rustup/downloads/34651bc0aae' (snip)

main_algo = ALGO_DH;
mode = MODE_GENKEYPAIR;
}
else if (!strcasecmp(argv[i], "RSA_GENKEYPAIR")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: for consistency:

			} else if (!strcasecmp(argv[i], "RSA_GENKEYPAIR")) {

Ditto at line 746.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

@clementfaure
Copy link
Contributor

clementfaure commented Jan 29, 2024

@yuzexiyzx No problem.
To submit your review, check the top right corner of the pull-request page. Once submitted, I will answer your comments.
image

@jforissier
Copy link
Contributor

@jforissier, there seems to be an issue with the Rust setup build. Any idea?

2024-01-29T11:32:27.2529700Z info: downloading component 'rust-std' for 'aarch64-unknown-linux-gnu'
2024-01-29T11:32:27.2547290Z error: component download failed for rust-docs-x86_64-unknown-linux-gnu: could not rename downloaded file from '/github/home/.rustup/downloads/34651bc0aae' (snip)

Not yet, I am trying to reproduce it locally.

@yuzexiyzx
Copy link
Contributor Author

@clementfaure Thanks.The right corner of pull request page is like this.
image
I still cannnot find the option of submit review.Could you please tell me in detail?Thank you very much!

@yuzexiyzx
Copy link
Contributor Author

There is no problem in other PRs.
image
My comment is without pending there.

@yuzexiyzx
Copy link
Contributor Author

@jforissier Could you please tell me how to solve it? Thank you very much!

@etienne-lms
Copy link
Contributor

Hi @yuzexiyzx,
To submit your review comments, you can go to the Files Changed tab of your P-R and use the green button "Review Changes" at the upper right corner. If there are pending comments, they will be posted.

@yuzexiyzx
Copy link
Contributor Author

@etienne-lms found it.Thank you very much!

@yuzexiyzx yuzexiyzx force-pushed the master branch 2 times, most recently from f1be7c1 to e68c45f Compare January 30, 2024 11:23
@yuzexiyzx
Copy link
Contributor Author

@clementfaure I have submitted my comments.

@yuzexiyzx
Copy link
Contributor Author

@clementfaure @omasse-linaro All comments have been solved.

}

/* initial test buffer allocation (eventual registering to TEEC) */
static void alloc_buffers(size_t sz, int verbosity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Better remove verbosity no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

check_res(res, "TEEC_InvokeCommand()", &ret_origin);
}

static void prepare_enc_sign(uint32_t size, uint32_t mode, uint32_t is_random,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use for uint32_t is_random ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. It's unused here, only used in asym_perf_run_test() to init input data. Can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_random has been removed

case ALGO_ECDH:
curve_id = get_curve_id(width_bits);
if (curve_id == TEE_CRYPTO_ELEMENT_NONE)
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to goto out; ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

break;
default:
fprintf(stderr, "Unexpected mode value\n");
exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to goto out; ?
Edit: in this function, we exit in several different ways, it's inconsistent. We've got goto out;, return 1, exit(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

if (param_types != exp_param_types)
return TEE_ERROR_BAD_PARAMETERS;

crypto_buf = TEE_Malloc(params[0].memref.size, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if the malloc is successful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

params[1].memref.buffer, params[1].memref.size,
params[2].memref.buffer, dummy_size);

CHECK(res, "TEE processing failed", goto out;);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can break instead of goto out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

params[1].memref.buffer, params[1].memref.size,
params[2].memref.buffer, &dummy_size);

CHECK(res, "TEE processing failed", goto out;);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can break instead of goto out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

@@ -54,7 +55,6 @@ TEE_Result TA_InvokeCommandEntryPoint(void *pSessionContext,
switch (nCommandID) {
case TA_CRYPTO_PERF_CMD_CIPHER_PREPARE_KEY:
return cmd_cipher_prepare_key(nParamTypes, pParams);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this change as it is not related to the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

uint32_t b;
};

static TEE_Result unpack_attrs(const uint8_t *buf, size_t blen,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this function is duplicated. I don't know if it's really an issue, however. @etienne-lms ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you refer to the unpack_attrs() function in ta/crypt/cryp_taf.c.
I don't think it's worth factorizing both as they relate to specific TAs.

else if (mode == MODE_DECRYPT)
do_asym = TEE_AsymmetricDecrypt;
else if (mode == MODE_SIGN)
do_asym = TEE_AsymmetricSignDigest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an else to return an error as do_asym()as it can be called as NULL.
Do the if (mode == MODE_VERIFY) {...} first and then the other modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

Add perf test for DH, RSA, ECDH, ECDSA algorithm

Signed-off-by: Zexi Yu <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Add perf test for DH, RSA, ECDH, ECDSA algorithm

Signed-off-by: Zexi Yu <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
@yuzexiyzx
Copy link
Contributor Author

@clementfaure All comments have been solved.

@etienne-lms
Copy link
Contributor

Thanks @yuzexiyzx for all the work.
I think it would be nice this P-R is merged (if all agree of course) so that everyone can play with it and find issues if there remains. This is not a regression test, it's only perf measurements so it's not a major issues if something is badly handle. This test tool will better mature if people are using it.

@jforissier jforissier merged commit bcd5583 into OP-TEE:master Feb 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants