attempt at removing cloning of VcpuFd from GDB

ShadowCurse · ShadowCurse · commit bd0263ef2c11 · 2025-09-03T13:56:16.000+01:00
Signed-off-by: Egor Lazarchuk &lt;yegorlz@amazon.co.uk&gt;
diff --git a/src/vmm/src/gdb/target.rs b/src/vmm/src/gdb/target.rs
@@ -39,38 +39,47 @@ use crate::utils::u64_to_usize;
 use crate::vstate::vcpu::VcpuSendEventError;
 use crate::{FcExitCode, VcpuEvent, VcpuResponse, Vmm};
 
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 /// Stores the current state of a Vcpu with a copy of the Vcpu file descriptor
 struct VcpuState {
     single_step: bool,
     paused: bool,
-    vcpu_fd: VcpuFd,
+    // vcpu_fd: VcpuFd,
 }
 
-impl VcpuState {
-    /// Constructs a new instance of a VcpuState from a VcpuFd
-    fn from_vcpu_fd(vcpu_fd: VcpuFd) -> Self {
+impl Default for VcpuState {
+    fn default() -> Self {
         Self {
             single_step: false,
             paused: false,
-            vcpu_fd,
         }
     }
+}
+
+impl VcpuState {
+    /// Constructs a new instance of a VcpuState from a VcpuFd
+    // fn from_vcpu_fd(vcpu_fd: VcpuFd) -> Self {
+    //     Self {
+    //         single_step: false,
+    //         paused: false,
+    //         vcpu_fd,
+    //     }
+    // }
 
     /// Disables single stepping on the Vcpu state
     fn reset_vcpu_state(&mut self) {
         self.single_step = false;
     }
 
-    /// Updates the kvm debug flags set against the Vcpu with a check
-    fn update_kvm_debug(&self, hw_breakpoints: &[GuestAddress]) -> Result<(), GdbTargetError> {
-        if !self.paused {
-            info!("Attempted to update kvm debug on a non paused Vcpu");
-            return Ok(());
-        }
-
-        arch::vcpu_set_debug(&self.vcpu_fd, hw_breakpoints, self.single_step)
-    }
+    // // Updates the kvm debug flags set against the Vcpu with a check
+    // fn update_kvm_debug(&self, hw_breakpoints: &[GuestAddress]) -> Result<(), GdbTargetError> {
+    //     if !self.paused {
+    //         info!("Attempted to update kvm debug on a non paused Vcpu");
+    //         return Ok(());
+    //     }
+    //
+    //     arch::vcpu_set_debug(&self.vcpu_fd, hw_breakpoints, self.single_step)
+    // }
 }
 
 /// Errors from interactions between GDB and the VMM
@@ -185,7 +194,7 @@ impl FirecrackerTarget {
         gdb_event: Receiver<usize>,
         entry_addr: GuestAddress,
     ) -> Self {
-        let mut vcpu_state: Vec<_> = vcpu_fds.into_iter().map(VcpuState::from_vcpu_fd).collect();
+        let mut vcpu_state = vec![VcpuState::default(); vmm.lock().unwrap().vcpus_handles.len()];
         // By default vcpu 1 will be paused at the entry point
         vcpu_state[0].paused = true;
 
@@ -202,16 +211,34 @@ impl FirecrackerTarget {
         }
     }
 
+    fn update_vcpu_kvm_debug(
+        &self,
+        vcpu_idx: usize,
+        hw_breakpoints: &[GuestAddress],
+    ) -> Result<(), GdbTargetError> {
+        let state = &self.vcpu_state[vcpu_idx];
+        if !state.paused {
+            info!("Attempted to update kvm debug on a non paused Vcpu");
+            return Ok(());
+        }
+
+        let vcpu_fd = &self.vmm.lock().unwrap().vcpus_handles[vcpu_idx].vcpu_fd;
+        arch::vcpu_set_debug(vcpu_fd, hw_breakpoints, state.single_step)
+    }
+
     /// Retrieves the currently paused Vcpu id returns an error if there is no currently paused Vcpu
     fn get_paused_vcpu_id(&self) -> Result<Tid, GdbTargetError> {
         self.paused_vcpu.ok_or(GdbTargetError::NoPausedVcpu)
     }
 
-    /// Retrieves the currently paused Vcpu state returns an error if there is no currently paused
-    /// Vcpu
-    fn get_paused_vcpu(&self) -> Result<&VcpuState, GdbTargetError> {
-        let vcpu_index = tid_to_vcpuid(self.get_paused_vcpu_id()?);
-        Ok(&self.vcpu_state[vcpu_index])
+    fn get_paused_vcpu_idx(&self) -> Result<usize, GdbTargetError> {
+        Ok(tid_to_vcpuid(self.get_paused_vcpu_id()?))
+    }
+
+    fn get_vcpu_fd(&self, vcpu_idx: usize) -> Result<&VcpuFd, GdbTargetError> {
+        // let vcpu_index = tid_to_vcpuid(self.get_paused_vcpu_id()?);
+        // Ok(&self.vcpu_state[vcpu_index])
+        Ok(&self.vmm.lock().unwrap().vcpus_handles[vcpu_idx].vcpu_fd)
     }
 
     /// Updates state to reference the currently paused Vcpu and store that the cpu is currently
@@ -224,9 +251,9 @@ impl FirecrackerTarget {
     /// Resumes execution of all paused Vcpus, update them with current kvm debug info
     /// and resumes
     fn resume_all_vcpus(&mut self) -> Result<(), GdbTargetError> {
-        self.vcpu_state
-            .iter()
-            .try_for_each(|state| state.update_kvm_debug(&self.hw_breakpoints))?;
+        for idx in 0..self.vcpu_state.len() {
+            self.update_vcpu_kvm_debug(idx, &self.hw_breakpoints)?;
+        }
 
         for cpu_id in 0..self.vcpu_state.len() {
             let tid = vcpuid_to_tid(cpu_id)?;
@@ -274,8 +301,8 @@ impl FirecrackerTarget {
 
     /// A helper function to allow the event loop to inject this breakpoint back into the Vcpu
     pub fn inject_bp_to_guest(&mut self, tid: Tid) -> Result<(), GdbTargetError> {
-        let vcpu_state = &mut self.vcpu_state[tid_to_vcpuid(tid)];
-        arch::vcpu_inject_bp(&vcpu_state.vcpu_fd, &self.hw_breakpoints, false)
+        let vcpu_fd = self.get_vcpu_fd(tid_to_vcpuid(tid))?;
+        arch::vcpu_inject_bp(vcpu_fd, &self.hw_breakpoints, false)
     }
 
     /// Resumes the Vcpu, will return early if the Vcpu is already running
@@ -307,21 +334,23 @@ impl FirecrackerTarget {
         &self,
         tid: Tid,
     ) -> Result<Option<BaseStopReason<Tid, u64>>, GdbTargetError> {
-        let vcpu_state = &self.vcpu_state[tid_to_vcpuid(tid)];
+        let vcpu_idx = tid_to_vcpuid(tid);
+        let vcpu_state = &self.vcpu_state[vcpu_idx];
         if vcpu_state.single_step {
             return Ok(Some(MultiThreadStopReason::SignalWithThread {
                 tid,
                 signal: Signal::SIGTRAP,
             }));
         }
 
-        let Ok(ip) = arch::get_instruction_pointer(&vcpu_state.vcpu_fd) else {
+        let vcpu_fd = self.get_vcpu_fd(vcpu_idx)?;
+        let Ok(ip) = arch::get_instruction_pointer(vcpu_fd) else {
             // If we error here we return an arbitrary Software Breakpoint, GDB will handle
             // this gracefully
             return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
         };
 
-        let gpa = arch::translate_gva(&vcpu_state.vcpu_fd, ip, &self.vmm.lock().unwrap())?;
+        let gpa = arch::translate_gva(vcpu_fd, ip, &self.vmm.lock().unwrap())?;
         if self.sw_breakpoints.contains_key(&gpa) {
             return Ok(Some(MultiThreadStopReason::SwBreak(tid)));
         }
@@ -364,14 +393,18 @@ 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> {
-        arch::read_registers(&self.vcpu_state[tid_to_vcpuid(tid)].vcpu_fd, regs)?;
+        let vcpu_idx = tid_to_vcpuid(tid);
+        let vcpu_fd = self.get_vcpu_fd(vcpu_idx)?;
+        arch::read_registers(vcpu_fd, regs)?;
 
         Ok(())
     }
 
     /// Writes to the registers for the Vcpu
     fn write_registers(&mut self, regs: &CoreRegs, tid: Tid) -> TargetResult<(), Self> {
-        arch::write_registers(&self.vcpu_state[tid_to_vcpuid(tid)].vcpu_fd, regs)?;
+        let vcpu_idx = tid_to_vcpuid(tid);
+        let vcpu_fd = self.get_vcpu_fd(vcpu_idx)?;
+        arch::write_registers(vcpu_fd, regs)?;
 
         Ok(())
     }
@@ -384,12 +417,14 @@ impl MultiThreadBase for FirecrackerTarget {
         tid: Tid,
     ) -> TargetResult<usize, Self> {
         let data_len = data.len();
-        let vcpu_state = &self.vcpu_state[tid_to_vcpuid(tid)];
+        let vcpu_idx = tid_to_vcpuid(tid);
+        let vcpu_state = &self.vcpu_state[vcpu_idx];
+        let vcpu_fd = self.get_vcpu_fd(vcpu_idx)?;
 
         let vmm = &self.vmm.lock().expect("Error locking vmm in read addr");
 
         while !data.is_empty() {
-            let gpa = arch::translate_gva(&vcpu_state.vcpu_fd, gva, &vmm).map_err(|e| {
+            let gpa = arch::translate_gva(vcpu_fd, gva, &vmm).map_err(|e| {
                 error!("Error {e:?} translating gva on read address: {gva:#X}");
             })?;
 
@@ -420,11 +455,13 @@ impl MultiThreadBase for FirecrackerTarget {
         mut data: &[u8],
         tid: Tid,
     ) -> TargetResult<(), Self> {
-        let vcpu_state = &self.vcpu_state[tid_to_vcpuid(tid)];
+        let vcpu_idx = tid_to_vcpuid(tid);
+        let vcpu_state = &self.vcpu_state[vcpu_idx];
+        let vcpu_fd = self.get_vcpu_fd(vcpu_idx)?;
         let vmm = &self.vmm.lock().expect("Error locking vmm in write addr");
 
         while !data.is_empty() {
-            let gpa = arch::translate_gva(&vcpu_state.vcpu_fd, gva, &vmm).map_err(|e| {
+            let gpa = arch::translate_gva(vcpu_fd, gva, &vmm).map_err(|e| {
                 error!("Error {e:?} translating gva on read address: {gva:#X}");
             })?;
 
@@ -546,8 +583,8 @@ impl HwBreakpoint for FirecrackerTarget {
             return Ok(false);
         }
 
-        let state = self.get_paused_vcpu()?;
-        state.update_kvm_debug(&self.hw_breakpoints)?;
+        let vcpu_idx = self.get_paused_vcpu_idx()?;
+        self.update_vcpu_kvm_debug(vcpu_idx, &self.hw_breakpoints)?;
 
         Ok(true)
     }
@@ -563,8 +600,8 @@ impl HwBreakpoint for FirecrackerTarget {
             Some(pos) => self.hw_breakpoints.remove(pos),
         };
 
-        let state = self.get_paused_vcpu()?;
-        state.update_kvm_debug(&self.hw_breakpoints)?;
+        let vcpu_idx = self.get_paused_vcpu_idx()?;
+        self.update_vcpu_kvm_debug(vcpu_idx, &self.hw_breakpoints)?;
 
         Ok(true)
     }
@@ -580,11 +617,8 @@ impl SwBreakpoint for FirecrackerTarget {
         addr: <Self::Arch as Arch>::Usize,
         _kind: <Self::Arch as Arch>::BreakpointKind,
     ) -> TargetResult<bool, Self> {
-        let gpa = arch::translate_gva(
-            &self.get_paused_vcpu()?.vcpu_fd,
-            addr,
-            &self.vmm.lock().unwrap(),
-        )?;
+        let vcpu_fd = self.get_vcpu_fd(self.get_paused_vcpu_idx()?)?;
+        let gpa = arch::translate_gva(vcpu_fd, addr, &self.vmm.lock().unwrap())?;
 
         if self.sw_breakpoints.contains_key(&gpa) {
             return Ok(true);
@@ -608,11 +642,8 @@ impl SwBreakpoint for FirecrackerTarget {
         addr: <Self::Arch as Arch>::Usize,
         _kind: <Self::Arch as Arch>::BreakpointKind,
     ) -> TargetResult<bool, Self> {
-        let gpa = arch::translate_gva(
-            &self.get_paused_vcpu()?.vcpu_fd,
-            addr,
-            &self.vmm.lock().unwrap(),
-        )?;
+        let vcpu_fd = self.get_vcpu_fd(self.get_paused_vcpu_idx()?)?;
+        let gpa = arch::translate_gva(vcpu_fd, addr, &self.vmm.lock().unwrap())?;
 
         if let Some(removed) = self.sw_breakpoints.remove(&gpa) {
             self.write_addrs(addr, &removed, self.get_paused_vcpu_id()?)?;
diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs
@@ -297,7 +297,7 @@ pub struct Vmm {
     // Save UFFD in order to keep it open in the Firecracker process, as well.
     #[allow(unused)]
     uffd: Option<Uffd>,
-    vcpus_handles: Vec<VcpuHandle>,
+    pub vcpus_handles: Vec<VcpuHandle>,
     // Used by Vcpus and devices to initiate teardown; Vmm should never write here.
     vcpus_exit_evt: EventFd,
     // Device manager
diff --git a/src/vmm/src/vstate/vcpu.rs b/src/vmm/src/vstate/vcpu.rs
@@ -598,7 +598,7 @@ pub struct VcpuHandle {
     response_receiver: Receiver<VcpuResponse>,
     // 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_fd: VcpuFd,
+    pub vcpu_fd: VcpuFd,
     vcpu_thread: Option<thread::JoinHandle<()>>,
 }
 

Original file line number	Diff line number	Diff line change
`@@ -598,7 +598,7 @@ pub struct VcpuHandle {`
`598`	`598`	`response_receiver: Receiver<VcpuResponse>,`
`599`	`599`	`// Rust JoinHandles have to be wrapped in Option if you ever plan on 'join()'ing them.`
`600`	`600`	`// We want to be able to join these threads in tests.`
`601`		`- vcpu_fd: VcpuFd,`
	`601`	`+ pub vcpu_fd: VcpuFd,`
`602`	`602`	`vcpu_thread: Option<thread::JoinHandle<()>>,`
`603`	`603`	`}`
`604`	`604`