nuttxpr commented on PR #14957: URL: https://github.com/apache/nuttx/pull/14957#issuecomment-2500690411
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details required for proper review. Here's a breakdown of what's missing and how to improve it: **Summary:** * **Missing "Why":** The summary lists *what* changed, but not *why*. Explain the reason for each change. For example, why was `set_cr3()` broken? Why was syscall support needed? Why did definitions need alignment? Provide context. * **Missing "How":** While the "what" is present, the "how" is generally lacking. How does the kernel stack support work? How is the TLB shootdown handled? Be more specific. * **Missing Issue References:** Are there any related NuttX issues or NuttX Apps issues/PRs that this addresses? If so, include the links. **Impact:** * **Too vague:** Saying "kernel build supported for x86_64" is not enough. Be specific about the impact of *each* change listed in the summary. * **Address all points:** The provided impact section only touches on the "new feature added" aspect. Explicitly address *all* the impact points (user impact, build impact, hardware impact, documentation, security, compatibility, and anything else). Use "NO" if there's no impact, but *explain why* there's no impact. For example: "Impact on user: NO. These changes are primarily internal kernel improvements and do not affect the user interface or APIs." **Testing:** * **Insufficient Detail:** "ostest on qemu and bare metal Intel HW" is far too brief. Provide specific details about your testing environment: * **Build Host:** e.g., "Linux Ubuntu 20.04, Intel i7-10700K, GCC 11.2.0" * **Target:** e.g., "qemu-system-x86_64 -M q35, NuttX configuration: `boards/qemu-intel64/configs/ostest`" and "Bare metal: Intel NUC11, Core i7-1165G7" * **Missing Logs:** The template specifically requests "Testing logs before change" and "Testing logs after change." Include relevant log snippets demonstrating the issue before the change and the successful operation after the change. Focus on the specific areas affected by your modifications. If the logs are excessively long, consider using a service like Pastebin and linking to it. **Example of an improved description (for just *one* of the changes):** ## Summary * arch/x86_64/: fix broken set_cr3() - The previous implementation of `set_cr3()` incorrectly handled address masking, leading to page faults when switching page tables. This change corrects the masking operation to ensure proper address translation. Fixes #1234 (NuttX issue link). ## Impact * Is new feature added? NO * Is existing feature changed? YES. The `set_cr3()` function is corrected. * Impact on user: NO. This is an internal kernel fix and should not have a user-visible impact. Existing applications using virtual memory should experience improved stability. * Impact on build: NO. No build process changes are required. * Impact on hardware: YES. This fix applies specifically to x86_64 architectures. * Impact on documentation: NO. No documentation changes are required as this is a bug fix to existing functionality. * Impact on security: YES. The incorrect address masking could potentially be exploited to gain unauthorized memory access. This fix mitigates this vulnerability. * Impact on compatibility: NO. This fix corrects existing behavior and maintains backward compatibility. * Anything else to consider? None. ## Testing * Build Host(s): Linux Ubuntu 22.04, Intel i9-12900K, GCC 12.1.0 * Target(s): qemu-system-x86_64 -M q35, NuttX configuration: `boards/qemu-intel64/configs/ostest` Testing logs before change: ``` [paste a snippet of the logs where you could see the wrong masking issue] ``` Testing logs after change: ``` [paste a snippet of the logs after you applied your fix where you demonstrate working page switching with correct address translation] ``` Repeat this detailed structure for *each* item listed in the original summary. By providing comprehensive information, you significantly improve the review process and increase the chances of your PR being accepted. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
