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>

1 files changed, 70 insertions, 27 deletions

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 @@ 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<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 @@ 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