wolfssl icon indicating copy to clipboard operation
wolfssl copied to clipboard

Add RX64/RX71 SHA hardware support

Open Tathorack opened this issue 3 years ago • 3 comments

Description

Adding support for SHA1, SHA224, and SHA256 hardware acceleration on Renesas RX64 and RX71 microcontrollers. This follows the a similar pattern as the Renesas TSIP support.

Testing

Tested with WolfSSL 5.6.0 running on a RX64M, verified through debugging that hardware acceleration functions were being called for SHA256 hashes, verified that operation continued as expected after enabling hardware acceleration, verified that several different known values calculated the correct SHA256 hash.

Speed comparison for hashing 64KB of data on a RX64M

Hash Speed SHA1 SHA256
Hardware 4ms 4ms
Software 11ms 89ms

Checklist

  • [ ] added tests
  • [ ] updated/added doxygen
  • [ ] updated appropriate READMEs
  • [ ] Updated manual and documentation

Tathorack avatar Sep 16 '22 13:09 Tathorack

Can one of the admins verify this patch?

wolfSSL-Bot avatar Sep 16 '22 13:09 wolfSSL-Bot

Hi @TakayukiMatsuo

Please give this a cursory review.

embhorn avatar Sep 16 '22 13:09 embhorn

OK to test. No contributor agreement on file yet.

dgarske avatar Sep 16 '22 16:09 dgarske

Thank you very much for the work. Please take a look at my comments.

Thanks for the comments, it might be a while before I will have time to make updates

Tathorack avatar Oct 04 '22 14:10 Tathorack

@miyazakh I've made some updates addressing the comments. What would be the next step here?

Tathorack avatar Dec 01 '22 20:12 Tathorack

Hi @Tathorack I add other three comments about copyright. I am assigning @cconlon as a senior engineer review. He will review by himself or assign another senior engineer for final review.

Also, there are three PRB test failures that should be fixed.

miyazakh avatar Dec 03 '22 06:12 miyazakh

Hello @cconlon Assigning this to you as final review. I have left open three comments about copyright. Could you give comments for those too?

miyazakh avatar Dec 03 '22 06:12 miyazakh

Hi @Tathorack I add other three comments about copyright. I am assigning @cconlon as a senior engineer review. He will review by himself or assign another senior engineer for final review.

Also, there are three PRB test failures that should be fixed.

How do I get access to Jenkins to see what the test failures are? I created an account but it says "jhanserh is missing the Overall/Read permission". Otherwise is there a way I can run the tests locally?

Tathorack avatar Dec 09 '22 16:12 Tathorack

Retest this please. Last run was too long ago, so need to rerun to see

dgarske avatar Dec 09 '22 17:12 dgarske

Seems like I still cannot access the Jenkins builds

Tathorack avatar Dec 09 '22 18:12 Tathorack

Seems like I still cannot access the Jenkins builds

The failures are because you have not rebased this PR to latest master and some of the commit messages have failed. There is no real failures I can see. Please rebase, squash and force push the PR.

@cconlon or @kaleb-himes will need to look into if we can give you Jenkins permissions?

dgarske avatar Dec 09 '22 18:12 dgarske

Rebased and looks like all the checks have passed now

Tathorack avatar Dec 09 '22 20:12 Tathorack

Signed contributor agreement received. Starting tests.

embhorn avatar Dec 13 '22 17:12 embhorn

@embhorn @cconlon What are the next steps for getting this merged in?

Tathorack avatar Dec 20 '22 16:12 Tathorack

Hi @Tathorack

The contributor agreement and the verbiage have been accepted. I've restarted the tests.

Thanks, @embhorn

embhorn avatar Jan 05 '23 17:01 embhorn

@dgarske Do I need to rebase this again to fix the test failures?

Tathorack avatar Jan 10 '23 19:01 Tathorack

retest this please.

kaleb-himes avatar Jan 17 '23 22:01 kaleb-himes

@cconlon @TakayukiMatsuo

Please resume the review on this PR.

embhorn avatar Jan 17 '23 22:01 embhorn

@dgarske Do I need to rebase this again to fix the test failures?

Jumping in here sorry. Yes @Tathorack please go ahead and rebase so your changes are on top of commit history, the way our automated build system attempts to checkout the changes in some cases is still having issues replaying the changes on top of master.

kaleb-himes avatar Jan 17 '23 22:01 kaleb-himes

Update: Rebase resolved the checkout issues, thank you! I hit retry on the one failing test (some background stuff caused it to timeout prematurely, not a true failure)

kaleb-himes avatar Jan 18 '23 17:01 kaleb-himes

@embhorn what's the next steps for this PR?

Tathorack avatar Jan 27 '23 15:01 Tathorack

@cconlon

Reviewers have approved. Please merge when you are ready.

embhorn avatar Jan 27 '23 15:01 embhorn

@dgarske I've addressed your comments and rebased on master

Tathorack avatar Feb 22 '23 14:02 Tathorack

Retest this please

dgarske avatar Feb 23 '23 00:02 dgarske

Hi @Tathorack , making sure you saw my comments for a few minor cleanups. I'd like to get this merged. Thanks, David Garske, wolfSSL

dgarske avatar Mar 03 '23 16:03 dgarske

@dgarske I did see them, working on them now and some final testing. I should have some changes up by the end of the day. I noticed some of the tests failed, anything I need to do there to address them or should the be fixed with a rebase?

Tathorack avatar Mar 03 '23 17:03 Tathorack

Wonderful. Things looks good. The tests for pipeline is a new one we are working on to speed things up. The multi-test is the "C++-style comments". The other had a network failure and when you push will resolve.

dgarske avatar Mar 03 '23 17:03 dgarske