Skip to content

STM32L486RG/mbedtls: add aes hw acceleration #3962

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

STM32L486RG/mbedtls: add aes hw acceleration #3962

wants to merge 1 commit into from

Conversation

adustm
Copy link
Member

@adustm adustm commented Mar 17, 2017

Description

Add hw acceleration for mbedTLS/aes on NUCLEO_L486RG
Please be aware that this device does not support 192bit key size.
The aes selftest and gcm selftest shall be modified not to run this key size. As it is located in the official mbedtls stack, I did not touch it.

Status

READY

Steps to test or reproduce

Run aes seltest from TESTS/mbedtls/selftest/main.cpp

+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| target            | platform_name | test suite             | test case                   | passed | failed | result | elapsed_time (sec) |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.9                |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.11               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.13               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 6.56               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.1                |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 1.75               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.31               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.53               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 1.66               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 4.51               |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 20, 2017

@sbutcher-arm Please review

@0xc0170 0xc0170 requested a review from simonbutcher March 20, 2017 11:13
RonEld
RonEld suggested changes Mar 29, 2017
View reviewed changes
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.

@adustm I reviewed your changes, two major comments:

  1. mbedtls_device.h should have only the mbed TLS configuration defintions, specifically, the target specific mbed TLS definitions.
  2. The alternative implementation should support all the API supported by the SW one. For example, if AES192 is not supported by your HW, you should use the SW implementation for AES192

features/mbedtls/targets/TARGET_STM/TARGET_STM32L4/TARGET_NUCLEO_L486RG/mbedtls_device.h
//the following defines are provided to maintain compatibility between STM32 families
#define __HAL_RCC_CRYP_CLK_ENABLE __HAL_RCC_AES_CLK_ENABLE
#define __HAL_RCC_CRYP_FORCE_RESET __HAL_RCC_AES_FORCE_RESET
#define __HAL_RCC_CRYP_RELEASE_RESET __HAL_RCC_AES_RELEASE_RESET
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these implementation specific definitions?
Shouldn't these be in some STM common header file, or in the aes_alt.c file?
mbedtls_device.h should be included only from mbedtls files(specifically platform_mbed.h), so I think these definitions will not be used anywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I'll move them

features/mbedtls/targets/TARGET_STM/TARGET_STM32L4/TARGET_NUCLEO_L486RG/mbedtls_device.h
#define __HAL_RCC_CRYP_CLK_ENABLE __HAL_RCC_AES_CLK_ENABLE
#define __HAL_RCC_CRYP_FORCE_RESET __HAL_RCC_AES_FORCE_RESET
#define __HAL_RCC_CRYP_RELEASE_RESET __HAL_RCC_AES_RELEASE_RESET
#define CRYP AES
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this definition called form?

features/mbedtls/targets/TARGET_STM/aes_alt.c
memcpy(ctx->aes_key, key, 24);
break;
#if defined (TARGET_STM32L486xG)
return(MBEDTLS_ERR_AES_INVALID_KEY_LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative implementation should support the full API. If HW doesn't support, then the SW implementation should be used

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like to increase the SW size for this feature, but I understand the need.
I'll have to rework the pull request to use SW in case of 192bits key size. @0xc0170 I guess you can add 'do not merge' label at the moment.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2017

@adustm Please review the comments above

@sg-
Copy link
Contributor

sg- commented Apr 10, 2017

@adustm Should this be moved against the workshop branch?

@adustm adustm mentioned this pull request Apr 11, 2017
Merged
@adustm adustm closed this Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonEld RonEld RonEld requested changes

@simonbutcher simonbutcher Awaiting requested review from simonbutcher

Assignees
No one assigned
Labels
needs: review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adustm @0xc0170 @sg- @RonEld