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: combine AES and hash into crypto_perf #706

Closed
wants to merge 1 commit into from

Conversation

yuzexiyzx
Copy link
Contributor

@yuzexiyzx yuzexiyzx commented Nov 13, 2023

There is quite a bit of copy in thes files.So, I cosonlidate th code to make it easier to maintain.

@jforissier
Copy link
Contributor

The asymmetric cipher app is buggy in many ways. Please test it and fix it so that it behaves as documented by xtest --asym-cipher-perf --help. I tried some random combinations that are supposedly valid as per the help string, but unfortunately I got errors:

$ xtest --asym-cipher-perf -n 10 -t DH
TEEC_InvokeCommand: 0xffff0006 (orig=4)
$ xtest --asym-cipher-perf -n 10 -t ECDSA
ECC curve is not supported!
$ xtest --asym-cipher-perf -n 10 -t ECDH
--asym-cipher-perf, invalid main_algo
Usage: --asym-cipher-perf [-h]
[...]
$ xtest --asym-cipher-perf -h 2>&1 | grep '???'
  -a ALGO          Crypto algorithm tested when TYPE is RSA [???]
$ xtest --asym-cipher-perf -m ENC
The algo[4294967295] is not valid!
$ xtest --asym-cipher-perf -a RSA_NOPAD -m ENC
GENKEYPAIR, invalid mode

@yuzexiyzx
Copy link
Contributor Author

OK.I will test it and fix it.

@yuzexiyzx
Copy link
Contributor Author

For $ xtest --asym-cipher-perf -a RSA_NOPAD -m ENC
-a should be after -t and -m

@jforissier
Copy link
Contributor

For $ xtest --asym-cipher-perf -a RSA_NOPAD -m ENC -a should be after -t and -m

Not acceptable. There is no reason for a specific order here.

@yuzexiyzx
Copy link
Contributor Author

-a is specific for RSA algorithm and it is different for enc or dec and sign or verify.So,it is valid without -m or -t parameters,but it is invalid to modify -m or -t parameter after -a parameter.

@yuzexiyzx
Copy link
Contributor Author

There are many connections between parameters, For example,-a is specific for RSA algorithm, and ECDSA only accepts sign and verify… Is it acceptable to show it in usage?Or I should add some check of these parameters.

@yuzexiyzx
Copy link
Contributor Author

The valid width_bits of dh,rsa,ecc are also different.I cannot use a default value to make all scenes valid.

@jforissier
Copy link
Contributor

Maybe drop -t and -m and keep only -a with all the supported algorithms? That's a big list but it doesn't matter, it should be simpler than to check consistency of the parameters, and even easier to use too.

@yuzexiyzx
Copy link
Contributor Author

Maybe drop -t and -m and keep only -a with all the supported algorithms? That's a big list but it doesn't matter, it should be simpler than to check consistency of the parameters, and even easier to use too.

May I seperate this pull requerest into two? It would take some time to solve these problems.Is it acceptalbe to merge the refactoring of aes and hash first? I will add SM4 and asymmetric cipher algothms later.

@etienne-lms
Copy link
Contributor

May I seperate this pull requerest into two? It would take some time to solve these problems.Is it acceptalbe to merge the refactoring of aes and hash first? I will add SM4 and asymmetric cipher algothims later.

Sure. I agree it would help having the 1st commit be merged. Please create a new P-R for this commit. You can keep the applied review tags.

@yuzexiyzx yuzexiyzx changed the title xtest: add asymmetric cipher algorithm perf test xtest: combine AES and hash into crypto_perf Nov 20, 2023
@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <[email protected]>

@etienne-lms
Copy link
Contributor

IBART fails. @yuzexiyzx, could you rebase on master tip. I hope it will fix issues found by IBART.

@jforissier
Copy link
Contributor

Not rebased

@yuzexiyzx
Copy link
Contributor Author

Does rebasing means updating?
image
I have done this.

@etienne-lms
Copy link
Contributor

etienne-lms commented Nov 22, 2023

Does rebasing means updating?

No, this magic button (Update branch button of Github WebUI) will not do the job needed here.

One way is to cherry pick your change on top of latest OP-TEE master branch, and then force push the result to your branch. For more info, see https://optee.readthedocs.io/en/latest/general/contribute.html#finalizing-your-contribution

There is quite a bit of copy in these files.So, I cosonlidate the
code to make it easier to maintain.

Signed-off-by: Zexi Yu <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
@yuzexiyzx yuzexiyzx reopened this Nov 22, 2023
@yuzexiyzx
Copy link
Contributor Author

@etienne-lms I have done that.Is the status right now?

@etienne-lms
Copy link
Contributor

All good, can be merged IMO.

@jforissier
Copy link
Contributor

I updated the commit subject and fixed a white space error reported by checkpatch. The commit is now merged into master as e18381f.

Thanks @yuzexiyzx!

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.

4 participants