From 77aa88dc7736f7e59a9336c9d723526d7c00df99 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 15:04:08 +0300 Subject: [PATCH 01/15] [i2c] move parsing of bus id outside of trait impl This is a first change in a series meant for moving all the parsing of strings in a single place, with the goal of separating parsing from the device operation. This helps with separating concerns, mocking, and writing unit tests. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 103a3da..7d1112d 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -232,7 +232,7 @@ pub struct I2cReq { /// I2C adapter and helpers pub trait I2cAdapterTrait: Send + Sync + 'static { - fn new(bus: &str) -> Result + fn new(bus: u32) -> Result where Self: Sized; @@ -307,11 +307,11 @@ pub struct I2cAdapter { } impl I2cAdapterTrait for I2cAdapter { - fn new(bus: &str) -> Result { - let i2cdev = String::from("/dev/i2c-") + bus; + fn new(bus: u32) -> Result { + let i2cdev = format!("/dev/i2c-{}", bus); Ok(I2cAdapter { - bus: bus.parse::().map_err(|_| Error::new(EINVAL))?, + bus, smbus: false, fd: OpenOptions::new().read(true).write(true).open(i2cdev)?, }) @@ -404,7 +404,8 @@ impl I2cMap { for (i, businfo) in busses.iter().enumerate() { let list: Vec<&str> = businfo.split(':').collect(); - let mut adapter = A::new(list[0])?; + let bus_addr = list[0].parse::().map_err(|_| Error::new(EINVAL))?; + let mut adapter = A::new(bus_addr)?; let devices = &list[1..]; adapter.get_func()?; @@ -478,9 +479,9 @@ pub mod tests { } impl I2cAdapterTrait for I2cMockAdapter { - fn new(bus: &str) -> Result { + fn new(bus: u32) -> Result { Ok(I2cMockAdapter { - bus: bus.parse::().map_err(|_| Error::new(EINVAL))?, + bus, smbus: false, result: Ok(()), }) From 850cbce112e7f7105f51e6a068622a36187b4911 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 15:18:16 +0300 Subject: [PATCH 02/15] [i2c] rename bus to adapter_no This seems to be the way that it is defined & used in other examples written for i2c. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 7d1112d..7039338 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -232,17 +232,18 @@ pub struct I2cReq { /// I2C adapter and helpers pub trait I2cAdapterTrait: Send + Sync + 'static { - fn new(bus: u32) -> Result + fn new(adapter_no: u32) -> Result where Self: Sized; - fn bus(&self) -> u32; + fn adapter_no(&self) -> u32; fn is_smbus(&self) -> bool; /// Sets device's address for an I2C adapter. fn set_device_addr(&self, addr: usize) -> Result<()>; /// Gets adapter's functionality + //TODO: this needs to be called as part of new because otherwise is_smbus is invalid. fn get_func(&mut self) -> Result<()>; /// Transfer data @@ -302,23 +303,23 @@ pub trait I2cAdapterTrait: Send + Sync + 'static { pub struct I2cAdapter { fd: File, - bus: u32, + adapter_no: u32, smbus: bool, } impl I2cAdapterTrait for I2cAdapter { - fn new(bus: u32) -> Result { - let i2cdev = format!("/dev/i2c-{}", bus); + fn new(adapter_no: u32) -> Result { + let i2cdev = format!("/dev/i2c-{}", adapter_no); Ok(I2cAdapter { - bus, + adapter_no, smbus: false, fd: OpenOptions::new().read(true).write(true).open(i2cdev)?, }) } - fn bus(&self) -> u32 { - self.bus + fn adapter_no(&self) -> u32 { + self.adapter_no } fn is_smbus(&self) -> bool { @@ -404,8 +405,8 @@ impl I2cMap { for (i, businfo) in busses.iter().enumerate() { let list: Vec<&str> = businfo.split(':').collect(); - let bus_addr = list[0].parse::().map_err(|_| Error::new(EINVAL))?; - let mut adapter = A::new(bus_addr)?; + let adapter_no = list[0].parse::().map_err(|_| Error::new(EINVAL))?; + let mut adapter = A::new(adapter_no)?; let devices = &list[1..]; adapter.get_func()?; @@ -422,7 +423,7 @@ impl I2cMap { println!( "Client address {} is already used by {}", device, - adapters[device_map[device] as usize].bus() + adapters[device_map[device] as usize].adapter_no() ); return Err(Error::new(EADDRINUSE)); } @@ -433,7 +434,7 @@ impl I2cMap { println!( "Added I2C master with bus id: {:x} for devices: {:?}", - adapter.bus(), + adapter.adapter_no(), devices ); @@ -487,7 +488,7 @@ pub mod tests { }) } - fn bus(&self) -> u32 { + fn adapter_no(&self) -> u32 { self.bus } From 1a9b9e3fd661b911dbede540dc95c50febb93923 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 15:19:41 +0300 Subject: [PATCH 03/15] [i2c] separate imports We've typically separated imports in 3 categories: - Rust standard library - external crates - project internal imports (such as imports from user defined modules) Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 3 ++- src/i2c/src/main.rs | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 7039338..65e1870 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -5,9 +5,10 @@ // // SPDX-License-Identifier: Apache-2.0 -use libc::{c_ulong, ioctl, EADDRINUSE, EADDRNOTAVAIL, EINVAL}; use std::fs::{File, OpenOptions}; use std::os::unix::io::AsRawFd; + +use libc::{c_ulong, ioctl, EADDRINUSE, EADDRNOTAVAIL, EINVAL}; use vmm_sys_util::errno::{errno_result, Error, Result}; // The type of the `req` parameter is different for the `musl` library. This will enable diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index 856aae3..d6c6bee 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -8,15 +8,19 @@ mod i2c; mod vhu_i2c; -use clap::{load_yaml, App, ArgMatches}; -use i2c::{I2cAdapter, I2cAdapterTrait, I2cMap}; use std::sync::{Arc, RwLock}; use std::thread::spawn; + +use clap::{load_yaml, App, ArgMatches}; use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; -use vhu_i2c::VhostUserI2cBackend; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; +use i2c::{I2cAdapter, I2cAdapterTrait, I2cMap}; +use std::convert::TryFrom; +use std::num::ParseIntError; +use vhu_i2c::VhostUserI2cBackend; + fn start_daemon( backend: Arc>>, listener: Listener, From 683869411e9b0775a49941131a01cb22c2dde709 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 16:17:10 +0300 Subject: [PATCH 04/15] [i2c] remove get_func from I2cAdapter trait This function does not need to be in the trait. Having it as a public function also makes the code hard to use because the return of the `is_smbus` function is only valid after an initial call to `get_func`. To get rid of this behavior, we can make the `get_func` a private function in the `I2cAdapter` implementation, and call this function on `new`. The function was renamed to `read_func` to make the name more "mutable" compatible (the function needs a mutable reference, but it was called `get`; get functions don't typically need mutable references). Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 69 +++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 37 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 65e1870..333e027 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -243,10 +243,6 @@ pub trait I2cAdapterTrait: Send + Sync + 'static { /// Sets device's address for an I2C adapter. fn set_device_addr(&self, addr: usize) -> Result<()>; - /// Gets adapter's functionality - //TODO: this needs to be called as part of new because otherwise is_smbus is invalid. - fn get_func(&mut self) -> Result<()>; - /// Transfer data fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()>; @@ -308,15 +304,43 @@ pub struct I2cAdapter { smbus: bool, } +impl I2cAdapter { + + // Helper function for reading the adaptor functionalities. + fn read_func(&mut self) -> Result<()> { + let func: u64 = I2C_FUNC_SMBUS_ALL; + + let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_FUNCS, &func) }; + + if ret == -1 { + println!("Failed to get I2C function"); + return errno_result(); + } + + if (func & I2C_FUNC_I2C) != 0 { + self.smbus = false; + } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { + self.smbus = true; + } else { + println!("Invalid functionality {:x}", func); + return Err(Error::new(EINVAL)); + } + + Ok(()) + } +} + impl I2cAdapterTrait for I2cAdapter { fn new(adapter_no: u32) -> Result { let i2cdev = format!("/dev/i2c-{}", adapter_no); - - Ok(I2cAdapter { + let mut adapter = I2cAdapter { adapter_no, smbus: false, - fd: OpenOptions::new().read(true).write(true).open(i2cdev)?, - }) + fd: OpenOptions::new().read(true).write(true).open(i2cdev)? + }; + adapter.read_func()?; + + Ok(adapter) } fn adapter_no(&self) -> u32 { @@ -339,29 +363,6 @@ impl I2cAdapterTrait for I2cAdapter { } } - /// Gets adapter's functionality - fn get_func(&mut self) -> Result<()> { - let func: u64 = I2C_FUNC_SMBUS_ALL; - - let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_FUNCS, &func) }; - - if ret == -1 { - println!("Failed to get I2C function"); - return errno_result(); - } - - if (func & I2C_FUNC_I2C) != 0 { - self.smbus = false; - } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { - self.smbus = true; - } else { - println!("Invalid functionality {:x}", func); - return Err(Error::new(EINVAL)); - } - - Ok(()) - } - /// Transfer data fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()> { let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_RDWR, data) }; @@ -410,8 +411,6 @@ impl I2cMap { let mut adapter = A::new(adapter_no)?; let devices = &list[1..]; - adapter.get_func()?; - for device in devices { let device = device.parse::().map_err(|_| Error::new(EINVAL))?; @@ -501,10 +500,6 @@ pub mod tests { Ok(()) } - fn get_func(&mut self) -> Result<()> { - Ok(()) - } - fn do_i2c_transfer(&self, _data: &I2cRdwrIoctlData, _addr: u16) -> Result<()> { println!("In i2c-transfer"); self.result From f1c4742e7188299ff8cd52e3ede4464730b41514 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 17:34:00 +0300 Subject: [PATCH 05/15] [i2c] reworked the abstractions The I2cAdapterTrait was introduced because of a need to test the i2c implementation without having access to a physical device on the host running the tests. This abstraction though still made the tests pretty hard to write, so it is now replaced by another abstractions: I2cDevice. This abstraction is written working backwards from what we DO NOT have access to on the host where we want to run the tests. This is the i2c device. So, this abstraction is just abstracting the way all the ioctl calls that need to be handled by an i2c device. This way, we can test all the other wrappers that are implemented in this crate. The abstraction is implemented for a physical device to keep backwards compatibility with existing code. This abstraction still needs improvements such as marking the functions as unsafe. For now the tests are commented out because they need to be re-written with this abstraction. Since we still have refactoring work to do (i.e. separate the parsing from the device operation), writing tests is postponed so that it does not involve duplicated work. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 219 ++++++++++++++++++++++------------------- src/i2c/src/main.rs | 21 ++-- src/i2c/src/vhu_i2c.rs | 13 +-- 3 files changed, 138 insertions(+), 115 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 333e027..f4e28d1 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -231,22 +231,101 @@ pub struct I2cReq { pub buf: Vec, } -/// I2C adapter and helpers -pub trait I2cAdapterTrait: Send + Sync + 'static { - fn new(adapter_no: u32) -> Result - where - Self: Sized; +pub struct I2cAdapter { + device: D, + adapter_no: u32, + smbus: bool, +} - fn adapter_no(&self) -> u32; - fn is_smbus(&self) -> bool; +/// Trait that represents an I2C Device. +/// +/// This trait is introduced for development purposes only, and should not +/// be used outside of this crate. The purpose of this trait is to provide a +/// mock implementation for the I2C driver so that we can test the I2C +/// functionality without the need of a physical device. +pub trait I2cDevice { + // Open the device specified by path. + fn open(device_path: String) -> Result where Self: Sized; - /// Sets device's address for an I2C adapter. - fn set_device_addr(&self, addr: usize) -> Result<()>; + // Corresponds to the I2C_FUNCS ioctl call. + fn funcs(&mut self, func: u64) -> i32; - /// Transfer data - fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()>; + // Corresponds to the I2C_RDWR ioctl call. + fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32; - fn do_smbus_transfer(&self, data: &I2cSmbusIoctlData, addr: u16) -> Result<()>; + // Corresponds to the I2C_SMBUS ioctl call. + fn smbus(&self, data: &I2cSmbusIoctlData) -> i32; + + // Corresponds to the I2C_SLAVE ioctl call. + fn slave(&self, addr: u64) -> i32; +} + +/// A physical I2C device. This structure can only be initialized on hosts +/// where `/dev/i2c-XX` is available. +pub struct PhysDevice { + file: File, +} + +impl I2cDevice for PhysDevice { + fn open(device_path: String) -> Result { + Ok(PhysDevice { + file: OpenOptions::new().read(true).write(true).open(device_path)? + }) + } + + fn funcs(&mut self, func: u64) -> i32 { + unsafe { ioctl(self.file.as_raw_fd(), I2C_FUNCS, &func) } + } + + fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32 { + unsafe { ioctl(self.file.as_raw_fd(), I2C_RDWR, data) } + } + + fn smbus(&self, data: &I2cSmbusIoctlData) -> i32 { + unsafe { ioctl(self.file.as_raw_fd(), I2C_SMBUS, data) } + } + + fn slave(&self, addr: u64) -> i32 { + unsafe { ioctl(self.file.as_raw_fd(), I2C_SLAVE, addr as c_ulong) } + } +} + +impl I2cAdapter { + // Creates a new adapter corresponding to the specified number. + fn new(adapter_no: u32) -> Result> { + let i2cdev = format!("/dev/i2c-{}", adapter_no); + let mut adapter = I2cAdapter { + adapter_no, + smbus: false, + device: D::open(i2cdev)? + }; + adapter.read_func()?; + + Ok(adapter) + } + + // Helper function for reading the adaptor functionalities. + fn read_func(&mut self) -> Result<()> { + let func: u64 = I2C_FUNC_SMBUS_ALL; + + let ret = self.device.funcs(func); + + if ret == -1 { + println!("Failed to get I2C function"); + return errno_result(); + } + + if (func & I2C_FUNC_I2C) != 0 { + self.smbus = false; + } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { + self.smbus = true; + } else { + println!("Invalid functionality {:x}", func); + return Err(Error::new(EINVAL)); + } + + Ok(()) + } /// Perform I2C_RDWR transfer fn i2c_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { @@ -268,14 +347,24 @@ pub trait I2cAdapterTrait: Send + Sync + 'static { nmsgs: len as u32, }; - self.do_i2c_transfer(&data, addr) + let ret = self.device.rdwr(&data); + if ret == -1 { + println!("Failed to transfer i2c data to device addr to {:x}", addr); + errno_result() + } else { + Ok(()) + } } /// Perform I2C_SMBUS transfer fn smbus_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { let smbus_data = I2cSmbusIoctlData::new(reqs)?; - self.do_smbus_transfer(&smbus_data, reqs[0].addr)?; + let ret = self.device.smbus(&smbus_data); + if ret == -1 { + println!("Failed to transfer smbus data to device addr to {:x}", reqs[0].addr); + return errno_result(); + } if smbus_data.read_write == I2C_SMBUS_READ { unsafe { @@ -296,52 +385,6 @@ pub trait I2cAdapterTrait: Send + Sync + 'static { } Ok(()) } -} - -pub struct I2cAdapter { - fd: File, - adapter_no: u32, - smbus: bool, -} - -impl I2cAdapter { - - // Helper function for reading the adaptor functionalities. - fn read_func(&mut self) -> Result<()> { - let func: u64 = I2C_FUNC_SMBUS_ALL; - - let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_FUNCS, &func) }; - - if ret == -1 { - println!("Failed to get I2C function"); - return errno_result(); - } - - if (func & I2C_FUNC_I2C) != 0 { - self.smbus = false; - } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { - self.smbus = true; - } else { - println!("Invalid functionality {:x}", func); - return Err(Error::new(EINVAL)); - } - - Ok(()) - } -} - -impl I2cAdapterTrait for I2cAdapter { - fn new(adapter_no: u32) -> Result { - let i2cdev = format!("/dev/i2c-{}", adapter_no); - let mut adapter = I2cAdapter { - adapter_no, - smbus: false, - fd: OpenOptions::new().read(true).write(true).open(i2cdev)? - }; - adapter.read_func()?; - - Ok(adapter) - } fn adapter_no(&self) -> u32 { self.adapter_no @@ -353,7 +396,7 @@ impl I2cAdapterTrait for I2cAdapter { /// Sets device's address for an I2C adapter. fn set_device_addr(&self, addr: usize) -> Result<()> { - let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_SLAVE, addr as c_ulong) }; + let ret = self.device.slave(addr as u64); if ret == -1 { println!("Failed to set device addr to {:x}", addr); @@ -363,52 +406,37 @@ impl I2cAdapterTrait for I2cAdapter { } } - /// Transfer data - fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()> { - let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_RDWR, data) }; - - if ret == -1 { - println!("Failed to transfer i2c data to device addr to {:x}", addr); - errno_result() + fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { + if self.is_smbus() { + self.smbus_transfer(reqs) } else { - Ok(()) + self.i2c_transfer(reqs) } } - - fn do_smbus_transfer(&self, data: &I2cSmbusIoctlData, addr: u16) -> Result<()> { - let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_SMBUS, data) }; - - if ret == -1 { - println!("Failed to transfer smbus data to device addr to {:x}", addr); - return errno_result(); - } - - Ok(()) - } } /// I2C map and helpers const MAX_I2C_VDEV: usize = 1 << 7; const I2C_INVALID_ADAPTER: u32 = 0xFFFFFFFF; -pub struct I2cMap { - adapters: Vec, +pub struct I2cMap { + adapters: Vec>, device_map: [u32; MAX_I2C_VDEV], } -impl I2cMap { +impl I2cMap { pub fn new(list: &str) -> Result where Self: Sized, { let mut device_map: [u32; MAX_I2C_VDEV] = [I2C_INVALID_ADAPTER; MAX_I2C_VDEV]; - let mut adapters: Vec = Vec::new(); + let mut adapters: Vec> = Vec::new(); let busses: Vec<&str> = list.split(',').collect(); for (i, businfo) in busses.iter().enumerate() { let list: Vec<&str> = businfo.split(':').collect(); let adapter_no = list[0].parse::().map_err(|_| Error::new(EINVAL))?; - let mut adapter = A::new(adapter_no)?; + let mut adapter = I2cAdapter::new(adapter_no)?; let devices = &list[1..]; for device in devices { @@ -449,6 +477,8 @@ impl I2cMap { pub fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { let device = reqs[0].addr as usize; + + // identify the device in the device_map let index = self.device_map[device]; // This can happen a lot while scanning the bus, don't print any errors. @@ -456,16 +486,12 @@ impl I2cMap { return Err(Error::new(EINVAL)); } + // get the corresponding adapter based on teh device config. let adapter = &self.adapters[index as usize]; // Set device's address adapter.set_device_addr(device)?; - - if adapter.is_smbus() { - adapter.smbus_transfer(reqs) - } else { - adapter.i2c_transfer(reqs) - } + adapter.transfer(reqs) } } @@ -479,6 +505,7 @@ pub mod tests { result: Result<()>, } + /* impl I2cAdapterTrait for I2cMockAdapter { fn new(bus: u32) -> Result { Ok(I2cMockAdapter { @@ -500,13 +527,7 @@ pub mod tests { Ok(()) } - fn do_i2c_transfer(&self, _data: &I2cRdwrIoctlData, _addr: u16) -> Result<()> { - println!("In i2c-transfer"); - self.result - } - - fn do_smbus_transfer(&self, _data: &I2cSmbusIoctlData, _addr: u16) -> Result<()> { - println!("In smbus-transfer"); + fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { self.result } } @@ -667,5 +688,5 @@ pub mod tests { }); assert_results(&mut i2c_map, &mut reqs, true, true); - } + }*/ } diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index d6c6bee..e4ddea2 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -16,13 +16,13 @@ use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; -use i2c::{I2cAdapter, I2cAdapterTrait, I2cMap}; +use i2c::{I2cDevice, I2cMap, PhysDevice}; use std::convert::TryFrom; use std::num::ParseIntError; use vhu_i2c::VhostUserI2cBackend; -fn start_daemon( - backend: Arc>>, +fn start_daemon( + backend: Arc>>, listener: Listener, ) -> bool { let mut daemon = VhostUserDaemon::new( @@ -57,9 +57,9 @@ fn start_daemon( false } -fn start_backend( +fn start_backend( cmd_args: ArgMatches, - start_daemon: fn(Arc>>, Listener) -> bool, + start_daemon: fn(Arc>>, Listener) -> bool, ) -> Result<(), String> { let mut handles = Vec::new(); @@ -77,7 +77,7 @@ fn start_backend( // The same i2c_map structure instance is shared between all the guests let i2c_map = - Arc::new(I2cMap::::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?); + Arc::new(I2cMap::::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?); for i in 0..count { let socket = path.to_owned() + &i.to_string(); @@ -108,7 +108,7 @@ fn main() -> Result<(), String> { let yaml = load_yaml!("cli.yaml"); let cmd_args = App::from(yaml).get_matches(); - start_backend::(cmd_args, start_daemon) + start_backend::(cmd_args, start_daemon) } #[cfg(test)] @@ -137,13 +137,13 @@ mod tests { } } - fn mock_start_daemon( - _backend: Arc>>, + fn mock_start_daemon( + _backend: Arc>>, _listener: Listener, ) -> bool { true } - + /* #[test] fn test_backend_single() { let cmd_args = get_cmd_args("vi2c.sock_single", "1:4,2:32:21,5:5:23", 0); @@ -167,4 +167,5 @@ mod tests { let cmd_args = get_cmd_args("vi2c.sock_duplicate", "1:4,2:32:21,5:4:23", 5); assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); } + */ } diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index cac7368..5460c32 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -84,15 +84,15 @@ struct VirtioI2cInHdr { } unsafe impl ByteValued for VirtioI2cInHdr {} -pub struct VhostUserI2cBackend { - i2c_map: Arc>, +pub struct VhostUserI2cBackend { + i2c_map: Arc>, event_idx: bool, mem: Option>, pub exit_event: EventFd, } -impl VhostUserI2cBackend { - pub fn new(i2c_map: Arc>) -> Result { +impl VhostUserI2cBackend { + pub fn new(i2c_map: Arc>) -> Result { Ok(VhostUserI2cBackend { i2c_map, event_idx: false, @@ -220,7 +220,7 @@ impl VhostUserI2cBackend { } /// VhostUserBackendMut trait methods -impl VhostUserBackendMut for VhostUserI2cBackend { +impl VhostUserBackendMut for VhostUserI2cBackend { fn num_queues(&self) -> usize { NUM_QUEUES } @@ -305,6 +305,7 @@ mod tests { use super::*; use crate::i2c::tests::I2cMockAdapter; + /* #[test] fn verify_backend() { let i2c_map: I2cMap = I2cMap::new("1:4,2:32:21,5:10:23").unwrap(); @@ -320,5 +321,5 @@ mod tests { backend.set_event_idx(true); assert!(backend.event_idx); - } + }*/ } From 162e1adc4fbbeb6a63df518ef6fe355e9095e27f Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 18:07:49 +0300 Subject: [PATCH 06/15] [i2c] separate parsing of strings from device This separation is needed so we can easily write unit tests for parsing without needing to create full blown objects. (as a side note, before this is possible, we also need to get rid of errno, and replace it with custom Errors, so that we can also write the much needed negative tests). This separation is achieved through creating configuration structures that can be either programatically initialized, or initialized through parsing the command line parameters. This commit is still WIP because we also need to make sure that configuration objects can only be created valid (to reduce some risks for future extensions where parameters might be passed some other way rather than yaml). Also, we need to move the check for uniquness of device addresses in the DeviceConfig instead of the `I2cMap`. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 46 ++++++++++++------------- src/i2c/src/main.rs | 82 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 87 insertions(+), 41 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index f4e28d1..3815219 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -416,7 +416,7 @@ impl I2cAdapter { } /// I2C map and helpers -const MAX_I2C_VDEV: usize = 1 << 7; +pub(crate) const MAX_I2C_VDEV: usize = 1 << 7; const I2C_INVALID_ADAPTER: u32 = 0xFFFFFFFF; pub struct I2cMap { @@ -424,46 +424,46 @@ pub struct I2cMap { device_map: [u32; MAX_I2C_VDEV], } +pub(crate) struct DeviceConfig { + pub(crate) adapter_no: u32, + pub(crate) addr: Vec, +} + +pub(crate) struct I2cConfiguration { + pub(crate) socket_path: String, + pub(crate) socket_count: usize, + pub(crate) devices: Vec, +} + impl I2cMap { - pub fn new(list: &str) -> Result + pub(crate) fn new(list: &Vec) -> Result where Self: Sized, { let mut device_map: [u32; MAX_I2C_VDEV] = [I2C_INVALID_ADAPTER; MAX_I2C_VDEV]; let mut adapters: Vec> = Vec::new(); - let busses: Vec<&str> = list.split(',').collect(); - for (i, businfo) in busses.iter().enumerate() { - let list: Vec<&str> = businfo.split(':').collect(); - let adapter_no = list[0].parse::().map_err(|_| Error::new(EINVAL))?; - let mut adapter = I2cAdapter::new(adapter_no)?; - let devices = &list[1..]; + for (i, device_cfg) in list.iter().enumerate() { + let adapter = I2cAdapter::new(device_cfg.adapter_no)?; - for device in devices { - let device = device.parse::().map_err(|_| Error::new(EINVAL))?; - - if device > MAX_I2C_VDEV { - println!("Invalid device address {}", device); - return Err(Error::new(EADDRNOTAVAIL)); - } - - if device_map[device] != I2C_INVALID_ADAPTER { + for addr in &device_cfg.addr { + if device_map[*addr as usize] != I2C_INVALID_ADAPTER { println!( "Client address {} is already used by {}", - device, - adapters[device_map[device] as usize].adapter_no() + *addr, + adapters[device_map[*addr as usize] as usize].adapter_no() ); return Err(Error::new(EADDRINUSE)); } - adapter.set_device_addr(device)?; - device_map[device] = i as u32; + // Calling this to check that the `addr` is valid. + adapter.set_device_addr(*addr)?; + device_map[*addr as usize] = i as u32; } println!( - "Added I2C master with bus id: {:x} for devices: {:?}", + "Added I2C master with bus id: {:x} for devices", adapter.adapter_no(), - devices ); adapters.push(adapter); diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index e4ddea2..d108df5 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -8,6 +8,8 @@ mod i2c; mod vhu_i2c; +use std::convert::TryFrom; +use std::num::ParseIntError; use std::sync::{Arc, RwLock}; use std::thread::spawn; @@ -16,11 +18,63 @@ use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; -use i2c::{I2cDevice, I2cMap, PhysDevice}; -use std::convert::TryFrom; -use std::num::ParseIntError; +use i2c::{DeviceConfig, I2cConfiguration, I2cDevice, I2cMap, PhysDevice, MAX_I2C_VDEV}; use vhu_i2c::VhostUserI2cBackend; +impl TryFrom for I2cConfiguration { + type Error = String; + + fn try_from(cmd_args: ArgMatches) -> Result { + let socket_path = cmd_args + .value_of("socket_path") + .ok_or("Invalid socket path")? + .to_string(); + + let socket_count = cmd_args + .value_of("socket_count") + .unwrap_or("1") + .parse::() + .map_err(|_| "Invalid socket_count")?; + + let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; + let busses: Vec<&str> = list.split(',').collect(); + + let mut devices = Vec::new(); + + for businfo in busses.iter() { + let list: Vec<&str> = businfo.split(':').collect(); + let bus_addr = list[0].parse::().map_err(|_| "Invalid bus address")?; + let bus_devices = list[1..] + .iter() + .map(|str| str.parse::()) + .collect::, ParseIntError>>() + .map_err(|_| "Invalid device")?; + + // Check if any of the devices has a size > the maximum allowed one. + if bus_devices + .iter() + .filter(|addr| **addr > MAX_I2C_VDEV) + .count() + > 0 + { + // TODO: if needed we can show which one is actually not respecting the max size. + return Err("Invalid addr.".to_string()); + } + + devices.push(DeviceConfig { + adapter_no: bus_addr, + addr: bus_devices, + }) + } + + Ok(I2cConfiguration { + socket_path, + socket_count, + devices, + }) + } +} + fn start_daemon( backend: Arc>>, listener: Listener, @@ -63,24 +117,16 @@ fn start_backend( ) -> Result<(), String> { let mut handles = Vec::new(); - let path = cmd_args - .value_of("socket_path") - .ok_or("Invalid socket path")?; - - let count = cmd_args - .value_of("socket_count") - .unwrap_or("1") - .parse::() - .map_err(|_| "Invalid socket_count")?; - - let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; + let i2c_config = I2cConfiguration::try_from(cmd_args)?; // The same i2c_map structure instance is shared between all the guests - let i2c_map = - Arc::new(I2cMap::::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?); + let i2c_map = Arc::new( + I2cMap::::new(&i2c_config.devices) + .map_err(|e| format!("Failed to create i2c_map ({})", e))?, + ); - for i in 0..count { - let socket = path.to_owned() + &i.to_string(); + for i in 0..i2c_config.socket_count { + let socket = i2c_config.socket_path.to_owned() + &i.to_string(); let i2c_map = i2c_map.clone(); let handle = spawn(move || loop { From 9c019f4657b0f6e3f00726072f004daa0d1a0020 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 18:12:17 +0300 Subject: [PATCH 07/15] [i2c] fix formatting Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 16 ++++++++++++---- src/i2c/src/vhu_i2c.rs | 4 +++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 3815219..a0849aa 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -245,7 +245,9 @@ pub struct I2cAdapter { /// functionality without the need of a physical device. pub trait I2cDevice { // Open the device specified by path. - fn open(device_path: String) -> Result where Self: Sized; + fn open(device_path: String) -> Result + where + Self: Sized; // Corresponds to the I2C_FUNCS ioctl call. fn funcs(&mut self, func: u64) -> i32; @@ -269,7 +271,10 @@ pub struct PhysDevice { impl I2cDevice for PhysDevice { fn open(device_path: String) -> Result { Ok(PhysDevice { - file: OpenOptions::new().read(true).write(true).open(device_path)? + file: OpenOptions::new() + .read(true) + .write(true) + .open(device_path)?, }) } @@ -297,7 +302,7 @@ impl I2cAdapter { let mut adapter = I2cAdapter { adapter_no, smbus: false, - device: D::open(i2cdev)? + device: D::open(i2cdev)?, }; adapter.read_func()?; @@ -362,7 +367,10 @@ impl I2cAdapter { let ret = self.device.smbus(&smbus_data); if ret == -1 { - println!("Failed to transfer smbus data to device addr to {:x}", reqs[0].addr); + println!( + "Failed to transfer smbus data to device addr to {:x}", + reqs[0].addr + ); return errno_result(); } diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index 5460c32..3ff538a 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -220,7 +220,9 @@ impl VhostUserI2cBackend { } /// VhostUserBackendMut trait methods -impl VhostUserBackendMut for VhostUserI2cBackend { +impl VhostUserBackendMut + for VhostUserI2cBackend +{ fn num_queues(&self) -> usize { NUM_QUEUES } From db8545c99a1568d73b1381e60e0988844ed1be8c Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 22 Sep 2021 19:19:45 +0300 Subject: [PATCH 08/15] [i2c] disallow invalid configurations of I2cMap This helps with simplifying the code by keeping all checks at init time. In practice it means that invalid objects can no longer be created, which simplifies the testing. All objects are created through either new, or `TryFrom`, and they can only be updated (outside of test functions) via functions that are checking the validity of the resulting object. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 80 +++++++++++++++++++++++++++++++++------------ src/i2c/src/main.rs | 66 +++++++++++++++++++------------------ 2 files changed, 94 insertions(+), 52 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index a0849aa..23d201d 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -8,7 +8,7 @@ use std::fs::{File, OpenOptions}; use std::os::unix::io::AsRawFd; -use libc::{c_ulong, ioctl, EADDRINUSE, EADDRNOTAVAIL, EINVAL}; +use libc::{c_ulong, ioctl, EINVAL}; use vmm_sys_util::errno::{errno_result, Error, Result}; // The type of the `req` parameter is different for the `musl` library. This will enable @@ -433,39 +433,79 @@ pub struct I2cMap { } pub(crate) struct DeviceConfig { - pub(crate) adapter_no: u32, - pub(crate) addr: Vec, + adapter_no: u32, + addr: Vec, } -pub(crate) struct I2cConfiguration { - pub(crate) socket_path: String, - pub(crate) socket_count: usize, - pub(crate) devices: Vec, +impl DeviceConfig { + pub fn new(adapter_no: u32) -> Self { + DeviceConfig { + adapter_no, + addr: Vec::new(), + } + } + + pub fn push(&mut self, addr: u16) -> std::result::Result<(), String> { + if addr as usize > MAX_I2C_VDEV { + return Err(format!("Invalid address: {} (> maximum allowed)", addr)); + } + + if self.addr.contains(&addr) { + return Err(format!("Address already in use: {}", addr)); + } + + self.addr.push(addr); + Ok(()) + } +} + +pub(crate) struct AdapterConfig { + inner: Vec, +} + +impl AdapterConfig { + pub fn new() -> Self { + Self { inner: Vec::new() } + } + + fn contains_adapter_no(&self, adapter_no: u32) -> bool { + self.inner.iter().any(|elem| elem.adapter_no == adapter_no) + } + + fn contains_addr(&self, addr: u16) -> bool { + self.inner.iter().any(|elem| elem.addr.contains(&addr)) + } + + pub fn push(&mut self, device: DeviceConfig) -> std::result::Result<(), String> { + if self.contains_adapter_no(device.adapter_no) { + return Err("Duplicated adapter number".to_string()); + } + + for addr in device.addr.iter() { + if self.contains_addr(*addr) { + return Err(format!("Address already in use: {}", addr)); + } + } + + self.inner.push(device); + Ok(()) + } } impl I2cMap { - pub(crate) fn new(list: &Vec) -> Result + pub(crate) fn new(device_config: &AdapterConfig) -> Result where Self: Sized, { let mut device_map: [u32; MAX_I2C_VDEV] = [I2C_INVALID_ADAPTER; MAX_I2C_VDEV]; let mut adapters: Vec> = Vec::new(); - for (i, device_cfg) in list.iter().enumerate() { + for (i, device_cfg) in device_config.inner.iter().enumerate() { let adapter = I2cAdapter::new(device_cfg.adapter_no)?; + // Check that all addresses corresponding to the adapter are valid. for addr in &device_cfg.addr { - if device_map[*addr as usize] != I2C_INVALID_ADAPTER { - println!( - "Client address {} is already used by {}", - *addr, - adapters[device_map[*addr as usize] as usize].adapter_no() - ); - return Err(Error::new(EADDRINUSE)); - } - - // Calling this to check that the `addr` is valid. - adapter.set_device_addr(*addr)?; + adapter.set_device_addr(*addr as usize)?; device_map[*addr as usize] = i as u32; } diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index d108df5..b76fabf 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -9,7 +9,6 @@ mod i2c; mod vhu_i2c; use std::convert::TryFrom; -use std::num::ParseIntError; use std::sync::{Arc, RwLock}; use std::thread::spawn; @@ -18,9 +17,41 @@ use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; -use i2c::{DeviceConfig, I2cConfiguration, I2cDevice, I2cMap, PhysDevice, MAX_I2C_VDEV}; +use crate::i2c::DeviceConfig; +use i2c::{AdapterConfig, I2cDevice, I2cMap, PhysDevice}; use vhu_i2c::VhostUserI2cBackend; +struct I2cConfiguration { + socket_path: String, + socket_count: usize, + devices: AdapterConfig, +} + +impl TryFrom<&str> for AdapterConfig { + type Error = String; + + fn try_from(list: &str) -> Result { + let busses: Vec<&str> = list.split(',').collect(); + let mut devices = AdapterConfig::new(); + + for businfo in busses.iter() { + let list: Vec<&str> = businfo.split(':').collect(); + let bus_addr = list[0].parse::().map_err(|_| "Invalid bus address")?; + let mut adapter = DeviceConfig::new(bus_addr); + + for device_str in list[1..].iter() { + let addr = device_str + .parse::() + .map_err(|_| "Invalid device addr: {}")?; + adapter.push(addr)?; + } + + devices.push(adapter)?; + } + Ok(devices) + } +} + impl TryFrom for I2cConfiguration { type Error = String; @@ -37,36 +68,7 @@ impl TryFrom for I2cConfiguration { .map_err(|_| "Invalid socket_count")?; let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; - let busses: Vec<&str> = list.split(',').collect(); - - let mut devices = Vec::new(); - - for businfo in busses.iter() { - let list: Vec<&str> = businfo.split(':').collect(); - let bus_addr = list[0].parse::().map_err(|_| "Invalid bus address")?; - let bus_devices = list[1..] - .iter() - .map(|str| str.parse::()) - .collect::, ParseIntError>>() - .map_err(|_| "Invalid device")?; - - // Check if any of the devices has a size > the maximum allowed one. - if bus_devices - .iter() - .filter(|addr| **addr > MAX_I2C_VDEV) - .count() - > 0 - { - // TODO: if needed we can show which one is actually not respecting the max size. - return Err("Invalid addr.".to_string()); - } - - devices.push(DeviceConfig { - adapter_no: bus_addr, - addr: bus_devices, - }) - } - + let devices = AdapterConfig::try_from(list)?; Ok(I2cConfiguration { socket_path, socket_count, From b49993b636cfb91b72980238acd44769ae80aebe Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 22 Sep 2021 19:22:09 +0300 Subject: [PATCH 09/15] [i2c] update existing unit tests Updated the existing unit tests to map to the new implemented abstraction. In the process of updating the tests, a few other worth mentioning changes are implemented: - when initializing a resource needed for the test, it is a best practice to keep any updates that change the test result in the same function. This was not followed as the requests vector was initialized in the test, and was cleared in a function that was asserting results. This is an unexpected behavior, and is now removed. The requests are always initialized in the test before using them (and clear is no longer used). - `assert_results` is deleted because it was hard to read which results where asserted. It is also not necessary to mock functions in that way because now we can mock only the parts we need from the device implementation. The tests are incomplete though because we also need to check the error returned. This should be implemented in a future commit to keep things separated. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 217 +++++++++++++++++++++-------------------- src/i2c/src/main.rs | 13 ++- src/i2c/src/vhu_i2c.rs | 9 +- 3 files changed, 122 insertions(+), 117 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 23d201d..2190808 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -546,67 +546,60 @@ impl I2cMap { #[cfg(test)] pub mod tests { use super::*; + use std::convert::TryFrom; - pub struct I2cMockAdapter { - bus: u32, - smbus: bool, - result: Result<()>, + #[derive(Debug, Default)] + pub struct DummyDevice { + funcs_result: i32, + rdwr_result: i32, + smbus_result: i32, + slave_result: i32, } - /* - impl I2cAdapterTrait for I2cMockAdapter { - fn new(bus: u32) -> Result { - Ok(I2cMockAdapter { - bus, - smbus: false, - result: Ok(()), - }) + impl I2cDevice for DummyDevice { + fn open(_device_path: String) -> Result + where + Self: Sized, + { + Ok(DummyDevice::default()) } - fn adapter_no(&self) -> u32 { - self.bus + fn funcs(&mut self, _func: u64) -> i32 { + self.funcs_result } - fn is_smbus(&self) -> bool { - self.smbus + fn rdwr(&self, _data: &I2cRdwrIoctlData) -> i32 { + self.rdwr_result } - fn set_device_addr(&self, _addr: usize) -> Result<()> { - Ok(()) + fn smbus(&self, _data: &I2cSmbusIoctlData) -> i32 { + self.smbus_result } - fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { - self.result + fn slave(&self, _addr: u64) -> i32 { + self.slave_result } } - fn assert_results( - i2c_map: &mut I2cMap, - reqs: &mut Vec, - before: bool, - after: bool, - ) { - i2c_map.adapters[0].result = Ok(()); - assert_eq!(i2c_map.transfer(reqs).is_err(), before); - i2c_map.adapters[0].result = Err(Error::new(EINVAL)); - assert_eq!(i2c_map.transfer(reqs).is_err(), after); - - reqs.clear(); - } - #[test] fn test_i2c_map_duplicate_device4() { - assert!(I2cMap::::new("1:4,2:32:21,5:4:23").is_err()); + assert!(AdapterConfig::try_from("1:4,2:32:21,5:4:23").is_err()); + } + + #[test] + fn test_duplicated_adapter_no() { + assert!(AdapterConfig::try_from("1:4,1:32:21,5:10:23").is_err()); } #[test] fn test_i2c_map() { - let i2c_map: I2cMap = I2cMap::new("1:4,2:32:21,5:10:23").unwrap(); + let adapter_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); + let i2c_map: I2cMap = I2cMap::new(&adapter_config).unwrap(); assert_eq!(i2c_map.adapters.len(), 3); - assert_eq!(i2c_map.adapters[0].bus, 1); - assert_eq!(i2c_map.adapters[1].bus, 2); - assert_eq!(i2c_map.adapters[2].bus, 5); + assert_eq!(i2c_map.adapters[0].adapter_no, 1); + assert_eq!(i2c_map.adapters[1].adapter_no, 2); + assert_eq!(i2c_map.adapters[2].adapter_no, 5); assert_eq!(i2c_map.device_map[4], 0); assert_eq!(i2c_map.device_map[32], 1); @@ -617,22 +610,26 @@ pub mod tests { #[test] fn test_i2c_transfer() { - let mut i2c_map: I2cMap = I2cMap::new("1:3").unwrap(); + let adapter_config = AdapterConfig::try_from("1:3").unwrap(); + let mut i2c_map: I2cMap = I2cMap::new(&adapter_config).unwrap(); + i2c_map.adapters[0].smbus = false; let mut reqs: Vec = vec![I2cReq { addr: 0x3, flags: 0, len: 2, - buf: [7, 4].to_vec(), + buf: vec![7, 4], }]; - assert_results(&mut i2c_map, &mut reqs, false, true); + i2c_map.transfer(&mut *reqs).unwrap(); } #[test] fn test_smbus_transfer() { - let mut i2c_map: I2cMap = I2cMap::new("1:3").unwrap(); + let adapter_config = AdapterConfig::try_from("1:3").unwrap(); + let mut i2c_map: I2cMap = I2cMap::new(&adapter_config).unwrap(); + i2c_map.adapters[0].smbus = true; let mut reqs: Vec = vec![I2cReq { @@ -643,28 +640,30 @@ pub mod tests { }]; // I2C_SMBUS_WRITE (I2C_SMBUS_BYTE_DATA) operation - assert_results(&mut i2c_map, &mut reqs, false, true); + i2c_map.transfer(&mut reqs).unwrap(); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) operation - reqs.push(I2cReq { - addr: 0x3, - flags: 0, - len: 1, - buf: [34].to_vec(), - }); - reqs.push(I2cReq { - addr: 0x3, - flags: 1, - len: 2, - buf: [3, 4].to_vec(), - }); - - assert_results(&mut i2c_map, &mut reqs, false, true); + let mut reqs = vec![ + I2cReq { + addr: 0x3, + flags: 0, + len: 1, + buf: [34].to_vec(), + }, + I2cReq { + addr: 0x3, + flags: 1, + len: 2, + buf: [3, 4].to_vec(), + }, + ]; + i2c_map.transfer(&mut reqs).unwrap(); } #[test] fn test_smbus_transfer_failure() { - let mut i2c_map: I2cMap = I2cMap::new("1:3").unwrap(); + let adapter_config = AdapterConfig::try_from("1:3").unwrap(); + let mut i2c_map: I2cMap = I2cMap::new(&adapter_config).unwrap(); i2c_map.adapters[0].smbus = true; let mut reqs: Vec = vec![ @@ -684,57 +683,63 @@ pub mod tests { ]; // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation - assert_results(&mut i2c_map, &mut reqs, true, true); + // TODO: check the actual error once we have an error type defined. + // TODO-continued: otherwise this test is unreliable because it might + // fail for another reason than the expected one. + assert!(i2c_map.transfer(&mut reqs).is_err()); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation - reqs.push(I2cReq { - addr: 0x3, - flags: 0, - len: 1, - buf: [34].to_vec(), - }); - reqs.push(I2cReq { - addr: 0x3, - // Will cause failure - flags: 0, - len: 2, - buf: [3, 4].to_vec(), - }); - - assert_results(&mut i2c_map, &mut reqs, true, true); + let mut reqs = vec![ + I2cReq { + addr: 0x3, + flags: 0, + len: 1, + buf: [34].to_vec(), + }, + I2cReq { + addr: 0x3, + // Will cause failure + flags: 0, + len: 2, + buf: [3, 4].to_vec(), + }, + ]; + assert!(i2c_map.transfer(&mut reqs).is_err()); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation - reqs.push(I2cReq { - addr: 0x3, - flags: 0, - // Will cause failure - len: 2, - buf: [3, 4].to_vec(), - }); - reqs.push(I2cReq { - addr: 0x3, - flags: 1, - len: 2, - buf: [3, 4].to_vec(), - }); - - assert_results(&mut i2c_map, &mut reqs, true, true); + let mut reqs = vec![ + I2cReq { + addr: 0x3, + flags: 0, + // Will cause failure + len: 2, + buf: [3, 4].to_vec(), + }, + I2cReq { + addr: 0x3, + flags: 1, + len: 2, + buf: [3, 4].to_vec(), + }, + ]; + assert!(i2c_map.transfer(&mut reqs).is_err()); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation - reqs.push(I2cReq { - addr: 0x3, - flags: 0, - len: 1, - buf: [34].to_vec(), - }); - reqs.push(I2cReq { - addr: 0x3, - flags: 1, - // Will cause failure - len: 3, - buf: [3, 4, 5].to_vec(), - }); - - assert_results(&mut i2c_map, &mut reqs, true, true); - }*/ + let mut reqs = vec![ + I2cReq { + addr: 0x3, + flags: 0, + len: 1, + buf: [34].to_vec(), + }, + I2cReq { + addr: 0x3, + flags: 1, + // Will cause failure + len: 3, + buf: [3, 4, 5].to_vec(), + }, + ]; + assert!(i2c_map.transfer(&mut reqs).is_err()); + } } diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index b76fabf..efa004a 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -162,7 +162,7 @@ fn main() -> Result<(), String> { #[cfg(test)] mod tests { use super::*; - use i2c::tests::I2cMockAdapter; + use i2c::tests::DummyDevice; fn get_cmd_args(name: &str, devices: &str, count: u32) -> ArgMatches { let yaml = load_yaml!("cli.yaml"); @@ -191,29 +191,28 @@ mod tests { ) -> bool { true } - /* + #[test] fn test_backend_single() { let cmd_args = get_cmd_args("vi2c.sock_single", "1:4,2:32:21,5:5:23", 0); - assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); + assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); } #[test] fn test_backend_multiple() { let cmd_args = get_cmd_args("vi2c.sock", "1:4,2:32:21,5:5:23", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); + assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); } #[test] fn test_backend_failure() { let cmd_args = get_cmd_args("vi2c.sock_failure", "1:4d", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); + assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); } #[test] fn test_backend_failure_duplicate_device4() { let cmd_args = get_cmd_args("vi2c.sock_duplicate", "1:4,2:32:21,5:4:23", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); + assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); } - */ } diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index 3ff538a..798f59f 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -305,12 +305,13 @@ impl VhostUserBackendMut #[cfg(test)] mod tests { use super::*; - use crate::i2c::tests::I2cMockAdapter; + use crate::i2c::tests::DummyDevice; + use std::convert::TryFrom; - /* #[test] fn verify_backend() { - let i2c_map: I2cMap = I2cMap::new("1:4,2:32:21,5:10:23").unwrap(); + let device_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); + let i2c_map: I2cMap = I2cMap::new(&device_config).unwrap(); let mut backend = VhostUserI2cBackend::new(Arc::new(i2c_map)).unwrap(); assert_eq!(backend.num_queues(), NUM_QUEUES); @@ -323,5 +324,5 @@ mod tests { backend.set_event_idx(true); assert!(backend.event_idx); - }*/ + } } From a6204b55d718fb9255862f306606cc837dadf97d Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 22 Sep 2021 20:27:12 +0300 Subject: [PATCH 10/15] fix dependency of vmm-sys-util to 0.8.0 Without the dependency fixed, the build is failing. Also, considering that this crate exports a binary, `Cargo.lock` must be committed to the repository. Added the `Cargo.lock` file. Signed-off-by: Andreea Florescu --- Cargo.lock | 371 +++++++++++++++++++++++++++++++++++++++++++++ src/i2c/Cargo.toml | 2 +- 2 files changed, 372 insertions(+), 1 deletion(-) create mode 100644 Cargo.lock diff --git a/Cargo.lock b/Cargo.lock new file mode 100644 index 0000000..9242376 --- /dev/null +++ b/Cargo.lock @@ -0,0 +1,371 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "arc-swap" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6df5aef5c5830360ce5218cecb8f018af3438af5686ae945094affc86fdec63" + +[[package]] +name = "atty" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9b39be18770d11421cdb1b9947a45dd3f37e93092cbf377614828a319d5fee8" +dependencies = [ + "hermit-abi", + "libc", + "winapi", +] + +[[package]] +name = "autocfg" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdb031dd78e28731d87d56cc8ffef4a8f36ca26c38fe2de700543e627f8a464a" + +[[package]] +name = "bitflags" +version = "1.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" + +[[package]] +name = "cfg-if" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" + +[[package]] +name = "clap" +version = "3.0.0-beta.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4bd1061998a501ee7d4b6d449020df3266ca3124b941ec56cf2005c3779ca142" +dependencies = [ + "atty", + "bitflags", + "clap_derive", + "indexmap", + "lazy_static", + "os_str_bytes", + "strsim", + "termcolor", + "textwrap", + "unicode-width", + "vec_map", + "yaml-rust", +] + +[[package]] +name = "clap_derive" +version = "3.0.0-beta.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b5bb0d655624a0b8770d1c178fb8ffcb1f91cc722cb08f451e3dc72465421ac" +dependencies = [ + "heck", + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "epoll" +version = "4.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "20df693c700404f7e19d4d6fae6b15215d2913c27955d2b9d6f2c0f537511cd0" +dependencies = [ + "bitflags", + "libc", +] + +[[package]] +name = "hashbrown" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ab5ef0d4909ef3724cc8cce6ccc8572c5c817592e9285f5464f8e86f8bd3726e" + +[[package]] +name = "heck" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d621efb26863f0e9924c6ac577e8275e5e6b77455db64ffa6c65c904e9e132c" +dependencies = [ + "unicode-segmentation", +] + +[[package]] +name = "hermit-abi" +version = "0.1.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "62b467343b94ba476dcb2500d242dadbb39557df889310ac77c5d99100aaac33" +dependencies = [ + "libc", +] + +[[package]] +name = "indexmap" +version = "1.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc633605454125dec4b66843673f01c7df2b89479b32e0ed634e43a91cff62a5" +dependencies = [ + "autocfg", + "hashbrown", +] + +[[package]] +name = "lazy_static" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" + +[[package]] +name = "libc" +version = "0.2.102" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2a5ac8f984bfcf3a823267e5fde638acc3325f6496633a5da6bb6eb2171e103" + +[[package]] +name = "linked-hash-map" +version = "0.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7fb9b38af92608140b86b693604b9ffcc5824240a484d1ecd4795bacb2fe88f3" + +[[package]] +name = "log" +version = "0.4.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "51b9bbe6c47d51fc3e1a9b945965946b4c44142ab8792c50835a980d362c2710" +dependencies = [ + "cfg-if", +] + +[[package]] +name = "os_str_bytes" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "afb2e1c3ee07430c2cf76151675e583e0f19985fa6efae47d6848a3e2c824f85" + +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2", + "quote", + "syn", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2", + "quote", + "version_check", +] + +[[package]] +name = "proc-macro2" +version = "1.0.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9f5105d4fdaab20335ca9565e106a5d9b82b6219b5ba735731124ac6711d23d" +dependencies = [ + "unicode-xid", +] + +[[package]] +name = "quote" +version = "1.0.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3d0b9745dc2debf507c8422de05d7226cc1f0644216dfdfead988f9b1ab32a7" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "strsim" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" + +[[package]] +name = "syn" +version = "1.0.76" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c6f107db402c2c2055242dbf4d2af0e69197202e9faacbef9571bbe47f5a1b84" +dependencies = [ + "proc-macro2", + "quote", + "unicode-xid", +] + +[[package]] +name = "termcolor" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4" +dependencies = [ + "winapi-util", +] + +[[package]] +name = "textwrap" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "203008d98caf094106cfaba70acfed15e18ed3ddb7d94e49baec153a2b462789" +dependencies = [ + "unicode-width", +] + +[[package]] +name = "unicode-segmentation" +version = "1.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8895849a949e7845e06bd6dc1aa51731a103c42707010a5b591c0038fb73385b" + +[[package]] +name = "unicode-width" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ed742d4ea2bd1176e236172c8429aaf54486e7ac098db29ffe6529e0ce50973" + +[[package]] +name = "unicode-xid" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" + +[[package]] +name = "vec_map" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f1bddf1187be692e79c5ffeab891132dfb0f236ed36a43c7ed39f1165ee20191" + +[[package]] +name = "version_check" +version = "0.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fecdca9a5291cc2b8dcf7dc02453fee791a280f3743cb0905f8822ae463b3fe" + +[[package]] +name = "vhost" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8a6b90237e10f1a61b35fba73885c3567e1a5a8c40d44daae335f7710210a7dc" +dependencies = [ + "bitflags", + "libc", + "vmm-sys-util", +] + +[[package]] +name = "vhost-device-i2c" +version = "0.1.0" +dependencies = [ + "clap", + "epoll", + "libc", + "log", + "vhost", + "vhost-user-backend", + "virtio-bindings", + "vm-memory", + "vmm-sys-util", +] + +[[package]] +name = "vhost-user-backend" +version = "0.1.0" +source = "git+https://github.com/rust-vmm/vhost-user-backend?rev=70f668a699d865f13ba40498897ad181a409bd41#70f668a699d865f13ba40498897ad181a409bd41" +dependencies = [ + "epoll", + "libc", + "log", + "vhost", + "virtio-bindings", + "virtio-queue", + "vm-memory", + "vmm-sys-util", +] + +[[package]] +name = "virtio-bindings" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ff512178285488516ed85f15b5d0113a7cdb89e9e8a760b269ae4f02b84bd6b" + +[[package]] +name = "virtio-queue" +version = "0.1.0" +source = "git+https://github.com/rust-vmm/vm-virtio?rev=6013dd9#6013dd91b2e6eb77ea10c6bdeda8f5eb18de6dda" +dependencies = [ + "log", + "vm-memory", + "vmm-sys-util", +] + +[[package]] +name = "vm-memory" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0a8ebcb86ca457f9d6e14cf97009f679952eba42f0113de5db596e514cd0e43b" +dependencies = [ + "arc-swap", + "libc", + "winapi", +] + +[[package]] +name = "vmm-sys-util" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "01cf11afbc4ebc0d5c7a7748a77d19e2042677fc15faa2f4ccccb27c18a60605" +dependencies = [ + "bitflags", + "libc", +] + +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + +[[package]] +name = "winapi-util" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70ec6ce85bb158151cae5e5c87f95a8e97d2c0c4b001223f33a334e3ce5de178" +dependencies = [ + "winapi", +] + +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + +[[package]] +name = "yaml-rust" +version = "0.4.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56c1936c4cc7a1c9ab21a1ebb602eb942ba868cbd44a99cb7cdc5892335e1c85" +dependencies = [ + "linked-hash-map", +] diff --git a/src/i2c/Cargo.toml b/src/i2c/Cargo.toml index 399440b..0d18cb6 100644 --- a/src/i2c/Cargo.toml +++ b/src/i2c/Cargo.toml @@ -20,4 +20,4 @@ vhost = { version = "0.1", features = ["vhost-user-slave"] } vhost-user-backend = { git = "https://github.com/rust-vmm/vhost-user-backend", rev = "70f668a699d865f13ba40498897ad181a409bd41" } virtio-bindings = ">=0.1" vm-memory = "0.6" -vmm-sys-util = ">=0.8.0" +vmm-sys-util = "=0.8.0" From aab3a25816317db6b077c8c4028688ec80a2b5fa Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 22 Sep 2021 22:21:25 +0300 Subject: [PATCH 11/15] [i2c] revert to a simplified main program Remove the dry run related functionality. The main function now just having boilerplate code that we should test via integration tests. We can test the parsing by validating the `TryFrom` implementation. Everything else is not possible to test with unit tests because they need I/O (such as working with sockets), and thus are more appropriate for integration tests. Added more tests for validating the expected behavior of parsing. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 14 +++++ src/i2c/src/main.rs | 137 ++++++++++++++++++++------------------------ 2 files changed, 77 insertions(+), 74 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 2190808..de0dbd6 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -432,6 +432,7 @@ pub struct I2cMap { device_map: [u32; MAX_I2C_VDEV], } +#[derive(Debug, PartialEq)] pub(crate) struct DeviceConfig { adapter_no: u32, addr: Vec, @@ -459,6 +460,7 @@ impl DeviceConfig { } } +#[derive(Debug, PartialEq)] pub(crate) struct AdapterConfig { inner: Vec, } @@ -548,6 +550,18 @@ pub mod tests { use super::*; use std::convert::TryFrom; + impl DeviceConfig { + pub fn new_with(adapter_no: u32, addr: Vec) -> Self { + DeviceConfig { adapter_no, addr } + } + } + + impl AdapterConfig { + pub fn new_with(devices: Vec) -> Self { + AdapterConfig { inner: devices } + } + } + #[derive(Debug, Default)] pub struct DummyDevice { funcs_result: i32, diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index efa004a..0f1079a 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -21,6 +21,7 @@ use crate::i2c::DeviceConfig; use i2c::{AdapterConfig, I2cDevice, I2cMap, PhysDevice}; use vhu_i2c::VhostUserI2cBackend; +#[derive(PartialEq, Debug)] struct I2cConfiguration { socket_path: String, socket_count: usize, @@ -77,58 +78,19 @@ impl TryFrom for I2cConfiguration { } } -fn start_daemon( - backend: Arc>>, - listener: Listener, -) -> bool { - let mut daemon = VhostUserDaemon::new( - String::from("vhost-device-i2c-backend"), - backend.clone(), - GuestMemoryAtomic::new(GuestMemoryMmap::new()), - ) - .unwrap(); - - daemon.start(listener).unwrap(); - - match daemon.wait() { - Ok(()) => { - println!("Stopping cleanly."); - } - Err(vhost_user_backend::Error::HandleRequest(vhost_user::Error::PartialMessage)) => { - println!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug."); - } - Err(e) => { - println!("Error running daemon: {:?}", e); - } - } - - // No matter the result, we need to shut down the worker thread. - backend - .read() - .unwrap() - .exit_event - .write(1) - .expect("Shutting down worker thread"); - - false -} - -fn start_backend( - cmd_args: ArgMatches, - start_daemon: fn(Arc>>, Listener) -> bool, +fn start_backend( + config: I2cConfiguration, ) -> Result<(), String> { - let mut handles = Vec::new(); - - let i2c_config = I2cConfiguration::try_from(cmd_args)?; - // The same i2c_map structure instance is shared between all the guests let i2c_map = Arc::new( - I2cMap::::new(&i2c_config.devices) + I2cMap::::new(&config.devices) .map_err(|e| format!("Failed to create i2c_map ({})", e))?, ); - for i in 0..i2c_config.socket_count { - let socket = i2c_config.socket_path.to_owned() + &i.to_string(); + let mut handles = Vec::new(); + + for i in 0..config.socket_count { + let socket = config.socket_path.to_owned() + &i.to_string(); let i2c_map = i2c_map.clone(); let handle = spawn(move || loop { @@ -137,9 +99,36 @@ fn start_backend( )); let listener = Listener::new(socket.clone(), true).unwrap(); - if start_daemon(backend, listener) { - break; + let mut daemon = VhostUserDaemon::new( + String::from("vhost-device-i2c-backend"), + backend.clone(), + GuestMemoryAtomic::new(GuestMemoryMmap::new()), + ) + .unwrap(); + + daemon.start(listener).unwrap(); + + match daemon.wait() { + Ok(()) => { + println!("Stopping cleanly."); + } + Err(vhost_user_backend::Error::HandleRequest( + vhost_user::Error::PartialMessage, + )) => { + println!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug."); + } + Err(e) => { + println!("Error running daemon: {:?}", e); + } } + + // No matter the result, we need to shut down the worker thread. + backend + .read() + .unwrap() + .exit_event + .write(1) + .expect("Shutting down worker thread"); }); handles.push(handle); @@ -156,13 +145,13 @@ fn main() -> Result<(), String> { let yaml = load_yaml!("cli.yaml"); let cmd_args = App::from(yaml).get_matches(); - start_backend::(cmd_args, start_daemon) + let config = I2cConfiguration::try_from(cmd_args).unwrap(); + start_backend::(config) } #[cfg(test)] mod tests { use super::*; - use i2c::tests::DummyDevice; fn get_cmd_args(name: &str, devices: &str, count: u32) -> ArgMatches { let yaml = load_yaml!("cli.yaml"); @@ -185,34 +174,34 @@ mod tests { } } - fn mock_start_daemon( - _backend: Arc>>, - _listener: Listener, - ) -> bool { - true - } - #[test] - fn test_backend_single() { - let cmd_args = get_cmd_args("vi2c.sock_single", "1:4,2:32:21,5:5:23", 0); - assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); - } - - #[test] - fn test_backend_multiple() { - let cmd_args = get_cmd_args("vi2c.sock", "1:4,2:32:21,5:5:23", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_ok()); - } - - #[test] - fn test_backend_failure() { + fn test_parse_failure() { let cmd_args = get_cmd_args("vi2c.sock_failure", "1:4d", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); + // TODO: Check against the actual error instead of `is_err`. + assert!(I2cConfiguration::try_from(cmd_args).is_err()); + + let cmd_args = get_cmd_args("vi2c.sock_duplicate", "1:4,2:32:21,5:4:23", 5); + // TODO: Check against the actual error instead of `is_err`. + assert!(I2cConfiguration::try_from(cmd_args).is_err()); } #[test] - fn test_backend_failure_duplicate_device4() { - let cmd_args = get_cmd_args("vi2c.sock_duplicate", "1:4,2:32:21,5:4:23", 5); - assert!(start_backend::(cmd_args, mock_start_daemon).is_err()); + fn test_parse_successful() { + let cmd_args = get_cmd_args("vi2c.sock_single", "1:4,2:32:21,5:5:23", 5); + let config = I2cConfiguration::try_from(cmd_args).unwrap(); + + let expected_devices = AdapterConfig::new_with(vec![ + DeviceConfig::new_with(1, vec![4]), + DeviceConfig::new_with(2, vec![32, 21]), + DeviceConfig::new_with(5, vec![5, 23]), + ]); + + let expected_config = I2cConfiguration { + socket_count: 5, + socket_path: String::from("vi2c.sock_single"), + devices: expected_devices, + }; + + assert_eq!(config, expected_config); } } From 32b9d1a2a21d6dd2a4d15c39a4b0c8307a7b8d83 Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 22 Sep 2021 22:40:20 +0300 Subject: [PATCH 12/15] [i2c] update coverage score Coverage is now increased by ~9%. Further tests should be relatively straightforward to write by using the mock structure `DummyDevice`. Signed-off-by: Andreea Florescu --- coverage_config_x86_64.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 0ed9680..0d49c11 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 40.6, + "coverage_score": 49.7, "exclude_path": "", "crate_features": "" } From c9e88b69abf5be6d7afbdb60b03e22f1dfbd8048 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 6 Oct 2021 12:12:35 +0530 Subject: [PATCH 13/15] [i2c] Remove read_func() It still doesn't look correct to keep a separate routine for this, which can be called by others. Merge it with new() itself. Signed-off-by: Viresh Kumar --- src/i2c/src/i2c.rs | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index de0dbd6..6622b68 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -299,37 +299,30 @@ impl I2cAdapter { // Creates a new adapter corresponding to the specified number. fn new(adapter_no: u32) -> Result> { let i2cdev = format!("/dev/i2c-{}", adapter_no); - let mut adapter = I2cAdapter { - adapter_no, - smbus: false, - device: D::open(i2cdev)?, - }; - adapter.read_func()?; - - Ok(adapter) - } - - // Helper function for reading the adaptor functionalities. - fn read_func(&mut self) -> Result<()> { let func: u64 = I2C_FUNC_SMBUS_ALL; + let mut device = D::open(i2cdev)?; + let smbus; - let ret = self.device.funcs(func); - + let ret = device.funcs(func); if ret == -1 { println!("Failed to get I2C function"); return errno_result(); } if (func & I2C_FUNC_I2C) != 0 { - self.smbus = false; + smbus = false; } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { - self.smbus = true; + smbus = true; } else { println!("Invalid functionality {:x}", func); return Err(Error::new(EINVAL)); } - Ok(()) + Ok(I2cAdapter { + device, + adapter_no, + smbus, + }) } /// Perform I2C_RDWR transfer From 88d62dd2ab33f191a6d601dccc652697da3c6769 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 6 Oct 2021 12:13:46 +0530 Subject: [PATCH 14/15] [i2c] Shuffle code around This moves the code around to keep structures together with their implementations, and also keep all configuration stuff to main.c itself. Signed-off-by: Viresh Kumar --- src/i2c/src/i2c.rs | 90 +++++------------------------------------- src/i2c/src/main.rs | 87 ++++++++++++++++++++++++++++++++++++---- src/i2c/src/vhu_i2c.rs | 5 ++- 3 files changed, 93 insertions(+), 89 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index 6622b68..47baa4c 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -11,6 +11,8 @@ use std::os::unix::io::AsRawFd; use libc::{c_ulong, ioctl, EINVAL}; use vmm_sys_util::errno::{errno_result, Error, Result}; +use super::AdapterConfig; + // The type of the `req` parameter is different for the `musl` library. This will enable // successful build for other non-musl libraries. #[cfg(target_env = "musl")] @@ -231,12 +233,6 @@ pub struct I2cReq { pub buf: Vec, } -pub struct I2cAdapter { - device: D, - adapter_no: u32, - smbus: bool, -} - /// Trait that represents an I2C Device. /// /// This trait is introduced for development purposes only, and should not @@ -295,6 +291,12 @@ impl I2cDevice for PhysDevice { } } +pub struct I2cAdapter { + device: D, + adapter_no: u32, + smbus: bool, +} + impl I2cAdapter { // Creates a new adapter corresponding to the specified number. fn new(adapter_no: u32) -> Result> { @@ -425,68 +427,6 @@ pub struct I2cMap { device_map: [u32; MAX_I2C_VDEV], } -#[derive(Debug, PartialEq)] -pub(crate) struct DeviceConfig { - adapter_no: u32, - addr: Vec, -} - -impl DeviceConfig { - pub fn new(adapter_no: u32) -> Self { - DeviceConfig { - adapter_no, - addr: Vec::new(), - } - } - - pub fn push(&mut self, addr: u16) -> std::result::Result<(), String> { - if addr as usize > MAX_I2C_VDEV { - return Err(format!("Invalid address: {} (> maximum allowed)", addr)); - } - - if self.addr.contains(&addr) { - return Err(format!("Address already in use: {}", addr)); - } - - self.addr.push(addr); - Ok(()) - } -} - -#[derive(Debug, PartialEq)] -pub(crate) struct AdapterConfig { - inner: Vec, -} - -impl AdapterConfig { - pub fn new() -> Self { - Self { inner: Vec::new() } - } - - fn contains_adapter_no(&self, adapter_no: u32) -> bool { - self.inner.iter().any(|elem| elem.adapter_no == adapter_no) - } - - fn contains_addr(&self, addr: u16) -> bool { - self.inner.iter().any(|elem| elem.addr.contains(&addr)) - } - - pub fn push(&mut self, device: DeviceConfig) -> std::result::Result<(), String> { - if self.contains_adapter_no(device.adapter_no) { - return Err("Duplicated adapter number".to_string()); - } - - for addr in device.addr.iter() { - if self.contains_addr(*addr) { - return Err(format!("Address already in use: {}", addr)); - } - } - - self.inner.push(device); - Ok(()) - } -} - impl I2cMap { pub(crate) fn new(device_config: &AdapterConfig) -> Result where @@ -529,7 +469,7 @@ impl I2cMap { return Err(Error::new(EINVAL)); } - // get the corresponding adapter based on teh device config. + // get the corresponding adapter based on the device config. let adapter = &self.adapters[index as usize]; // Set device's address @@ -543,18 +483,6 @@ pub mod tests { use super::*; use std::convert::TryFrom; - impl DeviceConfig { - pub fn new_with(adapter_no: u32, addr: Vec) -> Self { - DeviceConfig { adapter_no, addr } - } - } - - impl AdapterConfig { - pub fn new_with(devices: Vec) -> Self { - AdapterConfig { inner: devices } - } - } - #[derive(Debug, Default)] pub struct DummyDevice { funcs_result: i32, diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index 0f1079a..b5d1bf7 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -17,15 +17,69 @@ use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; -use crate::i2c::DeviceConfig; -use i2c::{AdapterConfig, I2cDevice, I2cMap, PhysDevice}; +use i2c::{I2cDevice, I2cMap, PhysDevice, MAX_I2C_VDEV}; use vhu_i2c::VhostUserI2cBackend; -#[derive(PartialEq, Debug)] -struct I2cConfiguration { - socket_path: String, - socket_count: usize, - devices: AdapterConfig, +#[derive(Debug, PartialEq)] +struct DeviceConfig { + adapter_no: u32, + addr: Vec, +} + +impl DeviceConfig { + fn new(adapter_no: u32) -> Self { + DeviceConfig { + adapter_no, + addr: Vec::new(), + } + } + + fn push(&mut self, addr: u16) -> std::result::Result<(), String> { + if addr as usize > MAX_I2C_VDEV { + return Err(format!("Invalid address: {} (> maximum allowed)", addr)); + } + + if self.addr.contains(&addr) { + return Err(format!("Address already in use: {}", addr)); + } + + self.addr.push(addr); + Ok(()) + } +} + +#[derive(Debug, PartialEq)] +pub(crate) struct AdapterConfig { + inner: Vec, +} + +impl AdapterConfig { + fn new() -> Self { + Self { inner: Vec::new() } + } + + fn contains_adapter_no(&self, adapter_no: u32) -> bool { + self.inner.iter().any(|elem| elem.adapter_no == adapter_no) + } + + fn contains_addr(&self, addr: u16) -> bool { + self.inner.iter().any(|elem| elem.addr.contains(&addr)) + } + + fn push(&mut self, device: DeviceConfig) -> std::result::Result<(), String> { + if self.contains_adapter_no(device.adapter_no) { + return Err("Duplicated adapter number".to_string()); + } + + for addr in device.addr.iter() { + if self.contains_addr(*addr) { + return Err(format!("Address already in use: {}", addr)); + } + } + + self.inner.push(device); + Ok(()) + } } impl TryFrom<&str> for AdapterConfig { @@ -53,6 +107,13 @@ impl TryFrom<&str> for AdapterConfig { } } +#[derive(PartialEq, Debug)] +struct I2cConfiguration { + socket_path: String, + socket_count: usize, + devices: AdapterConfig, +} + impl TryFrom for I2cConfiguration { type Error = String; @@ -153,6 +214,18 @@ fn main() -> Result<(), String> { mod tests { use super::*; + impl DeviceConfig { + pub fn new_with(adapter_no: u32, addr: Vec) -> Self { + DeviceConfig { adapter_no, addr } + } + } + + impl AdapterConfig { + pub fn new_with(devices: Vec) -> Self { + AdapterConfig { inner: devices } + } + } + fn get_cmd_args(name: &str, devices: &str, count: u32) -> ArgMatches { let yaml = load_yaml!("cli.yaml"); let app = App::from(yaml); diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index 798f59f..14e8540 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -5,10 +5,10 @@ // // SPDX-License-Identifier: Apache-2.0 -use crate::i2c::*; use std::mem::size_of; use std::sync::Arc; use std::{convert, error, fmt, io}; + use vhost::vhost_user::message::{VhostUserProtocolFeatures, VhostUserVirtioFeatures}; use vhost_user_backend::{VhostUserBackendMut, VringRwLock, VringT}; use virtio_bindings::bindings::virtio_net::{VIRTIO_F_NOTIFY_ON_EMPTY, VIRTIO_F_VERSION_1}; @@ -18,6 +18,8 @@ use virtio_bindings::bindings::virtio_ring::{ use vm_memory::{ByteValued, Bytes, GuestMemoryAtomic, GuestMemoryMmap, Le16, Le32}; use vmm_sys_util::eventfd::{EventFd, EFD_NONBLOCK}; +use crate::i2c::*; + const QUEUE_SIZE: usize = 1024; const NUM_QUEUES: usize = 1; @@ -306,6 +308,7 @@ impl VhostUserBackendMut mod tests { use super::*; use crate::i2c::tests::DummyDevice; + use crate::AdapterConfig; use std::convert::TryFrom; #[test] From bf59120d50a56493ef94f639dd74c0f978cf4c98 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 6 Oct 2021 12:54:56 +0530 Subject: [PATCH 15/15] [i2c] update coverage score Signed-off-by: Viresh Kumar --- coverage_config_x86_64.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 0d49c11..d8b86b0 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 49.7, + "coverage_score": 49.4, "exclude_path": "", "crate_features": "" }