devices: vsock: simplify device path to a PathBuf

The vhost-vsock device had some special-case code to handle a file
descriptor as the vhost device rather than a path. However, crosvm
already has base::open_file(), which has special handling for magic
/proc/self/fd/<N> paths and treats them as already-open file descriptors
without requiring a separate command-line option.

This change converts the vhost vsock device to accept only a path
instead of a special path/fd enum, using base::open_file() to support
passing already-open file descriptors via the /proc/self/fd path format.
The existing --vhost-vsock-fd option is kept for compatibility, but it
now just creates a /proc/self/fd path from the passed fd. Existing users
are encouraged to migrate to --vhost-vsock-device.

BUG=b:218223240
TEST=cargo test -p devices vsock::tests::params

Change-Id: Ifad2b7ad0824d4f24d9b12a4af1448557fadcdc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3564224
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
This commit is contained in:
Daniel Verkamp 2022-03-31 15:13:16 -07:00 committed by Chromeos LUCI
parent 91ad3eb689
commit 9975e79f78
4 changed files with 28 additions and 72 deletions

View file

@ -2,13 +2,13 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
use std::fs::{File, OpenOptions};
use std::os::unix::prelude::{FromRawFd, OpenOptionsExt};
use std::fs::OpenOptions;
use std::os::unix::prelude::OpenOptionsExt;
use std::path::PathBuf;
use std::thread;
use anyhow::Context;
use base::{error, validate_raw_descriptor, warn, AsRawDescriptor, Event, RawDescriptor};
use base::{error, open_file, warn, AsRawDescriptor, Event, RawDescriptor};
use data_model::{DataInit, Le64};
use serde::Deserialize;
use vhost::Vhost;
@ -27,24 +27,10 @@ static VHOST_VSOCK_DEFAULT_PATH: &str = "/dev/vhost-vsock";
#[derive(Debug, Deserialize, PartialEq)]
#[serde(deny_unknown_fields)]
pub struct VhostVsockConfig {
#[serde(default)]
pub device: VhostVsockDeviceParameter,
pub device: Option<PathBuf>,
pub cid: u64,
}
#[derive(Clone, Debug, Deserialize, PartialEq)]
#[serde(untagged)]
pub enum VhostVsockDeviceParameter {
Path(PathBuf),
Fd(RawDescriptor),
}
impl Default for VhostVsockDeviceParameter {
fn default() -> Self {
VhostVsockDeviceParameter::Path(PathBuf::from(VHOST_VSOCK_DEFAULT_PATH))
}
}
pub struct Vsock {
worker_kill_evt: Option<Event>,
kill_evt: Option<Event>,
@ -58,21 +44,17 @@ pub struct Vsock {
impl Vsock {
/// Create a new virtio-vsock device with the given VM cid.
pub fn new(base_features: u64, vhost_config: &VhostVsockConfig) -> anyhow::Result<Vsock> {
let device_file = match &vhost_config.device {
VhostVsockDeviceParameter::Fd(fd) => {
let fd = validate_raw_descriptor(*fd)
.context("failed to validate fd for virtual socket device")?;
// Safe because the `fd` is actually owned by this process and
// we have a unique handle to it.
unsafe { File::from_raw_fd(fd) }
}
VhostVsockDeviceParameter::Path(path) => OpenOptions::new()
let device_file = open_file(
vhost_config
.device
.as_ref()
.unwrap_or(&PathBuf::from(VHOST_VSOCK_DEFAULT_PATH)),
OpenOptions::new()
.read(true)
.write(true)
.custom_flags(libc::O_CLOEXEC | libc::O_NONBLOCK)
.open(path)
.context("failed to open virtual socket device")?,
};
.custom_flags(libc::O_CLOEXEC | libc::O_NONBLOCK),
)
.context("failed to open virtual socket device")?;
let kill_evt = Event::new().map_err(Error::CreateKillEvent)?;
let handle = VhostVsockHandle::new(device_file);
@ -338,30 +320,12 @@ mod tests {
#[test]
fn params_from_key_values() {
// Fd device
let params = from_vsock_arg("device=42,cid=56").unwrap();
assert_eq!(
params,
VhostVsockConfig {
device: VhostVsockDeviceParameter::Fd(42),
cid: 56,
}
);
// No key for fd device
let params = from_vsock_arg("42,cid=56").unwrap();
assert_eq!(
params,
VhostVsockConfig {
device: VhostVsockDeviceParameter::Fd(42),
cid: 56,
}
);
// Path device
let params = from_vsock_arg("device=/some/path,cid=56").unwrap();
assert_eq!(
params,
VhostVsockConfig {
device: VhostVsockDeviceParameter::Path("/some/path".into()),
device: Some("/some/path".into()),
cid: 56,
}
);
@ -370,7 +334,7 @@ mod tests {
assert_eq!(
params,
VhostVsockConfig {
device: VhostVsockDeviceParameter::Path("/some/path".into()),
device: Some("/some/path".into()),
cid: 56,
}
);
@ -379,7 +343,7 @@ mod tests {
assert_eq!(
params,
VhostVsockConfig {
device: VhostVsockDeviceParameter::Path(VHOST_VSOCK_DEFAULT_PATH.into()),
device: None,
cid: 56,
}
);

View file

@ -28,7 +28,6 @@ use devices::virtio::cras_backend::Parameters as CrasSndParameters;
use devices::virtio::fs::passthrough;
#[cfg(feature = "gpu")]
use devices::virtio::gpu::GpuParameters;
use devices::virtio::vhost::vsock::VhostVsockDeviceParameter;
#[cfg(any(feature = "video-decoder", feature = "video-encoder"))]
use devices::virtio::VideoBackendType;
#[cfg(feature = "audio")]
@ -355,7 +354,7 @@ impl Default for JailConfig {
/// Aggregate of all configurable options for a running VM.
pub struct Config {
pub kvm_device_path: PathBuf,
pub vhost_vsock_device: Option<VhostVsockDeviceParameter>,
pub vhost_vsock_device: Option<PathBuf>,
pub vhost_net_device_path: PathBuf,
pub vcpu_count: Option<usize>,
pub vcpu_cgroup_path: Option<PathBuf>,

View file

@ -23,7 +23,6 @@ use std::process;
#[cfg(all(target_arch = "x86_64", feature = "gdb"))]
use std::thread;
use devices::virtio::vhost::vsock::{VhostVsockConfig, VhostVsockDeviceParameter};
use libc;
use acpi_tables::sdt::SDT;
@ -34,6 +33,7 @@ use base::{UnixSeqpacket, UnixSeqpacketListener, UnlinkUnixSeqpacketListener};
use devices::serial_device::SerialHardware;
use devices::vfio::{VfioCommonSetup, VfioCommonTrait};
use devices::virtio::memory_mapper::MemoryMapperTrait;
use devices::virtio::vhost::vsock::VhostVsockConfig;
#[cfg(feature = "gpu")]
use devices::virtio::{self, EventDevice};
#[cfg(feature = "audio")]
@ -407,10 +407,7 @@ fn create_virtio_devices(
if let Some(cid) = cfg.cid {
let vhost_config = VhostVsockConfig {
device: cfg
.vhost_vsock_device
.clone()
.unwrap_or(VhostVsockDeviceParameter::default()),
device: cfg.vhost_vsock_device.clone(),
cid,
};
devs.push(create_vhost_vsock_device(cfg, &vhost_config)?);

View file

@ -44,7 +44,6 @@ use devices::virtio::vhost::user::device::{
run_block_device, run_console_device, run_fs_device, run_net_device, run_vsock_device,
run_wl_device,
};
use devices::virtio::vhost::vsock::VhostVsockDeviceParameter;
#[cfg(any(feature = "video-decoder", feature = "video-encoder"))]
use devices::virtio::VideoBackendType;
#[cfg(feature = "gpu")]
@ -1109,17 +1108,14 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument::
expected: String::from("A vhost-vsock device was already specified"),
});
}
cfg.vhost_vsock_device = Some(VhostVsockDeviceParameter::Fd(
value
.unwrap()
.parse()
.map_err(|_| argument::Error::InvalidValue {
value: value.unwrap().to_owned(),
expected: String::from(
"this value for `vhost-vsock-fd` needs to be integer",
),
})?,
));
let fd: i32 = value
.unwrap()
.parse()
.map_err(|_| argument::Error::InvalidValue {
value: value.unwrap().to_owned(),
expected: String::from("this value for `vhost-vsock-fd` needs to be integer"),
})?;
cfg.vhost_vsock_device = Some(PathBuf::from(format!("/proc/self/fd/{}", fd)));
}
"vhost-vsock-device" => {
if cfg.vhost_vsock_device.is_some() {
@ -1136,7 +1132,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument::
});
}
cfg.vhost_vsock_device = Some(VhostVsockDeviceParameter::Path(vhost_vsock_device_path));
cfg.vhost_vsock_device = Some(vhost_vsock_device_path);
}
"vhost-net-device" => {
let vhost_net_device_path = PathBuf::from(value.unwrap());