From 78da64c7f22a1bf512187c83830b4d7c86c5c5ce Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Mon, 10 Jan 2022 12:48:20 +0530 Subject: [PATCH 1/2] i2c: Always pass vring to process_requests() For tests we can pass a non related vring to just make it run, hence no need of making it optional. Always pass the vring. Signed-off-by: Viresh Kumar --- i2c/src/vhu_i2c.rs | 56 +++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/i2c/src/vhu_i2c.rs b/i2c/src/vhu_i2c.rs index 9ee9129..76360ee 100644 --- a/i2c/src/vhu_i2c.rs +++ b/i2c/src/vhu_i2c.rs @@ -114,7 +114,7 @@ impl VhostUserI2cBackend { fn process_requests( &self, requests: Vec, - vring: Option<&VringRwLock>, + vring: &VringRwLock, ) -> Result { let mut reqs: Vec = Vec::new(); @@ -239,10 +239,8 @@ impl VhostUserI2cBackend { .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"); - } + if vring.add_used(desc_chain.head_index(), len).is_err() { + warn!("Couldn't return used descriptors to the ring"); } } @@ -258,7 +256,7 @@ impl VhostUserI2cBackend { .map_err(|_| Error::DescriptorNotFound)? .collect(); - if self.process_requests(requests, Some(vring))? { + if self.process_requests(requests, vring)? { // Send notification once all the requests are processed vring .signal_used_queue() @@ -518,10 +516,14 @@ mod tests { let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); let i2c_map = I2cMap::::new(&device_config).unwrap(); let backend = VhostUserI2cBackend::new(Arc::new(i2c_map)).unwrap(); + let mem = GuestMemoryAtomic::new( + GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(), + ); + let vring = VringRwLock::new(mem, 0x1000); // Descriptor chain size zero, shouldn't fail backend - .process_requests(Vec::::new(), None) + .process_requests(Vec::::new(), &vring) .unwrap(); // Valid single read descriptor @@ -529,7 +531,9 @@ mod tests { let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); let desc_chains = vec![desc_chain]; - backend.process_requests(desc_chains.clone(), None).unwrap(); + backend + .process_requests(desc_chains.clone(), &vring) + .unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); // Valid single write descriptor @@ -537,7 +541,9 @@ mod tests { let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, 0, 4); let desc_chains = vec![desc_chain]; - backend.process_requests(desc_chains.clone(), None).unwrap(); + backend + .process_requests(desc_chains.clone(), &vring) + .unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); // Valid mixed read-write descriptors @@ -557,7 +563,9 @@ mod tests { prepare_desc_chain(GuestAddress(0), &mut buf[5], VIRTIO_I2C_FLAGS_M_RD, 4), ]; - backend.process_requests(desc_chains.clone(), None).unwrap(); + backend + .process_requests(desc_chains.clone(), &vring) + .unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_OK); } @@ -566,6 +574,10 @@ mod tests { let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); let i2c_map = I2cMap::::new(&device_config).unwrap(); let backend = VhostUserI2cBackend::new(Arc::new(i2c_map)).unwrap(); + let mem = GuestMemoryAtomic::new( + GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(), + ); + let vring = VringRwLock::new(mem, 0x1000); // One descriptors let flags: Vec = vec![0]; @@ -573,7 +585,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedDescriptorCount(1) ); @@ -584,7 +596,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedDescriptorCount(4) ); @@ -599,7 +611,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedWriteOnlyDescriptor(0) ); @@ -610,7 +622,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedDescriptorSize(size_of::(), 100) ); @@ -626,7 +638,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::DescriptorReadFailed ); @@ -641,7 +653,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedReadableDescriptor(2) ); @@ -652,7 +664,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedDescriptorSize(size_of::(), 100) ); @@ -668,7 +680,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::DescriptorWriteFailed ); @@ -683,7 +695,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(None, flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::UnexpectedDescriptorSize(1, 0) ); @@ -699,7 +711,7 @@ mod tests { let desc_chain = prepare_desc_chain_dummy(Some(addr), flags, len); assert_eq!( backend - .process_requests(vec![desc_chain], None) + .process_requests(vec![desc_chain], &vring) .unwrap_err(), Error::DescriptorReadFailed ); @@ -709,7 +721,9 @@ mod tests { let desc_chain = prepare_desc_chain(GuestAddress(0), &mut buf, VIRTIO_I2C_FLAGS_M_RD, 4); let desc_chains = vec![desc_chain]; - backend.process_requests(desc_chains.clone(), None).unwrap(); + backend + .process_requests(desc_chains.clone(), &vring) + .unwrap(); validate_desc_chains(desc_chains, VIRTIO_I2C_MSG_ERR); } From be995502692fc3b41bf793f18630ec06175fc8c9 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 7 Jan 2022 17:09:57 +0530 Subject: [PATCH 2/2] i2c: Improve test coverage Add more tests to increase coverage. Signed-off-by: Viresh Kumar --- coverage_config_x86_64.json | 2 +- i2c/src/i2c.rs | 31 +++++++++++++++++++++++ i2c/src/main.rs | 39 ++++++++++++++++++++++++++--- i2c/src/vhu_i2c.rs | 49 +++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 4 deletions(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 661c0d3..b63482b 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 73.9, + "coverage_score": 86.5, "exclude_path": "", "crate_features": "" } diff --git a/i2c/src/i2c.rs b/i2c/src/i2c.rs index 946a756..928e753 100644 --- a/i2c/src/i2c.rs +++ b/i2c/src/i2c.rs @@ -760,6 +760,14 @@ pub mod tests { verify_rdwr_data(&reqs); } + #[test] + fn test_verify_smbus_data() { + let data = I2cSmbusData { word: 0x050A }; + + assert_eq!(data.read_byte(), 0x0A); + assert_eq!(data.read_word(), 0x050A); + } + #[test] fn test_smbus_transfer() { let adapter_config = AdapterConfig::try_from("1:3").unwrap(); @@ -836,6 +844,16 @@ pub mod tests { i2c_map.transfer(&mut reqs).unwrap(); assert_eq!(reqs[1].buf[0], 1); + // I2C_SMBUS_WRITE (I2C_SMBUS_WORD_DATA) operation + let mut reqs: Vec = vec![I2cReq { + addr: 0x3, + flags: 0, + len: 3, + buf: [7, 4, 3].to_vec(), + }]; + + i2c_map.transfer(&mut reqs).unwrap(); + // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) operation let mut reqs = vec![ I2cReq { @@ -909,6 +927,19 @@ pub mod tests { Error::MessageLengthInvalid("write", 4) ); + // I2C_SMBUS_WRITE (I2C_SMBUS_WORD_DATA) failure operation + let mut reqs: Vec = vec![I2cReq { + addr: 0x3, + // Will cause failure + flags: I2C_M_RD, + len: 3, + buf: [7, 4, 3].to_vec(), + }]; + assert_eq!( + i2c_map.transfer(&mut reqs).unwrap_err(), + Error::MessageLengthInvalid("read", 3) + ); + // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation let mut reqs: Vec = vec![ I2cReq { diff --git a/i2c/src/main.rs b/i2c/src/main.rs index 1dd6b3f..a1a7ced 100644 --- a/i2c/src/main.rs +++ b/i2c/src/main.rs @@ -171,7 +171,9 @@ impl TryFrom for I2cConfiguration { } } -fn start_backend(config: I2cConfiguration) -> Result<()> { +fn start_backend(cmd_args: ArgMatches) -> Result<()> { + let config = I2cConfiguration::try_from(cmd_args).unwrap(); + // The same i2c_map structure instance is shared between all the guests let i2c_map = Arc::new(I2cMap::::new(&config.devices).map_err(Error::I2cFailure)?); @@ -238,13 +240,13 @@ fn main() -> Result<()> { let yaml = load_yaml!("cli.yaml"); let cmd_args = App::from(yaml).get_matches(); - let config = I2cConfiguration::try_from(cmd_args).unwrap(); - start_backend::(config) + start_backend::(cmd_args) } #[cfg(test)] mod tests { use super::*; + use crate::i2c::tests::DummyDevice; impl DeviceConfig { pub fn new_with(adapter_no: u32, addr: Vec) -> Self { @@ -273,6 +275,25 @@ mod tests { app.try_get_matches_from(args).unwrap() } + #[test] + fn test_device_config() { + let mut config = DeviceConfig::new(5); + let invalid_addr = (MAX_I2C_VDEV + 1) as u16; + + config.push(5).unwrap(); + config.push(6).unwrap(); + + assert_eq!( + config.push(invalid_addr).unwrap_err(), + Error::ClientAddressInvalid(invalid_addr) + ); + + assert_eq!( + config.push(5).unwrap_err(), + Error::ClientAddressDuplicate(5) + ); + } + #[test] fn test_parse_failure() { let socket_name = Some("vi2c.sock"); @@ -362,4 +383,16 @@ mod tests { Error::AdapterDuplicate(1) ); } + + #[test] + fn test_fail_listener() { + // This will fail the listeners and thread will panic. + let socket_name = Some("~/path/not/present/i2c"); + let cmd_args = get_cmd_args(socket_name, "1:4,3:5", Some("5")); + + assert_eq!( + start_backend::(cmd_args).unwrap_err(), + Error::FailedJoiningThreads + ); + } } diff --git a/i2c/src/vhu_i2c.rs b/i2c/src/vhu_i2c.rs index 76360ee..d697518 100644 --- a/i2c/src/vhu_i2c.rs +++ b/i2c/src/vhu_i2c.rs @@ -716,6 +716,21 @@ mod tests { Error::DescriptorReadFailed ); + // Write only buf for write operation + let flags: Vec = vec![0, VIRTQ_DESC_F_WRITE, VIRTQ_DESC_F_WRITE]; + let len: Vec = vec![ + size_of::() as u32, + 10, + size_of::() as u32, + ]; + let desc_chain = prepare_desc_chain_dummy(None, flags, len); + assert_eq!( + backend + .process_requests(vec![desc_chain], &vring) + .unwrap_err(), + Error::UnexpectedWriteOnlyDescriptor(1) + ); + // 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); @@ -743,5 +758,39 @@ mod tests { backend.set_event_idx(true); assert!(backend.event_idx); + + assert!(backend.exit_event(0).is_some()); + + let mem = GuestMemoryAtomic::new( + GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(), + ); + backend.update_memory(mem.clone()).unwrap(); + + let vring = VringRwLock::new(mem, 0x1000); + assert_eq!( + backend + .handle_event(0, EventSet::OUT, &[vring.clone()], 0) + .unwrap_err() + .kind(), + io::ErrorKind::Other + ); + + assert_eq!( + backend + .handle_event(1, EventSet::IN, &[vring.clone()], 0) + .unwrap_err() + .kind(), + io::ErrorKind::Other + ); + + // Hit the loop part + backend.set_event_idx(true); + backend + .handle_event(0, EventSet::IN, &[vring.clone()], 0) + .unwrap(); + + // Hit the non-loop part + backend.set_event_idx(false); + backend.handle_event(0, EventSet::IN, &[vring], 0).unwrap(); } }