Reset more vcpu state on snapshot::restore #1120

ludfjig · 2025-12-16T02:14:00Z

MSRs will be added in another PR.

Add debug register abstraction over hv implementation
Removed padding fields from fpu register to simplify comparisons. Not sure why I added them to begin with, we don't need C repr on these. It could possibly allow more efficient into() implementation for kvm/mshv due to single memcyp, but that seems like premature optimization to me
Reset vcpu state on snapshot::restore
Added bunch of tests

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

jsturtevant · 2025-12-16T21:40:44Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+        xsave
+    }
+
+    fn hyperlight_vm(code: &[u8]) -> Result<HyperlightVm> {


isn't this duplicating alot of the configuration code we do during VM setup in the test? how will we maintain this with the actual setup? What if there is a difference?

This is calling HyperlightVm::new so they should be the same

I am concerned about the logic setting up the exclusive memory, stack cookie, etc. It seems like alot of the same in set_up_hypervisor_partition but simplier and having that not be the same might cause difference in configuration that might not be caught. This seems like a good case of doing integration testing at the host API and guest api layer.

Yeah it does indeed do a lot of setup... But that's more related to memory, and whether it's different or not shouldn't affect these tests which exercises vcpu state tests (I think)

After rebasing on top of main branch with changes that changed snapshotting, it got even worse. But I still believe it's fine, because memory-specifics is irrelevant to these tests.

I don't love this but I guess it might be fine. We might be able to add an abstraction that "preps the vm" but I haven't really thought this through fully.

Copilot

Pull request overview

This PR enhances vCPU state management during snapshot restoration by adding debug register abstraction and ensuring all vCPU state is properly reset. The changes follow the established Common* struct pattern for hypervisor abstraction and include comprehensive test coverage across KVM, MSHV, and WHP hypervisors.

Key changes:

Added CommonDebugRegs abstraction with implementations for all supported hypervisors (KVM, MSHV, WHP)
Removed padding fields from CommonFpu to simplify comparisons while maintaining correct conversions to hypervisor-specific types
Implemented reset_vcpu() to reset general purpose registers, debug registers, FPU/XSAVE state, and special registers
Added reset_vcpu() call in MultiUseSandbox::restore() to ensure clean state after snapshot restoration
Added extensive tests covering all reset scenarios with both direct state setting and actual code execution

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`src/hyperlight_host/src/hypervisor/regs/debug_regs.rs`	New file implementing CommonDebugRegs abstraction with conversions for KVM, MSHV, and WHP
`src/hyperlight_host/src/hypervisor/regs/fpu.rs`	Removed pad1 and pad2 fields from CommonFpu; updated all conversions to use literal 0 for padding
`src/hyperlight_host/src/hypervisor/regs.rs`	Added debug_regs module export
`src/hyperlight_host/src/hypervisor/mod.rs`	Added debug_regs, set_debug_regs, reset_xsave, and set_xsave methods to Hypervisor trait
`src/hyperlight_host/src/hypervisor/kvm.rs`	Implemented debug register and xsave methods for KVM hypervisor
`src/hyperlight_host/src/hypervisor/hyperv_linux.rs`	Implemented debug register and xsave methods for MSHV hypervisor
`src/hyperlight_host/src/hypervisor/hyperv_windows.rs`	Implemented debug register and xsave methods for WHP; refactored gdb breakpoint code to use new abstractions
`src/hyperlight_host/src/hypervisor/hyperlight_vm.rs`	Added reset_vcpu() method and comprehensive test suite covering all vCPU state reset scenarios
`src/hyperlight_host/src/sandbox/initialized_multi_use.rs`	Added reset_vcpu() call in restore() and test to verify debug registers are reset
`src/hyperlight_host/src/sandbox/hypervisor.rs`	Added Copy and Clone derives to HypervisorType enum
`src/hyperlight_host/src/mem/layout.rs`	Changed visibility from pub(super) to pub(crate) for methods used by tests
`src/tests/rust_guests/simpleguest/src/main.rs`	Added SetDr0 and GetDr0 guest functions for testing debug register reset
`.github/workflows/ValidatePullRequest.yml`	Changed fail-fast from true to false for build-test job to allow all matrix combinations to complete

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

Signed-off-by: Ludvig Liljenberg <[email protected]>

Signed-off-by: Ludvig Liljenberg <[email protected]> Make set_xsave take slice Signed-off-by: Ludvig Liljenberg <[email protected]>

jsturtevant

This is great to see. Great work figuring out the differences between the hypervisors and paying attention and documenting the differences. I learned a bunch.

My main concern is around the readability and maintenance of the tests. I've left a few suggestions on how we could maybe clean them up to be more concise and easier to reason about

jsturtevant · 2026-01-16T20:55:25Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+        // Note: On KVM this ignores MXCSR so it's being set as part of reset_xsave.
+        // See https://github.com/torvalds/linux/blob/d358e5254674b70f34c847715ca509e46eb81e6f/arch/x86/kvm/x86.c#L12554-L12599
+        self.vm.set_fpu(&CommonFpu::default())?;
+        self.vm.reset_xsave()?;


I believe FPU state xsave overlap, could we just do the reset here and save a few cycles?

jsturtevant · 2026-01-16T20:58:01Z

src/hyperlight_host/src/hypervisor/regs/debug_regs.rs

thought: this would be a smaller pr if this was a sperate PR and would make its easier to review.

jsturtevant · 2026-01-16T21:08:59Z

src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs

+    #[cfg(feature = "init-paging")]
+    fn set_xsave(&self, xsave: &[u32]) -> Result<()>;


why is it only for the init-paging feature? shouldn't there be a test for all the features?

jsturtevant · 2026-01-16T21:14:20Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+            cr8: 0x5,
+            efer: 0x500, // LME + LMA
+            apic_base: 0xFEE00900,
+            // interrupt_bitmap: [0xFFFFFFFF; 4],


nit: may need to remove this comment? or do something differnet for mshv

jsturtevant · 2026-01-16T21:42:03Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+        expected_sregs.ss.db = got_sregs.ss.db;
+        // unusable and g are hypervisor implementation details (see comment below for details)
+        expected_sregs.cs.unusable = got_sregs.cs.unusable;
+        expected_sregs.cs.g = got_sregs.cs.g;
+        expected_sregs.ds.unusable = got_sregs.ds.unusable;
+        expected_sregs.ds.g = got_sregs.ds.g;
+        expected_sregs.es.unusable = got_sregs.es.unusable;
+        expected_sregs.es.g = got_sregs.es.g;
+        expected_sregs.fs.unusable = got_sregs.fs.unusable;
+        expected_sregs.fs.g = got_sregs.fs.g;
+        expected_sregs.gs.unusable = got_sregs.gs.unusable;
+        expected_sregs.gs.g = got_sregs.gs.g;
+        expected_sregs.ss.unusable = got_sregs.ss.unusable;
+        expected_sregs.ss.g = got_sregs.ss.g;
+        expected_sregs.tr.unusable = got_sregs.tr.unusable;
+        expected_sregs.tr.g = got_sregs.tr.g;
+        expected_sregs.ldt.unusable = got_sregs.ldt.unusable;
+        expected_sregs.ldt.g = got_sregs.ldt.g;
+        assert_eq!(got_sregs, expected_sregs);


could we move this to a single function (shared with the one below) so the comment stays the same? There is alot of duplication in the function and is really hard to read. and know the different phases we are in.

jsturtevant · 2026-01-16T21:43:45Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+        }
+        // fpr only uses 80 bits per register. Normalize upper bits for comparison.
+        for i in 0..8 {
+            expected_fpu.fpr[i][10..16].copy_from_slice(&got_fpu.fpr[i][10..16]);


this is duplicated in multiple spots in the function, could we condense it?

jsturtevant · 2026-01-16T21:44:15Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+    }
+
+    #[test]
+    fn reset_vcpu_simple() {


In general, this test is really hard to read. Could we evaluate a way to make it easier to read?

jsturtevant · 2026-01-16T21:47:33Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+                rip: 0,
+                rflags: 1 << 1, // Reserved bit 1 is always set
+            };
+            assert_eq!(regs, expected_reset);


this looks to be the same as in simple_reset:
// Verify the fpu was reset to defaults
assert_eq!(
hyperlight_vm.vm.regs().unwrap(),
CommonRegisters {
rflags: 1 << 1, // Reserved bit 1 is always set
..Default::default()
}
);

could we add helper funciton like verify_registers_reset and re-use this across all the tests? that would make off the tests more consistent and easier to read.

jsturtevant · 2026-01-16T21:59:23Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+            #[rustfmt::skip]
+            const CODE: [u8; 289] = [
+                // xmm0-xmm7: use movd + pshufd to fill with pattern
+                0xb8, 0x11, 0x11, 0x11, 0x11,       // mov eax, 0x11111111


This seems really hard to reason about, Could we use something like https://docs.rs/iced-x86/latest/iced_x86/ or put the files in test.asm and compile them at build time with nasm then bring them in with include_bytes

jsturtevant · 2026-01-16T22:01:27Z

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

+    /// Test that snapshot restore properly resets vCPU debug registers. This test verifies
+    /// that restore() calls reset_vcpu.
+    #[test]
+    fn snapshot_restore_resets_debug_registers() {


could we also add a test like this for other registers too? Doesn't need to be all the registers but atleast the common registers kind of like it is done here?

ludfjig added the kind/enhancement For PRs adding features, improving functionality, docs, tests, etc. label Dec 16, 2025

ludfjig force-pushed the reset_vcpu branch 3 times, most recently from 9cfcc9c to 8507c7a Compare December 16, 2025 19:52

jsturtevant reviewed Dec 16, 2025

View reviewed changes

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs Show resolved Hide resolved

jsturtevant reviewed Dec 16, 2025

View reviewed changes

ludfjig force-pushed the reset_vcpu branch 11 times, most recently from d09b1fc to 18d4ff9 Compare December 17, 2025 06:46

ludfjig requested a review from Copilot December 17, 2025 06:53

Copilot started reviewing on behalf of ludfjig December 17, 2025 06:54 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

ludfjig marked this pull request as ready for review December 17, 2025 07:26

ludfjig requested review from danbugs, dblnz, devigned, jprendes, marosset, simongdavies and syntactically as code owners December 17, 2025 07:26

jsturtevant reviewed Dec 18, 2025

View reviewed changes

src/hyperlight_host/src/sandbox/initialized_multi_use.rs Show resolved Hide resolved

ludfjig force-pushed the reset_vcpu branch from 18d4ff9 to 5e5a976 Compare January 10, 2026 01:08

ludfjig force-pushed the reset_vcpu branch from 4320d54 to 229328e Compare January 12, 2026 22:13

Implement debug register abstraction

48dc640

Signed-off-by: Ludvig Liljenberg <[email protected]>

ludfjig force-pushed the reset_vcpu branch 2 times, most recently from 4981f2d to 180efe7 Compare January 12, 2026 22:56

Reset vcpu state

6cf483b

Signed-off-by: Ludvig Liljenberg <[email protected]> Make set_xsave take slice Signed-off-by: Ludvig Liljenberg <[email protected]>

ludfjig force-pushed the reset_vcpu branch from 180efe7 to 6cf483b Compare January 12, 2026 23:35

jsturtevant requested changes Jan 16, 2026

View reviewed changes

		#[cfg(feature = "init-paging")]
		fn set_xsave(&self, xsave: &[u32]) -> Result<()>;

Reset more vcpu state on snapshot::restore #1120

Are you sure you want to change the base?

Reset more vcpu state on snapshot::restore #1120

Uh oh!

Conversation

ludfjig commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ludfjig Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ludfjig commented Dec 16, 2025 •

edited

Loading

ludfjig Jan 13, 2026 •

edited

Loading