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.

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

TF-M: Add SECURE_UART1 Kconfig option #5574

Merged
merged 2 commits into from
Oct 6, 2021
Merged

TF-M: Add SECURE_UART1 Kconfig option #5574

merged 2 commits into from
Oct 6, 2021

Conversation

Vge0rge
Copy link
Contributor

@Vge0rge Vge0rge commented Sep 9, 2021 •
edited
Loading

-This adds a Kconfig configuration option for SECURE_UART1
option of TF-M

Ref: NCSIDB-551

Signed-off-by: Georgios Vasilakis georgios.vasilakis@nordicsemi.no

@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. CI-matter-test manifest labels Sep 9, 2021
@github-actions
Copy link

github-actions bot commented Sep 9, 2021 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@b236d8c nrfconnect/sdk-nrfxlib@8103747 (master) nrfconnect/sdk-nrfxlib@b236d8c9..81037475
trusted-firmware-m nrfconnect/sdk-trusted-firmware-m@ca81d4a nrfconnect/sdk-trusted-firmware-m@d2e9315 (master) nrfconnect/sdk-trusted-firmware-m@ca81d4a7..d2e93153

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@Vge0rge Vge0rge marked this pull request as ready for review September 20, 2021 07:49
SebastianBoe
SebastianBoe approved these changes Sep 20, 2021
View reviewed changes
modules/tfm/tfm/boards/common/plat_init.c Outdated Show resolved Hide resolved
@mglettig
Copy link

mglettig commented Sep 21, 2021 •
edited
Loading

I observed that when setting CONFIG_TFM_SECURE_UART1=n and also setting the following Kconfig options

CONFIG_TFM_PARTITION_INITIAL_ATTESTATION=n
CONFIG_TFM_CRYPTO_KEY_DERIVATION_MODULE_ENABLED=n

The firmware no longer boots. You can reproduce it with NRF-SDK v1.7.0-rc1 and the crypto/rsa sample.
Kind regards,
Michael

@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Sep 21, 2021
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Sep 21, 2021
@Vge0rge
Copy link
Contributor Author

Vge0rge commented Sep 21, 2021

I observed that when setting CONFIG_TFM_SECURE_UART1=n and also setting the following Kconfig options

CONFIG_TFM_PARTITION_INITIAL_ATTESTATION=n
CONFIG_TFM_CRYPTO_KEY_DERIVATION_MODULE_ENABLED=n

The firmware no longer boots. You can reproduce it with NRF-SDK v1.7.0-rc1 and the crypto/rsa sample.
Kind regards,
Michael

@mglettig Thank you for the feedback! I could reproduce the issue that you are saying. I forced to this PR and now your issue should be solved. Let me know if that works for you :)

@mglettig
Copy link

@mglettig Thank you for the feedback! I could reproduce the issue that you are saying. I forced to this PR and now your issue should be solved. Let me know if that works for you :)

You're welcome. Thank you for the quick fix. I tested it and it works now for me.
Looking forward to having this changes back on master.

Kind regards,
Michael

tejlmand
tejlmand requested changes Sep 29, 2021
View reviewed changes
modules/tfm/tfm/boards/CMakeLists.txt Outdated Show resolved Hide resolved
modules/trusted-firmware-m/Kconfig Show resolved Hide resolved
@Vge0rge Vge0rge force-pushed the disable_uart_tfm branch 2 times, most recently from 16a2ed7 to f917835 Compare September 29, 2021 12:50
@tejlmand
Copy link
Contributor

@Vge0rge I was testing this PR with the tfm_hello_world sample and was getting this error.

CMake Warning (dev) at /projects/github/ncs/nrfxlib/nrf_security/cmake/extensions.cmake:652:
  Syntax Warning in cmake code at column 32                                                                                                                                                                                                                                                                                 

  Argument not separated from preceding token by whitespace.                                                                                                                                                                                                                                                                
Call Stack (most recent call first):                                                                                                                                                                                                                                                                                        
  /projects/github/ncs/nrfxlib/nrf_security/CMakeLists.txt:185 (include)                                                                                                                                                                                                                                                    
This warning is for project developers.  Use -Wno-dev to suppress it.

This has been fixed here:
nrfconnect/sdk-nrfxlib#561

so you may include that PR in your manifest update.

If you prefer not to do so, let me know, and i'll open a dedicated manifest PR with the fix.

tejlmand
tejlmand approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm.

Only a minor nit noticed.

modules/trusted-firmware-m/Kconfig Outdated Show resolved Hide resolved
@Vge0rge
Copy link
Contributor Author

Vge0rge commented Oct 1, 2021

@Vge0rge I was testing this PR with the tfm_hello_world sample and was getting this error.

CMake Warning (dev) at /projects/github/ncs/nrfxlib/nrf_security/cmake/extensions.cmake:652:
  Syntax Warning in cmake code at column 32                                                                                                                                                                                                                                                                                 

  Argument not separated from preceding token by whitespace.                                                                                                                                                                                                                                                                
Call Stack (most recent call first):                                                                                                                                                                                                                                                                                        
  /projects/github/ncs/nrfxlib/nrf_security/CMakeLists.txt:185 (include)                                                                                                                                                                                                                                                    
This warning is for project developers.  Use -Wno-dev to suppress it.

This has been fixed here: nrfconnect/sdk-nrfxlib#561

so you may include that PR in your manifest update.

If you prefer not to do so, let me know, and i'll open a dedicated manifest PR with the fix.

I will do it @tejlmand, it's no problem.

@github-actions github-actions bot added CI-all-test Run All integration tests manifest-nrfxlib labels Oct 1, 2021
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Oct 1, 2021
@Vge0rge Vge0rge removed the CI-all-test Run All integration tests label Oct 1, 2021
@github-actions github-actions bot added CI-all-test Run All integration tests and removed DNM labels Oct 5, 2021
@Vge0rge Vge0rge removed the CI-all-test Run All integration tests label Oct 5, 2021
@github-actions github-actions bot added the CI-all-test Run All integration tests label Oct 5, 2021
@Vge0rge Vge0rge removed the CI-all-test Run All integration tests label Oct 5, 2021
Vge0rge added 2 commits October 5, 2021 15:05
@Vge0rge
TF-M: Don't init sdio when SECURE_UART1 is disabled
d982caa 
-This removes the stdio_init call when the SECURE_UART1
 is disabled in TF-M. This call will cause a bus fault
 when the SECURE_UART1 is disabled.
-It also only uses the plat_init.c in our downstream
 tf-m repo

Ref: NCSIDB-551

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
@Vge0rge
manifest: TF-M repos with SECURE_UART1
b523210 
-This fetches the repo that have the configuration
 for the SECURE_UART1 option
-This also fetches a nrfxlib PR with a cmake fix

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
@github-actions github-actions bot added the CI-all-test Run All integration tests label Oct 5, 2021
@Vge0rge Vge0rge removed the CI-all-test Run All integration tests label Oct 5, 2021
@rlubos rlubos merged commit ee39d61 into nrfconnect:master Oct 6, 2021
@Vge0rge Vge0rge deleted the disable_uart_tfm branch November 9, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tejlmand tejlmand tejlmand approved these changes

@oyvindronningstad oyvindronningstad oyvindronningstad approved these changes

@SebastianBoe SebastianBoe SebastianBoe approved these changes

@hakonfam hakonfam Awaiting requested review from hakonfam

@sigvartmh sigvartmh Awaiting requested review from sigvartmh

@ioannisg ioannisg Awaiting requested review from ioannisg

@carlescufi carlescufi Awaiting requested review from carlescufi

@frkv frkv Awaiting requested review from frkv

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. manifest manifest-nrfxlib manifest-trusted-firmware-m manifest-zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Vge0rge @mglettig @NordicBuilder @hakonfam @SebastianBoe @tejlmand @oyvindronningstad @rlubos