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

armv7a kernel build bugfix, when shm & arm_addrenv_create_region #15846

Merged
merged 2 commits into from
Feb 15, 2025

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Feb 15, 2025

Summary

bugfix when test features in kernel build

Impact

fix when change CONFIG_ARCH_TEXT_NPAGES to 512 will cause crash in arm_addrenv_create_region with armv7a,
fix shm crash when use fb_demo in goldfish armv7a kernel build

Testing

ci-test, qemu-armv7a knsh & ostest, goldfish-armv7a fb_demo

When the l1entry first add to shm, mssing l1entry add.
shm mmu flags should use udata flags.
l1entry record in shm[] and should not use ~SECTION_MASK

Signed-off-by: buxiasen <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Feb 15, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 15, 2025

[Experimental Bot, please feedback here]

The provided PR description is missing crucial information required by the NuttX guidelines. While it addresses some points, it lacks the necessary detail and depth for proper review.

Here's a breakdown of what's missing and how it can be improved:

Summary:

  • Insufficient detail: "bugfix when test features in kernel build" is vague. What test features? What specific bug is being fixed? Explain the original issue, its symptoms, and the root cause. For example: "Fixes a kernel panic occurring on ARMv7-A when CONFIG_ARCH_TEXT_NPAGES is set to 512. The panic originates in arm_addrenv_create_region due to an incorrect memory allocation calculation." Mention the related NuttX issue number if one exists.
  • Missing functional change description: How exactly does the fix work? What code was modified, and what is the logic behind the change?

Impact:

While the impact section touches on some areas, it's still too brief.

  • User Impact: While stating "NO" might be correct, you should still provide a brief justification. For instance: "No user-facing changes. The fix addresses an internal kernel issue."
  • Build Impact: If the only build impact is that the code now compiles and runs correctly with CONFIG_ARCH_TEXT_NPAGES set to 512, state that explicitly. For example: "No build process changes are required. The fix corrects code that previously failed to build/run correctly under specific configurations."
  • Hardware Impact: Be specific about affected architectures/boards. "ARMv7-A, specifically tested on QEMU and Goldfish emulator" is much better than just "ARMv7-A".
  • Documentation: If no documentation updates are needed, state it clearly: "No documentation updates required."
  • Security, Compatibility: Even if these are "NO", explicitly state them and provide a brief reason. Example: "No security implications. The fix addresses a stability issue." / "No compatibility impact. The fix corrects a bug in existing functionality."

Testing:

  • Build Hosts: Be explicit about the host OS, compiler version, etc. Example: "Linux (Ubuntu 20.04), GCC 9.4.0"
  • Targets: Provide complete details: "QEMU-ARMv7-A (virt board)", "Goldfish ARMv7-A Emulator".
  • Logs: The section mentions "ci-test, qemu-armv7a knsh & ostest, goldfish-armv7a fb_demo". This isn't sufficient. Provide actual log snippets demonstrating the issue before the change and the corrected behavior after the change. Show the error messages/panics before, and the successful execution after. Ideally, the logs should be concise and focused on the relevant parts.

Example of an improved summary:

This PR fixes a kernel panic occurring on ARMv7-A platforms when `CONFIG_ARCH_TEXT_NPAGES` is set to 512. The panic originates in `arm_addrenv_create_region` due to an off-by-one error in a memory allocation calculation, leading to an invalid memory access. This issue also manifests as a crash in the `shm` driver when using the `fb_demo` application on the Goldfish emulator.  Fixes #[NuttX Issue Number (if applicable)].
The fix corrects the calculation in `arm_addrenv_create_region` to accurately determine the required memory size, preventing the invalid memory access and subsequent kernel panic.

By providing more complete information, reviewers can understand the changes, evaluate their impact, and verify their correctness more efficiently, leading to a faster and smoother review process.

@acassis acassis merged commit af0aa78 into apache:master Feb 15, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-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