From 93fcc403f557c42147bfd79519ad349d4cfc9727 Mon Sep 17 00:00:00 2001
From: Rob Bradford <robert.bradford@intel.com>
Date: Mon, 25 Jul 2022 12:34:46 +0100
Subject: [PATCH] vmm: Make gdb break/resuming more resilient

When starting the VM such that it is already on a breakpoint (via
stop_on_boot) when attached to gdb then start the vCPUs in a paused
state rather than starting the vCPUs later (upon resume).

Further, make the resumption/break of the VM more resilient by only
attemping to resume the vCPUs if were are already in a break point and
only attempting to pause/break if we were already running.

Fixes: #4354

Signed-off-by: Rob Bradford <robert.bradford@intel.com>
---
 vmm/src/cpu.rs | 40 ++++++++++++++++++++--------------------
 vmm/src/vm.rs  | 36 ++++++++++--------------------------
 2 files changed, 30 insertions(+), 46 deletions(-)

diff --git a/vmm/src/cpu.rs b/vmm/src/cpu.rs
index 5b84b50cf9..a6ae0f4eac 100644
--- a/vmm/src/cpu.rs
+++ b/vmm/src/cpu.rs
@@ -1048,7 +1048,12 @@ impl CpuManager {
     }
 
     /// Start up as many vCPUs threads as needed to reach `desired_vcpus`
-    fn activate_vcpus(&mut self, desired_vcpus: u8, inserting: bool) -> Result<()> {
+    fn activate_vcpus(
+        &mut self,
+        desired_vcpus: u8,
+        inserting: bool,
+        paused: Option<bool>,
+    ) -> Result<()> {
         if desired_vcpus > self.config.max_vcpus {
             return Err(Error::DesiredVCpuCountExceedsMax);
         }
@@ -1057,11 +1062,16 @@ impl CpuManager {
             (desired_vcpus - self.present_vcpus() + 1) as usize,
         ));
 
+        if let Some(paused) = paused {
+            self.vcpus_pause_signalled.store(paused, Ordering::SeqCst);
+        }
+
         info!(
-            "Starting vCPUs: desired = {}, allocated = {}, present = {}",
+            "Starting vCPUs: desired = {}, allocated = {}, present = {}, paused = {}",
             desired_vcpus,
             self.vcpus.len(),
-            self.present_vcpus()
+            self.present_vcpus(),
+            self.vcpus_pause_signalled.load(Ordering::SeqCst)
         );
 
         // This reuses any inactive vCPUs as well as any that were newly created
@@ -1101,26 +1111,16 @@ impl CpuManager {
     }
 
     // Starts all the vCPUs that the VM is booting with. Blocks until all vCPUs are running.
-    pub fn start_boot_vcpus(&mut self) -> Result<()> {
-        self.activate_vcpus(self.boot_vcpus(), false)
+    pub fn start_boot_vcpus(&mut self, paused: bool) -> Result<()> {
+        self.activate_vcpus(self.boot_vcpus(), false, Some(paused))
     }
 
     pub fn start_restored_vcpus(&mut self) -> Result<()> {
-        let vcpu_numbers = self.vcpus.len() as u8;
-        let vcpu_thread_barrier = Arc::new(Barrier::new((vcpu_numbers + 1) as usize));
-        // Restore the vCPUs in "paused" state.
-        self.vcpus_pause_signalled.store(true, Ordering::SeqCst);
-
-        for vcpu_id in 0..vcpu_numbers {
-            let vcpu = Arc::clone(&self.vcpus[vcpu_id as usize]);
+        self.activate_vcpus(self.vcpus.len() as u8, false, Some(true))
+            .map_err(|e| {
+                Error::StartRestoreVcpu(anyhow!("Failed to start restored vCPUs: {:#?}", e))
+            })?;
 
-            self.start_vcpu(vcpu, vcpu_id, vcpu_thread_barrier.clone(), false)
-                .map_err(|e| {
-                    Error::StartRestoreVcpu(anyhow!("Failed to start restored vCPUs: {:#?}", e))
-                })?;
-        }
-        // Unblock all restored CPU threads.
-        vcpu_thread_barrier.wait();
         Ok(())
     }
 
@@ -1136,7 +1136,7 @@ impl CpuManager {
         match desired_vcpus.cmp(&self.present_vcpus()) {
             cmp::Ordering::Greater => {
                 self.create_vcpus(desired_vcpus, None)?;
-                self.activate_vcpus(desired_vcpus, true)?;
+                self.activate_vcpus(desired_vcpus, true, None)?;
                 Ok(true)
             }
             cmp::Ordering::Less => {
diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs
index af6881ff0c..9dec1cdeef 100644
--- a/vmm/src/vm.rs
+++ b/vmm/src/vm.rs
@@ -2143,13 +2143,11 @@ impl Vm {
             self.vm.tdx_finalize().map_err(Error::FinalizeTdx)?;
         }
 
-        if new_state == VmState::Running {
-            self.cpu_manager
-                .lock()
-                .unwrap()
-                .start_boot_vcpus()
-                .map_err(Error::CpuManager)?;
-        }
+        self.cpu_manager
+            .lock()
+            .unwrap()
+            .start_boot_vcpus(new_state == VmState::BreakPoint)
+            .map_err(Error::CpuManager)?;
 
         let mut state = self.state.try_write().map_err(|_| Error::PoisonedState)?;
         *state = new_state;
@@ -2885,9 +2883,10 @@ impl Debuggable for Vm {
     }
 
     fn debug_pause(&mut self) -> std::result::Result<(), DebuggableError> {
-        if !self.cpu_manager.lock().unwrap().vcpus_paused() {
+        if *self.state.read().unwrap() == VmState::Running {
             self.pause().map_err(DebuggableError::Pause)?;
         }
+
         let mut state = self
             .state
             .try_write()
@@ -2897,25 +2896,10 @@ impl Debuggable for Vm {
     }
 
     fn debug_resume(&mut self) -> std::result::Result<(), DebuggableError> {
-        if !self.cpu_manager.lock().unwrap().vcpus_paused() {
-            self.cpu_manager
-                .lock()
-                .unwrap()
-                .start_boot_vcpus()
-                .map_err(|e| {
-                    DebuggableError::Resume(MigratableError::Resume(anyhow!(
-                        "Could not start boot vCPUs: {:?}",
-                        e
-                    )))
-                })?;
-        } else {
-            self.resume().map_err(DebuggableError::Resume)?;
+        if *self.state.read().unwrap() == VmState::BreakPoint {
+            self.resume().map_err(DebuggableError::Pause)?;
         }
-        let mut state = self
-            .state
-            .try_write()
-            .map_err(|_| DebuggableError::PoisonedState)?;
-        *state = VmState::Running;
+
         Ok(())
     }