Use WHvResetPartition on windows#1360
Conversation
ludfjig
added
the
kind/enhancement
ludfjig
force-pushed
the
windows_scrub2
branch
3 times, most recently
from
4d1a570 to
2b61178
Compare
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
ludfjig
requested review from
danbugs,
dblnz,
devigned,
jprendes,
marosset,
simongdavies and
syntactically
as code owners
simongdavies
left a comment
simongdavies
left a comment
There was a problem hiding this comment.
Looks good , just a few comments, it would be good to tighten up the AMD fix targeting and also see if we can get rid of the need to call sregs reset independently
| /// | ||
| /// This does NOT restore special registers (except on windows). Call `restore_sregs` separately | ||
| /// after memory mappings are established. | ||
| // TODO: check if other state needs to be reset |
There was a problem hiding this comment.
Is this TODO still valid (presumably not on Windows) is there an issue to track this
| /// GP registers, debug registers, XSAVE, MSRs, APIC, etc.) | ||
| /// - On Linux: explicitly resets GP registers, debug registers, and XSAVE | ||
| /// | ||
| /// This does NOT restore special registers (except on windows). Call `restore_sregs` separately |
There was a problem hiding this comment.
This seems a bit fragile, why can we can restore_sregs from here?
| ..Default::default() | ||
| }; | ||
| assert_ne!( | ||
| regs.rdx, 0x4444444444444444, |
There was a problem hiding this comment.
What's the significance of this value?
| @@ -107,7 +107,7 @@ pub(crate) enum HypervisorType { | |||
| /// Minimum XSAVE buffer size: 512 bytes legacy region + 64 bytes header. | |||
| /// Only used by MSHV and WHP which use compacted XSAVE format and need to | |||
| /// | ||
| /// This bug most commonly surfaces as the `interrupt_same_thread_no_barrier` | ||
| /// integration test failing on AMD Windows hosts. @ludfjig has verified | ||
| /// the fix works on Windows Insider builds; this workaround can be |
There was a problem hiding this comment.
Can we get an issue to track this? Even better is there a way we can have a test that fails when this is fixed in windows? That will force us then to gate this code on a specific windows version or lower since I think the comment is wrong, we will need this to endure but will only need to apply it on impacted versions of windows
| /// The workaround unmaps and remaps a dummy page after each | ||
| /// `WHvResetPartition`, which forces the hypervisor to invalidate | ||
| /// NPT entries. | ||
| mod npt_flush { |
There was a problem hiding this comment.
can we also gate this on affected CPUs only?
2 participants
Addresses #791 for windows (WHP), by resetting all guest visible state on the partition. Resets more vm state on
restore()at the expense of some performance (roughly ~100micros). MSHV is planned to use the same hv call, but doesn't exist in mshv crate yet, although dom0 kernel should have support already. KVM doesn't have a nice api like this, so will still manual work for KVM.Note: the second commit is a workaround for a hyper-v bug