From 6e8f33aa0a902aca5e56773d10c1cff7bf989acd Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Mon, 22 Mar 2021 10:26:12 -0800 Subject: [PATCH] rutabaga_gfx: convert to SafeDescriptor To be truly OS-agnostic, we need an OS-agnostic Rust wrapper over the OS-specific handle type. SafeDescriptor seems to be the best option, and I hope it on crates.io in the future. This converts virtio_gpu/rutabaga to use the SafeDescriptor handle when practical. minigbm still uses File in some places, since it needs to SeekFrom(..), but minigbm is a Linux only thing anyways. BUG=b:173630595 TEST=boot VM in 2D/3D mode Change-Id: I18d735844d479f52d82d7976bf9b4e383b2e2252 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2779492 Commit-Queue: Gurchetan Singh Commit-Queue: Zach Reizner Tested-by: Gurchetan Singh Reviewed-by: Michael Hoyle Reviewed-by: Zach Reizner --- base/src/shm.rs | 15 +++++++++++---- devices/src/virtio/gpu/virtio_gpu.rs | 4 ++-- rutabaga_gfx/src/rutabaga_gralloc/minigbm.rs | 4 ++-- .../src/rutabaga_gralloc/vulkano_gralloc.rs | 4 ++-- rutabaga_gfx/src/rutabaga_utils.rs | 5 ++--- rutabaga_gfx/src/virgl_renderer.rs | 7 +++---- sys_util/src/descriptor.rs | 14 ++++++++++++++ sys_util/src/shm.rs | 8 +++++++- 8 files changed, 43 insertions(+), 18 deletions(-) diff --git a/base/src/shm.rs b/base/src/shm.rs index 36c6f7e1a9..fa7355aca5 100644 --- a/base/src/shm.rs +++ b/base/src/shm.rs @@ -8,7 +8,7 @@ use crate::{ }; use std::ffi::CStr; use std::fs::File; -use std::os::unix::io::AsRawFd; +use std::os::unix::io::{AsRawFd, IntoRawFd}; use sys_util::SharedMemory as SysUtilSharedMemory; /// See [SharedMemory](sys_util::SharedMemory) for struct- and method-level @@ -77,8 +77,15 @@ impl AsRawDescriptor for SharedMemory { } } -impl Into for SharedMemory { - fn into(self) -> File { - self.0.into() +impl IntoRawDescriptor for SharedMemory { + fn into_raw_descriptor(self) -> RawDescriptor { + self.0.into_raw_fd() + } +} + +impl Into for SharedMemory { + fn into(self) -> SafeDescriptor { + // Safe because we own the SharedMemory at this point. + unsafe { SafeDescriptor::from_raw_descriptor(self.into_raw_descriptor()) } } } diff --git a/devices/src/virtio/gpu/virtio_gpu.rs b/devices/src/virtio/gpu/virtio_gpu.rs index 0e23c6a676..58d3dcfc1e 100644 --- a/devices/src/virtio/gpu/virtio_gpu.rs +++ b/devices/src/virtio/gpu/virtio_gpu.rs @@ -427,7 +427,7 @@ impl VirtioGpu { /// If supported, export the resource with the given `resource_id` to a file. pub fn export_resource(&mut self, resource_id: u32) -> ResourceResponse { let file = match self.rutabaga.export_blob(resource_id) { - Ok(handle) => handle.os_handle, + Ok(handle) => handle.os_handle.into(), Err(_) => return ResourceResponse::Invalid, }; @@ -463,7 +463,7 @@ impl VirtioGpu { pub fn export_fence(&self, fence_id: u32) -> ResourceResponse { match self.rutabaga.export_fence(fence_id) { Ok(handle) => ResourceResponse::Resource(ResourceInfo::Fence { - file: handle.os_handle, + file: handle.os_handle.into(), }), Err(_) => ResourceResponse::Invalid, } diff --git a/rutabaga_gfx/src/rutabaga_gralloc/minigbm.rs b/rutabaga_gfx/src/rutabaga_gralloc/minigbm.rs index bdf62e7538..c20438de36 100644 --- a/rutabaga_gfx/src/rutabaga_gralloc/minigbm.rs +++ b/rutabaga_gfx/src/rutabaga_gralloc/minigbm.rs @@ -143,7 +143,7 @@ impl Gralloc for MinigbmDevice { return Err(RutabagaError::SpecViolation); } - let dmabuf = gbm_buffer.export()?; + let dmabuf = gbm_buffer.export()?.into(); return Ok(RutabagaHandle { os_handle: dmabuf, handle_type: RUTABAGA_MEM_HANDLE_TYPE_DMABUF, @@ -165,7 +165,7 @@ impl Gralloc for MinigbmDevice { } let gbm_buffer = MinigbmBuffer(bo, self.clone()); - let dmabuf = gbm_buffer.export()?; + let dmabuf = gbm_buffer.export()?.into(); Ok(RutabagaHandle { os_handle: dmabuf, handle_type: RUTABAGA_MEM_HANDLE_TYPE_DMABUF, diff --git a/rutabaga_gfx/src/rutabaga_gralloc/vulkano_gralloc.rs b/rutabaga_gfx/src/rutabaga_gralloc/vulkano_gralloc.rs index b74908a081..c122224cd9 100644 --- a/rutabaga_gfx/src/rutabaga_gralloc/vulkano_gralloc.rs +++ b/rutabaga_gfx/src/rutabaga_gralloc/vulkano_gralloc.rs @@ -269,10 +269,10 @@ impl Gralloc for VulkanoGralloc { .export_info(handle_type) .build()?; - let file = device_memory.export_fd(handle_type)?; + let descriptor = device_memory.export_fd(handle_type)?.into(); Ok(RutabagaHandle { - os_handle: file, + os_handle: descriptor, handle_type: rutabaga_type, }) } diff --git a/rutabaga_gfx/src/rutabaga_utils.rs b/rutabaga_gfx/src/rutabaga_utils.rs index 943f36ccd6..b81989d763 100644 --- a/rutabaga_gfx/src/rutabaga_utils.rs +++ b/rutabaga_gfx/src/rutabaga_utils.rs @@ -5,13 +5,12 @@ //! rutabaga_utils: Utility enums, structs, and implementations needed by the rest of the crate. use std::fmt::{self, Display}; -use std::fs::File; use std::io::Error as IoError; use std::os::raw::c_void; use std::path::PathBuf; use std::str::Utf8Error; -use base::{Error as SysError, ExternalMappingError}; +use base::{Error as SysError, ExternalMappingError, SafeDescriptor}; use data_model::VolatileMemoryError; #[cfg(feature = "vulkano")] @@ -463,7 +462,7 @@ pub const RUTABAGE_FENCE_HANDLE_TYPE_OPAQUE_WIN32: u32 = 0x0006; /// Handle to OS-specific memory or synchronization objects. pub struct RutabagaHandle { - pub os_handle: File, + pub os_handle: SafeDescriptor, pub handle_type: u32, } diff --git a/rutabaga_gfx/src/virgl_renderer.rs b/rutabaga_gfx/src/virgl_renderer.rs index 55c5cda438..fcd21569c6 100644 --- a/rutabaga_gfx/src/virgl_renderer.rs +++ b/rutabaga_gfx/src/virgl_renderer.rs @@ -9,7 +9,6 @@ use std::cell::RefCell; use std::ffi::CString; -use std::fs::File; use std::mem::{size_of, transmute}; use std::os::raw::{c_char, c_void}; use std::ptr::null_mut; @@ -19,7 +18,7 @@ use std::sync::Arc; use base::{ warn, Error as SysError, ExternalMapping, ExternalMappingError, ExternalMappingResult, - FromRawDescriptor, + FromRawDescriptor, SafeDescriptor, }; use crate::generated::virgl_renderer_bindings::*; @@ -269,7 +268,7 @@ impl VirglRenderer { return Err(RutabagaError::Unsupported); } - let dmabuf = unsafe { File::from_raw_descriptor(fd) }; + let dmabuf = unsafe { SafeDescriptor::from_raw_descriptor(fd) }; Ok(Arc::new(RutabagaHandle { os_handle: dmabuf, handle_type: RUTABAGA_MEM_HANDLE_TYPE_DMABUF, @@ -539,7 +538,7 @@ impl RutabagaComponent for VirglRenderer { // Safe because the FD was just returned by a successful virglrenderer call so it must // be valid and owned by us. - let fence = unsafe { File::from_raw_descriptor(fd) }; + let fence = unsafe { SafeDescriptor::from_raw_descriptor(fd) }; Ok(RutabagaHandle { os_handle: fence, handle_type: RUTABAGA_FENCE_HANDLE_TYPE_SYNC_FD, diff --git a/sys_util/src/descriptor.rs b/sys_util/src/descriptor.rs index 5ee075a420..4c86ca2059 100644 --- a/sys_util/src/descriptor.rs +++ b/sys_util/src/descriptor.rs @@ -113,6 +113,20 @@ impl SafeDescriptor { } } +impl Into for SafeDescriptor { + fn into(self) -> File { + // Safe because we own the SafeDescriptor at this point. + unsafe { File::from_raw_fd(self.into_raw_descriptor()) } + } +} + +impl Into for File { + fn into(self) -> SafeDescriptor { + // Safe because we own the File at this point. + unsafe { SafeDescriptor::from_raw_descriptor(self.into_raw_descriptor()) } + } +} + /// For use cases where a simple wrapper around a RawDescriptor is needed. /// This is a simply a wrapper and does not manage the lifetime of the descriptor. /// Most usages should prefer SafeDescriptor or using a RawDescriptor directly diff --git a/sys_util/src/shm.rs b/sys_util/src/shm.rs index d9a583990c..6bfe206d98 100644 --- a/sys_util/src/shm.rs +++ b/sys_util/src/shm.rs @@ -5,7 +5,7 @@ use std::ffi::{CStr, CString}; use std::fs::{read_link, File}; use std::io::{self, Read, Seek, SeekFrom, Write}; -use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; +use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; use libc::{ self, c_char, c_int, c_long, c_uint, close, fcntl, ftruncate64, off64_t, syscall, EINVAL, @@ -267,6 +267,12 @@ impl AsRawFd for &SharedMemory { } } +impl IntoRawFd for SharedMemory { + fn into_raw_fd(self) -> RawFd { + self.fd.into_raw_fd() + } +} + impl Into for SharedMemory { fn into(self) -> File { self.fd