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

plat/kvm: Fix guest hang on UKPLAT_HALT during shutdown request #1039

Closed

Conversation

mogasergiu
Copy link
Member

@mogasergiu mogasergiu commented Aug 11, 2023

As soon as
commit 170a8a4 ("plat/kvm/shutdown.c: If on a UEFI system, rely on Runtime Services") got merged guests would not finish on successful runs by exiting, but instead by halting.

Although confusing, the intention was to use UKPLAT_HALT for a graceful shutdown instead of a cpu_halt as it acts now.

Therefore, fix this by deleting the UKPLAT_HALT case from the switch statement.

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): [e.g. x86_64 or N/A]
  • Platform(s): [e.g. kvm, xen or N/A]
  • Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

As soon as
commit 170a8a4 ("plat/kvm/shutdown.c: If on a `UEFI` system, rely on Runtime Services")
got merged guests would not finish on successful runs by exiting, but
instead by halting.

Although confusing, the intention was to use `UKPLAT_HALT` for a
graceful shutdown instead of a `cpu_halt` as it acts now.

Therefore, fix this by deleting the `UKPLAT_HALT` `case` from the
`switch` statement.

Signed-off-by: Sergiu Moga <sergiu@unikraft.io>
@mogasergiu mogasergiu requested a review from a team as a code owner August 11, 2023 17:55
@razvand razvand requested a review from skuenzer August 11, 2023 17:56
@razvand razvand self-assigned this Aug 11, 2023
@razvand razvand removed the request for review from a team August 11, 2023 17:56
@razvand razvand added the area/plat Unikraft Patform label Aug 11, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Aug 11, 2023
@unikraft-bot
Copy link
Member

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA commit checkpatch
a8943d4 plat/kvm: Fix guest hang on `UKPLAT_HALT` during shutdown request ⚠️

Truncated logs starting from first warning a8943d4:

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#8: 
commit 170a8a4fb242 ("plat/kvm/shutdown.c: If on a `UEFI` system, rely on Runtime Services")

total: 0 errors, 1 warnings, 10 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0001-plat-kvm-Fix-guest-hang-on-UKPLAT_HALT-during-shutdo.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

@unikraft-bot unikraft-bot added lang/c Issues or PRs to do with C/C++ plat/kvm Unikraft for KVM labels Aug 11, 2023
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

This did the trick for me, thanks!

Reviewed-by: Alexander Jung alex@unikraft.io

@skuenzer skuenzer self-assigned this Aug 11, 2023
Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Reviewed-by: Simon Kuenzer simon@unikraft.io
Approved-by: Simon Kuenzer simon@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants