Port virtio media simple device as a vhost user media device. #1895
Conversation
Why are you copying this code here instead of depending on it? Would it be possible to contribute changes upstream and only implement the cuttlefish-specific changes here? |
base/cvd/cuttlefish/host/commands/vhost_user_media/simple_device/Cargo.toml
Show resolved
Hide resolved
base/cvd/cuttlefish/host/commands/vhost_user_media/simple_device/src/simple_device.rs
Outdated
Show resolved
Hide resolved
ser-io
force-pushed
the
virtio-media-use-new-SHMEM
branch
from
d42f8bd to
db5b139
Compare
Removed the copy depending on the upstream implementation. |
ser-io
force-pushed
the
virtio-media-use-new-SHMEM
branch
3 times, most recently
from
833efed to
fadcf29
Compare
| serde = { version = "1.0", features = ["derive"] } | ||
| serde_json = "1" | ||
|
|
||
| [patch.crates-io] |
There was a problem hiding this comment.
I'll remove this once rust-vmm/vhost/pull/339 is merged in.
ser-io
force-pushed
the
virtio-media-use-new-SHMEM
branch
from
fadcf29 to
8b2a123
Compare
Run the device. ``` RUST_LOG=trace cargo run --bin simple_device -- --socket-path /tmp/simple_device.sock or RUST_LOG=trace bazel run simple_device:simple_device -- --socket-path /tmp/simple_device.sock ``` Run the VM. ``` cargo run -- --log-level=debug run --cpus 4 --mem 4096 \ --rwdisk /path/to/debian-12.img \ --params "root=/dev/vda1" \ --vhost-user media,socket=/tmp/simple_device.sock \ /path/to/bzImage v4l2-compliance -d0 -s ``` Bug: 445229097
ser-io
force-pushed
the
virtio-media-use-new-SHMEM
branch
from
8b2a123 to
774f02d
Compare
| } | ||
| }); | ||
|
|
||
| handle.join().map_err(std::panic::resume_unwind).unwrap() |
There was a problem hiding this comment.
I'm confused by resume_unwind here, which is documented as
This is designed to be used in conjunction with
catch_unwindto, for example, carry a panic across a layer of C code.
I don't see a catch_unwind in this PR, does it come out of one of the libraries?
thread::spawn's NOTES mentions catch_unwind but doesn't explicitly say the two should be used together.
| vhost_user_media_workspace_crates = use_extension("@rules_rust//crate_universe:extensions.bzl", "crate") | ||
| vhost_user_media_workspace_crates.from_cargo( | ||
| name = "vhost_user_media_workspace_crates", | ||
| cargo_config = "@//build_external/crosvm:crosvm.config.toml", | ||
| manifests = [ | ||
| "//cuttlefish/host/commands/vhost_user_media:Cargo.toml", | ||
| ], | ||
| host_tools = "@rust_host_tools_nightly", | ||
| ) | ||
| use_repo(vhost_user_media_workspace_crates, "vhost_user_media_workspace_crates") |
There was a problem hiding this comment.
Can this go in a vhost_user_media.MODULE.bazel file in the vhost_user_media directory?
Databean
left a comment
Databean
left a comment
There was a problem hiding this comment.
Some of the comments apply in multiple places, like reducing map_err usage in favor of automatic conversion with From.
| #[error("Failed to handle unknown event")] | ||
| HandleEventUnknown, | ||
| #[error("Descriptor chain error")] | ||
| DescriptorChainError(virtio_queue::Error), |
There was a problem hiding this comment.
A feature of thiserror is that this can be written as
DescriptorChainError(#[from] virtio_queue::Error),This automatically creates From<DescriptorChainError> implementation for VhuMediaBackendError. ? will automatically invoke From conversions, meaning that other uses can be simplified:
vring
.get_mut()
.add_used(head_index, writer.bytes_written() as u32)
.map_err(|e| VhuMediaBackendError::DescriptorChainError(e))?;becomes
vring
.get_mut()
.add_used(head_index, writer.bytes_written() as u32)?;| } | ||
|
|
||
| fn queues_per_thread(&self) -> Vec<u64> { | ||
| return vec![3]; |
There was a problem hiding this comment.
nit: this can be vec![3] (no return or semicolon) as Rust will use the final expression in a function as a return value.
| | VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() | ||
| } | ||
|
|
||
| fn protocol_features(&self) -> vhost::vhost_user::message::VhostUserProtocolFeatures { |
There was a problem hiding this comment.
nit: vhost::vhost_user::message::VhostUserProtocolFeatures has a use statement, it doesn't need a full path.
| if requests.is_empty() { | ||
| return Err(VhuMediaBackendError::DescriptorUnavailable); | ||
| } | ||
| for desc_chain in requests.clone() { |
There was a problem hiding this comment.
Does requests need to be clone()d here? It doesn't look like it is used anywhere else.
| if let Err(e) = match event { | ||
| V4l2Event::DequeueBuffer(e) => WriteToDescriptorChain::write_obj(&mut writer, e), | ||
| V4l2Event::Error(e) => WriteToDescriptorChain::write_obj(&mut writer, e), | ||
| V4l2Event::Event(e) => WriteToDescriptorChain::write_obj(&mut writer, e), | ||
| } { | ||
| return Err(VhuMediaBackendError::Io(e)); | ||
| } |
There was a problem hiding this comment.
This can be written as
match event {
V4l2Event::DequeueBuffer(e) | V4l2Event::Error(e) | V4l2Event::Event(e) => WriteToDescriptorChain::write_obj(&mut writer, e)?,
}From the reference
Multiple match patterns may be joined with the
|operator. Each pattern will be tested in left-to-right sequence until a successful match is found.
Every binding in each | separated pattern must appear in all of the patterns in the arm.
Every binding of the same name must have the same type, and have the same binding mode.
The ? operator will automatically use the From<std::io::Error> conversion defind fro VhuMediaBackendError.
| } | ||
| } | ||
| unknown_cmd => { | ||
| error!("unknown virtio media command: {}", unknown_cmd); |
There was a problem hiding this comment.
error! supports standard Rust format strings, which can use variable names inline, i.e.
error!("unknown virtio media command: {unknown_cmd}");| error!("device new session error: {}", e); | ||
| writer | ||
| .write_err_response(e) | ||
| .map_err(|e| VhuMediaBackendError::Io(e))?; |
There was a problem hiding this comment.
VhuMediaBackendError::Io defines a From<io::Error> implementation, so this could be collapsed to
writer.write_err_response(e)?;using automatic From conversion in the ? operator.
| let mut flags = VhostUserMMapFlags::empty(); | ||
| if rw { | ||
| flags |= VhostUserMMapFlags::WRITABLE; | ||
| } |
There was a problem hiding this comment.
nit: An alternative to making this variable mut is to use an if expression:
let flags = if rw { VhostUserMMapFlags::empty() } else { VhostUserMMapFlags::WRITABLE };| use vhost::vhost_user::message::VhostUserProtocolFeatures; | ||
| use vhost::vhost_user::message::VhostUserShMemConfig; | ||
| use vhost::vhost_user::message::VhostUserVirtioFeatures; | ||
| use vhost_user_backend::{VhostUserBackendMut, VringRwLock, VringT}; |
There was a problem hiding this comment.
What do you think of using the combined use x::{y, z} syntax more?
It supports multiple levels of nesting, e.g.
use std::{collections::HashMap, convert, io, os::{fd::BorrowedFd, unix::io::AsRawFd}};
...
use log::{error, info};
...It's even possible to combine everything into a single use statement:
use {
std::collection::HashMap,
...
libc::{_SC_PAGESIZE, sysconf},
log::{error, info},
};The style guide also has some opinions, though they don't seem like strict rules.
| DescriptorUnavailable, | ||
| } | ||
|
|
||
| impl convert::From<VhuMediaBackendError> for io::Error { |
There was a problem hiding this comment.
nit: since the 2021 edition, std::convert::From is in the prelude, so it doesn't have to be imported and can be referenced as From.
3 participants
Port virtio media reference device
simple_deviceas a vhost user implementation.First iteration of
simple_device/src/simple_device.rsis the same as in virtio-media upstream https://github.com/chromeos/virtio-media/blob/main/device/src/devices/simple_device.rs.Bug: 445229097