From 0cc0fe4f7abdfb460b61bd1f2fca41ea6857bd3b Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 17 Nov 2021 20:33:49 +0530 Subject: [PATCH 1/5] [i2c] Always update and validate buffers Not just read-buffers, also validate write buffers for rdwr() tests. Signed-off-by: Viresh Kumar --- src/i2c/src/i2c.rs | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 4662270..946a756 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -574,6 +574,20 @@ pub mod tests { use std::convert::TryFrom; use vmm_sys_util::tempfile::TempFile; + // Update read-buffer of each write-buffer with index + 1 value. + pub fn update_rdwr_buf(buf: &mut Vec) { + for (i, byte) in buf.iter_mut().enumerate() { + *byte = i as u8 + 1; + } + } + + // Verify the write-buffer passed to us + pub fn verify_rdwr_buf(buf: &[u8]) { + for (i, byte) in buf.iter().enumerate() { + assert_eq!(*byte, i as u8 + 1); + } + } + #[derive(Debug)] pub struct DummyDevice { funcs_result: Result, @@ -611,12 +625,15 @@ pub mod tests { } fn rdwr(&self, reqs: &mut [I2cReq]) -> Result<()> { - // Update buffer of each write-buffer with index + 1 value. for req in reqs { + if req.len == 0 { + return Err(Error::I2cTransferInvalid(0)); + } + if (req.flags & I2C_M_RD) != 0 { - for (j, byte) in req.buf.iter_mut().enumerate() { - *byte = j as u8 + 1; - } + update_rdwr_buf(&mut req.buf); + } else { + verify_rdwr_buf(&req.buf); } } @@ -644,9 +661,7 @@ pub mod tests { // Match what's done by DummyDevice::rdwr() for req in reqs { if (req.flags & I2C_M_RD) != 0 { - for (i, byte) in req.buf.iter().enumerate() { - assert_eq!(*byte, i as u8 + 1); - } + verify_rdwr_buf(&req.buf); } } } @@ -735,6 +750,12 @@ pub mod tests { }, ]; + for req in &mut reqs { + if (req.flags & I2C_M_RD) == 0 { + update_rdwr_buf(&mut req.buf); + } + } + i2c_map.transfer(&mut *reqs).unwrap(); verify_rdwr_data(&reqs); } From f9f3bdf8453a4e62205c73f6fd721946375853e8 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 17 Nov 2021 20:11:13 +0530 Subject: [PATCH 2/5] [i2c] Break process_queue() into two parts Break out most of the non vring specific stuff to process_requests(), this is required for testing request processing properly. Signed-off-by: Viresh Kumar --- src/i2c/Cargo.toml | 1 + src/i2c/src/vhu_i2c.rs | 264 ++++++++++++++++++++++------------------- 2 files changed, 142 insertions(+), 123 deletions(-) diff --git a/src/i2c/Cargo.toml b/src/i2c/Cargo.toml index 2738dda..6998c5a 100644 --- a/src/i2c/Cargo.toml +++ b/src/i2c/Cargo.toml @@ -19,5 +19,6 @@ thiserror = "1.0" vhost = { version = "0.2", features = ["vhost-user-slave"] } vhost-user-backend = { git = "https://github.com/rust-vmm/vhost-user-backend", rev = "4047c697470cc6c37e8e1835025b091d2b59c2f7" } virtio-bindings = ">=0.1" +virtio-queue = { git = "https://github.com/rust-vmm/vm-virtio", rev = "66cda80" } vm-memory = "0.7" vmm-sys-util = "=0.9.0" diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index f5ff3ea..bfa74a2 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -17,6 +17,7 @@ use virtio_bindings::bindings::virtio_net::{VIRTIO_F_NOTIFY_ON_EMPTY, VIRTIO_F_V use virtio_bindings::bindings::virtio_ring::{ VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, }; +use virtio_queue::DescriptorChain; use vm_memory::{ByteValued, Bytes, GuestMemoryAtomic, GuestMemoryMmap, Le16, Le32}; use vmm_sys_util::epoll::EventSet; use vmm_sys_util::eventfd::{EventFd, EFD_NONBLOCK}; @@ -96,6 +97,141 @@ pub struct VhostUserI2cBackend { pub exit_event: EventFd, } +type I2cDescriptorChain = DescriptorChain>>; + +/// Process the requests in the vring and dispatch replies +fn process_requests( + i2c_map: &Arc>, + requests: Vec, + vring: Option<&VringRwLock>, +) -> Result { + let mut reqs: Vec = Vec::new(); + + if requests.is_empty() { + return Ok(true); + } + + // Iterate over each I2C request and push it to "reqs" vector. + for desc_chain in requests.clone() { + let descriptors: Vec<_> = desc_chain.clone().collect(); + + if (descriptors.len() != 2) && (descriptors.len() != 3) { + return Err(Error::UnexpectedDescriptorCount); + } + + let desc_out_hdr = descriptors[0]; + + if desc_out_hdr.is_write_only() { + return Err(Error::UnexpectedWriteOnlyDescriptor); + } + + if desc_out_hdr.len() as usize != size_of::() { + return Err(Error::UnexpectedDescriptorSize); + } + + let out_hdr = desc_chain + .memory() + .read_obj::(desc_out_hdr.addr()) + .map_err(|_| Error::DescriptorReadFailed)?; + + let flags = match out_hdr.flags.to_native() & VIRTIO_I2C_FLAGS_M_RD { + VIRTIO_I2C_FLAGS_M_RD => I2C_M_RD, + _ => 0, + }; + + let desc_in_hdr = descriptors[descriptors.len() - 1]; + if !desc_in_hdr.is_write_only() { + return Err(Error::UnexpectedReadableDescriptor); + } + + if desc_in_hdr.len() as usize != size_of::() { + return Err(Error::UnexpectedDescriptorSize); + } + + let (buf, len) = match descriptors.len() { + // Buffer is available + 3 => { + let desc_buf = descriptors[1]; + let len = desc_buf.len(); + + if len == 0 { + return Err(Error::UnexpectedDescriptorSize); + } + let mut buf = vec![0; len as usize]; + + if flags != I2C_M_RD { + if desc_buf.is_write_only() { + return Err(Error::UnexpectedWriteOnlyDescriptor); + } + + desc_chain + .memory() + .read(&mut buf, desc_buf.addr()) + .map_err(|_| Error::DescriptorReadFailed)?; + } else if !desc_buf.is_write_only() { + return Err(Error::UnexpectedReadableDescriptor); + } + + (buf, len) + } + + _ => (Vec::::new(), 0), + }; + + reqs.push(I2cReq { + addr: out_hdr.addr.to_native() >> 1, + flags, + len: len as u16, + buf, + }); + } + + let in_hdr = { + VirtioI2cInHdr { + status: match i2c_map.transfer(&mut reqs) { + Ok(()) => VIRTIO_I2C_MSG_OK, + Err(_) => VIRTIO_I2C_MSG_ERR, + }, + } + }; + + for (i, desc_chain) in requests.iter().enumerate() { + let descriptors: Vec<_> = desc_chain.clone().collect(); + let desc_in_hdr = descriptors[descriptors.len() - 1]; + let mut len = size_of::() as u32; + + if descriptors.len() == 3 { + let desc_buf = descriptors[1]; + + // Write the data read from the I2C device + if reqs[i].flags == I2C_M_RD { + desc_chain + .memory() + .write(&reqs[i].buf, desc_buf.addr()) + .map_err(|_| Error::DescriptorWriteFailed)?; + } + + if in_hdr.status == VIRTIO_I2C_MSG_OK { + len += desc_buf.len(); + } + } + + // Write the transfer status + desc_chain + .memory() + .write_obj::(in_hdr, desc_in_hdr.addr()) + .map_err(|_| Error::DescriptorWriteFailed)?; + + if let Some(vring) = vring { + if vring.add_used(desc_chain.head_index(), len).is_err() { + warn!("Couldn't return used descriptors to the ring"); + } + } + } + + Ok(true) +} + impl VhostUserI2cBackend { pub fn new(i2c_map: Arc>) -> Result { Ok(VhostUserI2cBackend { @@ -107,8 +243,6 @@ impl VhostUserI2cBackend { /// Process the requests in the vring and dispatch replies fn process_queue(&self, vring: &VringRwLock) -> Result { - let mut reqs: Vec = Vec::new(); - let requests: Vec<_> = vring .get_mut() .get_queue_mut() @@ -116,129 +250,13 @@ impl VhostUserI2cBackend { .map_err(|_| Error::DescriptorNotFound)? .collect(); - if requests.is_empty() { - return Ok(true); + if process_requests(&self.i2c_map, requests, Some(vring))? { + // Send notification once all the requests are processed + vring + .signal_used_queue() + .map_err(|_| Error::NotificationFailed)?; } - // Iterate over each I2C request and push it to "reqs" vector. - for desc_chain in requests.clone() { - let descriptors: Vec<_> = desc_chain.clone().collect(); - - if (descriptors.len() != 2) && (descriptors.len() != 3) { - return Err(Error::UnexpectedDescriptorCount); - } - - let desc_out_hdr = descriptors[0]; - if desc_out_hdr.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor); - } - - if desc_out_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize); - } - - let out_hdr = desc_chain - .memory() - .read_obj::(desc_out_hdr.addr()) - .map_err(|_| Error::DescriptorReadFailed)?; - - let flags = match out_hdr.flags.to_native() & VIRTIO_I2C_FLAGS_M_RD { - VIRTIO_I2C_FLAGS_M_RD => I2C_M_RD, - _ => 0, - }; - - let desc_in_hdr = descriptors[descriptors.len() - 1]; - if !desc_in_hdr.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor); - } - - if desc_in_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize); - } - - let (buf, len) = match descriptors.len() { - // Buffer is available - 3 => { - let desc_buf = descriptors[1]; - let len = desc_buf.len(); - - if len == 0 { - return Err(Error::UnexpectedDescriptorSize); - } - let mut buf = vec![0; len as usize]; - - if flags != I2C_M_RD { - if desc_buf.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor); - } - - desc_chain - .memory() - .read(&mut buf, desc_buf.addr()) - .map_err(|_| Error::DescriptorReadFailed)?; - } else if !desc_buf.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor); - } - - (buf, len) - } - - _ => (Vec::::new(), 0), - }; - - reqs.push(I2cReq { - addr: out_hdr.addr.to_native() >> 1, - flags, - len: len as u16, - buf, - }); - } - - let in_hdr = { - VirtioI2cInHdr { - status: match self.i2c_map.transfer(&mut reqs) { - Ok(()) => VIRTIO_I2C_MSG_OK, - Err(_) => VIRTIO_I2C_MSG_ERR, - }, - } - }; - - for (i, desc_chain) in requests.iter().enumerate() { - let descriptors: Vec<_> = desc_chain.clone().collect(); - let desc_in_hdr = descriptors[descriptors.len() - 1]; - let mut len = size_of::() as u32; - - if descriptors.len() == 3 { - let desc_buf = descriptors[1]; - - // Write the data read from the I2C device - if reqs[i].flags == I2C_M_RD { - desc_chain - .memory() - .write(&reqs[i].buf, desc_buf.addr()) - .map_err(|_| Error::DescriptorWriteFailed)?; - } - - if in_hdr.status == VIRTIO_I2C_MSG_OK { - len += desc_buf.len(); - } - } - - // Write the transfer status - desc_chain - .memory() - .write_obj::(in_hdr, desc_in_hdr.addr()) - .map_err(|_| Error::DescriptorWriteFailed)?; - - if vring.add_used(desc_chain.head_index(), len).is_err() { - warn!("Couldn't return used descriptors to the ring"); - } - } - - // Send notification once all the requests are processed - vring - .signal_used_queue() - .map_err(|_| Error::NotificationFailed)?; Ok(true) } } From f96dc4e50d3c0545589cc9743bd8cef36d3fec30 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 18 Nov 2021 09:51:35 +0530 Subject: [PATCH 3/5] [i2c] Improve descriptor parsing errors Improve descriptor parsing errors to contain more information about the error. Signed-off-by: Viresh Kumar --- src/i2c/src/vhu_i2c.rs | 44 ++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index bfa74a2..eb5cf23 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -40,14 +40,14 @@ pub enum Error { HandleEventNotEpollIn, #[error("Failed to handle unknown event")] HandleEventUnknown, - #[error("Received unexpected write only descriptor")] - UnexpectedWriteOnlyDescriptor, - #[error("Received unexpected readable descriptor")] - UnexpectedReadableDescriptor, - #[error("Invalid descriptor count")] - UnexpectedDescriptorCount, - #[error("Invalid descriptor size")] - UnexpectedDescriptorSize, + #[error("Received unexpected write only descriptor at index {0}")] + UnexpectedWriteOnlyDescriptor(usize), + #[error("Received unexpected readable descriptor at index {0}")] + UnexpectedReadableDescriptor(usize), + #[error("Invalid descriptor count {0}")] + UnexpectedDescriptorCount(usize), + #[error("Invalid descriptor size, expected: {0}, found: {1}")] + UnexpectedDescriptorSize(usize, u32), #[error("Descriptor not found")] DescriptorNotFound, #[error("Descriptor read failed")] @@ -56,8 +56,8 @@ pub enum Error { DescriptorWriteFailed, #[error("Failed to send notification")] NotificationFailed, - #[error("Failed to create new EventFd: {0:?}")] - EventFdFailed(std::io::Error), + #[error("Failed to create new EventFd")] + EventFdFailed, } impl convert::From for io::Error { @@ -116,17 +116,20 @@ fn process_requests( let descriptors: Vec<_> = desc_chain.clone().collect(); if (descriptors.len() != 2) && (descriptors.len() != 3) { - return Err(Error::UnexpectedDescriptorCount); + return Err(Error::UnexpectedDescriptorCount(descriptors.len())); } let desc_out_hdr = descriptors[0]; if desc_out_hdr.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor); + return Err(Error::UnexpectedWriteOnlyDescriptor(0)); } if desc_out_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize); + return Err(Error::UnexpectedDescriptorSize( + size_of::(), + desc_out_hdr.len(), + )); } let out_hdr = desc_chain @@ -141,11 +144,14 @@ fn process_requests( let desc_in_hdr = descriptors[descriptors.len() - 1]; if !desc_in_hdr.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor); + return Err(Error::UnexpectedReadableDescriptor(descriptors.len() - 1)); } if desc_in_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize); + return Err(Error::UnexpectedDescriptorSize( + size_of::(), + desc_in_hdr.len(), + )); } let (buf, len) = match descriptors.len() { @@ -155,13 +161,13 @@ fn process_requests( let len = desc_buf.len(); if len == 0 { - return Err(Error::UnexpectedDescriptorSize); + return Err(Error::UnexpectedDescriptorSize(1, len)); } let mut buf = vec![0; len as usize]; if flags != I2C_M_RD { if desc_buf.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor); + return Err(Error::UnexpectedWriteOnlyDescriptor(1)); } desc_chain @@ -169,7 +175,7 @@ fn process_requests( .read(&mut buf, desc_buf.addr()) .map_err(|_| Error::DescriptorReadFailed)?; } else if !desc_buf.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor); + return Err(Error::UnexpectedReadableDescriptor(1)); } (buf, len) @@ -237,7 +243,7 @@ impl VhostUserI2cBackend { Ok(VhostUserI2cBackend { i2c_map, event_idx: false, - exit_event: EventFd::new(EFD_NONBLOCK).map_err(Error::EventFdFailed)?, + exit_event: EventFd::new(EFD_NONBLOCK).map_err(|_| Error::EventFdFailed)?, }) } From 8c92046701736f771e51a95d6e49ca7a7e14ffd9 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 17 Nov 2021 20:12:49 +0530 Subject: [PATCH 4/5] [i2c] Test descriptor processing Add tests to validate processing of descriptors and read/write data. The design is based on the virtio_queue::mock::MockSplitQueue implementation, which is used to add a set of descriptors to the memory. The same memory is then processed via DescriptorChain and the same set of descriptors magically appear. This patch adds various tests for success and failure and also validate the data read or written. Signed-off-by: Viresh Kumar --- Cargo.lock | 5 +- coverage_config_x86_64.json | 2 +- src/i2c/Cargo.toml | 4 + src/i2c/src/vhu_i2c.rs | 338 +++++++++++++++++++++++++++++++++++- 4 files changed, 342 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3d20679..7862d15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,9 +4,9 @@ version = 3 [[package]] name = "arc-swap" -version = "1.4.0" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6df5aef5c5830360ce5218cecb8f018af3438af5686ae945094affc86fdec63" +checksum = "c5d78ce20460b82d3fa150275ed9d55e21064fc7951177baacf86a145c4a4b1f" [[package]] name = "atty" @@ -287,6 +287,7 @@ dependencies = [ "vhost", "vhost-user-backend", "virtio-bindings", + "virtio-queue", "vm-memory 0.7.0", "vmm-sys-util", ] diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index d6ba776..861b91e 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 58.4, + "coverage_score": 74.1, "exclude_path": "", "crate_features": "" } diff --git a/src/i2c/Cargo.toml b/src/i2c/Cargo.toml index 6998c5a..4340251 100644 --- a/src/i2c/Cargo.toml +++ b/src/i2c/Cargo.toml @@ -22,3 +22,7 @@ virtio-bindings = ">=0.1" virtio-queue = { git = "https://github.com/rust-vmm/vm-virtio", rev = "66cda80" } vm-memory = "0.7" vmm-sys-util = "=0.9.0" + +[dev-dependencies] +virtio-queue = { git = "https://github.com/rust-vmm/vm-virtio", rev = "66cda80", features = ["test-utils"] } +vm-memory = { version = "0.7.0", features = ["backend-mmap", "backend-atomic"] } diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index eb5cf23..6e963b8 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -33,7 +33,7 @@ const NUM_QUEUES: usize = 1; type Result = std::result::Result; type VhostUserBackendResult = std::result::Result; -#[derive(Debug, ThisError)] +#[derive(Copy, Clone, Debug, PartialEq, ThisError)] /// Errors related to vhost-device-i2c daemon. pub enum Error { #[error("Failed to handle event, didn't match EPOLLIN")] @@ -352,11 +352,341 @@ impl VhostUserBackendMut #[cfg(test)] mod tests { - use super::*; - use crate::i2c::tests::DummyDevice; - use crate::AdapterConfig; use std::convert::TryFrom; + use virtio_queue::defs::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; + use virtio_queue::{mock::MockSplitQueue, Descriptor}; + use vm_memory::{Address, GuestAddress, GuestMemoryAtomic, GuestMemoryMmap}; + + use super::Error; + use super::*; + use crate::i2c::tests::{update_rdwr_buf, verify_rdwr_buf, DummyDevice}; + use crate::AdapterConfig; + + // Prepares a single chain of descriptors + fn prepare_desc_chain( + start_addr: GuestAddress, + buf: &mut Vec, + flag: u32, + client_addr: u16, + ) -> I2cDescriptorChain { + let mem = GuestMemoryMmap::<()>::from_ranges(&[(start_addr, 0x1000)]).unwrap(); + let vq = MockSplitQueue::new(&mem, 16); + let mut next_addr = vq.desc_table().total_size() + 0x100; + let mut index = 0; + + // Out header descriptor + let out_hdr = VirtioI2cOutHdr { + addr: From::from(client_addr << 1), + padding: From::from(0x0), + flags: From::from(flag), + }; + + let desc_out = Descriptor::new( + next_addr, + size_of::() as u32, + VIRTQ_DESC_F_NEXT, + index + 1, + ); + + mem.write_obj::(out_hdr, desc_out.addr()) + .unwrap(); + vq.desc_table().store(index, desc_out); + next_addr += desc_out.len() as u64; + index += 1; + + // Buf descriptor: optional + if !buf.is_empty() { + // Set buffer is write-only or not + let flag = if (flag & VIRTIO_I2C_FLAGS_M_RD) == 0 { + update_rdwr_buf(buf); + 0 + } else { + VIRTQ_DESC_F_WRITE + }; + + let desc_buf = Descriptor::new( + next_addr, + buf.len() as u32, + flag | VIRTQ_DESC_F_NEXT, + index + 1, + ); + mem.write(buf, desc_buf.addr()).unwrap(); + vq.desc_table().store(index, desc_buf); + next_addr += desc_buf.len() as u64; + index += 1; + } + + // In response descriptor + let desc_in = Descriptor::new(next_addr, size_of::() as u32, VIRTQ_DESC_F_WRITE, 0); + vq.desc_table().store(index, desc_in); + + // Put the descriptor index 0 in the first available ring position. + mem.write_obj(0u16, vq.avail_addr().unchecked_add(4)) + .unwrap(); + + // Set `avail_idx` to 1. + mem.write_obj(1u16, vq.avail_addr().unchecked_add(2)) + .unwrap(); + + // Create descriptor chain from pre-filled memory + vq.create_queue(GuestMemoryAtomic::::new(mem.clone())) + .iter() + .unwrap() + .next() + .unwrap() + } + + // Validate descriptor chains after processing them, checks pass/failure of + // operation and the value of the buffers updated by the `DummyDevice`. + fn validate_desc_chains(desc_chains: Vec, status: u8) { + for desc_chain in desc_chains { + let descriptors: Vec<_> = desc_chain.clone().collect(); + + let in_hdr = desc_chain + .memory() + .read_obj::(descriptors[descriptors.len() - 1].addr()) + .unwrap(); + + // Operation result should match expected status. + assert_eq!(in_hdr.status, status); + + let out_hdr = desc_chain + .memory() + .read_obj::(descriptors[0].addr()) + .unwrap(); + + if (out_hdr.flags.to_native() & VIRTIO_I2C_FLAGS_M_RD) != 0 && descriptors.len() == 3 { + let mut buf = vec![0; descriptors[1].len() as usize]; + desc_chain + .memory() + .read(&mut buf, descriptors[1].addr()) + .unwrap(); + + // Verify the content of the read-buffer + verify_rdwr_buf(&buf); + } + } + } + + // Prepares list of dummy descriptors, their content isn't significant + fn prepare_desc_chain_dummy( + addr: Option>, + flags: Vec, + len: Vec, + ) -> I2cDescriptorChain { + let mem = GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(); + let vq = MockSplitQueue::new(&mem, 16); + + for (i, flag) in flags.iter().enumerate() { + let mut f = if i == flags.len() - 1 { + 0 + } else { + VIRTQ_DESC_F_NEXT + }; + f |= flag; + + let offset = match addr { + Some(ref addr) => addr[i], + _ => 0x100, + }; + + let desc = Descriptor::new(offset, len[i], f, (i + 1) as u16); + vq.desc_table().store(i as u16, desc); + } + + // Put the descriptor index 0 in the first available ring position. + mem.write_obj(0u16, vq.avail_addr().unchecked_add(4)) + .unwrap(); + + // Set `avail_idx` to 1. + mem.write_obj(1u16, vq.avail_addr().unchecked_add(2)) + .unwrap(); + + // Create descriptor chain from pre-filled memory + vq.create_queue(GuestMemoryAtomic::::new(mem.clone())) + .iter() + .unwrap() + .next() + .unwrap() + } + + #[test] + fn process_requests_success() { + let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); + let i2c_map = Arc::new(I2cMap::::new(&device_config).unwrap()); + + // Descriptor chain size zero, shouldn't fail + process_requests(&i2c_map, Vec::::new(), None).unwrap(); + + // Valid single read descriptor + let mut buf: Vec = vec![0; 30]; + let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); + let desc_chains = vec![desc_chain]; + + process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); + + // Valid single write descriptor + let mut buf: Vec = vec![0; 30]; + let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, 0, 4); + let desc_chains = vec![desc_chain]; + + process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); + + // Valid mixed read-write descriptors + let mut buf: Vec> = vec![vec![0; 30]; 6]; + let desc_chains = vec![ + // Write + prepare_desc_chain(GuestAddress(0), &mut buf[0], 0, 4), + // Read + prepare_desc_chain(GuestAddress(0), &mut buf[1], VIRTIO_I2C_FLAGS_M_RD, 4), + // Write + prepare_desc_chain(GuestAddress(0), &mut buf[2], 0, 4), + // Read + prepare_desc_chain(GuestAddress(0), &mut buf[3], VIRTIO_I2C_FLAGS_M_RD, 4), + // Write + prepare_desc_chain(GuestAddress(0), &mut buf[4], 0, 4), + // Read + prepare_desc_chain(GuestAddress(0), &mut buf[5], VIRTIO_I2C_FLAGS_M_RD, 4), + ]; + + process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); + } + + #[test] + fn process_requests_failure() { + let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); + let i2c_map = Arc::new(I2cMap::::new(&device_config).unwrap()); + + // One descriptors + let flags: Vec = vec![0]; + let len: Vec = vec![0]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedDescriptorCount(1) + ); + + // Four descriptors + let flags: Vec = vec![0, 0, 0, 0]; + let len: Vec = vec![0, 0, 0, 0]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedDescriptorCount(4) + ); + + // Write only out hdr + let flags: Vec = vec![VIRTQ_DESC_F_WRITE, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 1, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedWriteOnlyDescriptor(0) + ); + + // Invalid out hdr length + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![100, 1, size_of::() as u32]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedDescriptorSize(size_of::(), 100) + ); + + // Invalid out hdr address + let addr: Vec = vec![0x10000, 0, 0]; + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 1, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::DescriptorReadFailed + ); + + // Read only in hdr + let flags: Vec = vec![0, 0, 0]; + let len: Vec = vec![ + size_of::() as u32, + 1, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedReadableDescriptor(2) + ); + + // Invalid in hdr length + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![size_of::() as u32, 1, 100]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedDescriptorSize(size_of::(), 100) + ); + + // Invalid in hdr address + let addr: Vec = vec![0, 0, 0x10000]; + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 1, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::DescriptorWriteFailed + ); + + // Invalid buf length + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 0, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::UnexpectedDescriptorSize(1, 0) + ); + + // Invalid buf address + let addr: Vec = vec![0, 0x10000, 0]; + let flags: Vec = vec![0, 0, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 1, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); + assert_eq!( + process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + Error::DescriptorReadFailed + ); + + // Missing buffer for I2C rdwr transfer + let mut buf = Vec::::new(); + let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); + let desc_chains = vec![desc_chain]; + + process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_ERR); + } + #[test] fn verify_backend() { let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); From 7aecd0314c55a56356924347f87b9f5a3a88c800 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 26 Nov 2021 09:57:13 +0530 Subject: [PATCH 5/5] [i2c] Move process_requests to VhostUserI2cBackend Move process_requests() as a function of VhostUserI2cBackend. No other changes. Signed-off-by: Viresh Kumar --- src/i2c/src/vhu_i2c.rs | 338 ++++++++++++++++++++++------------------- 1 file changed, 181 insertions(+), 157 deletions(-) diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index 6e963b8..9582afd 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -99,145 +99,6 @@ pub struct VhostUserI2cBackend { type I2cDescriptorChain = DescriptorChain>>; -/// Process the requests in the vring and dispatch replies -fn process_requests( - i2c_map: &Arc>, - requests: Vec, - vring: Option<&VringRwLock>, -) -> Result { - let mut reqs: Vec = Vec::new(); - - if requests.is_empty() { - return Ok(true); - } - - // Iterate over each I2C request and push it to "reqs" vector. - for desc_chain in requests.clone() { - let descriptors: Vec<_> = desc_chain.clone().collect(); - - if (descriptors.len() != 2) && (descriptors.len() != 3) { - return Err(Error::UnexpectedDescriptorCount(descriptors.len())); - } - - let desc_out_hdr = descriptors[0]; - - if desc_out_hdr.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor(0)); - } - - if desc_out_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize( - size_of::(), - desc_out_hdr.len(), - )); - } - - let out_hdr = desc_chain - .memory() - .read_obj::(desc_out_hdr.addr()) - .map_err(|_| Error::DescriptorReadFailed)?; - - let flags = match out_hdr.flags.to_native() & VIRTIO_I2C_FLAGS_M_RD { - VIRTIO_I2C_FLAGS_M_RD => I2C_M_RD, - _ => 0, - }; - - let desc_in_hdr = descriptors[descriptors.len() - 1]; - if !desc_in_hdr.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor(descriptors.len() - 1)); - } - - if desc_in_hdr.len() as usize != size_of::() { - return Err(Error::UnexpectedDescriptorSize( - size_of::(), - desc_in_hdr.len(), - )); - } - - let (buf, len) = match descriptors.len() { - // Buffer is available - 3 => { - let desc_buf = descriptors[1]; - let len = desc_buf.len(); - - if len == 0 { - return Err(Error::UnexpectedDescriptorSize(1, len)); - } - let mut buf = vec![0; len as usize]; - - if flags != I2C_M_RD { - if desc_buf.is_write_only() { - return Err(Error::UnexpectedWriteOnlyDescriptor(1)); - } - - desc_chain - .memory() - .read(&mut buf, desc_buf.addr()) - .map_err(|_| Error::DescriptorReadFailed)?; - } else if !desc_buf.is_write_only() { - return Err(Error::UnexpectedReadableDescriptor(1)); - } - - (buf, len) - } - - _ => (Vec::::new(), 0), - }; - - reqs.push(I2cReq { - addr: out_hdr.addr.to_native() >> 1, - flags, - len: len as u16, - buf, - }); - } - - let in_hdr = { - VirtioI2cInHdr { - status: match i2c_map.transfer(&mut reqs) { - Ok(()) => VIRTIO_I2C_MSG_OK, - Err(_) => VIRTIO_I2C_MSG_ERR, - }, - } - }; - - for (i, desc_chain) in requests.iter().enumerate() { - let descriptors: Vec<_> = desc_chain.clone().collect(); - let desc_in_hdr = descriptors[descriptors.len() - 1]; - let mut len = size_of::() as u32; - - if descriptors.len() == 3 { - let desc_buf = descriptors[1]; - - // Write the data read from the I2C device - if reqs[i].flags == I2C_M_RD { - desc_chain - .memory() - .write(&reqs[i].buf, desc_buf.addr()) - .map_err(|_| Error::DescriptorWriteFailed)?; - } - - if in_hdr.status == VIRTIO_I2C_MSG_OK { - len += desc_buf.len(); - } - } - - // Write the transfer status - desc_chain - .memory() - .write_obj::(in_hdr, desc_in_hdr.addr()) - .map_err(|_| Error::DescriptorWriteFailed)?; - - if let Some(vring) = vring { - if vring.add_used(desc_chain.head_index(), len).is_err() { - warn!("Couldn't return used descriptors to the ring"); - } - } - } - - Ok(true) -} - impl VhostUserI2cBackend { pub fn new(i2c_map: Arc>) -> Result { Ok(VhostUserI2cBackend { @@ -247,6 +108,145 @@ impl VhostUserI2cBackend { }) } + /// Process the requests in the vring and dispatch replies + fn process_requests( + &self, + requests: Vec, + vring: Option<&VringRwLock>, + ) -> Result { + let mut reqs: Vec = Vec::new(); + + if requests.is_empty() { + return Ok(true); + } + + // Iterate over each I2C request and push it to "reqs" vector. + for desc_chain in requests.clone() { + let descriptors: Vec<_> = desc_chain.clone().collect(); + + if (descriptors.len() != 2) && (descriptors.len() != 3) { + return Err(Error::UnexpectedDescriptorCount(descriptors.len())); + } + + let desc_out_hdr = descriptors[0]; + + if desc_out_hdr.is_write_only() { + return Err(Error::UnexpectedWriteOnlyDescriptor(0)); + } + + if desc_out_hdr.len() as usize != size_of::() { + return Err(Error::UnexpectedDescriptorSize( + size_of::(), + desc_out_hdr.len(), + )); + } + + let out_hdr = desc_chain + .memory() + .read_obj::(desc_out_hdr.addr()) + .map_err(|_| Error::DescriptorReadFailed)?; + + let flags = match out_hdr.flags.to_native() & VIRTIO_I2C_FLAGS_M_RD { + VIRTIO_I2C_FLAGS_M_RD => I2C_M_RD, + _ => 0, + }; + + let desc_in_hdr = descriptors[descriptors.len() - 1]; + if !desc_in_hdr.is_write_only() { + return Err(Error::UnexpectedReadableDescriptor(descriptors.len() - 1)); + } + + if desc_in_hdr.len() as usize != size_of::() { + return Err(Error::UnexpectedDescriptorSize( + size_of::(), + desc_in_hdr.len(), + )); + } + + let (buf, len) = match descriptors.len() { + // Buffer is available + 3 => { + let desc_buf = descriptors[1]; + let len = desc_buf.len(); + + if len == 0 { + return Err(Error::UnexpectedDescriptorSize(1, len)); + } + let mut buf = vec![0; len as usize]; + + if flags != I2C_M_RD { + if desc_buf.is_write_only() { + return Err(Error::UnexpectedWriteOnlyDescriptor(1)); + } + + desc_chain + .memory() + .read(&mut buf, desc_buf.addr()) + .map_err(|_| Error::DescriptorReadFailed)?; + } else if !desc_buf.is_write_only() { + return Err(Error::UnexpectedReadableDescriptor(1)); + } + + (buf, len) + } + + _ => (Vec::::new(), 0), + }; + + reqs.push(I2cReq { + addr: out_hdr.addr.to_native() >> 1, + flags, + len: len as u16, + buf, + }); + } + + let in_hdr = { + VirtioI2cInHdr { + status: match self.i2c_map.transfer(&mut reqs) { + Ok(()) => VIRTIO_I2C_MSG_OK, + Err(_) => VIRTIO_I2C_MSG_ERR, + }, + } + }; + + for (i, desc_chain) in requests.iter().enumerate() { + let descriptors: Vec<_> = desc_chain.clone().collect(); + let desc_in_hdr = descriptors[descriptors.len() - 1]; + let mut len = size_of::() as u32; + + if descriptors.len() == 3 { + let desc_buf = descriptors[1]; + + // Write the data read from the I2C device + if reqs[i].flags == I2C_M_RD { + desc_chain + .memory() + .write(&reqs[i].buf, desc_buf.addr()) + .map_err(|_| Error::DescriptorWriteFailed)?; + } + + if in_hdr.status == VIRTIO_I2C_MSG_OK { + len += desc_buf.len(); + } + } + + // Write the transfer status + desc_chain + .memory() + .write_obj::(in_hdr, desc_in_hdr.addr()) + .map_err(|_| Error::DescriptorWriteFailed)?; + + if let Some(vring) = vring { + if vring.add_used(desc_chain.head_index(), len).is_err() { + warn!("Couldn't return used descriptors to the ring"); + } + } + } + + Ok(true) + } + /// Process the requests in the vring and dispatch replies fn process_queue(&self, vring: &VringRwLock) -> Result { let requests: Vec<_> = vring @@ -256,7 +256,7 @@ impl VhostUserI2cBackend { .map_err(|_| Error::DescriptorNotFound)? .collect(); - if process_requests(&self.i2c_map, requests, Some(vring))? { + if self.process_requests(requests, Some(vring))? { // Send notification once all the requests are processed vring .signal_used_queue() @@ -514,17 +514,20 @@ mod tests { #[test] fn process_requests_success() { let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); - let i2c_map = Arc::new(I2cMap::::new(&device_config).unwrap()); + let i2c_map = I2cMap::::new(&device_config).unwrap(); + let backend = VhostUserI2cBackend::new(Arc::new(i2c_map)).unwrap(); // Descriptor chain size zero, shouldn't fail - process_requests(&i2c_map, Vec::::new(), None).unwrap(); + backend + .process_requests(Vec::::new(), None) + .unwrap(); // Valid single read descriptor let mut buf: Vec = vec![0; 30]; let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); let desc_chains = vec![desc_chain]; - process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + backend.process_requests(desc_chains.clone(), None).unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); // Valid single write descriptor @@ -532,7 +535,7 @@ mod tests { let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, 0, 4); let desc_chains = vec![desc_chain]; - process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + backend.process_requests(desc_chains.clone(), None).unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); // Valid mixed read-write descriptors @@ -552,21 +555,24 @@ mod tests { prepare_desc_chain(GuestAddress(0), &mut buf[5], VIRTIO_I2C_FLAGS_M_RD, 4), ]; - process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + backend.process_requests(desc_chains.clone(), None).unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); } #[test] fn process_requests_failure() { let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); - let i2c_map = Arc::new(I2cMap::::new(&device_config).unwrap()); + let i2c_map = I2cMap::::new(&device_config).unwrap(); + let backend = VhostUserI2cBackend::new(Arc::new(i2c_map)).unwrap(); // One descriptors let flags: Vec = vec![0]; let len: Vec = vec![0]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedDescriptorCount(1) ); @@ -575,7 +581,9 @@ mod tests { let len: Vec = vec![0, 0, 0, 0]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedDescriptorCount(4) ); @@ -588,7 +596,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedWriteOnlyDescriptor(0) ); @@ -597,7 +607,9 @@ mod tests { let len: Vec = vec![100, 1, size_of::() as u32]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedDescriptorSize(size_of::(), 100) ); @@ -611,7 +623,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::DescriptorReadFailed ); @@ -624,7 +638,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedReadableDescriptor(2) ); @@ -633,7 +649,9 @@ mod tests { let len: Vec = vec![size_of::() as u32, 1, 100]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedDescriptorSize(size_of::(), 100) ); @@ -647,7 +665,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::DescriptorWriteFailed ); @@ -660,7 +680,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::UnexpectedDescriptorSize(1, 0) ); @@ -674,7 +696,9 @@ mod tests { ]; let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( - process_requests(&i2c_map, vec![desc_chain], None).unwrap_err(), + backend + .process_requests(vec![desc_chain], None) + .unwrap_err(), Error::DescriptorReadFailed ); @@ -683,7 +707,7 @@ mod tests { let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); let desc_chains = vec![desc_chain]; - process_requests(&i2c_map, desc_chains.clone(), None).unwrap(); + backend.process_requests(desc_chains.clone(), None).unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_ERR); }