From 1c5f1c397645802ab523a8bde35ccf27ddb7d6e7 Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Fri, 8 Aug 2025 11:24:19 +0200 Subject: [PATCH 1/2] fix(virtio): avoid panic on device activation failure According to the VirtIO spec section 2.1.2, when we enter a failed state we need to to set `DEVICE_NEEDS_RESET` status field and if `DRIVER_OK` is set we need to must send a device configuration change interrupt to the driver. In MMIO code we were trying to follow this logic, but we were trying to get the interrupt object from the device, which fails (and panics) because interrupts are only set in the device upon successful activation. In the PCI transport, instead, we were not doing this at all. The transport layers hold a reference to the interrupts at all times. So, add the missing logic to the PCI transport and use the transport layer reference in both transports to avoid panics. Signed-off-by: Babis Chalios --- src/vmm/src/devices/virtio/transport/mmio.rs | 5 +---- .../devices/virtio/transport/pci/device.rs | 21 ++++++++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/vmm/src/devices/virtio/transport/mmio.rs b/src/vmm/src/devices/virtio/transport/mmio.rs index 42cfe2b3ed7..8fbf058e318 100644 --- a/src/vmm/src/devices/virtio/transport/mmio.rs +++ b/src/vmm/src/devices/virtio/transport/mmio.rs @@ -190,10 +190,7 @@ impl MmioTransport { // Section 2.1.2 of the specification states that we need to send a device // configuration change interrupt - let _ = self - .locked_device() - .interrupt_trigger() - .trigger(VirtioInterruptType::Config); + let _ = self.interrupt.trigger(VirtioInterruptType::Config); error!("Failed to activate virtio device: {}", err) } diff --git a/src/vmm/src/devices/virtio/transport/pci/device.rs b/src/vmm/src/devices/virtio/transport/pci/device.rs index 0c4b275bb96..ba91163fe49 100644 --- a/src/vmm/src/devices/virtio/transport/pci/device.rs +++ b/src/vmm/src/devices/virtio/transport/pci/device.rs @@ -1029,15 +1029,22 @@ impl PciDevice for VirtioPciDevice { // Try and activate the device if the driver status has changed if self.needs_activation() { debug!("Activating device"); - self.virtio_device() + let interrupt = Arc::clone(self.virtio_interrupt.as_ref().unwrap()); + match self + .virtio_device() .lock() .unwrap() - .activate( - self.memory.clone(), - Arc::clone(self.virtio_interrupt.as_ref().unwrap()), - ) - .unwrap_or_else(|err| error!("Error activating device: {err:?}")); - self.device_activated.store(true, Ordering::SeqCst); + .activate(self.memory.clone(), interrupt.clone()) + { + Ok(()) => self.device_activated.store(true, Ordering::SeqCst), + Err(err) => { + error!("Error activating device: {err:?}"); + + // Section 2.1.2 of the specification states that we need to send a device + // configuration change interrupt + let _ = interrupt.trigger(VirtioInterruptType::Config); + } + } } else { debug!("Device doesn't need activation"); } From 56830e5efd147b2bf80f8a394250bfc03c36c1b6 Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Fri, 8 Aug 2025 11:35:39 +0200 Subject: [PATCH 2/2] fix(mmio): avoid locking multiple times in same code branch MMIO transport layer holds a Mutex to the VirtIO device. Within the logic that handles the handshake between the driver and the device, there were cases where we would take and release the lock multiple times within the same code branch. Change this to only take the lock once. Signed-off-by: Babis Chalios --- src/vmm/src/devices/virtio/transport/mmio.rs | 27 +++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/vmm/src/devices/virtio/transport/mmio.rs b/src/vmm/src/devices/virtio/transport/mmio.rs index 8fbf058e318..4964f837aca 100644 --- a/src/vmm/src/devices/virtio/transport/mmio.rs +++ b/src/vmm/src/devices/virtio/transport/mmio.rs @@ -179,12 +179,12 @@ impl MmioTransport { } DRIVER_OK if self.device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) => { self.device_status = status; - let device_activated = self.locked_device().is_activated(); + let mut locked_device = self.device.lock().expect("Poisoned lock"); + let device_activated = locked_device.is_activated(); if !device_activated { // temporary variable needed for borrow checker - let activate_result = self - .locked_device() - .activate(self.mem.clone(), self.interrupt.clone()); + let activate_result = + locked_device.activate(self.mem.clone(), self.interrupt.clone()); if let Err(err) = activate_result { self.device_status |= DEVICE_NEEDS_RESET; @@ -201,16 +201,19 @@ impl MmioTransport { self.device_status |= FAILED; } _ if status == 0 => { - if self.locked_device().is_activated() { - let mut device_status = self.device_status; - let reset_result = self.locked_device().reset(); - match reset_result { - Some((_interrupt_evt, mut _queue_evts)) => {} - None => { - device_status |= FAILED; + { + let mut locked_device = self.device.lock().expect("Poisoned lock"); + if locked_device.is_activated() { + let mut device_status = self.device_status; + let reset_result = locked_device.reset(); + match reset_result { + Some((_interrupt_evt, mut _queue_evts)) => {} + None => { + device_status |= FAILED; + } } + self.device_status = device_status; } - self.device_status = device_status; } // If the backend device driver doesn't support reset,