Merge remote-tracking branch 'origin/master'

14 files changed, 445 insertions, 274 deletions

diff --git a/devices/Cargo.toml b/devices/Cargo.toml
index 629f29b..931ea36 100644
--- a/devices/Cargo.toml
+++ b/devices/Cargo.toml
@@ -12,6 +12,7 @@ x = ["gpu_display/x"]
 gfxstream = ["gpu"]
 
 [dependencies]
+acpi_tables = {path = "../acpi_tables" }
 audio_streams = "*"
 bit_field = { path = "../bit_field" }
 bitflags = "1"
@@ -21,6 +22,7 @@ enumn = { path = "../enumn" }
 gpu_buffer = { path = "../gpu_buffer", optional = true }
 gpu_display = { path = "../gpu_display", optional = true }
 gpu_renderer = { path = "../gpu_renderer", optional = true }
+hypervisor = { path = "../hypervisor" }
 io_jail = { path = "../io_jail" }
 kvm = { path = "../kvm" }
 kvm_sys = { path = "../kvm_sys" }
diff --git a/devices/src/acpi.rs b/devices/src/acpi.rs
index 990a782..1cfab35 100644
--- a/devices/src/acpi.rs
+++ b/devices/src/acpi.rs
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 use crate::{BusDevice, BusResumeDevice};
+use acpi_tables::{aml, aml::Aml};
 use sys_util::{error, warn, EventFd};
 
 /// ACPI PM resource for handling OS suspend/resume request
@@ -29,8 +30,7 @@ impl ACPIPMResource {
     }
 }
 
-/// the ACPI PM register's base and length.
-pub const ACPIPM_RESOURCE_BASE: u64 = 0x600;
+/// the ACPI PM register length.
 pub const ACPIPM_RESOURCE_LEN: u8 = 8;
 pub const ACPIPM_RESOURCE_EVENTBLK_LEN: u8 = 4;
 pub const ACPIPM_RESOURCE_CONTROLBLK_LEN: u8 = 2;
@@ -127,3 +127,14 @@ impl BusResumeDevice for ACPIPMResource {
         self.sleep_status = val | BITMASK_SLEEPCNT_WAKE_STATUS;
     }
 }
+
+impl Aml for ACPIPMResource {
+    fn to_aml_bytes(&self, bytes: &mut Vec<u8>) {
+        // S1
+        aml::Name::new(
+            "_S1_".into(),
+            &aml::Package::new(vec![&aml::ONE, &aml::ONE, &aml::ZERO, &aml::ZERO]),
+        )
+        .to_aml_bytes(bytes);
+    }
+}
diff --git a/devices/src/irqchip/kvm/mod.rs b/devices/src/irqchip/kvm/mod.rs
new file mode 100644
index 0000000..56794a3
--- /dev/null
+++ b/devices/src/irqchip/kvm/mod.rs
@@ -0,0 +1,122 @@
+// Copyright 2020 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+use hypervisor::kvm::{KvmVcpu, KvmVm};
+use hypervisor::IrqRoute;
+use std::sync::Arc;
+use sync::Mutex;
+use sys_util::{EventFd, Result};
+
+use crate::IrqChip;
+
+/// IrqChip implementation where the entire IrqChip is emulated by KVM.
+///
+/// This implementation will use the KVM API to create and configure the in-kernel irqchip.
+pub struct KvmKernelIrqChip {
+    _vm: KvmVm,
+    vcpus: Arc<Mutex<Vec<Option<KvmVcpu>>>>,
+}
+
+impl KvmKernelIrqChip {
+    /// Construct a new KvmKernelIrqchip.
+    pub fn new(vm: KvmVm, num_vcpus: usize) -> Result<KvmKernelIrqChip> {
+        Ok(KvmKernelIrqChip {
+            _vm: vm,
+            vcpus: Arc::new(Mutex::new((0..num_vcpus).map(|_| None).collect())),
+        })
+    }
+}
+
+/// This IrqChip only works with Kvm so we only implement it for KvmVcpu.
+impl IrqChip<KvmVcpu> for KvmKernelIrqChip {
+    /// Add a vcpu to the irq chip.
+    fn add_vcpu(&mut self, vcpu_id: usize, vcpu: KvmVcpu) -> Result<()> {
+        self.vcpus.lock().insert(vcpu_id, Some(vcpu));
+        Ok(())
+    }
+
+    /// Register an event that can trigger an interrupt for a particular GSI.
+    fn register_irq_event(
+        &mut self,
+        _irq: u32,
+        _irq_event: &EventFd,
+        _resample_event: Option<&EventFd>,
+    ) -> Result<()> {
+        unimplemented!("register_irq_event for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Unregister an event for a particular GSI.
+    fn unregister_irq_event(
+        &mut self,
+        _irq: u32,
+        _irq_event: &EventFd,
+        _resample_event: Option<&EventFd>,
+    ) -> Result<()> {
+        unimplemented!("unregister_irq_event for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Route an IRQ line to an interrupt controller, or to a particular MSI vector.
+    fn route_irq(&mut self, _route: IrqRoute) -> Result<()> {
+        unimplemented!("route_irq for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Return a vector of all registered irq numbers and their associated events.  To be used by
+    /// the main thread to wait for irq events to be triggered.
+    fn irq_event_tokens(&self) -> Result<Vec<(u32, EventFd)>> {
+        unimplemented!("irq_event_tokens for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Either assert or deassert an IRQ line.  Sends to either an interrupt controller, or does
+    /// a send_msi if the irq is associated with an MSI.
+    fn service_irq(&mut self, _irq: u32, _level: bool) -> Result<()> {
+        unimplemented!("service_irq for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Broadcast an end of interrupt.
+    fn broadcast_eoi(&mut self, _vector: u8) -> Result<()> {
+        unimplemented!("broadcast_eoi for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Return true if there is a pending interrupt for the specified vcpu.
+    fn interrupt_requested(&self, _vcpu_id: usize) -> bool {
+        unimplemented!("interrupt_requested for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Check if the specified vcpu has any pending interrupts. Returns None for no interrupts,
+    /// otherwise Some(u32) should be the injected interrupt vector.
+    fn get_external_interrupt(&mut self, _vcpu_id: usize) -> Result<Option<u32>> {
+        unimplemented!("get_external_interrupt for KvmKernelIrqChip is not yet implemented");
+    }
+
+    /// Attempt to clone this IrqChip instance.
+    fn try_clone(&self) -> Result<Self> {
+        unimplemented!("try_clone for KvmKernelIrqChip is not yet implemented");
+    }
+}
+
+#[cfg(test)]
+mod tests {
+
+    use hypervisor::kvm::{Kvm, KvmVm};
+    use sys_util::GuestMemory;
+
+    use crate::irqchip::{IrqChip, KvmKernelIrqChip};
+    #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
+    use hypervisor::VmAarch64;
+    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+    use hypervisor::VmX86_64;
+
+    #[test]
+    fn create_kvm_kernel_irqchip() {
+        let kvm = Kvm::new().expect("failed to instantiate Kvm");
+        let mem = GuestMemory::new(&[]).unwrap();
+        let vm = KvmVm::new(&kvm, mem).expect("failed to instantiate vm");
+        let vcpu = vm.create_vcpu(0).expect("failed to instantiate vcpu");
+
+        let mut chip =
+            KvmKernelIrqChip::new(vm, 1).expect("failed to instantiate KvmKernelIrqChip");
+
+        chip.add_vcpu(0, vcpu).expect("failed to add vcpu");
+    }
+}
diff --git a/devices/src/irqchip/mod.rs b/devices/src/irqchip/mod.rs
new file mode 100644
index 0000000..a8023e9
--- /dev/null
+++ b/devices/src/irqchip/mod.rs
@@ -0,0 +1,64 @@
+// Copyright 2020 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+mod kvm;
+pub use self::kvm::KvmKernelIrqChip;
+
+use std::marker::{Send, Sized};
+
+use hypervisor::{IrqRoute, Vcpu};
+use sys_util::{EventFd, Result};
+
+/// Trait that abstracts interactions with interrupt controllers.
+///
+/// Each VM will have one IrqChip instance which is responsible for routing IRQ lines and
+/// registering IRQ events. Depending on the implementation, the IrqChip may interact with an
+/// underlying hypervisor API or emulate devices in userspace.
+///
+/// This trait is generic over a Vcpu type because some IrqChip implementations can support
+/// multiple hypervisors with a single implementation.
+pub trait IrqChip<V: Vcpu>: Send + Sized {
+    /// Add a vcpu to the irq chip.
+    fn add_vcpu(&mut self, vcpu_id: usize, vcpu: V) -> Result<()>;
+
+    /// Register an event that can trigger an interrupt for a particular GSI.
+    fn register_irq_event(
+        &mut self,
+        irq: u32,
+        irq_event: &EventFd,
+        resample_event: Option<&EventFd>,
+    ) -> Result<()>;
+
+    /// Unregister an event for a particular GSI.
+    fn unregister_irq_event(
+        &mut self,
+        irq: u32,
+        irq_event: &EventFd,
+        resample_event: Option<&EventFd>,
+    ) -> Result<()>;
+
+    /// Route an IRQ line to an interrupt controller, or to a particular MSI vector.
+    fn route_irq(&mut self, route: IrqRoute) -> Result<()>;
+
+    /// Return a vector of all registered irq numbers and their associated events.  To be used by
+    /// the main thread to wait for irq events to be triggered.
+    fn irq_event_tokens(&self) -> Result<Vec<(u32, EventFd)>>;
+
+    /// Either assert or deassert an IRQ line.  Sends to either an interrupt controller, or does
+    /// a send_msi if the irq is associated with an MSI.
+    fn service_irq(&mut self, irq: u32, level: bool) -> Result<()>;
+
+    /// Broadcast an end of interrupt.
+    fn broadcast_eoi(&mut self, vector: u8) -> Result<()>;
+
+    /// Return true if there is a pending interrupt for the specified vcpu.
+    fn interrupt_requested(&self, vcpu_id: usize) -> bool;
+
+    /// Check if the specified vcpu has any pending interrupts. Returns None for no interrupts,
+    /// otherwise Some(u32) should be the injected interrupt vector.
+    fn get_external_interrupt(&mut self, vcpu_id: usize) -> Result<Option<u32>>;
+
+    /// Attempt to clone this IrqChip instance.
+    fn try_clone(&self) -> Result<Self>;
+}
diff --git a/devices/src/lib.rs b/devices/src/lib.rs
index 01c2b46..c945e00 100644
--- a/devices/src/lib.rs
+++ b/devices/src/lib.rs
@@ -8,6 +8,7 @@ mod bus;
 mod cmos;
 mod i8042;
 mod ioapic;
+mod irqchip;
 mod pci;
 mod pic;
 mod pit;
@@ -30,6 +31,7 @@ pub use self::bus::{Bus, BusDevice, BusRange, BusResumeDevice};
 pub use self::cmos::Cmos;
 pub use self::i8042::I8042Device;
 pub use self::ioapic::{Ioapic, IOAPIC_BASE_ADDRESS, IOAPIC_MEM_LENGTH_BYTES};
+pub use self::irqchip::*;
 pub use self::pci::{
     Ac97Backend, Ac97Dev, Ac97Parameters, PciAddress, PciConfigIo, PciConfigMmio, PciDevice,
     PciDeviceError, PciInterruptPin, PciRoot, VfioPciDevice,
diff --git a/devices/src/usb/xhci/event_ring.rs b/devices/src/usb/xhci/event_ring.rs
index 1742c77..4711c46 100644
--- a/devices/src/usb/xhci/event_ring.rs
+++ b/devices/src/usb/xhci/event_ring.rs
@@ -144,11 +144,6 @@ impl EventRing {
         self.dequeue_pointer = addr;
     }
 
-    /// Get the enqueue pointer.
-    pub fn get_enqueue_pointer(&self) -> GuestAddress {
-        self.enqueue_pointer
-    }
-
     /// Check if event ring is empty.
     pub fn is_empty(&self) -> bool {
         self.enqueue_pointer == self.dequeue_pointer
diff --git a/devices/src/usb/xhci/interrupter.rs b/devices/src/usb/xhci/interrupter.rs
index c58ae59..cf79fd8 100644
--- a/devices/src/usb/xhci/interrupter.rs
+++ b/devices/src/usb/xhci/interrupter.rs
@@ -45,7 +45,6 @@ pub struct Interrupter {
     erdp: Register<u64>,
     event_handler_busy: bool,
     enabled: bool,
-    pending: bool,
     moderation_interval: u16,
     moderation_counter: u16,
     event_ring: EventRing,
@@ -61,7 +60,6 @@ impl Interrupter {
             erdp: regs.erdp.clone(),
             event_handler_busy: false,
             enabled: false,
-            pending: false,
             moderation_interval: 0,
             moderation_counter: 0,
             event_ring: EventRing::new(mem),
@@ -76,7 +74,6 @@ impl Interrupter {
     /// Add event to event ring.
     fn add_event(&mut self, trb: Trb) -> Result<()> {
         self.event_ring.add_event(trb).map_err(Error::AddEvent)?;
-        self.pending = true;
         self.interrupt_if_needed()
     }
 
@@ -169,9 +166,6 @@ impl Interrupter {
     pub fn set_event_ring_dequeue_pointer(&mut self, addr: GuestAddress) -> Result<()> {
         usb_debug!("interrupter set dequeue ptr addr {:#x}", addr.0);
         self.event_ring.set_dequeue_pointer(addr);
-        if addr == self.event_ring.get_enqueue_pointer() {
-            self.pending = false;
-        }
         self.interrupt_if_needed()
     }
 
@@ -186,7 +180,6 @@ impl Interrupter {
     pub fn interrupt(&mut self) -> Result<()> {
         usb_debug!("sending interrupt");
         self.event_handler_busy = true;
-        self.pending = false;
         self.usbsts.set_bits(USB_STS_EVENT_INTERRUPT);
         self.iman.set_bits(IMAN_INTERRUPT_PENDING);
         self.erdp.set_bits(ERDP_EVENT_HANDLER_BUSY);
@@ -194,7 +187,8 @@ impl Interrupter {
     }
 
     fn interrupt_if_needed(&mut self) -> Result<()> {
-        if self.enabled && self.pending && !self.event_handler_busy {
+        // TODO(dverkamp): re-add !self.event_handler_busy after solving https://crbug.com/1082930
+        if self.enabled && !self.event_ring.is_empty() {
             self.interrupt()?;
         }
         Ok(())
diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index c9dda55..80d5103 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -267,9 +267,7 @@ impl Worker {
         let status_offset = available_bytes
             .checked_sub(1)
             .ok_or(ExecuteError::MissingStatus)?;
-        let mut status_writer = writer
-            .split_at(status_offset)
-            .map_err(ExecuteError::Descriptor)?;
+        let mut status_writer = writer.split_at(status_offset);
 
         let status = match Block::execute_request(
             &mut reader,
diff --git a/devices/src/virtio/descriptor_utils.rs b/devices/src/virtio/descriptor_utils.rs
index f90264a..d65341b 100644
--- a/devices/src/virtio/descriptor_utils.rs
+++ b/devices/src/virtio/descriptor_utils.rs
@@ -3,7 +3,8 @@
 // found in the LICENSE file.
 
 use std::cmp;
-use std::collections::VecDeque;
+use std::convert::TryInto;
+use std::ffi::c_void;
 use std::fmt::{self, Display};
 use std::io::{self, Read, Write};
 use std::iter::FromIterator;
@@ -53,8 +54,10 @@ impl std::error::Error for Error {}
 
 #[derive(Clone)]
 struct DescriptorChainConsumer<'a> {
-    buffers: VecDeque<VolatileSlice<'a>>,
+    buffers: Vec<libc::iovec>,
+    current: usize,
     bytes_consumed: usize,
+    mem: PhantomData<&'a GuestMemory>,
 }
 
 impl<'a> DescriptorChainConsumer<'a> {
@@ -62,140 +65,136 @@ impl<'a> DescriptorChainConsumer<'a> {
         // This is guaranteed not to overflow because the total length of the chain
         // is checked during all creations of `DescriptorChainConsumer` (see
         // `Reader::new()` and `Writer::new()`).
-        self.buffers
+        self.get_remaining()
             .iter()
-            .fold(0usize, |count, vs| count + vs.size() as usize)
+            .fold(0usize, |count, buf| count + buf.iov_len)
     }
 
     fn bytes_consumed(&self) -> usize {
         self.bytes_consumed
     }
 
-    /// Consumes at most `count` bytes from the `DescriptorChain`. Callers must provide a function
-    /// that takes a `&[VolatileSlice]` and returns the total number of bytes consumed. This
-    /// function guarantees that the combined length of all the slices in the `&[VolatileSlice]` is
-    /// less than or equal to `count`.
+    /// Returns all the remaining buffers in the `DescriptorChain`. Calling this function does not
+    /// consume any bytes from the `DescriptorChain`. Instead callers should use the `consume`
+    /// method to advance the `DescriptorChain`. Multiple calls to `get` with no intervening calls
+    /// to `consume` will return the same data.
+    fn get_remaining(&self) -> &[libc::iovec] {
+        &self.buffers[self.current..]
+    }
+
+    /// Consumes `count` bytes from the `DescriptorChain`. If `count` is larger than
+    /// `self.available_bytes()` then all remaining bytes in the `DescriptorChain` will be consumed.
     ///
     /// # Errors
     ///
-    /// If the provided function returns any error then no bytes are consumed from the buffer and
-    /// the error is returned to the caller.
-    fn consume<F>(&mut self, count: usize, f: F) -> io::Result<usize>
-    where
-        F: FnOnce(&[VolatileSlice]) -> io::Result<usize>,
-    {
-        let mut buflen = 0;
-        let mut bufs = Vec::with_capacity(self.buffers.len());
-        for &vs in &self.buffers {
-            if buflen >= count {
+    /// Returns an error if the total bytes consumed by this `DescriptorChainConsumer` overflows a
+    /// usize.
+    fn consume(&mut self, mut count: usize) {
+        // The implementation is adapted from `IoSlice::advance` in libstd. We can't use
+        // `get_remaining` here because then the compiler complains that `self.current` is already
+        // borrowed and doesn't allow us to modify it.  We also need to borrow the iovecs mutably.
+        let current = self.current;
+        for buf in &mut self.buffers[current..] {
+            if count == 0 {
                 break;
             }
 
-            let rem = count - buflen;
-            if (rem as u64) < vs.size() {
-                let buf = vs.sub_slice(0, rem as u64).map_err(|e| {
-                    io::Error::new(io::ErrorKind::InvalidData, Error::VolatileMemoryError(e))
-                })?;
-                bufs.push(buf);
-                buflen += rem;
+            let consumed = if count < buf.iov_len {
+                // Safe because we know that the iovec pointed to valid memory and we are adding a
+                // value that is smaller than the length of the memory.
+                buf.iov_base = unsafe { (buf.iov_base as *mut u8).add(count) as *mut c_void };
+                buf.iov_len -= count;
+                count
             } else {
-                bufs.push(vs);
-                buflen += vs.size() as usize;
-            }
+                self.current += 1;
+                buf.iov_len
+            };
+
+            // This shouldn't overflow because `consumed <= buf.iov_len` and we already verified
+            // that adding all `buf.iov_len` values will not overflow when the Reader/Writer was
+            // constructed.
+            self.bytes_consumed += consumed;
+            count -= consumed;
         }
+    }
 
-        if bufs.is_empty() {
-            return Ok(0);
-        }
+    fn split_at(&mut self, offset: usize) -> DescriptorChainConsumer<'a> {
+        let mut other = self.clone();
+        other.consume(offset);
+        other.bytes_consumed = 0;
 
-        let bytes_consumed = f(&*bufs)?;
-
-        // This can happen if a driver tricks a device into reading/writing more data than
-        // fits in a `usize`.
-        let total_bytes_consumed =
-            self.bytes_consumed
-                .checked_add(bytes_consumed)
-                .ok_or_else(|| {
-                    io::Error::new(io::ErrorKind::InvalidData, Error::DescriptorChainOverflow)
-                })?;
-
-        let mut rem = bytes_consumed;
-        while let Some(vs) = self.buffers.pop_front() {
-            if (rem as u64) < vs.size() {
-                // Split the slice and push the remainder back into the buffer list. Safe because we
-                // know that `rem` is not out of bounds due to the check and we checked the bounds
-                // on `vs` when we added it to the buffer list.
-                self.buffers.push_front(vs.offset(rem as u64).unwrap());
+        let mut rem = offset;
+        let mut end = self.current;
+        for buf in &mut self.buffers[self.current..] {
+            if rem < buf.iov_len {
+                buf.iov_len = rem;
                 break;
             }
 
-            // No need for checked math because we know that `vs.size() <= rem`.
-            rem -= vs.size() as usize;
+            end += 1;
+            rem -= buf.iov_len;
         }
 
-        self.bytes_consumed = total_bytes_consumed;
+        self.buffers.truncate(end + 1);
 
-        Ok(bytes_consumed)
+        other
     }
 
-    fn split_at(&mut self, offset: usize) -> Result<DescriptorChainConsumer<'a>> {
-        let mut rem = offset;
-        let pos = self.buffers.iter().position(|vs| {
-            if (rem as u64) < vs.size() {
-                true
-            } else {
-                rem -= vs.size() as usize;
-                false
-            }
-        });
-
-        if let Some(at) = pos {
-            let mut other = self.buffers.split_off(at);
-
-            if rem > 0 {
-                // There must be at least one element in `other` because we checked
-                // its `size` value in the call to `position` above.
-                let front = other.pop_front().expect("empty VecDeque after split");
-                self.buffers.push_back(
-                    front
-                        .sub_slice(0, rem as u64)
-                        .map_err(Error::VolatileMemoryError)?,
-                );
-                other.push_front(
-                    front
-                        .offset(rem as u64)
-                        .map_err(Error::VolatileMemoryError)?,
-                );
-            }
+    // Temporary method for converting iovecs into VolatileSlices until we can change the
+    // ReadWriteVolatile traits. The irony here is that the standard implementation of the
+    // ReadWriteVolatile traits will convert the VolatileSlices back into iovecs.
+    fn get_volatile_slices(&mut self, mut count: usize) -> Vec<VolatileSlice> {
+        let bufs = self.get_remaining();
+        let mut iovs = Vec::with_capacity(bufs.len());
+        for b in bufs {
+            // Safe because we verified during construction that the memory at `b.iov_base` is
+            // `b.iov_len` bytes long. The lifetime of the `VolatileSlice` is tied to the lifetime
+            // of this `DescriptorChainConsumer`, which is in turn tied to the lifetime of the
+            // `GuestMemory` used to create it and so the memory will be available for the duration
+            // of the `VolatileSlice`.
+            let iov = unsafe {
+                if count < b.iov_len {
+                    VolatileSlice::new(
+                        b.iov_base as *mut u8,
+                        count.try_into().expect("usize doesn't fit in u64"),
+                    )
+                } else {
+                    VolatileSlice::new(
+                        b.iov_base as *mut u8,
+                        b.iov_len.try_into().expect("usize doesn't fit in u64"),
+                    )
+                }
+            };
 
-            Ok(DescriptorChainConsumer {
-                buffers: other,
-                bytes_consumed: 0,
-            })
-        } else if rem == 0 {
-            Ok(DescriptorChainConsumer {
-                buffers: VecDeque::new(),
-                bytes_consumed: 0,
-            })
-        } else {
-            Err(Error::SplitOutOfBounds(offset))
+            count -= iov.size() as usize;
+            iovs.push(iov);
         }
+
+        iovs
     }
 
     fn get_iovec(&mut self, len: usize) -> io::Result<DescriptorIovec<'a>> {
-        let mut iovec = Vec::new();
+        let mut iovec = Vec::with_capacity(self.get_remaining().len());
+
+        let mut rem = len;
+        for buf in self.get_remaining() {
+            let iov = if rem < buf.iov_len {
+                libc::iovec {
+                    iov_base: buf.iov_base,
+                    iov_len: rem,
+                }
+            } else {
+                buf.clone()
+            };
 
-        self.consume(len, |bufs| {
-            let mut total = 0;
-            for vs in bufs {
-                iovec.push(libc::iovec {
-                    iov_base: vs.as_ptr() as *mut libc::c_void,
-                    iov_len: vs.size() as usize,
-                });
-                total += vs.size() as usize;
+            rem -= iov.iov_len;
+            iovec.push(iov);
+
+            if rem == 0 {
+                break;
             }
-            Ok(total)
-        })?;
+        }
+        self.consume(len);
 
         Ok(DescriptorIovec {
             iovec,
@@ -250,14 +249,21 @@ impl<'a> Reader<'a> {
                     .checked_add(desc.len as usize)
                     .ok_or(Error::DescriptorChainOverflow)?;
 
-                mem.get_slice(desc.addr.offset(), desc.len.into())
-                    .map_err(Error::VolatileMemoryError)
+                let vs = mem
+                    .get_slice(desc.addr.offset(), desc.len.into())
+                    .map_err(Error::VolatileMemoryError)?;
+                Ok(libc::iovec {
+                    iov_base: vs.as_ptr() as *mut c_void,
+                    iov_len: vs.size() as usize,
+                })
             })
-            .collect::<Result<VecDeque<VolatileSlice<'a>>>>()?;
+            .collect::<Result<Vec<libc::iovec>>>()?;
         Ok(Reader {
             buffer: DescriptorChainConsumer {
                 buffers,
+                current: 0,
                 bytes_consumed: 0,
+                mem: PhantomData,
             },
         })
     }
@@ -305,8 +311,10 @@ impl<'a> Reader<'a> {
         mut dst: F,
         count: usize,
     ) -> io::Result<usize> {
-        self.buffer
-            .consume(count, |bufs| dst.write_vectored_volatile(bufs))
+        let iovs = self.buffer.get_volatile_slices(count);
+        let written = dst.write_vectored_volatile(&iovs[..])?;
+        self.buffer.consume(written);
+        Ok(written)
     }
 
     /// Reads data from the descriptor chain buffer into a File at offset `off`.
@@ -319,8 +327,10 @@ impl<'a> Reader<'a> {
         count: usize,
         off: u64,
     ) -> io::Result<usize> {
-        self.buffer
-            .consume(count, |bufs| dst.write_vectored_at_volatile(bufs, off))
+        let iovs = self.buffer.get_volatile_slices(count);
+        let written = dst.write_vectored_at_volatile(&iovs[..], off)?;
+        self.buffer.consume(written);
+        Ok(written)
     }
 
     pub fn read_exact_to<F: FileReadWriteVolatile>(
@@ -382,12 +392,14 @@ impl<'a> Reader<'a> {
         self.buffer.bytes_consumed()
     }
 
-    /// Splits this `Reader` into two at the given offset in the `DescriptorChain` buffer.
-    /// After the split, `self` will be able to read up to `offset` bytes while the returned
-    /// `Reader` can read up to `available_bytes() - offset` bytes.  Returns an error if
-    /// `offset > self.available_bytes()`.
-    pub fn split_at(&mut self, offset: usize) -> Result<Reader<'a>> {
-        self.buffer.split_at(offset).map(|buffer| Reader { buffer })
+    /// Splits this `Reader` into two at the given offset in the `DescriptorChain` buffer. After the
+    /// split, `self` will be able to read up to `offset` bytes while the returned `Reader` can read
+    /// up to `available_bytes() - offset` bytes. If `offset > self.available_bytes()`, then the
+    /// returned `Reader` will not be able to read any bytes.
+    pub fn split_at(&mut self, offset: usize) -> Reader<'a> {
+        Reader {
+            buffer: self.buffer.split_at(offset),
+        }
     }
 
     /// Returns a DescriptorIovec for the next `len` bytes of the descriptor chain
@@ -399,27 +411,25 @@ impl<'a> Reader<'a> {
 
 impl<'a> io::Read for Reader<'a> {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        self.buffer.consume(buf.len(), |bufs| {
-            let mut rem = buf;
-            let mut total = 0;
-            for vs in bufs {
-                // This is guaranteed by the implementation of `consume`.
-                debug_assert_eq!(vs.size(), cmp::min(rem.len() as u64, vs.size()));
-
-                // Safe because we have already verified that `vs` points to valid memory.
-                unsafe {
-                    copy_nonoverlapping(
-                        vs.as_ptr() as *const u8,
-                        rem.as_mut_ptr(),
-                        vs.size() as usize,
-                    );
-                }
-                let copied = vs.size() as usize;
-                rem = &mut rem[copied..];
-                total += copied;
+        let mut rem = buf;
+        let mut total = 0;
+        for b in self.buffer.get_remaining() {
+            if rem.len() == 0 {
+                break;
             }
-            Ok(total)
-        })
+
+            let count = cmp::min(rem.len(), b.iov_len);
+
+            // Safe because we have already verified that `b` points to valid memory.
+            unsafe {
+                copy_nonoverlapping(b.iov_base as *const u8, rem.as_mut_ptr(), count);
+            }
+            rem = &mut rem[count..];
+            total += count;
+        }
+
+        self.buffer.consume(total);
+        Ok(total)
     }
 }
 
@@ -450,14 +460,21 @@ impl<'a> Writer<'a> {
                     .checked_add(desc.len as usize)
                     .ok_or(Error::DescriptorChainOverflow)?;
 
-                mem.get_slice(desc.addr.offset(), desc.len.into())
-                    .map_err(Error::VolatileMemoryError)
+                let vs = mem
+                    .get_slice(desc.addr.offset(), desc.len.into())
+                    .map_err(Error::VolatileMemoryError)?;
+                Ok(libc::iovec {
+                    iov_base: vs.as_ptr() as *mut c_void,
+                    iov_len: vs.size() as usize,
+                })
             })
-            .collect::<Result<VecDeque<VolatileSlice<'a>>>>()?;
+            .collect::<Result<Vec<libc::iovec>>>()?;
         Ok(Writer {
             buffer: DescriptorChainConsumer {
                 buffers,
+                current: 0,
                 bytes_consumed: 0,
+                mem: PhantomData,
             },
         })
     }
@@ -495,8 +512,10 @@ impl<'a> Writer<'a> {
         mut src: F,
         count: usize,
     ) -> io::Result<usize> {
-        self.buffer
-            .consume(count, |bufs| src.read_vectored_volatile(bufs))
+        let iovs = self.buffer.get_volatile_slices(count);
+        let read = src.read_vectored_volatile(&iovs[..])?;
+        self.buffer.consume(read);
+        Ok(read)
     }
 
     /// Writes data to the descriptor chain buffer from a File at offset `off`.
@@ -509,8 +528,10 @@ impl<'a> Writer<'a> {
         count: usize,
         off: u64,
     ) -> io::Result<usize> {
-        self.buffer
-            .consume(count, |bufs| src.read_vectored_at_volatile(bufs, off))
+        let iovs = self.buffer.get_volatile_slices(count);
+        let read = src.read_vectored_at_volatile(&iovs[..], off)?;
+        self.buffer.consume(read);
+        Ok(read)
     }
 
     pub fn write_all_from<F: FileReadWriteVolatile>(
@@ -565,12 +586,14 @@ impl<'a> Writer<'a> {
         self.buffer.bytes_consumed()
     }
 
-    /// Splits this `Writer` into two at the given offset in the `DescriptorChain` buffer.
-    /// After the split, `self` will be able to write up to `offset` bytes while the returned
-    /// `Writer` can write up to `available_bytes() - offset` bytes.  Returns an error if
-    /// `offset > self.available_bytes()`.
-    pub fn split_at(&mut self, offset: usize) -> Result<Writer<'a>> {
-        self.buffer.split_at(offset).map(|buffer| Writer { buffer })
+    /// Splits this `Writer` into two at the given offset in the `DescriptorChain` buffer. After the
+    /// split, `self` will be able to write up to `offset` bytes while the returned `Writer` can
+    /// write up to `available_bytes() - offset` bytes. If `offset > self.available_bytes()`, then
+    /// the returned `Writer` will not be able to write any bytes.
+    pub fn split_at(&mut self, offset: usize) -> Writer<'a> {
+        Writer {
+            buffer: self.buffer.split_at(offset),
+        }
     }
 
     /// Returns a DescriptorIovec for the next `len` bytes of the descriptor chain
@@ -582,23 +605,24 @@ impl<'a> Writer<'a> {
 
 impl<'a> io::Write for Writer<'a> {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        self.buffer.consume(buf.len(), |bufs| {
-            let mut rem = buf;
-            let mut total = 0;
-            for vs in bufs {
-                // This is guaranteed by the implementation of `consume`.
-                debug_assert_eq!(vs.size(), cmp::min(rem.len() as u64, vs.size()));
-
-                // Safe because we have already verified that `vs` points to valid memory.
-                unsafe {
-                    copy_nonoverlapping(rem.as_ptr(), vs.as_ptr(), vs.size() as usize);
-                }
-                let copied = vs.size() as usize;
-                rem = &rem[copied..];
-                total += copied;
+        let mut rem = buf;
+        let mut total = 0;
+        for b in self.buffer.get_remaining() {
+            if rem.len() == 0 {
+                break;
             }
-            Ok(total)
-        })
+
+            let count = cmp::min(rem.len(), b.iov_len);
+            // Safe because we have already verified that `vs` points to valid memory.
+            unsafe {
+                copy_nonoverlapping(rem.as_ptr(), b.iov_base as *mut u8, count);
+            }
+            rem = &rem[count..];
+            total += count;
+        }
+
+        self.buffer.consume(total);
+        Ok(total)
     }
 
     fn flush(&mut self) -> io::Result<()> {
@@ -1031,7 +1055,7 @@ mod tests {
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
-        let other = reader.split_at(32).expect("failed to split Reader");
+        let other = reader.split_at(32);
         assert_eq!(reader.available_bytes(), 32);
         assert_eq!(other.available_bytes(), 96);
     }
@@ -1060,7 +1084,7 @@ mod tests {
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
-        let other = reader.split_at(24).expect("failed to split Reader");
+        let other = reader.split_at(24);
         assert_eq!(reader.available_bytes(), 24);
         assert_eq!(other.available_bytes(), 104);
     }
@@ -1089,7 +1113,7 @@ mod tests {
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
-        let other = reader.split_at(128).expect("failed to split Reader");
+        let other = reader.split_at(128);
         assert_eq!(reader.available_bytes(), 128);
         assert_eq!(other.available_bytes(), 0);
     }
@@ -1118,7 +1142,7 @@ mod tests {
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
-        let other = reader.split_at(0).expect("failed to split Reader");
+        let other = reader.split_at(0);
         assert_eq!(reader.available_bytes(), 0);
         assert_eq!(other.available_bytes(), 128);
     }
@@ -1147,9 +1171,12 @@ mod tests {
         .expect("create_descriptor_chain failed");
         let mut reader = Reader::new(&memory, chain).expect("failed to create Reader");
 
-        if let Ok(_) = reader.split_at(256) {
-            panic!("successfully split Reader with out of bounds offset");
-        }
+        let other = reader.split_at(256);
+        assert_eq!(
+            other.available_bytes(),
+            0,
+            "Reader returned from out-of-bounds split still has available bytes"
+        );
     }
 
     #[test]
diff --git a/devices/src/virtio/fs/server.rs b/devices/src/virtio/fs/server.rs
index 33b7c98..c1af80c 100644
--- a/devices/src/virtio/fs/server.rs
+++ b/devices/src/virtio/fs/server.rs
@@ -496,10 +496,7 @@ impl<F: FileSystem + Sync> Server<F> {
         };
 
         // Split the writer into 2 pieces: one for the `OutHeader` and the rest for the data.
-        let data_writer = ZCWriter(
-            w.split_at(size_of::<OutHeader>())
-                .map_err(Error::InvalidDescriptorChain)?,
-        );
+        let data_writer = ZCWriter(w.split_at(size_of::<OutHeader>()));
 
         match self.fs.read(
             Context::from(in_header),
@@ -910,9 +907,7 @@ impl<F: FileSystem + Sync> Server<F> {
         }
 
         // Skip over enough bytes for the header.
-        let mut cursor = w
-            .split_at(size_of::<OutHeader>())
-            .map_err(Error::InvalidDescriptorChain)?;
+        let mut cursor = w.split_at(size_of::<OutHeader>());
 
         let res = if plus {
             self.fs.readdirplus(
diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs
index b305089..2873b74 100644
--- a/devices/src/virtio/gpu/mod.rs
+++ b/devices/src/virtio/gpu/mod.rs
@@ -111,7 +111,7 @@ trait Backend {
 
     /// Constructs a backend.
     fn build(
-        possible_displays: &[DisplayBackend],
+        display: GpuDisplay,
         display_width: u32,
         display_height: u32,
         renderer_flags: RendererFlags,
@@ -345,9 +345,28 @@ impl BackendKind {
         gpu_device_socket: VmMemoryControlRequestSocket,
         pci_bar: Alloc,
     ) -> Option<Box<dyn Backend>> {
+        let mut display_opt = None;
+        for display in possible_displays {
+            match display.build() {
+                Ok(c) => {
+                    display_opt = Some(c);
+                    break;
+                }
+                Err(e) => error!("failed to open display: {}", e),
+            };
+        }
+
+        let display = match display_opt {
+            Some(d) => d,
+            None => {
+                error!("failed to open any displays");
+                return None;
+            }
+        };
+
         match self {
             BackendKind::Virtio2D => Virtio2DBackend::build(
-                possible_displays,
+                display,
                 display_width,
                 display_height,
                 renderer_flags,
@@ -356,7 +375,7 @@ impl BackendKind {
                 pci_bar,
             ),
             BackendKind::Virtio3D => Virtio3DBackend::build(
-                possible_displays,
+                display,
                 display_width,
                 display_height,
                 renderer_flags,
@@ -366,7 +385,7 @@ impl BackendKind {
             ),
             #[cfg(feature = "gfxstream")]
             BackendKind::VirtioGfxStream => VirtioGfxStreamBackend::build(
-                possible_displays,
+                display,
                 display_width,
                 display_height,
                 renderer_flags,
@@ -977,10 +996,6 @@ impl DisplayBackend {
             DisplayBackend::Stub => GpuDisplay::open_stub(),
         }
     }
-
-    fn is_x(&self) -> bool {
-        matches!(self, DisplayBackend::X(_))
-    }
 }
 
 pub struct Gpu {
diff --git a/devices/src/virtio/gpu/virtio_2d_backend.rs b/devices/src/virtio/gpu/virtio_2d_backend.rs
index a51a664..fd85bef 100644
--- a/devices/src/virtio/gpu/virtio_2d_backend.rs
+++ b/devices/src/virtio/gpu/virtio_2d_backend.rs
@@ -22,7 +22,7 @@ use vm_control::VmMemoryControlRequestSocket;
 
 use super::protocol::GpuResponse;
 pub use super::virtio_backend::{VirtioBackend, VirtioResource};
-use crate::virtio::gpu::{Backend, DisplayBackend, VIRTIO_F_VERSION_1};
+use crate::virtio::gpu::{Backend, VIRTIO_F_VERSION_1};
 use crate::virtio::resource_bridge::ResourceResponse;
 
 #[derive(Debug)]
@@ -427,7 +427,7 @@ impl Backend for Virtio2DBackend {
 
     /// Returns the underlying Backend.
     fn build(
-        possible_displays: &[DisplayBackend],
+        display: GpuDisplay,
         display_width: u32,
         display_height: u32,
         _renderer_flags: RendererFlags,
@@ -435,24 +435,6 @@ impl Backend for Virtio2DBackend {
         _gpu_device_socket: VmMemoryControlRequestSocket,
         _pci_bar: Alloc,
     ) -> Option<Box<dyn Backend>> {
-        let mut display_opt = None;
-        for display in possible_displays {
-            match display.build() {
-                Ok(c) => {
-                    display_opt = Some(c);
-                    break;
-                }
-                Err(e) => error!("failed to open display: {}", e),
-            };
-        }
-        let display = match display_opt {
-            Some(d) => d,
-            None => {
-                error!("failed to open any displays");
-                return None;
-            }
-        };
-
         Some(Box::new(Virtio2DBackend::new(
             display,
             display_width,
diff --git a/devices/src/virtio/gpu/virtio_3d_backend.rs b/devices/src/virtio/gpu/virtio_3d_backend.rs
index 692bedc..f7899be 100644
--- a/devices/src/virtio/gpu/virtio_3d_backend.rs
+++ b/devices/src/virtio/gpu/virtio_3d_backend.rs
@@ -31,8 +31,8 @@ use super::protocol::{
 };
 pub use crate::virtio::gpu::virtio_backend::{VirtioBackend, VirtioResource};
 use crate::virtio::gpu::{
-    Backend, DisplayBackend, VIRTIO_F_VERSION_1, VIRTIO_GPU_F_HOST_VISIBLE,
-    VIRTIO_GPU_F_RESOURCE_UUID, VIRTIO_GPU_F_RESOURCE_V2, VIRTIO_GPU_F_VIRGL, VIRTIO_GPU_F_VULKAN,
+    Backend, VIRTIO_F_VERSION_1, VIRTIO_GPU_F_HOST_VISIBLE, VIRTIO_GPU_F_RESOURCE_UUID,
+    VIRTIO_GPU_F_RESOURCE_V2, VIRTIO_GPU_F_VIRGL, VIRTIO_GPU_F_VULKAN,
 };
 use crate::virtio::resource_bridge::{PlaneInfo, ResourceInfo, ResourceResponse};
 
@@ -246,7 +246,7 @@ impl Backend for Virtio3DBackend {
 
     /// Returns the underlying Backend.
     fn build(
-        possible_displays: &[DisplayBackend],
+        display: GpuDisplay,
         display_width: u32,
         display_height: u32,
         renderer_flags: RendererFlags,
@@ -255,31 +255,14 @@ impl Backend for Virtio3DBackend {
         pci_bar: Alloc,
     ) -> Option<Box<dyn Backend>> {
         let mut renderer_flags = renderer_flags;
-        let mut display_opt = None;
-        for display in possible_displays {
-            match display.build() {
-                Ok(c) => {
-                    // If X11 is being used, that's an indication that the renderer should also be
-                    // using glx. Otherwise, we are likely in an enviroment in which GBM will work
-                    // for doing allocations of buffers we wish to display. TODO(zachr): this is a
-                    // heuristic (or terrible hack depending on your POV). We should do something
-                    // either smarter or more configurable.
-                    if display.is_x() {
-                        renderer_flags = RendererFlags::new().use_glx(true);
-                    }
-                    display_opt = Some(c);
-                    break;
-                }
-                Err(e) => error!("failed to open display: {}", e),
-            };
+        if display.is_x() {
+            // If X11 is being used, that's an indication that the renderer should also be
+            // using glx. Otherwise, we are likely in an enviroment in which GBM will work
+            // for doing allocations of buffers we wish to display. TODO(zachr): this is a
+            // heuristic (or terrible hack depending on your POV). We should do something
+            // either smarter or more configurable.
+            renderer_flags = RendererFlags::new().use_glx(true);
         }
-        let display = match display_opt {
-            Some(d) => d,
-            None => {
-                error!("failed to open any displays");
-                return None;
-            }
-        };
 
         if cfg!(debug_assertions) {
             let ret = unsafe { libc::dup2(libc::STDOUT_FILENO, libc::STDERR_FILENO) };
diff --git a/devices/src/virtio/gpu/virtio_gfxstream_backend.rs b/devices/src/virtio/gpu/virtio_gfxstream_backend.rs
index 2a49da8..d8ef793 100644
--- a/devices/src/virtio/gpu/virtio_gfxstream_backend.rs
+++ b/devices/src/virtio/gpu/virtio_gfxstream_backend.rs
@@ -25,7 +25,7 @@ use vm_control::VmMemoryControlRequestSocket;
 
 use super::protocol::GpuResponse;
 pub use super::virtio_backend::{VirtioBackend, VirtioResource};
-use crate::virtio::gpu::{Backend, DisplayBackend, VIRTIO_F_VERSION_1, VIRTIO_GPU_F_VIRGL};
+use crate::virtio::gpu::{Backend, VIRTIO_F_VERSION_1, VIRTIO_GPU_F_VIRGL};
 use crate::virtio::resource_bridge::ResourceResponse;
 
 // C definitions related to gfxstream
@@ -282,7 +282,7 @@ impl Backend for VirtioGfxStreamBackend {
 
     /// Returns the underlying Backend.
     fn build(
-        possible_displays: &[DisplayBackend],
+        display: GpuDisplay,
         display_width: u32,
         display_height: u32,
         _renderer_flags: RendererFlags,
@@ -290,25 +290,6 @@ impl Backend for VirtioGfxStreamBackend {
         gpu_device_socket: VmMemoryControlRequestSocket,
         pci_bar: Alloc,
     ) -> Option<Box<dyn Backend>> {
-        let mut display_opt = None;
-        for display in possible_displays {
-            match display.build() {
-                Ok(c) => {
-                    display_opt = Some(c);
-                    break;
-                }
-                Err(e) => error!("failed to open display: {}", e),
-            };
-        }
-
-        let display = match display_opt {
-            Some(d) => d,
-            None => {
-                error!("failed to open any displays");
-                return None;
-            }
-        };
-
         Some(Box::new(VirtioGfxStreamBackend::new(
             display,
             display_width,