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_addrenv: Add support for 4 level MMU translations #13560

Merged
merged 2 commits into from
Sep 21, 2024

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented Sep 20, 2024

Summary

The original code made the incorrect assumption that the amount of
translation levels is 3, but this is incorrect. The amount of levels is 4
and the amount of levels that are utilized / in use is set dynamically
from the amount of VA bits in use.

Impact

Support for 4 level translations in ARM64 address environments

Testing

QEMU + iMX93 with kernel mode

The VMSAv8-64 translation system has 4 page table levels in total, ranging
from 0-3. The address environment code assumes only 3 levels, from 1-3 but
this is wrong; the amount of levels _utilized_ depends on the configured
VA size CONFIG_ARM64_VA_BITS. With <= 39 bits 3 levels is enough, while
if the va range is larger, the 4th translation table level is taken into
use dynamically by shifting the base translation table level.

From arm64_mmu.c, where va_bits is the amount of va bits used in address
translations:
(va_bits <= 21)       - base level 3
(22 <= va_bits <= 30) - base level 2
(31 <= va_bits <= 39) - base level 1
(40 <= va_bits <= 48) - base level 0

The base level is what is configured as the page directory root. This also
affects the performance of address translations i.e. if the VA range is
smaller, address translations are also faster as the page table walk is
shorter.
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Sep 20, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 20, 2024

[Experimental Bot, please feedback here]

Squash The Commits: This PR contains 2 Commits. Please Squash the Multiple Commits into a Single Commit.

While the provided information gives a general idea about the PR, it lacks details to assess if it fully meets the NuttX requirements. Here's a breakdown:

Strengths:

  • Summary: Clearly states the problem (incorrect assumption about translation levels) and the solution (dynamically setting levels based on VA bits).
  • Impact: Clearly mentions the added support for 4-level translations in ARM64.
  • Testing: Specifies the testing environment (QEMU + iMX93 with kernel mode).

Areas for Improvement:

  • Summary:
    • Functional part: Needs more specifics. Which part of the codebase handles address translation? (e.g., MMU driver, virtual memory subsystem, etc.)
    • NuttX Issue/PR references: Are there any related issues or PRs this change addresses? If so, include their links.
  • Impact:
    • User impact: Will users need to make any configuration changes or be aware of this modification?
    • Build impact: Does this change affect the build process in any way? New dependencies, configuration options, etc.?
    • Hardware impact: Specifies ARM64, but are there specific architectures or boards affected?
    • Documentation impact: Does this change necessitate documentation updates?
    • Security, compatibility: While you might not anticipate impacts in these areas, it's still a good practice to explicitly state "NO" with a brief explanation (e.g., "NO - This is a bug fix within the internal address translation logic and has no direct impact on security or compatibility").
  • Testing:
    • Build Host(s): Provide more specifics about your build host environment. OS version, compiler version are crucial for reproducibility.
    • Testing logs: The logs are placeholder comments. You need to include the actual logs from your testing both before and after the change. This is crucial for reviewers to understand the issue and verify the fix.

Recommendation:

Provide the missing details to make the PR review smoother and ensure it aligns with the NuttX requirements.

@acassis
Copy link
Contributor

acassis commented Sep 20, 2024

Hey bot @nuttxpr (and @lupyuen @pussuw ) the suggestion to squash the commits in this case doesn't make sense, having separated commits for different logic changes is better to understand the modifications. Squash only should be used when it is some fix for something missing in the previous commit or some small modification in the previous previous commits

The original code made the incorrect assumption that the amount of
translation levels is 3, but this is incorrect. The amount of levels is 4
and the amount of levels that are utilized / in use is set dynamically
from the amount of VA bits in use.
@github-actions github-actions bot added the Arch: arm64 Issues related to ARM64 (64-bit) architecture label Sep 21, 2024
@acassis acassis merged commit 6e15994 into apache:master Sep 21, 2024
29 checks passed
@pussuw pussuw deleted the arm64_4level_mmu_addrenv branch September 21, 2024 11:50
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: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants