-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add ARM_MUSCA_A1 target #9221
Add ARM_MUSCA_A1 target #9221
Conversation
@mikisch81, thank you for your changes. |
@mikisch81 Bare metal profile is not planned to handle changing the meaning of build profiles, and I don't think that it's a good idea (it's mixing concerns for 2, currently independent ideas: what code to include and what flags to use). |
@mikisch81 could you expand on why you need the build profiles? what is different between matching profiles? |
@theotherjimmy I encountered 2 issues when trying to build TF-M secure code using mbed-os c like:
|
|
||
#include "../cmsis_nvic.h" | ||
|
||
LR_CODE NS_CODE_START NS_CODE_SIZE { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NS_CODE_START
and NS_CODE_SIZE
- Where are these defines set? Are those configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are defined in region_defs.h, cmsis_nvic.h includes this file.
But to be honest I think I will add an explicit include in the ld/sct files as well (explicit is better than implicit 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikisch81 - We have few common defines (RAM / ROM start and size) already available in the tools, which are picked from CMSIS packages and passed as defines. It will be good to use the same instead of adding new ones.
Please check in build of MUSCA if MBED_RAM_START
MBED_ROM_START
are defined, if yes you can use them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepikabhavnani Yes, I know these defines, we use them in PSOC6.
MUSCA build doesn't define them, it takes the values from region_defs.h
which is from the TF-M port of Musca.
I can add a block like in PSOC6 which will set these defines accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will appreciate if defines in region_defs.h
and linker files are updated and used as PSOC6
, it will give flexibility to users instead of hard-coded values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be relevant only to NS side, as the secure side only runs TF-M on bare-metal.
Something like this in the NS linker scripts:
#if !defined(MBED_ROM_START)
#define MBED_ROM_START NS_CODE_START
#endif
#if !defined(MBED_ROM_SIZE)
#define MBED_ROM_SIZE ((TOTAL_CODE_SRAM_SIZE / 2) - BL2_HEADER_SIZE)
#endif
#if !defined(MBED_RAM_START)
#define MBED_RAM_START NVIC_RAM_VECTOR_LIMIT
#endif
#if !defined(MBED_RAM_SIZE)
#define MBED_RAM_SIZE (NS_DATA_SIZE - NVIC_RAM_VECTOR_SIZE)
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TF-M though a bare-metal layer, it has linker files https://github.com/ARMmbed/mbed-os/pull/9221/files#diff-477bb268cd4ef63fbe0f6f161bbfeb7cR25
Change is relevant for secure + non-secure side as well. Idea is to have defines at one place, instead of hard-coded in code, so if someone wants to change the size of secure / non-secure RAM / ROM it all should be at one place. Now it looks like it is region_defs.h
I will propose something like this instead
#if !defined(MBED_ROM_START)
#define MBED_ROM_START 0x10200000
#endif
#if !defined(MBED_ROM1_START)
#define MBED_ROM1_START 0x00200000
#endif
#define S_ROM_ALIAS_BASE MBED_ROM_START
#define NS_ROM_ALIAS_BASE MBED_ROM1_START
#define TOTAL_ROM_SIZE (MBED_ROM_SIZE + MBED_ROM1_SIZE)
MBED_ROM_START / MBED_ROM_SIZE - Will map to IROM1
MBED_ROM1_START / MBED_ROM1_SIZE - Will map to IROM2
and same is for RAM
LR_CODE S_CODE_START { | ||
|
||
/**** This initial section contains common code for TEE */ | ||
ER_TFM_CODE S_CODE_START S_CODE_SIZE { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S_CODE_START
and S_CODE_SIZE
- From where are these values parsed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use value from targets.json inherited from PSA_Target:
"overrides": {
"secure-rom-start": "xx",
"secure-rom-size": "xx",
"non-secure-rom-start": "xx",
"non-secure-rom-size": "xx",
"secure-ram-start": "xx",
"secure-ram-size": "xx",
"non-secure-ram-start": "xx",
"non-secure-ram-size": "xx",
"shared-ram-start": "xx",
"shared-ram-size": "xx"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeromecoutant This comment is outdated and not relevant anymore.
targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/device/COMPONENT_SPE/TOOLCHAIN_ARMC6/musca_s.sct
Outdated
Show resolved
Hide resolved
|
||
} | ||
|
||
LR_VENEER CMSE_VENEER_REGION_START { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is CMSE_VENEER_REGION_START
defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this answer
Since TFM source is pulled in as well, do we need tfm.bin and cmse_lib.o? Mcuboot.bin might still be needed. |
Eventually we will need some default pre-compiled binaries in the tree (for CI), need to agree on the location. |
@ARMmbed/mbed-os-maintainers |
That's a new platform support, we'll need full test result on all 3 compilers. Why do we need the certificate in the sources? What are the changes to the build profiles and how users are supposed to use the new files? I don't want to force everyone to know about and manually pass path to the files. |
@bulislaw - IAR is not supported for ARMv8 devices (IAR 8.x dependency) |
Do you mean the private key used by
I agree, see my answer to @theotherjimmy, bullet 2 is not relevant anymore as I was able today to build TF-M for ARMC6 with |
For .bin and .o files, please add readme with description (useful to have how it was compiled - version, and what it contains. also the info from above, when it should be precompiled, might not be clear to everyone?) and also license file |
- 68KB Secure - 60KB Non-secure
Imports working McuBoot for reset. Updates microsec ticker driver. Default baudrate is set to 115200 to see TF-M boot messages. Stack top is set to scatter file dependent and not hard-coded.
Use build directory instead of temp directory for for intermediate files during binaries merge.
@Devran01 I;ve rebased on master and fixed one of your issues. regarding your last comment i dont see the issue |
@orenc17 Thanks. Also, I couldn't see any issue related to this. I'll create an issue. |
@Devran01, |
CI started |
@alzix Thanks for your inputs. I'll create a ticket for future reference. |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
This is now ready for integration |
Description
This PR replaces #8845 as the proting PR of ARM Musca A1.
These targets are defined:
ARM_MUSCA_A1 as the base target with common code for Secure/Non-secure worlds.
ARM_MUSCA_A1_NS as the non-secure target.
ARM_MUSCA_A1_S as the secure target.
So to build an overall Musca_a1 image containing secure and non-secure with a pre-compiled bootloader just run these commands:
To build special SPM IPC tests do as follows:
Full run of Greentea tests hasn't been run yet, unitesting it by running the mbed-os-tests-psa-its and mbed-os-tests-mbed-crypto-sanity test suites.
Currently IAR is not supported.
TODO list
[ ] Full run of Greentea tests
Pull request type
Reviewers
@gaborkertesz @theotherjimmy @ashok-rao @dreemkiller @deepikabhavnani @bridadan @alzix