Skip to content

Enable crypto HW acceleration for STM32F437xG platforms #4898

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

Merged
merged 1 commit into from
Oct 13, 2017

Conversation

RobMeades
Copy link
Contributor

@RobMeades RobMeades commented Aug 14, 2017

Enable crypto HW acceleration for STM43F437xG platforms, i.e. ublox C030 family boards.

mbed-os-example-tls/benchmark/ before:

  SHA-256                  :       2936 KB/s
  SHA-512                  :        886 KB/s
  AES-CBC-128              :       2315 KB/s
  AES-CBC-192              :       2031 KB/s
  AES-CBC-256              :       1810 KB/s
  AES-GCM-128              :        708 KB/s
  AES-GCM-192              :        679 KB/s
  AES-GCM-256              :        652 KB/s
  AES-CCM-128              :        957 KB/s
  AES-CCM-192              :        857 KB/s
  AES-CCM-256              :        776 KB/s
  CTR_DRBG (NOPR)          :       1922 KB/s
  CTR_DRBG (PR)            :       1361 KB/s
  HMAC_DRBG SHA-256 (NOPR) :        337 KB/s
  HMAC_DRBG SHA-256 (PR)   :        296 KB/s
  RSA-2048                 :      20 ms/ public
  RSA-2048                 :     782 ms/private
  RSA-4096                 :      68 ms/ public
  RSA-4096                 :    4160 ms/private
  ECDHE-secp384r1          :     696 ms/handshake
  ECDHE-secp256r1          :     461 ms/handshake
  ECDHE-Curve25519         :     386 ms/handshake
  ECDH-secp384r1           :     343 ms/handshake
  ECDH-secp256r1           :     232 ms/handshake
  ECDH-Curve25519          :     192 ms/handshake

....and after:

  SHA-256                  :      15081 KB/s
  SHA-512                  :        886 KB/s
  AES-CBC-128              :      31542 KB/s
  AES-CBC-192              :      31542 KB/s
  AES-CBC-256              :      31542 KB/s
  AES-GCM-128              :        913 KB/s
  AES-GCM-192              :        913 KB/s
  AES-GCM-256              :        903 KB/s
  AES-CCM-128              :       2172 KB/s
  AES-CCM-192              :       2172 KB/s
  AES-CCM-256              :       2170 KB/s
  CTR_DRBG (NOPR)          :       8512 KB/s
  CTR_DRBG (PR)            :       5244 KB/s
  HMAC_DRBG SHA-256 (NOPR) :        470 KB/s
  HMAC_DRBG SHA-256 (PR)   :        411 KB/s
  RSA-2048                 :      20 ms/ public
  RSA-2048                 :     781 ms/private
  RSA-4096                 :      68 ms/ public
  RSA-4096                 :    4054 ms/private
  ECDHE-secp384r1          :     692 ms/handshake
  ECDHE-secp256r1          :     461 ms/handshake
  ECDHE-Curve25519         :     385 ms/handshake
  ECDH-secp384r1           :     339 ms/handshake
  ECDH-secp256r1           :     232 ms/handshake
  ECDH-Curve25519          :     199 ms/handshake

Test results for mbed-os-tests-mbedtls-selftest:

+-------------------------+-----------------+--------------------------------+---------------------------+--------+--------+--------+--------------------+
| target                  | platform_name   | test suite                     | test case                 | passed | failed | result | elapsed_time (sec) |
+-------------------------+-----------------+--------------------------------+---------------------------+--------+--------+--------+--------------------+
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-tests-mbedtls-selftest | mbedtls_entropy_self_test | 1      | 0      | OK     | 0.06               |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-tests-mbedtls-selftest | mbedtls_sha256_self_test  | 1      | 0      | OK     | 0.11               |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-tests-mbedtls-selftest | mbedtls_sha512_self_test  | 1      | 0      | OK     | 1.98               |
+-------------------------+-----------------+--------------------------------+---------------------------+--------+--------+--------+--------------------+

NOTE: this should only be merged once PR #5018 (which fixes an AES HW crypto driver issue) has been merged.

@theotherjimmy
Copy link
Contributor

retest uvisor

@theotherjimmy
Copy link
Contributor

@yanesca Could you review

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 15, 2017

retest uvisor

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 15, 2017

@RonEld @andresag01 please review tihs addition

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

Copy link

@andresag01 andresag01 left a comment

Choose a reason for hiding this comment

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

@RobMeades: Thanks for your contribution! The changes look good.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 17, 2017

retest uvisor

@andresag01
Copy link

@yanesca, @0xc0170: There was a recent bug report for AES hardware acceleration on STM32F439xI: #4928. This PR looks good to me, but given that STM32F439xI and STM32F437xG share the same aes_alt.h and aes_alt.c in this folder I suggest we test the change using the tls-client before merging.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 21, 2017

@yanesca, @0xc0170: There was a recent bug report for AES hardware acceleration on STM32F439xI: #4928. This PR looks good to me, but given that STM32F439xI and STM32F437xG share the same aes_alt.h and aes_alt.c in this folder I suggest we test the change using the tls-client before merging.

@RobMeades Can you confirm ?

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 21, 2017

retest uvisor

@RobMeades
Copy link
Contributor Author

@0xc0170 I only ran the benchmarks from within mbed-os-example-tls and these seemed to run as expected. I can confirm, though, that the HW crypto source files are shared between all STM32F4 devices, so if there's an issue it should be common.

@RobMeades
Copy link
Contributor Author

RobMeades commented Aug 21, 2017

@andresag01 , @yanesca, @0xc0170 : I've just tried running the tls-client example on this code and, with the change in #4934 not included, it returns:

Using Ethernet LWIP
Client IP Address is 10.38.20.108
Connecting with developer.mbed.org
Starting the TLS handshake...
TLS connection to developer.mbed.org established
Server certificate:

    cert. version     : 3
    serial number     : 65:7B:6D:8D:15:A5:B6:86:87:6B:5E:BC
    issuer name       : C=BE, O=GlobalSign nv-sa, CN=GlobalSign Organization Validation CA - SHA256 - G2
    subject name      : C=GB, ST=Cambridgeshire, L=Cambridge, O=ARM Ltd, CN=*.mbed.com
    issued  on        : 2017-04-03 13:54:02
    expires on        : 2018-05-06 10:31:02
    signed using      : RSA with SHA-256
    RSA key size      : 2048 bits
    basic constraints : CA=false
    subject alt name  : *.mbed.com, mbed.org, *.mbed.org, mbed.com
    key usage         : Digital Signature, Key Encipherment
    ext key usage     : TLS Web Server Authentication, TLS Web Client Authentication
Certificate verification passed

HTTPS: Received 438 chars from server
HTTPS: Received 200 OK status ... [OK]
HTTPS: Received 'Hello world!' status ... [OK]
HTTPS: Received message:

HTTP/1.1 200 OK
Server: nginx/1.11.12
Date: Mon, 21 Aug 2017 12:32:28 GMT
Content-Type: text/plain
Content-Length: 14
Connection: keep-alive
Last-Modified: Fri, 27 Jul 2012 13:30:34 GMT
Accept-Ranges: bytes
Cache-Control: max-age=36000
Expires: Mon, 21 Aug 2017 22:32:28 GMT
X-Upstream-L3: 172.17.0.4:80
X-Upstream-L2: developer-sjc-cyan-1-nginx
Strict-Transport-Security: max-age=31536000; includeSubdomains

Hello world!

This looks correct to me...?

@theotherjimmy
Copy link
Contributor

retest uvisor

@theotherjimmy
Copy link
Contributor

/morph test-nightly

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1064

All builds and test passed!

@yanesca
Copy link
Contributor

yanesca commented Aug 22, 2017

My current estimate is that I can start looking at this by the end of next week.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 23, 2017

retest uvisor

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 1, 2017

@yanesca, @0xc0170: There was a recent bug report for AES hardware acceleration on STM32F439xI: #4928. This PR looks good to me, but given that STM32F439xI and STM32F437xG share the same aes_alt.h and aes_alt.c in this folder I suggest we test the change using the tls-client before merging.

@RobMeades Is that correct? You above test says otherwise. We are currently disabling AES for those platform until it's fixed. If they share the implementation, how come this one does not fail?

@andresag01 I noticed you are happy with testing above, thus approved this addition?

@RobMeades
Copy link
Contributor Author

The HW crypto source files are certainly shared between all STM32F4 devices and I was able to pass the test. Maybe there is some other factor involved in causing the failure?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 1, 2017

The HW crypto source files are certainly shared between all STM32F4 devices and I was able to pass the test. Maybe there is some other factor involved in causing the failure?

that is what is unknown to me. To be certain, I would like to have one more test run on this target. @andresag01 @yanesca @RonEld What is your stand? Can you run quickly the test above on this platform to confirm the passing the test to accept this addition

@yanesca
Copy link
Contributor

yanesca commented Sep 1, 2017

I am sorry, but I still need to review this before we can accept it and I am even more sorry that I couldn't review this yet. Unfortunately at this point I have no estimate when can I do the review, but I will do it as soon as I can!

@RobMeades When you say that this has passed the test, do you mean compiling and successfully running on target the tls-client application from the https://github.com/ARMmbed/mbed-os-example-tls repository?

@RobMeades
Copy link
Contributor Author

@yanesca: yes, that's correct, with the change in #4934 removed so I am using HW crypto. I'll try it again from scratch on Monday just to be sure.

@RobMeades
Copy link
Contributor Author

RobMeades commented Sep 4, 2017

Well, it seems I was wrong. I've retrieved the latest https://github.com/ARMmbed/mbed-os-example-tls, made sure that tls-client has the latest mbed-os, added this change and then compiled and run the tls-client example on my STM32F437 target. If I have HW crypto on for AES it reports:

Using Ethernet LWIP
Client IP Address is 10.38.20.210
Connecting with developer.mbed.org
Starting the TLS handshake...
mbedtls_ssl_handshake() failed: -0x7780 (-30592): SSL - A fatal alert message was received from our peer

...and stops, while if I switch off HW crypto for AES it reports:

Using Ethernet LWIP
Client IP Address is 10.38.20.210
Connecting with developer.mbed.org
Starting the TLS handshake...
TLS connection to developer.mbed.org established
.etc...

...and ends with "Hello world!"

So I guess this is the issue reported in #4928. Over in #4928 I see that Martin comments: "The fix will get CI once CI is back running", but I can't see anything in the comments to indicate that there is a fix. If there is one in the pipeline, this PR can wait for it. If there is not, I will re-submit this PR with MBEDTLS_AES_ALT left out.

@RobMeades
Copy link
Contributor Author

I suggest that we only merge this change once #5018 has been merged, I will update the description to clarify that.

@ciarmcom
Copy link
Member

ciarmcom commented Sep 7, 2017

ARM Internal Ref: IOTSSL-1720

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 28, 2017

I suggest that we only merge this change once #5018 has been merged, I will update the description to clarify that.

That PR was merged. What is remaining here?

@andresag01
Copy link

@0xc0170: We should wait for @yanesca to approve the changes as well before merging.

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 10, 2017

@RobMeades All good with this one, to trigger CI?

@RobMeades
Copy link
Contributor Author

Yup, should be no other dependencies.

@mbed-ci
Copy link

mbed-ci commented Oct 10, 2017

Build : FAILURE

Build number : 54
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4898/

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 11, 2017

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 11, 2017

Build : SUCCESS

Build number : 118
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4898/

Triggering tests

/test mbed-os

@mbed-ci
Copy link

mbed-ci commented Oct 12, 2017

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 12, 2017

This patch is now ready for integration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants