Cortex-M23: Do not use PSPLIM_NS

urutva commented 1 year ago

Description

According to Armv8-M technical reference manual, if the main extension is not implemented then PSPLIM_NS is RES0. Update the cortex-M23 port to not use the reserved PSPLIM_NS if the main extension is not enabled.

Test Steps

Checklist:

[ ] I have tested my changes. No regression in existing tests.
[ ] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (a695b67) 93.62% compared to head (9e70f80) 93.62%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #791 +/- ## ======================================= Coverage 93.62% 93.62% ======================================= Files 6 6 Lines 2508 2508 Branches 598 598 ======================================= Hits 2348 2348 Misses 107 107 Partials 53 53 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/791/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/791/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.62% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jefftenney commented 1 year ago

Hi @urutva. I'm wondering if these changes are addressing a specific issue and if they are necessary. The ARM architectural requirement that PSPLIM_NS be RES0 is intended to simplify code that supports multiple ARMv8-M targets (targets with and without the main extension). The code is allowed to write any value to PSPLIM_NS, even if PSPLIM_NS is RES0. I think that means this PR is not strictly necessary. The port code prior to this PR is already compliant with ARM's recommendations.

But maybe this PR isn't about fixing ARM compliance? In that case there may still be value in clarifying the code as you have done. Your changes clearly indicate PSPLIM is not supported on the non-secure side in baseline implementations like CM23. And your changes reduce RAM requirements slightly per task. So there is value here. But maybe keeping the code cleaner, with fewer conditionals, as before this PR, has more value.

urutva commented 1 year ago

@jefftenney Thanks for your feedback. I started looking into this when the issue [BUG] ARM_CM23_nonsecure does not support psplim was reported.

The ARM architectural requirement that PSPLIM_NS be RES0 is intended to simplify code that supports multiple ARMv8-M targets (targets with and without the main extension).

Agreed. However, the Cortex-M23 port not only writes to PSPLIM_NS but relies on the value read b back from PSPLIM_NS to decide if a secure context needs to saved or not https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/ARMv8M/secure/context/secure_context.c#L343. The port shouldn't rely on PSPLIM_NS as it is RES0 on Cortex-M23.

The description of RES0 from the Armv8-M Architecture Reference Manual is as follows:

A reserved bit or field with Should-Be-Zero-or-Preserved behavior, or equivalent read-only or write-only behavior.
Used for fields in register descriptions, and for fields in architecturally defined data structures that are held in
memory.
Within the architecture, there are some cases where a register bit or field:
• Is RES0 in some defined architectural context.
• Has different defined behavior in a different architectural context.
Note
RES0 is not used in descriptions of instruction encodings.
This means the definition of RES0 for fields in read/write registers is:
If a bit is RES0 in all contexts
For a bit in a read/write register, it is IMPLEMENTATION DEFINED whether:
1. The bit is hardwired to 0. In this case:
• Reads of the bit always return 0.
• Writes to the bit are ignored.
2. The bit can be written. In this case:
• An indirect write to the register sets the bit to 0.
• A read of the bit returns the last value that is successfully written, by either a direct or an indirect write, to
the bit.
If the bit has not been successfully written since reset, then the read of the bit returns the reset value if
there is one, or otherwise returns an UNKNOWN value.
• A direct write to the bit must update a storage location that is associated with the bit.
• The value of the bit must have no effect on the operation of the PE, other than determining the value read
back from the bit, unless this manual explicitly defines additional properties for the bit.
Whether RES0 bits or fields follow behavior 1 or behavior 2 is IMPLEMENTATION DEFINED on a bit-by-bit basis.

With this PR, port uses PSP_NS (which is set during secure context loading) to check if the secure context needs to stored or not.

While fixing this, I realised that not using PSPLIM_NS does reduce RAM consumption when MPU config is enabled (as you described). Also, since PSPLIM_NS is RES0, it doesn't affect the operation of the PE and we are setting PSPLIM_NS during every context switch. With this change, one less instruction to execute during every context switch.

I do agree that, with this PR code complexity is increased, but I think it is justified provided that it does fix the aforementioned issue (not relying on the value in PSPLIM_NS), reduces memory consumption and one less instruction to execute during every context switch.

jefftenney commented 1 year ago

@urutva Thank you for the extra context.

The Cortex-M23 port not only writes to PSPLIM_NS but relies on the value read b back from PSPLIM_NS to decide if a secure context needs to be saved or not https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/ARMv8M/secure/context/secure_context.c#L343. The port shouldn't rely on PSPLIM_NS as it is RES0 on Cortex-M23.

I believe that code relies on PSPLIM_S, not PSPLIM_NS. Functions SecureContext_LoadContext() and SecureContext_SaveContext() run on the secure side where they have access to the secure context of the processor. That means when they access PSPLIM, they actually access PSPLIM_S. CM23 does implement PSPLIM_S, because it plays a critical role in the secure world.

urutva commented 1 year ago

@jefftenney Good catch. Thanks. Indeed, the SecureContext_* APIs are marked with cmse_nonsecure_entry. I've updated the PR to remove changes done to SecureContext_* APIs.

With that, conditional evaluation is limited to two files port.c and portmacrocommon.h. The PR still brings in reduces memory consumption and one less instruction to execute during every context switch as described before. Therefore, I still see a value add in this PR.

jefftenney commented 1 year ago

There is one other concern here.

This PR adds value for TZ applications and for NTZ applications that execute only on the non-secure side of CM23. However, in its current form this PR results in the loss of PSPLIM functionality for NTZ applications that execute only on the secure side. For NTZ applications, PSPLIM functionality may be a motivating factor for the application developer to put the entire application on the secure side. So we should keep support for it.

On principle I like the idea of eliminating unnecessary instructions (especially during context switches), and I like reducing memory usage too. So I think it's worth modifying the PR a little more. That way the reviewers can more easily make the value judgement. To see clearly what this optimization costs.

Let me know if there's something I can do to be helpful.

Dasheverless commented 1 year ago

Hi @jefftenney , using PSPLIM_NS on CM23 may cause hardfualt. See more here.

jefftenney commented 1 year ago

@Dasheverless Thanks for the comment. Seems this PR is more than just an optimization then. It may be required for ARM compliance on CM23 without the security extension, at the very least.

@urutva No real change to where I assume you are headed with this PR. Existing configuration macros appear to be sufficient to include/exclude PSPLIM usage conditionally: portHAS_ARMV8M_MAIN_EXTENSION, configENABLE_TRUSTZONE, and configRUN_FREERTOS_SECURE_ONLY. If portHAS_ARMV8M_MAIN_EXTENSION is 1, that alone is sufficient to include PSPLIM usage. Alternatively, if configENABLE_TRUSTZONE is 0 and configRUN_FREERTOS_SECURE_ONLY is 1, then that is also sufficient to include PSPLIM usage. This summary from the architecture manual is helpful:

aggarg commented 1 year ago

This PR adds conditional compilation in multiple places and we need to add more conditional checks in asm code to use PSPLIM when FreeRTOS is run on secure side only.

Given that we are doing it for a non-compliant implementation, what do you guys think about keeping the context same (i.e. save the PSPLIM as part of context) and just prevent reading and writing PSPLIM? This way we will be able to reduce complexity introduced by conditional checks at the cost of couple of extra bytes of memory.

@jefftenney @urutva

jefftenney commented 1 year ago

@aggarg That's a good balance.

As a side note, I'm not sure the current implementation (pre-PR) is non-compliant. The architecture spec is ambiguous. Why bother requiring PSPLIM_NS to be RES0 if MSR/MRS is not required to read/write it? Reading the operation code of MSR/MRS (which I posted on the issue page), consider a PE with the security extension and no main extension, with the PE in the nonsecure state. The operation code directly conflicts with the register description of PSPLIM_NS with no main extension. The operation code says unpredictable, but the register description says RES0 for that exact case.

In fact I suspect silicon based on actual ARM IP would not require this PR. Probably doesn't matter though. Seems best to move ahead with finishing this PR.

urutva commented 1 year ago

@aggarg @jefftenney Apologies for late reply.

@aggarg Thanks for the proposal, as @jefftenney said, it strikes a good balance.

@jefftenney Thanks for the response on the issue. I'm gathering more information on the inconsistency that you have described. I'll provide an update when I've more information.