Merge branch 'main' into update-devctr-v89

Author: JamesC1305

CHANGELOG.md

@@ -32,6 +32,12 @@ and this project adheres to
   HID (Hardware ID) of VMGenID device so that it aligns with the upstream Linux
   kernel. This caused the driver not to be bound correctly to the device prior
   to Linux kernel 6.10.
+- [#5764](https://github.com/firecracker-microvm/firecracker/pull/5764): Fixed a
+  bug that caused the guest UART driver to get stuck and stop transmitting after
+  snapshot restore. The bug was triggered by taking a snapshot while a serial
+  transmission was taking place. On restore the driver would wait for a TX
+  interrupt that would never arrive and no output would appear in the serial
+  console.
 - [#5780](https://github.com/firecracker-microvm/firecracker/pull/5780): Fixed
   missing `/sys/devices/system/cpu/cpu*/cache/*` in aarch64 guests when running
   on host kernels >= 6.3 with guest kernels >= 6.1.156.

src/vmm/src/builder.rs

@@ -411,8 +411,6 @@ pub enum BuildMicrovmFromSnapshotError {
     VmUpdateConfig(#[from] MachineConfigError),
     /// Failed to restore MMIO device: {0}
     RestoreMmioDevice(#[from] MicrovmStateError),
-    /// Failed to emulate MMIO serial: {0}
-    EmulateSerialInit(#[from] crate::EmulateSerialInitError),
     /// Failed to start vCPUs as no vCPU seccomp filter found.
     MissingVcpuSeccompFilters,
     /// Failed to start vCPUs: {0}

src/vmm/src/device_manager/mod.rs

@@ -21,15 +21,16 @@ use pci_mngr::{PciDevices, PciDevicesConstructorArgs, PciManagerError};
 use persist::MMIODevManagerConstructorArgs;
 use serde::{Deserialize, Serialize};
 use utils::time::TimestampUs;
+use vm_superio::serial;
 use vmm_sys_util::eventfd::EventFd;
 
 use crate::device_manager::acpi::ACPIDeviceError;
 #[cfg(target_arch = "x86_64")]
 use crate::devices::legacy::I8042Device;
 #[cfg(target_arch = "aarch64")]
 use crate::devices::legacy::RTCDevice;
+use crate::devices::legacy::SerialDevice;
 use crate::devices::legacy::serial::SerialOut;
-use crate::devices::legacy::{IER_RDA_BIT, IER_RDA_OFFSET, SerialDevice};
 use crate::devices::pseudo::BootTimer;
 use crate::devices::virtio::ActivateError;
 use crate::devices::virtio::balloon::BalloonError;
@@ -47,7 +48,7 @@ use crate::utils::open_file_nonblock;
 use crate::vmm_config::mmds::MmdsConfigError;
 use crate::vstate::bus::BusError;
 use crate::vstate::memory::GuestMemoryMmap;
-use crate::{EmulateSerialInitError, EventManager, Vm};
+use crate::{EventManager, Vm};
 
 /// ACPI device manager.
 pub mod acpi;
@@ -132,6 +133,7 @@ impl DeviceManager {
     fn setup_serial_device(
         event_manager: &mut EventManager,
         output: Option<&PathBuf>,
+        state: Option<&serial::SerialState>,
     ) -> Result<Arc<Mutex<SerialDevice>>, std::io::Error> {
         let (serial_in, serial_out) = match output {
             Some(path) => (None, open_file_nonblock(path).map(SerialOut::File)?),
@@ -142,20 +144,44 @@ impl DeviceManager {
             }
         };
 
-        let serial = Arc::new(Mutex::new(SerialDevice::new(serial_in, serial_out)?));
+        let serial = Arc::new(Mutex::new(SerialDevice::new(serial_in, serial_out, state)?));
         event_manager.add_subscriber(serial.clone());
         Ok(serial)
     }
 
+    fn serial_state(&self) -> Option<persist::SerialState> {
+        #[cfg(target_arch = "aarch64")]
+        {
+            self.mmio_devices.serial.as_ref().map(|device| {
+                let locked = device.inner.lock().expect("Poisoned lock");
+                locked.serial.state().into()
+            })
+        }
+
+        #[cfg(target_arch = "x86_64")]
+        {
+            Some(
+                self.legacy_devices
+                    .stdio_serial
+                    .lock()
+                    .expect("Poisoned lock")
+                    .serial
+                    .state()
+                    .into(),
+            )
+        }
+    }
+
     #[cfg(target_arch = "x86_64")]
     fn create_legacy_devices(
         event_manager: &mut EventManager,
         vcpus_exit_evt: &EventFd,
         vm: &Vm,
         serial_output: Option<&PathBuf>,
+        serial_state: Option<&serial::SerialState>,
     ) -> Result<PortIODeviceManager, DeviceManagerCreateError> {
         // Create serial device
-        let serial = Self::setup_serial_device(event_manager, serial_output)?;
+        let serial = Self::setup_serial_device(event_manager, serial_output, serial_state)?;
         let reset_evt = vcpus_exit_evt
             .try_clone()
             .map_err(DeviceManagerCreateError::EventFd)?;
@@ -180,7 +206,7 @@ impl DeviceManager {
     ) -> Result<Self, DeviceManagerCreateError> {
         #[cfg(target_arch = "x86_64")]
         let legacy_devices =
-            Self::create_legacy_devices(event_manager, vcpus_exit_evt, vm, serial_output)?;
+            Self::create_legacy_devices(event_manager, vcpus_exit_evt, vm, serial_output, None)?;
 
         Ok(DeviceManager {
             mmio_devices: MMIODeviceManager::new(),
@@ -276,7 +302,7 @@ impl DeviceManager {
             .contains("console=");
 
         if cmdline_contains_console {
-            let serial = Self::setup_serial_device(event_manager, serial_out_path)?;
+            let serial = Self::setup_serial_device(event_manager, serial_out_path, None)?;
             self.mmio_devices.register_mmio_serial(vm, serial, None)?;
             self.mmio_devices.add_mmio_serial_to_cmdline(cmdline)?;
         }
@@ -419,6 +445,8 @@ pub struct DevicesState {
     pub acpi_state: persist::ACPIDeviceManagerState,
     /// PCI devices state
     pub pci_state: pci_mngr::PciDevicesState,
+    /// Serial device state
+    pub serial_state: Option<persist::SerialState>,
 }
 
 /// Errors for (de)serialization of the devices.
@@ -466,8 +494,6 @@ pub enum DeviceManagerPersistError {
     AcpiRestore(#[from] ACPIDeviceError),
     /// Error restoring PCI devices: {0}
     PciRestore(DevicePersistError),
-    /// Error resetting serial console: {0}
-    SerialRestore(#[from] EmulateSerialInitError),
     /// Error inserting device in bus: {0}
     Bus(#[from] BusError),
     /// Error creating DeviceManager: {0}
@@ -504,6 +530,7 @@ impl<'a> Persist<'a> for DeviceManager {
             mmio_state: self.mmio_devices.save(),
             acpi_state: self.acpi_devices.save(),
             pci_state: self.pci_devices.save(),
+            serial_state: self.serial_state(),
         }
     }
 
@@ -513,11 +540,15 @@ impl<'a> Persist<'a> for DeviceManager {
     ) -> Result<Self, Self::Error> {
         // Setup legacy devices in case of x86
         #[cfg(target_arch = "x86_64")]
+        let serial_state: Option<vm_superio::serial::SerialState> =
+            state.serial_state.as_ref().map(Into::into);
+        #[cfg(target_arch = "x86_64")]
         let legacy_devices = Self::create_legacy_devices(
             constructor_args.event_manager,
             constructor_args.vcpus_exit_evt,
             constructor_args.vm,
             constructor_args.vm_resources.serial_out_path.as_ref(),
+            serial_state.as_ref(),
         )?;
 
         // Restore MMIO devices
@@ -527,6 +558,7 @@ impl<'a> Persist<'a> for DeviceManager {
             event_manager: constructor_args.event_manager,
             vm_resources: constructor_args.vm_resources,
             instance_id: constructor_args.instance_id,
+            serial_state: state.serial_state.as_ref(),
         };
         let mmio_devices = MMIODeviceManager::restore(mmio_ctor_args, &state.mmio_state)
             .map_err(DeviceManagerPersistError::MmioRestore)?;
@@ -545,59 +577,13 @@ impl<'a> Persist<'a> for DeviceManager {
         let pci_devices = PciDevices::restore(pci_ctor_args, &state.pci_state)
             .map_err(DeviceManagerPersistError::PciRestore)?;
 
-        let device_manager = DeviceManager {
+        Ok(DeviceManager {
             mmio_devices,
             #[cfg(target_arch = "x86_64")]
             legacy_devices,
             acpi_devices,
             pci_devices,
-        };
-
-        // Restore serial.
-        // We need to do that after we restore mmio devices, otherwise it won't succeed in Aarch64
-        device_manager.emulate_serial_init()?;
-
-        Ok(device_manager)
-    }
-}
-
-impl DeviceManager {
-    /// Sets RDA bit in serial console
-    pub fn emulate_serial_init(&self) -> Result<(), EmulateSerialInitError> {
-        // When restoring from a previously saved state, there is no serial
-        // driver initialization, therefore the RDA (Received Data Available)
-        // interrupt is not enabled. Because of that, the driver won't get
-        // notified of any bytes that we send to the guest. The clean solution
-        // would be to save the whole serial device state when we do the vm
-        // serialization. For now we set that bit manually
-
-        #[cfg(target_arch = "aarch64")]
-        {
-            if let Some(device) = &self.mmio_devices.serial {
-                let mut device_locked = device.inner.lock().expect("Poisoned lock");
-
-                device_locked
-                    .serial
-                    .write(IER_RDA_OFFSET, IER_RDA_BIT)
-                    .map_err(|_| EmulateSerialInitError(std::io::Error::last_os_error()))?;
-            }
-            Ok(())
-        }
-
-        #[cfg(target_arch = "x86_64")]
-        {
-            let mut serial = self
-                .legacy_devices
-                .stdio_serial
-                .lock()
-                .expect("Poisoned lock");
-
-            serial
-                .serial
-                .write(IER_RDA_OFFSET, IER_RDA_BIT)
-                .map_err(|_| EmulateSerialInitError(std::io::Error::last_os_error()))?;
-            Ok(())
-        }
+        })
     }
 }
 
@@ -622,7 +608,7 @@ pub(crate) mod tests {
         #[cfg(target_arch = "x86_64")]
         let legacy_devices = PortIODeviceManager {
             stdio_serial: Arc::new(Mutex::new(
-                SerialDevice::new(None, SerialOut::Sink).unwrap(),
+                SerialDevice::new(None, SerialOut::Sink, None).unwrap(),
             )),
             i8042: Arc::new(Mutex::new(
                 I8042Device::new(EventFd::new(libc::EFD_NONBLOCK).unwrap()).unwrap(),

src/vmm/src/device_manager/persist.rs

@@ -45,6 +45,54 @@ use crate::vmm_config::memory_hotplug::MemoryHotplugConfig;
 use crate::vstate::memory::GuestMemoryMmap;
 use crate::{EventManager, Vm};
 
+#[derive(Debug, Default, Clone, Serialize, Deserialize)]
+pub struct SerialState {
+    pub baud_divisor_low: u8,
+    pub baud_divisor_high: u8,
+    pub interrupt_enable: u8,
+    pub interrupt_identification: u8,
+    pub line_control: u8,
+    pub line_status: u8,
+    pub modem_control: u8,
+    pub modem_status: u8,
+    pub scratch: u8,
+    pub in_buffer: Vec<u8>,
+}
+
+impl From<vm_superio::serial::SerialState> for SerialState {
+    fn from(s: vm_superio::serial::SerialState) -> Self {
+        Self {
+            baud_divisor_low: s.baud_divisor_low,
+            baud_divisor_high: s.baud_divisor_high,
+            interrupt_enable: s.interrupt_enable,
+            interrupt_identification: s.interrupt_identification,
+            line_control: s.line_control,
+            line_status: s.line_status,
+            modem_control: s.modem_control,
+            modem_status: s.modem_status,
+            scratch: s.scratch,
+            in_buffer: s.in_buffer,
+        }
+    }
+}
+
+impl From<&SerialState> for vm_superio::serial::SerialState {
+    fn from(s: &SerialState) -> Self {
+        Self {
+            baud_divisor_low: s.baud_divisor_low,
+            baud_divisor_high: s.baud_divisor_high,
+            interrupt_enable: s.interrupt_enable,
+            interrupt_identification: s.interrupt_identification,
+            line_control: s.line_control,
+            line_status: s.line_status,
+            modem_control: s.modem_control,
+            modem_status: s.modem_status,
+            scratch: s.scratch,
+            in_buffer: s.in_buffer.clone(),
+        }
+    }
+}
+
 /// Holds the state of a MMIO VirtIO device
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct VirtioDeviceState<T> {
@@ -104,6 +152,7 @@ pub struct MMIODevManagerConstructorArgs<'a> {
     pub event_manager: &'a mut EventManager,
     pub vm_resources: &'a mut VmResources,
     pub instance_id: &'a str,
+    pub serial_state: Option<&'a SerialState>,
 }
 impl fmt::Debug for MMIODevManagerConstructorArgs<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -318,9 +367,12 @@ impl<'a> Persist<'a> for MMIODeviceManager {
         {
             for state in &state.legacy_devices {
                 if state.type_ == DeviceType::Serial {
+                    let serial_state: Option<vm_superio::serial::SerialState> =
+                        constructor_args.serial_state.map(Into::into);
                     let serial = crate::DeviceManager::setup_serial_device(
                         constructor_args.event_manager,
                         constructor_args.vm_resources.serial_out_path.as_ref(),
+                        serial_state.as_ref(),
                     )?;
 
                     dev_manager.register_mmio_serial(vm, serial, Some(state.device_info))?;
@@ -719,6 +771,7 @@ mod tests {
             event_manager: &mut event_manager,
             vm_resources,
             instance_id: "microvm-id",
+            serial_state: None,
         };
         let _restored_dev_manager =
             MMIODeviceManager::restore(restore_args, &device_manager_state.mmio_state).unwrap();

src/vmm/src/devices/legacy/mod.rs

@@ -22,9 +22,7 @@ use vmm_sys_util::eventfd::EventFd;
 pub use self::i8042::{I8042Device, I8042Error as I8042DeviceError};
 #[cfg(target_arch = "aarch64")]
 pub use self::rtc_pl031::RTCDevice;
-pub use self::serial::{
-    IER_RDA_BIT, IER_RDA_OFFSET, SerialDevice, SerialEventsWrapper, SerialWrapper,
-};
+pub use self::serial::{SerialDevice, SerialEventsWrapper, SerialWrapper};
 
 /// Wrapper for implementing the trigger functionality for `EventFd`.
 ///