Thread class tz #6486

deepikabhavnani · 2018-03-27T18:23:27Z

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 · 2018-03-27T19:20:35Z

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

cmonr · 2018-03-28T03:34:02Z

@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.

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 · 2018-03-28T07:47:14Z

@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 · 2018-03-28T12:48:33Z

@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 · 2018-03-28T20:12:47Z

./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*)'

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.

pan- · 2018-04-04T08:53:38Z

rtos/Thread.h

    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)


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.

@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.

pan- · 2018-04-04T08:59:13Z

rtos/Thread.h

@@ -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.


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 ?

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.

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 · 2018-04-10T07:58:06Z

@ccli8 @cyliangtw @SenRamakri

please review (approve/change requested)

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

0xc0170 · 2018-04-16T13:23:43Z

/morph build

mbed-ci · 2018-04-16T13:54:01Z

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 · 2018-04-16T16:30:25Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-04-16T21:03:52Z

Test : SUCCESS

Build number : 1564
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6486/1564

cmonr · 2018-04-17T05:12:17Z

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

0xc0170 · 2018-04-17T10:17:11Z

@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 · 2018-04-17T14:42:58Z

@0xc0170 - This PR is not dependent on CMSIS update, required CMSIS changes are already part of master branch. https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/rtx5/RTX/Include/rtx_os.h#L129
https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_thread.c#L606

deepikabhavnani force-pushed the thread_class_tz branch 2 times, most recently from dd3b64c to 0bdaaee Compare March 27, 2018 19:06

cmonr requested review from bulislaw, sg- and SenRamakri March 28, 2018 03:33

cmonr added the needs: review label Mar 28, 2018

bulislaw reviewed Mar 28, 2018

View reviewed changes

deepikabhavnani force-pushed the thread_class_tz branch 2 times, most recently from 5cffb24 to 52caf5e Compare March 28, 2018 18:22

deepikabhavnani force-pushed the thread_class_tz branch from 52caf5e to 8dfff17 Compare March 28, 2018 21:03

Trust zone module identifier added to thread class

044dfb1

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 force-pushed the thread_class_tz branch from 8dfff17 to 044dfb1 Compare March 28, 2018 21:06

deepikabhavnani requested a review from pan- March 29, 2018 21:57

Overloaded ctor for new parameter tz_module

ceb44f9

deepikabhavnani force-pushed the thread_class_tz branch from f4fe709 to ceb44f9 Compare March 29, 2018 22:01

pan- reviewed Apr 4, 2018

View reviewed changes

Added description of tz_module and MBED_TZ_DEFAULT_ACCESS

48002ff

ccli8 approved these changes Apr 10, 2018

View reviewed changes

cyliangtw approved these changes Apr 10, 2018

View reviewed changes

0xc0170 approved these changes Apr 10, 2018

View reviewed changes

pan- approved these changes Apr 11, 2018

View reviewed changes

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

cmonr added the release-version: 5.9.0-rc1 label Apr 17, 2018

SenRamakri approved these changes Apr 20, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Apr 20, 2018

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

cmonr removed the ready for merge label Apr 23, 2018

deepikabhavnani deleted the thread_class_tz branch April 23, 2018 18:38

cyliangtw mentioned this pull request May 22, 2018

Support default tz_module in event loop thread for M23/M33 #6978

Merged

Thread class tz #6486

Thread class tz #6486

Uh oh!

Conversation

deepikabhavnani commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

deepikabhavnani commented Mar 27, 2018

Uh oh!

cmonr commented Mar 28, 2018

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 28, 2018

Uh oh!

deepikabhavnani commented Mar 28, 2018

Uh oh!

deepikabhavnani commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 16, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 16, 2018

Uh oh!

cmonr commented Apr 17, 2018

Uh oh!

0xc0170 commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Apr 17, 2018

Uh oh!

Uh oh!

deepikabhavnani commented Mar 27, 2018 •

edited

Loading

0xc0170 commented Apr 17, 2018 •

edited

Loading