feat: Enable gdb debugging on x86

[JackThomson2]

This PR is adding GDB support into Firecracker. This is will allow us to connect GDB to the guest kernel and enable step through debugging.

This is achieved by leveraging https://github.com/daniel5151/gdbstub which allows us to debug the kernel over the GDB Remote Serial Protocol. The way this is managed is through a new GDB thread which is used to manage the interaction between the VCPUs and the GDB server.

A list of the current features supported in x86

• Multithreaded Debugging (multi core VM's)
• Register Read/Write
• Guest Memory Read/Write
• Software Breakpoints Add/Remove
• Hardware Breakpoints Add/Remove
• Single Step
• Continue

The implementation is feature gated behind the debug flag which aims to allow us to remove all the dependencies at compiletime

Changes

• Adding GDB support for guest kernel debugging
• Added new debug feature to firecracker which when enabled will start the GDB server and block the boot until GDB connects and resumes the execution

Reason

• Allow us to debug the guest kernel with GDB

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

• If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this
  PR.
• API changes follow the Runbook for Firecracker API changes.
• User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.
• New TODOs link to an issue.
• Commits meet
  contribution quality standards.

---

• This functionality cannot be added in rust-vmm.

[codecov]

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.37%. Comparing base (7803c42) to head (4a16698).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/vmm/src/builder.rs	72.41%	8 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4797   +/-   ##
=======================================
  Coverage   84.36%   84.37%           
=======================================
  Files         249      249           
  Lines       27501    27524   +23     
=======================================
+ Hits        23202    23222   +20     
- Misses       4299     4302    +3     

Flag	Coverage Δ
5.10-c5n.metal	84.58% <78.37%> (-0.03%)	⬇️
5.10-m5n.metal	84.56% <78.37%> (-0.03%)	⬇️
5.10-m6a.metal	83.85% <78.37%> (-0.03%)	⬇️
5.10-m6g.metal	80.98% <78.37%> (+0.04%)	⬆️
5.10-m6i.metal	84.55% <78.37%> (-0.04%)	⬇️
5.10-m7g.metal	80.98% <78.37%> (+0.04%)	⬆️
6.1-c5n.metal	84.58% <78.37%> (-0.03%)	⬇️
6.1-m5n.metal	84.56% <78.37%> (-0.04%)	⬇️
6.1-m6a.metal	83.85% <78.37%> (-0.03%)	⬇️
6.1-m6g.metal	80.97% <78.37%> (+0.04%)	⬆️
6.1-m6i.metal	84.55% <78.37%> (-0.03%)	⬇️
6.1-m7g.metal	80.98% <78.37%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ShadowCurse]

note for self: We need to replace this with displaydoc at some point.

[ShadowCurse]

Can we use id from vcpu itself instead of 0..cpu_count range?

[ShadowCurse]

Please squash 1 and 2 commits.

[ShadowCurse]

Shouldn't this be named select_paused_vcpu or smth simmilar? The notify part is confusing.

[ShadowCurse]

s/is already paused/is already running/

[ShadowCurse]

Any particular reason this function does't take tid: Tid instead of cpu_handle?

[ShadowCurse]

What would be a reason for tid to be out of range?

[ShadowCurse]

Maybe implement Default::default (or new method) for VCpuState.

[ShadowCurse]

This should either be configurable (maybe with config file only?) or it should be relative to the firecracker cwd. Otherwise who knows is /tmp/gdb.socket is already taken by smth. Or if you need to debug 2 firecracker processes at the same time.

[ShadowCurse]

We should add instructions for ./tools/devtool as well.

[ShadowCurse]

I assume vmlinux is the kernel blob?

[ShadowCurse]

Maybe worth adding a section about using exit to stop gdb and firecracker?

[roypat]

I've had a very haphazard look at it. Mostly it's just nits about Rust style things. The one thing I'd like to discuss is the passing of Tids all over the place. I outlined in a comment how we might be able to avoid this. Could you have a look at that? :o

[roypat]

Or just 0?

[roypat]

Can we just use an ArrayVec here (https://docs.rs/arrayvec/latest/arrayvec/struct.ArrayVec.html)? It'd be a new dependency, but I'd probably be in favor of adding it over implementing our own data structures.

[roypat]

(well, you could solve it with a closure, but that's a horrible API):

diff --git a/src/vmm/Cargo.toml b/src/vmm/Cargo.toml
index 6a6b05c91..50f0fd40d 100644
--- a/src/vmm/Cargo.toml
+++ b/src/vmm/Cargo.toml
@@ -58,7 +58,7 @@ itertools = "0.13.0"
 proptest = { version = "1.5.0", default-features = false, features = ["std"] }
 
 [features]
-default = []
+default = ["debug"]
 tracing = ["log-instrument"]
 debug = ["gdbstub", "gdbstub_arch"]
 
diff --git a/src/vmm/src/gdb/event_loop.rs b/src/vmm/src/gdb/event_loop.rs
index 6d584dcea..f55884e09 100644
--- a/src/vmm/src/gdb/event_loop.rs
+++ b/src/vmm/src/gdb/event_loop.rs
@@ -65,15 +65,17 @@ impl run_blocking::BlockingEventLoop for GdbBlockingEventLoop {
                         .map_err(WaitForStopReasonError::Target)?;
 
                     let Some(stop_response) = stop_reason else {
-                        // If we returned None this is a break which should be handled by
-                        // the guest kernel (e.g. kernel int3 self testing) so we won't notify
-                        // GDB and instead inject this back into the guest
-                        target
-                            .inject_bp_to_guest(tid)
-                            .map_err(WaitForStopReasonError::Target)?;
-                        target
-                            .request_resume(tid)
-                            .map_err(WaitForStopReasonError::Target)?;
+                        target.with_vcpu_handle(tid, |vcpu_handle| {
+                            // If we returned None this is a break which should be handled by
+                            // the guest kernel (e.g. kernel int3 self testing) so we won't notify
+                            // GDB and instead inject this back into the guest
+                            vcpu_handle
+                                .inject_bp_to_guest(target.hw_breakpoints.active_breakpoint())
+                                .map_err(WaitForStopReasonError::Target)?;
+                            vcpu_handle
+                                .request_resume()
+                                .map_err(WaitForStopReasonError::Target)
+                        })?;
 
                         trace!("Injected BP into guest early exit");
                         continue;
@@ -102,13 +104,13 @@ impl run_blocking::BlockingEventLoop for GdbBlockingEventLoop {
         target: &mut FirecrackerTarget,
     ) -> Result<Option<MultiThreadStopReason<u64>>, <FirecrackerTarget as Target>::Error> {
         // notify the target that a ctrl-c interrupt has occurred.
-        let main_core = vcpuid_to_tid(0)?;
+        let tid = vcpuid_to_tid(0)?;
+        target.with_vcpu_handle(tid, |handle| handle.request_pause())?;
 
-        target.request_pause(main_core)?;
-        target.update_paused_vcpu(main_core);
+        target.update_paused_vcpu(tid);
 
         let exit_reason = MultiThreadStopReason::SignalWithThread {
-            tid: main_core,
+            tid,
             signal: Signal::SIGINT,
         };
         Ok(Some(exit_reason))
diff --git a/src/vmm/src/gdb/target.rs b/src/vmm/src/gdb/target.rs
index f7cf88814..cde666731 100644
--- a/src/vmm/src/gdb/target.rs
+++ b/src/vmm/src/gdb/target.rs
@@ -28,25 +28,11 @@ use vm_memory::{Bytes, GuestAddress};
 use super::hw_breakpoint_store::HwBpStore;
 use crate::logger::{error, info};
 use crate::vstate::vcpu::{VcpuError, VcpuSendEventError};
-use crate::{arch, FcExitCode, VcpuEvent, VcpuResponse, Vmm};
+use crate::{arch, FcExitCode, VcpuEvent, VcpuHandle, VcpuResponse, Vmm};
 
 #[cfg(target_arch = "x86_64")]
 const X86_SW_BP_OP: u8 = 0xCC;
 
-#[derive(Default, Debug, Clone)]
-/// Stores the current state of a Vcpu
-struct VcpuState {
-    single_step: bool,
-    paused: bool,
-}
-
-impl VcpuState {
-    /// Disables single stepping on the Vcpu state
-    fn reset_vcpu_state(&mut self) {
-        self.single_step = false;
-    }
-}
-
 /// Errors from interactions between GDB and the VMM
 #[derive(Debug)]
 pub enum Error {
@@ -116,13 +102,12 @@ pub struct FirecrackerTarget {
     /// Used to track the currently configured hardware breakpoints.
     /// Limited to 4 in x86 see:
     /// https://elixir.bootlin.com/linux/v6.1/source/arch/x86/include/asm/kvm_host.h#L210
-    hw_breakpoints: HwBpStore<4>,
+    pub hw_breakpoints: HwBpStore<4>,
     /// Used to track the currently configured software breakpoints and store the op-code
     /// which was swapped out
     sw_breakpoints: HashMap<<GdbArch as Arch>::Usize, u8>,
 
-    /// Stores the current state of each Vcpu
-    vcpu_state: Vec<VcpuState>,
+    nr_cpus: usize,
 
     /// Stores the current paused thread id, GDB can inact commands without providing us a Tid to
     /// run on and expects us to use the last paused thread.
@@ -158,8 +143,6 @@ impl FirecrackerTarget {
                 .len()
         };
 
-        let vcpu_state = vec![Default::default(); cpu_count];
-
         Self {
             vmm,
             entry_addr,
@@ -167,12 +150,23 @@ impl FirecrackerTarget {
             // We only support 4 hw breakpoints on x86 this will need to be configurable on arm
             hw_breakpoints: Default::default(),
             sw_breakpoints: HashMap::new(),
-            vcpu_state,
+            nr_cpus: cpu_count,
 
             paused_vcpu: Tid::new(1),
         }
     }
 
+    pub fn with_vcpu_handle<R>(&self, tid: Tid, fun: impl FnOnce(&mut VcpuHandle) -> R) -> R {
+        let mut vmm = self.vmm.lock().unwrap();
+        let vcpu_handle = &mut vmm.vcpus_handles[tid_to_vcpuid(tid)];
+
+        fun(vcpu_handle)
+    }
+
+    pub fn with_paused_handle<R>(&self, fun: impl FnOnce(&mut VcpuHandle) -> Result<R, Error>) -> Result<R, Error> {
+        self.with_vcpu_handle(self.get_paused_vcpu()?, fun)
+    }
+
     /// Retrieves the currently paused Vcpu returns an error if there is no currently paused Vcpu
     fn get_paused_vcpu(&self) -> Result<Tid, Error> {
         self.paused_vcpu.ok_or(Error::NoPausedVCpu)
@@ -181,18 +175,19 @@ impl FirecrackerTarget {
     /// Updates state to reference the currently paused Vcpu and store that the cpu is currently
     /// paused
     pub fn update_paused_vcpu(&mut self, tid: Tid) {
-        self.vcpu_state[tid_to_vcpuid(tid)].paused = true;
         self.paused_vcpu = Some(tid);
     }
 
+    pub fn active_backpoint(&self) -> Vec<GuestAddress> {
+        self.hw_breakpoints.active_breakpoint()
+    }
+
     /// Resumes execution of the vmm, this will update all paused Vcpus with current kvm debug info
     /// and resume them
     fn resume_execution(&mut self) -> Result<(), Error> {
-        for cpu_id in 0..self.vcpu_state.len() {
-            let tid = vcpuid_to_tid(cpu_id)?;
-
-            self.update_kvm_debug(tid)?;
-            self.request_resume(tid)?;
+        for handle in &mut self.vmm.lock().unwrap().vcpus_handles {
+            handle.update_kvm_debug(self.active_backpoint())?;
+            handle.request_resume()?;
         }
 
         self.paused_vcpu = None;
@@ -200,10 +195,10 @@ impl FirecrackerTarget {
         Ok(())
     }
 
-    /// Resets all Vcpus to their base state
+    /// Resets all Vcpusrequest_pause to their base state
     fn reset_all_states(&mut self) {
-        for value in self.vcpu_state.iter_mut() {
-            value.reset_vcpu_state();
+        for value in self.vmm.lock().unwrap().vcpus_handles.iter_mut() {
+            value.single_step = false;
         }
     }
 
@@ -215,38 +210,61 @@ impl FirecrackerTarget {
             .stop(FcExitCode::Ok)
     }
 
-    /// Pauses the requested Vcpu
-    pub fn request_pause(&mut self, tid: Tid) -> Result<(), Error> {
-        let vcpu_state = &mut self.vcpu_state[tid_to_vcpuid(tid)];
+    /// Identifies why the specifc core was paused to be returned to GDB if None is returned this
+    /// indicates to handle this internally and don't notify GDB
+    pub fn get_stop_reason(&self, tid: Tid) -> Result<Option<BaseStopReason<Tid, u64>>, Error> {
+        self.with_vcpu_handle(tid, |handle| {
+            if handle.single_step {
+                return Ok(Some(MultiThreadStopReason::SignalWithThread {
+                    tid,
+                    signal: Signal::SIGTRAP,
+                }));
+            }
 
-        if vcpu_state.paused {
-            info!("Attempted to pause a vcpu already paused.");
-            // Pausing an already paused vcpu is not considered an error case from GDB
-            return Ok(());
-        }
+            let Ok(regs) = handle.get_regs() else {
+                return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
+            };
+
+            let physical_addr = handle.translate_gva(regs.rip)?;
+            if self.sw_breakpoints.contains_key(&physical_addr) {
+                return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
+            }
+
+            if self.hw_breakpoints.contains(&GuestAddress(regs.rip)) {
+                return Ok(Some(MultiThreadStopReason::HwBreak(tid)));
+            }
+
+            if regs.rip == self.entry_addr.0 {
+                return Ok(Some(MultiThreadStopReason::HwBreak(tid)));
+            }
 
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
+            // This is not a breakpoint we've set, likely one set by the guest
+            Ok(None)
+        })
+    }
+}
 
-        cpu_handle.send_event(VcpuEvent::Pause)?;
-        let _ = cpu_handle.response_receiver().recv()?;
+impl VcpuHandle {
+    /// Pauses the requested Vcpu
+    pub fn request_pause(&mut self) -> Result<(), Error> {
+        self.send_event(VcpuEvent::Pause)?;
+        let _ = self.response_receiver().recv()?;
 
-        vcpu_state.paused = false;
         Ok(())
     }
 
     /// Gets the register values for a Vcpu
-    fn get_regs(&self, tid: Tid) -> Result<kvm_regs, Error> {
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
-        cpu_handle.send_event(VcpuEvent::GetRegisters)?;
+    fn get_regs(&self) -> Result<kvm_regs, Error> {
+        self.send_event(VcpuEvent::GetRegisters)?;
 
-        let response = cpu_handle.response_receiver().recv()?;
+        let response = self.response_receiver().recv()?;
 
         if let VcpuResponse::KvmRegisters(response) = response {
             return Ok(response);
         }
 
         if let VcpuResponse::NotAllowed(message) = response {
-            error!("Response from get regs: {message} for TID {tid:?}");
+            error!("Response from get regs: {message} for TID ");
         }
 
         Err(Error::VcuRequestError)
@@ -254,11 +272,11 @@ impl FirecrackerTarget {
 
     /// Sets the register values for a Vcpu
     fn set_regs(&self, regs: kvm_regs, tid: Tid) -> Result<(), Error> {
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
-        cpu_handle.send_event(VcpuEvent::SetRegisters(regs))?;
+        self.send_event(VcpuEvent::SetRegisters(regs))?;
 
-        let response = cpu_handle.response_receiver().recv()?;
+        let response = self.response_receiver().recv()?;
         if let VcpuResponse::NotAllowed(message) = response {
+            // TODO: more this error message to caller
             error!("Response from set regs: {message} on tid: {tid:?}");
             return Err(Error::VcuRequestError);
         }
@@ -267,45 +285,26 @@ impl FirecrackerTarget {
     }
 
     /// Resumes the Vcpu, will return early if the Vcpu is already running
-    pub fn request_resume(&mut self, tid: Tid) -> Result<(), Error> {
-        let vcpu_state = &mut self.vcpu_state[tid_to_vcpuid(tid)];
-
-        if !vcpu_state.paused {
-            info!("Attempted to resume a vcpu already running.");
-            // Resuming an already running Vcpu is not considered an error case from GDB
-            return Ok(());
-        }
-
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
+    pub fn request_resume(&mut self) -> Result<(), Error> {
+        self.send_event(VcpuEvent::Resume)?;
 
-        cpu_handle.send_event(VcpuEvent::Resume)?;
-
-        let response = cpu_handle.response_receiver().recv()?;
+        let response = self.response_receiver().recv()?;
         if let VcpuResponse::NotAllowed(message) = response {
             error!("Response resume : {message}");
             return Err(Error::VcuRequestError);
         }
 
-        vcpu_state.paused = false;
         Ok(())
     }
 
     /// Updates the kvm debug flags set against the Vcpu
-    fn update_kvm_debug(&self, tid: Tid) -> Result<(), Error> {
-        let vcpu_state = &self.vcpu_state[tid_to_vcpuid(tid)];
-        if !vcpu_state.paused {
-            info!("Attempted to update kvm debug on a non paused Vcpu");
-            return Ok(());
-        }
-
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
-
-        cpu_handle.send_event(VcpuEvent::SetKvmDebug(
-            self.hw_breakpoints.active_breakpoint(),
-            vcpu_state.single_step,
+    fn update_kvm_debug(&self, active_breakpoint: Vec<GuestAddress>) -> Result<(), Error> {
+        self.send_event(VcpuEvent::SetKvmDebug(
+            active_breakpoint,
+            self.single_step,
         ))?;
 
-        match cpu_handle.response_receiver().recv() {
+        match self.response_receiver().recv() {
             Ok(VcpuResponse::NotAllowed(message)) => {
                 error!("Response set kvm debug: {message}");
                 Err(Error::VcuRequestError)
@@ -317,11 +316,10 @@ impl FirecrackerTarget {
     }
 
     /// Translates from a guest virtual address to a physical address
-    fn translate_gva(&self, tid: Tid, address: u64) -> Result<u64, Error> {
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
-        cpu_handle.send_event(VcpuEvent::GvaTranslate(address))?;
+    fn translate_gva(&self,  address: u64) -> Result<u64, Error> {
+        self.send_event(VcpuEvent::GvaTranslate(address))?;
 
-        let response = cpu_handle.response_receiver().recv()?;
+        let response = self.response_receiver().recv()?;
 
         if let VcpuResponse::GvaTranslation(response) = response {
             return match response {
@@ -339,14 +337,13 @@ impl FirecrackerTarget {
     }
 
     /// Injects a software breakpoint back into the guest
-    pub fn inject_bp_to_guest(&self, tid: Tid) -> Result<(), Error> {
-        let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
-        cpu_handle.send_event(VcpuEvent::InjectKvmBP(
-            self.hw_breakpoints.active_breakpoint(),
+    pub fn inject_bp_to_guest(&self, active_breakpoint: Vec<GuestAddress>) -> Result<(), Error> {
+        self.send_event(VcpuEvent::InjectKvmBP(
+            active_breakpoint,
             false,
         ))?;
 
-        match cpu_handle.response_receiver().recv() {
+        match self.response_receiver().recv() {
             Ok(VcpuResponse::NotAllowed(message)) => {
                 error!("Vcpu response to injecting bp: {message}");
                 Err(Error::VcuRequestError)
@@ -356,37 +353,6 @@ impl FirecrackerTarget {
             _ => Ok(()),
         }
     }
-
-    /// Identifies why the specifc core was paused to be returned to GDB if None is returned this
-    /// indicates to handle this internally and don't notify GDB
-    pub fn get_stop_reason(&self, tid: Tid) -> Result<Option<BaseStopReason<Tid, u64>>, Error> {
-        if self.vcpu_state[tid_to_vcpuid(tid)].single_step {
-            return Ok(Some(MultiThreadStopReason::SignalWithThread {
-                tid,
-                signal: Signal::SIGTRAP,
-            }));
-        }
-
-        let Ok(regs) = self.get_regs(tid) else {
-            return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
-        };
-
-        let physical_addr = self.translate_gva(tid, regs.rip)?;
-        if self.sw_breakpoints.contains_key(&physical_addr) {
-            return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
-        }
-
-        if self.hw_breakpoints.contains(&GuestAddress(regs.rip)) {
-            return Ok(Some(MultiThreadStopReason::HwBreak(tid)));
-        }
-
-        if regs.rip == self.entry_addr.0 {
-            return Ok(Some(MultiThreadStopReason::HwBreak(tid)));
-        }
-
-        // This is not a breakpoint we've set, likely one set by the guest
-        Ok(None)
-    }
 }
 
 impl Target for FirecrackerTarget {
@@ -414,7 +380,7 @@ impl Target for FirecrackerTarget {
 impl MultiThreadBase for FirecrackerTarget {
     /// Reads the registers for the Vcpu
     fn read_registers(&mut self, regs: &mut CoreRegs, tid: Tid) -> TargetResult<(), Self> {
-        let cpu_regs = self.get_regs(tid).map_err(|e| {
+        let cpu_regs = self.with_vcpu_handle(tid, |handle| handle.get_regs()).map_err(|e| {
             error!("Failed to read cpu registers: {e:?}");
             TargetError::NonFatal
         })?;
@@ -471,7 +437,7 @@ impl MultiThreadBase for FirecrackerTarget {
             rflags: regs.eflags as u64,
         };
 
-        self.set_regs(new_regs, tid).map_err(|e| {
+        self.with_vcpu_handle(tid, |handle| handle.set_regs(new_regs, tid)).map_err(|e| {
             error!("Error setting registers {e:?}");
             TargetError::NonFatal
         })
@@ -495,7 +461,7 @@ impl MultiThreadBase for FirecrackerTarget {
         while total_read < len {
             let gaddr = start_addr + total_read;
 
-            let paddr = self.translate_gva(tid, gaddr as u64).map_err(|e| {
+            let paddr = self.with_vcpu_handle(tid, |handle| handle.translate_gva(gaddr as u64)).map_err(|e| {
                 error!("Error {e:?} translating gva on read address: {start_addr:X}");
                 TargetError::NonFatal
             })?;
@@ -546,7 +512,7 @@ impl MultiThreadBase for FirecrackerTarget {
 
         while total_written < len {
             let gaddr = start_addr + total_written;
-            let paddr = match self.translate_gva(tid, gaddr as u64) {
+            let paddr = match self.with_vcpu_handle(tid, |handle| handle.translate_gva(gaddr as u64)) {
                 Ok(0) => gaddr,
                 Ok(paddr) => usize::try_from(paddr).map_err(|e| {
                     error!("Error converting addr to usize: {e:?}");
@@ -591,7 +557,7 @@ impl MultiThreadBase for FirecrackerTarget {
         &mut self,
         thread_is_active: &mut dyn FnMut(Tid),
     ) -> Result<(), Self::Error> {
-        for id in 0..self.vcpu_state.len() {
+        for id in 0..self.nr_cpus {
             thread_is_active(vcpuid_to_tid(id)?)
         }
 
@@ -616,8 +582,7 @@ impl MultiThreadResume for FirecrackerTarget {
         tid: Tid,
         _signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
-        let vcpu_state = &mut self.vcpu_state[tid_to_vcpuid(tid)];
-        vcpu_state.single_step = false;
+        self.vmm.lock().unwrap().vcpus_handles[tid_to_vcpuid(tid)].single_step = false;
 
         Ok(())
     }
@@ -647,8 +612,7 @@ impl MultiThreadSingleStep for FirecrackerTarget {
         tid: Tid,
         _signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
-        let vcpu_state = &mut self.vcpu_state[tid_to_vcpuid(tid)];
-        vcpu_state.single_step = true;
+        self.vmm.lock().unwrap().vcpus_handles[tid_to_vcpuid(tid)].single_step = true;
 
         Ok(())
     }
@@ -677,7 +641,7 @@ impl HwBreakpoint for FirecrackerTarget {
         if !self.hw_breakpoints.add(GuestAddress(addr)) {
             return Ok(false);
         }
-        self.update_kvm_debug(self.get_paused_vcpu()?)?;
+        self.with_paused_handle(|handle| handle.update_kvm_debug(self.active_backpoint()))?;
 
         Ok(true)
     }
@@ -689,7 +653,7 @@ impl HwBreakpoint for FirecrackerTarget {
         _kind: <Self::Arch as Arch>::BreakpointKind,
     ) -> TargetResult<bool, Self> {
         self.hw_breakpoints.remove(&GuestAddress(addr));
-        self.update_kvm_debug(self.get_paused_vcpu()?)?;
+        self.with_paused_handle(|handle| handle.update_kvm_debug(self.active_backpoint()))?;
 
         Ok(true)
     }
@@ -705,7 +669,7 @@ impl SwBreakpoint for FirecrackerTarget {
         addr: <Self::Arch as Arch>::Usize,
         _kind: <Self::Arch as Arch>::BreakpointKind,
     ) -> TargetResult<bool, Self> {
-        let physical_addr = self.translate_gva(self.get_paused_vcpu()?, addr)?;
+        let physical_addr = self.with_paused_handle(|handle| handle.translate_gva(addr))?;
 
         if self.sw_breakpoints.contains_key(&physical_addr) {
             return Ok(true);
@@ -728,7 +692,7 @@ impl SwBreakpoint for FirecrackerTarget {
         addr: <Self::Arch as Arch>::Usize,
         _kind: <Self::Arch as Arch>::BreakpointKind,
     ) -> TargetResult<bool, Self> {
-        let physical_addr = self.translate_gva(self.get_paused_vcpu()?, addr)?;
+        let physical_addr = self.with_paused_handle(|handle| handle.translate_gva(addr))?;
 
         if let Some(removed) = self.sw_breakpoints.remove(&physical_addr) {
             self.write_addrs(addr, &[removed], self.get_paused_vcpu()?)?;
diff --git a/src/vmm/src/vstate/vcpu/mod.rs b/src/vmm/src/vstate/vcpu/mod.rs
index 366e7555c..731d336b0 100644
--- a/src/vmm/src/vstate/vcpu/mod.rs
+++ b/src/vmm/src/vstate/vcpu/mod.rs
@@ -796,6 +796,9 @@ pub struct VcpuHandle {
     // Rust JoinHandles have to be wrapped in Option if you ever plan on 'join()'ing them.
     // We want to be able to join these threads in tests.
     vcpu_thread: Option<thread::JoinHandle<()>>,
+
+    #[cfg(feature = "debug")]
+    pub single_step: bool
 }
 
 /// Error type for [`VcpuHandle::send_event`].
@@ -819,6 +822,9 @@ impl VcpuHandle {
             event_sender,
             response_receiver,
             vcpu_thread: Some(vcpu_thread),
+
+            #[cfg(feature = "debug")]
+            single_step: false
         }
     }
     /// Sends event to vCPU.

[roypat]

You can use utils::u64_to_usize here. We dont support non-64bit targets, so this conversion can never fail (but sadly, the stdlib refuses to add #[cfg(pointer_width = "64")] impl Into<usize> for u64 😔 )

[roypat]

Mh, you're right that translate_gva cannot failt (the only error it might return is EFAULT if you pass in a nonsense kvm_translate pointer, but kvm-ioctls ensures we don't do that), but we can't really rely on that I think.

The other thing is that I don't immediately see they we assume that the result of the gva->gpa translation will never return 0 (we convert a 0 gpa into None via NonZeroU64::new). But I also don't understand why we're using NonZeroU64 in the first place. At the call site, we just convert it back into a u64, so I think we can just keep it a u64 all the way through. However, after converting to back to u64 in FirecrackerTarget::translate_gva, we have a special case in FirecrackerTarget::write_addrs that checks whether the gpa is 0 (which it cannot be because of the NonZeroU64 dance).

So how about we change this function be something like

Suggested change

	fn translate_gva(&self, address: u64) -> Option<NonZeroU64> {
	let tr = self.kvm_vcpu.fd.translate_gva(address).unwrap();
	if tr.valid == 0 {
	return None;
	}
	NonZeroU64::new(tr.physical_address)
	}
	fn translate_gva(&self, address: u64) -> Result<u64, KvmTranslateError> {
	let tr = self.kvm_vcpu.fd.translate_gva(address)?;
	if tr.valid == 0 {
	return Err(KvmTranslateError::NoTranslation);
	}
	Ok(tr.physical_address)
	}

[roypat]

I think I see why we have this Ok(address) here: The way the handlers for VcpuEvent::GvaTranslate are set up, all possible responses are handled above, and this line is unreachable (but the compiler cannot know this). However, this means that if ever change the error handling over in vcpu/mod.rs, then we suddenly start returning something wrong here.

The more I try to find a suggestion of what to do instead, the more I realize how much of a struggle this channel based communication is, lol. I think the following should do the trick?

Suggested change

	fn translate_gva(&self, tid: Tid, address: u64) -> Result<u64, Error> {
	let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
	cpu_handle.send_event(VcpuEvent::GvaTranslate(address))?;
	let response = cpu_handle.response_receiver().recv()?;
	if let VcpuResponse::GvaTranslation(response) = response {
	return match response {
	Some(res) => Ok(res.into()),
	None => Err(Error::VcuRequestError),
	};
	}
	if let VcpuResponse::NotAllowed(message) = response {
	error!("Response from gva: {message}");
	return Err(Error::VcuRequestError);
	}
	Ok(address)
	}
	fn translate_gva(&self, tid: Tid, address: u64) -> Result<u64, Error> {
	let cpu_handle = &self.vmm.lock()?.vcpus_handles[tid_to_vcpuid(tid)];
	cpu_handle.send_event(VcpuEvent::GvaTranslate(address))?;
	match cpu_handle.response_receiver().recv()? {
	VcpuResponse::GvaTranslation(translation) => Ok(translation?),
	other => Err(Error::UnexpectedResponse(other))
	}
	}

this assumes that GvaTranslation now holds a Result.

Tbh, now that I'm looking at it, I really dislike how we're using VcpuResponse as something that can hold both error and success variants. Imo it'd be so much cleaner if cpu_handle.response_receiver().recv() (which... why isn't it just cpu_handle.receive_response(), dual to cpu_handle.send_event? 😞) would return a Result<VcpuResponse, VcpuError>, where now VcpuResponse only contains success variants, and VcpuError contains both variants for "something went wrong while processing the last VcpuEvent", as well as the error case from .recv().

[roypat]

Another comment that's more about the existing code, but: Could Vcpu::NotAllowed be made to contain a VcpuEvent instead of a string? Then all these "not allowed" cases would turn into

Suggested change

	Ok(VcpuEvent::GetRegisters) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(String::from(
	"get registers is unavailable while running",
	)))
	.expect("vcpu channel unexpectedly closed");
	}
	#[cfg(feature = "debug")]
	Ok(VcpuEvent::SetRegisters(_)) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(String::from(
	"dump is unavailable while running",
	)))
	.expect("vcpu channel unexpectedly closed");
	}
	#[cfg(feature = "debug")]
	Ok(VcpuEvent::SetKvmDebug(_, _)) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(String::from(
	"kvm debug is unavailable while running",
	)))
	.expect("vcpu channel unexpectedly closed");
	}
	#[cfg(feature = "debug")]
	Ok(VcpuEvent::InjectKvmBP(_, _)) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(String::from(
	"Injecting bp is unavailable while running",
	)))
	.expect("vcpu channel unexpectedly closed");
	}
	#[cfg(feature = "debug")]
	Ok(VcpuEvent::GvaTranslate(_)) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(String::from(
	"gva translate is unavailable while running",
	)))
	.expect("vcpu channel unexpectedly closed");
	}
	Ok(event @ (VcpuEvent::GetRegisters | VcpuEvent::SetRegister(_) | VcpuEvent::SetKvmDebug(_, _) | VcpuEvent::InjectKvmBP(_, _) | VcpuEvent::GvaTranslate(_))) => {
	self.response_sender
	.send(VcpuResponse::NotAllowed(event))
	.expect("vcpu channel unexpectedly closed");
	}

in fact, here's the diff that does the refactor, I think we should definitely do this 🥺:

diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs
index 6a681431f..79d6de1d1 100644
--- a/src/vmm/src/lib.rs
+++ b/src/vmm/src/lib.rs
@@ -294,8 +294,8 @@ pub enum DumpCpuConfigError {
     UnexpectedResponse,
     /// Failed to dump CPU config: {0}
     DumpCpuConfig(#[from] vcpu::VcpuError),
-    /// Operation not allowed: {0}
-    NotAllowed(String),
+    /// Operation not allowed: {0:?}
+    NotAllowed(VcpuEvent),
 }
 
 /// Contains the state and associated methods required for the Firecracker VMM.
diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs
index 6410e4d6c..0b391effb 100644
--- a/src/vmm/src/persist.rs
+++ b/src/vmm/src/persist.rs
@@ -41,7 +41,7 @@ use crate::vstate::memory::{
 };
 use crate::vstate::vcpu::{VcpuSendEventError, VcpuState};
 use crate::vstate::vm::VmState;
-use crate::{mem_size_mib, vstate, EventManager, Vmm, VmmError};
+use crate::{mem_size_mib, vstate, EventManager, Vmm, VmmError, VcpuEvent};
 
 /// Holds information related to the VM that is not part of VmState.
 #[derive(Clone, Debug, Default, Deserialize, PartialEq, Eq, Serialize)]
@@ -115,8 +115,8 @@ pub enum MicrovmStateError {
     IncompatibleState(String),
     /// Provided MicroVM state is invalid.
     InvalidInput,
-    /// Operation not allowed: {0}
-    NotAllowed(String),
+    /// Operation not allowed: {0:?}
+    NotAllowed(VcpuEvent),
     /// Cannot restore devices: {0}
     RestoreDevices(DevicePersistError),
     /// Cannot restore Vcpu state: {0}
diff --git a/src/vmm/src/vstate/vcpu/mod.rs b/src/vmm/src/vstate/vcpu/mod.rs
index 366e7555c..3232412c1 100644
--- a/src/vmm/src/vstate/vcpu/mod.rs
+++ b/src/vmm/src/vstate/vcpu/mod.rs
@@ -346,59 +346,15 @@ impl Vcpu {
                     .expect("vcpu channel unexpectedly closed");
             }
             // SaveState cannot be performed on a running Vcpu.
-            Ok(VcpuEvent::SaveState) => {
-                self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "save/restore unavailable while running",
-                    )))
-                    .expect("vcpu channel unexpectedly closed");
-            }
-            // DumpCpuConfig cannot be performed on a running Vcpu.
-            Ok(VcpuEvent::DumpCpuConfig) => {
+            Ok(event @ (VcpuEvent::SaveState | VcpuEvent::DumpCpuConfig)) => {
                 self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "cpu config dump is unavailable while running",
-                    )))
+                    .send(VcpuResponse::NotAllowed(event))
                     .expect("vcpu channel unexpectedly closed");
             }
             #[cfg(feature = "debug")]
-            Ok(VcpuEvent::GetRegisters) => {
+            Ok(event @ (VcpuEvent::GetRegisters | VcpuEvent::SetRegisters(_) | VcpuEvent::SetKvmDebug(_, _) | VcpuEvent::InjectKvmBP(_, _) | VcpuEvent::GvaTranslate(_))) => {
                 self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "get registers is unavailable while running",
-                    )))
-                    .expect("vcpu channel unexpectedly closed");
-            }
-            #[cfg(feature = "debug")]
-            Ok(VcpuEvent::SetRegisters(_)) => {
-                self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "dump is unavailable while running",
-                    )))
-                    .expect("vcpu channel unexpectedly closed");
-            }
-            #[cfg(feature = "debug")]
-            Ok(VcpuEvent::SetKvmDebug(_, _)) => {
-                self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "kvm debug is unavailable while running",
-                    )))
-                    .expect("vcpu channel unexpectedly closed");
-            }
-            #[cfg(feature = "debug")]
-            Ok(VcpuEvent::InjectKvmBP(_, _)) => {
-                self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "Injecting bp is unavailable while running",
-                    )))
-                    .expect("vcpu channel unexpectedly closed");
-            }
-            #[cfg(feature = "debug")]
-            Ok(VcpuEvent::GvaTranslate(_)) => {
-                self.response_sender
-                    .send(VcpuResponse::NotAllowed(String::from(
-                        "gva translate is unavailable while running",
-                    )))
+                    .send(VcpuResponse::NotAllowed(event))
                     .expect("vcpu channel unexpectedly closed");
             }
             Ok(VcpuEvent::Finish) => return StateMachine::finish(),
@@ -752,7 +708,7 @@ pub enum VcpuResponse {
     /// Vcpu is stopped.
     Exited(FcExitCode),
     /// Requested action not allowed.
-    NotAllowed(String),
+    NotAllowed(VcpuEvent),
     /// Vcpu is paused.
     Paused,
     /// Vcpu is resumed.
@@ -778,7 +734,7 @@ impl fmt::Debug for VcpuResponse {
             Exited(code) => write!(f, "VcpuResponse::Exited({:?})", code),
             SavedState(_) => write!(f, "VcpuResponse::SavedState"),
             Error(ref err) => write!(f, "VcpuResponse::Error({:?})", err),
-            NotAllowed(ref reason) => write!(f, "VcpuResponse::NotAllowed({})", reason),
+            NotAllowed(ref reason) => write!(f, "VcpuResponse::NotAllowed({:?})", reason),
             DumpedCpuConfig(_) => write!(f, "VcpuResponse::DumpedCpuConfig"),
             #[cfg(feature = "debug")]
             KvmRegisters(_) => write!(f, "VcpuResponse::KvmRegisters"),

[roypat]

Why does 0 here mean that we should treat the gva as a gpa? (ignoring the fact that due to the NonZeroU64 stuff we'll never get 0 here anyway)

[roypat]

nit: in the kernel, these kind of variables are usually called "gva" (for "guest virtual address") and "gpa" (for "guest physical address")

[roypat]

nit 2: If you declare start_addr as mut, you can just advance it by total_written each loop iteration, and avoid a separate local variable.

[roypat]

nit: if you declare data as mut, then you can have this loop by while !data.is_empty(), and in the body after writing to guest memory you do data = data[write_len..]. Honestly, not sure if that's more readable, but it's a common pattern in the rust stdlib :D

clicked the wrong button, sorry