arch: Replace Box<dyn Error> with error enum

Avoiding Box<dyn Error> makes it less likely that we display errors with
insufficient context by accident.

Many of the errors touched in this CL already had helpful message
written! But those corresponding enum variants were never being
instantiated, and that bug was masked by Box<dyn Error>. For example see
the Error::LoadCmdline and Error::LoadKernel.

    pub enum Error {
        LoadCmdline(kernel_loader::Error),
        ...
    }

Before this CL:

    // Bug: boxes the underlying error without adding LoadCmdline
    kernel_loader::load_cmdline(...)?;

After this CL:

    kernel_loader::load_cmdline(...).map_err(Error::LoadCmdline)?;

TEST=cargo check
TEST=cargo check --all-features
TEST=cargo check --target aarch64-unknown-linux-gnu

Change-Id: I7c0cff843c2211565226b9dfb4142ad6b7fa15ac
Reviewed-on: https://chromium-review.googlesource.com/1502112
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: David Tolnay <dtolnay@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>

2 files changed, 68 insertions, 69 deletions

diff --git a/aarch64/src/fdt.rs b/aarch64/src/fdt.rs
index ff3274f..b03d039 100644
--- a/aarch64/src/fdt.rs
+++ b/aarch64/src/fdt.rs
@@ -6,7 +6,7 @@ use std::ffi::CStr;
 
 use arch::fdt::{
     begin_node, end_node, finish_fdt, generate_prop32, generate_prop64, property, property_cstring,
-    property_null, property_string, property_u32, property_u64, start_fdt, Error,
+    property_null, property_string, property_u32, property_u64, start_fdt, Error, Result,
 };
 use devices::PciInterruptPin;
 use sys_util::{GuestAddress, GuestMemory};
@@ -54,7 +54,7 @@ const IRQ_TYPE_EDGE_RISING: u32 = 0x00000001;
 const IRQ_TYPE_LEVEL_HIGH: u32 = 0x00000004;
 const IRQ_TYPE_LEVEL_LOW: u32 = 0x00000008;
 
-fn create_memory_node(fdt: &mut Vec<u8>, guest_mem: &GuestMemory) -> Result<(), Box<Error>> {
+fn create_memory_node(fdt: &mut Vec<u8>, guest_mem: &GuestMemory) -> Result<()> {
     let mem_size = guest_mem.memory_size();
     let mem_reg_prop = generate_prop64(&[AARCH64_PHYS_MEM_START, mem_size]);
 
@@ -65,7 +65,7 @@ fn create_memory_node(fdt: &mut Vec<u8>, guest_mem: &GuestMemory) -> Result<(),
     Ok(())
 }
 
-fn create_cpu_nodes(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<(), Box<Error>> {
+fn create_cpu_nodes(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<()> {
     begin_node(fdt, "cpus")?;
     property_u32(fdt, "#address-cells", 0x1)?;
     property_u32(fdt, "#size-cells", 0x0)?;
@@ -85,7 +85,7 @@ fn create_cpu_nodes(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<(), Box<Error>>
     Ok(())
 }
 
-fn create_gic_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
+fn create_gic_node(fdt: &mut Vec<u8>) -> Result<()> {
     let gic_reg_prop = generate_prop64(&[
         AARCH64_GIC_DIST_BASE,
         AARCH64_GIC_DIST_SIZE,
@@ -106,7 +106,7 @@ fn create_gic_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
     Ok(())
 }
 
-fn create_timer_node(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<(), Box<Error>> {
+fn create_timer_node(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<()> {
     // These are fixed interrupt numbers for the timer device.
     let irqs = [13, 14, 11, 10];
     let compatible = "arm,armv8-timer";
@@ -130,7 +130,7 @@ fn create_timer_node(fdt: &mut Vec<u8>, num_cpus: u32) -> Result<(), Box<Error>>
     Ok(())
 }
 
-fn create_serial_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
+fn create_serial_node(fdt: &mut Vec<u8>) -> Result<()> {
     let serial_reg_prop = generate_prop64(&[AARCH64_SERIAL_ADDR, AARCH64_SERIAL_SIZE]);
     let irq = generate_prop32(&[
         GIC_FDT_IRQ_TYPE_SPI,
@@ -149,7 +149,7 @@ fn create_serial_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
 }
 
 // TODO(sonnyrao) -- check to see if host kernel supports PSCI 0_2
-fn create_psci_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
+fn create_psci_node(fdt: &mut Vec<u8>) -> Result<()> {
     let compatible = "arm,psci-0.2";
     begin_node(fdt, "psci")?;
     property_string(fdt, "compatible", compatible)?;
@@ -169,7 +169,7 @@ fn create_chosen_node(
     fdt: &mut Vec<u8>,
     cmdline: &CStr,
     initrd: Option<(GuestAddress, usize)>,
-) -> Result<(), Box<Error>> {
+) -> Result<()> {
     begin_node(fdt, "chosen")?;
     property_u32(fdt, "linux,pci-probe-only", 1)?;
     property_cstring(fdt, "bootargs", cmdline)?;
@@ -185,10 +185,7 @@ fn create_chosen_node(
     Ok(())
 }
 
-fn create_pci_nodes(
-    fdt: &mut Vec<u8>,
-    pci_irqs: Vec<(u32, PciInterruptPin)>,
-) -> Result<(), Box<Error>> {
+fn create_pci_nodes(fdt: &mut Vec<u8>, pci_irqs: Vec<(u32, PciInterruptPin)>) -> Result<()> {
     // Add devicetree nodes describing a PCI generic host controller.
     // See Documentation/devicetree/bindings/pci/host-generic-pci.txt in the kernel
     // and "PCI Bus Binding to IEEE Std 1275-1994".
@@ -258,7 +255,7 @@ fn create_pci_nodes(
     Ok(())
 }
 
-fn create_rtc_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
+fn create_rtc_node(fdt: &mut Vec<u8>) -> Result<()> {
     // the kernel driver for pl030 really really wants a clock node
     // associated with an AMBA device or it will fail to probe, so we
     // need to make up a clock node to associate with the pl030 rtc
@@ -306,7 +303,7 @@ pub fn create_fdt(
     fdt_load_offset: u64,
     cmdline: &CStr,
     initrd: Option<(GuestAddress, usize)>,
-) -> Result<(), Box<Error>> {
+) -> Result<()> {
     let mut fdt = vec![0; fdt_max_size];
     start_fdt(&mut fdt, fdt_max_size)?;
 
@@ -338,7 +335,7 @@ pub fn create_fdt(
         .write_at_addr(fdt_final.as_slice(), fdt_address)
         .map_err(|_| Error::FdtGuestMemoryWriteError)?;
     if written < fdt_max_size {
-        return Err(Box::new(Error::FdtGuestMemoryWriteError));
+        return Err(Error::FdtGuestMemoryWriteError);
     }
     Ok(())
 }
diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs
index 58496ec..85e1076 100644
--- a/aarch64/src/lib.rs
+++ b/aarch64/src/lib.rs
@@ -14,6 +14,7 @@ extern crate resources;
 extern crate sync;
 extern crate sys_util;
 
+use std::error::Error as StdError;
 use std::ffi::{CStr, CString};
 use std::fmt::{self, Display};
 use std::fs::File;
@@ -26,12 +27,11 @@ use devices::{Bus, BusError, PciConfigMmio, PciDevice, PciInterruptPin};
 use io_jail::Minijail;
 use resources::{AddressRanges, SystemAllocator};
 use sync::Mutex;
-use sys_util::{EventFd, GuestAddress, GuestMemory};
+use sys_util::{EventFd, GuestAddress, GuestMemory, GuestMemoryError};
 
 use kvm::*;
 use kvm_sys::kvm_device_attr;
 
-use arch::Result;
 mod fdt;
 
 // We place the kernel at offset 8MB
@@ -117,40 +117,29 @@ const AARCH64_IRQ_BASE: u32 = 2;
 
 #[derive(Debug)]
 pub enum Error {
-    /// Unable to clone an EventFd
     CloneEventFd(sys_util::Error),
-    /// Error creating kernel command line.
     Cmdline(kernel_cmdline::Error),
-    /// Unable to make an EventFd
+    CreateDevices(Box<dyn StdError>),
     CreateEventFd(sys_util::Error),
-    /// Unable to create Kvm.
+    CreateFdt(arch::fdt::Error),
+    CreateGICFailure(sys_util::Error),
     CreateKvm(sys_util::Error),
-    /// Unable to create a PciRoot hub.
     CreatePciRoot(arch::DeviceRegistrationError),
-    /// Unable to create socket.
     CreateSocket(io::Error),
-    /// Unable to create Vcpu.
     CreateVcpu(sys_util::Error),
-    /// FDT could not be created
-    FDTCreateFailure(Box<std::error::Error>),
-    /// Kernel could not be loaded
-    KernelLoadFailure(arch::LoadImageError),
-    /// Initrd could not be loaded
+    CreateVm(sys_util::Error),
     InitrdLoadFailure(arch::LoadImageError),
-    /// Failure to Create GIC
-    CreateGICFailure(sys_util::Error),
-    /// Couldn't register PCI bus.
+    KernelLoadFailure(arch::LoadImageError),
+    ReadPreferredTarget(sys_util::Error),
+    RegisterIrqfd(sys_util::Error),
     RegisterPci(BusError),
-    /// Couldn't register virtio socket.
     RegisterVsock(arch::DeviceRegistrationError),
-    /// VCPU Init failed
-    VCPUInitFailure,
-    /// VCPU Set one reg failed
-    VCPUSetRegFailure,
+    SetDeviceAttr(sys_util::Error),
+    SetReg(sys_util::Error),
+    SetupGuestMemory(GuestMemoryError),
+    VcpuInit(sys_util::Error),
 }
 
-impl std::error::Error for Error {}
-
 impl Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use self::Error::*;
@@ -158,23 +147,33 @@ impl Display for Error {
         match self {
             CloneEventFd(e) => write!(f, "unable to clone an EventFd: {}", e),
             Cmdline(e) => write!(f, "the given kernel command line was invalid: {}", e),
+            CreateDevices(e) => write!(f, "error creating devices: {}", e),
             CreateEventFd(e) => write!(f, "unable to make an EventFd: {}", e),
+            CreateFdt(e) => write!(f, "FDT could not be created: {}", e),
+            CreateGICFailure(e) => write!(f, "failed to create GIC: {}", e),
             CreateKvm(e) => write!(f, "failed to open /dev/kvm: {}", e),
             CreatePciRoot(e) => write!(f, "failed to create a PCI root hub: {}", e),
             CreateSocket(e) => write!(f, "failed to create socket: {}", e),
             CreateVcpu(e) => write!(f, "failed to create VCPU: {}", e),
-            FDTCreateFailure(e) => write!(f, "FDT could not be created: {}", e),
-            KernelLoadFailure(e) => write!(f, "kernel cound not be loaded: {}", e),
+            CreateVm(e) => write!(f, "failed to create vm: {}", e),
             InitrdLoadFailure(e) => write!(f, "initrd cound not be loaded: {}", e),
-            CreateGICFailure(e) => write!(f, "failed to create GIC: {}", e),
+            KernelLoadFailure(e) => write!(f, "kernel cound not be loaded: {}", e),
+            ReadPreferredTarget(e) => write!(f, "failed to read preferred target: {}", e),
+            RegisterIrqfd(e) => write!(f, "failed to register irq fd: {}", e),
             RegisterPci(e) => write!(f, "error registering PCI bus: {}", e),
             RegisterVsock(e) => write!(f, "error registering virtual socket device: {}", e),
-            VCPUInitFailure => write!(f, "failed to initialize VCPU"),
-            VCPUSetRegFailure => write!(f, "failed to set register"),
+            SetDeviceAttr(e) => write!(f, "failed to set device attr: {}", e),
+            SetReg(e) => write!(f, "failed to set register: {}", e),
+            SetupGuestMemory(e) => write!(f, "failed to set up guest memory: {}", e),
+            VcpuInit(e) => write!(f, "failed to initialize VCPU: {}", e),
         }
     }
 }
 
+pub type Result<T> = std::result::Result<T, Error>;
+
+impl std::error::Error for Error {}
+
 /// Returns a Vec of the valid memory addresses.
 /// These should be used to configure the GuestMemory structure for the platfrom.
 pub fn arch_memory_regions(size: u64) -> Vec<(GuestAddress, u64)> {
@@ -191,7 +190,9 @@ fn fdt_offset(mem_size: u64) -> u64 {
 pub struct AArch64;
 
 impl arch::LinuxArch for AArch64 {
-    fn build_vm<F>(
+    type Error = Error;
+
+    fn build_vm<F, E>(
         mut components: VmComponents,
         _split_irqchip: bool,
         create_devices: F,
@@ -200,13 +201,14 @@ impl arch::LinuxArch for AArch64 {
         F: FnOnce(
             &GuestMemory,
             &EventFd,
-        ) -> Result<Vec<(Box<PciDevice + 'static>, Option<Minijail>)>>,
+        ) -> std::result::Result<Vec<(Box<PciDevice>, Option<Minijail>)>, E>,
+        E: StdError + 'static,
     {
         let mut resources =
             Self::get_resource_allocator(components.memory_mb, components.wayland_dmabuf);
         let mem = Self::setup_memory(components.memory_mb)?;
         let kvm = Kvm::new().map_err(Error::CreateKvm)?;
-        let mut vm = Self::create_vm(&kvm, mem.clone())?;
+        let mut vm = Vm::new(&kvm, mem.clone()).map_err(Error::CreateVm)?;
 
         let vcpu_count = components.vcpu_count;
         let mut vcpus = Vec::with_capacity(vcpu_count as usize);
@@ -230,7 +232,8 @@ impl arch::LinuxArch for AArch64 {
 
         let exit_evt = EventFd::new().map_err(Error::CreateEventFd)?;
 
-        let pci_devices = create_devices(&mem, &exit_evt)?;
+        let pci_devices =
+            create_devices(&mem, &exit_evt).map_err(|e| Error::CreateDevices(Box::new(e)))?;
         let (pci, pci_irqs, pid_debug_label_map) =
             arch::generate_pci_root(pci_devices, &mut mmio_bus, &mut resources, &mut vm)
                 .map_err(Error::CreatePciRoot)?;
@@ -321,18 +324,14 @@ impl AArch64 {
             fdt_offset(mem_size),
             cmdline,
             initrd,
-        )?;
+        )
+        .map_err(Error::CreateFdt)?;
         Ok(())
     }
 
-    fn create_vm(kvm: &Kvm, mem: GuestMemory) -> Result<Vm> {
-        let vm = Vm::new(&kvm, mem)?;
-        Ok(vm)
-    }
-
     fn setup_memory(mem_size: u64) -> Result<GuestMemory> {
         let arch_mem_regions = arch_memory_regions(mem_size);
-        let mem = GuestMemory::new(&arch_mem_regions)?;
+        let mem = GuestMemory::new(&arch_mem_regions).map_err(Error::SetupGuestMemory)?;
         Ok(mem)
     }
 
@@ -364,13 +363,15 @@ impl AArch64 {
     /// * `vm` - The vm to add irqs to.
     /// * `bus` - The bus to add devices to.
     fn add_arch_devs(vm: &mut Vm, bus: &mut Bus) -> Result<Arc<Mutex<devices::Serial>>> {
-        let rtc_evt = EventFd::new()?;
-        vm.register_irqfd(&rtc_evt, AARCH64_RTC_IRQ)?;
+        let rtc_evt = EventFd::new().map_err(Error::CreateEventFd)?;
+        vm.register_irqfd(&rtc_evt, AARCH64_RTC_IRQ)
+            .map_err(Error::RegisterIrqfd)?;
 
-        let com_evt_1_3 = EventFd::new()?;
-        vm.register_irqfd(&com_evt_1_3, AARCH64_SERIAL_IRQ)?;
+        let com_evt_1_3 = EventFd::new().map_err(Error::CreateEventFd)?;
+        vm.register_irqfd(&com_evt_1_3, AARCH64_SERIAL_IRQ)
+            .map_err(Error::RegisterIrqfd)?;
         let serial = Arc::new(Mutex::new(devices::Serial::new_out(
-            com_evt_1_3.try_clone()?,
+            com_evt_1_3.try_clone().map_err(Error::CloneEventFd)?,
             Box::new(stdout()),
         )));
         bus.insert(
@@ -429,7 +430,7 @@ impl AArch64 {
             sys_util::ioctl_with_ref(&vgic_fd, kvm_sys::KVM_SET_DEVICE_ATTR(), &cpu_if_attr)
         };
         if ret != 0 {
-            return Err(Box::new(Error::CreateGICFailure(sys_util::Error::new(ret))));
+            return Err(Error::CreateGICFailure(sys_util::Error::new(ret)));
         }
 
         // Safe because we allocated the struct that's being passed in
@@ -437,7 +438,7 @@ impl AArch64 {
             sys_util::ioctl_with_ref(&vgic_fd, kvm_sys::KVM_SET_DEVICE_ATTR(), &dist_attr)
         };
         if ret != 0 {
-            return Err(Box::new(Error::CreateGICFailure(sys_util::Error::new(ret))));
+            return Err(Error::CreateGICFailure(sys_util::Error::new(ret)));
         }
 
         // We need to tell the kernel how many irqs to support with this vgic
@@ -454,7 +455,7 @@ impl AArch64 {
             sys_util::ioctl_with_ref(&vgic_fd, kvm_sys::KVM_SET_DEVICE_ATTR(), &nr_irqs_attr)
         };
         if ret != 0 {
-            return Err(Box::new(Error::CreateGICFailure(sys_util::Error::new(ret))));
+            return Err(Error::CreateGICFailure(sys_util::Error::new(ret)));
         }
 
         // Finalize the GIC
@@ -470,7 +471,7 @@ impl AArch64 {
             sys_util::ioctl_with_ref(&vgic_fd, kvm_sys::KVM_SET_DEVICE_ATTR(), &init_gic_attr)
         };
         if ret != 0 {
-            return Err(Box::new(sys_util::Error::new(ret)));
+            return Err(Error::SetDeviceAttr(sys_util::Error::new(ret)));
         }
         Ok(Some(vgic_fd))
     }
@@ -489,7 +490,8 @@ impl AArch64 {
         };
 
         // This reads back the kernel's preferred target type.
-        vm.arm_preferred_target(&mut kvi)?;
+        vm.arm_preferred_target(&mut kvi)
+            .map_err(Error::ReadPreferredTarget)?;
 
         // TODO(sonnyrao): need to verify this feature is supported by host kernel
         kvi.features[0] |= 1 << kvm_sys::KVM_ARM_VCPU_PSCI_0_2;
@@ -498,7 +500,7 @@ impl AArch64 {
         if cpu_id > 0 {
             kvi.features[0] |= 1 << kvm_sys::KVM_ARM_VCPU_POWER_OFF;
         }
-        vcpu.arm_vcpu_init(&kvi)?;
+        vcpu.arm_vcpu_init(&kvi).map_err(Error::VcpuInit)?;
 
         // set up registers
         let mut data: u64;
@@ -507,20 +509,20 @@ impl AArch64 {
         // All interrupts masked
         data = PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | PSR_MODE_EL1H;
         reg_id = arm64_core_reg!(pstate);
-        vcpu.set_one_reg(reg_id, data)?;
+        vcpu.set_one_reg(reg_id, data).map_err(Error::SetReg)?;
 
         // Other cpus are powered off initially
         if cpu_id == 0 {
             data = AARCH64_PHYS_MEM_START + AARCH64_KERNEL_OFFSET;
             reg_id = arm64_core_reg!(pc);
-            vcpu.set_one_reg(reg_id, data)?;
+            vcpu.set_one_reg(reg_id, data).map_err(Error::SetReg)?;
 
             /* X0 -- fdt address */
             let mem_size = guest_mem.memory_size();
             data = (AARCH64_PHYS_MEM_START + fdt_offset(mem_size)) as u64;
             // hack -- can't get this to do offsetof(regs[0]) but luckily it's at offset 0
             reg_id = arm64_core_reg!(regs);
-            vcpu.set_one_reg(reg_id, data)?;
+            vcpu.set_one_reg(reg_id, data).map_err(Error::SetReg)?;
         }
         Ok(())
     }