Skip to content

SNP: Test and set guest busy bit atomically. #2695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jennagoddard wants to merge 5 commits into
base: main
Choose a base branch
from
Open

SNP: Test and set guest busy bit atomically. #2695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
530e452
Test and set busy bit atomically
jennagoddard Jan 26, 2026
046d1b4
fix small nit
jennagoddard Jan 26, 2026
338f80e
address feedback
jennagoddard Jan 29, 2026
c7c6a56
small update
jennagoddard Mar 19, 2026
493247b
cache exit int info
jennagoddard Mar 20, 2026
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
21 changes: 21 additions & 0 deletions openhcl/hcl/src/vmsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use std::array;
use std::ops::Deref;
use std::ops::DerefMut;
use std::sync::atomic::AtomicU64;
use std::sync::atomic::Ordering;
use x86defs::snp::SevEventInjectInfo;
use x86defs::snp::SevFeatures;
use x86defs::snp::SevSelector;
Expand Down Expand Up @@ -96,6 +98,16 @@ impl<T: DerefMut<Target = SevVmsa>> VmsaWrapper<'_, T> {
| ((self.set_u64((v >> 64) as u64, offset + 8) as u128) << 64)
}

/// Gets an atomic reference to the v_intr_cntrl field.
/// # Safety
/// v_intr_cntrl must not be accessed by other VPs.
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft Jan 30, 2026

Choose a reason for hiding this comment

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

Not just other VPs, no normal rust reference can exist to it while this one does, as modifying something behind a normal reference is UB. And actually, other VPs accessing it would be fine so long as they are also doing so atomically. Is it possible to restrict access to this field so that it can only be accessed atomically?

unsafe fn v_intr_cntrl_atomic(&mut self) -> &AtomicU64 {
// SAFETY: the vintr control field is on the VMSA which is per-VP per-VTL.
// As VPs cannot run on multiple processors at the same time there is a
// limited need for synchronization on the vintr control field.
unsafe { &*(core::ptr::from_ref(&self.vmsa.v_intr_cntrl).cast::<AtomicU64>()) }
Comment on lines +105 to +108
}

/// Create a new VMSA
pub fn reset(&mut self, vmsa_reg_prot: bool) {
*self.vmsa = FromZeros::new_zeroed();
Expand Down Expand Up @@ -157,6 +169,15 @@ impl<T: DerefMut<Target = SevVmsa>> VmsaWrapper<'_, T> {
self.vmsa.x87_registers[i] = val;
}
}

/// Atomically test and set the guest busy bit in v_intr_cntrl.
pub fn guest_busy_bit_test_and_set(&mut self) -> bool {
const VINTR_GUEST_BUSYBIT_MASK: u64 = 1u64 << 63;
// SAFETY: v_intr_cntrl is only accessed by this VP.
let prev = unsafe { self.v_intr_cntrl_atomic() }
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft Mar 9, 2026

Choose a reason for hiding this comment

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

Is this the only place where this atomic access is needed? If so can we just inline it?

Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft Mar 19, 2026

Choose a reason for hiding this comment

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

I'd definitely feel more comfortable with inlining this method instead of having it separate, it removes a lot of safety concerns.

.fetch_or(VINTR_GUEST_BUSYBIT_MASK, Ordering::SeqCst);
(prev & VINTR_GUEST_BUSYBIT_MASK) != 0
}
Comment on lines +174 to +180
Copy link

Copilot AI Jan 26, 2026 •
edited by jennagoddard
Loading

Choose a reason for hiding this comment

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

The atomic test-and-set operation mixes with non-atomic accesses to the same v_intr_cntrl field (e.g., in processor/snp/mod.rs at lines 1233, 1324, 1334). While the SAFETY comment acknowledges the caller's responsibility for synchronization, and the current sequential code structure ensures safety, consider whether all call sites properly maintain this invariant. If future changes introduce concurrent access patterns, this could become undefined behavior. Consider documenting this requirement more explicitly in the function's doc comment. #Resolved

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft Jan 26, 2026

Choose a reason for hiding this comment

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

Yeah, this needs to be fixed up, we should not make it so easy for callers to get things wrong here.

Copy link
Copy Markdown
Contributor Author

@jennagoddard jennagoddard Jan 26, 2026

Choose a reason for hiding this comment

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

the vintr control field is on the VMSA which is per-VP per-VTL. As VPs don't run on multiple processors at the same time there is a limited need for synchronization on the vintr control field in a performance sensitive path

Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft Jan 26, 2026

Choose a reason for hiding this comment

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

This is useful context that should be in the SAFETY comment to explain why the unsafe code in question is safe.

Comment on lines +173 to +180
}

/// Check bitmap to see if a register is included in masking.
Expand Down
83 changes: 54 additions & 29 deletions openhcl/virt_mshv_vtl/src/processor/snp/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,12 @@ enum SnpGhcbError {
}

#[derive(Debug, Error)]
#[error("failed to run")]
struct SnpRunVpError(#[source] hcl::ioctl::Error);
enum SnpRunVpError {
#[error("Guest AVIC backing page is not validated or cannot be accessed.")]
VpNotRestartableError,
#[error("failed to run")]
RunVpError(#[source] hcl::ioctl::Error),
}

/// A backing for SNP partitions.
#[derive(InspectMut)]
Expand Down Expand Up @@ -1256,31 +1260,50 @@ impl UhProcessor<'_, SnpBacked> {
let mut has_intercept = self
.runner
.run()
.map_err(|e| dev.fatal_error(SnpRunVpError(e).into()))?;
.map_err(|e| dev.fatal_error(SnpRunVpError::RunVpError(e).into()))?;

let entered_from_vtl = next_vtl;
let mut vmsa = self.runner.vmsa_mut(entered_from_vtl);
let exit_int_info_trace = SevEventInjectInfo::from(vmsa.exit_int_info());

// TODO SNP: The guest busy bit needs to be tested and set atomically.
let inject = if vmsa.sev_features().alternate_injection() {
if vmsa.v_intr_cntrl().guest_busy() {
if vmsa.sev_features().alternate_injection() {
let was_busy = vmsa.guest_busy_bit_test_and_set();
if was_busy {
self.backing.general_stats[entered_from_vtl]
.guest_busy
.increment();
// Software interrupts/exceptions cannot be automatically re-injected, but RIP still
// points to the instruction and the event should be re-generated when the
// instruction is re-executed. Note that hardware does not provide instruction
// length in this case so it's impossible to directly re-inject a software event if
// delivery generates an intercept.
//
// TODO SNP: Handle ICEBP.
let exit_int_info = SevEventInjectInfo::from(vmsa.exit_int_info());
assert!(
exit_int_info.valid(),
"event inject info should be valid {exit_int_info:x?}"
);

match exit_int_info.interruption_type() {
let sev_error_code = SevExitCode(vmsa.guest_error_code());
match sev_error_code {
SevExitCode::NOT_RESTARTABLE => {
// The guest APIC backing page is not validated in the RMP.
return Err(dev.fatal_error(SnpRunVpError::VpNotRestartableError.into()));
}
SevExitCode::NPF => {
let exit_info = SevNpfInfo::from(vmsa.exit_info1());
if exit_info.not_restartable() {
// An access to the guest's APIC backing page by AVIC hardware resulted
// in a nested page fault.
Comment on lines +1279 to +1286
return Err(
dev.fatal_error(SnpRunVpError::VpNotRestartableError.into())
);
}
}
_ => {}
}
}

// Software interrupts/exceptions cannot be automatically re-injected, but RIP still
// points to the instruction and the event should be re-generated when the
// instruction is re-executed. Note that hardware does not provide instruction
// length in this case so it's impossible to directly re-inject a software event if
// delivery generates an intercept.
//
// TODO SNP: Handle ICEBP.
let exit_int_info = SevEventInjectInfo::from(vmsa.exit_int_info());

if exit_int_info.valid() {
let inject = match exit_int_info.interruption_type() {
x86defs::snp::SEV_INTR_TYPE_EXCEPT => {
if exit_int_info.vector() != 3 && exit_int_info.vector() != 4 {
// If the event is an exception, we can inject it.
Expand All @@ -1291,21 +1314,23 @@ impl UhProcessor<'_, SnpBacked> {
}
x86defs::snp::SEV_INTR_TYPE_SW => None,
_ => Some(exit_int_info),
};

if let Some(inject) = inject {
vmsa.set_event_inject(inject);
}

// Since the exit interrupt information was processed, it must be
// cleared so that it is not examined again on a subsequent reentry to
// the HCL.
vmsa.set_exit_int_info(0);
Copy link
Copy Markdown
Contributor

@sluck-msft sluck-msft Jan 26, 2026 •
edited by jennagoddard
Loading

Choose a reason for hiding this comment

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

I don't remember this code well enough, but is it safe to clear the exit_int_info here? Looks like it's used below #Resolved

Copy link
Copy Markdown
Contributor Author

@jennagoddard jennagoddard Jan 29, 2026

Choose a reason for hiding this comment

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

yes added a comment to clarify

} else {
None
// Any previously injected event has been consumed.
}
} else {
unimplemented!("Only alternate injection is supported for SNP")
};

if let Some(inject) = inject {
vmsa.set_event_inject(inject);
}
if vmsa.sev_features().alternate_injection() {
vmsa.v_intr_cntrl_mut().set_guest_busy(true);
}

if last_interrupt_ctrl.irq() && !vmsa.v_intr_cntrl().irq() {
self.backing.general_stats[entered_from_vtl]
.int_ack
Expand Down Expand Up @@ -1624,7 +1649,7 @@ impl UhProcessor<'_, SnpBacked> {
tracing::error!(
CVM_CONFIDENTIAL,
"SEV exit code {sev_error_code:x?} sev features {:x?} v_intr_control {:x?} event inject {:x?} \
vmpl {:x?} cpl {:x?} exit_info1 {:x?} exit_info2 {:x?} exit_int_info {:x?} virtual_tom {:x?} \
vmpl {:x?} cpl {:x?} exit_info1 {:x?} exit_info2 {:x?} exit_int_info {:x?} virtual_tom {:x?} \
efer {:x?} cr4 {:x?} cr3 {:x?} cr0 {:x?} rflag {:x?} rip {:x?} next rip {:x?}",
vmsa.sev_features(),
vmsa.v_intr_cntrl(),
Expand All @@ -1633,7 +1658,7 @@ impl UhProcessor<'_, SnpBacked> {
vmsa.cpl(),
vmsa.exit_info1(),
vmsa.exit_info2(),
vmsa.exit_int_info(),
exit_int_info_trace,
vmsa.virtual_tom(),
vmsa.efer(),
vmsa.cr4(),
Expand Down
4 changes: 3 additions & 1 deletion vm/x86/x86defs/src/snp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,9 @@ pub struct SevNpfInfo {
pub rmp_size_mismatch: bool,
pub vmpl_violation: bool,
pub npt_supervisor_shadow_stack: bool,
#[bits(26)]
#[bits(25)]
rsvd38_63: u64,
pub not_restartable: bool,
}

/// SEV VMSA structure representing CPU state
Expand Down Expand Up @@ -646,6 +647,7 @@ open_enum::open_enum! {
AVIC_NOACCEL = 0x402,
VMGEXIT = 0x403,
PAGE_NOT_VALIDATED = 0x404,
NOT_RESTARTABLE = 0x406,

// SEV-ES software-defined exit codes
SNP_GUEST_REQUEST = 0x80000011,
Expand Down
Loading