Use Gdb results and store vcpus in vec

Updated to bubble up gdb -> vcpu errors back to the server to allow for
easier debugging. Updated to store vcpus in a vec rather than a map

Signed-off-by: Jack Thomson <[email protected]>

Author: JackThomson2

src/vmm/src/builder.rs

@@ -132,6 +132,9 @@ pub enum StartMicrovmError {
     /// Error configuring ACPI: {0}
     #[cfg(target_arch = "x86_64")]
     Acpi(#[from] crate::acpi::AcpiError),
+    /// Error starting GDB debug session
+    #[cfg(feature = "debug")]
+    GdbServer(gdb::target::Error),
 }
 
 /// It's convenient to automatically convert `linux_loader::cmdline::Error`s
@@ -313,8 +316,7 @@ pub fn build_microvm_for_boot(
     #[cfg(feature = "debug")]
     vcpus
         .iter_mut()
-        .enumerate()
-        .for_each(|(id, vcpu)| vcpu.attach_debug_info(gdb_tx.clone(), id));
+        .for_each(|vcpu| vcpu.attach_debug_info(gdb_tx.clone()));
 
     // The boot timer device needs to be the first device attached in order
     // to maintain the same MMIO address referenced in the documentation
@@ -366,7 +368,7 @@ pub fn build_microvm_for_boot(
     let vmm = Arc::new(Mutex::new(vmm));
 
     #[cfg(feature = "debug")]
-    gdb::server::gdb_thread(vmm.clone(), &vcpus, gdb_rx, entry_addr);
+    gdb::server::gdb_thread(vmm.clone(), &vcpus, gdb_rx, entry_addr).map_err(GdbServer)?;
 
     // Move vcpus to their own threads and start their state machine in the 'Paused' state.
     vmm.lock()

src/vmm/src/gdb/server.rs

@@ -1,7 +1,7 @@
 // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::os::unix::net::{UnixListener, UnixStream};
+use std::os::unix::net::UnixListener;
 use std::path::Path;
 use std::sync::mpsc::Receiver;
 use std::sync::{Arc, Mutex};
@@ -14,20 +14,13 @@ use kvm_ioctls::VcpuFd;
 use vm_memory::GuestAddress;
 
 use super::event_loop::event_loop;
-use crate::logger::{error, info};
+use super::target::Error;
+use crate::logger::info;
 use crate::{Vcpu, Vmm};
 
 const KVM_DEBUG_ENABLE: u64 = 0x0600;
 
-fn listen_for_connection(path: &std::path::Path) -> Result<UnixStream, Box<dyn std::error::Error>> {
-    let listener = UnixListener::bind(path)?;
-    info!("Waiting for GDB server connection on {}...", path.display());
-    let (stream, _addr) = listener.accept()?;
-
-    Ok(stream)
-}
-
-fn set_kvm_debug(control: u32, vcpu: &VcpuFd, addrs: &[GuestAddress]) {
+fn set_kvm_debug(control: u32, vcpu: &VcpuFd, addrs: &[GuestAddress]) -> Result<(), Error> {
     let mut dbg = kvm_guest_debug {
         control,
         ..Default::default()
@@ -37,17 +30,15 @@ fn set_kvm_debug(control: u32, vcpu: &VcpuFd, addrs: &[GuestAddress]) {
 
     for (i, addr) in addrs.iter().enumerate() {
         dbg.arch.debugreg[i] = addr.0;
-        // Set global breakpoint enable flag
+        // Set global breakpoint enable flag for the specific breakpoint number by setting the bit
         dbg.arch.debugreg[7] |= 2 << (i * 2);
     }
 
-    if vcpu.set_guest_debug(&dbg).is_err() {
-        error!("Error setting debug");
-    }
+    vcpu.set_guest_debug(&dbg).map_err(|_| Error::VcpuKvmError)
 }
 
-/// Configures the VCPU for
-pub fn kvm_debug(vcpu: &VcpuFd, addrs: &[GuestAddress], step: bool) {
+/// Configures the VCPU for debugging and sets the hardware breakpoints on the vcpu
+pub fn vcpu_set_debug(vcpu: &VcpuFd, addrs: &[GuestAddress], step: bool) -> Result<(), Error> {
     let mut control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP | KVM_GUESTDBG_USE_SW_BP;
     if step {
         control |= KVM_GUESTDBG_SINGLESTEP;
@@ -58,7 +49,7 @@ pub fn kvm_debug(vcpu: &VcpuFd, addrs: &[GuestAddress], step: bool) {
 
 /// Injects a BP back into the guest kernel for it to handle, this is particularly useful for the
 /// kernels selftesting which can happen during boot.
-pub fn kvm_inject_bp(vcpu: &VcpuFd, addrs: &[GuestAddress], step: bool) {
+pub fn vcpu_inject_bp(vcpu: &VcpuFd, addrs: &[GuestAddress], step: bool) -> Result<(), Error> {
     let mut control = KVM_GUESTDBG_ENABLE
         | KVM_GUESTDBG_USE_HW_BP
         | KVM_GUESTDBG_USE_SW_BP
@@ -87,20 +78,24 @@ pub fn gdb_thread(
     vcpus: &[Vcpu],
     gdb_event_receiver: Receiver<usize>,
     entry_addr: GuestAddress,
-) {
+) -> Result<(), Error> {
     // We register a hw breakpoint at the entry point as GDB expects the application
     // to be stopped as it connects. This also allows us to set breakpoints before kernel starts
-    kvm_debug(&vcpus[0].kvm_vcpu.fd, &[entry_addr], false);
+    vcpu_set_debug(&vcpus[0].kvm_vcpu.fd, &[entry_addr], false)?;
 
     for vcpu in vcpus.iter().skip(1) {
-        kvm_debug(&vcpu.kvm_vcpu.fd, &[], false);
+        vcpu_set_debug(&vcpu.kvm_vcpu.fd, &[], false)?;
     }
 
     let path = Path::new("/tmp/gdb.socket");
-    let connection = listen_for_connection(path).expect("Error waiting for connection");
+    let listener = UnixListener::bind(path).map_err(|_| Error::ServerSocketError)?;
+    info!("Waiting for GDB server connection on {}...", path.display());
+    let (connection, _addr) = listener.accept().map_err(|_| Error::ServerSocketError)?;
 
     std::thread::Builder::new()
         .name("gdb".into())
         .spawn(move || event_loop(connection, vmm, gdb_event_receiver, entry_addr))
-        .expect("Error spawning GDB thread");
+        .map_err(|_| Error::GdbThreadError)?;
+
+    Ok(())
 }

src/vmm/src/gdb/target.rs

@@ -18,10 +18,6 @@ use gdbstub::target::ext::breakpoints::{
 };
 use gdbstub::target::ext::thread_extra_info::{ThreadExtraInfo, ThreadExtraInfoOps};
 use gdbstub::target::{Target, TargetError, TargetResult};
-#[cfg(target_arch = "aarch64")]
-use gdbstub_arch::aarch64::reg::AArch64CoreRegs as CoreRegs;
-#[cfg(target_arch = "aarch64")]
-use gdbstub_arch::aarch64::AArch64 as GdbArch;
 #[cfg(target_arch = "x86_64")]
 use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs;
 #[cfg(target_arch = "x86_64")]
@@ -30,7 +26,8 @@ use kvm_bindings::kvm_regs;
 use vm_memory::{Bytes, GuestAddress};
 
 use crate::logger::{error, info};
-use crate::{FcExitCode, VcpuEvent, VcpuHandle, VcpuResponse, Vmm};
+use crate::vstate::vcpu::VcpuError;
+use crate::{arch, FcExitCode, VcpuEvent, VcpuHandle, VcpuResponse, Vmm};
 
 const X86_SW_BP_OP: u8 = 0xCC;
 
@@ -51,6 +48,12 @@ pub enum Error {
     VCPURequestError,
     /// No currently paused Vcpu error
     NoPausedVCpu,
+    /// Error when setting vcpu debug flags
+    VcpuKvmError,
+    /// Server socket Error
+    ServerSocketError,
+    /// Error with creating GDB thread
+    GdbThreadError,
 }
 
 impl<E> From<Error> for TargetError<E> {
@@ -72,7 +75,7 @@ pub struct FirecrackerTarget {
     hw_breakpoints: Vec<GuestAddress>,
     sw_breakpoints: HashMap<<GdbArch as Arch>::Usize, [u8; 1]>,
 
-    vcpu_state: HashMap<Tid, VCpuState>,
+    vcpu_state: Vec<VCpuState>,
 
     paused_vcpu: Option<Tid>,
 }
@@ -98,22 +101,19 @@ impl FirecrackerTarget {
     /// will handle requests from GDB and perform the appropriate actions, while also updating GDB
     /// with the state of the VMM / VCPU's as we hit debug events
     pub fn new(vmm: Arc<Mutex<Vmm>>, gdb_event: Receiver<usize>, entry_addr: GuestAddress) -> Self {
-        let mut vcpu_state = HashMap::new();
         let cpu_count = {
             vmm.lock()
                 .expect("Exception unlocking vmm")
                 .vcpus_handles
                 .len()
         };
 
-        for cpu_id in 0..cpu_count {
-            let new_state = VCpuState {
+        let vcpu_state = (0..cpu_count)
+            .map(|_| VCpuState {
                 paused: false,
                 single_step: false,
-            };
-
-            vcpu_state.insert(cpuid_to_tid(cpu_id), new_state);
-        }
+            })
+            .collect();
 
         Self {
             vmm,
@@ -138,24 +138,19 @@ impl FirecrackerTarget {
     /// This is used to notify the target that the provided Tid
     /// is in a paused state
     pub fn notify_paused_vcpu(&mut self, tid: Tid) {
-        let found = match self.vcpu_state.get_mut(&tid) {
-            Some(res) => res,
-            None => {
-                info!("TID {tid} was not known.");
-                return;
-            }
-        };
-
+        let found = &mut self.vcpu_state[tid_to_cpuid(tid)];
         found.paused = true;
+
         self.paused_vcpu = Some(tid);
     }
 
     fn resume_execution(&mut self) -> Result<(), Error> {
         let to_resume: Vec<Tid> = self
             .vcpu_state
             .iter()
-            .filter_map(|(tid, state)| match state.paused {
-                true => Some(*tid),
+            .enumerate()
+            .filter_map(|(cpu_id, state)| match state.paused {
+                true => Some(cpuid_to_tid(cpu_id)),
                 false => None,
             })
             .collect();
@@ -165,7 +160,7 @@ impl FirecrackerTarget {
             self.request_resume(tid)?;
         }
 
-        self.vcpu_state.iter_mut().for_each(|(_, state)| {
+        self.vcpu_state.iter_mut().for_each(|state| {
             state.paused = false;
         });
 
@@ -179,7 +174,7 @@ impl FirecrackerTarget {
     }
 
     fn reset_all_states(&mut self) {
-        for (_, value) in self.vcpu_state.iter_mut() {
+        for value in self.vcpu_state.iter_mut() {
             Self::reset_vcpu_state(value);
         }
     }
@@ -194,13 +189,7 @@ impl FirecrackerTarget {
 
     /// This method can be used to manually pause the requested vcpu
     pub fn request_pause(&mut self, tid: Tid) -> Result<(), Error> {
-        let vcpu_state = match self.vcpu_state.get_mut(&tid) {
-            Some(res) => res,
-            None => {
-                info!("Attempted to pause a vcpu we have no state for.");
-                return Err(Error::VCPURequestError);
-            }
-        };
+        let vcpu_state = &mut self.vcpu_state[tid_to_cpuid(tid)];
 
         if vcpu_state.paused {
             info!("Attempted to pause a vcpu already paused.");
@@ -268,13 +257,7 @@ impl FirecrackerTarget {
     /// Used to request the specified core to resume execution. The function
     /// will return early if the vcpu is already paused and not currently running
     pub fn request_resume(&mut self, tid: Tid) -> Result<(), Error> {
-        let vcpu_state = match self.vcpu_state.get_mut(&tid) {
-            Some(res) => res,
-            None => {
-                error!("Attempted to resume a vcpu we have no state for.");
-                return Err(Error::VCPURequestError);
-            }
-        };
+        let vcpu_state = &mut self.vcpu_state[tid_to_cpuid(tid)];
 
         if !vcpu_state.paused {
             info!("Attempted to resume a vcpu already running.");
@@ -288,6 +271,7 @@ impl FirecrackerTarget {
         cpu_handle
             .send_event(VcpuEvent::Resume)
             .expect("Error sending message to vcpu");
+
         let response = cpu_handle
             .response_receiver()
             .recv()
@@ -305,13 +289,7 @@ impl FirecrackerTarget {
         let cpu_handle =
             &self.vmm.lock().expect("error unlocking vmm").vcpus_handles[tid_to_cpuid(tid)];
 
-        let vcpu_state = match self.vcpu_state.get(&tid) {
-            Some(res) => res,
-            None => {
-                error!("Attempted to write kvm debug to a vcpu we have no state for.");
-                return Err(Error::VCPURequestError);
-            }
-        };
+        let vcpu_state = &self.vcpu_state[tid_to_cpuid(tid)];
 
         cpu_handle
             .send_event(VcpuEvent::SetKvmDebug(
@@ -325,6 +303,7 @@ impl FirecrackerTarget {
                 error!("Response resume : {message}");
                 Err(Error::VCPURequestError)
             }
+            Ok(VcpuResponse::Error(VcpuError::GdbRequest(e))) => Err(e),
             Err(_) => Err(Error::VCPURequestError),
             _ => Ok(()),
         }
@@ -340,7 +319,10 @@ impl FirecrackerTarget {
             .expect("Error recieving message from vcpu");
 
         if let VcpuResponse::GvaTranslation(response) = response {
-            return Ok(response);
+            return match response {
+                Some(res) => Ok(res.into()),
+                None => Err(Error::VCPURequestError),
+            };
         }
 
         if let VcpuResponse::NotAllowed(message) = response {
@@ -364,6 +346,7 @@ impl FirecrackerTarget {
                 error!("Response resume : {message}");
                 Err(Error::VCPURequestError)
             }
+            Ok(VcpuResponse::Error(VcpuError::GdbRequest(e))) => Err(e),
             Err(_) => Err(Error::VCPURequestError),
             _ => Ok(()),
         }
@@ -387,13 +370,7 @@ impl FirecrackerTarget {
         }
 
         let cpu_regs = self.get_regs(tid);
-
-        let vcpu_state = match self.vcpu_state.get(&tid) {
-            Some(res) => res,
-            None => {
-                return None;
-            }
-        };
+        let vcpu_state = &mut self.vcpu_state[tid_to_cpuid(tid)];
 
         if vcpu_state.single_step {
             return Some(MultiThreadStopReason::SignalWithThread {
@@ -539,7 +516,7 @@ impl MultiThreadBase for FirecrackerTarget {
                     }
                 };
 
-            let psize = 4096;
+            let psize = arch::PAGE_SIZE;
             let read_len = std::cmp::min(len - total_read, psize - (paddr & (psize - 1)));
 
             if memory
@@ -629,13 +606,7 @@ impl MultiThreadResume for FirecrackerTarget {
         tid: Tid,
         _signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
-        let vcpu_state = match self.vcpu_state.get_mut(&tid) {
-            Some(res) => res,
-            None => {
-                error!("Attempted to set action on a vcpu we have no state for.");
-                return Err(Error::VCPURequestError);
-            }
-        };
+        let vcpu_state = &mut self.vcpu_state[tid_to_cpuid(tid)];
         vcpu_state.single_step = false;
 
         Ok(())
@@ -663,13 +634,7 @@ impl MultiThreadSingleStep for FirecrackerTarget {
         tid: Tid,
         _signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
-        let vcpu_state = match self.vcpu_state.get_mut(&tid) {
-            Some(res) => res,
-            None => {
-                info!("Attempted to set action on a vcpu we have no state for.");
-                return Ok(());
-            }
-        };
+        let vcpu_state = &mut self.vcpu_state[tid_to_cpuid(tid)];
         vcpu_state.single_step = true;
 
         Ok(())