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

arm64: Allows thread to independent control the switch of sctlr #15437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Jan 6, 2025

Note: Please adhere to Contributing Guidelines.

Summary


    The method is the same as the method of saving the current DAIF state of the thread
    It will pave the way for the future implementation of hwasan's memory management
    Allows each thread to independently control the mte switch function

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@github-actions github-actions bot added the Arch: arm64 Issues related to ARM64 (64-bit) architecture label Jan 6, 2025
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Jan 6, 2025
@W-M-R W-M-R changed the title arm64/mte: Allows thread to automatically control the switch of mte arm64: Allows thread to automatically control the switch of sctlr Jan 6, 2025
@W-M-R W-M-R marked this pull request as draft January 6, 2025 12:22
@W-M-R W-M-R changed the title arm64: Allows thread to automatically control the switch of sctlr arm64: Allows thread to independent control the switch of sctlr Jan 6, 2025
The method is the same as the method of saving the current DAIF state of the thread
It will pave the way for the future implementation of hwasan's memory management
Allows each thread to independently control the mte switch function

Signed-off-by: wangmingrong1 <[email protected]>
@W-M-R W-M-R marked this pull request as ready for review January 6, 2025 12:34
@nuttxpr
Copy link

nuttxpr commented Jan 6, 2025

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements. While it provides a summary of what the change does, it lacks crucial information.

Here's a breakdown of what's missing:

Summary:

  • Missing "Why": The summary explains the how but not the why. Why is per-thread MTE control necessary? What problem does it solve? Is there a bug being fixed? Is this a performance optimization?
  • Missing Scope: What specific files/modules are affected? Saying "the method is the same as..." isn't descriptive enough. Name the specific functions or files modified.
  • Missing Issue References: Are there any related NuttX or NuttX-apps issues that prompted this change? If so, link them.

Impact:

  • Completely Empty: This section is critical. The maintainer needs to understand the ramifications of merging this change. Address all the points (user impact, build impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO", explicitly state it. For example: "Impact on user: NO"
  • Security implications are especially important here given that this relates to MTE (Memory Tagging Extension), a security feature. What are the potential security benefits or drawbacks of this change?

Testing:

  • Completely Empty: This section is also crucial. Without evidence of testing, the PR is unlikely to be merged. Provide specific details:
    • Build Host: e.g., "Linux (Ubuntu 22.04), x86_64, GCC 11.2.0"
    • Target: e.g., "qemu-rv64:nsh"
    • Logs: Include actual logs showing the behavior before and after the change. Demonstrate that the change works as intended and doesn't introduce regressions. If the logs are too long, consider providing a link to a gist or pastebin.

In short, the PR needs significant improvements before it can be considered for merging. It needs to be more thorough and provide the necessary information for reviewers to evaluate the change effectively. Follow the template more closely and fill in all the required sections.

@W-M-R
Copy link
Contributor Author

W-M-R commented Jan 6, 2025

This approach causes the code size of "nuttx/arch/arm64/src/common/arm64_vector_table.S: arm64_enter_exception“ to be larger than the 128 bytes required by the vector table. Can this problem be solved by changing the macro to a function?
image
I passed the test by turning off "CONFIG_ARCH_FPU"

@@ -80,6 +80,9 @@
#endif
stp \xreg0, \xreg1, [sp, #8 * REG_ELR]

mrs \xreg0, sctlr_el1
str \xreg0, [sp, #8 * REG_SCTLR_EL1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
str \xreg0, [sp, #8 * REG_SCTLR_EL1]
str \xreg0, [sp, #8 * REG_SCTLR_EL1]

@xiaoxiang781216
Copy link
Contributor

This approach causes the code size of "nuttx/arch/arm64/src/common/arm64_vector_table.S: arm64_enter_exception“ to be larger than the 128 bytes required by the vector table. Can this problem be solved by changing the macro to a function? ! I passed the test by turning off "CONFIG_ARCH_FPU"

let's try to reduce the code size first.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 7, 2025

This approach causes the code size of "nuttx/arch/arm64/src/common/arm64_vector_table.S: arm64_enter_exception“ to be larger than the 128 bytes required by the vector table. Can this problem be solved by changing the macro to a function? ! I passed the test by turning off "CONFIG_ARCH_FPU"

let's try to reduce the code size first.

@W-M-R some optimization can be done:

  1. Skip save and restore x0 since it's always zero
  2. Remove the duplication loading tpidrro_el0 at line 92
  3. Remove line 93 by using add with immediate value:
    https://developer.arm.com/documentation/ddi0596/2021-03/Base-Instructions/ADDS--immediate---Add--immediate---setting-flags-#:~:text=Add%20(immediate)%2C%20setting%20flags,the%20alias%20CMN%20(immediate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants