From 1f9c1bb88b68961c2a0167993f6df401d49a2e9f Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Thu, 20 Feb 2020 14:55:24 -0800 Subject: [PATCH] devices: pci: add error handling for msix_enable This more gracefully handles failure of msix_enable; in particular, if it fails, the self.enabled state is returned to false so that future device operations won't try to access uninitialized parts of self.irq_vec. In addition, the AddMsiRoute response is now checked (previously, it was just ignored), and errors are bubbled up via MsixError rather than just printing a message. BUG=chromium:1041351 TEST=Boot Crostini on nami Change-Id: I9999f149817bc9f49176487446b52e74fb8be9a9 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2067964 Reviewed-by: Dylan Reid Tested-by: kokoro Commit-Queue: Daniel Verkamp --- devices/src/pci/msix.rs | 97 +++++++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 27 deletions(-) diff --git a/devices/src/pci/msix.rs b/devices/src/pci/msix.rs index b1dc672f03..6c79b4afe0 100644 --- a/devices/src/pci/msix.rs +++ b/devices/src/pci/msix.rs @@ -3,11 +3,12 @@ // found in the LICENSE file. use crate::pci::{PciCapability, PciCapabilityID}; -use msg_socket::{MsgReceiver, MsgSender}; +use msg_socket::{MsgError, MsgReceiver, MsgSender}; use std::convert::TryInto; +use std::fmt::{self, Display}; use std::os::unix::io::{AsRawFd, RawFd}; use std::sync::Arc; -use sys_util::{error, EventFd}; +use sys_util::{error, Error as SysError, EventFd}; use vm_control::{MaybeOwnedFd, VmIrqRequest, VmIrqRequestSocket, VmIrqResponse}; use data_model::DataInit; @@ -60,6 +61,34 @@ pub struct MsixConfig { msix_num: u16, } +enum MsixError { + AddMsiRoute(SysError), + AddMsiRouteRecv(MsgError), + AddMsiRouteSend(MsgError), + AllocateOneMsi(SysError), + AllocateOneMsiRecv(MsgError), + AllocateOneMsiSend(MsgError), +} + +impl Display for MsixError { + #[remain::check] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use self::MsixError::*; + + #[sorted] + match self { + AddMsiRoute(e) => write!(f, "AddMsiRoute failed: {}", e), + AddMsiRouteRecv(e) => write!(f, "failed to receive AddMsiRoute response: {}", e), + AddMsiRouteSend(e) => write!(f, "failed to send AddMsiRoute request: {}", e), + AllocateOneMsi(e) => write!(f, "AllocateOneMsi failed: {}", e), + AllocateOneMsiRecv(e) => write!(f, "failed to receive AllocateOneMsi response: {}", e), + AllocateOneMsiSend(e) => write!(f, "failed to send AllocateOneMsi request: {}", e), + } + } +} + +type MsixResult = std::result::Result; + impl MsixConfig { pub fn new(msix_vectors: u16, vm_socket: Arc) -> Self { assert!(msix_vectors <= MAX_MSIX_VECTORS_PER_DEVICE); @@ -128,7 +157,10 @@ impl MsixConfig { self.enabled = (reg & MSIX_ENABLE_BIT) == MSIX_ENABLE_BIT; if !old_enabled && self.enabled { - self.msix_enable(); + if let Err(e) = self.msix_enable() { + error!("failed to enable MSI-X: {}", e); + self.enabled = false; + } } // If the Function Mask bit was set, and has just been cleared, it's @@ -150,7 +182,7 @@ impl MsixConfig { } } - fn add_msi_route(&self, index: u16, gsi: u32) { + fn add_msi_route(&self, index: u16, gsi: u32) -> MsixResult<()> { let mut data: [u8; 8] = [0, 0, 0, 0, 0, 0, 0, 0]; self.read_msix_table((index * 16).into(), data.as_mut()); let msi_address: u64 = u64::from_le_bytes(data); @@ -159,44 +191,53 @@ impl MsixConfig { let msi_data: u32 = u32::from_le_bytes(data); if msi_address == 0 { - return; + return Ok(()); } - if let Err(e) = self.msi_device_socket.send(&VmIrqRequest::AddMsiRoute { - gsi, - msi_address, - msi_data, - }) { - error!("failed to send AddMsiRoute request: {:?}", e); - return; - } - if self.msi_device_socket.recv().is_err() { - error!("Faied to receive AddMsiRoute Response"); + self.msi_device_socket + .send(&VmIrqRequest::AddMsiRoute { + gsi, + msi_address, + msi_data, + }) + .map_err(MsixError::AddMsiRouteSend)?; + if let VmIrqResponse::Err(e) = self + .msi_device_socket + .recv() + .map_err(MsixError::AddMsiRouteRecv)? + { + return Err(MsixError::AddMsiRoute(e)); } + Ok(()) } - fn msix_enable(&mut self) { + fn msix_enable(&mut self) -> MsixResult<()> { self.irq_vec.clear(); for i in 0..self.msix_num { let irqfd = EventFd::new().unwrap(); - if let Err(e) = self.msi_device_socket.send(&VmIrqRequest::AllocateOneMsi { - irqfd: MaybeOwnedFd::Borrowed(irqfd.as_raw_fd()), - }) { - error!("failed to send AllocateOneMsi request: {:?}", e); - continue; - } + self.msi_device_socket + .send(&VmIrqRequest::AllocateOneMsi { + irqfd: MaybeOwnedFd::Borrowed(irqfd.as_raw_fd()), + }) + .map_err(MsixError::AllocateOneMsiSend)?; let irq_num: u32; - match self.msi_device_socket.recv() { - Ok(VmIrqResponse::AllocateOneMsi { gsi }) => irq_num = gsi, - _ => continue, + match self + .msi_device_socket + .recv() + .map_err(MsixError::AllocateOneMsiRecv)? + { + VmIrqResponse::AllocateOneMsi { gsi } => irq_num = gsi, + VmIrqResponse::Err(e) => return Err(MsixError::AllocateOneMsi(e)), + _ => unreachable!(), } self.irq_vec.push(IrqfdGsi { irqfd, gsi: irq_num, }); - self.add_msi_route(i, irq_num); + self.add_msi_route(i, irq_num)?; } + Ok(()) } /// Read MSI-X table @@ -305,7 +346,9 @@ impl MsixConfig { || old_entry.msg_data != new_entry.msg_data) { let irq_num = self.irq_vec[index].gsi; - self.add_msi_route(index as u16, irq_num); + if let Err(e) = self.add_msi_route(index as u16, irq_num) { + error!("add_msi_route failed: {}", e); + } } // After the MSI-X table entry has been updated, it is necessary to