From 8c60b644543833579b0bec9f070bbf4e3bfcfbae Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 22 Jun 2022 16:27:07 -0700 Subject: [PATCH 1/9] vcpu: refactor scheduler config into a new function Move thread affinity and other scheduler configuration into a new set_vcpu_thread_scheduling() function, and call that function right away when each vcpu thread is created. This moves the affinity-related code out of the runnable_vcpu() function, making the responsibilities of each function clearer. The same thread affinity, core scheduling, cgroup membership, and real-time scheduling priority are applied in the same order as before, but these are all set up front before any other vcpu setup. BUG=None TEST=crosvm run -c 4 bzImage Change-Id: I825f71fd4a07165190ffe2a04b35ceffe3dc0308 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3719325 Tested-by: kokoro Commit-Queue: Daniel Verkamp Reviewed-by: Keiichi Watanabe Reviewed-by: Junichi Uekawa --- src/crosvm/linux/vcpu.rs | 85 +++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/src/crosvm/linux/vcpu.rs b/src/crosvm/linux/vcpu.rs index bbe40cc0d9..a910fbf19e 100644 --- a/src/crosvm/linux/vcpu.rs +++ b/src/crosvm/linux/vcpu.rs @@ -89,6 +89,45 @@ fn bus_io_handler(bus: &Bus) -> impl FnMut(IoParams) -> Option<[u8; 8]> + '_ { } } +/// Set the VCPU thread affinity and other per-thread scheduler properties. +/// This function will be called from each VCPU thread at startup. +pub fn set_vcpu_thread_scheduling( + vcpu_affinity: Vec, + enable_per_vm_core_scheduling: bool, + vcpu_cgroup_tasks_file: Option, + run_rt: bool, +) -> anyhow::Result<()> { + if !vcpu_affinity.is_empty() { + if let Err(e) = set_cpu_affinity(vcpu_affinity) { + error!("Failed to set CPU affinity: {}", e); + } + } + + if !enable_per_vm_core_scheduling { + // Do per-vCPU core scheduling by setting a unique cookie to each vCPU. + if let Err(e) = enable_core_scheduling() { + error!("Failed to enable core scheduling: {}", e); + } + } + + // Move vcpu thread to cgroup + if let Some(mut f) = vcpu_cgroup_tasks_file { + f.write_all(base::gettid().to_string().as_bytes()) + .context("failed to write vcpu tid to cgroup tasks")?; + } + + if run_rt { + const DEFAULT_VCPU_RT_LEVEL: u16 = 6; + if let Err(e) = set_rt_prio_limit(u64::from(DEFAULT_VCPU_RT_LEVEL)) + .and_then(|_| set_rt_round_robin(i32::from(DEFAULT_VCPU_RT_LEVEL))) + { + warn!("Failed to set vcpu to real time: {}", e); + } + } + + Ok(()) +} + // Sets up a vcpu and converts it into a runnable vcpu. pub fn runnable_vcpu( cpu_id: usize, @@ -98,16 +137,12 @@ pub fn runnable_vcpu( vm: impl VmArch, irq_chip: &mut dyn IrqChipArch, vcpu_count: usize, - run_rt: bool, - vcpu_affinity: Vec, no_smt: bool, has_bios: bool, use_hypervisor_signals: bool, - enable_per_vm_core_scheduling: bool, host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, - vcpu_cgroup_tasks_file: Option, ) -> Result<(V, VcpuRunHandle)> where V: VcpuArch, @@ -132,12 +167,6 @@ where .add_vcpu(cpu_id, &vcpu) .context("failed to add vcpu to irq chip")?; - if !vcpu_affinity.is_empty() { - if let Err(e) = set_cpu_affinity(vcpu_affinity) { - error!("Failed to set CPU affinity: {}", e); - } - } - Arch::configure_vcpu( &vm, vm.get_hypervisor(), @@ -154,28 +183,6 @@ where ) .context("failed to configure vcpu")?; - if !enable_per_vm_core_scheduling { - // Do per-vCPU core scheduling by setting a unique cookie to each vCPU. - if let Err(e) = enable_core_scheduling() { - error!("Failed to enable core scheduling: {}", e); - } - } - - // Move vcpu thread to cgroup - if let Some(mut f) = vcpu_cgroup_tasks_file { - f.write_all(base::gettid().to_string().as_bytes()) - .context("failed to write vcpu tid to cgroup tasks")?; - } - - if run_rt { - const DEFAULT_VCPU_RT_LEVEL: u16 = 6; - if let Err(e) = set_rt_prio_limit(u64::from(DEFAULT_VCPU_RT_LEVEL)) - .and_then(|_| set_rt_round_robin(i32::from(DEFAULT_VCPU_RT_LEVEL))) - { - warn!("Failed to set vcpu to real time: {}", e); - } - } - if use_hypervisor_signals { let mut v = get_blocked_signals().context("failed to retrieve signal mask for vcpu")?; v.retain(|&x| x != SIGRTMIN() + 0); @@ -583,6 +590,16 @@ where // send a VmEventType on all code paths after the closure // returns. let vcpu_fn = || -> ExitState { + if let Err(e) = set_vcpu_thread_scheduling( + vcpu_affinity, + enable_per_vm_core_scheduling, + vcpu_cgroup_tasks_file, + run_rt && !delay_rt, + ) { + error!("vcpu thread setup failed: {:#}", e); + return ExitState::Stop; + } + #[cfg(all(target_arch = "x86_64", feature = "gdb"))] let guest_mem = vm.get_memory().clone(); let runnable_vcpu = runnable_vcpu( @@ -593,16 +610,12 @@ where vm, irq_chip.as_mut(), vcpu_count, - run_rt && !delay_rt, - vcpu_affinity, no_smt, has_bios, use_hypervisor_signals, - enable_per_vm_core_scheduling, host_cpu_topology, enable_pnp_data, itmt, - vcpu_cgroup_tasks_file, ); // Add MSR handlers after CPU affinity setting. From 4f902ffc9e9a9e6f779ffbcb86a155b60e165a5a Mon Sep 17 00:00:00 2001 From: Alexandre Courbot Date: Wed, 22 Jun 2022 12:13:15 +0900 Subject: [PATCH 2/9] ffmpeg: specify supported resolution range for input formats This is required so the guest can know which resolutions are supported for encoded formats. BUG=b:169295147 TEST=ffplay from Linux guest can start streaming. Change-Id: I6f86108efbc8971f3ee4b9ec494cec16ebce323d Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3716017 Reviewed-by: Keiichi Watanabe Tested-by: kokoro Commit-Queue: Alexandre Courbot --- .../src/virtio/video/decoder/backend/ffmpeg.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/devices/src/virtio/video/decoder/backend/ffmpeg.rs b/devices/src/virtio/video/decoder/backend/ffmpeg.rs index 36922d7497..5558724b11 100644 --- a/devices/src/virtio/video/decoder/backend/ffmpeg.rs +++ b/devices/src/virtio/video/decoder/backend/ffmpeg.rs @@ -661,7 +661,22 @@ impl DecoderBackend for FfmpegDecoder { in_formats.push(FormatDesc { mask: !(u64::MAX << SUPPORTED_OUTPUT_FORMATS.len()), format, - frame_formats: vec![Default::default()], + frame_formats: vec![FrameFormat { + // These frame sizes are arbitrary, but avcodec does not seem to have any + // specific restriction in that regard (or any way to query the supported + // resolutions). + width: FormatRange { + min: 64, + max: 16384, + step: 1, + }, + height: FormatRange { + min: 64, + max: 16384, + step: 1, + }, + bitrates: Default::default(), + }], }); } From efde16f4d24c15e7970645bac42fbc699a3ac739 Mon Sep 17 00:00:00 2001 From: Richard Date: Fri, 24 Jun 2022 14:26:02 -0700 Subject: [PATCH 3/9] virtio: Enable build and tests for some virtio mods Still need to enable: * balloon (try_clone not implemented) * console (Need to enable Serial) * snd (Unix code not conditionally compiled out) * gpu (Waiting for graphics team to upstream) * vhost-user block and handler The vhost mod is also being built now, but the vhost devices that don't build on Windows have been disabled. BUG=b:237011316 TEST=ran "./tools/run_tests --target=host --arch=win64 --verbose" Change-Id: I3d06a9d49b4bdae14dea47fcfa030834b55925ac Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3723797 Reviewed-by: Vikram Auradkar Reviewed-by: Noah Gold Tested-by: kokoro Commit-Queue: Richard Zhang Reviewed-by: Keiichi Watanabe --- devices/src/virtio/mod.rs | 48 ++++++++++++--------- devices/src/virtio/vhost/user/device/mod.rs | 8 ++-- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/devices/src/virtio/mod.rs b/devices/src/virtio/mod.rs index 63aab119c7..ac4bbfb2a9 100644 --- a/devices/src/virtio/mod.rs +++ b/devices/src/virtio/mod.rs @@ -4,67 +4,73 @@ //! Implements virtio devices, queues, and transport mechanisms. +mod async_device; mod async_utils; mod descriptor_utils; +mod input; mod interrupt; mod iommu; mod queue; +mod rng; +#[cfg(feature = "tpm")] +mod tpm; +#[cfg(any(feature = "video-decoder", feature = "video-encoder"))] +mod video; mod virtio_device; +mod virtio_pci_common_config; +mod virtio_pci_device; + +pub mod block; +pub mod resource_bridge; +pub mod vhost; + +pub use self::block::*; pub use self::descriptor_utils::Error as DescriptorError; pub use self::descriptor_utils::*; +pub use self::input::*; pub use self::interrupt::*; pub use self::iommu::*; pub use self::queue::*; +pub use self::rng::*; +#[cfg(feature = "tpm")] +pub use self::tpm::*; +#[cfg(any(feature = "video-decoder", feature = "video-encoder"))] +pub use self::video::*; pub use self::virtio_device::*; +pub use self::virtio_pci_device::*; cfg_if::cfg_if! { if #[cfg(unix)] { - mod async_device; mod balloon; - mod input; mod p9; mod pmem; - mod rng; - #[cfg(feature = "tpm")] - mod tpm; - #[cfg(any(feature = "video-decoder", feature = "video-encoder"))] - mod video; - mod virtio_pci_common_config; - mod virtio_pci_device; pub mod wl; - pub mod block; pub mod console; pub mod fs; #[cfg(feature = "gpu")] pub mod gpu; pub mod net; - pub mod resource_bridge; #[cfg(feature = "audio")] pub mod snd; - pub mod vhost; pub use self::balloon::*; - pub use self::block::*; pub use self::console::*; #[cfg(feature = "gpu")] pub use self::gpu::*; - pub use self::input::*; - #[cfg(unix)] pub use self::iommu::sys::unix::vfio_wrapper; pub use self::net::*; pub use self::p9::*; pub use self::pmem::*; - pub use self::rng::*; #[cfg(feature = "audio")] pub use self::snd::*; - #[cfg(feature = "tpm")] - pub use self::tpm::*; - #[cfg(any(feature = "video-decoder", feature = "video-encoder"))] - pub use self::video::*; - pub use self::virtio_pci_device::*; pub use self::wl::*; } else if #[cfg(windows)] { + #[cfg(feature = "slirp")] + pub mod net; + + #[cfg(feature = "slirp")] + pub use self::net::*; } else { compile_error!("Unsupported platform"); } diff --git a/devices/src/virtio/vhost/user/device/mod.rs b/devices/src/virtio/vhost/user/device/mod.rs index d3296c3084..fcd437bdcb 100644 --- a/devices/src/virtio/vhost/user/device/mod.rs +++ b/devices/src/virtio/vhost/user/device/mod.rs @@ -2,24 +2,22 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -mod block; -mod handler; - -pub use block::{run_block_device, Options as BlockOptions}; - cfg_if::cfg_if! { if #[cfg(unix)] { + mod block; #[cfg(feature = "gpu")] mod gpu; mod console; #[cfg(feature = "audio_cras")] mod cras_snd; mod fs; + mod handler; mod net; mod vsock; mod vvu; mod wl; + pub use block::{run_block_device, Options as BlockOptions}; pub use vsock::{run_vsock_device, Options as VsockOptions}; pub use wl::{run_wl_device, parse_wayland_sock, Options as WlOptions}; pub use console::{run_console_device, Options as ConsoleOptions}; From 4ea25d1e3356d5715fb016aced2c8b38797fd434 Mon Sep 17 00:00:00 2001 From: Noah Gold Date: Fri, 10 Jun 2022 13:46:21 -0700 Subject: [PATCH 4/9] x86_64: add TSC leaf synthesis. There have been two evolutions of providing the TSC cpuid leaf (aka 0x15) to the guest. a) For CrosVM on Windows, we have been providing the leaf unconditionally. Furthermore, we've not been using the exact host leaf; instead, we calibrate the TSC frequency and provide that value in the leaf. This was done because the actual cpuid leaf values are not as accurate as we needed them to be to drive a guest clocksource. b) In CrosVM mainline, 4080aaf9b33e0d16247a7a92e2c4a3b1eaeb8e08 introduced the flag enable_pnp / enable_pnp_data, and provides the exact host 0x15 leaf to the guest if the flag is enabled. This CL adds a new hypervisor capability (CalibratedTscLeafRequired) to control whether or not the calibrated TSC leaf should be used, in addition to a new CLI option to force it on hypervisors where it isn't enabled by default. The new option is `--force_calibrated_tsc_leaf`. BUG=b:213152505 TEST=builds upstream, battletested downstream on WHPX. Change-Id: I611422808a9e10578c0ddcbd211ae902f937685f Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3698993 Commit-Queue: Noah Gold Reviewed-by: Junichi Uekawa Tested-by: kokoro --- aarch64/src/lib.rs | 1 + arch/src/lib.rs | 2 ++ hypervisor/src/caps.rs | 12 ++++++++++++ hypervisor/src/kvm/mod.rs | 2 ++ hypervisor/src/whpx/vm.rs | 2 ++ src/crosvm/cmdline.rs | 18 ++++++++++++++++++ src/crosvm/config.rs | 2 ++ src/crosvm/linux/mod.rs | 1 + src/crosvm/linux/vcpu.rs | 4 ++++ x86_64/src/cpuid.rs | 31 +++++++++++++++++++++++++++++-- x86_64/src/lib.rs | 2 ++ x86_64/src/test_integration.rs | 5 ++++- 12 files changed, 79 insertions(+), 3 deletions(-) diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs index 82c0f00896..9a3f075f39 100644 --- a/aarch64/src/lib.rs +++ b/aarch64/src/lib.rs @@ -561,6 +561,7 @@ impl arch::LinuxArch for AArch64 { _host_cpu_topology: bool, _enable_pnp_data: bool, _itmt: bool, + _force_calibrated_tsc_leaf: bool, ) -> std::result::Result<(), Self::Error> { // AArch64 doesn't configure vcpus on the vcpu thread, so nothing to do here. Ok(()) diff --git a/arch/src/lib.rs b/arch/src/lib.rs index 50d140ad6a..0c985e367e 100644 --- a/arch/src/lib.rs +++ b/arch/src/lib.rs @@ -238,6 +238,7 @@ pub trait LinuxArch { /// * `host_cpu_topology` - whether enabling host cpu topology. /// * `enable_pnp_data` - whether enabling PnP statistics data. /// * `itmt` - whether enabling ITMT scheduler + /// * `force_calibrated_tsc_leaf` - whether to force using a calibrated TSC leaf (0x15). fn configure_vcpu( vm: &V, hypervisor: &dyn HypervisorArch, @@ -251,6 +252,7 @@ pub trait LinuxArch { host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, ) -> Result<(), Self::Error>; /// Configures and add a pci device into vm diff --git a/hypervisor/src/caps.rs b/hypervisor/src/caps.rs index 0bc1c066b4..636d8b3cd1 100644 --- a/hypervisor/src/caps.rs +++ b/hypervisor/src/caps.rs @@ -12,6 +12,18 @@ pub enum HypervisorCap { UserMemory, #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] Xcrs, + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + /// CPUID leaf 0x15 is available on some Intel chips and contains the TSC + /// frequency, which can be used to calibrate the guest's TSC clocksource; + /// however, it is not typically accurate enough (being off by 1-2% is a + /// big problem for a clocksource), and inside the guest, calibration by + /// other means is not always reliable. + /// + /// Hypervisors which do not provide the TSC frequency (e.g. via the kvm + /// pvclock) or have another suitable calibration source can declare this + /// capability, which causes CrosVM to substitute a calibrated value in leaf + /// 0x15 that will be accurate enough for use in a clocksource. + CalibratedTscLeafRequired, } /// A capability the `Vm` can possibly expose. diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs index 2fe46a2ee9..d9873a2998 100644 --- a/hypervisor/src/kvm/mod.rs +++ b/hypervisor/src/kvm/mod.rs @@ -1152,6 +1152,8 @@ impl TryFrom for KvmCap { HypervisorCap::UserMemory => Ok(KvmCap::UserMemory), #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] HypervisorCap::Xcrs => Ok(KvmCap::Xcrs), + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + HypervisorCap::CalibratedTscLeafRequired => Err(Error::new(libc::EINVAL)), } } } diff --git a/hypervisor/src/whpx/vm.rs b/hypervisor/src/whpx/vm.rs index 47d90b25d0..ad5da2abe3 100644 --- a/hypervisor/src/whpx/vm.rs +++ b/hypervisor/src/whpx/vm.rs @@ -375,6 +375,8 @@ impl Vm for WhpxVm { VmCap::Protected => false, // whpx initializes cpuid early during VM creation. VmCap::EarlyInitCpuid => true, + // under whpx, guests rely on this leaf to calibrate their clocksource. + VmCap::CalibratedTscLeafRequired => true, } } diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index 443d6a4999..b25054805d 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -559,6 +559,11 @@ pub struct RunCommand { /// align - whether to adjust addr and size to page /// boundaries implicitly pub file_backed_mappings: Vec, + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + #[argh(switch)] + /// force use of a calibrated TSC cpuid leaf (0x15) even if the hypervisor + /// doesn't require one. + pub force_calibrated_tsc_leaf: bool, #[cfg(all(target_arch = "x86_64", feature = "gdb"))] #[argh(option, arg_name = "PORT")] /// (EXPERIMENTAL) gdb on the given port @@ -1594,8 +1599,21 @@ impl TryFrom for super::config::Config { cfg.itmt = cmd.itmt; + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + if cmd.enable_pnp_data && cmd.force_calibrated_tsc_leaf { + return Err( + "Only one of [enable_pnp_data,force_calibrated_tsc_leaf] can be specified" + .to_string(), + ); + } + cfg.enable_pnp_data = cmd.enable_pnp_data; + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + { + cfg.force_calibrated_tsc_leaf = cmd.force_calibrated_tsc_leaf; + } + cfg.privileged_vm = cmd.privileged_vm; cfg.stub_pci_devices = cmd.stub_pci_devices; diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs index 1dcc30d5cd..724693fe0c 100644 --- a/src/crosvm/config.rs +++ b/src/crosvm/config.rs @@ -1704,6 +1704,7 @@ pub struct Config { pub enable_pnp_data: bool, pub executable_path: Option, pub file_backed_mappings: Vec, + pub force_calibrated_tsc_leaf: bool, pub force_s2idle: bool, #[cfg(all(target_arch = "x86_64", feature = "gdb"))] pub gdb: Option, @@ -1835,6 +1836,7 @@ impl Default for Config { enable_pnp_data: false, executable_path: None, file_backed_mappings: Vec::new(), + force_calibrated_tsc_leaf: false, force_s2idle: false, #[cfg(all(target_arch = "x86_64", feature = "gdb"))] gdb: None, diff --git a/src/crosvm/linux/mod.rs b/src/crosvm/linux/mod.rs index b1d64a60fa..f8088b10f0 100644 --- a/src/crosvm/linux/mod.rs +++ b/src/crosvm/linux/mod.rs @@ -1986,6 +1986,7 @@ fn run_control( cfg.host_cpu_topology, cfg.enable_pnp_data, cfg.itmt, + cfg.force_calibrated_tsc_leaf, cfg.privileged_vm, match vcpu_cgroup_tasks_file { None => None, diff --git a/src/crosvm/linux/vcpu.rs b/src/crosvm/linux/vcpu.rs index a910fbf19e..08a8115e3b 100644 --- a/src/crosvm/linux/vcpu.rs +++ b/src/crosvm/linux/vcpu.rs @@ -143,6 +143,7 @@ pub fn runnable_vcpu( host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, ) -> Result<(V, VcpuRunHandle)> where V: VcpuArch, @@ -180,6 +181,7 @@ where host_cpu_topology, enable_pnp_data, itmt, + force_calibrated_tsc_leaf, ) .context("failed to configure vcpu")?; @@ -575,6 +577,7 @@ pub fn run_vcpu( host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, privileged_vm: bool, vcpu_cgroup_tasks_file: Option, userspace_msr: BTreeMap, @@ -616,6 +619,7 @@ where host_cpu_topology, enable_pnp_data, itmt, + force_calibrated_tsc_leaf, ); // Add MSR handlers after CPU affinity setting. diff --git a/x86_64/src/cpuid.rs b/x86_64/src/cpuid.rs index d33e547215..640e034d77 100644 --- a/x86_64/src/cpuid.rs +++ b/x86_64/src/cpuid.rs @@ -7,7 +7,7 @@ use std::cmp; use std::result; use devices::{Apic, IrqChipCap, IrqChipX86_64}; -use hypervisor::{CpuIdEntry, HypervisorX86_64, VcpuX86_64}; +use hypervisor::{CpuIdEntry, HypervisorCap, HypervisorX86_64, VcpuX86_64}; use crate::CpuManufacturer; use remain::sorted; @@ -63,6 +63,11 @@ pub struct CpuIdContext { apic_frequency: u32, /// The TSC frequency in Hz, if it could be determined. tsc_frequency: Option, + /// Whether to force the use of a calibrated TSC cpuid leaf (0x15) even if + /// the hypervisor doesn't require it. + force_calibrated_tsc_leaf: bool, + /// Whether the hypervisor requires a calibrated TSC cpuid leaf (0x15). + calibrated_tsc_leaf_required: bool, /// Whether or not VCPU IDs and APIC IDs should match host cpu IDs. host_cpu_topology: bool, enable_pnp_data: bool, @@ -83,6 +88,8 @@ impl CpuIdContext { irq_chip: Option<&dyn IrqChipX86_64>, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, + calibrated_tsc_leaf_required: bool, cpuid_count: unsafe fn(u32, u32) -> CpuidResult, cpuid: unsafe fn(u32) -> CpuidResult, ) -> CpuIdContext { @@ -96,6 +103,8 @@ impl CpuIdContext { }), apic_frequency: irq_chip.map_or(Apic::frequency(), |chip| chip.lapic_frequency()), tsc_frequency: devices::tsc::tsc_frequency().ok(), + force_calibrated_tsc_leaf, + calibrated_tsc_leaf_required, host_cpu_topology, enable_pnp_data, itmt, @@ -210,7 +219,18 @@ pub fn adjust_cpuid(entry: &mut CpuIdEntry, ctx: &CpuIdContext) { } } 0x15 => { - if ctx.enable_pnp_data { + if ctx.calibrated_tsc_leaf_required + || ctx.force_calibrated_tsc_leaf { + + let cpuid_15 = ctx + .tsc_frequency + .map(|tsc_freq| devices::tsc::fake_tsc_frequency_cpuid( + tsc_freq, ctx.apic_frequency)); + + if let Some(new_entry) = cpuid_15 { + entry.cpuid = new_entry.cpuid; + } + } else if ctx.enable_pnp_data { // Expose TSC frequency to guest // Safe because we pass 0x15 for this call and the host // supports the `cpuid` instruction @@ -321,6 +341,7 @@ pub fn setup_cpuid( host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, ) -> Result<()> { let mut cpuid = hypervisor .get_supported_cpuid() @@ -336,6 +357,8 @@ pub fn setup_cpuid( Some(irq_chip), enable_pnp_data, itmt, + force_calibrated_tsc_leaf, + hypervisor.check_capability(HypervisorCap::CalibratedTscLeafRequired), __cpuid_count, __cpuid, ), @@ -421,6 +444,8 @@ mod tests { Some(&irq_chip), false, false, + false, + false, __cpuid_count, __cpuid, ), @@ -466,6 +491,8 @@ mod tests { host_cpu_topology: true, enable_pnp_data: false, itmt: false, + force_calibrated_tsc_leaf: false, + calibrated_tsc_leaf_required: false, cpuid_count: fake_cpuid_count, cpuid: fake_cpuid, }; diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index 4b27748ac6..73370847ad 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -770,6 +770,7 @@ impl arch::LinuxArch for X8664arch { host_cpu_topology: bool, enable_pnp_data: bool, itmt: bool, + force_calibrated_tsc_leaf: bool, ) -> Result<()> { if !vm.check_capability(VmCap::EarlyInitCpuid) { cpuid::setup_cpuid( @@ -782,6 +783,7 @@ impl arch::LinuxArch for X8664arch { host_cpu_topology, enable_pnp_data, itmt, + force_calibrated_tsc_leaf, ) .map_err(Error::SetupCpuid)?; } diff --git a/x86_64/src/test_integration.rs b/x86_64/src/test_integration.rs index 2085854550..8dfcbdfad3 100644 --- a/x86_64/src/test_integration.rs +++ b/x86_64/src/test_integration.rs @@ -240,7 +240,10 @@ where .expect("failed to add vcpu to irqchip"); if !vm.check_capability(VmCap::EarlyInitCpuid) { - setup_cpuid(&hyp, &irq_chip, &vcpu, 0, 1, false, false, false, false).unwrap(); + setup_cpuid( + &hyp, &irq_chip, &vcpu, 0, 1, false, false, false, false, false, + ) + .unwrap(); } setup_msrs(&vm, &vcpu, read_pci_mmio_before_32bit().start).unwrap(); From 10574227dca65a595f961e9058a6f951380ce650 Mon Sep 17 00:00:00 2001 From: Noah Gold Date: Mon, 27 Jun 2022 17:19:47 -0700 Subject: [PATCH 5/9] hypervisor: enable TSC cpuid leaf synthesis for haxm. Similar to WHPX, haxm requires an accurate (calibrated) leaf value to make the guest clocksource work accurately. BUG=b:213152505 TEST=battle tested downstream Change-Id: I55a45c00758b2112aced6185970ad43df060d0ba Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3731287 Tested-by: kokoro Commit-Queue: Noah Gold Reviewed-by: Vikram Auradkar Reviewed-by: Vaibhav Nagarnaik Reviewed-by: Alexandre Courbot --- hypervisor/src/haxm/vm.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/hypervisor/src/haxm/vm.rs b/hypervisor/src/haxm/vm.rs index 2c5392cb99..9777eab676 100644 --- a/hypervisor/src/haxm/vm.rs +++ b/hypervisor/src/haxm/vm.rs @@ -182,8 +182,13 @@ impl Vm for HaxmVm { }) } - fn check_capability(&self, _c: VmCap) -> bool { - false + fn check_capability(&self, c: VmCap) -> bool { + match c { + // under haxm, guests rely on this leaf to calibrate their + // clocksource. + VmCap::CalibratedTscLeafRequired => true, + _ => false, + } } fn get_memory(&self) -> &GuestMemory { From ed071b6edd4344d319ecbbbd518ef74a7b8bc18d Mon Sep 17 00:00:00 2001 From: Alex Gimenez Date: Tue, 28 Jun 2022 18:28:41 +0900 Subject: [PATCH 6/9] crosvm: implement lock-guest-memory feature. This CL adds a command-line flag to crosm that locks the entire guest memory, per http://go/host-assisted-vm-swap. The change relies on existing balloon device code that punches holes within the guest memory region upon "inflate". BUG=b:236210703 TEST=manual startup, roblox+instagram and various chrome sessions Change-Id: I11e0e5b0b0cb6450f47124a6a8b6c4befd9de6ae Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3725731 Reviewed-by: Keiichi Watanabe Reviewed-by: David Stevens Tested-by: kokoro Commit-Queue: Alexandre Marciano Gimenez Auto-Submit: Alexandre Marciano Gimenez --- base/src/sys/unix/mmap.rs | 36 ++++++++++++++++++++++++++ src/crosvm/cmdline.rs | 5 ++++ src/crosvm/config.rs | 6 +++++ src/crosvm/linux/mod.rs | 4 +++ vm_memory/src/guest_memory/sys/unix.rs | 22 ++++++++++++++-- 5 files changed, 71 insertions(+), 2 deletions(-) diff --git a/base/src/sys/unix/mmap.rs b/base/src/sys/unix/mmap.rs index c434a5ccd0..146e67af98 100644 --- a/base/src/sys/unix/mmap.rs +++ b/base/src/sys/unix/mmap.rs @@ -12,6 +12,8 @@ use std::{ ptr::{copy_nonoverlapping, null_mut, read_unaligned, write_unaligned}, }; +use log::warn; + use crate::{ external_mapping::ExternalMapping, AsRawDescriptor, Descriptor, MemoryMapping as CrateMemoryMapping, MemoryMappingBuilder, RawDescriptor, SafeDescriptor, @@ -703,6 +705,34 @@ impl MemoryMapping { } } + /// Disable host swap for this mapping. + pub fn lock_all(&self) -> Result<()> { + let ret = unsafe { + // Safe because MLOCK_ONFAULT only affects the swap behavior of the kernel, so it + // has no impact on rust semantics. + // TODO(raging): use the explicit declaration of mlock2, which was being merged + // as of when the call below was being worked on. + libc::syscall( + libc::SYS_mlock2, + (self.addr as usize) as *const libc::c_void, + self.size(), + libc::MLOCK_ONFAULT, + ) + }; + if ret < 0 { + let errno = super::Error::last(); + warn!( + "failed to mlock at {:#x} with length {}: {}", + (self.addr as usize) as u64, + self.size(), + errno, + ); + Err(Error::SystemCallFailed(errno)) + } else { + Ok(()) + } + } + // Check that offset+count is valid and return the sum. fn range_end(&self, offset: usize, count: usize) -> Result { let mem_end = offset.checked_add(count).ok_or(Error::InvalidAddress)?; @@ -990,13 +1020,19 @@ impl CrateMemoryMapping { } pub trait Unix { + /// Remove the specified range from the mapping. fn remove_range(&self, mem_offset: usize, count: usize) -> Result<()>; + /// Disable host swap for this mapping. + fn lock_all(&self) -> Result<()>; } impl Unix for CrateMemoryMapping { fn remove_range(&self, mem_offset: usize, count: usize) -> Result<()> { self.mapping.remove_range(mem_offset, count) } + fn lock_all(&self) -> Result<()> { + self.mapping.lock_all() + } } pub trait MemoryMappingBuilderUnix<'a> { diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index b25054805d..fac33608ba 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -651,6 +651,9 @@ pub struct RunCommand { #[argh(option, long = "kvm-device", arg_name = "PATH")] /// path to the KVM device. (default /dev/kvm) pub kvm_device_path: Option, + #[argh(switch)] + /// disable host swap on guest VM pages. + pub lock_guest_memory: bool, #[cfg(unix)] #[argh(option, arg_name = "MAC", long = "mac")] /// MAC address for VM @@ -1197,6 +1200,8 @@ impl TryFrom for super::config::Config { cfg.hugepages = cmd.hugepages; + cfg.lock_guest_memory = cmd.lock_guest_memory; + #[cfg(feature = "audio")] { cfg.ac97_parameters = cmd.ac97; diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs index 724693fe0c..ad5b8bc296 100644 --- a/src/crosvm/config.rs +++ b/src/crosvm/config.rs @@ -1720,6 +1720,7 @@ pub struct Config { pub itmt: bool, pub jail_config: Option, pub kvm_device_path: PathBuf, + pub lock_guest_memory: bool, pub mac_address: Option, pub memory: Option, pub memory_file: Option, @@ -1856,6 +1857,7 @@ impl Default for Config { None }, kvm_device_path: PathBuf::from(KVM_PATH), + lock_guest_memory: false, mac_address: None, memory: None, memory_file: None, @@ -2137,6 +2139,10 @@ pub fn validate_config(cfg: &mut Config) -> std::result::Result<(), String> { return Err("'balloon-control' requires enabled balloon".to_string()); } + if cfg.lock_guest_memory && cfg.jail_config.is_none() { + return Err("'lock-guest-memory' and 'disable-sandbox' are mutually exclusive".to_string()); + } + set_default_serial_parameters( &mut cfg.serial_parameters, !cfg.vhost_user_console.is_empty(), diff --git a/src/crosvm/linux/mod.rs b/src/crosvm/linux/mod.rs index f8088b10f0..5e31ca82cc 100644 --- a/src/crosvm/linux/mod.rs +++ b/src/crosvm/linux/mod.rs @@ -1201,6 +1201,10 @@ pub fn run_config(cfg: Config) -> Result { if components.hugepages { mem_policy |= MemoryPolicy::USE_HUGEPAGES; } + + if cfg.lock_guest_memory { + mem_policy |= MemoryPolicy::LOCK_GUEST_MEMORY; + } guest_mem.set_memory_policy(mem_policy); if cfg.kvm_device_path.exists() { diff --git a/vm_memory/src/guest_memory/sys/unix.rs b/vm_memory/src/guest_memory/sys/unix.rs index d3b0624b12..555c9ef438 100644 --- a/vm_memory/src/guest_memory/sys/unix.rs +++ b/vm_memory/src/guest_memory/sys/unix.rs @@ -11,6 +11,7 @@ use crate::{Error, GuestAddress, GuestMemory, Result}; bitflags! { pub struct MemoryPolicy: u32 { const USE_HUGEPAGES = 1; + const LOCK_GUEST_MEMORY = (1 << 1); } } @@ -41,14 +42,31 @@ impl GuestMemory { /// Handles guest memory policy hints/advices. pub fn set_memory_policy(&self, mem_policy: MemoryPolicy) { - if mem_policy.contains(MemoryPolicy::USE_HUGEPAGES) { - for (_, region) in self.regions.iter().enumerate() { + if mem_policy.is_empty() { + return; + } + + for (_, region) in self.regions.iter().enumerate() { + if mem_policy.contains(MemoryPolicy::USE_HUGEPAGES) { let ret = region.mapping.use_hugepages(); if let Err(err) = ret { println!("Failed to enable HUGEPAGE for mapping {}", err); } } + + if mem_policy.contains(MemoryPolicy::LOCK_GUEST_MEMORY) { + // This is done in coordination with remove_range() calls, which are + // performed by the virtio-balloon process (they must be performed by + // a different process from the one that issues the locks). + // We also prevent this from happening in single-process configurations, + // when we compute configuration flags. + let ret = region.mapping.lock_all(); + + if let Err(err) = ret { + println!("Failed to lock memory for mapping {}", err); + } + } } } } From 88b3f563f8174f0ee904a7a70b7ae73951d82a62 Mon Sep 17 00:00:00 2001 From: Vikram Auradkar Date: Tue, 28 Jun 2022 19:55:13 +0000 Subject: [PATCH 7/9] crosvm: move gpu arg parsing related code into sys/unix windows parses gpu code quite differently. The cl also move some unix only(because they expect the file /dev/kvm to be around) tests. BUG=b:213146388 TEST=presubmit Change-Id: I1f9b11485e09808f4748c8b49d0bc8b29e8ca525 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3732374 Tested-by: kokoro Reviewed-by: Daniel Verkamp Commit-Queue: Vikram Auradkar --- src/crosvm/cmdline.rs | 2 + src/crosvm/config.rs | 699 +-------------------------------- src/crosvm/sys/unix/config.rs | 714 ++++++++++++++++++++++++++++++++++ 3 files changed, 719 insertions(+), 696 deletions(-) diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index fac33608ba..53bdd64220 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -13,6 +13,8 @@ use super::platform::GpuRenderServerParameters; use super::sys::config::parse_coiommu_params; #[cfg(all(feature = "gpu", feature = "virgl_renderer_next"))] use super::sys::config::parse_gpu_render_server_options; +#[cfg(feature = "gpu")] +use super::sys::config::{parse_gpu_display_options, parse_gpu_options}; #[cfg(any(feature = "video-decoder", feature = "video-encoder"))] use super::config::parse_video_options; diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs index ad5b8bc296..940caf7705 100644 --- a/src/crosvm/config.rs +++ b/src/crosvm/config.rs @@ -1324,350 +1324,6 @@ pub fn parse_stub_pci_parameters(s: &str) -> Result { Ok(params) } -#[cfg(feature = "gpu")] -#[derive(Default)] -struct GpuDisplayParametersBuilder { - width: Option, - height: Option, - args: Vec, -} - -#[cfg(feature = "gpu")] -impl GpuDisplayParametersBuilder { - fn parse(&mut self, arg: &str) -> argument::Result<()> { - let mut kv = arg.split('='); - let k = kv.next().unwrap_or(""); - let v = kv.next().unwrap_or(""); - match k { - "width" => { - let width = v - .parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'width' must be a valid integer"), - })?; - self.width = Some(width); - } - "height" => { - let height = v - .parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'height' must be a valid integer"), - })?; - self.height = Some(height); - } - _ => { - return Err(argument::Error::UnknownArgument(format!( - "gpu-display parameter {}", - k - ))) - } - } - self.args.push(arg.to_string()); - Ok(()) - } - - fn build(&self) -> Result, String> { - match (self.width, self.height) { - (None, None) => Ok(None), - (None, Some(_)) | (Some(_), None) => { - let mut value = self - .args - .clone() - .into_iter() - .fold(String::new(), |args_so_far, arg| args_so_far + &arg + ","); - value.pop(); - return Err(invalid_value_err( - value, - "gpu must include both 'width' and 'height' if either is supplied", - )); - } - (Some(width), Some(height)) => Ok(Some(GpuDisplayParameters { width, height })), - } - } -} - -#[cfg(feature = "gpu")] -pub fn parse_gpu_options(s: &str) -> Result { - use devices::virtio::GpuMode; - use rutabaga_gfx::RutabagaWsi; - - use crate::crosvm::sys::config::is_gpu_backend_deprecated; - - #[cfg(feature = "gfxstream")] - let mut vulkan_specified = false; - #[cfg(feature = "gfxstream")] - let mut syncfd_specified = false; - #[cfg(feature = "gfxstream")] - let mut angle_specified = false; - - let mut display_param_builder: GpuDisplayParametersBuilder = Default::default(); - let mut gpu_params = GpuParameters::default(); - - for frag in s.split(',') { - let mut rest: Option<&str> = None; - let mut kv = frag.split('='); - let k = kv.next().unwrap_or(""); - let v = kv.next().unwrap_or(""); - match k { - // Deprecated: Specifying --gpu= Not great as the mode can be set multiple - // times if the user specifies several modes (--gpu=2d,virglrenderer,gfxstream) - "2d" | "2D" => { - gpu_params.mode = GpuMode::Mode2D; - } - "3d" | "3D" | "virglrenderer" => { - gpu_params.mode = GpuMode::ModeVirglRenderer; - } - #[cfg(feature = "gfxstream")] - "gfxstream" => { - gpu_params.mode = GpuMode::ModeGfxstream; - } - // Preferred: Specifying --gpu,backend= - "backend" => match v { - "2d" | "2D" => { - if is_gpu_backend_deprecated(v) { - return Err(invalid_value_err( - v, - "this backend type is deprecated, please use gfxstream.", - )); - } else { - gpu_params.mode = GpuMode::Mode2D; - } - } - "3d" | "3D" | "virglrenderer" => { - if is_gpu_backend_deprecated(v) { - return Err(invalid_value_err( - v, - "this backend type is deprecated, please use gfxstream.", - )); - } else { - gpu_params.mode = GpuMode::ModeVirglRenderer; - } - } - #[cfg(feature = "gfxstream")] - "gfxstream" => { - gpu_params.mode = GpuMode::ModeGfxstream; - } - _ => { - return Err(invalid_value_err( - v, - #[cfg(feature = "gfxstream")] - "gpu parameter 'backend' should be one of (2d|virglrenderer|gfxstream)", - #[cfg(not(feature = "gfxstream"))] - "gpu parameter 'backend' should be one of (2d|3d)", - )); - } - }, - "egl" => match v { - "true" | "" => { - gpu_params.renderer_use_egl = true; - } - "false" => { - gpu_params.renderer_use_egl = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'egl' should be a boolean", - )); - } - }, - "gles" => match v { - "true" | "" => { - gpu_params.renderer_use_gles = true; - } - "false" => { - gpu_params.renderer_use_gles = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'gles' should be a boolean", - )); - } - }, - "glx" => match v { - "true" | "" => { - gpu_params.renderer_use_glx = true; - } - "false" => { - gpu_params.renderer_use_glx = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'glx' should be a boolean", - )); - } - }, - "surfaceless" => match v { - "true" | "" => { - gpu_params.renderer_use_surfaceless = true; - } - "false" => { - gpu_params.renderer_use_surfaceless = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'surfaceless' should be a boolean", - )); - } - }, - #[cfg(feature = "gfxstream")] - "syncfd" => { - syncfd_specified = true; - match v { - "true" | "" => { - gpu_params.gfxstream_use_syncfd = true; - } - "false" => { - gpu_params.gfxstream_use_syncfd = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'syncfd' should be a boolean"), - }); - } - } - } - #[cfg(feature = "gfxstream")] - "angle" => { - angle_specified = true; - match v { - "true" | "" => { - gpu_params.gfxstream_use_guest_angle = true; - } - "false" => { - gpu_params.gfxstream_use_guest_angle = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'angle' should be a boolean", - )); - } - } - } - "vulkan" => { - #[cfg(feature = "gfxstream")] - { - vulkan_specified = true; - } - match v { - "true" | "" => { - gpu_params.use_vulkan = true; - } - "false" => { - gpu_params.use_vulkan = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'vulkan' should be a boolean", - )); - } - } - } - "wsi" => match v { - "vk" => { - gpu_params.wsi = Some(RutabagaWsi::Vulkan); - } - _ => { - return Err(invalid_value_err(v, "gpu parameter 'wsi' should be vk")); - } - }, - "cache-path" => gpu_params.cache_path = Some(v.to_string()), - "cache-size" => gpu_params.cache_size = Some(v.to_string()), - "pci-bar-size" => { - let size = parse_hex_or_decimal(v).map_err(|_| { - "gpu parameter `pci-bar-size` must be a valid hex or decimal value" - })?; - gpu_params.pci_bar_size = size; - } - "udmabuf" => match v { - "true" | "" => { - gpu_params.udmabuf = true; - } - "false" => { - gpu_params.udmabuf = false; - } - _ => { - return Err(invalid_value_err( - v, - "gpu parameter 'udmabuf' should be a boolean", - )); - } - }, - "context-types" => { - let context_types: Vec = v.split(':').map(|s| s.to_string()).collect(); - gpu_params.context_mask = rutabaga_gfx::calculate_context_mask(context_types); - } - "" => {} - _ => { - rest = Some(frag); - } - } - if let Some(arg) = rest.take() { - match display_param_builder.parse(arg) { - Ok(()) => {} - Err(argument::Error::UnknownArgument(_)) => { - rest = Some(arg); - } - Err(err) => return Err(err.to_string()), - } - } - if let Some(arg) = rest.take() { - return Err(format!("unknown gpu parameter {}", arg)); - } - } - - if let Some(display_param) = display_param_builder.build()?.take() { - gpu_params.displays.push(display_param); - } - - #[cfg(feature = "gfxstream")] - { - if !vulkan_specified && gpu_params.mode == GpuMode::ModeGfxstream { - gpu_params.use_vulkan = sys::use_vulkan(); - } - - if syncfd_specified || angle_specified { - match gpu_params.mode { - GpuMode::ModeGfxstream => {} - _ => { - return Err( - "gpu parameter syncfd and angle are only supported for gfxstream backend" - .to_string(), - ); - } - } - } - } - - Ok(gpu_params) -} - -#[cfg(feature = "gpu")] -pub fn parse_gpu_display_options(s: &str) -> Result { - let mut display_param_builder: GpuDisplayParametersBuilder = Default::default(); - - for arg in s.split(',') { - display_param_builder - .parse(arg) - .map_err(|e| e.to_string())?; - } - - let display_param = display_param_builder.build()?; - let display_param = display_param.ok_or_else(|| { - invalid_value_err(s, "gpu-display must include both 'width' and 'height'") - })?; - - Ok(display_param) -} - /// Aggregate of all configurable options for a running VM. #[remain::sorted] pub struct Config { @@ -2276,20 +1932,6 @@ mod tests { .expect_err("parse should have failed"); } - #[cfg(feature = "audio_cras")] - #[test] - fn parse_ac97_socket_type() { - parse_ac97_options("socket_type=unified").expect("parse should have succeded"); - parse_ac97_options("socket_type=legacy").expect("parse should have succeded"); - } - - #[cfg(feature = "audio")] - #[test] - fn parse_ac97_vios_valid() { - parse_ac97_options("backend=vios,server=/path/to/server") - .expect("parse should have succeded"); - } - #[test] fn parse_serial_vaild() { parse_serial_options("type=syslog,num=1,console=true,stdin=true") @@ -2362,30 +2004,6 @@ mod tests { .is_err()) } - #[test] - fn parse_plugin_mount_valid() { - let opt: BindMount = "/dev/null:/dev/zero:true".parse().unwrap(); - - assert_eq!(opt.src, PathBuf::from("/dev/null")); - assert_eq!(opt.dst, PathBuf::from("/dev/zero")); - assert!(opt.writable); - } - - #[test] - fn parse_plugin_mount_valid_shorthand() { - let opt: BindMount = "/dev/null".parse().unwrap(); - assert_eq!(opt.dst, PathBuf::from("/dev/null")); - assert!(!opt.writable); - - let opt: BindMount = "/dev/null:/dev/zero".parse().unwrap(); - assert_eq!(opt.dst, PathBuf::from("/dev/zero")); - assert!(!opt.writable); - - let opt: BindMount = "/dev/null::true".parse().unwrap(); - assert_eq!(opt.dst, PathBuf::from("/dev/null")); - assert!(opt.writable); - } - #[test] fn parse_plugin_mount_invalid() { "".parse::().expect_err("parse should fail"); @@ -2404,6 +2022,7 @@ mod tests { .expect_err("parse should fail because flag is not boolean"); } + #[cfg(feature = "plugin")] #[test] fn parse_plugin_gid_map_valid() { let opt: GidMap = "1:2:3".parse().expect("parse should succeed"); @@ -2412,6 +2031,7 @@ mod tests { assert_eq!(opt.count, 3); } + #[cfg(feature = "plugin")] #[test] fn parse_plugin_gid_map_valid_shorthand() { let opt: GidMap = "1".parse().expect("parse should succeed"); @@ -2430,6 +2050,7 @@ mod tests { assert_eq!(opt.count, 3); } + #[cfg(feature = "plugin")] #[test] fn parse_plugin_gid_map_invalid() { "".parse::().expect_err("parse should fail"); @@ -2447,243 +2068,6 @@ mod tests { .expect_err("parse should fail because count is not a number"); } - #[test] - fn single_touch_spec_and_track_pad_spec_default_size() { - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--single-touch", - "/dev/single-touch-test", - "--trackpad", - "/dev/single-touch-test", - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - assert_eq!( - config.virtio_single_touch.first().unwrap().get_size(), - (DEFAULT_TOUCH_DEVICE_WIDTH, DEFAULT_TOUCH_DEVICE_HEIGHT) - ); - assert_eq!( - config.virtio_trackpad.first().unwrap().get_size(), - (DEFAULT_TOUCH_DEVICE_WIDTH, DEFAULT_TOUCH_DEVICE_HEIGHT) - ); - } - - #[cfg(feature = "gpu")] - #[test] - fn single_touch_spec_default_size_from_gpu() { - let width = 12345u32; - let height = 54321u32; - - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--single-touch", - "/dev/single-touch-test", - "--gpu", - &format!("width={},height={}", width, height), - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - assert_eq!( - config.virtio_single_touch.first().unwrap().get_size(), - (width, height) - ); - } - - #[test] - fn single_touch_spec_and_track_pad_spec_with_size() { - let width = 12345u32; - let height = 54321u32; - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--single-touch", - &format!("/dev/single-touch-test:{}:{}", width, height), - "--trackpad", - &format!("/dev/single-touch-test:{}:{}", width, height), - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - assert_eq!( - config.virtio_single_touch.first().unwrap().get_size(), - (width, height) - ); - assert_eq!( - config.virtio_trackpad.first().unwrap().get_size(), - (width, height) - ); - } - - #[cfg(feature = "gpu")] - #[test] - fn single_touch_spec_with_size_independent_from_gpu() { - let touch_width = 12345u32; - let touch_height = 54321u32; - let display_width = 1234u32; - let display_height = 5432u32; - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--single-touch", - &format!("/dev/single-touch-test:{}:{}", touch_width, touch_height), - "--gpu", - &format!("width={},height={}", display_width, display_height), - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - assert_eq!( - config.virtio_single_touch.first().unwrap().get_size(), - (touch_width, touch_height) - ); - } - - #[test] - fn virtio_switches() { - let mut config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &["--switches", "/dev/switches-test", "/dev/null"], - ) - .unwrap() - .try_into() - .unwrap(); - - assert_eq!( - config.virtio_switches.pop().unwrap(), - PathBuf::from("/dev/switches-test") - ); - } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_options_default_vulkan_support() { - { - let gpu_params: GpuParameters = parse_gpu_options("backend=virglrenderer").unwrap(); - assert!(!gpu_params.use_vulkan); - } - - #[cfg(feature = "gfxstream")] - { - let gpu_params: GpuParameters = parse_gpu_options("backend=gfxstream").unwrap(); - assert!(gpu_params.use_vulkan); - } - } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_options_with_vulkan_specified() { - { - let gpu_params: GpuParameters = parse_gpu_options("vulkan=true").unwrap(); - assert!(gpu_params.use_vulkan); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("backend=virglrenderer,vulkan=true").unwrap(); - assert!(gpu_params.use_vulkan); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("vulkan=true,backend=virglrenderer").unwrap(); - assert!(gpu_params.use_vulkan); - } - { - let gpu_params: GpuParameters = parse_gpu_options("vulkan=false").unwrap(); - assert!(!gpu_params.use_vulkan); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("backend=virglrenderer,vulkan=false").unwrap(); - assert!(!gpu_params.use_vulkan); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("vulkan=false,backend=virglrenderer").unwrap(); - assert!(!gpu_params.use_vulkan); - } - { - assert!(parse_gpu_options("backend=virglrenderer,vulkan=invalid_value").is_err()); - } - { - assert!(parse_gpu_options("vulkan=invalid_value,backend=virglrenderer").is_err()); - } - } - - #[cfg(all(feature = "gpu", feature = "gfxstream"))] - #[test] - fn parse_gpu_options_gfxstream_with_syncfd_specified() { - { - let gpu_params: GpuParameters = - parse_gpu_options("backend=gfxstream,syncfd=true").unwrap(); - - assert!(gpu_params.gfxstream_use_syncfd); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("syncfd=true,backend=gfxstream").unwrap(); - assert!(gpu_params.gfxstream_use_syncfd); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("backend=gfxstream,syncfd=false").unwrap(); - - assert!(!gpu_params.gfxstream_use_syncfd); - } - { - let gpu_params: GpuParameters = - parse_gpu_options("syncfd=false,backend=gfxstream").unwrap(); - assert!(!gpu_params.gfxstream_use_syncfd); - } - { - assert!(parse_gpu_options("backend=gfxstream,syncfd=invalid_value").is_err()); - } - { - assert!(parse_gpu_options("syncfd=invalid_value,backend=gfxstream").is_err()); - } - } - - #[cfg(all(feature = "gpu", feature = "gfxstream"))] - #[test] - fn parse_gpu_options_not_gfxstream_with_syncfd_specified() { - { - assert!(parse_gpu_options("backend=virglrenderer,syncfd=true").is_err()); - } - { - assert!(parse_gpu_options("syncfd=true,backend=virglrenderer").is_err()); - } - } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_options_gfxstream_with_wsi_specified() { - use rutabaga_gfx::RutabagaWsi; - - let gpu_params: GpuParameters = parse_gpu_options("backend=virglrenderer,wsi=vk").unwrap(); - assert!(matches!(gpu_params.wsi, Some(RutabagaWsi::Vulkan))); - - let gpu_params: GpuParameters = parse_gpu_options("wsi=vk,backend=virglrenderer").unwrap(); - assert!(matches!(gpu_params.wsi, Some(RutabagaWsi::Vulkan))); - - assert!(parse_gpu_options("backend=virglrenderer,wsi=invalid_value").is_err()); - - assert!(parse_gpu_options("wsi=invalid_value,backend=virglrenderer").is_err()); - } - #[test] fn parse_battery_vaild() { parse_battery_options("type=goldfish").expect("parse should have succeded"); @@ -2846,81 +2230,4 @@ mod tests { assert!(parse_userspace_msr_options("0x10").is_err()); assert!(parse_userspace_msr_options("hoge").is_err()); } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_display_options_valid() { - { - let gpu_params: GpuDisplayParameters = - parse_gpu_display_options("width=500,height=600").unwrap(); - assert_eq!(gpu_params.width, 500); - assert_eq!(gpu_params.height, 600); - } - } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_display_options_invalid() { - { - assert!(parse_gpu_display_options("width=500").is_err()); - } - { - assert!(parse_gpu_display_options("height=500").is_err()); - } - { - assert!(parse_gpu_display_options("width").is_err()); - } - { - assert!(parse_gpu_display_options("blah").is_err()); - } - } - - #[cfg(feature = "gpu")] - #[test] - fn parse_gpu_options_and_gpu_display_options_valid() { - { - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--gpu", - "2D,width=500,height=600", - "--gpu-display", - "width=700,height=800", - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - let gpu_params = config.gpu_parameters.unwrap(); - - assert_eq!(gpu_params.displays.len(), 2); - assert_eq!(gpu_params.displays[0].width, 500); - assert_eq!(gpu_params.displays[0].height, 600); - assert_eq!(gpu_params.displays[1].width, 700); - assert_eq!(gpu_params.displays[1].height, 800); - } - { - let config: Config = crate::crosvm::cmdline::RunCommand::from_args( - &[], - &[ - "--gpu", - "2D", - "--gpu-display", - "width=700,height=800", - "/dev/null", - ], - ) - .unwrap() - .try_into() - .unwrap(); - - let gpu_params = config.gpu_parameters.unwrap(); - - assert_eq!(gpu_params.displays.len(), 1); - assert_eq!(gpu_params.displays[0].width, 700); - assert_eq!(gpu_params.displays[0].height, 800); - } - } } diff --git a/src/crosvm/sys/unix/config.rs b/src/crosvm/sys/unix/config.rs index c9730380db..cece270944 100644 --- a/src/crosvm/sys/unix/config.rs +++ b/src/crosvm/sys/unix/config.rs @@ -6,7 +6,12 @@ use std::time::Duration; use devices::SerialParameters; +#[cfg(feature = "gpu")] +use devices::virtio::{GpuDisplayParameters, GpuParameters}; + use crate::crosvm::config::Config; +#[cfg(feature = "gpu")] +use crate::crosvm::{argument, argument::parse_hex_or_decimal, config::invalid_value_err}; #[cfg(all(feature = "gpu", feature = "virgl_renderer_next"))] pub fn parse_gpu_render_server_options( @@ -182,3 +187,712 @@ pub fn validate_config(cfg: &mut Config) -> std::result::Result<(), String> { Ok(()) } + +#[cfg(feature = "gpu")] +#[derive(Default)] +struct GpuDisplayParametersBuilder { + width: Option, + height: Option, + args: Vec, +} + +#[cfg(feature = "gpu")] +impl GpuDisplayParametersBuilder { + fn parse(&mut self, arg: &str) -> argument::Result<()> { + let mut kv = arg.split('='); + let k = kv.next().unwrap_or(""); + let v = kv.next().unwrap_or(""); + match k { + "width" => { + let width = v + .parse::() + .map_err(|_| argument::Error::InvalidValue { + value: v.to_string(), + expected: String::from("gpu parameter 'width' must be a valid integer"), + })?; + self.width = Some(width); + } + "height" => { + let height = v + .parse::() + .map_err(|_| argument::Error::InvalidValue { + value: v.to_string(), + expected: String::from("gpu parameter 'height' must be a valid integer"), + })?; + self.height = Some(height); + } + _ => { + return Err(argument::Error::UnknownArgument(format!( + "gpu-display parameter {}", + k + ))) + } + } + self.args.push(arg.to_string()); + Ok(()) + } + + fn build(&self) -> Result, String> { + match (self.width, self.height) { + (None, None) => Ok(None), + (None, Some(_)) | (Some(_), None) => { + let mut value = self + .args + .clone() + .into_iter() + .fold(String::new(), |args_so_far, arg| args_so_far + &arg + ","); + value.pop(); + return Err(invalid_value_err( + value, + "gpu must include both 'width' and 'height' if either is supplied", + )); + } + (Some(width), Some(height)) => Ok(Some(GpuDisplayParameters { width, height })), + } + } +} + +#[cfg(feature = "gpu")] +pub fn parse_gpu_options(s: &str) -> Result { + use devices::virtio::GpuMode; + use rutabaga_gfx::RutabagaWsi; + + use crate::crosvm::sys::config::is_gpu_backend_deprecated; + + #[cfg(feature = "gfxstream")] + let mut vulkan_specified = false; + #[cfg(feature = "gfxstream")] + let mut syncfd_specified = false; + #[cfg(feature = "gfxstream")] + let mut angle_specified = false; + + let mut display_param_builder: GpuDisplayParametersBuilder = Default::default(); + let mut gpu_params = GpuParameters::default(); + + for frag in s.split(',') { + let mut rest: Option<&str> = None; + let mut kv = frag.split('='); + let k = kv.next().unwrap_or(""); + let v = kv.next().unwrap_or(""); + match k { + // Deprecated: Specifying --gpu= Not great as the mode can be set multiple + // times if the user specifies several modes (--gpu=2d,virglrenderer,gfxstream) + "2d" | "2D" => { + gpu_params.mode = GpuMode::Mode2D; + } + "3d" | "3D" | "virglrenderer" => { + gpu_params.mode = GpuMode::ModeVirglRenderer; + } + #[cfg(feature = "gfxstream")] + "gfxstream" => { + gpu_params.mode = GpuMode::ModeGfxstream; + } + // Preferred: Specifying --gpu,backend= + "backend" => match v { + "2d" | "2D" => { + if is_gpu_backend_deprecated(v) { + return Err(invalid_value_err( + v, + "this backend type is deprecated, please use gfxstream.", + )); + } else { + gpu_params.mode = GpuMode::Mode2D; + } + } + "3d" | "3D" | "virglrenderer" => { + if is_gpu_backend_deprecated(v) { + return Err(invalid_value_err( + v, + "this backend type is deprecated, please use gfxstream.", + )); + } else { + gpu_params.mode = GpuMode::ModeVirglRenderer; + } + } + #[cfg(feature = "gfxstream")] + "gfxstream" => { + gpu_params.mode = GpuMode::ModeGfxstream; + } + _ => { + return Err(invalid_value_err( + v, + #[cfg(feature = "gfxstream")] + "gpu parameter 'backend' should be one of (2d|virglrenderer|gfxstream)", + #[cfg(not(feature = "gfxstream"))] + "gpu parameter 'backend' should be one of (2d|3d)", + )); + } + }, + "egl" => match v { + "true" | "" => { + gpu_params.renderer_use_egl = true; + } + "false" => { + gpu_params.renderer_use_egl = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'egl' should be a boolean", + )); + } + }, + "gles" => match v { + "true" | "" => { + gpu_params.renderer_use_gles = true; + } + "false" => { + gpu_params.renderer_use_gles = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'gles' should be a boolean", + )); + } + }, + "glx" => match v { + "true" | "" => { + gpu_params.renderer_use_glx = true; + } + "false" => { + gpu_params.renderer_use_glx = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'glx' should be a boolean", + )); + } + }, + "surfaceless" => match v { + "true" | "" => { + gpu_params.renderer_use_surfaceless = true; + } + "false" => { + gpu_params.renderer_use_surfaceless = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'surfaceless' should be a boolean", + )); + } + }, + #[cfg(feature = "gfxstream")] + "syncfd" => { + syncfd_specified = true; + match v { + "true" | "" => { + gpu_params.gfxstream_use_syncfd = true; + } + "false" => { + gpu_params.gfxstream_use_syncfd = false; + } + _ => { + return Err(argument::Error::InvalidValue { + value: v.to_string(), + expected: String::from("gpu parameter 'syncfd' should be a boolean"), + }); + } + } + } + #[cfg(feature = "gfxstream")] + "angle" => { + angle_specified = true; + match v { + "true" | "" => { + gpu_params.gfxstream_use_guest_angle = true; + } + "false" => { + gpu_params.gfxstream_use_guest_angle = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'angle' should be a boolean", + )); + } + } + } + "vulkan" => { + #[cfg(feature = "gfxstream")] + { + vulkan_specified = true; + } + match v { + "true" | "" => { + gpu_params.use_vulkan = true; + } + "false" => { + gpu_params.use_vulkan = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'vulkan' should be a boolean", + )); + } + } + } + "wsi" => match v { + "vk" => { + gpu_params.wsi = Some(RutabagaWsi::Vulkan); + } + _ => { + return Err(invalid_value_err(v, "gpu parameter 'wsi' should be vk")); + } + }, + "cache-path" => gpu_params.cache_path = Some(v.to_string()), + "cache-size" => gpu_params.cache_size = Some(v.to_string()), + "pci-bar-size" => { + let size = parse_hex_or_decimal(v).map_err(|_| { + "gpu parameter `pci-bar-size` must be a valid hex or decimal value" + })?; + gpu_params.pci_bar_size = size; + } + "udmabuf" => match v { + "true" | "" => { + gpu_params.udmabuf = true; + } + "false" => { + gpu_params.udmabuf = false; + } + _ => { + return Err(invalid_value_err( + v, + "gpu parameter 'udmabuf' should be a boolean", + )); + } + }, + "context-types" => { + let context_types: Vec = v.split(':').map(|s| s.to_string()).collect(); + gpu_params.context_mask = rutabaga_gfx::calculate_context_mask(context_types); + } + "" => {} + _ => { + rest = Some(frag); + } + } + if let Some(arg) = rest.take() { + match display_param_builder.parse(arg) { + Ok(()) => {} + Err(argument::Error::UnknownArgument(_)) => { + rest = Some(arg); + } + Err(err) => return Err(err.to_string()), + } + } + if let Some(arg) = rest.take() { + return Err(format!("unknown gpu parameter {}", arg)); + } + } + + if let Some(display_param) = display_param_builder.build()?.take() { + gpu_params.displays.push(display_param); + } + + #[cfg(feature = "gfxstream")] + { + if !vulkan_specified && gpu_params.mode == GpuMode::ModeGfxstream { + gpu_params.use_vulkan = sys::use_vulkan(); + } + + if syncfd_specified || angle_specified { + match gpu_params.mode { + GpuMode::ModeGfxstream => {} + _ => { + return Err( + "gpu parameter syncfd and angle are only supported for gfxstream backend" + .to_string(), + ); + } + } + } + } + + Ok(gpu_params) +} + +#[cfg(feature = "gpu")] +pub fn parse_gpu_display_options(s: &str) -> Result { + let mut display_param_builder: GpuDisplayParametersBuilder = Default::default(); + + for arg in s.split(',') { + display_param_builder + .parse(arg) + .map_err(|e| e.to_string())?; + } + + let display_param = display_param_builder.build()?; + let display_param = display_param.ok_or_else(|| { + invalid_value_err(s, "gpu-display must include both 'width' and 'height'") + })?; + + Ok(display_param) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::crosvm::config::{DEFAULT_TOUCH_DEVICE_HEIGHT, DEFAULT_TOUCH_DEVICE_WIDTH}; + + use argh::FromArgs; + use std::path::PathBuf; + + #[cfg(feature = "audio")] + use crate::crosvm::config::parse_ac97_options; + use crate::crosvm::config::BindMount; + + #[cfg(feature = "audio_cras")] + #[test] + fn parse_ac97_socket_type() { + parse_ac97_options("socket_type=unified").expect("parse should have succeded"); + parse_ac97_options("socket_type=legacy").expect("parse should have succeded"); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_default_vulkan_support() { + { + let gpu_params: GpuParameters = parse_gpu_options("backend=virglrenderer").unwrap(); + assert!(!gpu_params.use_vulkan); + } + + #[cfg(feature = "gfxstream")] + { + let gpu_params: GpuParameters = parse_gpu_options("backend=gfxstream").unwrap(); + assert!(gpu_params.use_vulkan); + } + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_with_vulkan_specified() { + { + let gpu_params: GpuParameters = parse_gpu_options("vulkan=true").unwrap(); + assert!(gpu_params.use_vulkan); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("backend=virglrenderer,vulkan=true").unwrap(); + assert!(gpu_params.use_vulkan); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("vulkan=true,backend=virglrenderer").unwrap(); + assert!(gpu_params.use_vulkan); + } + { + let gpu_params: GpuParameters = parse_gpu_options("vulkan=false").unwrap(); + assert!(!gpu_params.use_vulkan); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("backend=virglrenderer,vulkan=false").unwrap(); + assert!(!gpu_params.use_vulkan); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("vulkan=false,backend=virglrenderer").unwrap(); + assert!(!gpu_params.use_vulkan); + } + { + assert!(parse_gpu_options("backend=virglrenderer,vulkan=invalid_value").is_err()); + } + { + assert!(parse_gpu_options("vulkan=invalid_value,backend=virglrenderer").is_err()); + } + } + + #[cfg(all(feature = "gpu", feature = "gfxstream"))] + #[test] + fn parse_gpu_options_gfxstream_with_syncfd_specified() { + { + let gpu_params: GpuParameters = + parse_gpu_options("backend=gfxstream,syncfd=true").unwrap(); + + assert!(gpu_params.gfxstream_use_syncfd); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("syncfd=true,backend=gfxstream").unwrap(); + assert!(gpu_params.gfxstream_use_syncfd); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("backend=gfxstream,syncfd=false").unwrap(); + + assert!(!gpu_params.gfxstream_use_syncfd); + } + { + let gpu_params: GpuParameters = + parse_gpu_options("syncfd=false,backend=gfxstream").unwrap(); + assert!(!gpu_params.gfxstream_use_syncfd); + } + { + assert!(parse_gpu_options("backend=gfxstream,syncfd=invalid_value").is_err()); + } + { + assert!(parse_gpu_options("syncfd=invalid_value,backend=gfxstream").is_err()); + } + } + + #[cfg(all(feature = "gpu", feature = "gfxstream"))] + #[test] + fn parse_gpu_options_not_gfxstream_with_syncfd_specified() { + { + assert!(parse_gpu_options("backend=virglrenderer,syncfd=true").is_err()); + } + { + assert!(parse_gpu_options("syncfd=true,backend=virglrenderer").is_err()); + } + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_gfxstream_with_wsi_specified() { + use rutabaga_gfx::RutabagaWsi; + + let gpu_params: GpuParameters = parse_gpu_options("backend=virglrenderer,wsi=vk").unwrap(); + assert!(matches!(gpu_params.wsi, Some(RutabagaWsi::Vulkan))); + + let gpu_params: GpuParameters = parse_gpu_options("wsi=vk,backend=virglrenderer").unwrap(); + assert!(matches!(gpu_params.wsi, Some(RutabagaWsi::Vulkan))); + + assert!(parse_gpu_options("backend=virglrenderer,wsi=invalid_value").is_err()); + + assert!(parse_gpu_options("wsi=invalid_value,backend=virglrenderer").is_err()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_display_options_valid() { + { + let gpu_params: GpuDisplayParameters = + parse_gpu_display_options("width=500,height=600").unwrap(); + assert_eq!(gpu_params.width, 500); + assert_eq!(gpu_params.height, 600); + } + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_display_options_invalid() { + { + assert!(parse_gpu_display_options("width=500").is_err()); + } + { + assert!(parse_gpu_display_options("height=500").is_err()); + } + { + assert!(parse_gpu_display_options("width").is_err()); + } + { + assert!(parse_gpu_display_options("blah").is_err()); + } + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_and_gpu_display_options_valid() { + { + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + "2D,width=500,height=600", + "--gpu-display", + "width=700,height=800", + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + let gpu_params = config.gpu_parameters.unwrap(); + + assert_eq!(gpu_params.displays.len(), 2); + assert_eq!(gpu_params.displays[0].width, 500); + assert_eq!(gpu_params.displays[0].height, 600); + assert_eq!(gpu_params.displays[1].width, 700); + assert_eq!(gpu_params.displays[1].height, 800); + } + { + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + "2D", + "--gpu-display", + "width=700,height=800", + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + let gpu_params = config.gpu_parameters.unwrap(); + + assert_eq!(gpu_params.displays.len(), 1); + assert_eq!(gpu_params.displays[0].width, 700); + assert_eq!(gpu_params.displays[0].height, 800); + } + } + + #[cfg(feature = "audio")] + #[test] + fn parse_ac97_vios_valid() { + parse_ac97_options("backend=vios,server=/path/to/server") + .expect("parse should have succeded"); + } + + #[test] + fn parse_plugin_mount_valid() { + let opt: BindMount = "/dev/null:/dev/zero:true".parse().unwrap(); + + assert_eq!(opt.src, PathBuf::from("/dev/null")); + assert_eq!(opt.dst, PathBuf::from("/dev/zero")); + assert!(opt.writable); + } + + #[test] + fn parse_plugin_mount_valid_shorthand() { + let opt: BindMount = "/dev/null".parse().unwrap(); + assert_eq!(opt.dst, PathBuf::from("/dev/null")); + assert!(!opt.writable); + + let opt: BindMount = "/dev/null:/dev/zero".parse().unwrap(); + assert_eq!(opt.dst, PathBuf::from("/dev/zero")); + assert!(!opt.writable); + + let opt: BindMount = "/dev/null::true".parse().unwrap(); + assert_eq!(opt.dst, PathBuf::from("/dev/null")); + assert!(opt.writable); + } + + #[test] + fn single_touch_spec_and_track_pad_spec_default_size() { + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--single-touch", + "/dev/single-touch-test", + "--trackpad", + "/dev/single-touch-test", + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + assert_eq!( + config.virtio_single_touch.first().unwrap().get_size(), + (DEFAULT_TOUCH_DEVICE_WIDTH, DEFAULT_TOUCH_DEVICE_HEIGHT) + ); + assert_eq!( + config.virtio_trackpad.first().unwrap().get_size(), + (DEFAULT_TOUCH_DEVICE_WIDTH, DEFAULT_TOUCH_DEVICE_HEIGHT) + ); + } + + #[cfg(feature = "gpu")] + #[test] + fn single_touch_spec_default_size_from_gpu() { + let width = 12345u32; + let height = 54321u32; + + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--single-touch", + "/dev/single-touch-test", + "--gpu", + &format!("width={},height={}", width, height), + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + assert_eq!( + config.virtio_single_touch.first().unwrap().get_size(), + (width, height) + ); + } + + #[test] + fn single_touch_spec_and_track_pad_spec_with_size() { + let width = 12345u32; + let height = 54321u32; + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--single-touch", + &format!("/dev/single-touch-test:{}:{}", width, height), + "--trackpad", + &format!("/dev/single-touch-test:{}:{}", width, height), + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + assert_eq!( + config.virtio_single_touch.first().unwrap().get_size(), + (width, height) + ); + assert_eq!( + config.virtio_trackpad.first().unwrap().get_size(), + (width, height) + ); + } + + #[cfg(feature = "gpu")] + #[test] + fn single_touch_spec_with_size_independent_from_gpu() { + let touch_width = 12345u32; + let touch_height = 54321u32; + let display_width = 1234u32; + let display_height = 5432u32; + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--single-touch", + &format!("/dev/single-touch-test:{}:{}", touch_width, touch_height), + "--gpu", + &format!("width={},height={}", display_width, display_height), + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + assert_eq!( + config.virtio_single_touch.first().unwrap().get_size(), + (touch_width, touch_height) + ); + } + + #[test] + fn virtio_switches() { + let mut config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &["--switches", "/dev/switches-test", "/dev/null"], + ) + .unwrap() + .try_into() + .unwrap(); + + assert_eq!( + config.virtio_switches.pop().unwrap(), + PathBuf::from("/dev/switches-test") + ); + } +} From 1c80e0621fe60ed8c697bac5545bd7dc249da69c Mon Sep 17 00:00:00 2001 From: Junichi Uekawa Date: Tue, 21 Jun 2022 14:26:53 +0900 Subject: [PATCH 8/9] crosvm: Remove --no-legacy flag. This will land after references to --no-legacy flag disappears. BUG=b:236574949 TEST=boot volteer-manatee Change-Id: Ic2b8c601f7a0d193f52b8b096195341ca88f4ef2 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3715084 Reviewed-by: Daniel Verkamp Commit-Queue: Junichi Uekawa Tested-by: kokoro --- src/crosvm/cmdline.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index 53bdd64220..855805e269 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -685,10 +685,6 @@ pub struct RunCommand { #[argh(switch)] /// don't use legacy KBD devices emulation pub no_i8042: bool, - #[cfg(unix)] - #[argh(switch)] - /// don't use legacy KBD/RTC devices emulation - pub no_legacy: bool, #[argh(switch)] /// don't create RNG device in the guest pub no_rng: bool, @@ -1458,9 +1454,6 @@ impl TryFrom for super::config::Config { #[cfg(unix)] { - cfg.no_i8042 = cmd.no_legacy; - cfg.no_rtc = cmd.no_legacy; - if cmd.vhost_vsock_device.is_some() && cmd.vhost_vsock_fd.is_some() { return Err( "Only one of vhost-vsock-device vhost-vsock-fd has to be specified".to_string(), @@ -1566,8 +1559,8 @@ impl TryFrom for super::config::Config { cfg.force_s2idle = cmd.s2idle; cfg.pcie_ecam = cmd.pcie_ecam; cfg.pci_low_start = cmd.pci_low_start; - cfg.no_i8042 |= cmd.no_i8042; - cfg.no_rtc |= cmd.no_rtc; + cfg.no_i8042 = cmd.no_i8042; + cfg.no_rtc = cmd.no_rtc; for (index, msr_config) in cmd.userspace_msr { if cfg.userspace_msr.insert(index, msr_config).is_some() { From 517bfb7acfa6006547eac8c445808b54085ff1b4 Mon Sep 17 00:00:00 2001 From: Kameron Lutes Date: Tue, 28 Jun 2022 05:35:14 +0000 Subject: [PATCH 9/9] crosvm_control: Add link to library docs Adds a link to the crosvm_control docs in the library source. BUG=b:236909264 TEST=cq Change-Id: I144e4a3823d20cea2c1087b71c2efa86e7029de3 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3729256 Tested-by: kokoro Reviewed-by: Keiichi Watanabe Reviewed-by: Dennis Kempin Commit-Queue: Kameron Lutes --- crosvm_control/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crosvm_control/src/lib.rs b/crosvm_control/src/lib.rs index a7e102a649..10cfabd906 100644 --- a/crosvm_control/src/lib.rs +++ b/crosvm_control/src/lib.rs @@ -6,6 +6,12 @@ //! //! This crate is a programmatic alternative to invoking crosvm with subcommands that produce the //! result on stdout. +//! +//! Downstream projects rely on this library maintaining a stable API surface. +//! Do not make changes to this library without consulting the crosvm externalization team. +//! Email: crosvm-dev@chromium.org +//! For more information see: +//! use std::convert::{TryFrom, TryInto}; use std::ffi::CStr;