From 6d0d4bc5e2fdf9fc8da1ff6c6a37ff14f9718e25 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Thu, 9 Apr 2026 22:24:37 +0200 Subject: [PATCH] vmm: protect vcpu states in CpuManager with a mutex This is a prerequisite for the next commit where we need shared access. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster --- vmm/src/cpu.rs | 101 ++++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 39 deletions(-) diff --git a/vmm/src/cpu.rs b/vmm/src/cpu.rs index 4b15cffc3..061c20d47 100644 --- a/vmm/src/cpu.rs +++ b/vmm/src/cpu.rs @@ -682,7 +682,7 @@ pub struct CpuManager { reset_evt: EventFd, #[cfg(feature = "guest_debug")] vm_debug_evt: EventFd, - vcpu_states: Vec, + vcpu_states: Arc>>, selected_cpu: u32, vcpus: Vec>>, seccomp_action: SeccompAction, @@ -741,6 +741,7 @@ impl BusDevice for CpuManager { fn read(&mut self, _base: u64, offset: u64, data: &mut [u8]) { // The Linux kernel, quite reasonably, doesn't zero the memory it gives us. data.fill(0); + let vcpu_states = self.vcpu_states.lock().unwrap(); match offset { CPU_SELECTION_OFFSET => { @@ -750,7 +751,7 @@ impl BusDevice for CpuManager { } CPU_STATUS_OFFSET => { if self.selected_cpu < self.max_vcpus() { - let state = &self.vcpu_states[usize::try_from(self.selected_cpu).unwrap()]; + let state = &vcpu_states[usize::try_from(self.selected_cpu).unwrap()]; if state.active() { data[0] |= 1 << CPU_ENABLE_FLAG; } @@ -779,23 +780,28 @@ impl BusDevice for CpuManager { } CPU_STATUS_OFFSET => { if self.selected_cpu < self.max_vcpus() { - let state = &mut self.vcpu_states[usize::try_from(self.selected_cpu).unwrap()]; - // The ACPI code writes back a 1 to acknowledge the insertion - if (data[0] & (1 << CPU_INSERTING_FLAG) == 1 << CPU_INSERTING_FLAG) - && state.inserting - { - state.inserting = false; - } - // Ditto for removal - if (data[0] & (1 << CPU_REMOVING_FLAG) == 1 << CPU_REMOVING_FLAG) - && state.removing - { - state.removing = false; - } - // Trigger removal of vCPU - if data[0] & (1 << CPU_EJECT_FLAG) == 1 << CPU_EJECT_FLAG - && let Err(e) = self.remove_vcpu(self.selected_cpu) - { + let eject = { + // This structure is not shared with the vCPU thread, therefore, holding the + // lock for the entire function doesn't cause any deadlock. + let mut vcpu_states = self.vcpu_states.lock().unwrap(); + let state = &mut vcpu_states[usize::try_from(self.selected_cpu).unwrap()]; + + if (data[0] & (1 << CPU_INSERTING_FLAG) == 1 << CPU_INSERTING_FLAG) + && state.inserting + { + state.inserting = false; + } + + if (data[0] & (1 << CPU_REMOVING_FLAG) == 1 << CPU_REMOVING_FLAG) + && state.removing + { + state.removing = false; + } + + data[0] & (1 << CPU_EJECT_FLAG) == 1 << CPU_EJECT_FLAG + }; + + if eject && let Err(e) = self.remove_vcpu(self.selected_cpu) { error!("Error removing vCPU: {e:?}"); } } else { @@ -907,6 +913,7 @@ impl CpuManager { let max_vcpus = usize::try_from(config.max_vcpus).unwrap(); let mut vcpu_states = Vec::with_capacity(max_vcpus); vcpu_states.resize_with(max_vcpus, VcpuState::default); + let vcpu_states = Arc::new(Mutex::new(vcpu_states)); let hypervisor_type = hypervisor.hypervisor_type(); #[cfg(target_arch = "x86_64")] let cpu_vendor = hypervisor.get_cpu_vendor(); @@ -1176,14 +1183,14 @@ impl CpuManager { let vcpus_pause_signalled = self.vcpus_pause_signalled.clone(); let vcpus_kick_signalled = self.vcpus_kick_signalled.clone(); - let vcpu_kill = self.vcpu_states[usize::try_from(vcpu_id).unwrap()] - .kill - .clone(); - let vcpu_run_interrupted = self.vcpu_states[usize::try_from(vcpu_id).unwrap()] + let mut vcpu_states = self.vcpu_states.lock().unwrap(); + + let vcpu_kill = vcpu_states[usize::try_from(vcpu_id).unwrap()].kill.clone(); + let vcpu_run_interrupted = vcpu_states[usize::try_from(vcpu_id).unwrap()] .vcpu_run_interrupted .clone(); let panic_vcpu_run_interrupted = vcpu_run_interrupted.clone(); - let vcpu_paused = self.vcpu_states[usize::try_from(vcpu_id).unwrap()] + let vcpu_paused = vcpu_states[usize::try_from(vcpu_id).unwrap()] .paused .clone(); @@ -1470,8 +1477,8 @@ impl CpuManager { // On hot plug calls into this function entry_point is None. It is for // those hotplug CPU additions that we need to set the inserting flag. - self.vcpu_states[usize::try_from(vcpu_id).unwrap()].handle = handle; - self.vcpu_states[usize::try_from(vcpu_id).unwrap()].inserting = inserting; + vcpu_states[usize::try_from(vcpu_id).unwrap()].handle = handle; + vcpu_states[usize::try_from(vcpu_id).unwrap()].inserting = inserting; Ok(()) } @@ -1515,17 +1522,20 @@ impl CpuManager { } fn mark_vcpus_for_removal(&mut self, desired_vcpus: u32) { + let mut vcpu_states = self.vcpu_states.lock().unwrap(); + let present_vcpus = Self::active_vcpus(&vcpu_states); + // Mark vCPUs for removal, actual removal happens on ejection - for cpu_id in desired_vcpus..self.present_vcpus() { - self.vcpu_states[usize::try_from(cpu_id).unwrap()].removing = true; - self.vcpu_states[usize::try_from(cpu_id).unwrap()] + for cpu_id in desired_vcpus..present_vcpus { + vcpu_states[usize::try_from(cpu_id).unwrap()].removing = true; + vcpu_states[usize::try_from(cpu_id).unwrap()] .pending_removal .store(true, Ordering::SeqCst); } } pub fn check_pending_removed_vcpu(&mut self) -> bool { - for state in self.vcpu_states.iter() { + for state in self.vcpu_states.lock().unwrap().iter() { if state.active() && state.pending_removal.load(Ordering::SeqCst) { return true; } @@ -1535,7 +1545,8 @@ impl CpuManager { fn remove_vcpu(&mut self, cpu_id: u32) -> Result<()> { info!("Removing vCPU: cpu_id = {cpu_id}"); - let state = &mut self.vcpu_states[usize::try_from(cpu_id).unwrap()]; + let mut vcpu_states = self.vcpu_states.lock().unwrap(); + let state = &mut vcpu_states[usize::try_from(cpu_id).unwrap()]; state.kill.store(true, Ordering::SeqCst); state.signal_thread(); state.wait_until_signal_acknowledged()?; @@ -1631,12 +1642,15 @@ impl CpuManager { /// For the vCPU threads this will interrupt the KVM_RUN ioctl() allowing /// the loop to check the shared state booleans. fn signal_vcpus(&mut self) -> Result<()> { + // Holding the lock for the whole operation is correct: + let vcpu_states = self.vcpu_states.lock().unwrap(); + // Splitting this into two loops reduced the time to pause many vCPUs // massively. Example: 254 vCPUs. >254ms -> ~4ms. - for state in self.vcpu_states.iter() { + for state in vcpu_states.iter() { state.signal_thread(); } - for state in self.vcpu_states.iter() { + for state in vcpu_states.iter() { state.wait_until_signal_acknowledged()?; } @@ -1651,14 +1665,14 @@ impl CpuManager { self.vcpus_pause_signalled.store(false, Ordering::SeqCst); // Unpark all the VCPU threads. - for state in self.vcpu_states.iter() { + for state in self.vcpu_states.lock().unwrap().iter() { state.unpark_thread(); } self.signal_vcpus()?; // Wait for all the threads to finish. This removes the state from the vector. - for mut state in self.vcpu_states.drain(..) { + for mut state in self.vcpu_states.lock().unwrap().drain(..) { state.join_thread()?; } @@ -1691,8 +1705,15 @@ impl CpuManager { self.cpuid.clone() } + /// Locks the vCPU states and calls [`Self::active_vcpus`]. fn present_vcpus(&self) -> u32 { - self.vcpu_states + let lock = self.vcpu_states.lock().unwrap(); + Self::active_vcpus(&lock) + } + + /// Counts the number of active vCPUs (running vCPU threads). + fn active_vcpus(vcpu_states: &[VcpuState]) -> u32 { + vcpu_states .iter() .fold(0, |acc, state| acc + state.active() as u32) } @@ -2651,7 +2672,7 @@ impl Pausable for CpuManager { // The vCPU thread will change its paused state before parking, wait here for each // activated vCPU change their state to ensure they have parked. - for state in self.vcpu_states.iter() { + for state in self.vcpu_states.lock().unwrap().iter() { if state.active() { // wait for vCPU to update state while !state.paused.load(Ordering::SeqCst) { @@ -2669,16 +2690,18 @@ impl Pausable for CpuManager { // their run vCPU loop. self.vcpus_pause_signalled.store(false, Ordering::SeqCst); + let vcpu_states = self.vcpu_states.lock().unwrap(); + // Unpark all the vCPU threads. // Step 1/2: signal each thread { - for state in self.vcpu_states.iter() { + for state in vcpu_states.iter() { state.unpark_thread(); } } // Step 2/2: wait for state ACK { - for state in self.vcpu_states.iter() { + for state in vcpu_states.iter() { // wait for vCPU to update state while state.paused.load(Ordering::SeqCst) { // To avoid a priority inversion with the vCPU thread