move pci root creation to arch

passing everything in to the pci code is getting annoying. Instead build
it up in arch which already has access to all the needed resources.
Change-Id: If42f994443c4f11152fca8da16f27fa4cd80580d
Reviewed-on: https://chromium-review.googlesource.com/1237357
Commit-Ready: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

7 files changed, 112 insertions, 120 deletions

diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs
index cb39897..e78ee55 100644
--- a/aarch64/src/lib.rs
+++ b/aarch64/src/lib.rs
@@ -107,8 +107,6 @@ const AARCH64_IRQ_BASE: u32 = 1;
 
 #[derive(Debug)]
 pub enum Error {
-    /// Error Adding a PCI device.
-    AddPciDev(devices::PciRootError),
     /// Unable to clone an EventFd
     CloneEventFd(sys_util::Error),
     /// Error creating kernel command line.
@@ -118,7 +116,7 @@ pub enum Error {
     /// Unable to create Kvm.
     CreateKvm(sys_util::Error),
     /// Unable to create a PciRoot hub.
-    CreatePciRoot(devices::PciRootError),
+    CreatePciRoot(arch::DeviceRegistrationError),
     /// Unable to create socket.
     CreateSocket(io::Error),
     /// Unable to create Vcpu.
@@ -130,7 +128,7 @@ pub enum Error {
     /// Failure to Create GIC
     CreateGICFailure(sys_util::Error),
     /// Couldn't register virtio socket.
-    RegisterVsock(arch::MmioRegisterError),
+    RegisterVsock(arch::DeviceRegistrationError),
     /// VCPU Init failed
     VCPUInitFailure,
     /// VCPU Set one reg failed
@@ -140,7 +138,6 @@ pub enum Error {
 impl error::Error for Error {
     fn description(&self) -> &str {
         match self {
-            &Error::AddPciDev(_) => "Failed to add device to PCI",
             &Error::CloneEventFd(_) => "Unable to clone an EventFd",
             &Error::Cmdline(_) => "the given kernel command line was invalid",
             &Error::CreateEventFd(_) => "Unable to make an EventFd",
@@ -210,7 +207,9 @@ impl arch::LinuxArch for AArch64 {
 
         let mut mmio_bus = devices::Bus::new();
 
-        let (pci, pci_irqs) = components.pci_devices.generate_root(&mut mmio_bus, &mut resources)
+        let (pci, pci_irqs) = arch::generate_pci_root(components.pci_devices,
+                                                      &mut mmio_bus,
+                                                      &mut resources)
             .map_err(Error::CreatePciRoot)?;
 
         let exit_evt = EventFd::new().map_err(Error::CreateEventFd)?;
diff --git a/arch/src/lib.rs b/arch/src/lib.rs
index d7a1a63..6cd4dde 100644
--- a/arch/src/lib.rs
+++ b/arch/src/lib.rs
@@ -16,7 +16,8 @@ use std::os::unix::io::{AsRawFd, RawFd};
 use std::result;
 use std::sync::{Arc, Mutex};
 
-use devices::{Bus, PciDeviceList, Serial};
+use devices::{Bus, BusError, PciDevice, PciDeviceError, PciInterruptPin,
+              PciRoot, ProxyDevice, Serial};
 use devices::virtio::VirtioDevice;
 use io_jail::Minijail;
 use kvm::{IoeventAddress, Kvm, Vm, Vcpu};
@@ -28,7 +29,7 @@ pub type Result<T> = result::Result<T, Box<std::error::Error>>;
 /// Holds the pieces needed to build a VM. Passed to `build_vm` in the `LinuxArch` trait below to
 /// create a `RunnableLinuxVm`.
 pub struct VmComponents {
-    pub pci_devices: PciDeviceList,
+    pub pci_devices: Vec<(Box<PciDevice + 'static>,  Minijail)>,
     pub memory_mb: u64,
     pub vcpu_count: u32,
     pub kernel_image: File,
@@ -71,9 +72,17 @@ pub trait LinuxArch {
 
 /// Errors for device manager.
 #[derive(Debug)]
-pub enum MmioRegisterError {
+pub enum DeviceRegistrationError {
+    /// Could not allocate IO space for the device.
+    AllocateIoAddrs(PciDeviceError),
+    /// Could not allocate an IRQ number.
+    AllocateIrq,
     /// Could not create the mmio device to wrap a VirtioDevice.
     CreateMmioDevice(sys_util::Error),
+    /// Could not create an event fd.
+    EventFdCreate(sys_util::Error),
+    /// Could not add a device to the mmio bus.
+    MmioInsert(BusError),
     /// Failed to register ioevent with VM.
     RegisterIoevent(sys_util::Error),
     /// Failed to register irq eventfd with VM.
@@ -88,24 +97,79 @@ pub enum MmioRegisterError {
     AddrsExhausted,
 }
 
-impl fmt::Display for MmioRegisterError {
+impl fmt::Display for DeviceRegistrationError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            &MmioRegisterError::CreateMmioDevice(ref e) => write!(f, "failed to create mmio device: {:?}", e),
-            &MmioRegisterError::Cmdline(ref e) => {
+            &DeviceRegistrationError::AllocateIoAddrs(ref e) => {
+                write!(f, "Allocating IO addresses: {:?}", e)
+            }
+            &DeviceRegistrationError::AllocateIrq => {
+                write!(f, "Allocating IRQ number")
+            }
+            &DeviceRegistrationError::CreateMmioDevice(ref e) => {
+                write!(f, "failed to create mmio device: {:?}", e)
+            }
+            &DeviceRegistrationError::Cmdline(ref e) => {
                 write!(f, "unable to add device to kernel command line: {}", e)
             }
-            &MmioRegisterError::RegisterIoevent(ref e) => {
+            &DeviceRegistrationError::EventFdCreate(ref e) => {
+                write!(f, "failed to create eventfd: {:?}", e)
+            }
+            &DeviceRegistrationError::MmioInsert(ref e) => {
+                write!(f, "failed to add to mmio bus: {:?}", e)
+            }
+            &DeviceRegistrationError::RegisterIoevent(ref e) => {
                 write!(f, "failed to register ioevent to VM: {:?}", e)
             }
-            &MmioRegisterError::RegisterIrqfd(ref e) => {
+            &DeviceRegistrationError::RegisterIrqfd(ref e) => {
                 write!(f, "failed to register irq eventfd to VM: {:?}", e)
             }
-            &MmioRegisterError::ProxyDeviceCreation(ref e) => write!(f, "failed to create proxy device: {}", e),
-            &MmioRegisterError::IrqsExhausted => write!(f, "no more IRQs are available"),
-            &MmioRegisterError::AddrsExhausted => write!(f, "no more addresses are available"),
+            &DeviceRegistrationError::ProxyDeviceCreation(ref e) => {
+                write!(f, "failed to create proxy device: {}", e)
+            }
+            &DeviceRegistrationError::IrqsExhausted => write!(f, "no more IRQs are available"),
+            &DeviceRegistrationError::AddrsExhausted => write!(f, "no more addresses are available"),
+        }
+    }
+}
+
+/// Creates a root PCI device for use by this Vm.
+pub fn generate_pci_root(devices: Vec<(Box<PciDevice + 'static>, Minijail)>,
+                         mmio_bus: &mut Bus,
+                         resources: &mut SystemAllocator)
+    -> std::result::Result<(PciRoot, Vec<(u32, PciInterruptPin)>), DeviceRegistrationError>
+{
+    let mut root = PciRoot::new();
+    let mut pci_irqs = Vec::new();
+    for (dev_idx, (mut device, jail)) in devices.into_iter().enumerate() {
+        let mut keep_fds = Vec::new();
+        syslog::push_fds(&mut keep_fds);
+
+        let irqfd = EventFd::new().map_err(DeviceRegistrationError::EventFdCreate)?;
+        let irq_num = resources.allocate_irq().ok_or(DeviceRegistrationError::AllocateIrq)? as u32;
+        let pci_irq_pin = match dev_idx % 4 {
+            0 => PciInterruptPin::IntA,
+            1 => PciInterruptPin::IntB,
+            2 => PciInterruptPin::IntC,
+            3 => PciInterruptPin::IntD,
+            _ => panic!(""), // Obviously not possible, but the compiler is not smart enough.
+        };
+        device.assign_irq(irqfd, irq_num, pci_irq_pin);
+        pci_irqs.push((irq_num, pci_irq_pin));
+
+        let ranges = device
+            .allocate_io_bars(resources)
+            .map_err(DeviceRegistrationError::AllocateIoAddrs)?;
+        let proxy = ProxyDevice::new(device, &jail, keep_fds)
+            .map_err(DeviceRegistrationError::ProxyDeviceCreation)?;
+        let arced_dev = Arc::new(Mutex::new(proxy));
+        root.add_device(arced_dev.clone());
+        for range in &ranges {
+            mmio_bus.insert(arced_dev.clone(), range.0, range.1, true)
+                .map_err(DeviceRegistrationError::MmioInsert)?;
         }
     }
+    Ok((root, pci_irqs))
 }
 
 /// Register a device to be used via MMIO transport.
@@ -115,9 +179,9 @@ pub fn register_mmio(bus: &mut devices::Bus,
                      jail: Option<Minijail>,
                      resources: &mut SystemAllocator,
                      cmdline: &mut kernel_cmdline::Cmdline)
-                     -> std::result::Result<(), MmioRegisterError> {
+                     -> std::result::Result<(), DeviceRegistrationError> {
     let irq = match resources.allocate_irq() {
-        None => return Err(MmioRegisterError::IrqsExhausted),
+        None => return Err(DeviceRegistrationError::IrqsExhausted),
         Some(i) => i,
     };
 
@@ -125,29 +189,28 @@ pub fn register_mmio(bus: &mut devices::Bus,
     let mut keep_fds: Vec<RawFd> = device.keep_fds();
     syslog::push_fds(&mut keep_fds);
 
-    let mmio_device = devices::virtio::MmioDevice::new((*vm.get_memory()).clone(),
-    device)
-        .map_err(MmioRegisterError::CreateMmioDevice)?;
+    let mmio_device = devices::virtio::MmioDevice::new((*vm.get_memory()).clone(), device)
+        .map_err(DeviceRegistrationError::CreateMmioDevice)?;
     let mmio_len = 0x1000; // TODO(dgreid) - configurable per arch?
     let mmio_base = resources.allocate_mmio_addresses(mmio_len)
-        .ok_or(MmioRegisterError::AddrsExhausted)?;
+        .ok_or(DeviceRegistrationError::AddrsExhausted)?;
     for (i, queue_evt) in mmio_device.queue_evts().iter().enumerate() {
         let io_addr = IoeventAddress::Mmio(mmio_base +
                                            devices::virtio::NOTIFY_REG_OFFSET as u64);
         vm.register_ioevent(&queue_evt, io_addr, i as u32)
-          .map_err(MmioRegisterError::RegisterIoevent)?;
+          .map_err(DeviceRegistrationError::RegisterIoevent)?;
         keep_fds.push(queue_evt.as_raw_fd());
     }
 
     if let Some(interrupt_evt) = mmio_device.interrupt_evt() {
         vm.register_irqfd(&interrupt_evt, irq)
-          .map_err(MmioRegisterError::RegisterIrqfd)?;
+          .map_err(DeviceRegistrationError::RegisterIrqfd)?;
         keep_fds.push(interrupt_evt.as_raw_fd());
     }
 
     if let Some(jail) = jail {
         let proxy_dev = devices::ProxyDevice::new(mmio_device, &jail, keep_fds)
-            .map_err(MmioRegisterError::ProxyDeviceCreation)?;
+            .map_err(DeviceRegistrationError::ProxyDeviceCreation)?;
 
         bus.insert(Arc::new(Mutex::new(proxy_dev)), mmio_base, mmio_len, false).unwrap();
     } else {
@@ -157,7 +220,7 @@ pub fn register_mmio(bus: &mut devices::Bus,
     cmdline
         .insert("virtio_mmio.device",
                 &format!("4K@0x{:08x}:{}", mmio_base, irq))
-        .map_err(MmioRegisterError::Cmdline)?;
+        .map_err(DeviceRegistrationError::Cmdline)?;
 
     Ok(())
 }
diff --git a/devices/src/lib.rs b/devices/src/lib.rs
index 1478849..46664fc 100644
--- a/devices/src/lib.rs
+++ b/devices/src/lib.rs
@@ -28,11 +28,11 @@ pub mod pl030;
 pub mod virtio;
 
 pub use self::bus::{Bus, BusDevice, BusRange};
+pub use self::bus::Error as BusError;
 pub use self::cmos::Cmos;
 pub use self::pl030::Pl030;
 pub use self::i8042::I8042Device;
-pub use self::pci::{PciDevice, PciDeviceList, PciInterruptPin, PciRoot};
-pub use self::pci::PciRootError as PciRootError;
+pub use self::pci::{PciDevice, PciDeviceError, PciInterruptPin, PciRoot};
 pub use self::proxy::ProxyDevice;
 pub use self::proxy::Error as ProxyError;
 pub use self::serial::Serial;
diff --git a/devices/src/pci/mod.rs b/devices/src/pci/mod.rs
index f5e5d1f..a1d211d 100644
--- a/devices/src/pci/mod.rs
+++ b/devices/src/pci/mod.rs
@@ -8,9 +8,10 @@ mod pci_configuration;
 mod pci_device;
 mod pci_root;
 
+pub use self::pci_configuration::{PciCapability, PciCapabilityID, PciClassCode, PciConfiguration, PciHeaderType, PciSubclass};
+pub use self::pci_device::Error as PciDeviceError;
 pub use self::pci_device::PciDevice;
-pub use self::pci_root::Error as PciRootError;
-pub use self::pci_root::{PciDeviceList, PciRoot};
+pub use self::pci_root::PciRoot;
 
 /// PCI has four interrupt pins A->D.
 #[derive(Copy, Clone)]
diff --git a/devices/src/pci/pci_root.rs b/devices/src/pci/pci_root.rs
index e048743..7988a9f 100644
--- a/devices/src/pci/pci_root.rs
+++ b/devices/src/pci/pci_root.rs
@@ -2,73 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-use std;
 use std::sync::{Arc, Mutex};
 
 use byteorder::{ByteOrder, LittleEndian};
 
-use io_jail::Minijail;
-use sys_util::{self, EventFd};
-use resources::SystemAllocator;
-
-use Bus;
 use BusDevice;
-use bus::Error as BusError;
-use proxy::Error as ProxyError;
 use ProxyDevice;
 
 use pci::pci_configuration::{PciBridgeSubclass, PciClassCode, PciConfiguration,
                              PciHeaderType};
-use pci::pci_device::{self, PciDevice};
-use pci::PciInterruptPin;
-
-#[derive(Debug)]
-pub enum Error {
-    CreateEventFd(sys_util::Error),
-    MmioRegistration(BusError),
-    ProxyCreation(ProxyError),
-    DeviceIoSpaceAllocation(pci_device::Error),
-}
-pub type Result<T> = std::result::Result<T, Error>;
-
-/// Contains the devices that will be on a PCI bus. Used to configure a PCI bus before adding it to
-/// a VM. Use `generate_hub` to produce a PciRoot for use in a Vm.
-pub struct PciDeviceList {
-    devices: Vec<(Box<PciDevice + 'static>, Minijail)>,
-}
-
-impl PciDeviceList {
-    pub fn new() -> Self {
-        PciDeviceList {
-            devices: Vec::new(),
-        }
-    }
-
-    pub fn add_device(&mut self, device: Box<PciDevice + 'static>, jail: Minijail) {
-        self.devices.push((device, jail));
-    }
-
-    pub fn generate_root(self, mmio_bus: &mut Bus, resources: &mut SystemAllocator)
-            -> Result<(PciRoot, Vec<(u32, PciInterruptPin)>)> {
-        let mut root = PciRoot::new();
-        let mut pci_irqs = Vec::new();
-        for (dev_idx, (mut device, jail)) in self.devices.into_iter().enumerate() {
-            let irqfd = EventFd::new().map_err(Error::CreateEventFd)?;
-            let irq_num = resources.allocate_irq().unwrap() as u32;
-            let pci_irq_pin = match dev_idx % 4 {
-                0 => PciInterruptPin::IntA,
-                1 => PciInterruptPin::IntB,
-                2 => PciInterruptPin::IntC,
-                3 => PciInterruptPin::IntD,
-                _ => panic!(""), // Obviously not possible, but the compiler is not smart enough.
-            };
-            device.assign_irq(irqfd, irq_num, pci_irq_pin);
-            pci_irqs.push((irq_num, pci_irq_pin));
-            root.add_device(device, &jail, mmio_bus, resources)?;
-        }
-        Ok((root, pci_irqs))
-    }
-}
+use pci::pci_device::PciDevice;
 
 // A PciDevice that holds the root hub's configuration.
 struct PciRootConfiguration {
@@ -101,7 +44,7 @@ pub struct PciRoot {
 
 impl PciRoot {
     /// Create an empty PCI root bus.
-    fn new() -> Self {
+    pub fn new() -> Self {
         PciRoot {
             root_configuration: PciRootConfiguration {
                 config: PciConfiguration::new(
@@ -120,21 +63,8 @@ impl PciRoot {
     }
 
     /// Add a `device` to this root PCI bus.
-    pub fn add_device<D: PciDevice>(&mut self, mut device: D, jail: &Minijail,
-                      mmio_bus: &mut Bus, // TODO - move to resources or something.
-                      resources: &mut SystemAllocator) -> Result<()> {
-        let ranges = device
-            .allocate_io_bars(resources)
-            .map_err(Error::DeviceIoSpaceAllocation)?;
-        let proxy = ProxyDevice::new(device, &jail, Vec::new())
-            .map_err(Error::ProxyCreation)?;
-        let arced_dev = Arc::new(Mutex::new(proxy));
-        for range in &ranges {
-            mmio_bus.insert(arced_dev.clone(), range.0, range.1, true)
-                .map_err(Error::MmioRegistration)?;
-        }
-        self.devices.push(arced_dev);
-        Ok(())
+    pub fn add_device(&mut self, device: Arc<Mutex<ProxyDevice>>) {
+        self.devices.push(device);
     }
 
     fn config_space_read(&self) -> u32 {
diff --git a/src/linux.rs b/src/linux.rs
index 5b46663..1496998 100644
--- a/src/linux.rs
+++ b/src/linux.rs
@@ -25,7 +25,7 @@ use rand::thread_rng;
 use rand::distributions::{IndependentSample, Range};
 
 use byteorder::{ByteOrder, LittleEndian};
-use devices;
+use devices::{self, PciDevice};
 use io_jail::{self, Minijail};
 use kvm::*;
 use net_util::Tap;
@@ -75,14 +75,14 @@ pub enum Error {
     QcowDeviceCreate(qcow::Error),
     ReadLowmemAvailable(io::Error),
     ReadLowmemMargin(io::Error),
-    RegisterBalloon(arch::MmioRegisterError),
-    RegisterBlock(arch::MmioRegisterError),
-    RegisterGpu(arch::MmioRegisterError),
-    RegisterNet(arch::MmioRegisterError),
-    RegisterP9(arch::MmioRegisterError),
-    RegisterRng(arch::MmioRegisterError),
+    RegisterBalloon(arch::DeviceRegistrationError),
+    RegisterBlock(arch::DeviceRegistrationError),
+    RegisterGpu(arch::DeviceRegistrationError),
+    RegisterNet(arch::DeviceRegistrationError),
+    RegisterP9(arch::DeviceRegistrationError),
+    RegisterRng(arch::DeviceRegistrationError),
     RegisterSignalHandler(sys_util::Error),
-    RegisterWayland(arch::MmioRegisterError),
+    RegisterWayland(arch::DeviceRegistrationError),
     ResetTimerFd(sys_util::Error),
     RngDeviceNew(devices::virtio::RngError),
     SettingGidMap(io_jail::Error),
@@ -695,7 +695,7 @@ pub fn run_config(cfg: Config) -> Result<()> {
         info!("crosvm entering multiprocess mode");
     }
 
-    let pci_devices = devices::PciDeviceList::new();
+    let pci_devices: Vec<(Box<PciDevice + 'static>, Minijail)> = Vec::new();
 
     // Masking signals is inherently dangerous, since this can persist across clones/execs. Do this
     // before any jailed devices have been spawned, so that we can catch any of them that fail very
diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs
index fe7de3b..bee8bff 100644
--- a/x86_64/src/lib.rs
+++ b/x86_64/src/lib.rs
@@ -77,8 +77,6 @@ use kvm::*;
 
 #[derive(Debug)]
 pub enum Error {
-    /// Error Adding a PCI device.
-    AddPciDev(devices::PciRootError),
     /// Error configuring the system
     ConfigureSystem,
     /// Unable to clone an EventFd
@@ -90,7 +88,7 @@ pub enum Error {
     /// Unable to create Kvm.
     CreateKvm(sys_util::Error),
     /// Unable to create a PciRoot hub.
-    CreatePciRoot(devices::PciRootError),
+    CreatePciRoot(arch::DeviceRegistrationError),
     /// Unable to create socket.
     CreateSocket(io::Error),
     /// Unable to create Vcpu.
@@ -100,7 +98,7 @@ pub enum Error {
     /// Error registering an IrqFd
     RegisterIrqfd(sys_util::Error),
     /// Couldn't register virtio socket.
-    RegisterVsock(arch::MmioRegisterError),
+    RegisterVsock(arch::DeviceRegistrationError),
     LoadCmdline(kernel_loader::Error),
     LoadKernel(kernel_loader::Error),
     /// Error writing the zero page of guest memory.
@@ -114,7 +112,6 @@ pub enum Error {
 impl error::Error for Error {
     fn description(&self) -> &str {
         match self {
-            &Error::AddPciDev(_) => "Failed to add device to PCI",
             &Error::ConfigureSystem => "Error configuring the system",
             &Error::CloneEventFd(_) => "Unable to clone an EventFd",
             &Error::Cmdline(_) => "the given kernel command line was invalid",
@@ -278,7 +275,9 @@ impl arch::LinuxArch for X8664arch {
 
         let mut mmio_bus = devices::Bus::new();
 
-        let (pci, pci_irqs) = components.pci_devices.generate_root(&mut mmio_bus, &mut resources)
+        let (pci, pci_irqs) = arch::generate_pci_root(components.pci_devices,
+                                                      &mut mmio_bus,
+                                                      &mut resources)
             .map_err(Error::CreatePciRoot)?;
         let pci_bus = Arc::new(Mutex::new(pci));