PCI: Return results from pci setup functions

Enough failure cases have been added to `add_pci_bar` and
`add_pci_capabilities` that they should return unique errors instead of
an `Option`.

BUG=none
TEST=cargo test in devices

Signed-off-by: Dylan Reid <dgreid@chromium.org>
Change-Id: Ice2a06d2944011f95707f113f9d709da15c90cfe
Reviewed-on: https://chromium-review.googlesource.com/1497740
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>

4 files changed, 101 insertions, 31 deletions

diff --git a/devices/src/pci/ac97.rs b/devices/src/pci/ac97.rs
index 58fe5b3..fc120a4 100644
--- a/devices/src/pci/ac97.rs
+++ b/devices/src/pci/ac97.rs
@@ -147,7 +147,7 @@ impl PciDevice for Ac97Dev {
             .set_size(MIXER_REGS_SIZE);
         self.config_regs
             .add_pci_bar(&mixer_config)
-            .ok_or_else(|| pci_device::Error::IoRegistrationFailed(mixer_regs_addr))?;
+            .map_err(|e| pci_device::Error::IoRegistrationFailed(mixer_regs_addr, e))?;
         ranges.push((mixer_regs_addr, MIXER_REGS_SIZE));
 
         let master_regs_addr = resources
@@ -159,7 +159,7 @@ impl PciDevice for Ac97Dev {
             .set_size(MASTER_REGS_SIZE);
         self.config_regs
             .add_pci_bar(&master_config)
-            .ok_or_else(|| pci_device::Error::IoRegistrationFailed(master_regs_addr))?;
+            .map_err(|e| pci_device::Error::IoRegistrationFailed(master_regs_addr, e))?;
         ranges.push((master_regs_addr, MASTER_REGS_SIZE));
         Ok(ranges)
     }
diff --git a/devices/src/pci/pci_configuration.rs b/devices/src/pci/pci_configuration.rs
index d93ae03..ee46e38 100644
--- a/devices/src/pci/pci_configuration.rs
+++ b/devices/src/pci/pci_configuration.rs
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+use std::fmt::{self, Display};
+
 use pci::PciInterruptPin;
 
 // The number of 32bit registers in the config space, 256 bytes.
@@ -194,6 +196,44 @@ pub struct PciBarConfiguration {
     prefetchable: PciBarPrefetchable,
 }
 
+#[derive(Debug)]
+pub enum Error {
+    BarAddressInvalid(u64, u64),
+    BarInUse(usize),
+    BarInUse64(usize),
+    BarInvalid(usize),
+    BarInvalid64(usize),
+    BarSizeInvalid(u64),
+    CapabilityEmpty,
+    CapabilityLengthInvalid(usize),
+    CapabilitySpaceFull(usize),
+}
+pub type Result<T> = std::result::Result<T, Error>;
+
+impl std::error::Error for Error {}
+
+impl Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use self::Error::*;
+        match self {
+            BarAddressInvalid(a, s) => write!(f, "address {} size {} too big", a, s),
+            BarInUse(b) => write!(f, "bar {} already used", b),
+            BarInUse64(b) => write!(f, "64bit bar {} already used(requires two regs)", b),
+            BarInvalid(b) => write!(f, "bar {} invalid, max {}", b, NUM_BAR_REGS - 1),
+            BarInvalid64(b) => write!(
+                f,
+                "64bitbar {} invalid, requires two regs, max {}",
+                b,
+                NUM_BAR_REGS - 1
+            ),
+            BarSizeInvalid(s) => write!(f, "bar address {} not a power of two", s),
+            CapabilityEmpty => write!(f, "empty capabilities are invalid"),
+            CapabilityLengthInvalid(l) => write!(f, "Invalid capability length {}", l),
+            CapabilitySpaceFull(s) => write!(f, "capability of size {} doesn't fit", s),
+        }
+    }
+}
+
 impl PciConfiguration {
     pub fn new(
         vendor_id: u16,
@@ -304,37 +344,41 @@ impl PciConfiguration {
     /// report this region and size to the guest kernel.  Enforces a few constraints
     /// (i.e, region size must be power of two, register not already used). Returns 'None' on
     /// failure all, `Some(BarIndex)` on success.
-    pub fn add_pci_bar(&mut self, config: &PciBarConfiguration) -> Option<usize> {
+    pub fn add_pci_bar(&mut self, config: &PciBarConfiguration) -> Result<usize> {
         if self.bar_used[config.reg_idx] {
-            return None;
+            return Err(Error::BarInUse(config.reg_idx));
         }
 
         if config.size.count_ones() != 1 {
-            return None;
+            return Err(Error::BarSizeInvalid(config.size));
         }
 
         if config.reg_idx >= NUM_BAR_REGS {
-            return None;
+            return Err(Error::BarInvalid(config.reg_idx));
         }
 
         let bar_idx = BAR0_REG + config.reg_idx;
+        let end_addr = config
+            .addr
+            .checked_add(config.size)
+            .ok_or(Error::BarAddressInvalid(config.addr, config.size))?;
         match config.region_type {
             PciBarRegionType::Memory32BitRegion | PciBarRegionType::IORegion => {
-                if config.addr.checked_add(config.size)? > u64::from(u32::max_value()) {
-                    return None;
+                if end_addr > u64::from(u32::max_value()) {
+                    return Err(Error::BarAddressInvalid(config.addr, config.size));
                 }
             }
             PciBarRegionType::Memory64BitRegion => {
                 if config.reg_idx + 1 >= NUM_BAR_REGS {
-                    return None;
+                    return Err(Error::BarInvalid64(config.reg_idx));
                 }
 
-                if config.addr.checked_add(config.size)? > u64::max_value() {
-                    return None;
+                if end_addr > u64::max_value() {
+                    return Err(Error::BarAddressInvalid(config.addr, config.size));
                 }
 
                 if self.bar_used[config.reg_idx + 1] {
-                    return None;
+                    return Err(Error::BarInUse64(config.reg_idx));
                 }
 
                 self.registers[bar_idx + 1] = (config.addr >> 32) as u32;
@@ -354,7 +398,7 @@ impl PciConfiguration {
         self.registers[bar_idx] = ((config.addr as u32) & mask) | lower_bits;
         self.writable_bits[bar_idx] = !(config.size - 1) as u32;
         self.bar_used[config.reg_idx] = true;
-        Some(config.reg_idx)
+        Ok(config.reg_idx)
     }
 
     /// Returns the address of the given BAR region.
@@ -377,18 +421,25 @@ impl PciConfiguration {
     /// Adds the capability `cap_data` to the list of capabilities.
     /// `cap_data` should not include the two-byte PCI capability header (type, next);
     /// it will be generated automatically based on `cap_data.id()`.
-    pub fn add_capability(&mut self, cap_data: &PciCapability) -> Option<usize> {
-        let total_len = cap_data.bytes().len() + 2;
+    pub fn add_capability(&mut self, cap_data: &PciCapability) -> Result<usize> {
+        let total_len = cap_data
+            .bytes()
+            .len()
+            .checked_add(2)
+            .ok_or(Error::CapabilityLengthInvalid(cap_data.bytes().len()))?;
         // Check that the length is valid.
         if cap_data.bytes().is_empty() {
-            return None;
+            return Err(Error::CapabilityEmpty);
         }
         let (cap_offset, tail_offset) = match self.last_capability {
             Some((offset, len)) => (Self::next_dword(offset, len), offset + 1),
             None => (FIRST_CAPABILITY_OFFSET, CAPABILITY_LIST_HEAD_OFFSET),
         };
-        if cap_offset.checked_add(cap_data.bytes().len() + 2)? > CAPABILITY_MAX_OFFSET {
-            return None;
+        let end_offset = cap_offset
+            .checked_add(total_len)
+            .ok_or(Error::CapabilitySpaceFull(total_len))?;
+        if end_offset > CAPABILITY_MAX_OFFSET {
+            return Err(Error::CapabilitySpaceFull(total_len));
         }
         self.registers[STATUS_REG] |= STATUS_REG_CAPABILITIES_USED_MASK;
         self.write_byte_internal(tail_offset, cap_offset as u8, false);
@@ -398,7 +449,7 @@ impl PciConfiguration {
             self.write_byte_internal(cap_offset + i + 2, *byte, false);
         }
         self.last_capability = Some((cap_offset, total_len));
-        Some(cap_offset)
+        Ok(cap_offset)
     }
 
     // Find the next aligned offset after the one given.
diff --git a/devices/src/pci/pci_device.rs b/devices/src/pci/pci_device.rs
index 3b8b93e..d5bc741 100644
--- a/devices/src/pci/pci_device.rs
+++ b/devices/src/pci/pci_device.rs
@@ -9,7 +9,7 @@ use std::fmt::{self, Display};
 use std::os::unix::io::RawFd;
 
 use kvm::Datamatch;
-use pci::pci_configuration::PciConfiguration;
+use pci::pci_configuration::{self, PciConfiguration};
 use pci::PciInterruptPin;
 use resources::SystemAllocator;
 use sys_util::EventFd;
@@ -18,10 +18,12 @@ use BusDevice;
 
 #[derive(Debug)]
 pub enum Error {
+    /// Setup of the device capabilities failed.
+    CapabilitiesSetup(pci_configuration::Error),
     /// Allocating space for an IO BAR failed.
     IoAllocationFailed(u64),
     /// Registering an IO BAR failed.
-    IoRegistrationFailed(u64),
+    IoRegistrationFailed(u64, pci_configuration::Error),
 }
 pub type Result<T> = std::result::Result<T, Error>;
 
@@ -30,10 +32,13 @@ impl Display for Error {
         use self::Error::*;
 
         match self {
+            CapabilitiesSetup(e) => write!(f, "failed to add capability {}", e),
             IoAllocationFailed(size) => {
                 write!(f, "failed to allocate space for an IO BAR, size={}", size)
             }
-            IoRegistrationFailed(addr) => write!(f, "failed to register an IO BAR, addr={}", addr),
+            IoRegistrationFailed(addr, e) => {
+                write!(f, "failed to register an IO BAR, addr={} err={}", addr, e)
+            }
         }
     }
 }
diff --git a/devices/src/virtio/virtio_pci_device.rs b/devices/src/virtio/virtio_pci_device.rs
index e764569..bfff317 100644
--- a/devices/src/virtio/virtio_pci_device.rs
+++ b/devices/src/virtio/virtio_pci_device.rs
@@ -231,7 +231,10 @@ impl VirtioPciDevice {
         }
     }
 
-    fn add_pci_capabilities(&mut self, settings_bar: u8) {
+    fn add_pci_capabilities(
+        &mut self,
+        settings_bar: u8,
+    ) -> std::result::Result<(), PciDeviceError> {
         // Add pointers to the different configuration structures from the PCI capabilities.
         let common_cap = VirtioPciCap::new(
             PciCapabilityType::CommonConfig,
@@ -239,7 +242,9 @@ impl VirtioPciDevice {
             COMMON_CONFIG_BAR_OFFSET as u32,
             COMMON_CONFIG_SIZE as u32,
         );
-        self.config_regs.add_capability(&common_cap);
+        self.config_regs
+            .add_capability(&common_cap)
+            .map_err(|e| PciDeviceError::CapabilitiesSetup(e))?;
 
         let isr_cap = VirtioPciCap::new(
             PciCapabilityType::IsrConfig,
@@ -247,7 +252,9 @@ impl VirtioPciDevice {
             ISR_CONFIG_BAR_OFFSET as u32,
             ISR_CONFIG_SIZE as u32,
         );
-        self.config_regs.add_capability(&isr_cap);
+        self.config_regs
+            .add_capability(&isr_cap)
+            .map_err(|e| PciDeviceError::CapabilitiesSetup(e))?;
 
         // TODO(dgreid) - set based on device's configuration size?
         let device_cap = VirtioPciCap::new(
@@ -256,7 +263,9 @@ impl VirtioPciDevice {
             DEVICE_CONFIG_BAR_OFFSET as u32,
             DEVICE_CONFIG_SIZE as u32,
         );
-        self.config_regs.add_capability(&device_cap);
+        self.config_regs
+            .add_capability(&device_cap)
+            .map_err(|e| PciDeviceError::CapabilitiesSetup(e))?;
 
         let notify_cap = VirtioPciNotifyCap::new(
             PciCapabilityType::NotifyConfig,
@@ -265,13 +274,18 @@ impl VirtioPciDevice {
             NOTIFICATION_SIZE as u32,
             Le32::from(NOTIFY_OFF_MULTIPLIER),
         );
-        self.config_regs.add_capability(&notify_cap);
+        self.config_regs
+            .add_capability(&notify_cap)
+            .map_err(|e| PciDeviceError::CapabilitiesSetup(e))?;
 
         //TODO(dgreid) - How will the configuration_cap work?
         let configuration_cap = VirtioPciCap::new(PciCapabilityType::PciConfig, 0, 0, 0);
-        self.config_regs.add_capability(&configuration_cap);
+        self.config_regs
+            .add_capability(&configuration_cap)
+            .map_err(|e| PciDeviceError::CapabilitiesSetup(e))?;
 
         self.settings_bar = settings_bar;
+        Ok(())
     }
 }
 
@@ -319,12 +333,12 @@ impl PciDevice for VirtioPciDevice {
         let settings_bar = self
             .config_regs
             .add_pci_bar(&config)
-            .ok_or(PciDeviceError::IoRegistrationFailed(settings_config_addr))?
+            .map_err(|e| PciDeviceError::IoRegistrationFailed(settings_config_addr, e))?
             as u8;
         ranges.push((settings_config_addr, CAPABILITY_BAR_SIZE));
 
         // Once the BARs are allocated, the capabilities can be added to the PCI configuration.
-        self.add_pci_capabilities(settings_bar);
+        self.add_pci_capabilities(settings_bar)?;
 
         Ok(ranges)
     }
@@ -345,7 +359,7 @@ impl PciDevice for VirtioPciDevice {
                     let _device_bar = self
                         .config_regs
                         .add_pci_bar(&config)
-                        .ok_or(PciDeviceError::IoRegistrationFailed(device_addr))?;
+                        .map_err(|e| PciDeviceError::IoRegistrationFailed(device_addr, e))?;
                     ranges.push((device_addr, config.get_size()));
                 }
             }