diff options
author | Chirantan Ekbote <chirantan@chromium.org> | 2019-08-16 15:40:48 +0900 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2019-10-15 18:26:29 +0000 |
commit | b5964164c4d6187971495ccf82fd64a1d5bde232 (patch) | |
tree | 29342794c8f3adbcb8f1d6657cd897aad1d70a59 /devices/src/virtio/gpu/mod.rs | |
parent | 2515b756302235e69fbc1b07ae70270be204a91d (diff) | |
download | crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar.gz crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar.bz2 crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar.lz crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar.xz crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.tar.zst crosvm-b5964164c4d6187971495ccf82fd64a1d5bde232.zip |
devices: Refactor DescriptorChainConsumer, Reader, and Writer
Refactor the Reader and Writer implementations for DescriptorChains. This has several changes: * Change the DescriptorChainConsumer to keep a VecDeque<VolatileSlice> instead of an iterator. This delegates the fiddly business of sub-slicing chunks of memory to the VolatileSlice implementation. * Read in the entire DescriptorChain once when the Reader or Writer is first constructed. This allows us to validate the DescriptorChain in the beginning rather than having to deal with an invalid DescriptorChain in the middle of the device operating on it. Combined with the check that enforces the ordering of read/write descriptors in a previous change we can be sure that the entire descriptor chain that we have copied in is valid. * Add a new `split_at` method so that we can split the Reader/Writer into multiple pieces, each responsible for reading/writing a separate part of the DescriptorChain. This is particularly useful for implementing zero-copy data transfer as we sometimes need to write the data first and then update an earlier part of the buffer with the number of bytes written. * Stop caching the available bytes in the DescriptorChain. The previous implementation iterated over the remaining descriptors in the chain and then only updated the cached value. If a mis-behaving guest then changed one of the later descriptors, the cached value would no longer be valid. * Check for integer overflow when calculating the number of bytes available in the chain. A guest could fill a chain with five 1GB descriptors and cause an integer overflow on a 32-bit machine. This would previously crash the device process since we compile with integer overflow checks enabled but it would be better to return an error instead. * Clean up the Read/Write impls. Having 2 different functions called `read`, with different behavior is just confusing. Consolidate on the Read/Write traits from `std::io`. * Change the `read_to` and `write_from` functions to be generic over types that implement `FileReadWriteVolatile` since we are not allowed to assume that it's safe to call read or write on something just because it implements `AsRawFd`. Also add `*at` variants that read or write to a particular offset rather than the kernel offset. * Change the callback passed to the `consume` function of `DescriptorChainConsumer` to take a `&[VolatileSlice]` instead. This way we can use the `*vectored` versions of some methods to reduce the number of I/O syscalls we need to make. * Change the `Result` types that are returned. Functions that perform I/O return an `io::Result`. Functions that only work on guest memory return a `guest_memory::Result`. This makes it easier to inter-operate with the functions from `std::io`. * Change some u64/u32 parameters to usize to avoid having to convert back and forth between the two in various places. BUG=b:136128319 TEST=unit tests Change-Id: I15102f7b4035d66b5ce0891df42b656411e8279f Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1757240 Auto-Submit: Chirantan Ekbote <chirantan@chromium.org> Tested-by: kokoro <noreply+kokoro@google.com> Reviewed-by: Stephen Barber <smbarber@chromium.org> Reviewed-by: Zach Reizner <zachr@chromium.org> Reviewed-by: Daniel Verkamp <dverkamp@chromium.org> Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Diffstat (limited to 'devices/src/virtio/gpu/mod.rs')
-rw-r--r-- | devices/src/virtio/gpu/mod.rs | 78 |
1 files changed, 63 insertions, 15 deletions
diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs index 29142ea..97c84d5 100644 --- a/devices/src/virtio/gpu/mod.rs +++ b/devices/src/virtio/gpu/mod.rs @@ -8,6 +8,7 @@ mod protocol; use std::cell::RefCell; use std::collections::VecDeque; use std::i64; +use std::io::Read; use std::mem::{self, size_of}; use std::num::NonZeroU8; use std::os::unix::io::{AsRawFd, RawFd}; @@ -124,7 +125,14 @@ impl Frontend { mem, ), GpuCommand::ResourceAttachBacking(info) => { - if reader.available_bytes() != 0 { + let available_bytes = match reader.available_bytes() { + Ok(count) => count, + Err(e) => { + debug!("invalid descriptor: {}", e); + 0 + } + }; + if available_bytes != 0 { let entry_count = info.nr_entries.to_native() as usize; let mut iovecs = Vec::with_capacity(entry_count); for _ in 0..entry_count { @@ -247,10 +255,17 @@ impl Frontend { ) } GpuCommand::CmdSubmit3d(info) => { - if reader.available_bytes() != 0 { + let available_bytes = match reader.available_bytes() { + Ok(count) => count, + Err(e) => { + debug!("invalid descriptor: {}", e); + 0 + } + }; + if available_bytes != 0 { let cmd_size = info.size.to_native() as usize; let mut cmd_buf = vec![0; cmd_size]; - if reader.read(&mut cmd_buf[..]).is_ok() { + if reader.read_exact(&mut cmd_buf[..]).is_ok() { self.backend .submit_command(info.hdr.ctx_id.to_native(), &mut cmd_buf[..]) } else { @@ -263,7 +278,14 @@ impl Frontend { } } GpuCommand::AllocationMetadata(info) => { - if reader.available_bytes() != 0 { + let available_bytes = match reader.available_bytes() { + Ok(count) => count, + Err(e) => { + debug!("invalid descriptor: {}", e); + 0 + } + }; + if 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(); @@ -275,7 +297,7 @@ impl Frontend { let mut request_buf = vec![0; request_size as usize]; let response_buf = vec![0; response_size as usize]; - if reader.read(&mut request_buf[..]).is_ok() { + if reader.read_exact(&mut request_buf[..]).is_ok() { self.backend .allocation_metadata(id, request_buf, response_buf) } else { @@ -286,7 +308,14 @@ impl Frontend { } } GpuCommand::ResourceCreateV2(info) => { - if reader.available_bytes() != 0 { + let available_bytes = match reader.available_bytes() { + Ok(count) => count, + Err(e) => { + debug!("invalid descriptor: {}", e); + 0 + } + }; + if 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(); @@ -314,7 +343,7 @@ impl Frontend { } } - match reader.read(&mut args[..]) { + match reader.read_exact(&mut args[..]) { Ok(_) => self.backend.resource_create_v2( resource_id, guest_memory_type, @@ -346,13 +375,23 @@ impl Frontend { let mut signal_used = false; while let Some(desc) = queue.pop(mem) { if Frontend::validate_desc(&desc) { - let mut reader = Reader::new(mem, desc.clone()); - let mut writer = Writer::new(mem, desc.clone()); - if let Some(ret_desc) = - self.process_descriptor(mem, desc.index, &mut reader, &mut writer) - { - queue.add_used(&mem, ret_desc.index, ret_desc.len); - signal_used = true; + match ( + Reader::new(mem, desc.clone()), + Writer::new(mem, desc.clone()), + ) { + (Ok(mut reader), Ok(mut writer)) => { + if let Some(ret_desc) = + self.process_descriptor(mem, desc.index, &mut reader, &mut writer) + { + queue.add_used(&mem, ret_desc.index, ret_desc.len); + signal_used = true; + } + } + (_, Err(e)) | (Err(e), _) => { + debug!("invalid descriptor: {}", e); + queue.add_used(&mem, desc.index, 0); + signal_used = true; + } } } else { let likely_type = mem.read_obj_from_addr(desc.addr).unwrap_or(Le32::from(0)); @@ -391,7 +430,16 @@ impl Frontend { if resp.is_err() { debug!("{:?} -> {:?}", gpu_cmd, resp); } - if writer.available_bytes() != 0 { + + let available_bytes = match writer.available_bytes() { + Ok(count) => count, + Err(e) => { + debug!("invalid descriptor: {}", e); + 0 + } + }; + + if available_bytes != 0 { let mut fence_id = 0; let mut ctx_id = 0; let mut flags = 0; |