refactor(test): Eliminate mocking/cfg(not(test)) from vcpu/mod.rs

Separate the call to `KVM_RUN` from the handling of the result value.
This makes the handling of the `VcpuExit` unit-testable without needing
to hack in `cfg(not(test))` code that compiles out the `KVM_RUN` call at
compile time.

Co-Authored-By: Egor Lazarchuk <[email protected]>
Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/vmm/src/vstate/vcpu/mod.rs

@@ -8,8 +8,6 @@
 use std::cell::Cell;
 use std::sync::atomic::{fence, Ordering};
 use std::sync::mpsc::{channel, Receiver, Sender, TryRecvError};
-#[cfg(test)]
-use std::sync::Mutex;
 use std::sync::{Arc, Barrier};
 use std::{fmt, io, thread};
 
@@ -104,10 +102,6 @@ pub struct Vcpu {
     response_receiver: Option<Receiver<VcpuResponse>>,
     /// The transmitting end of the responses channel owned by the vcpu side.
     response_sender: Sender<VcpuResponse>,
-
-    /// Exit reason used to test run_emulation function.
-    #[cfg(test)]
-    test_vcpu_exit_reason: Mutex<Option<Result<VcpuExit<'static>, errno::Error>>>,
 }
 
 impl Vcpu {
@@ -218,8 +212,6 @@ impl Vcpu {
             response_sender,
             kvm_vcpu,
             fd,
-            #[cfg(test)]
-            test_vcpu_exit_reason: Mutex::new(None),
         })
     }
 
@@ -449,15 +441,6 @@ impl Vcpu {
         StateMachine::finish()
     }
 
-    #[cfg(not(test))]
-    /// Calls `KVM_RUN` with this [`Vcpu`]'s underlying file descriptor.
-    ///
-    /// Blocks until a `VM_EXIT` is received, in which case this function returns a [`VcpuExit`]
-    /// containing the reason.
-    pub fn emulate(&self) -> Result<VcpuExit, errno::Error> {
-        self.fd.run()
-    }
-
     /// Runs the vCPU in KVM context and handles the kvm exit reason.
     ///
     /// Returns error or enum specifying whether emulation was handled or interrupted.
@@ -467,18 +450,36 @@ impl Vcpu {
             self.fd.set_kvm_immediate_exit(0);
             return Ok(VcpuEmulation::Interrupted);
         }
-        match self.emulate() {
+
+        match self.fd.run() {
+            Err(ref err) if err.errno() == libc::EINTR => {
+                self.fd.set_kvm_immediate_exit(0);
+                // Notify that this KVM_RUN was interrupted.
+                Ok(VcpuEmulation::Interrupted)
+            }
+            emulation_result => self.kvm_vcpu.handle_kvm_exit(emulation_result),
+        }
+    }
+}
+
+impl KvmVcpu {
+    /// Handle the return value of a call to [`VcpuFd::run`] and update our emulation accordingly
+    pub fn handle_kvm_exit(
+        &mut self,
+        emulation_result: Result<VcpuExit, errno::Error>,
+    ) -> Result<VcpuEmulation, VcpuError> {
+        match emulation_result {
             Ok(run) => match run {
                 VcpuExit::MmioRead(addr, data) => {
-                    if let Some(mmio_bus) = &self.kvm_vcpu.mmio_bus {
+                    if let Some(mmio_bus) = &self.mmio_bus {
                         let _metric = METRICS.vcpu.exit_mmio_read_agg.record_latency_metrics();
                         mmio_bus.read(addr, data);
                         METRICS.vcpu.exit_mmio_read.inc();
                     }
                     Ok(VcpuEmulation::Handled)
                 }
                 VcpuExit::MmioWrite(addr, data) => {
-                    if let Some(mmio_bus) = &self.kvm_vcpu.mmio_bus {
+                    if let Some(mmio_bus) = &self.mmio_bus {
                         let _metric = METRICS.vcpu.exit_mmio_write_agg.record_latency_metrics();
                         mmio_bus.write(addr, data);
                         METRICS.vcpu.exit_mmio_write.inc();
@@ -538,36 +539,27 @@ impl Vcpu {
                 },
                 arch_specific_reason => {
                     // run specific architecture emulation.
-                    self.kvm_vcpu.run_arch_emulation(arch_specific_reason)
+                    self.run_arch_emulation(arch_specific_reason)
                 }
             },
             // The unwrap on raw_os_error can only fail if we have a logic
             // error in our code in which case it is better to panic.
-            Err(ref err) => {
-                match err.errno() {
-                    libc::EAGAIN => Ok(VcpuEmulation::Handled),
-                    libc::EINTR => {
-                        self.fd.set_kvm_immediate_exit(0);
-                        // Notify that this KVM_RUN was interrupted.
-                        Ok(VcpuEmulation::Interrupted)
-                    }
-                    libc::ENOSYS => {
-                        METRICS.vcpu.failures.inc();
-                        error!(
-                            "Received ENOSYS error because KVM failed to emulate an instruction."
-                        );
-                        Err(VcpuError::FaultyKvmExit(
-                            "Received ENOSYS error because KVM failed to emulate an instruction."
-                                .to_string(),
-                        ))
-                    }
-                    _ => {
-                        METRICS.vcpu.failures.inc();
-                        error!("Failure during vcpu run: {}", err);
-                        Err(VcpuError::FaultyKvmExit(format!("{}", err)))
-                    }
+            Err(ref err) => match err.errno() {
+                libc::EAGAIN => Ok(VcpuEmulation::Handled),
+                libc::ENOSYS => {
+                    METRICS.vcpu.failures.inc();
+                    error!("Received ENOSYS error because KVM failed to emulate an instruction.");
+                    Err(VcpuError::FaultyKvmExit(
+                        "Received ENOSYS error because KVM failed to emulate an instruction."
+                            .to_string(),
+                    ))
                 }
-            }
+                _ => {
+                    METRICS.vcpu.failures.inc();
+                    error!("Failure during vcpu run: {}", err);
+                    Err(VcpuError::FaultyKvmExit(format!("{}", err)))
+                }
+            },
         }
     }
 }
@@ -730,29 +722,16 @@ pub mod tests {
     use crate::vstate::vm::Vm;
     use crate::RECV_TIMEOUT_SEC;
 
-    impl Vcpu {
-        pub fn emulate(&self) -> Result<VcpuExit, errno::Error> {
-            self.test_vcpu_exit_reason
-                .lock()
-                .unwrap()
-                .take()
-                .unwrap_or_else(|| Err(errno::Error::new(libc::SIGILL)))
-        }
-    }
-
     #[test]
-    fn test_run_emulation() {
+    fn test_handle_kvm_exit() {
         let (_vm, mut vcpu, _vm_mem) = setup_vcpu(0x1000);
-        vcpu.test_vcpu_exit_reason = Mutex::new(Some(Ok(VcpuExit::Hlt)));
-        let res = vcpu.run_emulation();
+        let res = vcpu.kvm_vcpu.handle_kvm_exit(Ok(VcpuExit::Hlt));
         assert_eq!(res.unwrap(), VcpuEmulation::Stopped);
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::Shutdown));
-        let res = vcpu.run_emulation();
+        let res = vcpu.kvm_vcpu.handle_kvm_exit(Ok(VcpuExit::Shutdown));
         assert_eq!(res.unwrap(), VcpuEmulation::Stopped);
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::FailEntry(0, 0)));
-        let res = vcpu.run_emulation();
+        let res = vcpu.kvm_vcpu.handle_kvm_exit(Ok(VcpuExit::FailEntry(0, 0)));
         assert_eq!(
             format!("{:?}", res.unwrap_err()),
             format!(
@@ -761,8 +740,7 @@ pub mod tests {
             )
         );
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::InternalError));
-        let res = vcpu.run_emulation();
+        let res = vcpu.kvm_vcpu.handle_kvm_exit(Ok(VcpuExit::InternalError));
         assert_eq!(
             format!("{:?}", res.unwrap_err()),
             format!(
@@ -771,16 +749,19 @@ pub mod tests {
             )
         );
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::SystemEvent(2, &[])));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Ok(VcpuExit::SystemEvent(2, &[])));
         assert_eq!(res.unwrap(), VcpuEmulation::Stopped);
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::SystemEvent(1, &[])));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Ok(VcpuExit::SystemEvent(1, &[])));
         assert_eq!(res.unwrap(), VcpuEmulation::Stopped);
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::SystemEvent(3, &[])));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Ok(VcpuExit::SystemEvent(3, &[])));
         assert_eq!(
             format!("{:?}", res.unwrap_err()),
             format!(
@@ -790,19 +771,20 @@ pub mod tests {
         );
 
         // Check what happens with an unhandled exit reason.
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::Unknown));
-        let res = vcpu.run_emulation();
+        let res = vcpu.kvm_vcpu.handle_kvm_exit(Ok(VcpuExit::Unknown));
         assert_eq!(
             res.unwrap_err().to_string(),
             "Unexpected kvm exit received: Unknown".to_string()
         );
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Err(errno::Error::new(libc::EAGAIN)));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Err(errno::Error::new(libc::EAGAIN)));
         assert_eq!(res.unwrap(), VcpuEmulation::Handled);
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Err(errno::Error::new(libc::ENOSYS)));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Err(errno::Error::new(libc::ENOSYS)));
         assert_eq!(
             format!("{:?}", res.unwrap_err()),
             format!(
@@ -814,12 +796,9 @@ pub mod tests {
             )
         );
 
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Err(errno::Error::new(libc::EINTR)));
-        let res = vcpu.run_emulation();
-        assert_eq!(res.unwrap(), VcpuEmulation::Interrupted);
-
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Err(errno::Error::new(libc::EINVAL)));
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Err(errno::Error::new(libc::EINVAL)));
         assert_eq!(
             format!("{:?}", res.unwrap_err()),
             format!(
@@ -833,20 +812,15 @@ pub mod tests {
         bus.insert(dummy, 0x10, 0x10).unwrap();
         vcpu.set_mmio_bus(bus);
         let addr = 0x10;
-        static mut DATA: [u8; 4] = [0, 0, 0, 0];
 
-        unsafe {
-            *(vcpu.test_vcpu_exit_reason.lock().unwrap()) =
-                Some(Ok(VcpuExit::MmioRead(addr, &mut DATA)));
-        }
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Ok(VcpuExit::MmioRead(addr, &mut [0, 0, 0, 0])));
         assert_eq!(res.unwrap(), VcpuEmulation::Handled);
 
-        unsafe {
-            *(vcpu.test_vcpu_exit_reason.lock().unwrap()) =
-                Some(Ok(VcpuExit::MmioWrite(addr, &DATA)));
-        }
-        let res = vcpu.run_emulation();
+        let res = vcpu
+            .kvm_vcpu
+            .handle_kvm_exit(Ok(VcpuExit::MmioWrite(addr, &[0, 0, 0, 0])));
         assert_eq!(res.unwrap(), VcpuEmulation::Handled);
     }
 
@@ -1083,7 +1057,6 @@ pub mod tests {
 
         vcpu.fd.set_kvm_immediate_exit(1);
         // Set a dummy value to be returned by the emulate call
-        *(vcpu.test_vcpu_exit_reason.lock().unwrap()) = Some(Ok(VcpuExit::Shutdown));
         let result = vcpu.run_emulation().expect("Failed to run emulation");
         assert_eq!(
             result,