fix(gdb): remove wrong ordering and double locking

With the vcpu refactoring gdb support got broken. While we have a test
to verify it builds, we still don't have one to verify it works.
In particular, the gdb thread was created too early, and the vmm was
locked twice leading to a deadlock.

This change fixes both issues and adds a little bit more logging for the
errors in GDB which were helpful to debug the issue.

Signed-off-by: Riccardo Mancini <[email protected]>

Author: Manciukic

src/vmm/src/builder.rs

@@ -115,7 +115,7 @@ pub enum StartMicrovmError {
     CreateEntropyDevice(crate::devices::virtio::rng::EntropyError),
     /// Failed to allocate guest resource: {0}
     AllocateResources(#[from] vm_allocator::Error),
-    /// Error starting GDB debug session
+    /// Error starting GDB debug session: {0}
     #[cfg(feature = "gdb")]
     GdbServer(gdb::target::GdbTargetError),
     /// Error cloning Vcpu fds
@@ -292,19 +292,12 @@ pub fn build_microvm_for_boot(
     let vmm = Arc::new(Mutex::new(vmm));
 
     #[cfg(feature = "gdb")]
-    {
-        let (gdb_tx, gdb_rx) = mpsc::channel();
-        vcpus
-            .iter_mut()
-            .for_each(|vcpu| vcpu.attach_debug_info(gdb_tx.clone()));
-
-        if let Some(gdb_socket_path) = &vm_resources.machine_config.gdb_socket_path {
-            gdb::gdb_thread(vmm.clone(), gdb_rx, entry_point.entry_addr, gdb_socket_path)
-                .map_err(StartMicrovmError::GdbServer)?;
-        } else {
-            debug!("No GDB socket provided not starting gdb server.");
-        }
-    }
+    let (gdb_tx, gdb_rx) = mpsc::channel();
+
+    #[cfg(feature = "gdb")]
+    vcpus
+        .iter_mut()
+        .for_each(|vcpu| vcpu.attach_debug_info(gdb_tx.clone()));
 
     // Move vcpus to their own threads and start their state machine in the 'Paused' state.
     vmm.lock()
@@ -318,6 +311,14 @@ pub fn build_microvm_for_boot(
         )
         .map_err(VmmError::VcpuStart)?;
 
+    #[cfg(feature = "gdb")]
+    if let Some(gdb_socket_path) = &vm_resources.machine_config.gdb_socket_path {
+        gdb::gdb_thread(vmm.clone(), gdb_rx, entry_point.entry_addr, gdb_socket_path)
+            .map_err(StartMicrovmError::GdbServer)?;
+    } else {
+        debug!("No GDB socket provided not starting gdb server.");
+    }
+
     // Load seccomp filters for the VMM thread.
     // Execution panics if filters cannot be loaded, use --no-seccomp if skipping filters
     // altogether is the desired behaviour.

src/vmm/src/gdb/mod.rs

@@ -51,11 +51,11 @@ pub fn gdb_thread(
     }
 
     let path = Path::new(socket_addr);
-    let listener = UnixListener::bind(path).map_err(|_| GdbTargetError::ServerSocketError)?;
+    let listener = UnixListener::bind(path).map_err(GdbTargetError::ServerSocketError)?;
     trace!("Waiting for GDB server connection on {}...", path.display());
     let (connection, _addr) = listener
         .accept()
-        .map_err(|_| GdbTargetError::ServerSocketError)?;
+        .map_err(GdbTargetError::ServerSocketError)?;
 
     std::thread::Builder::new()
         .name("gdb".into())

src/vmm/src/gdb/target.rs

@@ -65,8 +65,8 @@ pub enum GdbTargetError {
     NoPausedVcpu,
     /// Error when setting Vcpu debug flags
     VcpuKvmError,
-    /// Server socket Error
-    ServerSocketError,
+    /// Server socket Error: {0}
+    ServerSocketError(std::io::Error),
     /// Error with creating GDB thread
     GdbThreadError,
     /// VMM locking error
@@ -394,8 +394,6 @@ impl MultiThreadBase for FirecrackerTarget {
         let vcpu_idx = tid_to_vcpuid(tid);
         let vcpu_fd = &vmm.vcpus_handles[vcpu_idx].vcpu_fd;
 
-        let vmm = &self.vmm.lock().expect("Error locking vmm in read addr");
-
         while !data.is_empty() {
             let gpa = arch::translate_gva(vcpu_fd, gva, &vmm).map_err(|e| {
                 error!("Error {e:?} translating gva on read address: {gva:#X}");