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

Thread class tz #6486

Merged
merged 3 commits into from Apr 23, 2018

Conversation

Projects
None yet
9 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Mar 27, 2018

Description

tz_module Thread attribute for trustzone identifier is added in RTX, to allow non-secure thread to access secure functions. Adding access of this parameter in Thread class constructor.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

@ccli8 @cyliangtw

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:thread_class_tz branch 2 times, most recently from dd3b64c to 0bdaaee Mar 27, 2018

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Mar 27, 2018

@0xc0170 - Optional parameter uint32_t tz_module is added to thread constructor. Is that considered as breaking change?

@cmonr cmonr requested review from bulislaw, sg- and SenRamakri Mar 28, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 28, 2018

@deepikabhavnani It's definitely considered a feature, but I don't think this would be considered a breaking change, especially since its an optional addition.

@cmonr cmonr added the needs: review label Mar 28, 2018

@bulislaw

I'm very concerned with not having docs for that:

  • doxygen
  • Some proper technology page in handbook explaining what is it tz_module
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 28, 2018

@deepikabhavnani It's definitely considered a feature, but I don't think this would be considered a breaking change, especially since its an optional addition.

it is a binary breakage so would be careful , as odin might be affected. To have it in constructor method OK, but Thread - why not overload it and add it there?

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Mar 28, 2018

@bulislaw - Will add docs for sure.

@0xc0170 - All thread parameters are optional, if I add additional optional parameter then it will complain of multiple definitions found. I can have this as mandatory first parameter in case of overload, will that be fine?

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:thread_class_tz branch 2 times, most recently from 5cffb24 to 52caf5e Mar 28, 2018

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Mar 28, 2018

./mbed-os/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/TARGET_MODULE_UBLOX_ODIN_W2/sdk/TOOLCHAIN_GCC_ARM/libublox-odin-w2-driver.a(cb_main.o): In function `cbMAIN_startOS()':
cb_main.cpp:(.text._Z14cbMAIN_startOSv+0x3a): undefined reference to `rtos::Thread::constructor(osPriority_t, unsigned long, unsigned char*, char const*)'
./mbed-os/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/TARGET_MODULE_UBLOX_ODIN_W2/sdk/TOOLCHAIN_GCC_ARM/libublox-odin-w2-driver.a(cb_ticker_wrapper.o): In function `cbTICKER_WRAPPER_attach':
cb_ticker_wrapper.cpp:(.text.cbTICKER_WRAPPER_attach+0x44): undefined reference to `rtos::Thread::constructor(osPriority_t, unsigned long, unsigned char*, char const*)'

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:thread_class_tz branch from 52caf5e to 8dfff17 Mar 28, 2018

Trust zone module identifier added to thread class
Non-Secure threads can access secure calls only when tz_module attribute is set
as 1(OS_SECURE_CALLABLE_THREAD), while thread creation.
Hence adding tz_module as an argument to ctor.

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:thread_class_tz branch from 8dfff17 to 044dfb1 Mar 28, 2018

@deepikabhavnani deepikabhavnani requested a review from pan- Mar 29, 2018

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:thread_class_tz branch from f4fe709 to ceb44f9 Mar 29, 2018

Thread(osPriority priority=osPriorityNormal,
uint32_t stack_size=OS_STACK_SIZE,
unsigned char *stack_mem=NULL, const char *name=NULL) {
constructor(priority, stack_size, stack_mem, name);
}

/** Allocate a new thread without starting execution
@param tz_module trustzone thread identifier (osThreadAttr_t::tz_module)

This comment has been minimized.

@pan-

pan- Apr 4, 2018

Member

What's a trustzone thread identifier ? How a user gets one and how does it alter a thread behavior ?

It would be interesting to have pointers to those answers in the function documentation.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 10, 2018

Contributor

@0xc0170 @pan- I have added description after this comment. Description is below this line, hence the comment is not outdated.
Does below description explain enough?

Context of RTOS threads in non-secure state must be saved when calling secure functions. tz_module ID is used to allocate context memory for threads, and it can be safely set to zero for threads not using secure calls at all. See "TrustZone RTOS Context Management" for more details.

@@ -73,6 +73,15 @@ namespace rtos {
* and underlying RTOS objects (static or dynamic RTOS memory pools are not being used).
* Additionally the stack memory for this thread will be allocated on the heap, if it wasn't supplied to the constructor.
*/

/* This flag can be used to change the default access of all threads in non-secure mode.

This comment has been minimized.

@pan-

pan- Apr 4, 2018

Member

Is there any reason to have this in the Thread header file ? Looks to me that it is only used as the default tz_module value used in implementation file.

If this value is useful for users; could you move this block above the thread documentation and doxygen it ?

Could you also indicates what it means if MBED_TZ_DEFAULT_ACCESS is set to 0 ?

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 4, 2018

Contributor

Is there any reason to have this in the Thread header file ?

I wanted to leave the description in header file, since different target devices can use it the way they want. In case of Nuvoton few of there peripherals are in secure mode, hence they need all user threads to have secure access (specially for green-tea testing). But some other target device might keep it as no secure access for user threads.

This comment has been minimized.

@cyliangtw

cyliangtw Apr 10, 2018

Contributor

For compatibility and minimized impact, this is the better way for target specific setting and we could pass all green-tea & CI-Test on Nuvoton Cortex-M23 chip based on this change.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

@ccli8 @cyliangtw @SenRamakri

please review (approve/change requested)

@ccli8

ccli8 approved these changes Apr 10, 2018

@0xc0170

It looks fine to me, but have similar question to what @pan- asked here https://github.com/ARMmbed/mbed-os/pull/6486/files#r179068247

@pan-

pan- approved these changes Apr 11, 2018

@0xc0170 0xc0170 added needs: review and removed needs: review labels Apr 11, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 16, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 16, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

@sg- @bulislaw @SenRamakri Care to review/re-review?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

@deepikabhavnani One check - isn't this dependent on the CMSIS update that is currently under review? If yes, should be referenced and let us know about this.

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Apr 17, 2018

@cmonr cmonr merged commit 5fcc961 into ARMmbed:master Apr 23, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8974 cycles (-321 cycles)
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 23, 2018

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:thread_class_tz branch Apr 23, 2018

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