Skip to content
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

Armv8.1-m: Add pacbti support #1147

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

AhmedIsmail02
Copy link
Contributor

@AhmedIsmail02 AhmedIsmail02 commented Sep 13, 2024

Description

In this Pull Request, Pointer Authentication, and Branch Target Identification Extension (PACBTI) support is added for Non-TrustZone variant of Cortex-M85 FreeRTOS-Kernel Port.

The PACBTI support is added for Arm Compiler For Embedded, and IAR toolchains only. The support in the kernel is not yet enabled for GNU toolchain due to known issues.

Blocks FreeRTOS-Partner-Supported-Demos PR!17

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.

@AhmedIsmail02 AhmedIsmail02 requested a review from a team as a code owner September 13, 2024 14:53
@AhmedIsmail02 AhmedIsmail02 force-pushed the armv8-1-m-add-pacbti-support branch from 3a52b68 to ef74d93 Compare September 13, 2024 15:41
@AhmedIsmail02 AhmedIsmail02 force-pushed the armv8-1-m-add-pacbti-support branch 2 times, most recently from a61bc5f to 2fab8cb Compare September 27, 2024 14:26
@aggarg
Copy link
Member

aggarg commented Oct 16, 2024

@AhmedIsmail02 Thank you for your contribution. I reviewed the code and I have the following suggestions -

  1. Use portHAS_PACBTI_FEATURE instead of portPROCESSOR_VARIANT. This way we won't need to change code in multiple places when we add support for next processor variant which has PACBTI feature.
  2. The current implementation seems to assume that the code is always used in CMake environment which is not true. I suggest to add configENABLE_PAC and configENABLE_BTI which can be set in FreeRTOSConfig.h. This is also consistent with the current flags configENABLE_FPU, configENABLE_MPU etc.
  3. The change done in CM33_NTZ ports is also needed in CM33 ports.
  4. Readability improvements.

All my suggestions are in this patch - 0001-Code-review-suggestions.patch. Please apply this patch.

Also, I think we do not need portable/CMakeLists.txt changes if we accept the above suggestions.

@AhmedIsmail02
Copy link
Contributor Author

@aggarg Thank you for your suggestions. I do agree with almost all the code suggestions in the patch. Regarding the changes in portable/CMakeLists.txt which are related to the second suggestion, the reason why we chose to design our implementation based on CMake and not using the config solution is that some build flags (compiler and linker options) should be set based on the selected PACBTI configuration and these options also vary based on the selected compiler. These build options can't be controlled from the C code level and has to be added for the kernel and port targets to allow the compiler to insert the respective instructions based on the PACBTI configuration.
As an improvement we can minimize the number of macros checked for PAC and BTI enablement:

Before:
#if ( ( portARM_V_8_1_M_PACBTI_CONFIG == portARM_V_8_1_M_PACBTI_CONFIG_STANDARD ) || \ ( portARM_V_8_1_M_PACBTI_CONFIG == portARM_V_8_1_M_PACBTI_CONFIG_PACRET_LEAF_BTI ) ) || \ ( portARM_V_8_1_M_PACBTI_CONFIG == portARM_V_8_1_M_PACBTI_CONFIG_PACRET ) || \ ( portARM_V_8_1_M_PACBTI_CONFIG == portARM_V_8_1_M_PACBTI_CONFIG_PACRET_LEAF ) )

After:
#if ( portARM_V_8_1_M_PAC_ENABLED == 1 )

Thanks!

@aggarg
Copy link
Member

aggarg commented Oct 17, 2024

Thank you @AhmedIsmail02 for looking at my suggestions.

These build options can't be controlled from the C code level and has to be added for the kernel and port targets to allow the compiler to insert the respective instructions based on the PACBTI configuration.

Understood. But can I not set the correct options for my compiler using my IDE and then set the FreeRTOS flags in FreeRTOSConfig.h? And with that we should be able to support CMake as well:

if(FREERTOS_ARM_V_8_1_M_PACBTI_CONFIG STREQUAL "ARM_V_8_1_M_PACBTI_CONFIG_PACRET_LEAF_BTI")
    if(${CMAKE_C_COMPILER_ID} STREQUAL "ARMClang")
        target_compile_options(freertos_kernel_port
            PUBLIC
                -mbranch-protection=bti+pac-ret+leaf
        )
        target_compile_definitions(freertos_kernel_port
            PUBLIC
                configENABLE_PAC=1
                configENABLE_BTI=1
        )

This way CMake users can still set FREERTOS_ARM_V_8_1_M_PACBTI_CONFIG to their desired values and do not need to worry about setting configENABLE_PAC and configENABLE_BTI in FreeRTOSConfig.h. Non CMake users, on the other hand, need to set correct compiler options and then set configENABLE_PAC and configENABLE_BTI in FreeRTOSConfig.h.

aggarg added a commit to aggarg/FreeRTOS that referenced this pull request Oct 17, 2024
This allows derived classes to override customCheck method to inject
additional checks for some files. This is currently needed to accept
ARM copyright in ARMv8-M files - FreeRTOS/FreeRTOS-Kernel#1147.

Signed-off-by: Gaurav Aggarwal <[email protected]>
@aggarg
Copy link
Member

aggarg commented Oct 17, 2024

Please apply the following patch to address CI header check failure - header_check.patch.

This will also require that this PR is merged - FreeRTOS/FreeRTOS#1285.

aggarg added a commit to FreeRTOS/FreeRTOS that referenced this pull request Oct 17, 2024
This allows derived classes to override customCheck method to inject
additional checks for some files. This is currently needed to accept
ARM copyright in ARMv8-M files - FreeRTOS/FreeRTOS-Kernel#1147.

Signed-off-by: Gaurav Aggarwal <[email protected]>
@AhmedIsmail02 AhmedIsmail02 force-pushed the armv8-1-m-add-pacbti-support branch from d0ead8b to ef388c1 Compare October 21, 2024 11:34
@AhmedIsmail02
Copy link
Contributor Author

Hi @aggarg,
Thanks for the suggestions, the approach looks good to me as it ensures that both CMake and non-CMake based projects' behavior are aligned. I've applied the suggested changes for both the PACBTI configurations and the CI header check patch.

Thanks!

@AhmedIsmail02 AhmedIsmail02 force-pushed the armv8-1-m-add-pacbti-support branch from 3f24cde to 9e24b0f Compare October 22, 2024 08:36
AhmedIsmail02 and others added 4 commits October 22, 2024 16:52
FreeRTOS Arm collab files shall have both Amazon's
and Arm's copyright headers. Hence, the copyright
checker is modified to check for both copyrights.

Signed-off-by: Gaurav Aggarwal <[email protected]>
As the case for ARMClang, and GCC toolchains, IAR
with TFM FreeRTOS Port support is added.

Signed-off-by: Ahmed Ismail <[email protected]>
The current ARMv8-M FreeRTOS-Kernel Port code
implementation is modified in a way that allows
the CONTROL register's value to be retained
rather than being overwritten.

This is needed for adding PACBTI support as
the special-purpose CONTROL register `PAC_EN`,
`UPAC_EN`, `BTI_EN`, and `UBTI_EN` PACBTI
enablement bits should be configured before calling
`vRestoreContextOfFirstTask()` function which currently
overwrite the value inside the CONTROL register.

Signed-off-by: Ahmed Ismail <[email protected]>
In this commit, Pointer Authentication, and Branch Target
Identification Extension (PACBTI) support is added for
Non-Secure and Non-TrustZone variants of Cortex-M85
FreeRTOS-Kernel Port.

The PACBTI support is added for Arm Compiler For
Embedded, and IAR toolchains only. The support in
the kernel is not yet enabled for GNU toolchain
due to known issues.

Signed-off-by: Ahmed Ismail <[email protected]>
@AhmedIsmail02 AhmedIsmail02 force-pushed the armv8-1-m-add-pacbti-support branch from bdc4b61 to 11fe156 Compare October 23, 2024 08:50
Signed-off-by: Gaurav Aggarwal <[email protected]>
Copy link

sonarcloud bot commented Oct 24, 2024

@aggarg aggarg merged commit 7081e76 into FreeRTOS:main Oct 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants