From fb2c10b9b158b3ab94fed0a59cbd07f4757b1e37 Mon Sep 17 00:00:00 2001 From: "Jes B. Klinke" Date: Sat, 10 Feb 2024 22:25:32 -0800 Subject: [PATCH] [opentitantool] Refactorings Prepare for adding support for GSC ready signal by refactoring some functions, enabling the next CL to more clearly show the functionality added. The only functional change in this CL is setting a timeout of 500 miliseconds, rather than 10 retries. The latter depends on debugger latency in an unpredictable way. Change-Id: I40c0d33c78dfd491842b4e3867a10d8a78ce45f5 Signed-off-by: Jes B. Klinke --- sw/host/opentitanlib/src/tpm/driver.rs | 69 +++++++++++++++-------- sw/host/opentitantool/src/command/i2c.rs | 7 ++- sw/host/opentitantool/src/command/spi.rs | 6 +- sw/host/tpm2_test_server/src/interface.rs | 19 ++++--- sw/host/tpm2_test_server/src/main.rs | 22 +++++--- 5 files changed, 76 insertions(+), 47 deletions(-) diff --git a/sw/host/opentitanlib/src/tpm/driver.rs b/sw/host/opentitanlib/src/tpm/driver.rs index a1601c840a88d..13745b13c3512 100644 --- a/sw/host/opentitanlib/src/tpm/driver.rs +++ b/sw/host/opentitanlib/src/tpm/driver.rs @@ -193,8 +193,10 @@ pub struct SpiDriver { } impl SpiDriver { - pub fn new(spi: Rc) -> Self { - Self { spi } + pub fn new( + spi: Rc, + ) -> Result { + Ok(Self { spi }) } /// Numerical TPM register address as used in SPI protocol. @@ -232,21 +234,39 @@ impl SpiDriver { let req = self.compose_header(register, len, is_read); self.spi .run_transaction(&mut [spi::Transfer::Both(&req, &mut buffer)])?; + // The TPM has a chance to indicate that it is ready to produce the response in the very + // next byte. As the fourth and final byte of the header is being sent, if the TPM sends + // back 0x01 on the other data line, data will come next. if buffer[3] & 1 == 0 { - let mut retries = 10; + // The TPM was not immediately ready, keep polling, until we receive a byte of 0x01. + let start_time = Instant::now(); while { self.spi .run_transaction(&mut [spi::Transfer::Read(&mut buffer[0..1])])?; buffer[0] & 1 == 0 } { - retries -= 1; - if retries == 0 { + if Instant::now().duration_since(start_time) > TIMEOUT { bail!(TpmError::Timeout) } } } Ok(()) } + + fn do_read_register(&self, register: Register, data: &mut [u8]) -> Result<()> { + let _cs_asserted = Rc::clone(&self.spi).assert_cs()?; // Deasserts when going out of scope. + self.write_header(register, data.len(), true)?; + self.spi.run_transaction(&mut [spi::Transfer::Read(data)])?; + Ok(()) + } + + fn do_write_register(&self, register: Register, data: &[u8]) -> Result<()> { + let _cs_asserted = Rc::clone(&self.spi).assert_cs()?; // Deasserts when going out of scope. + self.write_header(register, data.len(), false)?; + self.spi + .run_transaction(&mut [spi::Transfer::Write(data)])?; + Ok(()) + } } const SPI_TPM_READ: u32 = 0xC0000000; @@ -257,6 +277,7 @@ const SPI_TPM_ADDRESS_OFFSET: u32 = 0x00D40000; const MAX_TRANSACTION_SIZE: usize = 32; const RESPONSE_HEADER_SIZE: usize = 6; const MAX_RESPONSE_SIZE: usize = 4096; +const TIMEOUT: Duration = Duration::from_millis(500); impl Driver for SpiDriver { fn read_register(&self, register: Register, data: &mut [u8]) -> Result<()> { @@ -273,10 +294,8 @@ impl Driver for SpiDriver { data.clone_from_slice(&buffer[1..]); return Ok(()); } - let _cs_asserted = Rc::clone(&self.spi).assert_cs()?; // Deasserts when going out of scope. - self.write_header(register, data.len(), true)?; - self.spi.run_transaction(&mut [spi::Transfer::Read(data)])?; - Ok(()) + let result = self.do_read_register(register, data); + result } fn write_register(&self, register: Register, data: &[u8]) -> Result<()> { @@ -295,11 +314,8 @@ impl Driver for SpiDriver { ensure!(buffer[0] & 1 != 0, "TPM did not respond as expected",); return Ok(()); } - let _cs_asserted = Rc::clone(&self.spi).assert_cs()?; // Deasserts when going out of scope. - self.write_header(register, data.len(), false)?; - self.spi - .run_transaction(&mut [spi::Transfer::Write(data)])?; - Ok(()) + let result = self.do_write_register(register, data); + result } } @@ -309,8 +325,10 @@ pub struct I2cDriver { } impl I2cDriver { - pub fn new(i2c: Rc) -> Self { - Self { i2c } + pub fn new( + i2c: Rc, + ) -> Result { + Ok(Self { i2c }) } /// Numerical TPM register address as used in Google I2C protocol. @@ -323,6 +341,16 @@ impl I2cDriver { _ => None, } } + + fn try_read_register(&self, register: Register, data: &mut [u8]) -> Result<()> { + self.i2c.run_transaction( + None, /* default addr */ + &mut [ + i2c::Transfer::Write(&[Self::addr(register).unwrap()]), + i2c::Transfer::Read(data), + ], + ) + } } impl Driver for I2cDriver { @@ -332,14 +360,7 @@ impl Driver for I2cDriver { // Retry in case the I2C bus wasn't ready. let res = loop { count += 1; - let res = self.i2c.run_transaction( - None, /* default addr */ - &mut [ - i2c::Transfer::Write(&[Self::addr(register).unwrap()]), - i2c::Transfer::Read(data), - ], - ); - match res { + match self.try_read_register(register, data) { Err(e) => { log::trace!( "Register 0x{:X} access error: {}", diff --git a/sw/host/opentitantool/src/command/i2c.rs b/sw/host/opentitantool/src/command/i2c.rs index 87e28cc842b37..675b9022470d5 100644 --- a/sw/host/opentitantool/src/command/i2c.rs +++ b/sw/host/opentitantool/src/command/i2c.rs @@ -270,9 +270,10 @@ impl CommandDispatch for I2cTpm { transport: &TransportWrapper, ) -> Result>> { let context = context.downcast_ref::().unwrap(); - let tpm_driver = tpm::I2cDriver::new(context.params.create(transport, "TPM")?); - let bus: Box = Box::new(tpm_driver); - self.command.run(&bus, transport) + let tpm_driver = Box::new(tpm::I2cDriver::new( + context.params.create(transport, "TPM")?, + )?); + self.command.run(&tpm_driver, transport) } } diff --git a/sw/host/opentitantool/src/command/spi.rs b/sw/host/opentitantool/src/command/spi.rs index c904bee2be212..644b4163ec8eb 100644 --- a/sw/host/opentitantool/src/command/spi.rs +++ b/sw/host/opentitantool/src/command/spi.rs @@ -319,10 +319,10 @@ impl CommandDispatch for SpiTpm { transport: &TransportWrapper, ) -> Result>> { let context = context.downcast_ref::().unwrap(); - let bus: Box = Box::new(tpm::SpiDriver::new( + let tpm_driver: Box = Box::new(tpm::SpiDriver::new( context.params.create(transport, "TPM")?, - )); - self.command.run(&bus, transport) + )?); + self.command.run(&tpm_driver, transport) } } diff --git a/sw/host/tpm2_test_server/src/interface.rs b/sw/host/tpm2_test_server/src/interface.rs index 0348ef060fefd..09f238a6d0cb7 100644 --- a/sw/host/tpm2_test_server/src/interface.rs +++ b/sw/host/tpm2_test_server/src/interface.rs @@ -122,14 +122,17 @@ fn handle_send(stream: &mut TcpStream, tpm: &dyn Driver) -> Result<()> { } log::debug!("TPM cmd {:02x?}", cmd); - if let Ok(res) = tpm.execute_command(&cmd) { - stream.write_all(&(res.len() as u32).to_be_bytes())?; - stream.write_all(&res)?; - stream.write_all(&[0u8; 4])?; - } else { - log::error!("Command fail."); - stream.write_all(&[0u8; 4])?; - stream.write_all(&[0u8; 4])?; + match tpm.execute_command(&cmd) { + Ok(res) => { + stream.write_all(&(res.len() as u32).to_be_bytes())?; + stream.write_all(&res)?; + stream.write_all(&[0u8; 4])?; + } + Err(e) => { + log::error!("Command fail: {}", e); + stream.write_all(&[0u8; 4])?; + stream.write_all(&[0u8; 4])?; + } } Ok(()) diff --git a/sw/host/tpm2_test_server/src/main.rs b/sw/host/tpm2_test_server/src/main.rs index 0e431ca71333f..0dc6045fecdb2 100644 --- a/sw/host/tpm2_test_server/src/main.rs +++ b/sw/host/tpm2_test_server/src/main.rs @@ -49,7 +49,7 @@ struct Opts { const CMD_TOKEN: Token = Token(0); const PLATFORM_TOKEN: Token = Token(1); -pub fn main() -> std::io::Result<()> { +pub fn main() -> anyhow::Result<()> { let options = Opts::parse(); env_logger::Builder::from_default_env() .filter(None, options.logging) @@ -72,18 +72,22 @@ pub fn main() -> std::io::Result<()> { poll.registry() .register(&mut cmd_stream, CMD_TOKEN, Interest::READABLE)?; - let transport = backend::create(&options.backend_opts).unwrap(); + let transport = backend::create(&options.backend_opts)?; let bus: Box = match options.bus { TpmBus::Spi { params } => { - let spi = params.create(&transport, "TPM").unwrap(); - Box::new(SpiDriver::new(spi)) + let spi = params.create(&transport, "TPM")?; + Box::new(SpiDriver::new( + spi, + )?) } TpmBus::I2C { params } => { - let i2c = params.create(&transport, "TPM").unwrap(); - Box::new(I2cDriver::new(i2c)) + let i2c = params.create(&transport, "TPM")?; + Box::new(I2cDriver::new( + i2c, + )?) } }; - bus.init().unwrap(); + bus.init()?; loop { poll.poll(&mut events, None)?; @@ -91,12 +95,12 @@ pub fn main() -> std::io::Result<()> { for event in events.iter() { match event.token() { CMD_TOKEN => { - if serve_command(&mut cmd_stream, &*bus).unwrap() { + if serve_command(&mut cmd_stream, &*bus)? { return Ok(()); } } PLATFORM_TOKEN => { - if serve_command(&mut platform_stream, &*bus).unwrap() { + if serve_command(&mut platform_stream, &*bus)? { return Ok(()); } }