From 265967bcda2922e688557a69e04c1eec0ba8e990 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Mon, 9 Mar 2020 18:09:51 +0900 Subject: descriptor_utils: Add `iter` method Add an `iter` method to the `Reader` struct so that callers can iterate over the objects in place rather than having to store them in a separate collection. BUG=none TEST=unit tests Change-Id: I29671910a4244a8d7786ca2eb241416ae72b8c9f Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2093966 Tested-by: Chirantan Ekbote Tested-by: kokoro Reviewed-by: Daniel Verkamp Commit-Queue: Chirantan Ekbote --- devices/src/virtio/descriptor_utils.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/descriptor_utils.rs b/devices/src/virtio/descriptor_utils.rs index 2e5dfd3..492cc13 100644 --- a/devices/src/virtio/descriptor_utils.rs +++ b/devices/src/virtio/descriptor_utils.rs @@ -216,8 +216,8 @@ pub struct Reader<'a> { buffer: DescriptorChainConsumer<'a>, } -// An iterator over `DataInit` objects on readable descriptors in the descriptor chain. -struct ReaderIterator<'a, T: DataInit> { +/// An iterator over `DataInit` objects on readable descriptors in the descriptor chain. +pub struct ReaderIterator<'a, T: DataInit> { reader: &'a mut Reader<'a>, phantom: PhantomData, } @@ -283,10 +283,17 @@ impl<'a> Reader<'a> { /// them as a collection. Returns an error if the size of the remaining data is indivisible by /// the size of an object of type `T`. pub fn collect>, T: DataInit>(&'a mut self) -> C { - C::from_iter(ReaderIterator { + C::from_iter(self.iter()) + } + + /// Creates an iterator for sequentially reading `DataInit` objects from the `Reader`. Unlike + /// `collect`, this doesn't consume all the remaining data in the `Reader` and doesn't require + /// the objects to be stored in a separate collection. + pub fn iter(&'a mut self) -> ReaderIterator<'a, T> { + ReaderIterator { reader: self, phantom: PhantomData, - }) + } } /// Reads data from the descriptor chain buffer into a file descriptor. -- cgit 1.4.1 From cba39f2fefc7dd1f38ffdae8451e530f16c06038 Mon Sep 17 00:00:00 2001 From: "Jorge E. Moreira" Date: Sun, 8 Mar 2020 01:26:27 +0000 Subject: Allow all serial port types to read from stdin Bug=b/148677254 Change-Id: I1fa38bc95ca303c7a2c38dbe4b938a6042c910c6 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2093800 Reviewed-by: Zach Reizner Reviewed-by: Daniel Verkamp Tested-by: Zach Reizner Tested-by: kokoro Commit-Queue: Zach Reizner Auto-Submit: Jorge Moreira Broche --- devices/src/serial.rs | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) (limited to 'devices/src') diff --git a/devices/src/serial.rs b/devices/src/serial.rs index 98f39dc..6629d7e 100644 --- a/devices/src/serial.rs +++ b/devices/src/serial.rs @@ -149,37 +149,35 @@ impl SerialParameters { ) -> std::result::Result { let evt_fd = evt_fd.try_clone().map_err(Error::CloneEventFd)?; keep_fds.push(evt_fd.as_raw_fd()); + let input: Option> = if self.stdin { + keep_fds.push(stdin().as_raw_fd()); + // This wrapper is used in place of the libstd native version because we don't want + // buffering for stdin. + struct StdinWrapper; + impl io::Read for StdinWrapper { + fn read(&mut self, out: &mut [u8]) -> io::Result { + read_raw_stdin(out).map_err(|e| e.into()) + } + } + Some(Box::new(StdinWrapper)) + } else { + None + }; match self.type_ { SerialType::Stdout => { keep_fds.push(stdout().as_raw_fd()); - if self.stdin { - keep_fds.push(stdin().as_raw_fd()); - // This wrapper is used in place of the libstd native version because we don't - // want buffering for stdin. - struct StdinWrapper; - impl io::Read for StdinWrapper { - fn read(&mut self, out: &mut [u8]) -> io::Result { - read_raw_stdin(out).map_err(|e| e.into()) - } - } - Ok(Serial::new_in_out( - evt_fd, - Box::new(StdinWrapper), - Box::new(stdout()), - )) - } else { - Ok(Serial::new_out(evt_fd, Box::new(stdout()))) - } + Ok(Serial::new(evt_fd, input, Some(Box::new(stdout())))) } - SerialType::Sink => Ok(Serial::new_sink(evt_fd)), + SerialType::Sink => Ok(Serial::new(evt_fd, input, None)), SerialType::Syslog => { syslog::push_fds(keep_fds); - Ok(Serial::new_out( + Ok(Serial::new( evt_fd, - Box::new(syslog::Syslogger::new( + input, + Some(Box::new(syslog::Syslogger::new( syslog::Priority::Info, syslog::Facility::Daemon, - )), + ))), )) } SerialType::File => match &self.path { @@ -187,7 +185,7 @@ impl SerialParameters { Some(path) => { let file = File::create(path.as_path()).map_err(Error::FileError)?; keep_fds.push(file.as_raw_fd()); - Ok(Serial::new_out(evt_fd, Box::new(file))) + Ok(Serial::new(evt_fd, input, Some(Box::new(file)))) } }, SerialType::UnixSocket => Err(Error::Unimplemented(SerialType::UnixSocket)), -- cgit 1.4.1 From 752f9c28703e447ab5b127cbac94493a7539eb36 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Mon, 9 Mar 2020 18:46:55 +0900 Subject: devices: Ignore O_DIRECT in 9p and fs devices Specifying O_DIRECT in the 9p device doesn't actually work correctly and leads to an error. O_DIRECT handling in the fs device works correctly but also makes it look much worse in disk I/O benchmarks because the block device gets the benefit of the host cache while the fs device depends on the performance of the actual storage device. BUG=none TEST=`tast run vm.Fio.*` Change-Id: I738e4032081e331ef956c9d4c33616607e403d86 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2093967 Commit-Queue: Chirantan Ekbote Tested-by: Chirantan Ekbote Tested-by: kokoro Reviewed-by: Dylan Reid Reviewed-by: Daniel Verkamp --- devices/src/virtio/fs/passthrough.rs | 5 +++-- p9/src/server.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/fs/passthrough.rs b/devices/src/virtio/fs/passthrough.rs index bdc1c6f..d23009b 100644 --- a/devices/src/virtio/fs/passthrough.rs +++ b/devices/src/virtio/fs/passthrough.rs @@ -395,7 +395,7 @@ impl PassthroughFs { libc::openat( self.proc.as_raw_fd(), pathname.as_ptr(), - (flags | libc::O_CLOEXEC) & (!libc::O_NOFOLLOW), + (flags | libc::O_CLOEXEC) & !(libc::O_NOFOLLOW | libc::O_DIRECT), ) }; if fd < 0 { @@ -965,7 +965,8 @@ impl FileSystem for PassthroughFs { libc::openat( data.file.as_raw_fd(), name.as_ptr(), - flags as i32 | libc::O_CREAT | libc::O_CLOEXEC | libc::O_NOFOLLOW, + (flags as i32 | libc::O_CREAT | libc::O_CLOEXEC | libc::O_NOFOLLOW) + & !libc::O_DIRECT, mode & !(umask & 0o777), ) }; diff --git a/p9/src/server.rs b/p9/src/server.rs index b8a026f..e1ffd16 100644 --- a/p9/src/server.rs +++ b/p9/src/server.rs @@ -44,7 +44,7 @@ const MAPPED_FLAGS: [(u32, i32); 10] = [ (P9_NONBLOCK, libc::O_NONBLOCK), (P9_DSYNC, libc::O_DSYNC), (P9_FASYNC, 0), // Unsupported - (P9_DIRECT, libc::O_DIRECT), + (P9_DIRECT, 0), // Unsupported (P9_LARGEFILE, libc::O_LARGEFILE), (P9_DIRECTORY, libc::O_DIRECTORY), (P9_NOFOLLOW, libc::O_NOFOLLOW), -- cgit 1.4.1 From 8476d79a3c0976355695dde780786179c3417667 Mon Sep 17 00:00:00 2001 From: Dylan Reid Date: Mon, 16 Mar 2020 04:49:21 +0000 Subject: Fix warnings added in rust 1.42 rustc now warns about return statements that have an extra set of parenthesis. Remove such instances so that the code is warning free. TEST=cargo build completes without warnings Change-Id: I55148f8aceca8ba90f6bead2b6929e2c843351aa Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2104767 Reviewed-by: Stephen Barber Reviewed-by: Daniel Verkamp Reviewed-by: Zach Reizner Tested-by: kokoro Tested-by: Dylan Reid Commit-Queue: Dylan Reid --- devices/src/pit.rs | 2 +- x86_64/src/gdt.rs | 8 ++++---- x86_64/src/interrupts.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'devices/src') diff --git a/devices/src/pit.rs b/devices/src/pit.rs index 86ad4c5..68ed6e0 100644 --- a/devices/src/pit.rs +++ b/devices/src/pit.rs @@ -691,7 +691,7 @@ impl PitCounter { Some(t) => { let dur = self.clock.lock().now().duration_since(t); let dur_ns: u64 = dur.as_secs() * NANOS_PER_SEC + u64::from(dur.subsec_nanos()); - (dur_ns * FREQUENCY_HZ / NANOS_PER_SEC) + dur_ns * FREQUENCY_HZ / NANOS_PER_SEC } } } diff --git a/x86_64/src/gdt.rs b/x86_64/src/gdt.rs index 7eb1ff7..a37ddd4 100644 --- a/x86_64/src/gdt.rs +++ b/x86_64/src/gdt.rs @@ -8,17 +8,17 @@ use kvm_sys::kvm_segment; /// Constructor for a conventional segment GDT (or LDT) entry. Derived from the kernel's segment.h. pub fn gdt_entry(flags: u16, base: u32, limit: u32) -> u64 { - ((((base as u64) & 0xff000000u64) << (56 - 24)) + (((base as u64) & 0xff000000u64) << (56 - 24)) | (((flags as u64) & 0x0000f0ffu64) << 40) | (((limit as u64) & 0x000f0000u64) << (48 - 16)) | (((base as u64) & 0x00ffffffu64) << 16) - | ((limit as u64) & 0x0000ffffu64)) + | ((limit as u64) & 0x0000ffffu64) } fn get_base(entry: u64) -> u64 { - ((((entry) & 0xFF00000000000000) >> 32) + (((entry) & 0xFF00000000000000) >> 32) | (((entry) & 0x000000FF00000000) >> 16) - | (((entry) & 0x00000000FFFF0000) >> 16)) + | (((entry) & 0x00000000FFFF0000) >> 16) } fn get_limit(entry: u64) -> u32 { diff --git a/x86_64/src/interrupts.rs b/x86_64/src/interrupts.rs index a2b0f1c..5fba859 100644 --- a/x86_64/src/interrupts.rs +++ b/x86_64/src/interrupts.rs @@ -57,7 +57,7 @@ fn set_klapic_reg(klapic: &mut kvm_lapic_state, reg_offset: usize, value: u32) { } fn set_apic_delivery_mode(reg: u32, mode: u32) -> u32 { - (((reg) & !0x700) | ((mode) << 8)) + ((reg) & !0x700) | ((mode) << 8) } /// Configures LAPICs. LAPIC0 is set for external interrupts, LAPIC1 is set for NMI. -- cgit 1.4.1 From a07d84ad6876197368ed23b641c2400a44809e69 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Tue, 17 Mar 2020 17:51:19 +0900 Subject: devices: fs: Add support for fuse minor version 28 BUG=b:150264964 TEST=vm.Virtiofs Change-Id: I544329b63352956647d07aefdfce3118947d0821 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2105820 Reviewed-by: Daniel Verkamp Tested-by: kokoro Commit-Queue: Chirantan Ekbote --- devices/src/virtio/fs/filesystem.rs | 30 ++++++++++++++++++++++++++ devices/src/virtio/fs/fuse.rs | 43 +++++++++++++++++++++++++++++++++++-- devices/src/virtio/fs/server.rs | 37 ++++++++++++++++++++++++++++++- 3 files changed, 107 insertions(+), 3 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/fs/filesystem.rs b/devices/src/virtio/fs/filesystem.rs index 232ff99..eb9726c 100644 --- a/devices/src/virtio/fs/filesystem.rs +++ b/devices/src/virtio/fs/filesystem.rs @@ -1139,4 +1139,34 @@ pub trait FileSystem { fn lseek(&self) -> io::Result<()> { Err(io::Error::from_raw_os_error(libc::ENOSYS)) } + + /// Copy a range of data from one file to another + /// + /// Performs an optimized copy between two file descriptors without the additional cost of + /// transferring data through the kernel module to user space (glibc) and then back into + /// the file system again. + /// + /// In case this method is not implemented, glibc falls back to reading data from the source and + /// writing to the destination. + /// + /// If this method fails with an `ENOSYS` error, then the kernel will treat that as a permanent + /// failure. The kernel will return `EOPNOTSUPP` for all future calls to `copy_file_range` + /// without forwarding them to the file system. + /// + /// All values accepted by the `copy_file_range(2)` system call are valid values for `flags` and + /// must be handled by the file system. + fn copy_file_range( + &self, + ctx: Context, + inode_src: Self::Inode, + handle_src: Self::Handle, + offset_src: u64, + inode_dst: Self::Inode, + handle_dst: Self::Handle, + offset_dst: u64, + length: u64, + flags: u64, + ) -> io::Result { + Err(io::Error::from_raw_os_error(libc::ENOSYS)) + } } diff --git a/devices/src/virtio/fs/fuse.rs b/devices/src/virtio/fs/fuse.rs index 612202b..0533a5c 100644 --- a/devices/src/virtio/fs/fuse.rs +++ b/devices/src/virtio/fs/fuse.rs @@ -12,8 +12,11 @@ use libc; /// Version number of this interface. pub const KERNEL_VERSION: u32 = 7; +/// Oldest supported minor version of the fuse interface. +pub const OLDEST_SUPPORTED_KERNEL_MINOR_VERSION: u32 = 27; + /// Minor version number of this interface. -pub const KERNEL_MINOR_VERSION: u32 = 27; +pub const KERNEL_MINOR_VERSION: u32 = 28; /// The ID of the inode corresponding to the root directory of the file system. pub const ROOT_ID: u64 = 1; @@ -56,6 +59,12 @@ const FOPEN_KEEP_CACHE: u32 = 2; /// The file is not seekable. const FOPEN_NONSEEKABLE: u32 = 4; +/// Allow caching the directory entries. +const FOPEN_CACHE_DIR: u32 = 8; + +/// This file is stream-like (i.e., no file position). +const FOPEN_STREAM: u32 = 16; + bitflags! { /// Options controlling the behavior of files opened by the server in response /// to an open or create request. @@ -63,6 +72,8 @@ bitflags! { const DIRECT_IO = FOPEN_DIRECT_IO; const KEEP_CACHE = FOPEN_KEEP_CACHE; const NONSEEKABLE = FOPEN_NONSEEKABLE; + const CACHE_DIR = FOPEN_CACHE_DIR; + const STREAM = FOPEN_STREAM; } } @@ -131,6 +142,15 @@ const HANDLE_KILLPRIV: u32 = 524288; /// FileSystem supports posix acls. const POSIX_ACL: u32 = 1048576; +/// Reading the device after an abort returns `ECONNABORTED`. +const ABORT_ERROR: u32 = 2097152; + +/// The reply to the `init` message contains the max number of request pages. +const MAX_PAGES: u32 = 4194304; + +/// Cache `readlink` responses. +const CACHE_SYMLINKS: u32 = 8388608; + bitflags! { /// A bitfield passed in as a parameter to and returned from the `init` method of the /// `FileSystem` trait. @@ -297,6 +317,9 @@ bitflags! { /// /// This feature is disabled by default. const POSIX_ACL = POSIX_ACL; + + /// Indicates that the kernel may cache responses to `readlink` calls. + const CACHE_SYMLINKS = CACHE_SYMLINKS; } } @@ -511,6 +534,7 @@ pub enum Opcode { Readdirplus = 44, Rename2 = 45, Lseek = 46, + CopyFileRange = 47, } #[repr(u32)] @@ -830,7 +854,9 @@ pub struct InitOut { pub congestion_threshold: u16, pub max_write: u32, pub time_gran: u32, - pub unused: [u32; 9], + pub max_pages: u16, + pub padding: u16, + pub unused: [u32; 8], } unsafe impl DataInit for InitOut {} @@ -1049,3 +1075,16 @@ pub struct LseekOut { pub offset: u64, } unsafe impl DataInit for LseekOut {} + +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct CopyFileRangeIn { + pub fh_src: u64, + pub off_src: u64, + pub nodeid_dst: u64, + pub fh_dst: u64, + pub off_dst: u64, + pub len: u64, + pub flags: u64, +} +unsafe impl DataInit for CopyFileRangeIn {} diff --git a/devices/src/virtio/fs/server.rs b/devices/src/virtio/fs/server.rs index 32b5008..76f2830 100644 --- a/devices/src/virtio/fs/server.rs +++ b/devices/src/virtio/fs/server.rs @@ -118,6 +118,7 @@ impl Server { Some(Opcode::Readdirplus) => self.readdirplus(in_header, r, w), Some(Opcode::Rename2) => self.rename2(in_header, r, w), Some(Opcode::Lseek) => self.lseek(in_header, r, w), + Some(Opcode::CopyFileRange) => self.copy_file_range(in_header, r, w), None => reply_error( io::Error::from_raw_os_error(libc::ENOSYS), in_header.unique, @@ -809,7 +810,7 @@ impl Server { return reply_ok(Some(out), None, in_header.unique, w); } - if minor < KERNEL_MINOR_VERSION { + if minor < OLDEST_SUPPORTED_KERNEL_MINOR_VERSION { error!( "Unsupported fuse protocol minor version: {}.{}", major, minor @@ -1208,6 +1209,40 @@ impl Server { Ok(0) } } + + fn copy_file_range(&self, in_header: InHeader, mut r: Reader, w: Writer) -> Result { + let CopyFileRangeIn { + fh_src, + off_src, + nodeid_dst, + fh_dst, + off_dst, + len, + flags, + } = r.read_obj().map_err(Error::DecodeMessage)?; + + match self.fs.copy_file_range( + Context::from(in_header), + in_header.nodeid.into(), + fh_src.into(), + off_src, + nodeid_dst.into(), + fh_dst.into(), + off_dst, + len, + flags, + ) { + Ok(count) => { + let out = WriteOut { + size: count as u32, + ..Default::default() + }; + + reply_ok(Some(out), None, in_header.unique, w) + } + Err(e) => reply_error(e, in_header.unique, w), + } + } } fn retry_ioctl( -- cgit 1.4.1 From 93a987705782a882c9f1d05b8b472276c20ae50f Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Tue, 17 Mar 2020 17:52:02 +0900 Subject: devices: fs: Support FOPEN_CACHE_DIR Add support for FOPEN_CACHE_DIR so that the guest can cache directory entries for longer. BUG=b:150264964 TEST=vm.Virtiofs Change-Id: Iade67b54084ed72378afa70af9e9e0f7f0bc03e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2105821 Reviewed-by: Daniel Verkamp Tested-by: kokoro Commit-Queue: Chirantan Ekbote --- devices/src/virtio/fs/passthrough.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'devices/src') diff --git a/devices/src/virtio/fs/passthrough.rs b/devices/src/virtio/fs/passthrough.rs index d23009b..aab1403 100644 --- a/devices/src/virtio/fs/passthrough.rs +++ b/devices/src/virtio/fs/passthrough.rs @@ -592,7 +592,13 @@ impl PassthroughFs { OpenOptions::DIRECT_IO, flags & (libc::O_DIRECTORY as u32) == 0, ), - CachePolicy::Always => opts |= OpenOptions::KEEP_CACHE, + CachePolicy::Always => { + opts |= if flags & (libc::O_DIRECTORY as u32) == 0 { + OpenOptions::KEEP_CACHE + } else { + OpenOptions::CACHE_DIR + } + } _ => {} }; -- cgit 1.4.1 From 6dfa1e4ce5c260eabbda21c12d7bd9769f1cc995 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Tue, 17 Mar 2020 18:20:10 +0900 Subject: devices: fs: Implement copy_file_range BUG=none TEST=vm.Virtiofs Change-Id: I2ed7137a901e6e506e6b1562b77fdb042bdc58ab Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2105822 Tested-by: Chirantan Ekbote Tested-by: kokoro Reviewed-by: Daniel Verkamp Commit-Queue: Chirantan Ekbote --- devices/src/virtio/fs/passthrough.rs | 54 ++++++++++++++++++++++++++++++++++++ seccomp/aarch64/fs_device.policy | 1 + seccomp/arm/fs_device.policy | 1 + seccomp/x86_64/fs_device.policy | 1 + 4 files changed, 57 insertions(+) (limited to 'devices/src') diff --git a/devices/src/virtio/fs/passthrough.rs b/devices/src/virtio/fs/passthrough.rs index aab1403..419d466 100644 --- a/devices/src/virtio/fs/passthrough.rs +++ b/devices/src/virtio/fs/passthrough.rs @@ -1696,4 +1696,58 @@ impl FileSystem for PassthroughFs { Err(io::Error::from_raw_os_error(libc::ENOTTY)) } } + + fn copy_file_range( + &self, + ctx: Context, + inode_src: Inode, + handle_src: Handle, + offset_src: u64, + inode_dst: Inode, + handle_dst: Handle, + offset_dst: u64, + length: u64, + flags: u64, + ) -> io::Result { + // We need to change credentials during a write so that the kernel will remove setuid or + // setgid bits from the file if it was written to by someone other than the owner. + let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?; + let src_data = self + .handles + .read() + .unwrap() + .get(&handle_src) + .filter(|hd| hd.inode == inode_src) + .map(Arc::clone) + .ok_or_else(ebadf)?; + let dst_data = self + .handles + .read() + .unwrap() + .get(&handle_dst) + .filter(|hd| hd.inode == inode_dst) + .map(Arc::clone) + .ok_or_else(ebadf)?; + + let src = src_data.file.lock().as_raw_fd(); + let dst = dst_data.file.lock().as_raw_fd(); + + let res = unsafe { + libc::syscall( + libc::SYS_copy_file_range, + src, + &offset_src, + dst, + offset_dst, + length, + flags, + ) + }; + + if res >= 0 { + Ok(res as usize) + } else { + Err(io::Error::last_os_error()) + } + } } diff --git a/seccomp/aarch64/fs_device.policy b/seccomp/aarch64/fs_device.policy index 9fd4c8b..ec9d155 100644 --- a/seccomp/aarch64/fs_device.policy +++ b/seccomp/aarch64/fs_device.policy @@ -4,6 +4,7 @@ @include /usr/share/policy/crosvm/common_device.policy +copy_file_range: 1 fallocate: 1 fchmodat: 1 fchownat: 1 diff --git a/seccomp/arm/fs_device.policy b/seccomp/arm/fs_device.policy index eb9df16..4078f41 100644 --- a/seccomp/arm/fs_device.policy +++ b/seccomp/arm/fs_device.policy @@ -4,6 +4,7 @@ @include /usr/share/policy/crosvm/common_device.policy +copy_file_range: 1 fallocate: 1 fchmodat: 1 fchownat: 1 diff --git a/seccomp/x86_64/fs_device.policy b/seccomp/x86_64/fs_device.policy index ddb2a51..eb5a1c4 100644 --- a/seccomp/x86_64/fs_device.policy +++ b/seccomp/x86_64/fs_device.policy @@ -4,6 +4,7 @@ @include /usr/share/policy/crosvm/common_device.policy +copy_file_range: 1 fallocate: 1 fchmodat: 1 fchownat: 1 -- cgit 1.4.1 From 5b2447ceaf8ec4fa12afa32936af075c61e900df Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Tue, 17 Mar 2020 18:46:52 +0900 Subject: devices: fs: Bump fuse minor version to 31 Add new definitions and constants to support fuse minor version 31. These include the FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING opcodes used by the virtio-fs driver for implementing DAX support. BUG=b:147341783 TEST=vm.Virtiofs Change-Id: Ie59ec1a44e555910f2ee2c5ba7afccb8bd435db9 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2105823 Tested-by: Chirantan Ekbote Tested-by: kokoro Reviewed-by: Daniel Verkamp Reviewed-by: Stephen Barber Commit-Queue: Chirantan Ekbote --- devices/src/virtio/fs/fuse.rs | 52 +++++++++++++++++++++++++++++++++++++++-- devices/src/virtio/fs/server.rs | 2 +- 2 files changed, 51 insertions(+), 3 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/fs/fuse.rs b/devices/src/virtio/fs/fuse.rs index 0533a5c..5c531cf 100644 --- a/devices/src/virtio/fs/fuse.rs +++ b/devices/src/virtio/fs/fuse.rs @@ -16,7 +16,7 @@ pub const KERNEL_VERSION: u32 = 7; pub const OLDEST_SUPPORTED_KERNEL_MINOR_VERSION: u32 = 27; /// Minor version number of this interface. -pub const KERNEL_MINOR_VERSION: u32 = 28; +pub const KERNEL_MINOR_VERSION: u32 = 31; /// The ID of the inode corresponding to the root directory of the file system. pub const ROOT_ID: u64 = 1; @@ -151,6 +151,15 @@ const MAX_PAGES: u32 = 4194304; /// Cache `readlink` responses. const CACHE_SYMLINKS: u32 = 8388608; +/// Kernel supports zero-message opens for directories. +const NO_OPENDIR_SUPPORT: u32 = 16777216; + +/// Kernel supports explicit cache invalidation. +const EXPLICIT_INVAL_DATA: u32 = 33554432; + +/// The `map_alignment` field of the `InitOut` struct is valid. +const MAP_ALIGNMENT: u32 = 67108864; + bitflags! { /// A bitfield passed in as a parameter to and returned from the `init` method of the /// `FileSystem` trait. @@ -320,6 +329,34 @@ bitflags! { /// Indicates that the kernel may cache responses to `readlink` calls. const CACHE_SYMLINKS = CACHE_SYMLINKS; + + /// Indicates support for zero-message opens for directories. If this flag is set in the + /// `capable` parameter of the `init` trait method, then the file system may return `ENOSYS` + /// from the opendir() handler to indicate success. Further attempts to open directories + /// will be handled in the kernel. (If this flag is not set, returning ENOSYS will be + /// treated as an error and signaled to the caller). + /// + /// Setting (or not setting) the field in the `FsOptions` returned from the `init` method + /// has no effect. + const ZERO_MESSAGE_OPENDIR = NO_OPENDIR_SUPPORT; + + /// Indicates support for invalidating cached pages only on explicit request. + /// + /// If this flag is set in the `capable` parameter of the `init` trait method, then the FUSE + /// kernel module supports invalidating cached pages only on explicit request by the + /// filesystem. + /// + /// By setting this flag in the return value of the `init` trait method, the filesystem is + /// responsible for invalidating cached pages through explicit requests to the kernel. + /// + /// Note that setting this flag does not prevent the cached pages from being flushed by OS + /// itself and/or through user actions. + /// + /// Note that if both EXPLICIT_INVAL_DATA and AUTO_INVAL_DATA are set in the `capable` + /// parameter of the `init` trait method then AUTO_INVAL_DATA takes precedence. + /// + /// This feature is disabled by default. + const EXPLICIT_INVAL_DATA = EXPLICIT_INVAL_DATA; } } @@ -341,6 +378,9 @@ pub const WRITE_CACHE: u32 = 1; /// `lock_owner` field is valid. pub const WRITE_LOCKOWNER: u32 = 2; +/// Kill the suid and sgid bits. +pub const WRITE_KILL_PRIV: u32 = 3; + // Read flags. pub const READ_LOCKOWNER: u32 = 2; @@ -361,6 +401,9 @@ const IOCTL_32BIT: u32 = 8; /// Is a directory const IOCTL_DIR: u32 = 16; +/// 32-bit compat ioctl on 64-bit machine with 64-bit time_t. +const IOCTL_COMPAT_X32: u32 = 32; + /// Maximum of in_iovecs + out_iovecs pub const IOCTL_MAX_IOV: usize = 256; @@ -380,6 +423,9 @@ bitflags! { /// Is a directory const DIR = IOCTL_DIR; + + /// 32-bit compat ioctl on 64-bit machine with 64-bit time_t. + const COMPAT_X32 = IOCTL_COMPAT_X32; } } @@ -535,6 +581,8 @@ pub enum Opcode { Rename2 = 45, Lseek = 46, CopyFileRange = 47, + SetUpMapping = 48, + RemoveMapping = 49, } #[repr(u32)] @@ -855,7 +903,7 @@ pub struct InitOut { pub max_write: u32, pub time_gran: u32, pub max_pages: u16, - pub padding: u16, + pub map_alignment: u16, pub unused: [u32; 8], } unsafe impl DataInit for InitOut {} diff --git a/devices/src/virtio/fs/server.rs b/devices/src/virtio/fs/server.rs index 76f2830..c9025f2 100644 --- a/devices/src/virtio/fs/server.rs +++ b/devices/src/virtio/fs/server.rs @@ -119,7 +119,7 @@ impl Server { Some(Opcode::Rename2) => self.rename2(in_header, r, w), Some(Opcode::Lseek) => self.lseek(in_header, r, w), Some(Opcode::CopyFileRange) => self.copy_file_range(in_header, r, w), - None => reply_error( + Some(Opcode::SetUpMapping) | Some(Opcode::RemoveMapping) | None => reply_error( io::Error::from_raw_os_error(libc::ENOSYS), in_header.unique, w, -- cgit 1.4.1 From e5113047d5440bf09b2596f8a43f12d8e2b0061d Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Mon, 9 Mar 2020 13:04:00 -0700 Subject: devices: virtio: add virtio-console device This is a virtio device that provides a serial console. It has constructors matching the existing Serial device (new_in_out, new_out, and new_sink) that take generic io::Read and io::Write streams. This change just adds the device code; additional changes are required to add the console device to the command-line parsing and device setup code. BUG=chromium:1059924 TEST=boot linux with console=hvc0 Change-Id: I917157d5ecb5160c9b00b499eabe6fb08486776c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2095534 Reviewed-by: Dylan Reid Tested-by: kokoro Commit-Queue: Daniel Verkamp --- devices/src/virtio/console.rs | 449 ++++++++++++++++++++++++++++++++++++++++++ devices/src/virtio/mod.rs | 2 + 2 files changed, 451 insertions(+) create mode 100644 devices/src/virtio/console.rs (limited to 'devices/src') diff --git a/devices/src/virtio/console.rs b/devices/src/virtio/console.rs new file mode 100644 index 0000000..38f5bf1 --- /dev/null +++ b/devices/src/virtio/console.rs @@ -0,0 +1,449 @@ +// 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 std::io::{self, Read}; +use std::os::unix::io::RawFd; +use std::sync::mpsc::{channel, Receiver, TryRecvError}; +use std::thread; + +use data_model::{DataInit, Le16, Le32}; +use sys_util::{error, EventFd, GuestMemory, PollContext, PollToken}; + +use super::{ + copy_config, Interrupt, Queue, Reader, VirtioDevice, Writer, TYPE_CONSOLE, VIRTIO_F_VERSION_1, +}; + +const QUEUE_SIZE: u16 = 256; + +// For now, just implement port 0 (receiveq and transmitq). +// If VIRTIO_CONSOLE_F_MULTIPORT is implemented, more queues will be needed. +const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE, QUEUE_SIZE]; + +#[derive(Copy, Clone, Debug, Default)] +#[repr(C)] +struct virtio_console_config { + cols: Le16, + rows: Le16, + max_nr_ports: Le32, + emerg_wr: Le32, +} + +// Safe because it only has data and has no implicit padding. +unsafe impl DataInit for virtio_console_config {} + +struct Worker { + mem: GuestMemory, + interrupt: Interrupt, + input: Option>, + output: Option>, +} + +fn write_output(output: &mut Box, data: &[u8]) -> io::Result<()> { + output.write_all(&data)?; + output.flush() +} + +impl Worker { + fn process_transmit_request( + mut reader: Reader, + output: &mut Box, + ) -> io::Result { + let len = reader.available_bytes(); + let mut data = vec![0u8; len]; + reader.read_exact(&mut data)?; + write_output(output, &data)?; + Ok(0) + } + + fn process_transmit_queue( + &mut self, + transmit_queue: &mut Queue, + output: &mut Box, + ) { + let mut needs_interrupt = false; + while let Some(avail_desc) = transmit_queue.pop(&self.mem) { + let desc_index = avail_desc.index; + + let reader = match Reader::new(&self.mem, avail_desc) { + Ok(r) => r, + Err(e) => { + error!("console: failed to create reader: {}", e); + transmit_queue.add_used(&self.mem, desc_index, 0); + needs_interrupt = true; + continue; + } + }; + + let len = match Self::process_transmit_request(reader, output) { + Ok(written) => written, + Err(e) => { + error!("console: process_transmit_request failed: {}", e); + 0 + } + }; + + transmit_queue.add_used(&self.mem, desc_index, len); + needs_interrupt = true; + } + + if needs_interrupt { + self.interrupt.signal_used_queue(transmit_queue.vector); + } + } + + // Start a thread that reads self.input and sends the input back via the returned channel. + // + // `in_avail_evt` will be triggered by the thread when new input is available. + fn spawn_input_thread(&mut self, in_avail_evt: &EventFd) -> Option> { + let mut rx = match self.input.take() { + Some(input) => input, + None => return None, + }; + + let (send_channel, recv_channel) = channel(); + + let thread_in_avail_evt = match in_avail_evt.try_clone() { + Ok(evt) => evt, + Err(e) => { + error!("failed to clone in_avail_evt: {}", e); + return None; + } + }; + + // The input thread runs in detached mode and will exit when channel is disconnected because + // the console device has been dropped. + let res = thread::Builder::new() + .name(format!("console_input")) + .spawn(move || { + let mut rx_buf = [0u8; 1]; + loop { + match rx.read(&mut rx_buf) { + Ok(0) => break, // Assume the stream of input has ended. + Ok(_) => { + if send_channel.send(rx_buf[0]).is_err() { + // The receiver has disconnected. + break; + } + thread_in_avail_evt.write(1).unwrap(); + } + Err(e) => { + // Being interrupted is not an error, but everything else is. + if e.kind() != io::ErrorKind::Interrupted { + error!( + "failed to read for bytes to queue into console device: {}", + e + ); + break; + } + } + } + } + }); + if let Err(e) = res { + error!("failed to spawn input thread: {}", e); + return None; + } + Some(recv_channel) + } + + // Check for input from `in_channel_opt` and transfer it to the receive queue, if any. + fn handle_input( + &mut self, + in_channel_opt: &mut Option>, + receive_queue: &mut Queue, + ) { + let in_channel = match in_channel_opt.as_ref() { + Some(v) => v, + None => return, + }; + + while let Some(desc) = receive_queue.peek(&self.mem) { + let desc_index = desc.index; + let mut writer = match Writer::new(&self.mem, desc) { + Ok(w) => w, + Err(e) => { + error!("console: failed to create Writer: {}", e); + break; + } + }; + + let mut disconnected = false; + while writer.available_bytes() > 0 { + match in_channel.try_recv() { + Ok(byte) => { + writer.write_obj(byte).unwrap(); + } + Err(TryRecvError::Empty) => break, + Err(TryRecvError::Disconnected) => { + disconnected = true; + break; + } + } + } + + let bytes_written = writer.bytes_written() as u32; + + if bytes_written > 0 { + receive_queue.pop_peeked(&self.mem); + receive_queue.add_used(&self.mem, desc_index, bytes_written); + self.interrupt.signal_used_queue(receive_queue.vector); + } + + if disconnected { + // Set in_channel to None so that future handle_input calls exit early. + in_channel_opt.take(); + return; + } + + if bytes_written == 0 { + break; + } + } + } + + fn run(&mut self, mut queues: Vec, mut queue_evts: Vec, kill_evt: EventFd) { + #[derive(PollToken)] + enum Token { + ReceiveQueueAvailable, + TransmitQueueAvailable, + InputAvailable, + InterruptResample, + Kill, + } + + // Device -> driver + let (mut receive_queue, receive_evt) = (queues.remove(0), queue_evts.remove(0)); + + // Driver -> device + let (mut transmit_queue, transmit_evt) = (queues.remove(0), queue_evts.remove(0)); + + let in_avail_evt = match EventFd::new() { + Ok(evt) => evt, + Err(e) => { + error!("failed creating EventFd: {}", e); + return; + } + }; + + // Spawn a separate thread to poll self.input. + // A thread is used because io::Read only provides a blocking interface, and there is no + // generic way to add an io::Read instance to a poll context (it may not be backed by a file + // descriptor). Moving the blocking read call to a separate thread and sending data back to + // the main worker thread with an event for notification bridges this gap. + let mut in_channel = self.spawn_input_thread(&in_avail_evt); + + let poll_ctx: PollContext = match PollContext::build_with(&[ + (&transmit_evt, Token::TransmitQueueAvailable), + (&receive_evt, Token::ReceiveQueueAvailable), + (&in_avail_evt, Token::InputAvailable), + (self.interrupt.get_resample_evt(), Token::InterruptResample), + (&kill_evt, Token::Kill), + ]) { + Ok(pc) => pc, + Err(e) => { + error!("failed creating PollContext: {}", e); + return; + } + }; + + let mut output: Box = match self.output.take() { + Some(o) => o, + None => Box::new(io::sink()), + }; + + 'poll: loop { + let events = match poll_ctx.wait() { + Ok(v) => v, + Err(e) => { + error!("failed polling for events: {}", e); + break; + } + }; + + for event in events.iter_readable() { + match event.token() { + Token::TransmitQueueAvailable => { + if let Err(e) = transmit_evt.read() { + error!("failed reading transmit queue EventFd: {}", e); + break 'poll; + } + self.process_transmit_queue(&mut transmit_queue, &mut output); + } + Token::ReceiveQueueAvailable => { + if let Err(e) = receive_evt.read() { + error!("failed reading receive queue EventFd: {}", e); + break 'poll; + } + self.handle_input(&mut in_channel, &mut receive_queue); + } + Token::InputAvailable => { + if let Err(e) = in_avail_evt.read() { + error!("failed reading in_avail_evt: {}", e); + break 'poll; + } + self.handle_input(&mut in_channel, &mut receive_queue); + } + Token::InterruptResample => { + self.interrupt.interrupt_resample(); + } + Token::Kill => break 'poll, + } + } + } + } +} + +/// Virtio console device. +pub struct Console { + kill_evt: Option, + worker_thread: Option>, + input: Option>, + output: Option>, + keep_fds: Vec, +} + +impl Console { + fn new( + input: Option>, + output: Option>, + keep_fds: Vec, + ) -> Console { + Console { + kill_evt: None, + worker_thread: None, + input, + output, + keep_fds, + } + } + + /// Constructs a console with input and output streams. + pub fn new_in_out( + input: Box, + out: Box, + keep_fds: Vec, + ) -> Console { + Self::new(Some(input), Some(out), keep_fds) + } + + /// Constructs a console with an output stream but no input. + pub fn new_out(out: Box, keep_fds: Vec) -> Console { + Self::new(None, Some(out), keep_fds) + } + + /// Constructs a console with no connected input or output. + pub fn new_sink() -> Console { + Self::new(None, None, Vec::new()) + } +} + +impl Drop for Console { + fn drop(&mut self) { + if let Some(kill_evt) = self.kill_evt.take() { + // Ignore the result because there is nothing we can do about it. + let _ = kill_evt.write(1); + } + + if let Some(worker_thread) = self.worker_thread.take() { + let _ = worker_thread.join(); + } + } +} + +impl VirtioDevice for Console { + fn keep_fds(&self) -> Vec { + self.keep_fds.clone() + } + + fn features(&self) -> u64 { + 1 << VIRTIO_F_VERSION_1 + } + + fn device_type(&self) -> u32 { + TYPE_CONSOLE + } + + fn queue_max_sizes(&self) -> &[u16] { + QUEUE_SIZES + } + + fn read_config(&self, offset: u64, data: &mut [u8]) { + let config = virtio_console_config { + max_nr_ports: 1.into(), + ..Default::default() + }; + copy_config(data, 0, config.as_slice(), offset); + } + + fn activate( + &mut self, + mem: GuestMemory, + interrupt: Interrupt, + queues: Vec, + queue_evts: Vec, + ) { + if queues.len() < 2 || queue_evts.len() < 2 { + return; + } + + let (self_kill_evt, kill_evt) = match EventFd::new().and_then(|e| Ok((e.try_clone()?, e))) { + Ok(v) => v, + Err(e) => { + error!("failed creating kill EventFd pair: {}", e); + return; + } + }; + self.kill_evt = Some(self_kill_evt); + + let input = self.input.take(); + let output = self.output.take(); + + let worker_result = thread::Builder::new() + .name("virtio_console".to_string()) + .spawn(move || { + let mut worker = Worker { + mem, + interrupt, + input, + output, + }; + worker.run(queues, queue_evts, kill_evt); + worker + }); + + match worker_result { + Err(e) => { + error!("failed to spawn virtio_console worker: {}", e); + return; + } + Ok(join_handle) => { + self.worker_thread = Some(join_handle); + } + } + } + + fn reset(&mut self) -> bool { + if let Some(kill_evt) = self.kill_evt.take() { + if kill_evt.write(1).is_err() { + error!("{}: failed to notify the kill event", self.debug_label()); + return false; + } + } + + if let Some(worker_thread) = self.worker_thread.take() { + match worker_thread.join() { + Err(_) => { + error!("{}: failed to get back resources", self.debug_label()); + return false; + } + Ok(worker) => { + self.input = worker.input; + self.output = worker.output; + return true; + } + } + } + false + } +} diff --git a/devices/src/virtio/mod.rs b/devices/src/virtio/mod.rs index 7716fe0..4d5d2cb 100644 --- a/devices/src/virtio/mod.rs +++ b/devices/src/virtio/mod.rs @@ -6,6 +6,7 @@ mod balloon; mod block; +mod console; mod descriptor_utils; mod input; mod interrupt; @@ -29,6 +30,7 @@ pub mod vhost; pub use self::balloon::*; pub use self::block::*; +pub use self::console::*; pub use self::descriptor_utils::Error as DescriptorError; pub use self::descriptor_utils::*; #[cfg(feature = "gpu")] -- cgit 1.4.1 From d5c1e968c9454af0da573242a8dc3dcf63dd1820 Mon Sep 17 00:00:00 2001 From: Judy Hsiao Date: Tue, 4 Feb 2020 12:30:01 +0800 Subject: audio: Create AC97 device with --ac97 option 1. Replace --cras-audio, --cras-capture, null-audio options by --ac97 option to create audio devices. 2. "--ac97 backend=BACKEND\ [capture=true,capture_effect=EFFECT]" is comma separated key=value pairs for setting up Ac97 devices. It can be given more than once to create multiple devices. Possible key values are: backend=(null, cras) - Where to route the audio device. `null` for /dev/null, and cras for CRAS server. capture=true - Enable audio capture. capture_effects - | separated effects to be enabled for recording. The only supported effect value now is EchoCancellation or aec. BUG=b:140866281 TEST=1.crosvm run -r ./vm_rootfs.img -c 4 -m 1024 -s /run --cid 5 --host_ip\ 100.115.92.25 --netmask 255.255.255.252 --ac97\ backend=cras,capture=true,capture_effect=aec\ --mac d2:47:f7:c5:9e:53 ./vm_kernel 2. Record with the vm by: arecord -D hw:0,0 -d5 -fS16_LE -c2 -r48000 /tmp/test.mp3 3. Verify that AEC is enabled within the recording stream by cras_test_cleint. Cq-Depend: chromium:2053654 Cq-Depend: chromium:2095644 Cq-Depend: chromium:2038221 Change-Id: Ia9e0e7cda1671a4842ec77a354efaa4a2dc745eb Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2038413 Tested-by: Judy Hsiao Commit-Queue: Judy Hsiao Reviewed-by: Chih-Yang Hsia Auto-Submit: Judy Hsiao --- Cargo.lock | 1 + devices/Cargo.toml | 1 + devices/src/lib.rs | 4 +- devices/src/pci/ac97.rs | 88 +++++++++++++++++++++++++++++++- devices/src/pci/ac97_bus_master.rs | 11 +++- devices/src/pci/mod.rs | 2 +- devices/src/pci/pci_device.rs | 3 ++ src/crosvm.rs | 10 ++-- src/linux.rs | 37 ++++---------- src/main.rs | 101 ++++++++++++++++++++++++++++++++----- 10 files changed, 208 insertions(+), 50 deletions(-) (limited to 'devices/src') diff --git a/Cargo.lock b/Cargo.lock index bb75235..e9da7f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -180,6 +180,7 @@ dependencies = [ "kvm 0.1.0", "kvm_sys 0.1.0", "libc 0.2.44 (registry+https://github.com/rust-lang/crates.io-index)", + "libcras 0.1.0", "linux_input_sys 0.1.0", "msg_on_socket_derive 0.1.0", "msg_socket 0.1.0", diff --git a/devices/Cargo.toml b/devices/Cargo.toml index 83aa406..629f29b 100644 --- a/devices/Cargo.toml +++ b/devices/Cargo.toml @@ -25,6 +25,7 @@ io_jail = { path = "../io_jail" } kvm = { path = "../kvm" } kvm_sys = { path = "../kvm_sys" } libc = "*" +libcras = "*" linux_input_sys = { path = "../linux_input_sys" } msg_on_socket_derive = { path = "../msg_socket/msg_on_socket_derive" } msg_socket = { path = "../msg_socket" } diff --git a/devices/src/lib.rs b/devices/src/lib.rs index 1ecfc9c..9ed8417 100644 --- a/devices/src/lib.rs +++ b/devices/src/lib.rs @@ -30,8 +30,8 @@ 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::pci::{ - Ac97Dev, PciConfigIo, PciConfigMmio, PciDevice, PciDeviceError, PciInterruptPin, PciRoot, - VfioPciDevice, + Ac97Backend, Ac97Dev, Ac97Parameters, PciConfigIo, PciConfigMmio, PciDevice, PciDeviceError, + PciInterruptPin, PciRoot, VfioPciDevice, }; pub use self::pic::Pic; pub use self::pit::{Pit, PitError}; diff --git a/devices/src/pci/ac97.rs b/devices/src/pci/ac97.rs index 792df24..5f59165 100644 --- a/devices/src/pci/ac97.rs +++ b/devices/src/pci/ac97.rs @@ -2,9 +2,17 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +use std::default::Default; +use std::error; +use std::fmt::{self, Display}; use std::os::unix::io::RawFd; +use std::str::FromStr; -use audio_streams::shm_streams::ShmStreamSource; +use audio_streams::{ + shm_streams::{NullShmStreamSource, ShmStreamSource}, + StreamEffect, +}; +use libcras::CrasClient; use resources::{Alloc, MmioType, SystemAllocator}; use sys_util::{error, EventFd, GuestMemory}; @@ -25,6 +33,53 @@ const PCI_DEVICE_ID_INTEL_82801AA_5: u16 = 0x2415; /// Internally the `Ac97BusMaster` and `Ac97Mixer` structs are used to emulated the bus master and /// mixer registers respectively. `Ac97BusMaster` handles moving smaples between guest memory and /// the audio backend. +#[derive(Debug, Clone)] +pub enum Ac97Backend { + NULL, + CRAS, +} + +impl Default for Ac97Backend { + fn default() -> Self { + Ac97Backend::NULL + } +} + +/// Errors that are possible from a `Ac97`. +#[derive(Debug)] +pub enum Ac97Error { + InvalidBackend, +} + +impl error::Error for Ac97Error {} + +impl Display for Ac97Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Ac97Error::InvalidBackend => write!(f, "Must be cras or null"), + } + } +} + +impl FromStr for Ac97Backend { + type Err = Ac97Error; + fn from_str(s: &str) -> std::result::Result { + match s { + "cras" => Ok(Ac97Backend::CRAS), + "null" => Ok(Ac97Backend::NULL), + _ => Err(Ac97Error::InvalidBackend), + } + } +} + +/// Holds the parameters for a AC97 device +#[derive(Default, Debug, Clone)] +pub struct Ac97Parameters { + pub backend: Ac97Backend, + pub capture: bool, + pub capture_effects: Vec, +} + pub struct Ac97Dev { config_regs: PciConfiguration, pci_bus_dev: Option<(u8, u8)>, @@ -61,6 +116,37 @@ impl Ac97Dev { } } + fn create_cras_audio_device(params: Ac97Parameters, mem: GuestMemory) -> Result { + let mut server = + Box::new(CrasClient::new().map_err(|e| pci_device::Error::CreateCrasClientFailed(e))?); + if params.capture { + server.enable_cras_capture(); + } + + let mut cras_audio = Ac97Dev::new(mem, server); + cras_audio.set_capture_effects(params.capture_effects); + Ok(cras_audio) + } + + fn create_null_audio_device(mem: GuestMemory) -> Result { + let server = Box::new(NullShmStreamSource::new()); + let null_audio = Ac97Dev::new(mem, server); + Ok(null_audio) + } + + /// Creates an 'Ac97Dev' with suitable audio server inside based on Ac97Parameters + pub fn try_new(mem: GuestMemory, param: Ac97Parameters) -> Result { + match param.backend { + Ac97Backend::CRAS => Ac97Dev::create_cras_audio_device(param, mem), + Ac97Backend::NULL => Ac97Dev::create_null_audio_device(mem), + } + } + + /// Provides the effect needed in capture stream creation + pub fn set_capture_effects(&mut self, effect: Vec) { + self.bus_master.set_capture_effects(effect); + } + fn read_mixer(&mut self, offset: u64, data: &mut [u8]) { match data.len() { // The mixer is only accessed with 16-bit words. diff --git a/devices/src/pci/ac97_bus_master.rs b/devices/src/pci/ac97_bus_master.rs index cb28c5a..5f4ca75 100644 --- a/devices/src/pci/ac97_bus_master.rs +++ b/devices/src/pci/ac97_bus_master.rs @@ -165,6 +165,9 @@ pub struct Ac97BusMaster { po_info: AudioThreadInfo, pi_info: AudioThreadInfo, + // Audio effect + capture_effects: Vec, + // Audio server used to create playback or capture streams. audio_server: Box, @@ -184,12 +187,18 @@ impl Ac97BusMaster { po_info: AudioThreadInfo::new(), pi_info: AudioThreadInfo::new(), + capture_effects: Vec::new(), audio_server, irq_resample_thread: None, } } + /// Provides the effect needed in capture stream creation. + pub fn set_capture_effects(&mut self, effects: Vec) { + self.capture_effects = effects; + } + /// Returns any file descriptors that need to be kept open when entering a jail. pub fn keep_fds(&self) -> Option> { let mut fds = self.audio_server.keep_fds(); @@ -518,7 +527,7 @@ impl Ac97BusMaster { SampleFormat::S16LE, DEVICE_SAMPLE_RATE, buffer_frames, - StreamEffect::NoEffect, + self.capture_effects.as_slice(), self.mem.as_ref(), starting_offsets, ) diff --git a/devices/src/pci/mod.rs b/devices/src/pci/mod.rs index a9e8749..8c5380e 100644 --- a/devices/src/pci/mod.rs +++ b/devices/src/pci/mod.rs @@ -14,7 +14,7 @@ mod pci_device; mod pci_root; mod vfio_pci; -pub use self::ac97::Ac97Dev; +pub use self::ac97::{Ac97Backend, Ac97Dev, Ac97Parameters}; pub use self::msix::{MsixCap, MsixConfig}; pub use self::pci_configuration::{ PciBarConfiguration, PciBarPrefetchable, PciBarRegionType, PciCapability, PciCapabilityID, diff --git a/devices/src/pci/pci_device.rs b/devices/src/pci/pci_device.rs index 4c62e05..a1a3cca 100644 --- a/devices/src/pci/pci_device.rs +++ b/devices/src/pci/pci_device.rs @@ -22,6 +22,8 @@ pub enum Error { IoAllocationFailed(u64, SystemAllocatorFaliure), /// Registering an IO BAR failed. IoRegistrationFailed(u64, pci_configuration::Error), + /// Create cras client failed. + CreateCrasClientFailed(libcras::Error), } pub type Result = std::result::Result; @@ -31,6 +33,7 @@ impl Display for Error { match self { CapabilitiesSetup(e) => write!(f, "failed to add capability {}", e), + CreateCrasClientFailed(e) => write!(f, "failed to create CRAS Client: {}", e), IoAllocationFailed(size, e) => write!( f, "failed to allocate space for an IO BAR, size={}: {}", diff --git a/src/crosvm.rs b/src/crosvm.rs index b3a9233..81344c3 100644 --- a/src/crosvm.rs +++ b/src/crosvm.rs @@ -20,7 +20,7 @@ use arch::Pstore; use devices::virtio::fs::passthrough; #[cfg(feature = "gpu")] use devices::virtio::gpu::GpuParameters; -use devices::SerialParameters; +use devices::{Ac97Parameters, SerialParameters}; use libc::{getegid, geteuid}; static SECCOMP_POLICY_DIR: &str = "/usr/share/policy/crosvm"; @@ -189,11 +189,9 @@ pub struct Config { #[cfg(feature = "gpu")] pub gpu_parameters: Option, pub software_tpm: bool, - pub cras_audio: bool, - pub cras_capture: bool, - pub null_audio: bool, pub display_window_keyboard: bool, pub display_window_mouse: bool, + pub ac97_parameters: Vec, pub serial_parameters: BTreeMap, pub syslog_tag: Option, pub virtio_single_touch: Option, @@ -240,9 +238,7 @@ impl Default for Config { sandbox: !cfg!(feature = "default-no-sandbox"), seccomp_policy_dir: PathBuf::from(SECCOMP_POLICY_DIR), seccomp_log_failures: false, - cras_audio: false, - cras_capture: false, - null_audio: false, + ac97_parameters: Vec::new(), serial_parameters: BTreeMap::new(), syslog_tag: None, virtio_single_touch: None, diff --git a/src/linux.rs b/src/linux.rs index d63ab32..130644d 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -27,17 +27,15 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use libc::{self, c_int, gid_t, uid_t}; -use audio_streams::shm_streams::NullShmStreamSource; #[cfg(feature = "gpu")] use devices::virtio::EventDevice; use devices::virtio::{self, VirtioDevice}; use devices::{ - self, HostBackendDeviceProvider, PciDevice, VfioContainer, VfioDevice, VfioPciDevice, - VirtioPciDevice, XhciController, + self, Ac97Backend, Ac97Dev, HostBackendDeviceProvider, PciDevice, VfioContainer, VfioDevice, + VfioPciDevice, VirtioPciDevice, XhciController, }; use io_jail::{self, Minijail}; use kvm::*; -use libcras::CrasClient; use msg_socket::{MsgError, MsgReceiver, MsgSender, MsgSocket}; use net_util::{Error as NetError, MacAddress, Tap}; use rand_ish::SimpleRng; @@ -83,7 +81,7 @@ pub enum Error { BuildVm(::Error), ChownTpmStorage(sys_util::Error), CloneEventFd(sys_util::Error), - CreateCrasClient(libcras::Error), + CreateAc97(devices::PciDeviceError), CreateDiskError(disk::Error), CreateEventFd(sys_util::Error), CreatePollContext(sys_util::Error), @@ -168,7 +166,7 @@ impl Display for Error { BuildVm(e) => write!(f, "The architecture failed to build the vm: {}", e), ChownTpmStorage(e) => write!(f, "failed to chown tpm storage: {}", e), CloneEventFd(e) => write!(f, "failed to clone eventfd: {}", e), - CreateCrasClient(e) => write!(f, "failed to create cras client: {}", e), + CreateAc97(e) => write!(f, "failed to create ac97 device: {}", e), CreateDiskError(e) => write!(f, "failed to create virtual disk: {}", e), CreateEventFd(e) => write!(f, "failed to create eventfd: {}", e), CreatePollContext(e) => write!(f, "failed to create poll context: {}", e), @@ -1150,27 +1148,14 @@ fn create_devices( pci_devices.push((dev, stub.jail)); } - if cfg.cras_audio { - let mut server = Box::new(CrasClient::new().map_err(Error::CreateCrasClient)?); - if cfg.cras_capture { - server.enable_cras_capture(); - } - let cras_audio = devices::Ac97Dev::new(mem.clone(), server); - - pci_devices.push(( - Box::new(cras_audio), - simple_jail(&cfg, "cras_audio_device")?, - )); - } - - if cfg.null_audio { - let server = Box::new(NullShmStreamSource::new()); - let null_audio = devices::Ac97Dev::new(mem.clone(), server); + for ac97_param in &cfg.ac97_parameters { + let dev = Ac97Dev::try_new(mem.clone(), ac97_param.clone()).map_err(Error::CreateAc97)?; + let policy = match ac97_param.backend { + Ac97Backend::CRAS => "cras_audio_device", + Ac97Backend::NULL => "null_audio_device", + }; - pci_devices.push(( - Box::new(null_audio), - simple_jail(&cfg, "null_audio_device")?, - )); + pci_devices.push((Box::new(dev), simple_jail(&cfg, &policy)?)); } // Create xhci controller. let usb_controller = Box::new(XhciController::new(mem.clone(), usb_provider)); diff --git a/src/main.rs b/src/main.rs index a068057..3afca8e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,7 @@ pub mod panic_hook; +use std::default::Default; use std::fmt; use std::fs::{File, OpenOptions}; use std::io::{BufRead, BufReader}; @@ -17,13 +18,14 @@ use std::thread::sleep; use std::time::Duration; use arch::Pstore; +use audio_streams::StreamEffect; use crosvm::{ argument::{self, print_help, set_arguments, Argument}, linux, BindMount, Config, DiskOption, Executable, GidMap, SharedDir, TouchDeviceOption, }; #[cfg(feature = "gpu")] use devices::virtio::gpu::{GpuMode, GpuParameters}; -use devices::{SerialParameters, SerialType}; +use devices::{Ac97Backend, Ac97Parameters, SerialParameters, SerialType}; use disk::QcowFile; use msg_socket::{MsgReceiver, MsgSender, MsgSocket}; use sys_util::{ @@ -249,6 +251,53 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { Ok(gpu_params) } +fn parse_ac97_options(s: &str) -> argument::Result { + let mut ac97_params: Ac97Parameters = Default::default(); + + let opts = s + .split(",") + .map(|frag| frag.split("=")) + .map(|mut kv| (kv.next().unwrap_or(""), kv.next().unwrap_or(""))); + + for (k, v) in opts { + match k { + "backend" => { + ac97_params.backend = + v.parse::() + .map_err(|e| argument::Error::InvalidValue { + value: v.to_string(), + expected: e.to_string(), + })?; + } + "capture" => { + ac97_params.capture = v.parse::().map_err(|e| { + argument::Error::Syntax(format!("invalid capture option: {}", e)) + })?; + } + "capture_effects" => { + ac97_params.capture_effects = v + .split("|") + .map(|val| { + val.parse::() + .map_err(|e| argument::Error::InvalidValue { + value: val.to_string(), + expected: e.to_string(), + }) + }) + .collect::>>()?; + } + _ => { + return Err(argument::Error::UnknownArgument(format!( + "unknown ac97 parameter {}", + k + ))); + } + } + } + + Ok(ac97_params) +} + fn parse_serial_options(s: &str) -> argument::Result { let mut serial_setting = SerialParameters { type_: SerialType::Sink, @@ -479,14 +528,9 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: })?, ) } - "cras-audio" => { - cfg.cras_audio = true; - } - "cras-capture" => { - cfg.cras_capture = true; - } - "null-audio" => { - cfg.null_audio = true; + "ac97" => { + let ac97_params = parse_ac97_options(value.unwrap())?; + cfg.ac97_parameters.push(ac97_params); } "serial" => { let serial_params = parse_serial_options(value.unwrap())?; @@ -1193,9 +1237,14 @@ fn run_vm(args: std::env::Args) -> std::result::Result<(), ()> { "IP address to assign to host tap interface."), Argument::value("netmask", "NETMASK", "Netmask for VM subnet."), Argument::value("mac", "MAC", "MAC address for VM."), - Argument::flag("cras-audio", "Add an audio device to the VM that plays samples through CRAS server"), - Argument::flag("cras-capture", "Enable capturing audio from CRAS server to the cras-audio device"), - Argument::flag("null-audio", "Add an audio device to the VM that plays samples to /dev/null"), + Argument::value("ac97", + "[backend=BACKEND,capture=true,capture_effect=EFFECT]", + "Comma separated key=value pairs for setting up Ac97 devices. Can be given more than once . + Possible key values: + backend=(null, cras) - Where to route the audio device. If not provided, backend will default to null. + `null` for /dev/null, and cras for CRAS server. + capture - Enable audio capture + capture_effects - | separated effects to be enabled for recording. The only supported effect value now is EchoCancellation or aec."), Argument::value("serial", "type=TYPE,[num=NUM,path=PATH,console,stdin]", "Comma separated key=value pairs for setting up serial devices. Can be given more than once. @@ -1844,6 +1893,34 @@ mod tests { parse_cpu_set("0,1,2,").expect_err("parse should have failed"); } + #[test] + fn parse_ac97_vaild() { + parse_ac97_options("backend=cras").expect("parse should have succeded"); + } + + #[test] + fn parse_ac97_null_vaild() { + parse_ac97_options("backend=null").expect("parse should have succeded"); + } + + #[test] + fn parse_ac97_dup_effect_vaild() { + parse_ac97_options("backend=cras,capture=true,capture_effects=aec|aec") + .expect("parse should have succeded"); + } + + #[test] + fn parse_ac97_effect_invaild() { + parse_ac97_options("backend=cras,capture=true,capture_effects=abc") + .expect_err("parse should have failed"); + } + + #[test] + fn parse_ac97_effect_vaild() { + parse_ac97_options("backend=cras,capture=true,capture_effects=aec") + .expect("parse should have succeded"); + } + #[test] fn parse_serial_vaild() { parse_serial_options("type=syslog,num=1,console=true,stdin=true") -- cgit 1.4.1 From 151af70ac9778f5247f6fd58fe8b20c089604429 Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Tue, 21 Jan 2020 20:04:32 -0800 Subject: devices: gpu: modify resource v2 Rebase of zero-copy virtio-gpu flow: * Removes guest_memory_type/guest_caching_type in favor of a bitmask * Removes ALLOCATION_METADATA, since ideally we'd just read from guest memory to get guest responses * Renames HOST_COHERENT to HOST_VISIBLE * Adds a few more feature flags BUG=chromium:924405 TEST=compile Change-Id: I0d5a84b66cfa6d09f7e2d07ed8e761e7ba850284 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2013767 Tested-by: Gurchetan Singh Commit-Queue: Gurchetan Singh Reviewed-by: Lingfeng Yang Reviewed-by: Zach Reizner --- devices/src/virtio/gpu/mod.rs | 117 +++++++++------------------- devices/src/virtio/gpu/protocol.rs | 92 +++++----------------- devices/src/virtio/gpu/virtio_3d_backend.rs | 59 +++++--------- gpu_renderer/src/generated/virglrenderer.rs | 27 +++---- gpu_renderer/src/lib.rs | 59 +++++--------- 5 files changed, 110 insertions(+), 244 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs index 8cc211f..d211768 100644 --- a/devices/src/virtio/gpu/mod.rs +++ b/devices/src/virtio/gpu/mod.rs @@ -277,25 +277,16 @@ trait Backend { GpuResponse::ErrUnspec } - fn allocation_metadata( - &mut self, - _request_id: u32, - _request: Vec, - mut _response: Vec, - ) -> GpuResponse { - GpuResponse::ErrUnspec - } - fn resource_create_v2( &mut self, _resource_id: u32, - _guest_memory_type: u32, - _guest_caching_type: u32, + _ctx_id: u32, + _flags: u32, _size: u64, + _memory_id: u64, _pci_addr: u64, - _mem: &GuestMemory, _vecs: Vec<(GuestAddress, usize)>, - _args: Vec, + _mem: &GuestMemory, ) -> GpuResponse { GpuResponse::ErrUnspec } @@ -474,19 +465,19 @@ impl Frontend { let available_bytes = reader.available_bytes(); if available_bytes != 0 { let entry_count = info.nr_entries.to_native() as usize; - let mut iovecs = Vec::with_capacity(entry_count); + let mut vecs = Vec::with_capacity(entry_count); for _ in 0..entry_count { match reader.read_obj::() { Ok(entry) => { let addr = GuestAddress(entry.addr.to_native()); let len = entry.length.to_native() as usize; - iovecs.push((addr, len)) + vecs.push((addr, len)) } Err(_) => return GpuResponse::ErrUnspec, } } self.backend - .attach_backing(info.resource_id.to_native(), mem, iovecs) + .attach_backing(info.resource_id.to_native(), mem, vecs) } else { error!("missing data for command {:?}", cmd); GpuResponse::ErrUnspec @@ -610,74 +601,42 @@ impl Frontend { GpuResponse::OkNoData } } - GpuCommand::AllocationMetadata(info) => { - if reader.available_bytes() != 0 { - let id = info.request_id.to_native(); - let request_size = info.request_size.to_native(); - let response_size = info.response_size.to_native(); - if request_size > VIRTIO_GPU_MAX_BLOB_ARGUMENT_SIZE - || response_size > VIRTIO_GPU_MAX_BLOB_ARGUMENT_SIZE - { - return GpuResponse::ErrUnspec; - } - - let mut request_buf = vec![0; request_size as usize]; - let response_buf = vec![0; response_size as usize]; - if reader.read_exact(&mut request_buf[..]).is_ok() { - self.backend - .allocation_metadata(id, request_buf, response_buf) - } else { - GpuResponse::ErrInvalidParameter - } - } else { - GpuResponse::ErrUnspec - } - } GpuCommand::ResourceCreateV2(info) => { - if reader.available_bytes() != 0 { - let resource_id = info.resource_id.to_native(); - let guest_memory_type = info.guest_memory_type.to_native(); - let size = info.size.to_native(); - let guest_caching_type = info.guest_caching_type.to_native(); - let pci_addr = info.pci_addr.to_native(); - let entry_count = info.nr_entries.to_native(); - let args_size = info.args_size.to_native(); - if args_size > VIRTIO_GPU_MAX_BLOB_ARGUMENT_SIZE - || entry_count > VIRTIO_GPU_MAX_IOVEC_ENTRIES - { - return GpuResponse::ErrUnspec; - } - - let mut iovecs = Vec::with_capacity(entry_count as usize); - let mut args = vec![0; args_size as usize]; + let resource_id = info.resource_id.to_native(); + let ctx_id = info.hdr.ctx_id.to_native(); + let flags = info.flags.to_native(); + let size = info.size.to_native(); + let pci_addr = info.pci_addr.to_native(); + let memory_id = info.memory_id.to_native(); + let entry_count = info.nr_entries.to_native(); + if entry_count > VIRTIO_GPU_MAX_IOVEC_ENTRIES + || (reader.available_bytes() == 0 && entry_count > 0) + { + return GpuResponse::ErrUnspec; + } - for _ in 0..entry_count { - match reader.read_obj::() { - Ok(entry) => { - let addr = GuestAddress(entry.addr.to_native()); - let len = entry.length.to_native() as usize; - iovecs.push((addr, len)) - } - Err(_) => return GpuResponse::ErrUnspec, + let mut vecs = Vec::with_capacity(entry_count as usize); + for _ in 0..entry_count { + match reader.read_obj::() { + Ok(entry) => { + let addr = GuestAddress(entry.addr.to_native()); + let len = entry.length.to_native() as usize; + vecs.push((addr, len)) } + Err(_) => return GpuResponse::ErrUnspec, } - - match reader.read_exact(&mut args[..]) { - Ok(_) => self.backend.resource_create_v2( - resource_id, - guest_memory_type, - guest_caching_type, - size, - pci_addr, - mem, - iovecs, - args, - ), - Err(_) => GpuResponse::ErrUnspec, - } - } else { - GpuResponse::ErrUnspec } + + self.backend.resource_create_v2( + resource_id, + ctx_id, + flags, + size, + pci_addr, + memory_id, + vecs, + mem, + ) } GpuCommand::ResourceV2Unref(info) => { let resource_id = info.resource_id.to_native(); diff --git a/devices/src/virtio/gpu/protocol.rs b/devices/src/virtio/gpu/protocol.rs index b4610b2..0563c1b 100644 --- a/devices/src/virtio/gpu/protocol.rs +++ b/devices/src/virtio/gpu/protocol.rs @@ -19,9 +19,10 @@ use data_model::{DataInit, Le32, Le64}; pub const VIRTIO_GPU_F_VIRGL: u32 = 0; pub const VIRTIO_GPU_F_EDID: u32 = 1; /* The following capabilities are not upstreamed. */ -pub const VIRTIO_GPU_F_MEMORY: u32 = 2; -pub const VIRTIO_GPU_F_SHARED_GUEST: u32 = 3; -pub const VIRTIO_GPU_F_HOST_COHERENT: u32 = 4; +pub const VIRTIO_GPU_F_RESOURCE_UUID: u32 = 2; +pub const VIRTIO_GPU_F_RESOURCE_V2: u32 = 3; +pub const VIRTIO_GPU_F_HOST_VISIBLE: u32 = 4; +pub const VIRTIO_GPU_F_VULKAN: u32 = 5; pub const VIRTIO_GPU_UNDEFINED: u32 = 0x0; @@ -50,7 +51,6 @@ pub const VIRTIO_GPU_CMD_SUBMIT_3D: u32 = 0x207; /* The following hypercalls are not upstreamed. */ pub const VIRTIO_GPU_CMD_RESOURCE_CREATE_V2: u32 = 0x208; pub const VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF: u32 = 0x209; -pub const VIRTIO_GPU_CMD_ALLOCATION_METADATA: u32 = 0x20a; /* cursor commands */ pub const VIRTIO_GPU_CMD_UPDATE_CURSOR: u32 = 0x300; @@ -63,7 +63,6 @@ pub const VIRTIO_GPU_RESP_OK_CAPSET_INFO: u32 = 0x1102; pub const VIRTIO_GPU_RESP_OK_CAPSET: u32 = 0x1103; pub const VIRTIO_GPU_RESP_OK_RESOURCE_PLANE_INFO: u32 = 0x1104; pub const VIRTIO_GPU_RESP_OK_EDID: u32 = 0x1105; -pub const VIRTIO_GPU_RESP_OK_ALLOCATION_METADATA: u32 = 0x1106; /* error responses */ pub const VIRTIO_GPU_RESP_ERR_UNSPEC: u32 = 0x1200; @@ -73,20 +72,22 @@ pub const VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID: u32 = 0x1203; pub const VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID: u32 = 0x1204; pub const VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER: u32 = 0x1205; -/* guest memory types (not upstreamed) */ -pub const VIRTIO_GPU_MEMORY_UNDEFINED: u32 = 0; -pub const VIRTIO_GPU_MEMORY_TRANSFER: u32 = 1; -pub const VIRTIO_GPU_MEMORY_SHARED_GUEST: u32 = 2; -pub const VIRTIO_GPU_MEMORY_HOST_COHERENT: u32 = 3; +/* Resource flags (not upstreamed) */ +pub const VIRTIO_GPU_RESOURCE_GUEST_NONE: u32 = 0x0000; +pub const VIRTIO_GPU_RESOURCE_GUEST_MASK: u32 = 0x000f; +pub const VIRTIO_GPU_RESOURCE_GUEST_SYSTEM: u32 = 0x0001; -/* guest caching types (not upstreamed) */ -pub const VIRTIO_GPU_UNDEFINED_CACHING: u32 = 0; -pub const VIRTIO_GPU_CACHED: u32 = 1; -pub const VIRTIO_GPU_WRITE_COMBINE: u32 = 2; -pub const VIRTIO_GPU_UNCACHED: u32 = 3; +pub const VIRTIO_GPU_RESOURCE_HOST_NONE: u32 = 0x0000; +pub const VIRTIO_GPU_RESOURCE_HOST_MASK: u32 = 0x00f0; +pub const VIRTIO_GPU_RESOURCE_HOST: u32 = 0x0010; +pub const VIRTIO_GPU_RESOURCE_HOST_FROM_GUEST: u32 = 0x0020; + +pub const VIRTIO_GPU_RESOURCE_USE_NONE: u32 = 0x0000; +pub const VIRTIO_GPU_RESOURCE_USE_MASK: u32 = 0x0f00; +pub const VIRTIO_GPU_RESOURCE_USE_MAPPABLE: u32 = 0x0100; +pub const VIRTIO_GPU_RESOURCE_USE_SHAREABLE: u32 = 0x0200; +pub const VIRTIO_GPU_RESOURCE_USE_CROSS_DEVICE: u32 = 0x0400; -/* Limits on virtio-gpu stream (not upstreamed) */ -pub const VIRTIO_GPU_MAX_BLOB_ARGUMENT_SIZE: u32 = 4096; /* This matches the limit in udmabuf.c */ pub const VIRTIO_GPU_MAX_IOVEC_ENTRIES: u32 = 1024; @@ -113,7 +114,6 @@ pub fn virtio_gpu_cmd_str(cmd: u32) -> &'static str { VIRTIO_GPU_CMD_SUBMIT_3D => "VIRTIO_GPU_CMD_SUBMIT_3D", VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 => "VIRTIO_GPU_CMD_RESOURCE_CREATE_V2", VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF => "VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF", - VIRTIO_GPU_CMD_ALLOCATION_METADATA => "VIRTIO_GPU_CMD_ALLOCATION_METADATA", VIRTIO_GPU_CMD_UPDATE_CURSOR => "VIRTIO_GPU_CMD_UPDATE_CURSOR", VIRTIO_GPU_CMD_MOVE_CURSOR => "VIRTIO_GPU_CMD_MOVE_CURSOR", VIRTIO_GPU_RESP_OK_NODATA => "VIRTIO_GPU_RESP_OK_NODATA", @@ -495,41 +495,17 @@ pub struct virtio_gpu_config { unsafe impl DataInit for virtio_gpu_config {} -#[derive(Copy, Clone, Debug, Default)] -#[repr(C)] -pub struct virtio_gpu_allocation_metadata { - pub hdr: virtio_gpu_ctrl_hdr, - pub request_id: Le32, - pub padding: Le32, - pub request_size: Le32, - pub response_size: Le32, -} - -unsafe impl DataInit for virtio_gpu_allocation_metadata {} - -/* VIRTIO_GPU_RESP_OK_ALLOCATION_METADATA */ -#[derive(Copy, Clone, Debug, Default)] -#[repr(C)] -pub struct virtio_gpu_resp_allocation_metadata { - pub hdr: virtio_gpu_ctrl_hdr, - pub request_id: Le32, - pub response_size: Le32, -} - -unsafe impl DataInit for virtio_gpu_resp_allocation_metadata {} - #[derive(Copy, Clone, Debug, Default)] #[repr(C)] pub struct virtio_gpu_resource_create_v2 { pub hdr: virtio_gpu_ctrl_hdr, pub resource_id: Le32, - pub guest_memory_type: Le32, - pub guest_caching_type: Le32, - pub padding: Le32, + pub flags: Le32, pub size: Le64, pub pci_addr: Le64, - pub args_size: Le32, + pub memory_id: Le64, pub nr_entries: Le32, + pub padding: Le32, } unsafe impl DataInit for virtio_gpu_resource_create_v2 {} @@ -577,7 +553,6 @@ pub enum GpuCommand { CmdSubmit3d(virtio_gpu_cmd_submit), ResourceCreateV2(virtio_gpu_resource_create_v2), ResourceV2Unref(virtio_gpu_resource_v2_unref), - AllocationMetadata(virtio_gpu_allocation_metadata), UpdateCursor(virtio_gpu_update_cursor), MoveCursor(virtio_gpu_update_cursor), } @@ -646,7 +621,6 @@ impl fmt::Debug for GpuCommand { CmdSubmit3d(_info) => f.debug_struct("CmdSubmit3d").finish(), ResourceCreateV2(_info) => f.debug_struct("ResourceCreateV2").finish(), ResourceV2Unref(_info) => f.debug_struct("ResourceV2Unref").finish(), - AllocationMetadata(_info) => f.debug_struct("AllocationMetadata").finish(), UpdateCursor(_info) => f.debug_struct("UpdateCursor").finish(), MoveCursor(_info) => f.debug_struct("MoveCursor").finish(), } @@ -679,7 +653,6 @@ impl GpuCommand { VIRTIO_GPU_CMD_SUBMIT_3D => CmdSubmit3d(cmd.read_obj()?), VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 => ResourceCreateV2(cmd.read_obj()?), VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF => ResourceV2Unref(cmd.read_obj()?), - VIRTIO_GPU_CMD_ALLOCATION_METADATA => AllocationMetadata(cmd.read_obj()?), VIRTIO_GPU_CMD_UPDATE_CURSOR => UpdateCursor(cmd.read_obj()?), VIRTIO_GPU_CMD_MOVE_CURSOR => MoveCursor(cmd.read_obj()?), _ => return Err(GpuCommandDecodeError::InvalidType(hdr.type_.into())), @@ -710,7 +683,6 @@ impl GpuCommand { CmdSubmit3d(info) => &info.hdr, ResourceCreateV2(info) => &info.hdr, ResourceV2Unref(info) => &info.hdr, - AllocationMetadata(info) => &info.hdr, UpdateCursor(info) => &info.hdr, MoveCursor(info) => &info.hdr, } @@ -723,12 +695,6 @@ pub struct GpuResponsePlaneInfo { pub offset: u32, } -#[derive(Default, Debug, PartialEq)] -pub struct AllocationMetadataResponse { - pub request_id: u32, - pub response: Vec, -} - /// A response to a `GpuCommand`. These correspond to `VIRTIO_GPU_RESP_*`. #[derive(Debug, PartialEq)] pub enum GpuResponse { @@ -744,9 +710,6 @@ pub enum GpuResponse { format_modifier: u64, plane_info: Vec, }, - OkAllocationMetadata { - res_info: AllocationMetadataResponse, - }, ErrUnspec, ErrOutOfMemory, ErrInvalidScanoutId, @@ -880,17 +843,6 @@ impl GpuResponse { size_of_val(&hdr) } } - GpuResponse::OkAllocationMetadata { ref res_info } => { - let resp_info = virtio_gpu_resp_allocation_metadata { - hdr, - request_id: Le32::from(res_info.request_id), - response_size: Le32::from(res_info.response.len() as u32), - }; - - resp.write_obj(resp_info)?; - resp.write_all(&res_info.response)?; - size_of_val(&resp_info) + res_info.response.len() - } _ => { resp.write_obj(hdr)?; size_of_val(&hdr) @@ -907,7 +859,6 @@ impl GpuResponse { GpuResponse::OkCapsetInfo { .. } => VIRTIO_GPU_RESP_OK_CAPSET_INFO, GpuResponse::OkCapset(_) => VIRTIO_GPU_RESP_OK_CAPSET, GpuResponse::OkResourcePlaneInfo { .. } => VIRTIO_GPU_RESP_OK_RESOURCE_PLANE_INFO, - GpuResponse::OkAllocationMetadata { .. } => VIRTIO_GPU_RESP_OK_ALLOCATION_METADATA, GpuResponse::ErrUnspec => VIRTIO_GPU_RESP_ERR_UNSPEC, GpuResponse::ErrOutOfMemory => VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY, GpuResponse::ErrInvalidScanoutId => VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID, @@ -925,7 +876,6 @@ impl GpuResponse { GpuResponse::OkCapsetInfo { .. } => true, GpuResponse::OkCapset(_) => true, GpuResponse::OkResourcePlaneInfo { .. } => true, - GpuResponse::OkAllocationMetadata { .. } => true, _ => false, } } diff --git a/devices/src/virtio/gpu/virtio_3d_backend.rs b/devices/src/virtio/gpu/virtio_3d_backend.rs index 7ae044c..21b5bf4 100644 --- a/devices/src/virtio/gpu/virtio_3d_backend.rs +++ b/devices/src/virtio/gpu/virtio_3d_backend.rs @@ -26,13 +26,13 @@ use gpu_renderer::{ }; use super::protocol::{ - AllocationMetadataResponse, GpuResponse, GpuResponsePlaneInfo, VIRTIO_GPU_CAPSET3, - VIRTIO_GPU_CAPSET_VIRGL, VIRTIO_GPU_CAPSET_VIRGL2, VIRTIO_GPU_MEMORY_HOST_COHERENT, + GpuResponse, GpuResponsePlaneInfo, VIRTIO_GPU_CAPSET3, VIRTIO_GPU_CAPSET_VIRGL, + VIRTIO_GPU_CAPSET_VIRGL2, VIRTIO_GPU_RESOURCE_USE_MAPPABLE, VIRTIO_GPU_RESOURCE_USE_MASK, }; pub use crate::virtio::gpu::virtio_backend::{VirtioBackend, VirtioResource}; use crate::virtio::gpu::{ - Backend, DisplayBackend, VIRTIO_F_VERSION_1, VIRTIO_GPU_F_HOST_COHERENT, VIRTIO_GPU_F_MEMORY, - VIRTIO_GPU_F_VIRGL, + 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, }; use crate::virtio::resource_bridge::{PlaneInfo, ResourceInfo, ResourceResponse}; @@ -225,8 +225,10 @@ impl Backend for Virtio3DBackend { fn features() -> u64 { 1 << VIRTIO_GPU_F_VIRGL | 1 << VIRTIO_F_VERSION_1 - | 1 << VIRTIO_GPU_F_MEMORY - | 1 << VIRTIO_GPU_F_HOST_COHERENT + | 1 << VIRTIO_GPU_F_RESOURCE_UUID + | 1 << VIRTIO_GPU_F_RESOURCE_V2 + | 1 << VIRTIO_GPU_F_HOST_VISIBLE + | 1 << VIRTIO_GPU_F_VULKAN } /// Returns the underlying Backend. @@ -757,51 +759,27 @@ impl Backend for Virtio3DBackend { } } - fn allocation_metadata( - &mut self, - request_id: u32, - request: Vec, - mut response: Vec, - ) -> GpuResponse { - let res = self.renderer.allocation_metadata(&request, &mut response); - - match res { - Ok(_) => { - let res_info = AllocationMetadataResponse { - request_id, - response, - }; - - GpuResponse::OkAllocationMetadata { res_info } - } - Err(_) => { - error!("failed to get metadata"); - GpuResponse::ErrUnspec - } - } - } - fn resource_create_v2( &mut self, resource_id: u32, - guest_memory_type: u32, - guest_caching_type: u32, + ctx_id: u32, + flags: u32, size: u64, pci_addr: u64, - mem: &GuestMemory, + memory_id: u64, vecs: Vec<(GuestAddress, usize)>, - args: Vec, + mem: &GuestMemory, ) -> GpuResponse { match self.resources.entry(resource_id) { Entry::Vacant(entry) => { let resource = match self.renderer.resource_create_v2( resource_id, - guest_memory_type, - guest_caching_type, + ctx_id, + flags, size, - mem, + memory_id, &vecs, - &args, + mem, ) { Ok(resource) => resource, Err(e) => { @@ -810,8 +788,9 @@ impl Backend for Virtio3DBackend { } }; - match guest_memory_type { - VIRTIO_GPU_MEMORY_HOST_COHERENT => { + let use_flags = VIRTIO_GPU_RESOURCE_USE_MASK & flags; + match use_flags { + VIRTIO_GPU_RESOURCE_USE_MAPPABLE => { let dma_buf_fd = match resource.export() { Ok(export) => export.1, Err(e) => { diff --git a/gpu_renderer/src/generated/virglrenderer.rs b/gpu_renderer/src/generated/virglrenderer.rs index 01bf219..020d69f 100644 --- a/gpu_renderer/src/generated/virglrenderer.rs +++ b/gpu_renderer/src/generated/virglrenderer.rs @@ -307,24 +307,21 @@ extern "C" { execute_size: u32, ) -> ::std::os::raw::c_int; } -extern "C" { - pub fn virgl_renderer_allocation_metadata( - request: *const ::std::os::raw::c_void, - response: *mut ::std::os::raw::c_void, - request_size: u32, - response_size: u32, - ) -> ::std::os::raw::c_int; +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct virgl_renderer_resource_create_v2_args { + pub version: u32, + pub res_handle: u32, + pub ctx_id: u32, + pub flags: u32, + pub size: u64, + pub memory_id: u64, + pub iovecs: *mut iovec, + pub num_iovs: u32, } extern "C" { pub fn virgl_renderer_resource_create_v2( - resource_id: u32, - guest_memory_type: u32, - guest_caching_type: u32, - size: u64, - iovec: *const iovec, - num_iovs: u32, - args: *const ::std::os::raw::c_void, - args_size: u32, + args: *mut virgl_renderer_resource_create_v2_args, ) -> ::std::os::raw::c_int; } pub type __builtin_va_list = [__va_list_tag; 1usize]; diff --git a/gpu_renderer/src/lib.rs b/gpu_renderer/src/lib.rs index d75add1..5e5ab95 100644 --- a/gpu_renderer/src/lib.rs +++ b/gpu_renderer/src/lib.rs @@ -37,7 +37,7 @@ pub use crate::command_buffer::CommandBufferBuilder; #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] pub use crate::vsnprintf::vsnprintf; -/// Arguments used in `Renderer::create_resource`.. +/// Arguments used in `Renderer::create_resource`. pub type ResourceCreateArgs = virgl_renderer_resource_create_args; /// Some of the information returned from `Resource::export_query`. pub type Query = virgl_renderer_export_query; @@ -405,72 +405,53 @@ impl Renderer { unsafe { virgl_renderer_force_ctx_0() }; } - #[allow(unused_variables)] - pub fn allocation_metadata(&self, request: &[u8], response: &mut Vec) -> Result<()> { - #[cfg(feature = "virtio-gpu-next")] - { - let ret = unsafe { - virgl_renderer_allocation_metadata( - request.as_ptr() as *const c_void, - response.as_mut_ptr() as *mut c_void, - request.len() as u32, - response.len() as u32, - ) - }; - ret_to_res(ret) - } - #[cfg(not(feature = "virtio-gpu-next"))] - Err(Error::Unsupported) - } - #[allow(unused_variables)] pub fn resource_create_v2( &self, resource_id: u32, - guest_memory_type: u32, - guest_caching_type: u32, + ctx_id: u32, + flags: u32, size: u64, + memory_id: u64, + vecs: &[(GuestAddress, usize)], mem: &GuestMemory, - iovecs: &[(GuestAddress, usize)], - args: &[u8], ) -> Result { #[cfg(feature = "virtio-gpu-next")] { - if iovecs + if vecs .iter() .any(|&(addr, len)| mem.get_slice(addr.offset(), len as u64).is_err()) { return Err(Error::InvalidIovec); } - let mut vecs = Vec::new(); - for &(addr, len) in iovecs { + let mut iovecs = Vec::new(); + for &(addr, len) in vecs { // Unwrap will not panic because we already checked the slices. let slice = mem.get_slice(addr.offset(), len as u64).unwrap(); - vecs.push(VirglVec { + iovecs.push(VirglVec { base: slice.as_ptr() as *mut c_void, len, }); } - let ret = unsafe { - virgl_renderer_resource_create_v2( - resource_id, - guest_memory_type, - guest_caching_type, - size, - vecs.as_ptr() as *const iovec, - vecs.len() as u32, - args.as_ptr() as *const c_void, - args.len() as u32, - ) + let mut resource_create_args = virgl_renderer_resource_create_v2_args { + version: 1, + res_handle: resource_id, + ctx_id, + flags, + size, + memory_id, + iovecs: iovecs.as_mut_ptr() as *mut iovec, + num_iovs: iovecs.len() as u32, }; + let ret = unsafe { virgl_renderer_resource_create_v2(&mut resource_create_args) }; ret_to_res(ret)?; Ok(Resource { id: resource_id, - backing_iovecs: vecs, + backing_iovecs: iovecs, backing_mem: None, no_sync_send: PhantomData, }) -- cgit 1.4.1 From 83fc5c49ba930a62ef1f4d93bc2004d779b6d16f Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Wed, 22 Jan 2020 17:59:22 -0800 Subject: devices: gpu: complete resource V2 rebase * Remove RESOURCE_V2_UNREF * Add RESOURCE_MAP/RESOURCE_UNMAP to enable resources without guest storage that don't need to be mapped directly either BUG=chromium:924405 TEST=compile and test Change-Id: I10d6cd120d86131fa7ed8917ddad25cdb99ae50c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2015587 Tested-by: Gurchetan Singh Auto-Submit: Gurchetan Singh Reviewed-by: Zach Reizner Commit-Queue: Gurchetan Singh --- devices/src/virtio/gpu/mod.rs | 18 ++- devices/src/virtio/gpu/protocol.rs | 34 +++-- devices/src/virtio/gpu/virtio_3d_backend.rs | 216 ++++++++++++++++------------ vm_control/src/lib.rs | 9 +- 4 files changed, 164 insertions(+), 113 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs index d211768..93d004f 100644 --- a/devices/src/virtio/gpu/mod.rs +++ b/devices/src/virtio/gpu/mod.rs @@ -284,14 +284,17 @@ trait Backend { _flags: u32, _size: u64, _memory_id: u64, - _pci_addr: u64, _vecs: Vec<(GuestAddress, usize)>, _mem: &GuestMemory, ) -> GpuResponse { GpuResponse::ErrUnspec } - fn resource_v2_unref(&mut self, _resource_id: u32) -> GpuResponse { + fn resource_map(&mut self, _resource_id: u32, _pci_addr: u64) -> GpuResponse { + GpuResponse::ErrUnspec + } + + fn resource_unmap(&mut self, _resource_id: u32) -> GpuResponse { GpuResponse::ErrUnspec } } @@ -606,7 +609,6 @@ impl Frontend { let ctx_id = info.hdr.ctx_id.to_native(); let flags = info.flags.to_native(); let size = info.size.to_native(); - let pci_addr = info.pci_addr.to_native(); let memory_id = info.memory_id.to_native(); let entry_count = info.nr_entries.to_native(); if entry_count > VIRTIO_GPU_MAX_IOVEC_ENTRIES @@ -632,15 +634,19 @@ impl Frontend { ctx_id, flags, size, - pci_addr, memory_id, vecs, mem, ) } - GpuCommand::ResourceV2Unref(info) => { + GpuCommand::ResourceMap(info) => { + let resource_id = info.resource_id.to_native(); + let offset = info.offset.to_native(); + self.backend.resource_map(resource_id, offset) + } + GpuCommand::ResourceUnmap(info) => { let resource_id = info.resource_id.to_native(); - self.backend.resource_v2_unref(resource_id) + self.backend.resource_unmap(resource_id) } } } diff --git a/devices/src/virtio/gpu/protocol.rs b/devices/src/virtio/gpu/protocol.rs index 0563c1b..c98e289 100644 --- a/devices/src/virtio/gpu/protocol.rs +++ b/devices/src/virtio/gpu/protocol.rs @@ -50,7 +50,8 @@ pub const VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D: u32 = 0x206; pub const VIRTIO_GPU_CMD_SUBMIT_3D: u32 = 0x207; /* The following hypercalls are not upstreamed. */ pub const VIRTIO_GPU_CMD_RESOURCE_CREATE_V2: u32 = 0x208; -pub const VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF: u32 = 0x209; +pub const VIRTIO_GPU_CMD_RESOURCE_MAP: u32 = 0x209; +pub const VIRTIO_GPU_CMD_RESOURCE_UNMAP: u32 = 0x20a; /* cursor commands */ pub const VIRTIO_GPU_CMD_UPDATE_CURSOR: u32 = 0x300; @@ -113,7 +114,8 @@ pub fn virtio_gpu_cmd_str(cmd: u32) -> &'static str { VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D => "VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D", VIRTIO_GPU_CMD_SUBMIT_3D => "VIRTIO_GPU_CMD_SUBMIT_3D", VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 => "VIRTIO_GPU_CMD_RESOURCE_CREATE_V2", - VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF => "VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF", + VIRTIO_GPU_CMD_RESOURCE_MAP => "VIRTIO_GPU_RESOURCE_MAP", + VIRTIO_GPU_CMD_RESOURCE_UNMAP => "VIRTIO_GPU_RESOURCE_UNMAP", VIRTIO_GPU_CMD_UPDATE_CURSOR => "VIRTIO_GPU_CMD_UPDATE_CURSOR", VIRTIO_GPU_CMD_MOVE_CURSOR => "VIRTIO_GPU_CMD_MOVE_CURSOR", VIRTIO_GPU_RESP_OK_NODATA => "VIRTIO_GPU_RESP_OK_NODATA", @@ -502,7 +504,6 @@ pub struct virtio_gpu_resource_create_v2 { pub resource_id: Le32, pub flags: Le32, pub size: Le64, - pub pci_addr: Le64, pub memory_id: Le64, pub nr_entries: Le32, pub padding: Le32, @@ -512,13 +513,24 @@ unsafe impl DataInit for virtio_gpu_resource_create_v2 {} #[derive(Copy, Clone, Debug, Default)] #[repr(C)] -pub struct virtio_gpu_resource_v2_unref { +pub struct virtio_gpu_resource_map { + pub hdr: virtio_gpu_ctrl_hdr, + pub resource_id: Le32, + pub map_flags: Le32, + pub offset: Le64, +} + +unsafe impl DataInit for virtio_gpu_resource_map {} + +#[derive(Copy, Clone, Debug, Default)] +#[repr(C)] +pub struct virtio_gpu_resource_unmap { pub hdr: virtio_gpu_ctrl_hdr, pub resource_id: Le32, pub padding: Le32, } -unsafe impl DataInit for virtio_gpu_resource_v2_unref {} +unsafe impl DataInit for virtio_gpu_resource_unmap {} /* simple formats for fbcon/X use */ pub const VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM: u32 = 1; @@ -552,7 +564,8 @@ pub enum GpuCommand { TransferFromHost3d(virtio_gpu_transfer_host_3d), CmdSubmit3d(virtio_gpu_cmd_submit), ResourceCreateV2(virtio_gpu_resource_create_v2), - ResourceV2Unref(virtio_gpu_resource_v2_unref), + ResourceMap(virtio_gpu_resource_map), + ResourceUnmap(virtio_gpu_resource_unmap), UpdateCursor(virtio_gpu_update_cursor), MoveCursor(virtio_gpu_update_cursor), } @@ -620,7 +633,8 @@ impl fmt::Debug for GpuCommand { TransferFromHost3d(_info) => f.debug_struct("TransferFromHost3d").finish(), CmdSubmit3d(_info) => f.debug_struct("CmdSubmit3d").finish(), ResourceCreateV2(_info) => f.debug_struct("ResourceCreateV2").finish(), - ResourceV2Unref(_info) => f.debug_struct("ResourceV2Unref").finish(), + ResourceMap(_info) => f.debug_struct("ResourceMap").finish(), + ResourceUnmap(_info) => f.debug_struct("ResourceUnmap").finish(), UpdateCursor(_info) => f.debug_struct("UpdateCursor").finish(), MoveCursor(_info) => f.debug_struct("MoveCursor").finish(), } @@ -652,7 +666,8 @@ impl GpuCommand { VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D => TransferFromHost3d(cmd.read_obj()?), VIRTIO_GPU_CMD_SUBMIT_3D => CmdSubmit3d(cmd.read_obj()?), VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 => ResourceCreateV2(cmd.read_obj()?), - VIRTIO_GPU_CMD_RESOURCE_CREATE_V2_UNREF => ResourceV2Unref(cmd.read_obj()?), + VIRTIO_GPU_CMD_RESOURCE_MAP => ResourceMap(cmd.read_obj()?), + VIRTIO_GPU_CMD_RESOURCE_UNMAP => ResourceUnmap(cmd.read_obj()?), VIRTIO_GPU_CMD_UPDATE_CURSOR => UpdateCursor(cmd.read_obj()?), VIRTIO_GPU_CMD_MOVE_CURSOR => MoveCursor(cmd.read_obj()?), _ => return Err(GpuCommandDecodeError::InvalidType(hdr.type_.into())), @@ -682,7 +697,8 @@ impl GpuCommand { TransferFromHost3d(info) => &info.hdr, CmdSubmit3d(info) => &info.hdr, ResourceCreateV2(info) => &info.hdr, - ResourceV2Unref(info) => &info.hdr, + ResourceMap(info) => &info.hdr, + ResourceUnmap(info) => &info.hdr, UpdateCursor(info) => &info.hdr, MoveCursor(info) => &info.hdr, } diff --git a/devices/src/virtio/gpu/virtio_3d_backend.rs b/devices/src/virtio/gpu/virtio_3d_backend.rs index 21b5bf4..3ede8a6 100644 --- a/devices/src/virtio/gpu/virtio_3d_backend.rs +++ b/devices/src/virtio/gpu/virtio_3d_backend.rs @@ -44,6 +44,8 @@ struct Virtio3DResource { gpu_resource: GpuRendererResource, display_import: Option<(Rc>, u32)>, kvm_slot: Option, + size: u64, + flags: u32, } impl Virtio3DResource { @@ -54,27 +56,38 @@ impl Virtio3DResource { gpu_resource, display_import: None, kvm_slot: None, + flags: 0, + // The size of the host resource isn't really zero, but it's undefined by + // virtio_gpu_resource_create_3d + size: 0, } } pub fn v2_new( width: u32, height: u32, - kvm_slot: u32, gpu_resource: GpuRendererResource, + flags: u32, + size: u64, ) -> Virtio3DResource { Virtio3DResource { width, height, gpu_resource, display_import: None, - kvm_slot: Some(kvm_slot), + kvm_slot: None, + flags, + size, } } fn as_mut(&mut self) -> &mut dyn VirtioResource { self } + + fn use_flags(&self) -> u32 { + self.flags & VIRTIO_GPU_RESOURCE_USE_MASK + } } impl VirtioResource for Virtio3DResource { @@ -765,7 +778,6 @@ impl Backend for Virtio3DBackend { ctx_id: u32, flags: u32, size: u64, - pci_addr: u64, memory_id: u64, vecs: Vec<(GuestAddress, usize)>, mem: &GuestMemory, @@ -788,103 +800,119 @@ impl Backend for Virtio3DBackend { } }; - let use_flags = VIRTIO_GPU_RESOURCE_USE_MASK & flags; - match use_flags { - VIRTIO_GPU_RESOURCE_USE_MAPPABLE => { - let dma_buf_fd = match resource.export() { - Ok(export) => export.1, - Err(e) => { - error!("failed to export plane fd: {}", e); - return GpuResponse::ErrUnspec; - } - }; - - let request = VmMemoryRequest::RegisterMemoryAtAddress( - self.pci_bar, - MaybeOwnedFd::Borrowed(dma_buf_fd.as_raw_fd()), - size as usize, - pci_addr, - ); - - match self.gpu_device_socket.send(&request) { - Ok(_resq) => match self.gpu_device_socket.recv() { - Ok(response) => match response { - VmMemoryResponse::RegisterMemory { pfn: _, slot } => { - entry.insert(Virtio3DResource::v2_new( - self.base.display_width, - self.base.display_height, - slot, - resource, - )); - GpuResponse::OkNoData - } - VmMemoryResponse::Err(e) => { - error!("received an error: {}", e); - GpuResponse::ErrUnspec - } - _ => { - error!("recieved an unexpected response"); - GpuResponse::ErrUnspec - } - }, - Err(e) => { - error!("failed to receive data: {}", e); - GpuResponse::ErrUnspec - } - }, - Err(e) => { - error!("failed to send request: {}", e); - GpuResponse::ErrUnspec - } - } - } - _ => { - entry.insert(Virtio3DResource::new( - self.base.display_width, - self.base.display_height, - resource, - )); + entry.insert(Virtio3DResource::v2_new( + self.base.display_width, + self.base.display_height, + resource, + flags, + size, + )); - GpuResponse::OkNoData - } - } + GpuResponse::OkNoData } Entry::Occupied(_) => GpuResponse::ErrInvalidResourceId, } } - fn resource_v2_unref(&mut self, resource_id: u32) -> GpuResponse { - match self.resources.remove(&resource_id) { - Some(entry) => match entry.kvm_slot { - Some(kvm_slot) => { - let request = VmMemoryRequest::UnregisterMemory(kvm_slot); - match self.gpu_device_socket.send(&request) { - Ok(_resq) => match self.gpu_device_socket.recv() { - Ok(response) => match response { - VmMemoryResponse::Ok => GpuResponse::OkNoData, - VmMemoryResponse::Err(e) => { - error!("received an error: {}", e); - GpuResponse::ErrUnspec - } - _ => { - error!("recieved an unexpected response"); - GpuResponse::ErrUnspec - } - }, - Err(e) => { - error!("failed to receive data: {}", e); - GpuResponse::ErrUnspec - } - }, - Err(e) => { - error!("failed to send request: {}", e); - GpuResponse::ErrUnspec - } - } - } - None => GpuResponse::OkNoData, - }, - None => GpuResponse::ErrInvalidResourceId, + fn resource_map(&mut self, resource_id: u32, offset: u64) -> GpuResponse { + let resource = match self.resources.get_mut(&resource_id) { + Some(r) => r, + None => return GpuResponse::ErrInvalidResourceId, + }; + + if resource.use_flags() & VIRTIO_GPU_RESOURCE_USE_MAPPABLE == 0 { + error!("resource not mappable"); + return GpuResponse::ErrUnspec; + } + + let dma_buf_fd = match resource.gpu_resource.export() { + Ok(export) => export.1, + Err(e) => { + error!("failed to export plane fd: {}", e); + return GpuResponse::ErrUnspec; + } + }; + + let request = VmMemoryRequest::RegisterFdAtPciBarOffset( + self.pci_bar, + MaybeOwnedFd::Borrowed(dma_buf_fd.as_raw_fd()), + resource.size as usize, + offset, + ); + + match self.gpu_device_socket.send(&request) { + Ok(_) => (), + Err(e) => { + error!("failed to send request: {}", e); + return GpuResponse::ErrUnspec; + } + } + + let response = match self.gpu_device_socket.recv() { + Ok(response) => response, + Err(e) => { + error!("failed to receive data: {}", e); + return GpuResponse::ErrUnspec; + } + }; + + match response { + VmMemoryResponse::RegisterMemory { pfn: _, slot } => { + resource.kvm_slot = Some(slot); + GpuResponse::OkNoData + } + VmMemoryResponse::Err(e) => { + error!("received an error: {}", e); + GpuResponse::ErrUnspec + } + _ => { + error!("recieved an unexpected response"); + GpuResponse::ErrUnspec + } + } + } + + fn resource_unmap(&mut self, resource_id: u32) -> GpuResponse { + let resource = match self.resources.get_mut(&resource_id) { + Some(r) => r, + None => return GpuResponse::ErrInvalidResourceId, + }; + + let kvm_slot = match resource.kvm_slot { + Some(kvm_slot) => kvm_slot, + None => return GpuResponse::ErrUnspec, + }; + + let request = VmMemoryRequest::UnregisterMemory(kvm_slot); + match self.gpu_device_socket.send(&request) { + Ok(_) => (), + Err(e) => { + error!("failed to send request: {}", e); + return GpuResponse::ErrUnspec; + } + } + + let response = match self.gpu_device_socket.recv() { + Ok(response) => response, + Err(e) => { + error!("failed to receive data: {}", e); + return GpuResponse::ErrUnspec; + } + }; + + match response { + VmMemoryResponse::Ok => { + resource.kvm_slot = None; + GpuResponse::OkNoData + } + VmMemoryResponse::Err(e) => { + error!("received an error: {}", e); + GpuResponse::ErrUnspec + } + _ => { + error!("recieved an unexpected response"); + GpuResponse::ErrUnspec + } } } } diff --git a/vm_control/src/lib.rs b/vm_control/src/lib.rs index 298d800..b2577d9 100644 --- a/vm_control/src/lib.rs +++ b/vm_control/src/lib.rs @@ -192,7 +192,7 @@ pub enum VmMemoryRequest { RegisterMemory(MaybeOwnedFd, usize), /// Similiar to `VmMemoryRequest::RegisterMemory`, but doesn't allocate new address space. /// Useful for cases where the address space is already allocated (PCI regions). - RegisterMemoryAtAddress(Alloc, MaybeOwnedFd, usize, u64), + RegisterFdAtPciBarOffset(Alloc, MaybeOwnedFd, usize, u64), /// Unregister the given memory slot that was previously registereed with `RegisterMemory`. UnregisterMemory(u32), /// Allocate GPU buffer of a given size/format and register the memory into guest address space. @@ -230,8 +230,8 @@ impl VmMemoryRequest { Err(e) => VmMemoryResponse::Err(e), } } - RegisterMemoryAtAddress(alloc, ref fd, size, guest_addr) => { - match register_memory(vm, sys_allocator, fd, size, Some((alloc, guest_addr))) { + RegisterFdAtPciBarOffset(alloc, ref fd, size, offset) => { + match register_memory(vm, sys_allocator, fd, size, Some((alloc, offset))) { Ok((pfn, slot)) => VmMemoryResponse::RegisterMemory { pfn, slot }, Err(e) => VmMemoryResponse::Err(e), } @@ -433,12 +433,13 @@ fn register_memory( }; let addr = match allocation { - Some((Alloc::PciBar { bus, dev, bar }, address)) => { + Some((Alloc::PciBar { bus, dev, bar }, offset)) => { match allocator .mmio_allocator(MmioType::High) .get(&Alloc::PciBar { bus, dev, bar }) { Some((start_addr, length, _)) => { + let address = *start_addr + offset; let range = *start_addr..*start_addr + *length; let end = address + (size as u64); match (range.contains(&address), range.contains(&end)) { -- cgit 1.4.1 From 0dcbfa1176386d1e2f8f52947fe93004db195abe Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 18 Mar 2020 13:10:49 -0700 Subject: usb: avoid setting configuration when unnecessary On devices with only one configuration, skip the code that attempts to change the device's active configuration, since it must always be the single available configuration. This works around an issue observed with some USB devices (e.g. Servo Micro) where the initial Get Configuration control request fails with -EPIPE. BUG=chromium:1061382 BUG=b:151408644 TEST=Attach servo micro, see /dev/ttyUSB[012], screen /dev/ttyUSB0 Change-Id: Ic3333e1d70a0c57b090de64e4d3b7932ce2cf81d Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2108871 Reviewed-by: Stephen Barber Tested-by: kokoro Tested-by: George Engelbrecht Commit-Queue: George Engelbrecht --- devices/src/usb/host_backend/host_device.rs | 40 +++++++++++++++++++---------- usb_util/src/device.rs | 13 +++++----- 2 files changed, 34 insertions(+), 19 deletions(-) (limited to 'devices/src') diff --git a/devices/src/usb/host_backend/host_device.rs b/devices/src/usb/host_backend/host_device.rs index 3b85ea2..196fa50 100644 --- a/devices/src/usb/host_backend/host_device.rs +++ b/devices/src/usb/host_backend/host_device.rs @@ -305,22 +305,31 @@ impl HostDevice { config ); self.release_interfaces(); - let cur_config = self - .device - .lock() - .get_active_configuration() - .map_err(Error::GetActiveConfig)?; - usb_debug!("current config is: {}", cur_config); - if config != cur_config { - self.device - .lock() - .set_active_configuration(config) - .map_err(Error::SetActiveConfig)?; + if self.device.lock().get_num_configurations() > 1 { + let cur_config = match self.device.lock().get_active_configuration() { + Ok(c) => Some(c), + Err(e) => { + // The device may be in the default state, in which case + // GET_CONFIGURATION may fail. Assume the device needs to be + // reconfigured. + usb_debug!("Failed to get active configuration: {}", e); + error!("Failed to get active configuration: {}", e); + None + } + }; + if Some(config) != cur_config { + self.device + .lock() + .set_active_configuration(config) + .map_err(Error::SetActiveConfig)?; + } + } else { + usb_debug!("Only one configuration - not calling set_active_configuration"); } let config_descriptor = self .device .lock() - .get_active_config_descriptor() + .get_config_descriptor(config) .map_err(Error::GetActiveConfig)?; self.claim_interfaces(&config_descriptor); self.create_endpoints(&config_descriptor)?; @@ -337,10 +346,15 @@ impl HostDevice { .set_interface_alt_setting(interface, alt_setting) .map_err(Error::SetInterfaceAltSetting)?; self.alt_settings.insert(interface, alt_setting); + let config = self + .device + .lock() + .get_active_configuration() + .map_err(Error::GetActiveConfig)?; let config_descriptor = self .device .lock() - .get_active_config_descriptor() + .get_config_descriptor(config) .map_err(Error::GetActiveConfig)?; self.create_endpoints(&config_descriptor)?; Ok(TransferStatus::Completed) diff --git a/usb_util/src/device.rs b/usb_util/src/device.rs index aad77db..3ac1403 100644 --- a/usb_util/src/device.rs +++ b/usb_util/src/device.rs @@ -261,12 +261,8 @@ impl Device { } /// Get active config descriptor of this device. - pub fn get_active_config_descriptor(&self) -> Result { - let active_config = self.get_active_configuration()?; - match self - .device_descriptor_tree - .get_config_descriptor(active_config) - { + pub fn get_config_descriptor(&self, config: u8) -> Result { + match self.device_descriptor_tree.get_config_descriptor(config) { Some(config_descriptor) => Ok(config_descriptor.clone()), None => Err(Error::NoSuchDescriptor), } @@ -297,6 +293,11 @@ impl Device { Ok(active_config) } + /// Get the total number of configurations for this device. + pub fn get_num_configurations(&self) -> u8 { + self.device_descriptor_tree.bNumConfigurations + } + /// Clear the halt/stall condition for an endpoint. pub fn clear_halt(&self, ep_addr: u8) -> Result<()> { let endpoint: c_uint = ep_addr.into(); -- cgit 1.4.1 From 02f58e6d74bc1926424de43e9caec707f17c1b80 Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Sat, 21 Mar 2020 08:17:35 +0800 Subject: vhost-net: add control socket basic support Add a pair of control socket for vhost net. This pair can be used for the vhost-net device model to control and communicate with its activate thread. BUG=None TEST=cargo test -p devices Change-Id: I8bacdb9132787dc499ef00eea1df26ff3b35028d Signed-off-by: Chuanxiao Dong Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2046450 Reviewed-by: Stephen Barber Tested-by: kokoro --- devices/src/virtio/vhost/control_socket.rs | 36 ++++++++++++++++++++++++++++++ devices/src/virtio/vhost/mod.rs | 2 ++ devices/src/virtio/vhost/net.rs | 15 +++++++++++++ 3 files changed, 53 insertions(+) create mode 100644 devices/src/virtio/vhost/control_socket.rs (limited to 'devices/src') diff --git a/devices/src/virtio/vhost/control_socket.rs b/devices/src/virtio/vhost/control_socket.rs new file mode 100644 index 0000000..a1ccfaf --- /dev/null +++ b/devices/src/virtio/vhost/control_socket.rs @@ -0,0 +1,36 @@ +// 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 msg_socket::{MsgOnSocket, MsgSocket}; +use sys_util::Error as SysError; + +#[derive(MsgOnSocket, Debug)] +pub enum VhostDevRequest { + /// Mask or unmask all the MSI entries for a Virtio Vhost device. + MsixChanged, + /// Mask or unmask a MSI entry for a Virtio Vhost device. + MsixEntryChanged(usize), +} + +#[derive(MsgOnSocket, Debug)] +pub enum VhostDevResponse { + Ok, + Err(SysError), +} + +pub type VhostDevRequestSocket = MsgSocket; +pub type VhostDevResponseSocket = MsgSocket; + +/// Create control socket pair. This pair is used to communicate with the +/// virtio device process. +/// Mainly between the virtio and activate thread. +pub fn create_control_sockets() -> ( + Option, + Option, +) { + match msg_socket::pair::() { + Ok((request, response)) => (Some(request), Some(response)), + _ => (None, None), + } +} diff --git a/devices/src/virtio/vhost/mod.rs b/devices/src/virtio/vhost/mod.rs index 66c62d0..86ed81e 100644 --- a/devices/src/virtio/vhost/mod.rs +++ b/devices/src/virtio/vhost/mod.rs @@ -12,10 +12,12 @@ use remain::sorted; use sys_util::Error as SysError; use vhost::Error as VhostError; +mod control_socket; mod net; mod vsock; mod worker; +pub use self::control_socket::*; pub use self::net::Net; pub use self::vsock::Vsock; diff --git a/devices/src/virtio/vhost/net.rs b/devices/src/virtio/vhost/net.rs index ff72970..21f1587 100644 --- a/devices/src/virtio/vhost/net.rs +++ b/devices/src/virtio/vhost/net.rs @@ -14,6 +14,7 @@ use sys_util::{error, warn, EventFd, GuestMemory}; use vhost::NetT as VhostNetT; use virtio_sys::virtio_net; +use super::control_socket::*; use super::worker::Worker; use super::{Error, Result}; use crate::virtio::{Interrupt, Queue, VirtioDevice, TYPE_NET}; @@ -31,6 +32,8 @@ pub struct Net> { vhost_interrupt: Option>, avail_features: u64, acked_features: u64, + request_socket: Option, + response_socket: Option, } impl Net @@ -85,6 +88,8 @@ where vhost_interrupt.push(EventFd::new().map_err(Error::VhostIrqCreate)?); } + let (request_socket, response_socket) = create_control_sockets(); + Ok(Net { workers_kill_evt: Some(kill_evt.try_clone().map_err(Error::CloneKillEventFd)?), kill_evt, @@ -94,6 +99,8 @@ where vhost_interrupt: Some(vhost_interrupt), avail_features, acked_features: 0u64, + request_socket, + response_socket, }) } } @@ -143,6 +150,14 @@ where } keep_fds.push(self.kill_evt.as_raw_fd()); + if let Some(request_socket) = &self.request_socket { + keep_fds.push(request_socket.as_raw_fd()); + } + + if let Some(response_socket) = &self.response_socket { + keep_fds.push(response_socket.as_raw_fd()); + } + keep_fds } -- cgit 1.4.1 From 57b8201eeb75d9b2cbf424fa7a91d6738fe8610c Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Fri, 7 Feb 2020 10:36:25 +0800 Subject: vhost: pass response_socket to activate thread Pass the Option of the response socket should be used by the activate thread, to communicate with its device model. BUG=None TEST=cargo test -p devices Change-Id: I929f4c901468e920116f2a744ec73571d91080e3 Signed-off-by: Chuanxiao Dong Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2046451 Reviewed-by: Stephen Barber Tested-by: kokoro --- devices/src/virtio/vhost/net.rs | 7 +++++++ devices/src/virtio/vhost/vsock.rs | 1 + devices/src/virtio/vhost/worker.rs | 4 ++++ 3 files changed, 12 insertions(+) (limited to 'devices/src') diff --git a/devices/src/virtio/vhost/net.rs b/devices/src/virtio/vhost/net.rs index 21f1587..cc9521a 100644 --- a/devices/src/virtio/vhost/net.rs +++ b/devices/src/virtio/vhost/net.rs @@ -204,6 +204,11 @@ where if let Some(vhost_interrupt) = self.vhost_interrupt.take() { if let Some(kill_evt) = self.workers_kill_evt.take() { let acked_features = self.acked_features; + let socket = if self.response_socket.is_some() { + self.response_socket.take() + } else { + None + }; let worker_result = thread::Builder::new() .name("vhost_net".to_string()) .spawn(move || { @@ -214,6 +219,7 @@ where interrupt, acked_features, kill_evt, + socket, ); let activate_vqs = |handle: &U| -> Result<()> { for idx in 0..NUM_QUEUES { @@ -284,6 +290,7 @@ where self.tap = Some(tap); self.vhost_interrupt = Some(worker.vhost_interrupt); self.workers_kill_evt = Some(worker.kill_evt); + self.response_socket = worker.response_socket; return true; } } diff --git a/devices/src/virtio/vhost/vsock.rs b/devices/src/virtio/vhost/vsock.rs index 03826ff..390c857 100644 --- a/devices/src/virtio/vhost/vsock.rs +++ b/devices/src/virtio/vhost/vsock.rs @@ -169,6 +169,7 @@ impl VirtioDevice for Vsock { interrupt, acked_features, kill_evt, + None, ); let activate_vqs = |handle: &VhostVsockHandle| -> Result<()> { handle.set_cid(cid).map_err(Error::VhostVsockSetCid)?; diff --git a/devices/src/virtio/vhost/worker.rs b/devices/src/virtio/vhost/worker.rs index 1eff01f..03c1066 100644 --- a/devices/src/virtio/vhost/worker.rs +++ b/devices/src/virtio/vhost/worker.rs @@ -7,6 +7,7 @@ use std::os::raw::c_ulonglong; use sys_util::{EventFd, PollContext, PollToken}; use vhost::Vhost; +use super::control_socket::VhostDevResponseSocket; use super::{Error, Result}; use crate::virtio::{Interrupt, Queue}; @@ -21,6 +22,7 @@ pub struct Worker { pub vhost_interrupt: Vec, acked_features: u64, pub kill_evt: EventFd, + pub response_socket: Option, } impl Worker { @@ -31,6 +33,7 @@ impl Worker { interrupt: Interrupt, acked_features: u64, kill_evt: EventFd, + response_socket: Option, ) -> Worker { Worker { interrupt, @@ -39,6 +42,7 @@ impl Worker { vhost_interrupt, acked_features, kill_evt, + response_socket, } } -- cgit 1.4.1 From 45a94bedaea17d8ae95e860b8a4beb2e465c187e Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Sat, 21 Mar 2020 09:15:16 +0800 Subject: vhost-net: implement direct msix irq fd The current vhost-net msix irq injection flow is from vhost-kernel to crosvm vhost-net, then to the KVM for irq injection. It still need crosvm vhost-net to trigger irq, which is because the set_vring_call is not directly using the msix irq fd. To optimize this flow to be from vhost-kernel to KVM directly, need: 1. if the msix is enabled and unmasked, use the misx irq fd for the vring_call directly so that all the misx injection can directly to KVM from vhost-kernel. 2. if the msix is disabled or masked, use the indirect vhost_interrupt fd to let the crosvm to control the irq injection. BUG=None TEST=cargo test -p devices TEST=start crosvm with vhost-net, and run the iperf3 on the network without any issue Change-Id: Idb3427f69f23b728305ed63d88973156a03e7c6b Signed-off-by: Chuanxiao Dong Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2046452 Reviewed-by: Stephen Barber Tested-by: kokoro --- devices/src/pci/mod.rs | 2 +- devices/src/pci/msix.rs | 42 ++++++++-- devices/src/virtio/interrupt.rs | 2 +- devices/src/virtio/vhost/net.rs | 44 +++++++++++ devices/src/virtio/vhost/worker.rs | 133 +++++++++++++++++++++++++++++--- devices/src/virtio/virtio_device.rs | 4 +- devices/src/virtio/virtio_pci_device.rs | 7 +- 7 files changed, 212 insertions(+), 22 deletions(-) (limited to 'devices/src') diff --git a/devices/src/pci/mod.rs b/devices/src/pci/mod.rs index 8c5380e..b9ebc19 100644 --- a/devices/src/pci/mod.rs +++ b/devices/src/pci/mod.rs @@ -15,7 +15,7 @@ mod pci_root; mod vfio_pci; pub use self::ac97::{Ac97Backend, Ac97Dev, Ac97Parameters}; -pub use self::msix::{MsixCap, MsixConfig}; +pub use self::msix::{MsixCap, MsixConfig, MsixStatus}; pub use self::pci_configuration::{ PciBarConfiguration, PciBarPrefetchable, PciBarRegionType, PciCapability, PciCapabilityID, PciClassCode, PciConfiguration, PciHeaderType, PciProgrammingInterface, PciSerialBusSubClass, diff --git a/devices/src/pci/msix.rs b/devices/src/pci/msix.rs index 6c79b4a..b69114b 100644 --- a/devices/src/pci/msix.rs +++ b/devices/src/pci/msix.rs @@ -89,6 +89,12 @@ impl Display for MsixError { type MsixResult = std::result::Result; +pub enum MsixStatus { + Changed, + EntryChanged(usize), + NothingToDo, +} + impl MsixConfig { pub fn new(msix_vectors: u16, vm_socket: Arc) -> Self { assert!(msix_vectors <= MAX_MSIX_VECTORS_PER_DEVICE); @@ -123,6 +129,18 @@ impl MsixConfig { self.masked } + /// Check whether the Function Mask bit in MSIX table Message Control + /// word in set or not. + /// If true, the vector is masked. + /// If false, the vector is unmasked. + pub fn table_masked(&self, index: usize) -> bool { + if index >= self.table_entries.len() { + true + } else { + self.table_entries[index].masked() + } + } + /// Check whether the MSI-X Enable bit in Message Control word in set or not. /// if 1, the function is permitted to use MSI-X to request service. pub fn enabled(&self) -> bool { @@ -147,7 +165,7 @@ impl MsixConfig { /// Write to the MSI-X Capability Structure. /// Only the top 2 bits in Message Control Word are writable. - pub fn write_msix_capability(&mut self, offset: u64, data: &[u8]) { + pub fn write_msix_capability(&mut self, offset: u64, data: &[u8]) -> MsixStatus { if offset == 2 && data.len() == 2 { let reg = u16::from_le_bytes([data[0], data[1]]); let old_masked = self.masked; @@ -173,6 +191,9 @@ impl MsixConfig { self.inject_msix_and_clear_pba(index); } } + return MsixStatus::Changed; + } else if !old_masked && self.masked { + return MsixStatus::Changed; } } else { error!( @@ -180,6 +201,7 @@ impl MsixConfig { offset ); } + MsixStatus::NothingToDo } fn add_msi_route(&self, index: u16, gsi: u32) -> MsixResult<()> { @@ -304,7 +326,7 @@ impl MsixConfig { /// Vector Control: only bit 0 (Mask Bit) is not reserved: when this bit /// is set, the function is prohibited from sending a message using /// this MSI-X Table entry. - pub fn write_msix_table(&mut self, offset: u64, data: &[u8]) { + pub fn write_msix_table(&mut self, offset: u64, data: &[u8]) -> MsixStatus { let index: usize = (offset / MSIX_TABLE_ENTRIES_MODULO) as usize; let modulo_offset = offset % MSIX_TABLE_ENTRIES_MODULO; @@ -360,13 +382,17 @@ impl MsixConfig { // device. // Check if bit has been flipped - if !self.masked() - && old_entry.masked() - && !self.table_entries[index].masked() - && self.get_pba_bit(index as u16) == 1 - { - self.inject_msix_and_clear_pba(index); + if !self.masked() { + if old_entry.masked() && !self.table_entries[index].masked() { + if self.get_pba_bit(index as u16) == 1 { + self.inject_msix_and_clear_pba(index); + } + return MsixStatus::EntryChanged(index); + } else if !old_entry.masked() && self.table_entries[index].masked() { + return MsixStatus::EntryChanged(index); + } } + MsixStatus::NothingToDo } /// Read PBA Entries diff --git a/devices/src/virtio/interrupt.rs b/devices/src/virtio/interrupt.rs index f808d84..91a3942 100644 --- a/devices/src/virtio/interrupt.rs +++ b/devices/src/virtio/interrupt.rs @@ -13,7 +13,7 @@ pub struct Interrupt { interrupt_status: Arc, interrupt_evt: EventFd, interrupt_resample_evt: EventFd, - msix_config: Option>>, + pub msix_config: Option>>, config_msix_vector: u16, } diff --git a/devices/src/virtio/vhost/net.rs b/devices/src/virtio/vhost/net.rs index cc9521a..542423a 100644 --- a/devices/src/virtio/vhost/net.rs +++ b/devices/src/virtio/vhost/net.rs @@ -17,7 +17,9 @@ use virtio_sys::virtio_net; use super::control_socket::*; use super::worker::Worker; use super::{Error, Result}; +use crate::pci::MsixStatus; use crate::virtio::{Interrupt, Queue, VirtioDevice, TYPE_NET}; +use msg_socket::{MsgReceiver, MsgSender}; const QUEUE_SIZE: u16 = 256; const NUM_QUEUES: usize = 2; @@ -272,6 +274,48 @@ where } } + fn control_notify(&self, behavior: MsixStatus) { + if self.worker_thread.is_none() || self.request_socket.is_none() { + return; + } + if let Some(socket) = &self.request_socket { + match behavior { + MsixStatus::EntryChanged(index) => { + if let Err(e) = socket.send(&VhostDevRequest::MsixEntryChanged(index)) { + error!( + "{} failed to send VhostMsixEntryChanged request for entry {}: {:?}", + self.debug_label(), + index, + e + ); + return; + } + if let Err(e) = socket.recv() { + error!("{} failed to receive VhostMsixEntryChanged response for entry {}: {:?}", self.debug_label(), index, e); + } + } + MsixStatus::Changed => { + if let Err(e) = socket.send(&VhostDevRequest::MsixChanged) { + error!( + "{} failed to send VhostMsixChanged request: {:?}", + self.debug_label(), + e + ); + return; + } + if let Err(e) = socket.recv() { + error!( + "{} failed to receive VhostMsixChanged response {:?}", + self.debug_label(), + e + ); + } + } + _ => {} + } + } + } + fn reset(&mut self) -> bool { // Only kill the child if it claimed its eventfd. if self.workers_kill_evt.is_none() && self.kill_evt.write(1).is_err() { diff --git a/devices/src/virtio/vhost/worker.rs b/devices/src/virtio/vhost/worker.rs index 03c1066..ca02a63 100644 --- a/devices/src/virtio/vhost/worker.rs +++ b/devices/src/virtio/vhost/worker.rs @@ -4,17 +4,16 @@ use std::os::raw::c_ulonglong; -use sys_util::{EventFd, PollContext, PollToken}; +use sys_util::{error, Error as SysError, EventFd, PollContext, PollToken}; use vhost::Vhost; -use super::control_socket::VhostDevResponseSocket; +use super::control_socket::{VhostDevRequest, VhostDevResponse, VhostDevResponseSocket}; use super::{Error, Result}; use crate::virtio::{Interrupt, Queue}; +use libc::EIO; +use msg_socket::{MsgReceiver, MsgSender}; -/// Worker that takes care of running the vhost device. This mainly involves forwarding interrupts -/// from the vhost driver to the guest VM because crosvm only supports the virtio-mmio transport, -/// which requires a bit to be set in the interrupt status register before triggering the interrupt -/// and the vhost driver doesn't do this for us. +/// Worker that takes care of running the vhost device. pub struct Worker { interrupt: Interrupt, queues: Vec, @@ -91,9 +90,7 @@ impl Worker { self.vhost_handle .set_vring_base(queue_index, 0) .map_err(Error::VhostSetVringBase)?; - self.vhost_handle - .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) - .map_err(Error::VhostSetVringCall)?; + self.set_vring_call_for_entry(queue_index, queue.vector as usize)?; self.vhost_handle .set_vring_kick(queue_index, &queue_evts[queue_index]) .map_err(Error::VhostSetVringKick)?; @@ -106,6 +103,7 @@ impl Worker { VhostIrqi { index: usize }, InterruptResample, Kill, + ControlNotify, } let poll_ctx: PollContext = PollContext::build_with(&[ @@ -119,6 +117,11 @@ impl Worker { .add(vhost_int, Token::VhostIrqi { index }) .map_err(Error::CreatePollContext)?; } + if let Some(socket) = &self.response_socket { + poll_ctx + .add(socket, Token::ControlNotify) + .map_err(Error::CreatePollContext)?; + } 'poll: loop { let events = poll_ctx.wait().map_err(Error::PollError)?; @@ -138,10 +141,122 @@ impl Worker { let _ = self.kill_evt.read(); break 'poll; } + Token::ControlNotify => { + if let Some(socket) = &self.response_socket { + match socket.recv() { + Ok(VhostDevRequest::MsixEntryChanged(index)) => { + let mut qindex = 0; + for (queue_index, queue) in self.queues.iter().enumerate() { + if queue.vector == index as u16 { + qindex = queue_index; + break; + } + } + let response = + match self.set_vring_call_for_entry(qindex, index) { + Ok(()) => VhostDevResponse::Ok, + Err(e) => { + error!( + "Set vring call failed for masked entry {}: {:?}", + index, e + ); + VhostDevResponse::Err(SysError::new(EIO)) + } + }; + if let Err(e) = socket.send(&response) { + error!("Vhost failed to send VhostMsixEntryMasked Response for entry {}: {:?}", index, e); + } + } + Ok(VhostDevRequest::MsixChanged) => { + let response = match self.set_vring_calls() { + Ok(()) => VhostDevResponse::Ok, + Err(e) => { + error!("Set vring calls failed: {:?}", e); + VhostDevResponse::Err(SysError::new(EIO)) + } + }; + if let Err(e) = socket.send(&response) { + error!( + "Vhost failed to send VhostMsixMasked Response: {:?}", + e + ); + } + } + Err(e) => { + error!("Vhost failed to receive Control request: {:?}", e); + } + } + } + } } } } cleanup_vqs(&self.vhost_handle)?; Ok(()) } + + fn set_vring_call_for_entry(&self, queue_index: usize, vector: usize) -> Result<()> { + // No response_socket means it doesn't have any control related + // with the msix. Due to this, cannot use the direct irq fd but + // should fall back to indirect irq fd. + if self.response_socket.is_some() { + if let Some(msix_config) = &self.interrupt.msix_config { + let msix_config = msix_config.lock(); + let msix_masked = msix_config.masked(); + if msix_masked { + return Ok(()); + } + if !msix_config.table_masked(vector) { + if let Some(irqfd) = msix_config.get_irqfd(vector) { + self.vhost_handle + .set_vring_call(queue_index, irqfd) + .map_err(Error::VhostSetVringCall)?; + } else { + self.vhost_handle + .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) + .map_err(Error::VhostSetVringCall)?; + } + return Ok(()); + } + } + } + + self.vhost_handle + .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) + .map_err(Error::VhostSetVringCall)?; + Ok(()) + } + + fn set_vring_calls(&self) -> Result<()> { + if let Some(msix_config) = &self.interrupt.msix_config { + let msix_config = msix_config.lock(); + if msix_config.masked() { + for (queue_index, _) in self.queues.iter().enumerate() { + self.vhost_handle + .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) + .map_err(Error::VhostSetVringCall)?; + } + } else { + for (queue_index, queue) in self.queues.iter().enumerate() { + let vector = queue.vector as usize; + if !msix_config.table_masked(vector) { + if let Some(irqfd) = msix_config.get_irqfd(vector) { + self.vhost_handle + .set_vring_call(queue_index, irqfd) + .map_err(Error::VhostSetVringCall)?; + } else { + self.vhost_handle + .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) + .map_err(Error::VhostSetVringCall)?; + } + } else { + self.vhost_handle + .set_vring_call(queue_index, &self.vhost_interrupt[queue_index]) + .map_err(Error::VhostSetVringCall)?; + } + } + } + } + Ok(()) + } } diff --git a/devices/src/virtio/virtio_device.rs b/devices/src/virtio/virtio_device.rs index 806a98f..6eb5548 100644 --- a/devices/src/virtio/virtio_device.rs +++ b/devices/src/virtio/virtio_device.rs @@ -7,7 +7,7 @@ use std::os::unix::io::RawFd; use sys_util::{EventFd, GuestMemory}; use super::*; -use crate::pci::{PciBarConfiguration, PciCapability}; +use crate::pci::{MsixStatus, PciBarConfiguration, PciCapability}; /// Trait for virtio devices to be driven by a virtio transport. /// @@ -86,4 +86,6 @@ pub trait VirtioDevice: Send { /// Invoked when the device is sandboxed. fn on_device_sandboxed(&mut self) {} + + fn control_notify(&self, _behavior: MsixStatus) {} } diff --git a/devices/src/virtio/virtio_pci_device.rs b/devices/src/virtio/virtio_pci_device.rs index c6d6786..e63abe9 100644 --- a/devices/src/virtio/virtio_pci_device.rs +++ b/devices/src/virtio/virtio_pci_device.rs @@ -535,7 +535,8 @@ impl PciDevice for VirtioPciDevice { fn write_config_register(&mut self, reg_idx: usize, offset: u64, data: &[u8]) { if let Some(msix_cap_reg_idx) = self.msix_cap_reg_idx { if msix_cap_reg_idx == reg_idx { - self.msix_config.lock().write_msix_capability(offset, data); + let behavior = self.msix_config.lock().write_msix_capability(offset, data); + self.device.control_notify(behavior); } } @@ -626,9 +627,11 @@ impl PciDevice for VirtioPciDevice { // Handled with ioeventfds. } o if MSIX_TABLE_BAR_OFFSET <= o && o < MSIX_TABLE_BAR_OFFSET + MSIX_TABLE_SIZE => { - self.msix_config + let behavior = self + .msix_config .lock() .write_msix_table(o - MSIX_TABLE_BAR_OFFSET, data); + self.device.control_notify(behavior); } o if MSIX_PBA_BAR_OFFSET <= o && o < MSIX_PBA_BAR_OFFSET + MSIX_PBA_SIZE => { self.msix_config -- cgit 1.4.1 From 22964eab8874d41cf0eadf03dfeb1ffb653283e5 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Fri, 13 Mar 2020 09:06:50 -0700 Subject: gfxstream: fix build API for export_resource changed Bug: b/146066070 Change-Id: I614880704658bbe7aae2f7ad8b10c76555d99c1f Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2102760 Tested-by: Lingfeng Yang Tested-by: kokoro Commit-Queue: Lingfeng Yang Reviewed-by: Jason Macnak Reviewed-by: Gurchetan Singh Reviewed-by: Zach Reizner --- devices/src/virtio/gpu/virtio_gfxstream_backend.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'devices/src') diff --git a/devices/src/virtio/gpu/virtio_gfxstream_backend.rs b/devices/src/virtio/gpu/virtio_gfxstream_backend.rs index aa02e15..2a49da8 100644 --- a/devices/src/virtio/gpu/virtio_gfxstream_backend.rs +++ b/devices/src/virtio/gpu/virtio_gfxstream_backend.rs @@ -10,7 +10,6 @@ use std::cell::RefCell; use std::collections::btree_map::Entry; use std::collections::BTreeMap as Map; -use std::fs::File; use std::mem::transmute; use std::os::raw::{c_char, c_int, c_uchar, c_uint, c_void}; use std::panic; @@ -27,6 +26,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::resource_bridge::ResourceResponse; // C definitions related to gfxstream // In gfxstream, only write_fence is used @@ -340,8 +340,8 @@ impl Backend for VirtioGfxStreamBackend { } /// If supported, export the resource with the given id to a file. - fn export_resource(&mut self, _id: u32) -> Option { - None + fn export_resource(&mut self, _id: u32) -> ResourceResponse { + ResourceResponse::Invalid } /// Creates a fence with the given id that can be used to determine when the previous command -- cgit 1.4.1