Reset more vcpu state on snapshot::restore #1120

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

MSRs will be added in another PR.

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
MSRs are not included in this PR. Will add MSRs in future PR

Addresses #791 partially

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.

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

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

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

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

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

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

jsturtevant · 2026-01-23T18:08:51Z

.github/workflows/ValidatePullRequest.yml

      - build-guests
    strategy:
-      fail-fast: true
+      fail-fast: false


is this purposeful?

no it just helps me debugging, will remove this before merging 👍

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

dblnz

Good work! I left some comments below.

dblnz · 2026-01-27T07:17:08Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+    // Resets the following vCPU state:
+    // - General purpose registers
+    // - Debug registers
+    // - XSAVE (includes FPU/SSE state with proper FCW and MXCSR defaults)
+    // - Special registers (with saved PML4 if feature enabled)
+    // TODO: check if other state needs to be reset


Nit

Suggested change

// Resets the following vCPU state:

// - General purpose registers

// - Debug registers

// - XSAVE (includes FPU/SSE state with proper FCW and MXCSR defaults)

// - Special registers (with saved PML4 if feature enabled)

// TODO: check if other state needs to be reset

/// Resets the following vCPU state:

/// - General purpose registers

/// - Debug registers

/// - XSAVE (includes FPU/SSE state with proper FCW and MXCSR defaults)

/// - Special registers (with saved PML4 if feature enabled)

/// TODO: check if other state needs to be reset

dblnz · 2026-01-27T07:28:05Z

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

        }

+        // if this fails, then the sandbox will not have been restored so keep it poisoned (if it is)
+        self.vm.reset_vcpu().map_err(HyperlightVmError::Restore)?;


What happens if the sandbox is not poisoned (good state), and it fails here?
Won't it be partially reset?
I mean, 15 lines above we run the restore_snapshot method, which restores the memory, so if this fails, it will remain not poisoned, with the memory reset, but not the registers, right?

dblnz · 2026-01-27T07:37:03Z

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


 /// The hypervisor types available for the current platform
-#[derive(PartialEq, Eq, Debug)]
+#[derive(PartialEq, Eq, Debug, Copy, Clone)]


Unrelated question: Why is the HypervisorType defined twice?
Once here, the other in sandbox/hypervisor.rs.

jsturtevant · 2026-01-27T19:29:56Z

src/hyperlight_host/src/hypervisor/virtual_machine/mshv.rs

+        let mut buf = XSave::default(); // default is zeroed 4KB buffer
+
+        // Copy XCOMP_BV (offset 520-527) - preserves feature mask + compacted bit
+        buf.buffer[520..528].copy_from_slice(&current_xsave.buffer[520..528]);


Do we need to preserve this in kvm as well?

jsturtevant · 2026-01-27T19:32:33Z

src/hyperlight_host/src/hypervisor/virtual_machine/kvm.rs

+        // - Bytes 0-1: FCW (x87 FPU Control Word)
+        // - Bytes 24-27: MXCSR
+        // - Bytes 512-519: XSTATE_BV (bitmap of valid state components)
+        xsave.region[0] = FP_CONTROL_WORD_DEFAULT as u32;


this is writing 32 bits to a 16 bit field. We only write the 16 bits in mshv. Is this intended?

jsturtevant · 2026-01-27T19:39:50Z

src/hyperlight_host/src/hypervisor/virtual_machine/mshv.rs

    }
+
+    fn reset_xsave(&self) -> std::result::Result<(), RegisterError> {
+        const XSAVE_MIN_SIZE: usize = 576; // 512 legacy + 64 header


this is the same constant as in whp, consider sharing it?

jsturtevant · 2026-01-27T19:46:19Z

src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs

+        }
+
+        let provided_size = std::mem::size_of_val(xsave) as u32;
+        if buffer_size_needed > provided_size {


in kvm and mshv we do an explicit check for std::mem::size_of_val(xsave) != KVM_XSAVE_SIZE_BYTES and here we don't. it means we might provide a buffer to overwrite values and only set certain amount. this is just test code but could help with strange bugs where we think we are doing something different.

jsturtevant · 2026-01-27T19:50:43Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+            // Check FPU is reset to defaults
+            assert_fpu_reset(hyperlight_vm.vm.as_ref());
+
+            // Verify MXCSR via xsave on KVM


might just leave a comment here, that using fpu to check this doesn't work on kvm and is why we do the extra read via xsave

jsturtevant · 2026-01-27T19:51:26Z

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

+            hyperlight_vm.reset_vcpu().unwrap();
+
+            // Check registers are reset to defaults
+            assert_regs_reset(hyperlight_vm.vm.as_ref());


this is much cleaner thanks!

jsturtevant

looking good. A few comments to address otherwise I like it!

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

This comment was marked as outdated.

Sign in to view

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 4 times, most recently from 180efe7 to 6cf483b Compare January 12, 2026 23:35

jsturtevant requested changes Jan 16, 2026

View reviewed changes

ludfjig mentioned this pull request Jan 20, 2026

Add debug register abstraction for Vm #1180

Merged

ludfjig force-pushed the reset_vcpu branch 7 times, most recently from 1e8d95a to 71d6a38 Compare January 22, 2026 00:09

jsturtevant reviewed Jan 23, 2026

View reviewed changes

ludfjig force-pushed the reset_vcpu branch 2 times, most recently from 2ccb53d to 2091ef4 Compare January 26, 2026 20:59

Reset more vcpu state on snapshot restore

5ebe998

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

ludfjig force-pushed the reset_vcpu branch from 2091ef4 to 5ebe998 Compare January 26, 2026 21:09

dblnz reviewed Jan 27, 2026

View reviewed changes

jsturtevant reviewed Jan 27, 2026

View reviewed changes

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

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!

This comment was marked as outdated.

Uh oh!

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblnz 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!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ludfjig commented Dec 16, 2025 •

edited

Loading

ludfjig Jan 13, 2026 •

edited

Loading