hypervisor: add Vm user memory region functions

The separate Vm functions for MemoryMappings and MemoryMappingArenas
have been combined and now use a MappedRegion trait that the mappings
implement.  msync_memory_region replaces the get_mmap_arena function,
which is used by VmMsyncRequest.  Since Vm uses mutexes for cloning, it
can't return mem region references.

BUG=chromium:1077058
TEST=cargo test, cargo test -p sys_util, cargo test -p hypervisor

Change-Id: If257b16ee34d07820ae7ebdb9a3a598a41df013c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2202845
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Gurchetan Singh <gurchetansingh@chromium.org>

7 files changed, 276 insertions, 79 deletions

diff --git a/devices/src/pci/vfio_pci.rs b/devices/src/pci/vfio_pci.rs
index 7b5c233..fa27dec 100644
--- a/devices/src/pci/vfio_pci.rs
+++ b/devices/src/pci/vfio_pci.rs
@@ -9,7 +9,7 @@ use std::u32;
 use kvm::Datamatch;
 use msg_socket::{MsgReceiver, MsgSender};
 use resources::{Alloc, MmioType, SystemAllocator};
-use sys_util::{error, EventFd, MemoryMapping};
+use sys_util::{error, EventFd, MappedRegion, MemoryMapping};
 
 use vfio_sys::*;
 use vm_control::{
diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs
index bf87e3d..abffdd5 100644
--- a/hypervisor/src/kvm/mod.rs
+++ b/hypervisor/src/kvm/mod.rs
@@ -15,18 +15,16 @@ use std::os::raw::{c_char, c_ulong};
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::sync::Arc;
 
-use libc::{open, O_CLOEXEC, O_RDWR};
+use libc::{open, EFAULT, EINVAL, EIO, ENOENT, ENOSPC, EOVERFLOW, O_CLOEXEC, O_RDWR};
 
 use kvm_sys::*;
 use sync::Mutex;
 use sys_util::{
     errno_result, ioctl, ioctl_with_ref, ioctl_with_val, AsRawDescriptor, Error, FromRawDescriptor,
-    GuestMemory, RawDescriptor, Result, SafeDescriptor,
+    GuestAddress, GuestMemory, MappedRegion, MmapError, RawDescriptor, Result, SafeDescriptor,
 };
 
-use crate::{
-    ClockState, Hypervisor, HypervisorCap, MappedRegion, RunnableVcpu, Vcpu, VcpuExit, Vm,
-};
+use crate::{ClockState, Hypervisor, HypervisorCap, RunnableVcpu, Vcpu, VcpuExit, Vm};
 
 // Wrapper around KVM_SET_USER_MEMORY_REGION ioctl, which creates, modifies, or deletes a mapping
 // from guest physical to host user pages.
@@ -153,7 +151,7 @@ impl KvmVm {
                     index as u32,
                     false,
                     false,
-                    guest_addr.offset() as u64,
+                    guest_addr.offset(),
                     size as u64,
                     host_addr as *mut u8,
                 )
@@ -215,6 +213,75 @@ impl Vm for KvmVm {
         &self.guest_mem
     }
 
+    fn add_memory_region(
+        &mut self,
+        guest_addr: GuestAddress,
+        mem: Box<dyn MappedRegion>,
+        read_only: bool,
+        log_dirty_pages: bool,
+    ) -> Result<u32> {
+        let size = mem.size() as u64;
+        let end_addr = guest_addr.checked_add(size).ok_or(Error::new(EOVERFLOW))?;
+        if self.guest_mem.range_overlap(guest_addr, end_addr) {
+            return Err(Error::new(ENOSPC));
+        }
+        let mut regions = self.mem_regions.lock();
+        let mut gaps = self.mem_slot_gaps.lock();
+        let slot = match gaps.pop() {
+            Some(gap) => gap.0,
+            None => (regions.len() + self.guest_mem.num_regions() as usize) as u32,
+        };
+
+        // Safe because we check that the given guest address is valid and has no overlaps. We also
+        // know that the pointer and size are correct because the MemoryMapping interface ensures
+        // this. We take ownership of the memory mapping so that it won't be unmapped until the slot
+        // is removed.
+        let res = unsafe {
+            set_user_memory_region(
+                &self.vm,
+                slot,
+                read_only,
+                log_dirty_pages,
+                guest_addr.offset() as u64,
+                size,
+                mem.as_ptr(),
+            )
+        };
+
+        if let Err(e) = res {
+            gaps.push(MemSlot(slot));
+            return Err(e);
+        }
+        regions.insert(slot, mem);
+        Ok(slot)
+    }
+
+    fn msync_memory_region(&mut self, slot: u32, offset: usize, size: usize) -> Result<()> {
+        let mut regions = self.mem_regions.lock();
+        let mem = regions.get_mut(&slot).ok_or(Error::new(ENOENT))?;
+
+        mem.msync(offset, size).map_err(|err| match err {
+            MmapError::InvalidAddress => Error::new(EFAULT),
+            MmapError::NotPageAligned => Error::new(EINVAL),
+            MmapError::SystemCallFailed(e) => e,
+            _ => Error::new(EIO),
+        })
+    }
+
+    fn remove_memory_region(&mut self, slot: u32) -> Result<()> {
+        let mut regions = self.mem_regions.lock();
+        if !regions.contains_key(&slot) {
+            return Err(Error::new(ENOENT));
+        }
+        // Safe because the slot is checked against the list of memory slots.
+        unsafe {
+            set_user_memory_region(&self.vm, slot, false, false, 0, 0, std::ptr::null_mut())?;
+        }
+        self.mem_slot_gaps.lock().push(MemSlot(slot));
+        regions.remove(&slot);
+        Ok(())
+    }
+
     fn get_pvclock(&self) -> Result<ClockState> {
         self.get_pvclock_arch()
     }
@@ -303,7 +370,7 @@ impl<'a> TryFrom<&'a HypervisorCap> for KvmCap {
 mod tests {
     use super::*;
     use std::thread;
-    use sys_util::GuestAddress;
+    use sys_util::{GuestAddress, MemoryMapping, MemoryMappingArena};
 
     #[test]
     fn new() {
@@ -354,4 +421,79 @@ mod tests {
         let read_val: u8 = vm.get_memory().read_obj_from_addr(obj_addr).unwrap();
         assert_eq!(read_val, 67u8);
     }
+
+    #[test]
+    fn add_memory() {
+        let kvm = Kvm::new().unwrap();
+        let gm =
+            GuestMemory::new(&[(GuestAddress(0), 0x1000), (GuestAddress(0x5000), 0x5000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        let mem_size = 0x1000;
+        let mem = MemoryMapping::new(mem_size).unwrap();
+        vm.add_memory_region(GuestAddress(0x1000), Box::new(mem), false, false)
+            .unwrap();
+        let mem = MemoryMapping::new(mem_size).unwrap();
+        vm.add_memory_region(GuestAddress(0x10000), Box::new(mem), false, false)
+            .unwrap();
+    }
+
+    #[test]
+    fn add_memory_ro() {
+        let kvm = Kvm::new().unwrap();
+        let gm = GuestMemory::new(&[(GuestAddress(0), 0x1000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        let mem_size = 0x1000;
+        let mem = MemoryMapping::new(mem_size).unwrap();
+        vm.add_memory_region(GuestAddress(0x1000), Box::new(mem), true, false)
+            .unwrap();
+    }
+
+    #[test]
+    fn remove_memory() {
+        let kvm = Kvm::new().unwrap();
+        let gm = GuestMemory::new(&[(GuestAddress(0), 0x1000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        let mem_size = 0x1000;
+        let mem = MemoryMapping::new(mem_size).unwrap();
+        let slot = vm
+            .add_memory_region(GuestAddress(0x1000), Box::new(mem), false, false)
+            .unwrap();
+        vm.remove_memory_region(slot).unwrap();
+    }
+
+    #[test]
+    fn remove_invalid_memory() {
+        let kvm = Kvm::new().unwrap();
+        let gm = GuestMemory::new(&[(GuestAddress(0), 0x1000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        assert!(vm.remove_memory_region(0).is_err());
+    }
+
+    #[test]
+    fn overlap_memory() {
+        let kvm = Kvm::new().unwrap();
+        let gm = GuestMemory::new(&[(GuestAddress(0), 0x10000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        let mem_size = 0x2000;
+        let mem = MemoryMapping::new(mem_size).unwrap();
+        assert!(vm
+            .add_memory_region(GuestAddress(0x2000), Box::new(mem), false, false)
+            .is_err());
+    }
+
+    #[test]
+    fn sync_memory() {
+        let kvm = Kvm::new().unwrap();
+        let gm =
+            GuestMemory::new(&[(GuestAddress(0), 0x1000), (GuestAddress(0x5000), 0x5000)]).unwrap();
+        let mut vm = KvmVm::new(&kvm, gm).unwrap();
+        let mem_size = 0x1000;
+        let mem = MemoryMappingArena::new(mem_size).unwrap();
+        let slot = vm
+            .add_memory_region(GuestAddress(0x1000), Box::new(mem), false, false)
+            .unwrap();
+        vm.msync_memory_region(slot, mem_size, 0).unwrap();
+        assert!(vm.msync_memory_region(slot, mem_size + 1, 0).is_err());
+        assert!(vm.msync_memory_region(slot + 1, mem_size, 0).is_err());
+    }
 }
diff --git a/hypervisor/src/lib.rs b/hypervisor/src/lib.rs
index ee0bcaf..f098720 100644
--- a/hypervisor/src/lib.rs
+++ b/hypervisor/src/lib.rs
@@ -12,7 +12,7 @@ pub mod x86_64;
 
 use std::ops::{Deref, DerefMut};
 
-use sys_util::{GuestMemory, Result};
+use sys_util::{GuestAddress, GuestMemory, MappedRegion, Result};
 
 #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
 pub use crate::aarch64::*;
@@ -34,6 +34,35 @@ pub trait Vm: Send + Sized {
     /// Gets the guest-mapped memory for the Vm.
     fn get_memory(&self) -> &GuestMemory;
 
+    /// Inserts the given `MappedRegion` into the VM's address space at `guest_addr`.
+    ///
+    /// The slot that was assigned the memory mapping is returned on success.  The slot can be given
+    /// to `Vm::remove_memory_region` to remove the memory from the VM's address space and take back
+    /// ownership of `mem_region`.
+    ///
+    /// Note that memory inserted into the VM's address space must not overlap with any other memory
+    /// slot's region.
+    ///
+    /// If `read_only` is true, the guest will be able to read the memory as normal, but attempts to
+    /// write will trigger a mmio VM exit, leaving the memory untouched.
+    ///
+    /// If `log_dirty_pages` is true, the slot number can be used to retrieve the pages written to
+    /// by the guest with `get_dirty_log`.
+    fn add_memory_region(
+        &mut self,
+        guest_addr: GuestAddress,
+        mem_region: Box<dyn MappedRegion>,
+        read_only: bool,
+        log_dirty_pages: bool,
+    ) -> Result<u32>;
+
+    /// Does a synchronous msync of the memory mapped at `slot`, syncing `size` bytes starting at
+    /// `offset` from the start of the region.  `offset` must be page aligned.
+    fn msync_memory_region(&mut self, slot: u32, offset: usize, size: usize) -> Result<()>;
+
+    /// Removes and drops the `UserMemoryRegion` that was previously added at the given slot.
+    fn remove_memory_region(&mut self, slot: u32) -> Result<()>;
+
     /// Retrieves the current timestamp of the paravirtual clock as seen by the current guest.
     /// Only works on VMs that support `VmCap::PvClock`.
     fn get_pvclock(&self) -> Result<ClockState>;
@@ -69,22 +98,6 @@ pub trait RunnableVcpu: Deref<Target = <Self as RunnableVcpu>::Vcpu> + DerefMut
     fn run(&self) -> Result<VcpuExit>;
 }
 
-/// A memory region in the current process that can be mapped into the guest's memory.
-///
-/// Safe when implementers guarantee `ptr`..`ptr+size` is an mmaped region owned by this object that
-/// can't be unmapped during the `MappedRegion`'s lifetime.
-pub unsafe trait MappedRegion: Send + Sync {
-    /// Returns a pointer to the beginning of the memory region. Should only be
-    /// used for passing this region to ioctls for setting guest memory.
-    fn as_ptr(&self) -> *mut u8;
-
-    /// Returns the size of the memory region in bytes.
-    fn size(&self) -> usize;
-
-    /// Flushes changes to this memory region to the backing file.
-    fn msync(&self) -> Result<()>;
-}
-
 /// A reason why a VCPU exited. One of these returns every time `Vcpu::run` is called.
 #[derive(Debug)]
 pub enum VcpuExit {
diff --git a/kvm/src/lib.rs b/kvm/src/lib.rs
index 1b06b39..3695f47 100644
--- a/kvm/src/lib.rs
+++ b/kvm/src/lib.rs
@@ -28,7 +28,7 @@ use msg_socket::MsgOnSocket;
 use sys_util::{
     block_signal, ioctl, ioctl_with_mut_ptr, ioctl_with_mut_ref, ioctl_with_ptr, ioctl_with_ref,
     ioctl_with_val, pagesize, signal, unblock_signal, warn, Error, EventFd, GuestAddress,
-    GuestMemory, MemoryMapping, MemoryMappingArena, Result, SIGRTMIN,
+    GuestMemory, MappedRegion, MemoryMapping, MemoryMappingArena, Result, SIGRTMIN,
 };
 
 pub use crate::cap::*;
diff --git a/sys_util/src/guest_memory.rs b/sys_util/src/guest_memory.rs
index 60b775a..b13b8cc 100644
--- a/sys_util/src/guest_memory.rs
+++ b/sys_util/src/guest_memory.rs
@@ -13,7 +13,7 @@ use std::result;
 use std::sync::Arc;
 
 use crate::guest_address::GuestAddress;
-use crate::mmap::{self, MemoryMapping};
+use crate::mmap::{self, MappedRegion, MemoryMapping};
 use crate::shm::{MemfdSeals, SharedMemory};
 use crate::{errno, pagesize};
 use data_model::volatile_memory::*;
diff --git a/sys_util/src/mmap.rs b/sys_util/src/mmap.rs
index 64ffe17..0a67d88 100644
--- a/sys_util/src/mmap.rs
+++ b/sys_util/src/mmap.rs
@@ -105,6 +105,58 @@ impl Into<c_int> for Protection {
     }
 }
 
+/// Validates that `offset`..`offset+range_size` lies within the bounds of a memory mapping of
+/// `mmap_size` bytes.  Also checks for any overflow.
+fn validate_includes_range(mmap_size: usize, offset: usize, range_size: usize) -> Result<()> {
+    // Ensure offset + size doesn't overflow
+    let end_offset = offset
+        .checked_add(range_size)
+        .ok_or(Error::InvalidAddress)?;
+    // Ensure offset + size are within the mapping bounds
+    if end_offset <= mmap_size {
+        Ok(())
+    } else {
+        Err(Error::InvalidAddress)
+    }
+}
+
+/// A range of memory that can be msynced, for abstracting over different types of memory mappings.
+///
+/// Safe when implementers guarantee `ptr`..`ptr+size` is an mmaped region owned by this object that
+/// can't be unmapped during the `MappedRegion`'s lifetime.
+pub unsafe trait MappedRegion: Send + Sync {
+    /// Returns a pointer to the beginning of the memory region. Should only be
+    /// used for passing this region to ioctls for setting guest memory.
+    fn as_ptr(&self) -> *mut u8;
+
+    /// Returns the size of the memory region in bytes.
+    fn size(&self) -> usize;
+}
+
+impl dyn MappedRegion {
+    /// Calls msync with MS_SYNC on a mapping of `size` bytes starting at `offset` from the start of
+    /// the region.  `offset`..`offset+size` must be contained within the `MappedRegion`.
+    pub fn msync(&self, offset: usize, size: usize) -> Result<()> {
+        validate_includes_range(self.size(), offset, size)?;
+
+        // Safe because the MemoryMapping/MemoryMappingArena interface ensures our pointer and size
+        // are correct, and we've validated that `offset`..`offset+size` is in the range owned by
+        // this `MappedRegion`.
+        let ret = unsafe {
+            libc::msync(
+                (self.as_ptr() as usize + offset) as *mut libc::c_void,
+                size,
+                libc::MS_SYNC,
+            )
+        };
+        if ret != -1 {
+            Ok(())
+        } else {
+            Err(Error::SystemCallFailed(errno::Error::last()))
+        }
+    }
+}
+
 /// Wraps an anonymous shared memory mapping in the current process.
 #[derive(Debug)]
 pub struct MemoryMapping {
@@ -314,17 +366,6 @@ impl MemoryMapping {
         })
     }
 
-    /// Returns a pointer to the beginning of the memory region. Should only be
-    /// used for passing this region to ioctls for setting guest memory.
-    pub fn as_ptr(&self) -> *mut u8 {
-        self.addr
-    }
-
-    /// Returns the size of the memory region in bytes.
-    pub fn size(&self) -> usize {
-        self.size
-    }
-
     /// Calls msync with MS_SYNC on the mapping.
     pub fn msync(&self) -> Result<()> {
         // This is safe since we use the exact address and length of a known
@@ -607,6 +648,18 @@ impl MemoryMapping {
     }
 }
 
+// Safe because the pointer and size point to a memory range owned by this MemoryMapping that won't
+// be unmapped until it's Dropped.
+unsafe impl MappedRegion for MemoryMapping {
+    fn as_ptr(&self) -> *mut u8 {
+        self.addr
+    }
+
+    fn size(&self) -> usize {
+        self.size
+    }
+}
+
 impl VolatileMemory for MemoryMapping {
     fn get_slice(&self, offset: usize, count: usize) -> VolatileMemoryResult<VolatileSlice> {
         let mem_end = calc_offset(offset, count)?;
@@ -732,7 +785,11 @@ impl MemoryMappingArena {
         prot: Protection,
         fd: Option<(&dyn AsRawFd, u64)>,
     ) -> Result<()> {
-        self.validate_range(offset, size)?;
+        // Ensure offset is page-aligned
+        if offset % pagesize() != 0 {
+            return Err(Error::NotPageAligned);
+        }
+        validate_includes_range(self.size(), offset, size)?;
 
         // This is safe since the range has been validated.
         let mmap = unsafe {
@@ -762,50 +819,18 @@ impl MemoryMappingArena {
     pub fn remove(&mut self, offset: usize, size: usize) -> Result<()> {
         self.try_add(offset, size, Protection::read(), None)
     }
+}
 
-    /// Calls msync with MS_SYNC on a mapping of `size` bytes starting at `offset` from the start of
-    /// the arena. `offset` must be page aligned.
-    pub fn msync(&self, offset: usize, size: usize) -> Result<()> {
-        self.validate_range(offset, size)?;
-
-        // Safe because we've validated that this memory range is owned by this `MemoryMappingArena`.
-        let ret =
-            unsafe { libc::msync(self.addr as *mut libc::c_void, self.size(), libc::MS_SYNC) };
-        if ret == -1 {
-            return Err(Error::SystemCallFailed(errno::Error::last()));
-        }
-        Ok(())
-    }
-
-    /// Returns a pointer to the beginning of the memory region.  Should only be
-    /// used for passing this region to ioctls for setting guest memory.
-    pub fn as_ptr(&self) -> *mut u8 {
+// Safe because the pointer and size point to a memory range owned by this MemoryMappingArena that
+// won't be unmapped until it's Dropped.
+unsafe impl MappedRegion for MemoryMappingArena {
+    fn as_ptr(&self) -> *mut u8 {
         self.addr
     }
 
-    /// Returns the size of the memory region in bytes.
-    pub fn size(&self) -> usize {
+    fn size(&self) -> usize {
         self.size
     }
-
-    /// Validates `offset` and `size`.
-    /// Checks that offset..offset+size doesn't overlap with existing mappings.
-    /// Also ensures correct alignment, and checks for any overflow.
-    /// Note: offset..offset+size is considered valid if it _perfectly_ overlaps
-    /// with single other region.
-    fn validate_range(&self, offset: usize, size: usize) -> Result<()> {
-        // Ensure offset is page-aligned
-        if offset % pagesize() != 0 {
-            return Err(Error::NotPageAligned);
-        }
-        // Ensure offset + size doesn't overflow
-        let end_offset = offset.checked_add(size).ok_or(Error::InvalidAddress)?;
-        // Ensure offset + size are within the arena bounds
-        if end_offset > self.size {
-            return Err(Error::InvalidAddress);
-        }
-        Ok(())
-    }
 }
 
 impl From<MemoryMapping> for MemoryMappingArena {
@@ -1021,4 +1046,19 @@ mod tests {
         m.remove(0, ps - 1)
             .expect("failed to remove unaligned mapping");
     }
+
+    #[test]
+    fn arena_msync() {
+        let size = 0x40000;
+        let m = MemoryMappingArena::new(size).unwrap();
+        let ps = pagesize();
+        MappedRegion::msync(&m, 0, ps).unwrap();
+        MappedRegion::msync(&m, 0, size).unwrap();
+        MappedRegion::msync(&m, ps, size - ps).unwrap();
+        let res = MappedRegion::msync(&m, ps, size).unwrap_err();
+        match res {
+            Error::InvalidAddress => {}
+            e => panic!("unexpected error: {}", e),
+        }
+    }
 }
diff --git a/vm_control/src/lib.rs b/vm_control/src/lib.rs
index 81a11d3..1d06e4b 100644
--- a/vm_control/src/lib.rs
+++ b/vm_control/src/lib.rs
@@ -21,7 +21,9 @@ use libc::{EINVAL, EIO, ENODEV};
 use kvm::{IrqRoute, IrqSource, Vm};
 use msg_socket::{MsgError, MsgOnSocket, MsgReceiver, MsgResult, MsgSender, MsgSocket};
 use resources::{Alloc, GpuMemoryDesc, MmioType, SystemAllocator};
-use sys_util::{error, Error as SysError, EventFd, GuestAddress, MemoryMapping, MmapError, Result};
+use sys_util::{
+    error, Error as SysError, EventFd, GuestAddress, MappedRegion, MemoryMapping, MmapError, Result,
+};
 
 /// A file descriptor either borrowed or owned by this.
 #[derive(Debug)]
@@ -481,7 +483,7 @@ impl VmMsyncRequest {
         match *self {
             MsyncArena { slot, offset, size } => {
                 if let Some(arena) = vm.get_mmap_arena(slot) {
-                    match arena.msync(offset, size) {
+                    match MappedRegion::msync(arena, offset, size) {
                         Ok(()) => VmMsyncResponse::Ok,
                         Err(e) => match e {
                             MmapError::SystemCallFailed(errno) => VmMsyncResponse::Err(errno),