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.

Sign up for GitHub

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

Jump to bottom

plat/kvm: Align the bootstack base address #1047

Closed
wants to merge 1 commit into from
Closed

plat/kvm: Align the bootstack base address #1047

wants to merge 1 commit into from

Conversation

michpappas
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): [arm64,x86_64]
  • Platform(s): [kvm]
  • Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

The base address of the stack must be 16-byte aligned on both arm64 and x86_64. Fix the alignent where this is currently missing.

@michpappas michpappas requested a review from a team as a code owner August 14, 2023 08:54
@razvand razvand requested review from mogasergiu and removed request for a team August 14, 2023 08:55
@razvand razvand self-assigned this Aug 14, 2023
@razvand razvand added area/plat Unikraft Patform plat/kvm Unikraft for KVM lang/c Issues or PRs to do with C/C++ arch/x86_64 arch/arm arch/arm64 labels Aug 14, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Aug 14, 2023
mogasergiu
mogasergiu requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@mogasergiu mogasergiu left a comment

Choose a reason for hiding this comment

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

Thank you for making these changes 🙏! One minor comment.

plat/kvm/arm/entry64.S Outdated
@@ -40,6 +40,7 @@
* paging_init (it invalidates our stack)
*/
.section .bss
.align 16
Copy link
Member

Choose a reason for hiding this comment

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

I see that, unlike x86, we do not have an explicit aligning of the stack like the andq $~0xf, %rsp. Would it be worth adding this to ARM as well, just to double make sure.

Copy link
Member Author

@michpappas michpappas Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Based on offline discussion, updated PR to:

  • Remove changes in x86_64, the alignment is enforced there on runtime already
  • Update arm64 to enforce alignment on runtime too

mogasergiu
mogasergiu approved these changes Aug 14, 2023
View reviewed changes
Copy link
Member

@mogasergiu mogasergiu left a comment

Choose a reason for hiding this comment

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

I do not like how this is inconsistent with x86, as there is no __STACK_ALIGN_SIZE, but, alas, this is getting out of scope and will maybe be addressed by plat re-arch, hopefully.

Reviewed-by: Sergiu Moga sergiu@unikraft.io

@michpappas
Copy link
Member Author

I do not like how this is inconsistent with x86, as there is no __STACK_ALIGN_SIZE, but, alas, this is getting out of scope and will maybe be addressed by plat re-arch, hopefully.

Reviewed-by: Sergiu Moga sergiu@unikraft.io

Agreed. In fact I didn't like the __STACK_ALIGN_SIZE naming either, but avoided to change it as it's part of a wider series of #definesin that header and would need to change it in other places too. Thanks!

@michpappas
plat/kvm/arm: Fix bootstack base alignment on arm64
be5b0b5 
280c695 introduces a regression to the alignment of the
bootstack. Update __libkvmplat_entry to enforce the correct
alignment before assigning to the sp.

Signed-off-by: Michalis Pappas <michalis@unikraft.io>
@unikraft-bot
Copy link
Member

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
be5b0b5 plat/kvm/arm: Fix bootstack base alignment on arm64 ✅

michpappas added a commit to michpappas/unikraft that referenced this pull request Aug 14, 2023
@michpappas
PR unikraft#1047: Fix bootstack base alignment on arm64
85c271b 
Signed-off-by: Michalis Pappas <michalis@unikraft.io>
razvand
razvand approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@razvand razvand razvand approved these changes

@mogasergiu mogasergiu mogasergiu approved these changes

Assignees

@razvand razvand

Labels
arch/arm arch/arm64 arch/x86_64 area/plat Unikraft Patform ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ plat/kvm Unikraft for KVM
Projects
Unikraft Core Roadmap
Status: ✅ Done!
Milestone
v0.14.0 (Prometheus)
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@michpappas @unikraft-bot @razvand @mogasergiu