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(-) 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 a0fea29e0bcd6d9b39e1ea8f5abf0655f8ac3c0a Mon Sep 17 00:00:00 2001 From: David Stevens Date: Fri, 6 Mar 2020 14:32:33 +0900 Subject: gpu_renderer: add virglrenderer logging callback BUG=None TEST=boot ARCVM Change-Id: Iea6c5ecc2475ec6f78df100b5df62622c41bd88a Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2090956 Tested-by: kokoro Reviewed-by: Chirantan Ekbote Commit-Queue: David Stevens --- gpu_renderer/src/lib.rs | 28 +++++++++++++++++++++++++++- gpu_renderer/src/vsnprintf.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 gpu_renderer/src/vsnprintf.rs diff --git a/gpu_renderer/src/lib.rs b/gpu_renderer/src/lib.rs index be8c17a..1c8461c 100644 --- a/gpu_renderer/src/lib.rs +++ b/gpu_renderer/src/lib.rs @@ -6,8 +6,10 @@ mod command_buffer; mod generated; +mod vsnprintf; use std::cell::RefCell; +use std::ffi::CString; use std::fmt::{self, Display}; use std::fs::File; use std::marker::PhantomData; @@ -22,7 +24,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use libc::close; use data_model::{VolatileMemory, VolatileSlice}; -use sys_util::{GuestAddress, GuestMemory}; +use sys_util::{debug, GuestAddress, GuestMemory}; use crate::generated::p_defines::{ PIPE_BIND_RENDER_TARGET, PIPE_BIND_SAMPLER_VIEW, PIPE_TEXTURE_1D, PIPE_TEXTURE_2D, @@ -31,6 +33,7 @@ use crate::generated::p_format::PIPE_FORMAT_B8G8R8X8_UNORM; use crate::generated::virglrenderer::*; pub use crate::command_buffer::CommandBufferBuilder; +pub use crate::vsnprintf::vsnprintf; /// Arguments used in `Renderer::create_resource`.. pub type ResourceCreateArgs = virgl_renderer_resource_create_args; @@ -245,6 +248,8 @@ impl Renderer { fence_state: Rc::clone(&fence_state), })); + unsafe { virgl_set_debug_callback(Some(Renderer::debug_callback)) }; + // Safe because a valid cookie and set of callbacks is used and the result is checked for // error. let ret = unsafe { @@ -468,6 +473,27 @@ impl Renderer { #[cfg(not(feature = "virtio-gpu-next"))] Err(Error::Unsupported) } + + extern "C" fn debug_callback( + fmt: *const ::std::os::raw::c_char, + ap: *mut generated::virglrenderer::__va_list_tag, + ) { + let len: u32 = 256; + let mut c_str = CString::new(vec![' ' as u8; len as usize]).unwrap(); + unsafe { + let mut varargs = vsnprintf::__va_list_tag { + gp_offset: (*ap).gp_offset, + fp_offset: (*ap).fp_offset, + overflow_arg_area: (*ap).overflow_arg_area, + reg_save_area: (*ap).reg_save_area, + }; + + let raw = c_str.into_raw(); + vsnprintf(raw, len.into(), fmt, &mut varargs); + c_str = CString::from_raw(raw); + } + debug!("{}", c_str.to_string_lossy()); + } } /// A context in which resources can be attached/detached and commands can be submitted. diff --git a/gpu_renderer/src/vsnprintf.rs b/gpu_renderer/src/vsnprintf.rs new file mode 100644 index 0000000..72583c5 --- /dev/null +++ b/gpu_renderer/src/vsnprintf.rs @@ -0,0 +1,30 @@ +// 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. + +#![allow(dead_code, non_snake_case, non_camel_case_types)] + +/* + * automatically generated by rust-bindgen + * $ bindgen /usr/include/stdio.h \ + * --no-layout-tests \ + * --whitelist-function vsnprintf \ + * -o vsnprintf.rs + */ + +extern "C" { + pub fn vsnprintf( + __s: *mut ::std::os::raw::c_char, + __maxlen: ::std::os::raw::c_ulong, + __format: *const ::std::os::raw::c_char, + __arg: *mut __va_list_tag, + ) -> ::std::os::raw::c_int; +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct __va_list_tag { + pub gp_offset: ::std::os::raw::c_uint, + pub fp_offset: ::std::os::raw::c_uint, + pub overflow_arg_area: *mut ::std::os::raw::c_void, + pub reg_save_area: *mut ::std::os::raw::c_void, +} -- cgit 1.4.1 From 63c239496da61c1fbdc1c2c866f5719534e22503 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Fri, 6 Mar 2020 16:32:20 -0800 Subject: Fix into_iter() usage where iter() suffices Fixes warnings of the form: warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added. BUG=None TEST=emerge-nami crosvm Change-Id: I2b46b55f0e967d985d04678c240604b542e27e07 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2093287 Reviewed-by: Dylan Reid Tested-by: kokoro Commit-Queue: Daniel Verkamp --- gpu_display/src/keycode_converter/mod.rs | 2 +- src/linux.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gpu_display/src/keycode_converter/mod.rs b/gpu_display/src/keycode_converter/mod.rs index 40fdb95..236e65d 100644 --- a/gpu_display/src/keycode_converter/mod.rs +++ b/gpu_display/src/keycode_converter/mod.rs @@ -23,7 +23,7 @@ impl KeycodeTranslator { /// Create a new KeycodeTranslator that translates from the `from_type` type to Linux keycodes. pub fn new(from_type: KeycodeTypes) -> KeycodeTranslator { let mut kcm: HashMap = HashMap::new(); - for entry in KEYCODE_MAP.into_iter() { + for entry in KEYCODE_MAP.iter() { kcm.insert( match from_type { KeycodeTypes::XkbScancode => entry.xkb, diff --git a/src/linux.rs b/src/linux.rs index 2de1aaa..d63ab32 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -1751,7 +1751,7 @@ fn run_control( .map_err(Error::PollContextAdd)?; if let Some(gsi_relay) = &linux.gsi_relay { - for (gsi, evt) in gsi_relay.irqfd.into_iter().enumerate() { + for (gsi, evt) in gsi_relay.irqfd.iter().enumerate() { if let Some(evt) = evt { poll_ctx .add(evt, Token::IrqFd { gsi }) -- cgit 1.4.1 From d9082cff491433caa9f73dc412156b73b820ce88 Mon Sep 17 00:00:00 2001 From: Stephen Barber Date: Fri, 14 Feb 2020 13:38:03 -0800 Subject: README: add instructions for building on Linux + CrOS We have instructions for building with Docker, but it's now possible to build for normal Linux distros too. Also add a pointer to the main CrOS developer guide. BUG=none TEST=follow the instructions Change-Id: Ic7ce498268f8057fbe90a88166017f54108d0e16 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2057747 Reviewed-by: Dylan Reid Tested-by: kokoro Tested-by: Stephen Barber Commit-Queue: Stephen Barber --- README.md | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 51c200d..f2d72aa 100644 --- a/README.md +++ b/README.md @@ -11,11 +11,61 @@ an exploit in the devices. The channel #crosvm on [freenode](https://webchat.freenode.net/#crosvm) is used for technical discussion related to crosvm development and integration. -## Building with Docker +## Getting started + +### Building for CrOS + +crosvm on Chromium OS is built with Portage, so it follows the same general +workflow as any `cros_workon` package. The full package name is +`chromeos-base/crosvm`. + +See the [Chromium OS developer guide] for more on how to build and deploy with +Portage. + +[Chromium OS developer guide]: https://chromium.googlesource.com/chromiumos/docs/+/master/developer_guide.md + +### Building with Docker See the [README](docker/README.md) from the `docker` subdirectory to learn how to build crosvm in enviroments outside of the Chrome OS chroot. +### Building for Linux + +>**NOTE:** Building for Linux natively is new and not fully supported. + +First, [set up depot_tools] and use `repo` to sync down the crosvm source +tree. This is a subset of the entire Chromium OS manifest with just enough repos +to build crosvm. + +```sh +mkdir crosvm +cd crosvm +repo init -g crosvm -u https://chromium.googlesource.com/chromiumos/manifest.git --repo-url=https://chromium.googlesource.com/external/repo.git +repo sync +``` + +A basic crosvm build links against `libcap` and `libfdt`. On a Debian-based system, +you can install `libcap-dev` and `libfdt-dev`. + +Handy Debian one-liner for all build and runtime deps, particularly if you're +running Crostini: +```sh +sudo apt install build-essential libcap-dev libfdt-dev pkg-config python +``` + +Known issues: +* Seccomp policy files have hardcoded absolute paths. You can either fix up + the paths locally, or set up an awesome hacky symlink: `sudo mkdir + /usr/share/policy && sudo ln -s /path/to/crosvm/seccomp/x86_64 + /usr/share/policy/crosvm`. We'll eventually build the precompiled + policies [into the crosvm binary](http://crbug.com/1052126). +* Devices can't be jailed if `/var/empty` doesn't exist. `sudo mkdir -p + /var/empty` to work around this for now. + +And that's it! You should be able to `cargo build/run/test`. + +[set up depot_tools]: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html#_setting_up + ## Usage To see the usage information for your version of crosvm, run `crosvm` or `crosvm -- 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(-) 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(-) 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(-) 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 5934305f37a765f8c611d9a24d9d5f012845d39b Mon Sep 17 00:00:00 2001 From: Judy Hsiao Date: Mon, 16 Mar 2020 15:58:03 +0800 Subject: crosvm: Change expected field to String in InvalidValue Change the type of argument::Error::InvalidValue::expected from "&'static str" to String. It allows the lower level parse error object to handle the output of the expected value so that the rule of parsing will not be duplicated. For example, instead of: ``` v.parse::() .map_err(|e| argument::Error::InvalidValue { value: v.to_string(), expected: "The value of setting should null or cras", })?; ``` we can have: ``` v.parse::() .map_err(|e| argument::Error::InvalidValue { value: v.to_string(), expected: e.to_string(), })?; ``` and the expected value can be handled in the Display implementaion of Settings::ParseError. BUG=b:140866281 TEST=cargo build Change-Id: Ieba12a21103945fe0e47c70a190a4e5d985af537 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2103605 Tested-by: Judy Hsiao Tested-by: kokoro Reviewed-by: Chih-Yang Hsia Reviewed-by: Zach Reizner Auto-Submit: Judy Hsiao Commit-Queue: Judy Hsiao --- src/argument.rs | 11 ++--- src/main.rs | 147 ++++++++++++++++++++++++++++++++------------------------ 2 files changed, 88 insertions(+), 70 deletions(-) diff --git a/src/argument.rs b/src/argument.rs index f59d503..9c00ae5 100644 --- a/src/argument.rs +++ b/src/argument.rs @@ -25,7 +25,7 @@ //! let v: u32 = value.unwrap().parse().map_err(|_| { //! Error::InvalidValue { //! value: value.unwrap().to_owned(), -//! expected: "this value for `cpus` needs to be integer", +//! expected: String::from("this value for `cpus` needs to be integer"), //! } //! })?; //! } @@ -56,10 +56,7 @@ pub enum Error { /// The argument was required. ExpectedArgument(String), /// The argument's given value is invalid. - InvalidValue { - value: String, - expected: &'static str, - }, + InvalidValue { value: String, expected: String }, /// The argument was already given and none more are expected. TooManyArguments(String), /// The argument expects a value. @@ -447,7 +444,7 @@ mod tests { "cpus" => { let c: u32 = value.unwrap().parse().map_err(|_| Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this value for `cpus` needs to be integer", + expected: String::from("this value for `cpus` needs to be integer"), })?; assert_eq!(c, 3); } @@ -521,7 +518,7 @@ mod tests { _ => { return Err(Error::InvalidValue { value: v.to_string(), - expected: "2D or 3D", + expected: String::from("2D or 3D"), }) } } diff --git a/src/main.rs b/src/main.rs index da7c06c..a068057 100644 --- a/src/main.rs +++ b/src/main.rs @@ -78,21 +78,21 @@ fn parse_cpu_set(s: &str) -> argument::Result> { if range.len() == 0 || range.len() > 2 { return Err(argument::Error::InvalidValue { value: part.to_owned(), - expected: "invalid list syntax", + expected: String::from("invalid list syntax"), }); } let first_cpu: usize = range[0] .parse() .map_err(|_| argument::Error::InvalidValue { value: part.to_owned(), - expected: "CPU index must be a non-negative integer", + expected: String::from("CPU index must be a non-negative integer"), })?; let last_cpu: usize = if range.len() == 2 { range[1] .parse() .map_err(|_| argument::Error::InvalidValue { value: part.to_owned(), - expected: "CPU index must be a non-negative integer", + expected: String::from("CPU index must be a non-negative integer"), })? } else { first_cpu @@ -101,7 +101,7 @@ fn parse_cpu_set(s: &str) -> argument::Result> { if last_cpu < first_cpu { return Err(argument::Error::InvalidValue { value: part.to_owned(), - expected: "CPU ranges must be from low to high", + expected: String::from("CPU ranges must be from low to high"), }); } @@ -151,7 +151,9 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { _ => { return Err(argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'backend' should be one of (2d|3d|gfxstream)", + expected: String::from( + "gpu parameter 'backend' should be one of (2d|3d|gfxstream)", + ), }); } }, @@ -165,7 +167,7 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { _ => { return Err(argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'egl' should be a boolean", + expected: String::from("gpu parameter 'egl' should be a boolean"), }); } }, @@ -179,7 +181,7 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { _ => { return Err(argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'gles' should be a boolean", + expected: String::from("gpu parameter 'gles' should be a boolean"), }); } }, @@ -193,7 +195,7 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { _ => { return Err(argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'glx' should be a boolean", + expected: String::from("gpu parameter 'glx' should be a boolean"), }); } }, @@ -207,7 +209,9 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { _ => { return Err(argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'surfaceless' should be a boolean", + expected: String::from( + "gpu parameter 'surfaceless' should be a boolean", + ), }); } }, @@ -216,7 +220,9 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { v.parse::() .map_err(|_| argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'width' must be a valid integer", + expected: String::from( + "gpu parameter 'width' must be a valid integer", + ), })?; } "height" => { @@ -224,7 +230,9 @@ fn parse_gpu_options(s: Option<&str>) -> argument::Result { v.parse::() .map_err(|_| argument::Error::InvalidValue { value: v.to_string(), - expected: "gpu parameter 'height' must be a valid integer", + expected: String::from( + "gpu parameter 'height' must be a valid integer", + ), })?; } "" => {} @@ -269,7 +277,7 @@ fn parse_serial_options(s: &str) -> argument::Result { if num < 1 || num > 4 { return Err(argument::Error::InvalidValue { value: num.to_string(), - expected: "Serial port num must be between 1 - 4", + expected: String::from("Serial port num must be between 1 - 4"), }); } serial_setting.num = num; @@ -305,7 +313,9 @@ fn parse_plugin_mount_option(value: &str) -> argument::Result { if components.is_empty() || components.len() > 3 || components[0].is_empty() { return Err(argument::Error::InvalidValue { value: value.to_owned(), - expected: "`plugin-mount` should be in a form of: [:[][:]]", + expected: String::from( + "`plugin-mount` should be in a form of: [:[][:]]", + ), }); } @@ -313,13 +323,13 @@ fn parse_plugin_mount_option(value: &str) -> argument::Result { if src.is_relative() { return Err(argument::Error::InvalidValue { value: components[0].to_owned(), - expected: "the source path for `plugin-mount` must be absolute", + expected: String::from("the source path for `plugin-mount` must be absolute"), }); } if !src.exists() { return Err(argument::Error::InvalidValue { value: components[0].to_owned(), - expected: "the source path for `plugin-mount` does not exist", + expected: String::from("the source path for `plugin-mount` does not exist"), }); } @@ -330,7 +340,7 @@ fn parse_plugin_mount_option(value: &str) -> argument::Result { if dst.is_relative() { return Err(argument::Error::InvalidValue { value: components[1].to_owned(), - expected: "the destination path for `plugin-mount` must be absolute", + expected: String::from("the destination path for `plugin-mount` must be absolute"), }); } @@ -338,7 +348,7 @@ fn parse_plugin_mount_option(value: &str) -> argument::Result { None => false, Some(s) => s.parse().map_err(|_| argument::Error::InvalidValue { value: components[2].to_owned(), - expected: "the component for `plugin-mount` is not valid bool", + expected: String::from("the component for `plugin-mount` is not valid bool"), })?, }; @@ -350,8 +360,9 @@ fn parse_plugin_gid_map_option(value: &str) -> argument::Result { if components.is_empty() || components.len() > 3 || components[0].is_empty() { return Err(argument::Error::InvalidValue { value: value.to_owned(), - expected: + expected: String::from( "`plugin-gid-map` must have exactly 3 components: [:[][:]]", + ), }); } @@ -359,14 +370,14 @@ fn parse_plugin_gid_map_option(value: &str) -> argument::Result { .parse() .map_err(|_| argument::Error::InvalidValue { value: components[0].to_owned(), - expected: "the component for `plugin-gid-map` is not valid gid", + expected: String::from("the component for `plugin-gid-map` is not valid gid"), })?; let outer: libc::gid_t = match components.get(1) { None | Some(&"") => inner, Some(s) => s.parse().map_err(|_| argument::Error::InvalidValue { value: components[1].to_owned(), - expected: "the component for `plugin-gid-map` is not valid gid", + expected: String::from("the component for `plugin-gid-map` is not valid gid"), })?, }; @@ -374,7 +385,9 @@ fn parse_plugin_gid_map_option(value: &str) -> argument::Result { None => 1, Some(s) => s.parse().map_err(|_| argument::Error::InvalidValue { value: components[2].to_owned(), - expected: "the component for `plugin-gid-map` is not valid number", + expected: String::from( + "the component for `plugin-gid-map` is not valid number", + ), })?, }; @@ -398,7 +411,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if !kernel_path.exists() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this kernel path does not exist", + expected: String::from("this kernel path does not exist"), }); } cfg.executable_path = Some(Executable::Kernel(kernel_path)); @@ -415,7 +428,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if !android_fstab.exists() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this android fstab path does not exist", + expected: String::from("this android fstab path does not exist"), }); } cfg.android_fstab = Some(android_fstab); @@ -437,7 +450,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this value for `cpus` needs to be integer", + expected: String::from("this value for `cpus` needs to be integer"), })?, ) } @@ -462,7 +475,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this value for `mem` needs to be integer", + expected: String::from("this value for `mem` needs to be integer"), })?, ) } @@ -526,13 +539,13 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .next() .ok_or_else(|| argument::Error::InvalidValue { value: param.to_owned(), - expected: "missing disk path", + expected: String::from("missing disk path"), })?, ); if !disk_path.exists() { return Err(argument::Error::InvalidValue { value: param.to_owned(), - expected: "this disk path does not exist", + expected: String::from("this disk path does not exist"), }); } if name.ends_with("root") { @@ -559,18 +572,18 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: let mut o = opt.splitn(2, '='); let kind = o.next().ok_or_else(|| argument::Error::InvalidValue { value: opt.to_owned(), - expected: "disk options must not be empty", + expected: String::from("disk options must not be empty"), })?; let value = o.next().ok_or_else(|| argument::Error::InvalidValue { value: opt.to_owned(), - expected: "disk options must be of the form `kind=value`", + expected: String::from("disk options must be of the form `kind=value`"), })?; match kind { "sparse" => { let sparse = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`sparse` must be a boolean", + expected: String::from("`sparse` must be a boolean"), })?; disk.sparse = sparse; } @@ -578,14 +591,14 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: let block_size = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`block_size` must be an integer", + expected: String::from("`block_size` must be an integer"), })?; disk.block_size = block_size; } _ => { return Err(argument::Error::InvalidValue { value: kind.to_owned(), - expected: "unrecognized disk option", + expected: String::from("unrecognized disk option"), }); } } @@ -598,7 +611,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if !disk_path.exists() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this disk path does not exist", + expected: String::from("this disk path does not exist"), }); } @@ -621,7 +634,9 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if components.len() != 2 { return Err(argument::Error::InvalidValue { value: value.to_owned(), - expected: "pstore must have exactly 2 components: path=,size=", + expected: String::from( + "pstore must have exactly 2 components: path=,size=", + ), }); } cfg.pstore = Some(Pstore { @@ -629,7 +644,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if components[0].len() <= 5 || !components[0].starts_with("path=") { return Err(argument::Error::InvalidValue { value: components[0].to_owned(), - expected: "pstore path must follow with `path=`", + expected: String::from("pstore path must follow with `path=`"), }); }; PathBuf::from(&components[0][5..]) @@ -638,14 +653,14 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if components[1].len() <= 5 || !components[1].starts_with("size=") { return Err(argument::Error::InvalidValue { value: components[1].to_owned(), - expected: "pstore size must follow with `size=`", + expected: String::from("pstore size must follow with `size=`"), }); }; components[1][5..] .parse() .map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "pstore size must be an integer", + expected: String::from("pstore size must be an integer"), })? }, }); @@ -663,7 +678,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "`host_ip` needs to be in the form \"x.x.x.x\"", + expected: String::from("`host_ip` needs to be in the form \"x.x.x.x\""), })?, ) } @@ -680,7 +695,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "`netmask` needs to be in the form \"x.x.x.x\"", + expected: String::from("`netmask` needs to be in the form \"x.x.x.x\""), })?, ) } @@ -697,7 +712,9 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "`mac` needs to be in the form \"XX:XX:XX:XX:XX:XX\"", + expected: String::from( + "`mac` needs to be in the form \"XX:XX:XX:XX:XX:XX\"", + ), })?, ) } @@ -709,7 +726,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .next() .ok_or_else(|| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "missing socket path", + expected: String::from("missing socket path"), })?, ); let mut name = ""; @@ -720,7 +737,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: _ => { return Err(argument::Error::InvalidValue { value: c.to_owned(), - expected: "option must be of the form `kind=value`", + expected: String::from("option must be of the form `kind=value`"), }) } }; @@ -729,7 +746,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: _ => { return Err(argument::Error::InvalidValue { value: kind.to_owned(), - expected: "unrecognized option", + expected: String::from("unrecognized option"), }) } } @@ -771,7 +788,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if socket_path.exists() { return Err(argument::Error::InvalidValue { value: socket_path.to_string_lossy().into_owned(), - expected: "this socket path already exists", + expected: String::from("this socket path already exists"), }); } cfg.socket_path = Some(socket_path); @@ -791,7 +808,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this value for `cid` must be an unsigned integer", + expected: String::from("this value for `cid` must be an unsigned integer"), })?, ); } @@ -816,21 +833,21 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .next() .ok_or_else(|| argument::Error::InvalidValue { value: param.to_owned(), - expected: "missing source path for `shared-dir`", + expected: String::from("missing source path for `shared-dir`"), })?, ); let tag = components .next() .ok_or_else(|| argument::Error::InvalidValue { value: param.to_owned(), - expected: "missing tag for `shared-dir`", + expected: String::from("missing tag for `shared-dir`"), })? .to_owned(); if !src.is_dir() { return Err(argument::Error::InvalidValue { value: param.to_owned(), - expected: "source path for `shared-dir` must be a directory", + expected: String::from("source path for `shared-dir` must be a directory"), }); } @@ -843,11 +860,11 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: let mut o = opt.splitn(2, '='); let kind = o.next().ok_or_else(|| argument::Error::InvalidValue { value: opt.to_owned(), - expected: "`shared-dir` options must not be empty", + expected: String::from("`shared-dir` options must not be empty"), })?; let value = o.next().ok_or_else(|| argument::Error::InvalidValue { value: opt.to_owned(), - expected: "`shared-dir` options must be of the form `kind=value`", + expected: String::from("`shared-dir` options must be of the form `kind=value`"), })?; match kind { @@ -855,7 +872,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: shared_dir.kind = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`type` must be one of `fs` or `9p`", + expected: String::from("`type` must be one of `fs` or `9p`"), })? } "uidmap" => shared_dir.uid_map = value.into(), @@ -863,7 +880,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: "timeout" => { let seconds = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`timeout` must be an integer", + expected: String::from("`timeout` must be an integer"), })?; let dur = Duration::from_secs(seconds); @@ -873,7 +890,9 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: "cache" => { let policy = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`cache` must be one of `never`, `always`, or `auto`", + expected: String::from( + "`cache` must be one of `never`, `always`, or `auto`", + ), })?; shared_dir.cfg.cache_policy = policy; } @@ -881,14 +900,14 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: let writeback = value.parse().map_err(|_| argument::Error::InvalidValue { value: value.to_owned(), - expected: "`writeback` must be a boolean", + expected: String::from("`writeback` must be a boolean"), })?; shared_dir.cfg.writeback = writeback; } _ => { return Err(argument::Error::InvalidValue { value: kind.to_owned(), - expected: "unrecognized option for `shared-dir`", + expected: String::from("unrecognized option for `shared-dir`"), }) } } @@ -930,7 +949,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if plugin.is_relative() { return Err(argument::Error::InvalidValue { value: plugin.to_string_lossy().into_owned(), - expected: "the plugin path must be an absolute path", + expected: String::from("the plugin path must be an absolute path"), }); } cfg.executable_path = Some(Executable::Plugin(plugin)); @@ -945,7 +964,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: "plugin-mount-file" => { let file = File::open(value.unwrap()).map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "unable to open `plugin-mount-file` file", + expected: String::from("unable to open `plugin-mount-file` file"), })?; let reader = BufReader::new(file); for l in reader.lines() { @@ -964,7 +983,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: "plugin-gid-map-file" => { let file = File::open(value.unwrap()).map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "unable to open `plugin-gid-map-file` file", + expected: String::from("unable to open `plugin-gid-map-file` file"), })?; let reader = BufReader::new(file); for l in reader.lines() { @@ -984,7 +1003,9 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: .parse() .map_err(|_| argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this value for `tap-fd` must be an unsigned integer", + expected: String::from( + "this value for `tap-fd` must be an unsigned integer", + ), })?, ); } @@ -1053,7 +1074,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if !dev_path.exists() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "this input device path does not exist", + expected: String::from("this input device path does not exist"), }); } cfg.virtio_input_evdevs.push(dev_path); @@ -1078,13 +1099,13 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: if !vfio_path.exists() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "the vfio path does not exist", + expected: String::from("the vfio path does not exist"), }); } if !vfio_path.is_dir() { return Err(argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "the vfio path should be directory", + expected: String::from("the vfio path should be directory"), }); } @@ -1410,7 +1431,7 @@ fn create_qcow2(args: std::env::Args) -> std::result::Result<(), ()> { size = Some(value.unwrap().parse::().map_err(|_| { argument::Error::InvalidValue { value: value.unwrap().to_owned(), - expected: "SIZE should be a nonnegative integer", + expected: String::from("SIZE should be a nonnegative integer"), } })?); } -- cgit 1.4.1 From 6d2a83482737cac0e5777a00e27d34e704eedb71 Mon Sep 17 00:00:00 2001 From: Matt Delco Date: Thu, 16 Jan 2020 12:41:06 -0800 Subject: seccomp: add frequency file to x86_64 Add a frequency file that teaches the seccomp compiler to weight the comparison tree in favor of the most frequenctly called syscalls. This frequency file was created by running strace against vm_conciege's pid (e.g., "strace -p PID -ff -e raw=all -o /tmp/strace") when performing a start and stop of a VM, deleting the trace files that weren't for a crosvm process, passing the files to minijail's tools/generate_seccomp_policy.py (using the -frequency option), and combining the results of the frequency file. I rounded the #s to the nearest multiple of 5 and only retained the syscalls that had at least 10 calls. BUG=None TEST=Local build and deploy. Verified that crostini VM still boots and shuts down properly. Used scmp_bpf_disasm to disassemble a few bpf files before and after this change to confirm that with the frequency file the first comparision is "jge 2" (to quickly whitelist syscalls 0 and 1 ['read' and 'write']) instead of a comparison around the middle of the range of syscall numbers that are used. Change-Id: Icace2b5cdbcae6e51cfd67a3034a1a17fdb6d59e Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2005793 Commit-Queue: Matt Delco Commit-Queue: Stephen Barber Tested-by: Matt Delco Tested-by: kokoro Auto-Submit: Matt Delco Reviewed-by: Stephen Barber --- seccomp/x86_64/common_device.frequency | 45 ++++++++++++++++++++++++++++++++++ seccomp/x86_64/common_device.policy | 1 + 2 files changed, 46 insertions(+) create mode 100644 seccomp/x86_64/common_device.frequency diff --git a/seccomp/x86_64/common_device.frequency b/seccomp/x86_64/common_device.frequency new file mode 100644 index 0000000..618c44d --- /dev/null +++ b/seccomp/x86_64/common_device.frequency @@ -0,0 +1,45 @@ +# 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. + +capget: 30 +capset: 30 +chdir: 30 +chroot: 15 +clone: 30 +close: 1185 +dup: 50 +dup2: 160 +epoll_ctl: 25 +epoll_wait: 90 +eventfd2: 75 +exit: 15 +exit_group: 15 +fchdir: 30 +fstat: 90 +futex: 20 +getdents: 55 +ioctl: 350 +mmap: 95 +mount: 45 +mprotect: 45 +openat: 515 +pipe: 15 +pivot_root: 15 +prctl: 570 +prlimit64: 15 +read: 82415 +recvmsg: 85 +restart_syscall: 15 +rt_sigaction: 20 +rt_sigreturn: 15 +seccomp: 25 +sendmsg: 390 +setsockopt: 30 +socket: 20 +socketpair: 30 +stat: 30 +umount2: 15 +unshare: 30 +wait4: 20 +write: 56100 diff --git a/seccomp/x86_64/common_device.policy b/seccomp/x86_64/common_device.policy index 8464c4b..453719d 100644 --- a/seccomp/x86_64/common_device.policy +++ b/seccomp/x86_64/common_device.policy @@ -2,6 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. +@frequency ./common_device.frequency brk: 1 clone: arg0 & CLONE_THREAD close: 1 -- 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(-) 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(-) 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(+) 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(-) 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 9486e57a097aeb98ccb33776aefa523be795b661 Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Tue, 10 Mar 2020 13:33:09 +0800 Subject: ACPI: enable ACPI from command line Previously the "acpi=off" in cmdline has disabled the ACPI for the guest kernel. With removing the "acpi=off", the ACPI will be enabled for the guest kernel by default. With acpi enabled, the SCI irq will be needed by the ACPI core driver. Register the SCI irq in MP table so that it can use IO-APIC routing. The reason to have "pci=noacpi" is that, in the current DSDT there is only suspend capability, so PCI scan still need to be done by the traditional way. BUG=chromium:1018674 TEST=Linux guest is able to boot up with the virtio devices functional. Also able to see the S1 capability from kernel dmesg. Change-Id: Id54e788f4aa4c944fac5e3fa1c92b76865dd5021 Signed-off-by: Chuanxiao Dong Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2078967 Reviewed-by: Tomasz Jeznach Tested-by: kokoro --- x86_64/src/lib.rs | 20 +++++++++++--------- x86_64/src/mpspec.rs | 1 + x86_64/src/mptable.rs | 26 +++++++++++++++++++++++--- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index a912edd..a4ba444 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -189,7 +189,15 @@ const CMDLINE_OFFSET: u64 = 0x20000; const CMDLINE_MAX_SIZE: u64 = KERNEL_START_OFFSET - CMDLINE_OFFSET; const X86_64_SERIAL_1_3_IRQ: u32 = 4; const X86_64_SERIAL_2_4_IRQ: u32 = 3; -const X86_64_IRQ_BASE: u32 = 5; +// X86_64_SCI_IRQ is used to fill the ACPI FACP table. +// The sci_irq number is better to be a legacy +// IRQ number which is less than 16(actually most of the +// platforms have fixed IRQ number 9). So we can +// reserve the IRQ number 5 for SCI and let the +// the other devices starts from next. +pub const X86_64_SCI_IRQ: u32 = 5; +// So the IRQ_BASE start from SCI_IRQ + 1 +pub const X86_64_IRQ_BASE: u32 = X86_64_SCI_IRQ + 1; const ACPI_HI_RSDP_WINDOW_BASE: u64 = 0x000E0000; fn configure_system( @@ -203,7 +211,6 @@ fn configure_system( setup_data: Option, initrd: Option<(GuestAddress, usize)>, mut params: boot_params, - sci_irq: u32, ) -> Result<()> { const EBDA_START: u64 = 0x0009fc00; const KERNEL_BOOT_FLAG_MAGIC: u16 = 0xaa55; @@ -267,7 +274,7 @@ fn configure_system( .write_obj_at_addr(params, zero_page_addr) .map_err(|_| Error::ZeroPageSetup)?; - let rsdp_addr = acpi::create_acpi_tables(guest_mem, num_cpus, sci_irq); + let rsdp_addr = acpi::create_acpi_tables(guest_mem, num_cpus, X86_64_SCI_IRQ); params.acpi_rsdp_addr = rsdp_addr.0; Ok(()) @@ -404,8 +411,6 @@ impl arch::LinuxArch for X8664arch { // Event used to notify crosvm that guest OS is trying to suspend. let suspend_evt = EventFd::new().map_err(Error::CreateEventFd)?; - // allocate sci_irq to fill the ACPI FACP table - let sci_irq = resources.allocate_irq().ok_or(Error::AllocateIrq)?; let mut io_bus = Self::setup_io_bus( &mut vm, @@ -492,7 +497,6 @@ impl arch::LinuxArch for X8664arch { components.android_fstab, kernel_end, params, - sci_irq, )?; } } @@ -579,7 +583,6 @@ impl X8664arch { android_fstab: Option, kernel_end: u64, params: boot_params, - sci_irq: u32, ) -> Result<()> { kernel_loader::load_cmdline(mem, GuestAddress(CMDLINE_OFFSET), cmdline) .map_err(Error::LoadCmdline)?; @@ -641,7 +644,6 @@ impl X8664arch { setup_data, initrd, params, - sci_irq, )?; Ok(()) } @@ -723,7 +725,7 @@ impl X8664arch { let tty_string = get_serial_tty_string(stdio_serial_num); cmdline.insert("console", &tty_string).unwrap(); } - cmdline.insert_str("acpi=off reboot=k panic=-1").unwrap(); + cmdline.insert_str("pci=noacpi reboot=k panic=-1").unwrap(); cmdline } diff --git a/x86_64/src/mpspec.rs b/x86_64/src/mpspec.rs index ab7af51..5340d9e 100644 --- a/x86_64/src/mpspec.rs +++ b/x86_64/src/mpspec.rs @@ -38,6 +38,7 @@ pub const MPC_APIC_USABLE: ::std::os::raw::c_uint = 1; pub const MP_IRQDIR_DEFAULT: ::std::os::raw::c_uint = 0; pub const MP_IRQDIR_HIGH: ::std::os::raw::c_uint = 1; pub const MP_IRQDIR_LOW: ::std::os::raw::c_uint = 3; +pub const MP_LEVEL_TRIGGER: ::std::os::raw::c_uint = 0xc; pub const MP_APIC_ALL: ::std::os::raw::c_uint = 255; pub const MPC_OEM_SIGNATURE: &'static [u8; 5usize] = b"_OEM\x00"; #[repr(C)] diff --git a/x86_64/src/mptable.rs b/x86_64/src/mptable.rs index 9aded3f..cac9e58 100644 --- a/x86_64/src/mptable.rs +++ b/x86_64/src/mptable.rs @@ -231,8 +231,9 @@ pub fn setup_mptable( base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_intsrc)); } + let sci_irq = super::X86_64_SCI_IRQ as u8; // Per kvm_setup_default_irq_routing() in kernel - for i in 0..5 { + for i in 0..sci_irq { let size = mem::size_of::(); let mut mpc_intsrc = mpc_intsrc::default(); mpc_intsrc.type_ = MP_INTSRC as u8; @@ -247,6 +248,25 @@ pub fn setup_mptable( base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_intsrc)); } + // Insert SCI interrupt before PCI interrupts. Set the SCI interrupt + // to be the default trigger/polarity of PCI bus, which is level/low. + // This setting can be changed in future if necessary. + { + let size = mem::size_of::(); + let mut mpc_intsrc = mpc_intsrc::default(); + mpc_intsrc.type_ = MP_INTSRC as u8; + mpc_intsrc.irqtype = mp_irq_source_types_mp_INT as u8; + mpc_intsrc.irqflag = (MP_IRQDIR_HIGH | MP_LEVEL_TRIGGER) as u16; + mpc_intsrc.srcbus = ISA_BUS_ID; + mpc_intsrc.srcbusirq = sci_irq; + mpc_intsrc.dstapic = ioapicid; + mpc_intsrc.dstirq = sci_irq; + mem.write_obj_at_addr(mpc_intsrc, base_mp) + .map_err(|_| Error::WriteMpcIntsrc)?; + base_mp = base_mp.unchecked_add(size as u64); + checksum = checksum.wrapping_add(compute_checksum(&mpc_intsrc)); + } + let pci_irq_base = super::X86_64_IRQ_BASE as u8; // Insert PCI interrupts after platform IRQs. for (i, pci_irq) in pci_irqs.iter().enumerate() { let size = mem::size_of::(); @@ -257,14 +277,14 @@ pub fn setup_mptable( mpc_intsrc.srcbus = PCI_BUS_ID; mpc_intsrc.srcbusirq = (pci_irq.0 as u8 + 1) << 2 | pci_irq.1.to_mask() as u8; mpc_intsrc.dstapic = ioapicid; - mpc_intsrc.dstirq = 5 + i as u8; + mpc_intsrc.dstirq = pci_irq_base + i as u8; mem.write_obj_at_addr(mpc_intsrc, base_mp) .map_err(|_| Error::WriteMpcIntsrc)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_intsrc)); } // Finally insert ISA interrupts. - for i in 5 + pci_irqs.len()..16 { + for i in pci_irq_base + pci_irqs.len() as u8..16 { let size = mem::size_of::(); let mut mpc_intsrc = mpc_intsrc::default(); mpc_intsrc.type_ = MP_INTSRC as u8; -- 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 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(-) 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 911e21e3925634833cf91ca1dd688c6047dc4b47 Mon Sep 17 00:00:00 2001 From: David Stevens Date: Mon, 23 Mar 2020 14:38:34 +0900 Subject: gpu_renderer: disable debug callback on arm The vararg bindings are different for different architectures. Limit support to x86 and x86_64, since those are the bindings that are checked in. BUG=chromium:1063640 TEST=compiles Change-Id: Ic69753959684f55855fd7a8577a422638cd05f8b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2114633 Reviewed-by: Lepton Wu Reviewed-by: Chirantan Ekbote Commit-Queue: David Stevens Tested-by: David Stevens --- gpu_renderer/src/lib.rs | 8 +++++++- gpu_renderer/src/vsnprintf.rs | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gpu_renderer/src/lib.rs b/gpu_renderer/src/lib.rs index 1c8461c..d75add1 100644 --- a/gpu_renderer/src/lib.rs +++ b/gpu_renderer/src/lib.rs @@ -9,6 +9,7 @@ mod generated; mod vsnprintf; use std::cell::RefCell; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] use std::ffi::CString; use std::fmt::{self, Display}; use std::fs::File; @@ -33,6 +34,7 @@ use crate::generated::p_format::PIPE_FORMAT_B8G8R8X8_UNORM; use crate::generated::virglrenderer::*; 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`.. @@ -248,7 +250,10 @@ impl Renderer { fence_state: Rc::clone(&fence_state), })); - unsafe { virgl_set_debug_callback(Some(Renderer::debug_callback)) }; + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + unsafe { + virgl_set_debug_callback(Some(Renderer::debug_callback)) + }; // Safe because a valid cookie and set of callbacks is used and the result is checked for // error. @@ -474,6 +479,7 @@ impl Renderer { Err(Error::Unsupported) } + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] extern "C" fn debug_callback( fmt: *const ::std::os::raw::c_char, ap: *mut generated::virglrenderer::__va_list_tag, diff --git a/gpu_renderer/src/vsnprintf.rs b/gpu_renderer/src/vsnprintf.rs index 72583c5..ec121ad 100644 --- a/gpu_renderer/src/vsnprintf.rs +++ b/gpu_renderer/src/vsnprintf.rs @@ -12,6 +12,7 @@ * -o vsnprintf.rs */ +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] extern "C" { pub fn vsnprintf( __s: *mut ::std::os::raw::c_char, @@ -20,6 +21,8 @@ extern "C" { __arg: *mut __va_list_tag, ) -> ::std::os::raw::c_int; } + +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] #[repr(C)] #[derive(Debug, Copy, Clone)] pub struct __va_list_tag { -- 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(-) 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(-) 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(-) 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 928a8395e04425b72c4af272ffae7b97f252238a Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Mon, 23 Mar 2020 11:06:47 -0700 Subject: docker: update ADHD commit to fix kokoro build Update the audio_streams dependency to fix a build failure introduced in https://crrev.com/c/2038413. BUG=None TEST=docker/build_crosvm_base.sh && docker/wrapped_smoke_test.sh Change-Id: I99b26c925a8bc5bb6f77575eb64c0972a5a5e0ae Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2116274 Reviewed-by: Dylan Reid Reviewed-by: Stephen Barber Reviewed-by: Zach Reizner Tested-by: Daniel Verkamp Tested-by: George Engelbrecht Commit-Queue: Daniel Verkamp --- docker/checkout_commits.env | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/checkout_commits.env b/docker/checkout_commits.env index 92586bc..60aecc1 100644 --- a/docker/checkout_commits.env +++ b/docker/checkout_commits.env @@ -2,5 +2,5 @@ MESON_COMMIT=a1a8772034aef90e8d58230d8bcfce54ab27bf6a LIBEPOXY_COMMIT=af38a466caf9c2ae49b8acda4ff842ae44d57f78 TPM2_COMMIT=a9bc45bb7fafc65ea8a787894434d409f533b1f1 PLATFORM2_COMMIT=9239a43f2dc2e98e57e9d77aac72fa3ce8169e5f -ADHD_COMMIT=d99fa2312e03594c1de236e7ac74b332b0ddc329 +ADHD_COMMIT=db796cecdea7013b8679f90dfae34915edc9246f DRM_COMMIT=00320d7d68ddc7d815d073bb7c92d9a1f9bb8c31 -- 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 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(+) 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(-) 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(-) 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