Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Set mxcsr explicity on kvm and add tests
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
  • Loading branch information
jsturtevant committed Sep 23, 2025
commit 5a11cb05e597a59fc5c15a3a696c962bd6a37fcc
21 changes: 20 additions & 1 deletion src/hyperlight_host/src/hypervisor/kvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,8 @@ impl Hypervisor for KVMDriver {
};
self.vcpu_fd.set_regs(&regs)?;

// reset fpu state
// note kvm set_fpu doesn't actually set or read the mxcsr value
// https://elixir.bootlin.com/linux/v6.16/source/arch/x86/kvm/x86.c#L12229
let fpu = kvm_fpu {
fcw: FP_CONTROL_WORD_DEFAULT,
ftwx: FP_TAG_WORD_DEFAULT,
Copy link
Contributor

@ludfjig ludfjig Sep 23, 2025

Choose a reason for hiding this comment

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

nit: Since it's 0 we can just remove it from here and let the ..Default::default() zero it. We can probably remove the constant completely in this case unless it's used for something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking on this a bit and I think keeping it here as a const is a good idea since it allows us to be explicit about using the FXSAVE format (00 vs 11). It shouldn't have any impact but allows us to document that we are using the setting we explicitly want.

Expand All @@ -588,6 +589,24 @@ impl Hypervisor for KVMDriver {
};
self.vcpu_fd.set_fpu(&fpu)?;

// Set MXCSR from XSAVE (MXCSR is at byte offset 24 -> u32 index 6)
// Locations are from
// AMD64 Architecture Programmer's Manual, Volume 2
// 11.5.10 Mode-Specific XSAVE/XRSTOR State Management
let mut xsave = match self.vcpu_fd.get_xsave() {
Copy link
Contributor

@ludfjig ludfjig Sep 23, 2025

Choose a reason for hiding this comment

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

If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call? Do they overlap in functionality?

Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call?

I considered this but felt the the xsave structure was pretty opaque with lots that could change. It felt clearer/safer to do this and there didn't seem to be a noticeable perf difference.

Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?

This is a good idea! Let me take a look at this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?

This is a good idea! Let me take a look at this.

I tried this out by initializing the FP registers once and storing them. I didn't notice any impact on performance (but it's hard to measure at the function call level due to so many variances). Saving does incur an additional ~4k of memory per vm instance. I lean towards leaving this approach and revisiting it if required. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine. However, I think I am a little skeptical in general about using xsave just for the purpose of resetting just the single MXCSR register. There are plenty other registers we don't reset currently, that we probably should. Maybe let's take the opportunity to reset some more registers? What do you think?

Also, regarding

If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call?

I considered this but felt the the xsave structure was pretty opaque with lots that could change. It felt clearer/safer to do this and there didn't seem to be a noticeable perf difference.

I think xsave should be pretty standardized, at least per-hypervisor, so I do think it's worth looking into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are plenty other registers we don't reset currently, that we probably should. Maybe let's take the opportunity to reset some more registers? What do you think?

Yes, we have #791 and I started to look into other registers. I was using this as a way to get a pattern set up without going after everything in one go.

I think xsave should be pretty standardized, at least per-hypervisor, so I do think it's worth looking into this

xsave does have a specific layout depending on the features enabled. Maybe we could do this after #907 and other related changes.

Ok(xsave) => xsave,
Err(e) => {
return Err(new_error!("Could not write guest registers: {:?}", e));
}
};

xsave.region[6] = MXCSR_DEFAULT;
unsafe {
self.vcpu_fd
.set_xsave(&xsave)
.map_err(|e| new_error!("Could not write guest registers: {:?}", e))?
};

// run
VirtualCPU::run(
self.as_mut_hypervisor(),
Expand Down
27 changes: 27 additions & 0 deletions src/hyperlight_host/src/sandbox/initialized_multi_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ mod tests {
use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode;
use hyperlight_testing::simple_guest_as_string;

use crate::hypervisor::fpu;
#[cfg(target_os = "linux")]
use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType};
#[cfg(target_os = "linux")]
Expand Down Expand Up @@ -1021,4 +1022,30 @@ mod tests {
};
assert_ne!(sandbox3.id, sandbox_id);
}

/// Test that FPU control word and MXCSR register are properly reset between guest calls
#[test]
fn test_fpu_mxcsr_register_reset() {
let path = simple_guest_as_string().unwrap();
let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap();
let mut sandbox = u_sbox.evolve().unwrap();

// First, read the initial state of FPU and MXCSR registers
let initial_state: String = sandbox.call("ReadFpuMxcsr", ()).unwrap();
// Verify the initial state matches expected default values
let expected = format!("fcw:{:04X},mxcsr:{:08X}", fpu::FP_CONTROL_WORD_DEFAULT, fpu::MXCSR_DEFAULT);
assert_eq!(initial_state, expected,
"Initial FPU/MXCSR state does not match expected defaults");

// Modify the FPU and MXCSR registers to non-default values
let modify_result: String = sandbox.call("ModifyFpuMxcsr", ()).unwrap();
let expected = format!("fcw:{:04X},mxcsr:{:08X}", 0x027F, 0x1F00);
assert_eq!(modify_result, expected,
"Modified state does not match");

// A new call should reset these
let reset_state: String = sandbox.call("ReadFpuMxcsr", ()).unwrap();
assert_eq!(initial_state, reset_state,
"FPU/MXCSR registers were not properly reset between calls");
}
}
84 changes: 84 additions & 0 deletions src/tests/rust_guests/simpleguest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,73 @@ fn exec_mapped_buffer(function_call: &FunctionCall) -> Result<Vec<u8>> {
}
}

/// Simple function to modify FPU control word and MXCSR register
#[hyperlight_guest_tracing::trace_function]
fn modify_fpu_mxcsr(_function_call: &FunctionCall) -> Result<Vec<u8>> {
unsafe {
// Set FPU control word to a non-default value (default is usually 0x037F)
// We'll set it to 0x027F (change precision control and rounding)
let new_fcw: u16 = 0x027F;
core::arch::asm!(
"fldcw [{fcw_ptr}]",
fcw_ptr = in(reg) &new_fcw,
);

// Set MXCSR to a non-default value (default is usually 0x1F80)
// We'll set it to 0x1F00 (disable some exception masks)
let new_mxcsr: u32 = 0x1F00;
core::arch::asm!(
"ldmxcsr [{mxcsr_ptr}]",
mxcsr_ptr = in(reg) &new_mxcsr,
);
}

// now read them and return those values
let mut fcw: u16 = 0;
let mut mxcsr: u32 = 0;

unsafe {
// Read FPU control word
core::arch::asm!(
"fnstcw [{fcw_ptr}]",
fcw_ptr = in(reg) &mut fcw,
);

// Read MXCSR
core::arch::asm!(
"stmxcsr [{mxcsr_ptr}]",
mxcsr_ptr = in(reg) &mut mxcsr,
);
}

let result = format!("fcw:{:04X},mxcsr:{:08X}", fcw, mxcsr);
Ok(get_flatbuffer_result(result.as_str()))
}

/// Simple function to read current FPU control word and MXCSR values
#[hyperlight_guest_tracing::trace_function]
fn read_fpu_mxcsr(_function_call: &FunctionCall) -> Result<Vec<u8>> {
let mut fcw: u16 = 0;
let mut mxcsr: u32 = 0;

unsafe {
// Read FPU control word
core::arch::asm!(
"fnstcw [{fcw_ptr}]",
fcw_ptr = in(reg) &mut fcw,
);

// Read MXCSR
core::arch::asm!(
"stmxcsr [{mxcsr_ptr}]",
mxcsr_ptr = in(reg) &mut mxcsr,
);
}

let result = format!("fcw:{:04X},mxcsr:{:08X}", fcw, mxcsr);
Ok(get_flatbuffer_result(result.as_str()))
}

#[no_mangle]
#[hyperlight_guest_tracing::trace_function]
pub extern "C" fn hyperlight_main() {
Expand Down Expand Up @@ -1477,6 +1544,23 @@ pub extern "C" fn hyperlight_main() {
call_given_paramless_hostfunc_that_returns_i64 as usize,
);
register_function(call_given_hostfunc_def);

// Register FPU and MXCSR test functions
let modify_fpu_mxcsr_def = GuestFunctionDefinition::new(
"ModifyFpuMxcsr".to_string(),
Vec::new(),
ReturnType::String,
modify_fpu_mxcsr as usize,
);
register_function(modify_fpu_mxcsr_def);

let read_fpu_mxcsr_def = GuestFunctionDefinition::new(
"ReadFpuMxcsr".to_string(),
Vec::new(),
ReturnType::String,
read_fpu_mxcsr as usize,
);
register_function(read_fpu_mxcsr_def);
}

#[hyperlight_guest_tracing::trace_function]
Expand Down