From d6ac1ab3a7453ba6d0e91b47d6242cab0d905fd2 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Fri, 31 Jan 2020 13:55:35 -0800 Subject: [PATCH] kvm: allow registering memory from current address space This is good for enabling non-exportable Vulkan host coherent memory, along with anything else where an exportable OS object isn't supported. This CL introduces: 1. ExternalMapping, which wraps an external library mapping using function callbacks, for purposes of sharing device memory to the guest in the case where the device memory is not compatible with the mmap interface. This is common in Vulkan when VkDeviceMemory is host visible but not external, or external but based on an opaque fd. The lifetime of the library mapping is tied to the lifetime of the ExternalMapping. 2. Usually, we would send such memory requests over a socket to the main thread. However, since these new objects require more metadata than other requests that are sent over the wire (because there's information about inheritance and refcounts), we also plumb the "map_request" field, which wraps a single ExternalMapping. Note that this ExternalMapping will not work in the sandbox case. In the sandbox case, we will then have to figure out how to serialize/deserialize ExternalMapping requests over a socket. BUG=b/146066070, b/153580313 TEST=compile and test Change-Id: I3b099b308aec45a313a8278ed6274f9dec66c30b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2034029 Tested-by: Gurchetan Singh Commit-Queue: Gurchetan Singh Reviewed-by: Zach Reizner --- Cargo.lock | 1 + src/linux.rs | 23 +++-- sys_util/src/external_mapping.rs | 140 +++++++++++++++++++++++++++++++ sys_util/src/lib.rs | 4 + vm_control/Cargo.toml | 1 + vm_control/src/lib.rs | 45 +++++++++- 6 files changed, 206 insertions(+), 8 deletions(-) create mode 100644 sys_util/src/external_mapping.rs diff --git a/Cargo.lock b/Cargo.lock index 0d449bd9d7..dd035a5037 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -790,6 +790,7 @@ dependencies = [ "libc 0.2.65 (registry+https://github.com/rust-lang/crates.io-index)", "msg_socket 0.1.0", "resources 0.1.0", + "sync 0.1.0", "sys_util 0.1.0", ] diff --git a/src/linux.rs b/src/linux.rs index 86b10019ea..5d133ae42c 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -49,9 +49,9 @@ use sys_util::net::{UnixSeqpacket, UnixSeqpacketListener, UnlinkUnixSeqpacketLis use sys_util::{ self, block_signal, clear_signal, drop_capabilities, error, flock, get_blocked_signals, get_group_id, get_user_id, getegid, geteuid, info, register_rt_signal_handler, - set_cpu_affinity, validate_raw_fd, warn, EventFd, FlockOperation, GuestAddress, GuestMemory, - Killable, MemoryMappingArena, PollContext, PollToken, Protection, ScopedEvent, SignalFd, - Terminal, TimerFd, WatchingEvents, SIGRTMIN, + set_cpu_affinity, validate_raw_fd, warn, EventFd, ExternalMapping, FlockOperation, + GuestAddress, GuestMemory, Killable, MemoryMappingArena, PollContext, PollToken, Protection, + ScopedEvent, SignalFd, Terminal, TimerFd, WatchingEvents, SIGRTMIN, }; use vm_control::{ BalloonControlCommand, BalloonControlRequestSocket, BalloonControlResponseSocket, @@ -656,6 +656,7 @@ fn create_gpu_device( wayland_socket_path: Option<&PathBuf>, x_display: Option, event_devices: Vec, + _map_request: Arc>>, ) -> DeviceResult { let jailed_wayland_path = Path::new("/wayland-0"); @@ -1087,6 +1088,7 @@ fn create_virtio_devices( balloon_device_socket: BalloonControlResponseSocket, disk_device_sockets: &mut Vec, pmem_device_sockets: &mut Vec, + map_request: Arc>>, ) -> DeviceResult> { let mut devs = Vec::new(); @@ -1249,6 +1251,7 @@ fn create_virtio_devices( cfg.wayland_socket_paths.get(""), cfg.x_display.clone(), event_devices, + map_request, )?); } } @@ -1290,6 +1293,7 @@ fn create_devices( disk_device_sockets: &mut Vec, pmem_device_sockets: &mut Vec, usb_provider: HostBackendDeviceProvider, + map_request: Arc>>, ) -> DeviceResult, Option)>> { let stubs = create_virtio_devices( &cfg, @@ -1302,6 +1306,7 @@ fn create_devices( balloon_device_socket, disk_device_sockets, pmem_device_sockets, + map_request, )?; let mut pci_devices = Vec::new(); @@ -1801,6 +1806,8 @@ pub fn run_config(cfg: Config) -> Result<()> { msg_socket::pair::().map_err(Error::CreateSocket)?; control_sockets.push(TaggedControlSocket::VmIrq(ioapic_host_socket)); + let map_request: Arc>> = Arc::new(Mutex::new(None)); + let sandbox = cfg.sandbox; let linux = Arch::build_vm( components, @@ -1822,6 +1829,7 @@ pub fn run_config(cfg: Config) -> Result<()> { &mut disk_device_sockets, &mut pmem_device_sockets, usb_provider, + Arc::clone(&map_request), ) }, ) @@ -1836,6 +1844,7 @@ pub fn run_config(cfg: Config) -> Result<()> { usb_control_socket, sigchld_fd, sandbox, + Arc::clone(&map_request), ) } @@ -1848,6 +1857,7 @@ fn run_control( usb_control_socket: UsbControlSocket, sigchld_fd: SignalFd, sandbox: bool, + map_request: Arc>>, ) -> Result<()> { const LOWMEM_AVAILABLE: &str = "/sys/kernel/mm/chromeos-low_mem/available"; @@ -2189,8 +2199,11 @@ fn run_control( }, TaggedControlSocket::VmMemory(socket) => match socket.recv() { Ok(request) => { - let response = - request.execute(&mut linux.vm, &mut linux.resources); + let response = request.execute( + &mut linux.vm, + &mut linux.resources, + Arc::clone(&map_request), + ); if let Err(e) = socket.send(&response) { error!("failed to send VmMemoryControlResponse: {}", e); } diff --git a/sys_util/src/external_mapping.rs b/sys_util/src/external_mapping.rs new file mode 100644 index 0000000000..67272bd9cf --- /dev/null +++ b/sys_util/src/external_mapping.rs @@ -0,0 +1,140 @@ +// 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 crate::MappedRegion; +use std::fmt::{self, Display}; + +#[derive(Debug, Eq, PartialEq)] +pub enum Error { + // A null address is typically bad. mmap allows it, but not external libraries + NullAddress, + // For external mappings that have weird sizes + InvalidSize, + // External library failed to map + LibraryError(i32), + // If external mapping is unsupported. + Unsupported, +} + +impl Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use self::Error::*; + + match self { + NullAddress => write!(f, "null address returned"), + InvalidSize => write!(f, "invalid size returned"), + LibraryError(ret) => write!(f, "library failed to map with {}", ret), + Unsupported => write!(f, "external mapping unsupported"), + } + } +} + +pub type Result = std::result::Result; + +// Maps a external library resource given an id, returning address and size upon success +pub type Map = fn(u32) -> Result<(u64, usize)>; +// Unmaps the resource given a resource id. +pub type Unmap = fn(u32) -> (); + +/// ExternalMapping wraps an external library mapping. This is useful in cases where where the +/// device memory is not compatible with the mmap interface, such as Vulkan VkDeviceMemory in the +/// non-exportable case or when exported as an opaque fd. +#[derive(Debug, PartialEq)] +pub struct ExternalMapping { + resource_id: u32, + ptr: u64, + size: usize, + unmap: Unmap, +} + +unsafe impl Send for ExternalMapping {} +unsafe impl Sync for ExternalMapping {} +impl ExternalMapping { + /// Creates an ExternalMapping given a library-specific resource id and map/unmap functions. + /// The map function must return a valid host memory region. In addition, callers of the + /// function must guarantee that the map and unmap functions are thread-safe, never return a + /// region overlapping already Rust referenced-data, and the backing store of the resource + /// doesn't disappear before the unmap function is called. + pub unsafe fn new(resource_id: u32, map: Map, unmap: Unmap) -> Result { + let (ptr, size) = map(resource_id)?; + + if ptr as *mut u8 == std::ptr::null_mut() { + return Err(Error::NullAddress); + } + if size == 0 { + return Err(Error::InvalidSize); + } + + Ok(ExternalMapping { + resource_id, + ptr, + size, + unmap, + }) + } +} + +unsafe impl MappedRegion for ExternalMapping { + /// used for passing this region to ioctls for setting guest memory. + fn as_ptr(&self) -> *mut u8 { + self.ptr as *mut u8 + } + + /// Returns the size of the memory region in bytes. + fn size(&self) -> usize { + self.size + } +} + +impl Drop for ExternalMapping { + fn drop(&mut self) { + // This is safe because we own this memory range, and nobody else is holding a reference to + // it. + (self.unmap)(self.resource_id) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn check_valid_external_map() { + let map1: Map = |_resource_id| Ok((0xAAAABBBB, 500)); + let map2: Map = |_resource_id| Ok((0xBBBBAAAA, 1000)); + let unmap: Unmap = |_resource_id| {}; + let external_map1 = unsafe { ExternalMapping::new(0, map1, unmap).unwrap() }; + let external_map2 = unsafe { ExternalMapping::new(0, map2, unmap).unwrap() }; + + assert_eq!(external_map1.as_ptr(), 0xAAAABBBB as *mut u8); + assert_eq!(external_map1.size(), 500); + assert_eq!(external_map2.as_ptr(), 0xBBBBAAAA as *mut u8); + assert_eq!(external_map2.size(), 1000); + } + + #[test] + fn check_invalid_external_map() { + let map1: Map = |_resource_id| Ok((0xAAAABBBB, 0)); + let map2: Map = |_resource_id| Ok((0, 500)); + let unmap: Unmap = |_resource_id| {}; + + assert_eq!( + unsafe { ExternalMapping::new(0, map1, unmap) }, + Err(Error::InvalidSize) + ); + + assert_eq!( + unsafe { ExternalMapping::new(0, map2, unmap) }, + Err(Error::NullAddress) + ); + } + + #[test] + #[should_panic] + fn check_external_map_drop() { + let map = |_resource_id| Ok((0xAAAABBBB, 500)); + let unmap = |_resource_id| panic!(); + let _external_map = unsafe { ExternalMapping::new(0, map, unmap) }; + } +} diff --git a/sys_util/src/lib.rs b/sys_util/src/lib.rs index 82e81f4c55..5c9ac38f65 100644 --- a/sys_util/src/lib.rs +++ b/sys_util/src/lib.rs @@ -16,6 +16,7 @@ mod clock; mod descriptor; mod errno; mod eventfd; +mod external_mapping; mod file_flags; pub mod file_traits; mod fork; @@ -44,6 +45,7 @@ pub use crate::clock::{Clock, FakeClock}; pub use crate::descriptor::*; pub use crate::errno::{errno_result, Error, Result}; pub use crate::eventfd::*; +pub use crate::external_mapping::*; pub use crate::file_flags::*; pub use crate::fork::*; pub use crate::guest_address::*; @@ -64,6 +66,8 @@ pub use crate::terminal::*; pub use crate::timerfd::*; pub use poll_token_derive::*; +pub use crate::external_mapping::Error as ExternalMappingError; +pub use crate::external_mapping::Result as ExternalMappingResult; pub use crate::file_traits::{ AsRawFds, FileAllocate, FileGetLen, FileReadWriteAtVolatile, FileReadWriteVolatile, FileSetLen, FileSync, diff --git a/vm_control/Cargo.toml b/vm_control/Cargo.toml index 564aea13ff..c5dbfee0a5 100644 --- a/vm_control/Cargo.toml +++ b/vm_control/Cargo.toml @@ -10,4 +10,5 @@ kvm = { path = "../kvm" } libc = "*" msg_socket = { path = "../msg_socket" } resources = { path = "../resources" } +sync = { path = "../sync" } sys_util = { path = "../sys_util" } diff --git a/vm_control/src/lib.rs b/vm_control/src/lib.rs index 5336f3bbc6..72e7b97576 100644 --- a/vm_control/src/lib.rs +++ b/vm_control/src/lib.rs @@ -15,13 +15,18 @@ use std::fs::File; use std::io::{Seek, SeekFrom}; use std::mem::ManuallyDrop; use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; +use std::sync::Arc; 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 sync::Mutex; +use sys_util::{ + error, Error as SysError, EventFd, ExternalMapping, GuestAddress, MappedRegion, MemoryMapping, + MmapError, Result, +}; /// A file descriptor either borrowed or owned by this. #[derive(Debug)] @@ -262,7 +267,9 @@ pub enum VmMemoryRequest { /// Similiar to `VmMemoryRequest::RegisterMemory`, but doesn't allocate new address space. /// Useful for cases where the address space is already allocated (PCI regions). RegisterFdAtPciBarOffset(Alloc, MaybeOwnedFd, usize, u64), - /// Unregister the given memory slot that was previously registereed with `RegisterMemory`. + /// Similar to RegisterFdAtPciBarOffset, but is for buffers in the current address space. + RegisterHostPointerAtPciBarOffset(Alloc, u64), + /// Unregister the given memory slot that was previously registered with `RegisterMemory*`. UnregisterMemory(u32), /// Allocate GPU buffer of a given size/format and register the memory into guest address space. /// The response variant is `VmResponse::AllocateAndRegisterGpuMemory` @@ -290,7 +297,12 @@ impl VmMemoryRequest { /// This does not return a result, instead encapsulating the success or failure in a /// `VmMemoryResponse` with the intended purpose of sending the response back over the socket /// that received this `VmMemoryResponse`. - pub fn execute(&self, vm: &mut Vm, sys_allocator: &mut SystemAllocator) -> VmMemoryResponse { + pub fn execute( + &self, + vm: &mut Vm, + sys_allocator: &mut SystemAllocator, + map_request: Arc>>, + ) -> VmMemoryResponse { use self::VmMemoryRequest::*; match *self { RegisterMemory(ref fd, size) => { @@ -309,6 +321,18 @@ impl VmMemoryRequest { Ok(_) => VmMemoryResponse::Ok, Err(e) => VmMemoryResponse::Err(e), }, + RegisterHostPointerAtPciBarOffset(alloc, offset) => { + let mem = map_request + .lock() + .take() + .ok_or(VmMemoryResponse::Err(SysError::new(EINVAL))) + .unwrap(); + + match register_memory_hva(vm, sys_allocator, Box::new(mem), (alloc, offset)) { + Ok((pfn, slot)) => VmMemoryResponse::RegisterMemory { pfn, slot }, + Err(e) => VmMemoryResponse::Err(e), + } + } AllocateAndRegisterGpuMemory { width, height, @@ -562,6 +586,21 @@ fn register_memory( Ok((addr >> 12, slot)) } +fn register_memory_hva( + vm: &mut Vm, + allocator: &mut SystemAllocator, + mem: Box, + pci_allocation: (Alloc, u64), +) -> Result<(u64, u32)> { + let addr = allocator + .mmio_allocator(MmioType::High) + .address_from_pci_offset(pci_allocation.0, pci_allocation.1, mem.size() as u64) + .map_err(|_e| SysError::new(EINVAL))?; + + let slot = vm.add_memory_region(GuestAddress(addr), mem, false, false)?; + Ok((addr >> 12, slot)) +} + impl VmRequest { /// Executes this request on the given Vm and other mutable state. ///