From 06dc7085155e2d8b6c0b025993d1c3c367d76820 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Thu, 3 Aug 2023 15:13:03 +0100 Subject: [PATCH] vmm: Only return from reset driven I/O once event received The reset system is asynchronous with an I/O event (PIO or MMIO) for ACPI/i8042/CMOS triggering a write to the reset_evt event handler. The VMM thread will pick up this event on the VMM main loop and then trigger a shutdown in the CpuManager. However since there is some delay between the CPU threads being marked to be killed (through the CpuManager::cpus_kill_signalled bool) it is possible for the guest vCPU that triggered the exit to be re-entered when the vCPU KVM_RUN is called after the I/O exit is completed. This is undesirable and in particular the Linux kernel will attempt to jump to real mode after a CMOS based exit - this is unsupported in nested KVM on AMD on Azure and will trigger an error in KVM_RUN. Solve this problem by spinning in the device that has triggered the reset until the vcpus_kill_signalled boolean has been updated indicating that the VMM thread has received the event and called CpuManager::shutdown(). In particular if this bool is set then the vCPU threads will not re-enter the guest. Signed-off-by: Rob Bradford --- devices/src/acpi.rs | 24 +++++++++++++++++++++++- devices/src/legacy/cmos.rs | 18 +++++++++++++++++- devices/src/legacy/i8042.rs | 21 ++++++++++++++++++--- fuzz/fuzz_targets/cmos.rs | 3 +++ vmm/src/cpu.rs | 4 ++++ vmm/src/device_manager.rs | 18 +++++++++++++++++- 6 files changed, 82 insertions(+), 6 deletions(-) diff --git a/devices/src/acpi.rs b/devices/src/acpi.rs index eda6b61be..b8d1247aa 100644 --- a/devices/src/acpi.rs +++ b/devices/src/acpi.rs @@ -5,7 +5,9 @@ use super::AcpiNotificationFlags; use acpi_tables::{aml, Aml, AmlSink}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Barrier}; +use std::thread; use std::time::Instant; use vm_device::interrupt::InterruptSourceGroup; use vm_device::BusDevice; @@ -18,14 +20,20 @@ pub const GED_DEVICE_ACPI_SIZE: usize = 0x1; pub struct AcpiShutdownDevice { exit_evt: EventFd, reset_evt: EventFd, + vcpus_kill_signalled: Arc, } impl AcpiShutdownDevice { /// Constructs a device that will signal the given event when the guest requests it. - pub fn new(exit_evt: EventFd, reset_evt: EventFd) -> AcpiShutdownDevice { + pub fn new( + exit_evt: EventFd, + reset_evt: EventFd, + vcpus_kill_signalled: Arc, + ) -> AcpiShutdownDevice { AcpiShutdownDevice { exit_evt, reset_evt, + vcpus_kill_signalled, } } } @@ -43,6 +51,13 @@ impl BusDevice for AcpiShutdownDevice { if let Err(e) = self.reset_evt.write(1) { error!("Error triggering ACPI reset event: {}", e); } + // Spin until we are sure the reset_evt has been handled and that when + // we return from the KVM_RUN we will exit rather than re-enter the guest. + while !self.vcpus_kill_signalled.load(Ordering::SeqCst) { + // This is more effective than thread::yield_now() at + // avoiding a priority inversion with the VMM thread + thread::sleep(std::time::Duration::from_millis(1)); + } } // The ACPI DSDT table specifies the S5 sleep state (shutdown) as value 5 const S5_SLEEP_VALUE: u8 = 5; @@ -53,6 +68,13 @@ impl BusDevice for AcpiShutdownDevice { if let Err(e) = self.exit_evt.write(1) { error!("Error triggering ACPI shutdown event: {}", e); } + // Spin until we are sure the reset_evt has been handled and that when + // we return from the KVM_RUN we will exit rather than re-enter the guest. + while !self.vcpus_kill_signalled.load(Ordering::SeqCst) { + // This is more effective than thread::yield_now() at + // avoiding a priority inversion with the VMM thread + thread::sleep(std::time::Duration::from_millis(1)); + } } None } diff --git a/devices/src/legacy/cmos.rs b/devices/src/legacy/cmos.rs index b6ac3bace..4ca331356 100644 --- a/devices/src/legacy/cmos.rs +++ b/devices/src/legacy/cmos.rs @@ -5,7 +5,9 @@ use libc::{clock_gettime, gmtime_r, timespec, tm, CLOCK_REALTIME}; use std::cmp::min; use std::mem; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Barrier}; +use std::thread; use vm_device::BusDevice; use vmm_sys_util::eventfd::EventFd; @@ -23,13 +25,19 @@ pub struct Cmos { index: u8, data: [u8; DATA_LEN], reset_evt: EventFd, + vcpus_kill_signalled: Arc, } impl Cmos { /// Constructs a CMOS/RTC device with initial data. /// `mem_below_4g` is the size of memory in bytes below the 32-bit gap. /// `mem_above_4g` is the size of memory in bytes above the 32-bit gap. - pub fn new(mem_below_4g: u64, mem_above_4g: u64, reset_evt: EventFd) -> Cmos { + pub fn new( + mem_below_4g: u64, + mem_above_4g: u64, + reset_evt: EventFd, + vcpus_kill_signalled: Arc, + ) -> Cmos { let mut data = [0u8; DATA_LEN]; // Extended memory from 16 MB to 4 GB in units of 64 KB @@ -50,6 +58,7 @@ impl Cmos { index: 0, data, reset_evt, + vcpus_kill_signalled, } } } @@ -67,6 +76,13 @@ impl BusDevice for Cmos { if self.index == 0x8f && data[0] == 0 { info!("CMOS reset"); self.reset_evt.write(1).unwrap(); + // Spin until we are sure the reset_evt has been handled and that when + // we return from the KVM_RUN we will exit rather than re-enter the guest. + while !self.vcpus_kill_signalled.load(Ordering::SeqCst) { + // This is more effective than thread::yield_now() at + // avoiding a priority inversion with the VMM thread + thread::sleep(std::time::Duration::from_millis(1)); + } } else { self.data[(self.index & INDEX_MASK) as usize] = data[0] } diff --git a/devices/src/legacy/i8042.rs b/devices/src/legacy/i8042.rs index 5192bbde6..6edfc73a3 100644 --- a/devices/src/legacy/i8042.rs +++ b/devices/src/legacy/i8042.rs @@ -2,19 +2,27 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE-BSD-3-Clause file. -use std::sync::{Arc, Barrier}; +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, Barrier, +}; +use std::thread; use vm_device::BusDevice; use vmm_sys_util::eventfd::EventFd; /// A i8042 PS/2 controller that emulates just enough to shutdown the machine. pub struct I8042Device { reset_evt: EventFd, + vcpus_kill_signalled: Arc, } impl I8042Device { /// Constructs a i8042 device that will signal the given event when the guest requests it. - pub fn new(reset_evt: EventFd) -> I8042Device { - I8042Device { reset_evt } + pub fn new(reset_evt: EventFd, vcpus_kill_signalled: Arc) -> I8042Device { + I8042Device { + reset_evt, + vcpus_kill_signalled, + } } } @@ -38,6 +46,13 @@ impl BusDevice for I8042Device { if let Err(e) = self.reset_evt.write(1) { error!("Error triggering i8042 reset event: {}", e); } + // Spin until we are sure the reset_evt has been handled and that when + // we return from the KVM_RUN we will exit rather than re-enter the guest. + while !self.vcpus_kill_signalled.load(Ordering::SeqCst) { + // This is more effective than thread::yield_now() at + // avoiding a priority inversion with the VMM thread + thread::sleep(std::time::Duration::from_millis(1)); + } } None diff --git a/fuzz/fuzz_targets/cmos.rs b/fuzz/fuzz_targets/cmos.rs index 5afcfa227..fba49db11 100644 --- a/fuzz/fuzz_targets/cmos.rs +++ b/fuzz/fuzz_targets/cmos.rs @@ -6,6 +6,8 @@ use devices::legacy::Cmos; use libc::EFD_NONBLOCK; use libfuzzer_sys::fuzz_target; +use std::sync::atomic::AtomicBool; +use std::sync::Arc; use vm_device::BusDevice; use vmm_sys_util::eventfd::EventFd; @@ -25,6 +27,7 @@ fuzz_target!(|bytes| { u64::from_le_bytes(below_4g), u64::from_le_bytes(above_4g), EventFd::new(EFD_NONBLOCK).unwrap(), + Arc::new(AtomicBool::default()), ); let mut i = 16; diff --git a/vmm/src/cpu.rs b/vmm/src/cpu.rs index e98ea8dec..95cfa18e2 100644 --- a/vmm/src/cpu.rs +++ b/vmm/src/cpu.rs @@ -1725,6 +1725,10 @@ impl CpuManager { ) { self.interrupt_controller = Some(interrupt_controller); } + + pub(crate) fn vcpus_kill_signalled(&self) -> &Arc { + &self.vcpus_kill_signalled + } } struct Cpu { diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 1549b7468..038d65838 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -1485,8 +1485,16 @@ impl DeviceManager { reset_evt: EventFd, exit_evt: EventFd, ) -> DeviceManagerResult>>> { + let vcpus_kill_signalled = self + .cpu_manager + .lock() + .unwrap() + .vcpus_kill_signalled() + .clone(); let shutdown_device = Arc::new(Mutex::new(devices::AcpiShutdownDevice::new( - exit_evt, reset_evt, + exit_evt, + reset_evt, + vcpus_kill_signalled, ))); self.bus_devices @@ -1585,9 +1593,16 @@ impl DeviceManager { #[cfg(target_arch = "x86_64")] fn add_legacy_devices(&mut self, reset_evt: EventFd) -> DeviceManagerResult<()> { + let vcpus_kill_signalled = self + .cpu_manager + .lock() + .unwrap() + .vcpus_kill_signalled() + .clone(); // Add a shutdown device (i8042) let i8042 = Arc::new(Mutex::new(devices::legacy::I8042Device::new( reset_evt.try_clone().unwrap(), + vcpus_kill_signalled.clone(), ))); self.bus_devices @@ -1615,6 +1630,7 @@ impl DeviceManager { mem_below_4g, mem_above_4g, reset_evt, + vcpus_kill_signalled, ))); self.bus_devices