move started to uhcvmvpinner

sluck-msft · sluck-msft · commit 2547a0e43030 · 2025-02-26T13:56:55.000-08:00
diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs
@@ -346,8 +346,6 @@ pub struct UhCvmVpState {
     hv: VtlArray<ProcessorVtlHv, 2>,
     /// LAPIC state.
     lapics: VtlArray<LapicState, 2>,
-    vtl1_enabled: Mutex<bool>,
-    vp_started: bool,
 }
 
 #[cfg(guest_arch = "x86_64")]
@@ -384,8 +382,6 @@ impl UhCvmVpState {
             exit_vtl: GuestVtl::Vtl0,
             hv,
             lapics,
-            vtl1_enabled: Mutex::new(false),
-            vp_started: false,
         }
     }
 }
@@ -424,6 +420,8 @@ pub struct UhCvmVpInner {
     tlb_lock_info: VtlArray<TlbLockInfo, 2>,
     /// Whether VTL 1 has been enabled on the vp
     vtl1_enabled: Mutex<bool>,
+    /// Whether the VP has been started via the StartVp hypercall.
+    started: AtomicBool,
 }
 
 #[cfg_attr(guest_arch = "aarch64", expect(dead_code))]
@@ -499,7 +497,7 @@ struct VbsIsolatedVtl1State {
 #[derive(Clone, Copy, Default, Inspect)]
 struct HardwareCvmVtl1State {
     /// Whether VTL 1 has been enabled on any vp
-    enabled_on_vp_count: u32,
+    enabled_on_any_vp: bool,
     /// Whether guest memory should be zeroed before it resets.
     zero_memory_on_reset: bool,
     /// Whether a vp can be started or reset by a lower vtl.
@@ -654,12 +652,23 @@ struct UhVpInner {
     sidecar_exit_reason: Mutex<Option<SidecarExitReason>>,
 }
 
+#[cfg_attr(not(guest_arch = "x86_64"), allow(dead_code))]
+#[derive(Debug, Inspect)]
+/// Which operation is setting the initial vp context
+pub enum InitialVpContextOperation {
+    /// The VP is being started via the StartVp hypercall.
+    StartVp,
+    /// The VP is being started via the EnableVpVtl hypercall.
+    EnableVpVtl,
+}
+
 #[cfg_attr(not(guest_arch = "x86_64"), allow(dead_code))]
 #[derive(Debug, Inspect)]
 /// State for handling StartVp/EnableVpVtl hypercalls.
 pub struct VpStartEnableVtl {
-    /// True if the context is for startvp, false for enablevpvtl
-    is_start: bool,
+    /// Which operation, startvp or enablevpvtl, is setting the initial vp
+    /// context
+    operation: InitialVpContextOperation,
     #[inspect(skip)]
     context: hvdef::hypercall::InitialVpContextX64,
 }
@@ -1872,9 +1881,10 @@ impl UhProtoPartition<'_> {
     ) -> Result<UhCvmPartitionState, Error> {
         let vp_count = params.topology.vp_count() as usize;
         let vps = (0..vp_count)
-            .map(|_vp_index| UhCvmVpInner {
+            .map(|vp_index| UhCvmVpInner {
                 tlb_lock_info: VtlArray::from_fn(|_| TlbLockInfo::new(vp_count)),
                 vtl1_enabled: Mutex::new(false),
+                started: AtomicBool::new(vp_index == 0),
             })
             .collect();
         let tlb_locked_vps =
diff --git a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs
@@ -15,6 +15,7 @@ use crate::validate_vtl_gpa_flags;
 use crate::GuestVsmState;
 use crate::GuestVsmVtl1State;
 use crate::GuestVtl;
+use crate::InitialVpContextOperation;
 use crate::TlbFlushLockAccess;
 use crate::VpStartEnableVtl;
 use crate::WakeReason;
@@ -936,12 +937,12 @@ impl<T, B: HardwareIsolatedBacking>
         }
 
         let target_vtl = self.target_vtl_no_higher(target_vtl)?;
-        let target_vp = &self.vp.partition.vps[target_vp as usize];
+        let target_vp_inner = self.vp.cvm_partition().vp_inner(target_vp);
 
         // The target VTL must have been enabled. In addition, if lower VTL
         // startup has been suppressed, then the request must be coming from a
         // secure VTL.
-        if target_vtl == GuestVtl::Vtl1 && !target_vp.hcvm_vtl1_state.lock().enabled {
+        if target_vtl == GuestVtl::Vtl1 && !*target_vp_inner.vtl1_enabled.lock() {
             return Err(HvError::InvalidVpState);
         }
 
@@ -952,7 +953,7 @@ impl<T, B: HardwareIsolatedBacking>
                 .guest_vsm
                 .read()
                 .get_hardware_cvm()
-                .is_some_and(|inner| inner.deny_lower_vtl_startup)
+                .is_some_and(|state| state.deny_lower_vtl_startup)
         {
             return Err(HvError::AccessDenied);
         }
@@ -966,29 +967,27 @@ impl<T, B: HardwareIsolatedBacking>
         // becomes a problem. Note that this will not apply to non-hardware cvms
         // as this may regress existing VMs.
 
-        let mut target_vp_vtl1_state = target_vp.hcvm_vtl1_state.lock();
-        if self
-            .vp
-            .partition
-            .guest_vsm
-            .read()
-            .get_hardware_cvm()
-            .is_some()
-            && target_vp_vtl1_state.started
+        // After this check, there can be no more failures, so try setting the
+        // fact that the VM started to true here.
+        if target_vp_inner
+            .started
+            .compare_exchange(
+                false,
+                true,
+                std::sync::atomic::Ordering::Relaxed,
+                std::sync::atomic::Ordering::Relaxed,
+            )
+            .is_err()
         {
             return Err(HvError::InvalidVpState);
         }
 
-        // There can be no more errors from here, so just set started to
-        // true while holding the lock
-
-        target_vp_vtl1_state.started = true;
-
         let start_state = VpStartEnableVtl {
-            is_start: true,
+            operation: InitialVpContextOperation::StartVp,
             context: *vp_context,
         };
 
+        let target_vp = &self.vp.partition.vps[target_vp as usize];
         *target_vp.hv_start_enable_vtl_vp[target_vtl].lock() = Some(Box::new(start_state));
         target_vp.wake(target_vtl, WakeReason::HV_START_ENABLE_VP_VTL);
 
@@ -1067,6 +1066,12 @@ impl<T, B: HardwareIsolatedBacking>
         vtl: Vtl,
         vp_context: &hvdef::hypercall::InitialVpContextX64,
     ) -> HvResult<()> {
+        tracing::debug!(
+            vp_index = self.vp.vp_index().index(),
+            target_vp = vp_index,
+            ?vtl,
+            "HvEnableVpVtl"
+        );
         if partition_id != hvdef::HV_PARTITION_ID_SELF {
             return Err(HvError::InvalidPartitionId);
         }
@@ -1097,7 +1102,7 @@ impl<T, B: HardwareIsolatedBacking>
             // the higher VTL has not been enabled on any other VP because at that
             // point, the higher VTL should be orchestrating its own enablement.
             if self.intercepted_vtl < GuestVtl::Vtl1 {
-                if vtl1_state.enabled_on_vp_count > 0 || vp_index != current_vp_index {
+                if vtl1_state.enabled_on_any_vp || vp_index != current_vp_index {
                     return Err(HvError::AccessDenied);
                 }
 
@@ -1106,7 +1111,7 @@ impl<T, B: HardwareIsolatedBacking>
                 // If handling on behalf of VTL 1, then some other VP (i.e. the
                 // bsp) must have already handled EnableVpVtl. No partition-wide
                 // state is changing, so no need to hold the lock
-                assert!(vtl1_state.enabled_on_vp_count > 0);
+                assert!(vtl1_state.enabled_on_any_vp);
                 None
             }
         };
@@ -1120,7 +1125,7 @@ impl<T, B: HardwareIsolatedBacking>
             .vtl1_enabled
             .lock();
 
-        if inner_vtl1_state.enabled {
+        if *vtl1_enabled {
             return Err(HvError::VtlAlreadyEnabled);
         }
 
@@ -1153,13 +1158,16 @@ impl<T, B: HardwareIsolatedBacking>
 
         // Cannot fail from here
         if let Some(mut gvsm) = gvsm_state {
-            gvsm.get_hardware_cvm_mut().unwrap().enabled_on_vp_count += 1;
+            // It's valid to only set this when gvsm_state is Some (when VTL 0
+            // was intercepted) only because we assert above that if VTL 1 was
+            // intercepted, some vp has already enabled VTL 1 on it.
+            gvsm.get_hardware_cvm_mut().unwrap().enabled_on_any_vp = true;
         }
 
-        inner_vtl1_state.enabled = true;
+        *vtl1_enabled = true;
 
         let enable_vp_vtl_state = VpStartEnableVtl {
-            is_start: false,
+            operation: InitialVpContextOperation::EnableVpVtl,
             context: *vp_context,
         };
 
@@ -1493,7 +1501,8 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
             tracing::debug!(
                 vp_index = self.inner.cpu_index,
                 ?vtl,
-                "starting vp with initial registers"
+                ?start_enable_vtl_state.operation,
+                "setting up vp with initial registers"
             );
 
             hv1_emulator::hypercall::set_x86_vp_context(
@@ -1502,10 +1511,13 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
             )
             .map_err(UhRunVpError::State)?;
 
-            if start_enable_vtl_state.is_start {
+            if matches!(
+                start_enable_vtl_state.operation,
+                InitialVpContextOperation::StartVp
+            ) {
                 match vtl {
                     GuestVtl::Vtl0 => {
-                        if self.inner.hcvm_vtl1_state.lock().enabled {
+                        if *self.cvm_vp_inner().vtl1_enabled.lock() {
                             // When starting a VP targeting VTL on a
                             // hardware confidential VM, if VTL 1 has been
                             // enabled, switch to it (the highest enabled
@@ -1516,20 +1528,17 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
                             // a second+ startvp call for a vp should be
                             // revisited.
                             //
-                            // For other VM types, the hypervisor is
-                            // responsible for running the correct VTL.
-                            //
                             // Furthermore, there is no need to copy the
                             // shared VTL registers if starting the VP on an
                             // already running VP is disallowed. Even if
                             // this was allowed, copying the registers may
                             // not be desirable.
 
-                            B::set_exit_vtl(self, GuestVtl::Vtl1);
+                            self.backing.cvm_state_mut().exit_vtl = GuestVtl::Vtl1;
                         }
                     }
                     GuestVtl::Vtl1 => {
-                        B::set_exit_vtl(self, GuestVtl::Vtl1);
+                        self.backing.cvm_state_mut().exit_vtl = GuestVtl::Vtl1;
                     }
                 }
             }
@@ -1571,6 +1580,11 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
         target_vtl: GuestVtl,
         config: HvRegisterVsmVpSecureVtlConfig,
     ) -> Result<(), HvError> {
+        tracing::debug!(
+            ?requesting_vtl,
+            ?target_vtl,
+            "setting vsm vp secure config vtl"
+        );
         if requesting_vtl <= target_vtl {
             return Err(HvError::AccessDenied);
         }
diff --git a/openhcl/virt_mshv_vtl/src/processor/mod.rs b/openhcl/virt_mshv_vtl/src/processor/mod.rs
@@ -36,7 +36,6 @@ use super::UhPartitionInner;
 use super::UhVpInner;
 use crate::GuestVsmState;
 use crate::GuestVtl;
-// use crate::UhVpCvmVtl1State;
 use crate::WakeReason;
 use hcl::ioctl;
 use hcl::ioctl::ProcessorRunner;
@@ -241,17 +240,6 @@ mod private {
         /// This is used for hypervisor-managed and untrusted SINTs.
         fn request_untrusted_sint_readiness(this: &mut UhProcessor<'_, Self>, sints: u16);
 
-        // /// Copies shared registers (per VSM TLFS spec) from the source VTL to
-        // /// the target VTL that will become active, and marks the target_vtl as
-        // /// the exit vtl.
-        // fn switch_vtl(
-        //     _this: &mut UhProcessor<'_, Self>,
-        //     _source_vtl: GuestVtl,
-        //     _target_vtl: GuestVtl,
-        // ) {
-        //     unimplemented!("switch_vtl not implemented");
-        // }
-
         /// Checks interrupt status for all VTLs, and handles cross VTL interrupt preemption and VINA.
         /// Returns whether interrupt reprocessing is required.
         fn handle_cross_vtl_interrupts(
@@ -303,10 +291,6 @@ pub trait HardwareIsolatedBacking: Backing {
         this: &UhProcessor<'_, Self>,
         vtl: GuestVtl,
     ) -> TranslationRegisters;
-    /// Sets the exit vtl
-    fn set_exit_vtl(this: &mut UhProcessor<'_, Self>, vtl: GuestVtl) {
-        this.backing.cvm_state_mut().exit_vtl = vtl;
-    }
 }
 
 #[cfg_attr(guest_arch = "aarch64", allow(dead_code))]
@@ -364,10 +348,6 @@ impl UhVpInner {
             waker: Default::default(),
             cpu_index,
             vp_info,
-            // hcvm_vtl1_state: Mutex::new(UhVpCvmVtl1State {
-            //     enabled: false,
-            //     started: cpu_index == 0,
-            // }),
             hv_start_enable_vtl_vp: VtlArray::from_fn(|_| Mutex::new(None)),
             sidecar_exit_reason: Default::default(),
         }
diff --git a/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs b/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs
@@ -196,10 +196,6 @@ impl BackingPrivate for HypervisorBackedArm64 {
             .set_sints(this.backing.next_deliverability_notifications.sints() | sints);
     }
 
-    fn set_exit_vtl(_this: &mut UhProcessor<'_, Self>, _vtl: GuestVtl) {
-        unimplemented!();
-    }
-
     fn handle_cross_vtl_interrupts(
         _this: &mut UhProcessor<'_, Self>,
         _dev: &impl CpuIo,
diff --git a/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs b/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs
@@ -514,9 +514,6 @@ impl BackingPrivate for SnpBacked {
     fn vtl1_inspectable(this: &UhProcessor<'_, Self>) -> bool {
         this.hcvm_vtl1_inspectable()
     }
-    // fn set_exit_vtl(this: &mut UhProcessor<'_, Self>, vtl: GuestVtl) {
-    //     this.backing.cvm_state_mut().exit_vtl = vtl;
-    // }
 }
 
 fn virt_seg_to_snp(val: SegmentRegister) -> SevSelector {
@@ -2201,39 +2198,6 @@ impl<T: CpuIo> hv1_hypercall::EnablePartitionVtl for UhHypercallHandler<'_, '_,
     }
 }
 
-impl<T: CpuIo> hv1_hypercall::EnableVpVtl<hvdef::hypercall::InitialVpContextX64>
-    for UhHypercallHandler<'_, '_, T, SnpBacked>
-{
-    fn enable_vp_vtl(
-        &mut self,
-        partition_id: u64,
-        vp_index: u32,
-        vtl: Vtl,
-        vp_context: &hvdef::hypercall::InitialVpContextX64,
-    ) -> hvdef::HvResult<()> {
-        self.hcvm_enable_vp_vtl(partition_id, vp_index, vtl, vp_context)
-    }
-}
-
-impl<T: CpuIo> hv1_hypercall::RetargetDeviceInterrupt for UhHypercallHandler<'_, '_, T, SnpBacked> {
-    fn retarget_interrupt(
-        &mut self,
-        device_id: u64,
-        address: u64,
-        data: u32,
-        params: &hv1_hypercall::HvInterruptParameters<'_>,
-    ) -> hvdef::HvResult<()> {
-        self.hcvm_retarget_interrupt(
-            device_id,
-            address,
-            data,
-            params.vector,
-            params.multicast,
-            params.target_processors,
-        )
-    }
-}
-
 impl<T: CpuIo> hv1_hypercall::VtlSwitchOps for UhHypercallHandler<'_, '_, T, SnpBacked> {
     fn advance_ip(&mut self) {
         let is_64bit = self.vp.long_mode(self.intercepted_vtl);
diff --git a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs
