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 <dgreid@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
diff --git a/devices/src/pci/msix.rs b/devices/src/pci/msix.rs
index b1dc672..6c79b4a 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 @@
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<T> = std::result::Result<T, MsixError>;
+
impl MsixConfig {
pub fn new(msix_vectors: u16, vm_socket: Arc<VmIrqRequestSocket>) -> Self {
assert!(msix_vectors <= MAX_MSIX_VECTORS_PER_DEVICE);
@@ -128,7 +157,10 @@
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 @@
}
}
- 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 @@
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;
+ 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));
}
- if self.msi_device_socket.recv().is_err() {
- error!("Faied to receive AddMsiRoute Response");
- }
+ 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 @@
|| 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