portIS_PRIVILEGED / xIsPrivileged / xPortRaisePrivilege Cortex-M33

Partners & Sponsors ARM
1

Edit: Frustratingly I am not permitted to use links, so you’ll have to infer my sources.

Closely related to: forums[.]freertos[.]org/t/get-task-handle-from-isr-with-mpu/8060

I have also noticed a hard fault when calling non-fromISR suffix functions (xTaskGetCurrentTaskHandle in this case) from ISRs. I know I know, naughty.

I understand that not calling xTaskGetCurrentTaskHandle neatly avoids the problem, however to my mind the internal/core behaviour of xIsPrivileged/portIS_PRIVILEGED is incorrect.

The FreeRTOS port for the M33 checks the state of bit #0 (nPRIV) of the CONTROL register to judge whether the core is currently in a privileged or unprivileged state.
However the nPRIV bit is only valid when the core is also in Thread mode.
developer[.]arm[.]com/documentation/100235/0004/the-cortex-m33-processor/programmer-s-model/core-registers
(As far as I can tell, when the core exits Thread mode (ie enters Handler mode) it leaves this set to whatever it happens to be set to, because it shouldn’t be used in Handler mode anyway.)

It is clear from: developer[.]arm[.]com/documentation/100235/0004/the-cortex-m33-processor/programmer-s-model/processor-modes-and-privilege-levels-for-software-execution that “In Handler mode, software execution is always privileged.”

So, regardless of where/when/how xIsPrivileged is used, it only accurately returns the current core Privilege level if it takes into account Thread/Handler mode as well as the nPRIV bit.

Seems like it would be better if a given function “did what it said on the tin” at all times rather than mitigate by not using it in certain situations.

I think from reading the ARM documentation that CONTROL[1] (ie CONTROL.SPSEL) can be used as a valid indicator of Handler/Thread mode. Somehow they’ve managed to leave it ambiguous in the documentation though.
Sorry, side rant, but: Why on earth does the ARM documentation give almost enough information to be clear but frequently stop short of being explicit. I don’t like having to infer core behaviour. “In Handler mode, this bit reads as zero” and in Thread mode?.. One assumes it’s always 1 in Thread mode, but we’re left with the possibility that it can be either 0 or 1 in Thread mode depending on reasons. I presume that if we’re using the MSP we’re always in Handler mode and PSP always Thread mode?

I would propose that the BaseType_t xIsPrivileged(void) function be altered to something like:

__asm volatile
(
  "	.syntax unified  \n"/* When CONTROL[1]==1 CONTROL[0]==1 mode is unprivileged, */
  "					 \n"/*  otherwise we're either in Handler mode or Thread mode + Privileged.  */
  "	mrs r0, control	 \n"/* r0 = CONTROL. */
  "	teq r0, #3		 \n"/* Perform bitwise r0 XOR 0b11 and update the conditions flag. */
  "	ite ne			 \n"/* Result was zero Z==1 for unprivileged, otherwise Z==0 */
  "	movne r0, #1	 \n"/* Z==0: CONTROL[1] + CONTROL[0] != 0b11. Return true to indicate that the processor is privileged. */
  "	moveq r0, #0	 \n"/* Z==1: CONTROL[1] + CONTROL[0] == 0b11. Return false to indicate that the processor is unprivileged. */
  "	bx lr			 \n"/* Return. */
  "					 \n"
  "	.align 4		 \n"
  ::: "r0", "memory"
);
2

I think the key point is that not only is calling non-FromISR functions when in an ISR “naughty”, it is specifically defined something not to do and under suggested development conditions (with configASSERT defined) will trigger those asserts in most cases.

Therefore, xIsPrivileged is DEFINED to only be called from a task, and thus it is correct to assume that. Adding code to check if you are in an ISR is just wasted cycles.

Creating your own xIsPrivilegedPossibleFromISR function as you have defined it would be ok.

But, a key point is that ISRs should be short, and the code should know that it is an ISR, so such a function should not be needed. This is a basic FreeRTOS philosophy. Yes, some other RTOS let you hide the distinction and make the API calls just act differently between ISR and non-ISR calls, but in my opinion this just tends to encourage “bad” code (and often introduces bugs).

3

Hi Richard,
Thanks for your reply and input.
I (think I) understand your position, and it is completely valid.

My point is perhaps a little pedantic; for me, the M33 port of xIsPrivileged should/could technically be called xIsPrivilegedThreadMode according to the ARM documentation. This is completely acceptable because, as you have pointed out, it should only be called when in Thread mode anyway.
However, for the same number of assembly instructions (so not “Adding code to check if you are in an ISR is just wasted cycles.”) it can truly be xIsPrivileged.
If it’s free, wouldn’t it be better for xIsPrivileged to work in all circumstances? A moot point perhaps.

The only potential cost impact I can imagine is in

tst r0, #1

vs

teq r0, #3

…the #3 literal may incur an additional load vs the #1?

Regarding ISRs; I agree in general but exception handlers (as is the case I happen to have in front of me) are a little different. Often an exception handler is terminal, so I’m not at all interested in the stable and efficient operation of my application; my application is in pieces already.

4

First, the “Arm” naming convention isn’t applicable, as this isn’t a CMSIS library, and that function is named to be used on ANY processor that supports a similar protected mode, so referring to “ThreadMode” isn’t reasonable, as other processors may call it something else.

Second, inside an exception handler you ESPECIALLY need to watch out about using FreeRTOS task level functionality, as they may re-enable the interrupts that were disabled by the exception.

You may want to think about injectioning your exception handlers into task.c via the freertos_task_c_additions.h hook so your handlers can directly access the needed information and structure definitions.

5

I completely agree; to be clear I’m not suggesting that anything in FreeRTOS should be called …ThreadMode. I’m saying that, as it is currently implemented, xIsPrivileged is not (and I’m being pedantic here) actually asking a Cortex-M33: “Are you in a privileged execution mode”. In order to answer that question unambiguously, you must read more than 1 bit of the CONTROL register on an M33 core. By only reading that one (nPRIV) bit, it is instead asking specifically: “Are you in privileged execution mode if you are also in Thread mode”. Hence my fictitious and purely hypothetical renaming of that function (to try and illustrate my point).
My initial proposal was to change the implementation to unambiguously/accurately answer the question with a Cortex-M33, rather than “change the question”/rename the function.
Apologies if that wasn’t clear.

I do understand that in practice it is unnecessary to change anything, because one should not be calling from a non-thread context anyway. I think we’re debating semantics at this point.

Second, inside an exception handler you ESPECIALLY need to watch out about using FreeRTOS task level functionality, as they may re-enable the interrupts that were disabled by the exception.

I did not know this, thank you for raising this possibility.

You may want to think about injectioning your exception handlers into task.c via the freertos_task_c_additions.h hook so your handlers can directly access the needed information and structure definitions.

This is also a very valuable suggestion, thanks for this.

6

If it really is ZERO cost, and also works on all the other applicable Cortex-M processors, it might make sense to do. Mentioning the other processors as I believe there is a goal to keep the code between the Cortex-M ports as similar as possible to reduce support/debugging cost.