fix: Always restore MSR_IA32_TSC_DEADLINE after MSR_IA32_TSC #4666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4666 +/- ##
=======================================
Coverage 82.11% 82.12%
=======================================
Files 255 255
Lines 31261 31281 +20
=======================================
+ Hits 25671 25689 +18
- Misses 5590 5592 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
added
the
Status: Awaiting review
kalyazin
reviewed
Jul 5, 2024
pb8o
reviewed
Jul 5, 2024
thanks!!
True, done :) |
ShadowCurse
reviewed
Jul 8, 2024
kalyazin
previously approved these changes
Jul 8, 2024
When restoring MSRs, we currently do not enforce any sort of relative ordering. However, some MSRs have implicit dependencies on other MSRs being restored before them, and failing to fulfill these dependencies can result in incorrect VM behavior after resuming. One example of such an implicit dependency between MSRs is the pair (`MSR_IA32_TSC_DEADLINE`, `MSR_IA32_TSC`). When restoring `MSR_IA32_TSC_DEADLINE`, KVM internally checks whether the value of this restoration implies that the guest was waiting for the tsc_deadline timer to expire at the time of being paused for snapshotting. If yes, it primes a (either harddware or software depending on support) timer on the host to make sure the guest will receive the expected interrupt after restoration. To determine whether this is needed, KVM looks at the guest's timestamp counter (TSC) and compares it with the requested tsc_deadline value. The value KVM reads for the guest's TSC depends on the value of MSR_IA32_TSC. Thus, if MSR_IA32_TSC_DEADLINE is set before MSR_IA32_TSC is restored, this comparison will yield a wrong result (as the deadline value is compared with something uninitialized). This can either result in KVM determining the guest wasn't waiting for a timing expiry at the time of snapshotting, or cause it to schedule the timer interrupt too far into the future. Note that the former is only a problem on AMD platforms, which do not support the TSC_DEADLINE feature at the hardware level. Here, KVM falls back to a software timer, which explicitly does not notify the vCPU if the deadline value is "in the past". The hardware timer used on other x86 platforms on the other hand always fires (potentially firing immediately if the deadline value is in the past). This commit fixes the above by ensuring we always restore MSR_IA32_TSC before restoring MSR_IA32_TSC_DEADLINE. We realize this by splitting the lists of MSRs that KVM gives us into one additional chunk containing all "deferred" MSRs that needs to be restored "as late as possible". This splitting happens at snapshot creation time, to get it off the hot-path. Fixes firecracker-microvm#4099 Signed-off-by: Patrick Roy <[email protected]>
add changelog entry about firecracker-microvm#4099 Signed-off-by: Patrick Roy <[email protected]>
zulinx86
previously approved these changes
Jul 8, 2024
pb8o
reviewed
Jul 8, 2024
9 tasks
As the previous commit shows, the order of restoration of MSRs can be important. Thus, we should not effectively randomly shuffle them whenever we construct a VM. With this commit, we preserve the order in which KVM tells us about MSRs in `KVM_GET_MSR_INDEX_LIST`, under the assumption that if any dependencies exist, KVM will account for them as part of the above IOCTL. Signed-off-by: Patrick Roy <[email protected]>
ShadowCurse
approved these changes
Jul 8, 2024
kalyazin
approved these changes
Jul 8, 2024
pb8o
approved these changes
Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When restoring MSRs, we currently do not enforce any sort of relative ordering. However, some MSRs have implicit dependencies on other MSRs being restored before them, and failing to fulfill these dependencies can result in incorrect VM behavior after resuming.
One example of such an implicit dependency between MSRs is the pair (
MSR_IA32_TSC_DEADLINE,MSR_IA32_TSC). When restoringMSR_IA32_TSC_DEADLINE, KVM internally checks whether the value of this restoration implies that the guest was waiting for the tsc_deadline timer to expire at the time of being paused for snapshotting. If yes, it primes a (either hardware or software depending on support) timer on the host to make sure the guest will receive the expected interrupt after restoration. To determine whether this is needed, KVM looks at the guest's timestamp counter (TSC) and compares it with the requested tsc_deadline value. The value KVM reads for the guest's TSC depends on the value ofMSR_IA32_TSC[6][7]. Thus, ifMSR_IA32_TSC_DEADLINEis set beforeMSR_IA32_TSCis restored, this comparison will yield a wrong result (as the deadline value is compared with something uninitialized) [5]. This can either result in KVM determining the guest wasn't waiting for a timing expiry at the time of snapshotting, or cause it to schedule the timer interrupt too far into the future.Note that the former is only a problem on AMD platforms, which do not support the TSC_DEADLINE feature at the hardware level. Here, KVM falls back to a software timer [1], which explicitly does not notify the vCPU if the deadline value is "in the past" [2][3]. The hardware timer used on other x86 platforms on the other hand always fires (potentially firing immediately if the deadline value is in the past) [4].
This commit fixes the above by ensuring we always restore
MSR_IA32_TSCbefore restoringMSR_IA32_TSC_DEADLINE. We realize this by splitting the lists of MSRs that KVM gives us into one additional chunk containing all "deferred" MSRs that needs to be restored "as late as possible". This splitting happens at snapshot creation time, to get it off the hot-path.Fixes #4099 (validated by running the reproducer script for 50 iterations on an m6a.metal and not observing the issue anymore)
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.