SNP: Test and set guest busy bit atomically. #2695
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon #Closed |
| } | ||
|
|
||
| /// Gets an atomic reference to the v_intr_cntrl field. | ||
| pub fn v_intr_cntrl_atomic(&self) -> &AtomicU64 { |
There was a problem hiding this comment.
Is it possible for a caller to get a non-atomic ref to this field? Can we make that not be possible?
Does this function need to be unsafe? Does it need to be pub?
There was a problem hiding this comment.
yes the goal of this is to allow that
There was a problem hiding this comment.
We need to be careful here, the Rust aliasing requirements state that if code holds a shared reference (&) to some memory then that memory can not change out from under it. This code as written allows such a change to happen, as we can hold an &u64 and &AtomicU64 to the same memory, which could allow for the value pointed to by said &u64 to change out from under it, which would be Undefined Behavior.
There was a problem hiding this comment.
I would suggest either requiring &mut self for this (to ensure aliasing isn't possible (although that might not be sufficient if a caller can create a new &u64 to this field after calling this method and keeping the &AtomicU64 around)), or requiring all accesses to this field to go through this new Atomic, and prevent doing it through regular accesses. And then mentioning whichever you go with as justification in the SAFETY comment
There was a problem hiding this comment.
Pull request overview
This PR addresses the need to atomically test and set the guest busy bit when running SNP guest VPs, and adds error handling for cases where the VP cannot be restarted due to issues with the guest APIC backing page.
Changes:
- Added
not_restartablefield toSevNpfInfostructure andNOT_RESTARTABLEexit code to handle non-restartable errors - Implemented atomic test-and-set operation for the guest busy bit in v_intr_cntrl field
- Refactored event injection logic to use atomic operations and handle NOT_RESTARTABLE errors by triggering triple fault
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vm/x86/x86defs/src/snp.rs | Added not_restartable bit to SevNpfInfo structure (bit 63) and NOT_RESTARTABLE exit code (0x406) |
| openhcl/hcl/src/vmsa.rs | Added atomic operations for v_intr_cntrl field including guest_busy_bit_test_and_set() method using fetch_or with SeqCst ordering |
| openhcl/virt_mshv_vtl/src/processor/snp/mod.rs | Refactored to use atomic test-and-set for guest_busy bit, added error handling for NOT_RESTARTABLE cases, and made exit_int_info validation conditional instead of asserting |
| pub fn guest_busy_bit_test_and_set(&mut self) -> bool { | ||
| const VINTR_GUEST_BUSYBIT_MASK: u64 = 1u64 << 63; | ||
| let prev = self | ||
| .v_intr_cntrl_atomic() | ||
| .fetch_or(VINTR_GUEST_BUSYBIT_MASK, Ordering::SeqCst); | ||
| (prev & VINTR_GUEST_BUSYBIT_MASK) != 0 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, this needs to be fixed up, we should not make it so easy for callers to get things wrong here.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This is useful context that should be in the SAFETY comment to explain why the unsafe code in question is safe.
| // The guest APIC backing page is not validated in the RMP. | ||
| return Err(VpHaltReason::TripleFault { | ||
| vtl: entered_from_vtl.into(), | ||
| }); |
There was a problem hiding this comment.
Is there a way to set bugcheck params?
There was a problem hiding this comment.
You should use dev.fatal_error instead of returning a TripleFault
|
|
||
| /// Gets an atomic reference to the v_intr_cntrl field. | ||
| pub fn v_intr_cntrl_atomic(&self) -> &AtomicU64 { | ||
| // SAFETY: caller responsible for synchronizing with any non-atomic access. |
There was a problem hiding this comment.
SAFETY comments are meant to explain why a piece of unsafe code is impossible to misuse. Simply stating that the responsibility is on the caller is not sufficient, unless the function is also marked unsafe and contains its own Safety comment explaining what the caller needs to do to maintain safety.
| _ => Some(exit_int_info), | ||
| }; | ||
|
|
||
| vmsa.set_exit_int_info(0); |
There was a problem hiding this comment.
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
3 participants
When running an SNP guest VP, test and set the guest busy bit atomically. Handle cases where the VP is not able to be restarted due to access issues with the guest APIC backing page. The guest busy bit is used for both secure AVIC and alternate injection scenarios.