Update cmsis/rtx to version 5.3 #6273

bulislaw · 2018-03-05T15:57:18Z

Description

Update CMSIS and RTX to version 5.3.

Pull request type

@deepikabhavnani could you have a look at the v8 integration.

bulislaw · 2018-03-05T16:49:35Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

@@ -97,8 +91,7 @@ static void default_idle_hook(void)
 {
    uint32_t elapsed_ticks = 0;

-    core_util_critical_section_enter();


@c1728p9 we need to review this critical section. RTX5.3 marks the svcRtxKernelSuspend/Resume as static so we can't use it here and I'd rather not modify RTX again. Can you remember why are we entering the critical section before suspending the kernel?

The general problem is that the check that it is safe to enter sleep and the act of entering sleep need to be done atomically. If this is not done an interrupt could change state by waking a thread before WFI or WFE has been called.

This problem is visible this Tick-less Low-Power Operation code:

void osRtxIdleThread (void) { for (;;) { tc_wakeup = osKernelSuspend(); /* Is there some time to sleep? */ if (tc_wakeup > 0) { tc = 0; /* Enter the low power state */ MSP432_LP_Entry(); //<- If an interrupt signals a thread to wake up at this point will the call to WFE() won't wake because of it. There is a thread that should be running now but won’t be able to until tc_wakeup ticks have passed. __WFE(); } /* Adjust the kernel ticks with the amount of ticks slept */ osKernelResume (tc); } }

@JonatanAntoni I don't think it's currently possible to protect against this race using RTX APIs, do you guys have a way of dealing with it?

__WFE should not have a race here, only __WFI would be problematic to my knowledge.
To my understanding each interrupt that is enabled to set the event register should do so. __WFE returns immediately if the event register is already set. Otherwise it puts the device to sleep until the event flag is set by any active event source. __WFE clears the event register on return.

For instance refer to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0553a/CHDEAAHJ.html

That sounds good to me, I had a look at the Architecture Reference and couple of manuals for specific designs from partners and it seems that no one does anything weird. Anyone against replacing WFI with WFE? @c1728p9 @0xc0170

That solution sounds good to me as well as long as we don't have any devices with the errata mentioned here. I was under the impression that __WFE required any interrupt that could wake the system to call __SEV, which is not the case. This should be set by hardware automatically and its just errata that caused it not to be set for some devices.

Lets change it then and resolve this internal API usage

bulislaw · 2018-03-15T16:53:39Z

Updated to use WFE

bulislaw · 2018-03-19T10:48:46Z

@c1728p9 @0xc0170 I've replaced the WFI with WFE now.

0xc0170

Looks fine

I checked the wfe changes, found that few drivers are used in our codebase (I identified silicon labs, freescale, maxim, nuvoton - they invoke their driver sleep functions that use WFI by default, I could not locate WFE counterparts, thus have to change their driver code or implement sleep API without driver invocation).

0xc0170 · 2018-03-19T11:26:36Z

targets/TARGET_ONSEMI/TARGET_NCS36510/sleep.c

@@ -7,11 +7,11 @@
 * $Rev:  0.1 $
 * $Date: 01-21-2016 $
 ******************************************************************************
- * Copyright 2016 Semiconductor Components Industries LLC (d/b/a �ON Semiconductor�).
+ * Copyright 2016 Semiconductor Components Industries LLC (d/b/a �ON Semiconductor�).


why are these changed? github does not show the characters properly now?

Weird, must be some encoding issue with the file or they are not " but some weird unicode and my editor went crazy seeing that. I'll revert that.

kjbracey · 2018-03-21T13:36:56Z

I recently posted a bit of a "WFI/WFE for dummies" here:

https://os.mbed.com/forum/mbed/topic/29547/?page=1#comment-55419

The normal thing for an idle/sleeping OS to do is disable interrupts, shut down hardware, WFI, restart hardware, enable interrupts. And that's a privileged operation. You must disable interrupts around WFI. It's an OS instruction, and the interrupt disable makes the entire thing atomic, including whatever special hardware shutdown you're doing.

WFE is an unprivileged instruction for applications, who can't disable interrupts and just need to wait for something like a spinlock to be released, but don't want to yield back to the OS. I don't think it's what you should be doing here.

kjbracey

I'm not convinced the WFE change is conceptually right.

kjbracey · 2018-03-21T13:46:45Z

As I've been discussing this sort of thing with @c1728p9 - the logic of WFI is similar to that of a condition variable. The condition variable wait assumes you must be holding a mutex to be inspecting the condition you're monitoring without a race, so incorporates that mutex to make it atomic.

Similarly WFI assumes you must be in a critical section to be doing hardware shutdown without a race, and thus is designed to wake for an interrupt source even though you're in that critical section.

It's not clear to me how removing the critical section you have is safe.

kjbracey · 2018-03-22T11:55:37Z

This change also effectively changes the unspecified API of sleep() by no longer calling it from a critical section.

Stared at the RTX implementation a bit more - the suspend call to RTX is notionally atomic, and designed to work when called from normal thread context. After exit, the OS is suspended, with its timers stopped.

But interrupts are still enabled so device drivers could be queuing stuff for RTX post-processing - this could potentially quite rapidly hit ISR queue overflow, as the post-processing is suspended.

And this post-processing is a race - if an interrupt decides to release a semaphore after osKernelSuspend, osRtxPostProcess will set osRtxInfo.kernel.pendSV. We need to make sure we don't sleep if that happens. Now, using WFE could maybe avoid that - the interrupt setting pendSV would set the event flag. But the catch is that WFE must be used with interrupts enabled (always on ARMv7A, and unless a special config flag is set on ARMv7M) - a masked interrupt doesn't cause an event. If a hardware shutdown is (as seems likely) wants to shut off interrupts as part of the hardware shutdown, they can't portably use WFE.

To make this atomic and not change sleep semantics, I would suggest:

osKernelSuspend();              // outside critical section only because we're forced to be
critical_section_enter();       // to stop any more device activity
if (!osRtxInfo.kernel.pendSV) { // check devices didn't run any interrupt handlers kicking the OS between suspend and critical section
    sleep();                    // uses WFI - will wake as soon as any pending interrupts
}
critical_section_exit();        // lets pending interrupt handlers run (adding to ISR queue again!)
osKernelResume();               // off we go - finally empties the ISR queue

c1728p9 · 2018-04-03T22:01:34Z

It may be safest to stick with WFI for now, as it doesn't require changing all the existing ports. I have a patch which updates the idle loop as @kjbracey-arm suggested in #6534.

cmonr · 2018-04-16T16:01:46Z

@bulislaw Are there any updates on your side with this PR?

bulislaw · 2018-04-18T11:15:31Z

Yes, once i find some time ;)

bulislaw · 2018-04-24T10:03:50Z

Updates:

Rebased on master
WFE removed and the idle loop updated as suggested by @kjbracey-arm

@c1728p9 @0xc0170 @kjbracey-arm please review

kjbracey · 2018-04-24T10:45:04Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

    core_util_critical_section_exit();
+
+    // Ensure interrupts get a chance to fire
+    __ISB();


I think this ISB is overkill - we have SVC calls to osKernelResume and osKernelSuspend before our next disable, and SVC calls will allow interrupts to trigger according to the ARM ARM. (But then again, is it defined behaviour that these are SVC calls?)

c1728p9 · 2018-04-24T14:04:02Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

    core_util_critical_section_enter();
-    uint32_t ticks_to_sleep = svcRtxKernelSuspend();
-    if (ticks_to_sleep) {
+    if (!osRtxInfo.kernel.pendSV && ticks_to_sleep) {
        os_timer->schedule_tick(ticks_to_sleep);


SysTimer (os_timer) was serialized to a critical section, but no longer is with this change. You'll either need to update SysTimer to add the additional synchronization or wait for #6534 to get merged.

bulislaw · 2018-04-25T15:41:07Z

Rebased on top of #6534

deepikabhavnani · 2018-04-25T16:36:39Z

rtos/TARGET_CORTEX/rtx5/RTX/Source/TOOLCHAIN_ARM/TARGET_M23/irq_armv8mbl.S

@@ -79,7 +87,7 @@ SVC_Context
                CBZ      R1,SVC_ContextSwitch   ; Branch if running thread is deleted

 SVC_ContextSave
-                IF       __DOMAIN_NS = 1
+                IF       DOMAIN_NS = 1


Flag name is changed by CMSIS, will have to check Mbed OS and change the flag name everywhere.

Sorry for that inconvenience. The double underscore prefix is "reserved" for compiler intrinsic. Hence we removed this prefix to clarify that DOMAIN_NS is a user define and not a compiler one.

There are two "versions" of the IRQ assembly file for Armv8-M: irq_armv8mbl.s and irq_armv8mbl_ns.s. The former one contains the actual implementation. The second one just sets this define and includes the former one. There should be no need to know about this define anywhere else than locally within those two files.

Locally within this file (at the top) we have added pre-preprocessor directives, as Mbed OS does not support multiple assembly files. Those are not updated in this PR.
Also build tools enable this flag when building for NS core.

Nice catch! Will change that.

deepikabhavnani · 2018-04-27T16:04:23Z

@bulislaw - New PR which might have small clash here is in "ready to merge" stage. https://github.com/ARMmbed/mbed-os/pull/6747/files

This PR might need a rebase.

bulislaw · 2018-05-22T16:58:08Z

/morph build

orenc17 · 2018-05-22T17:42:08Z

/morph uvisor-test

mbed-ci · 2018-05-22T17:59:05Z

Build : ABORTED

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

cmonr · 2018-05-22T19:50:41Z

Fyi, we're sorting through some things with the CI. As soon as it's resolved, we'll restart the builds.

cmonr · 2018-05-22T21:22:59Z

/morph build

mbed-ci · 2018-05-22T23:29:38Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-05-23T01:24:46Z

Test : SUCCESS

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

mbed-ci · 2018-05-23T06:56:42Z

Exporter Build : SUCCESS

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

0xc0170

I can't find this question but recall seeing it earlier, why are theres commits on this branch - looking at them on master, at least that import v0.30.0 does not match - the file added here is not there (46f9d46).

@Patater
@bulislaw
RTX5: uVisor: Add OsEventObserver  …
32d04a0
 @Patater
@bulislaw
RTX5: uVisor: Extend thread control block with context  …
86b91be
 @Patater
@bulislaw
RTX5: uVisor: Defer to uVisor for SVCall priority  …
2f7a841
 @Patater
@bulislaw
RTX5: uVisor: Switch threads very carefully  …
73a9579
 @Patater
@bulislaw
CMSIS/RTX: uVisor: Import v0.30.0
cb2b91c

0xc0170 · 2018-05-23T12:06:45Z

This PR should be now ready for integration, please last round of review and get this in for 5.9!

cmonr · 2018-05-23T15:16:32Z

@JonatanAntoni @c1728p9 @kjbracey-arm @orenc17 @deepikabhavnani Final re-review?

cmonr · 2018-05-23T22:14:57Z

@bulislaw Mind answering @0xc0170's question?

bulislaw · 2018-05-23T22:35:47Z

I'm guessing rest of the files were already in the sources so rebase picked up only the file that had to be added.

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by ARMmbed#6273.

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by #6273.

0xc0170 added the needs: review label Mar 5, 2018

0xc0170 requested a review from deepikabhavnani March 5, 2018 16:04

bulislaw commented Mar 5, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Mar 13, 2018

0xc0170 reviewed Mar 19, 2018

View reviewed changes

bulislaw force-pushed the update_cmsis_5.3 branch from eeef8bd to 86379df Compare March 19, 2018 14:30

kjbracey suggested changes Mar 21, 2018

View reviewed changes

This was referenced Apr 5, 2018

Update idle loop to reduce calls to suspend #6534

Merged

Request to incorporate the revision of RTX5 for Cortex-A #6617

Closed

bulislaw force-pushed the update_cmsis_5.3 branch from 86379df to e312ce7 Compare April 24, 2018 09:56

bulislaw changed the title ~~[WIP] Update cmsis/rtx to version 5.3~~ Update cmsis/rtx to version 5.3 Apr 24, 2018

kjbracey previously approved these changes Apr 24, 2018

View reviewed changes

c1728p9 reviewed Apr 24, 2018

View reviewed changes

bulislaw dismissed kjbracey’s stale review via 98e5ab9 April 25, 2018 15:40

bulislaw force-pushed the update_cmsis_5.3 branch from e312ce7 to 98e5ab9 Compare April 25, 2018 15:40

deepikabhavnani reviewed Apr 25, 2018

View reviewed changes

bulislaw force-pushed the update_cmsis_5.3 branch 2 times, most recently from b1c9e4f to 4315e11 Compare April 26, 2018 14:35

0xc0170 approved these changes May 23, 2018

View reviewed changes

0xc0170 added needs: review and removed needs: preceding PR needs: work labels May 23, 2018

deepikabhavnani approved these changes May 23, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels May 24, 2018

0xc0170 merged commit d8cb72a into ARMmbed:master May 24, 2018

0xc0170 removed the ready for merge label May 24, 2018

TomoYamanaka added a commit to TomoYamanaka/mbed-os that referenced this pull request May 28, 2018

Fix redeclaration of type name "bool_t"

f348577

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by ARMmbed#6273.

This was referenced May 28, 2018

Fix redeclaration of type name "bool_t" for Renesas #7034

Merged

Fail only ARMCC of Cortex-A on Mbed OS that incorporated RTX5.3 #7065

Closed

adbridge pushed a commit that referenced this pull request Jun 4, 2018

Fix redeclaration of type name "bool_t"

50046d2

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by #6273.

adbridge pushed a commit that referenced this pull request Jun 4, 2018

Fix redeclaration of type name "bool_t"

5d5559d

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by #6273.

toyowata mentioned this pull request Jun 6, 2018

Fix greentea test failure on Renesas Cortex-A9 targets #7139

Merged

adbridge pushed a commit that referenced this pull request Jun 15, 2018

Fix redeclaration of type name "bool_t"

875c0eb

I fixed redeclaration of type name "bool_t" for target Renesas because this typedef has been defined in rtx_core_ca.h by #6273.

Update cmsis/rtx to version 5.3 #6273

Update cmsis/rtx to version 5.3 #6273

Uh oh!

Conversation

bulislaw commented Mar 5, 2018

Description

Pull request type

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!

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

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!

bulislaw commented Mar 15, 2018

Uh oh!

bulislaw commented Mar 19, 2018

Uh oh!

0xc0170 left a comment

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!

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

Uh oh!

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Mar 21, 2018

Uh oh!

kjbracey commented Mar 22, 2018

Uh oh!

c1728p9 commented Apr 3, 2018

Uh oh!

cmonr commented Apr 16, 2018

Uh oh!

bulislaw commented Apr 18, 2018

Uh oh!

bulislaw commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonatanAntoni Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

deepikabhavnani commented Apr 27, 2018

Uh oh!

bulislaw commented May 22, 2018

Uh oh!

orenc17 commented May 22, 2018

Uh oh!

mbed-ci commented May 22, 2018

Uh oh!

cmonr commented May 22, 2018

Uh oh!

cmonr commented May 22, 2018

JonatanAntoni Mar 6, 2018 •

edited

Loading

kjbracey commented Mar 21, 2018 •

edited

Loading

JonatanAntoni Apr 26, 2018 •

edited

Loading

0xc0170 left a comment •

edited

Loading