Skip to content

Conversation

@szafonimateusz-mi
Copy link
Contributor

Summary

  • arch/x86_64/intel64/intel64_head.S: move initial RSP for AP cores below regs area
    move initial RSP for AP cores below regs area.
    otherwise IDLE thread for AP cores can be corrupted.
    XCP region now match regs allocation in up_initial_state()

  • arch/intel64_cpuidlestack.c: stack_alloc should point to stack base not stack top

  • arch/x86_64/intel64/intel64_schedulesigaction.c: properly align signal handler stack for vector operations

Impact

various fixes for problems observed with high optimization enabled and SMP

Testing

qemu with smp test and ostest

…ow regs area

move initial RSP for AP cores below regs area.
otherwise IDLE thread for AP cores can be corrupted

XCP region now match regs allocation in up_initial_state()

Signed-off-by: p-szafonimateusz <p-szafonimateusz@xiaomi.com>
…to stack base not stack top

stack_alloc should point to stack base not stack top

Signed-off-by: p-szafonimateusz <p-szafonimateusz@xiaomi.com>
@github-actions github-actions bot added Arch: x86_64 Issues related to the x86_64 architecture Size: S The size of the change in this PR is small labels Oct 3, 2024
…l handler stack for vector operations

signal handler stack must be properly aligned, otherwise vector instructions doesn't work in signal handler

Signed-off-by: p-szafonimateusz <p-szafonimateusz@xiaomi.com>
@nuttxpr
Copy link

nuttxpr commented Oct 3, 2024

[Experimental Bot, please feedback here]

The PR summary is missing some key information, and the testing section is insufficient. Here's a breakdown:

What's Missing in the Summary:

  • Why is the change necessary? The summary mentions "problems observed," but doesn't explain what those problems are. Be specific about the bugs or limitations being addressed.
  • How does the change work? The summary provides a high-level overview, but lacks technical details. Explain the mechanism behind the fixes (e.g., why moving the RSP is critical, how stack alignment affects signal handlers).

What's Missing in Testing:

  • No "before" context: The logs should demonstrate the problem before the PR's changes. Without this, it's impossible to assess if the PR actually fixes anything.
  • Lack of detail: "qemu with smp test and ostest" is too vague. Specify:
    • The exact QEMU version
    • The specific SMP test cases used
    • Which ostest tests were relevant
  • No evidence of success: The "after" logs are missing entirely. Show that the tests now pass or that the problematic behavior is gone.

To improve the PR:

  1. Expand the summary: Clearly articulate the "why" and "how" behind each change. Include links to any relevant bug reports or discussions.
  2. Provide complete testing logs:
    • Before: Show the errors or unexpected behavior.
    • After: Demonstrate the fixes in action.
  3. Be specific about your setup: Include versions and configurations for QEMU, the compiler, and any other relevant tools.

By addressing these points, you'll make it much easier for reviewers to understand and verify your changes.

@xiaoxiang781216 xiaoxiang781216 merged commit ce61347 into apache:master Oct 3, 2024
@szafonimateusz-mi szafonimateusz-mi deleted the fixes branch April 29, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: x86_64 Issues related to the x86_64 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.

3 participants