[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 <[email protected]>
lowRISC · Feb 12, 2024 · 9e7d275 · 9e7d275
1 parent 2d0e611
commit 9e7d275
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 48 deletions.
diff --git a/sw/host/opentitanlib/src/tpm/driver.rs b/sw/host/opentitanlib/src/tpm/driver.rs
@@ -5,9 +5,10 @@
 use anyhow::{bail, ensure, Result};
 use clap::ValueEnum;
 use serde::{Deserialize, Serialize};
+use std::cmp::Ordering;
 use std::rc::Rc;
 use std::thread;
-use std::time::{Duration, Instant};
+use std::time::{Duration, Instant, SystemTime};
 use thiserror::Error;
 
 use crate::io::i2c;
@@ -193,8 +194,10 @@ pub struct SpiDriver {
 }
 
 impl SpiDriver {
-    pub fn new(spi: Rc<dyn spi::Target>) -> Self {
-        Self { spi }
+    pub fn new(
+        spi: Rc<dyn spi::Target>,
+    ) -> Result<Self> {
+        Ok(Self { spi })
     }
 
     /// Numerical TPM register address as used in SPI protocol.
@@ -233,20 +236,35 @@ impl SpiDriver {
         self.spi
             .run_transaction(&mut [spi::Transfer::Both(&req, &mut buffer)])?;
         if buffer[3] & 1 == 0 {
-            let mut retries = 10;
+            let start_time = SystemTime::now();
             while {
                 self.spi
                     .run_transaction(&mut [spi::Transfer::Read(&mut buffer[0..1])])?;
                 buffer[0] & 1 == 0
             } {
-                retries -= 1;
-                if retries == 0 {
+                if SystemTime::now().duration_since(start_time)?.cmp(&TIMEOUT) == Ordering::Greater
+                {
                     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 +275,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 +292,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 +312,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 +323,10 @@ pub struct I2cDriver {
 }
 
 impl I2cDriver {
-    pub fn new(i2c: Rc<dyn i2c::Bus>) -> Self {
-        Self { i2c }
+    pub fn new(
+        i2c: Rc<dyn i2c::Bus>,
+    ) -> Result<Self> {
+        Ok(Self { i2c })
     }
 
     /// Numerical TPM register address as used in Google I2C protocol.
@@ -323,6 +339,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 +358,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
@@ -270,9 +270,10 @@ impl CommandDispatch for I2cTpm {
         transport: &TransportWrapper,
     ) -> Result<Option<Box<dyn Annotate>>> {
         let context = context.downcast_ref::<I2cCommand>().unwrap();
-        let tpm_driver = tpm::I2cDriver::new(context.params.create(transport, "TPM")?);
-        let bus: Box<dyn tpm::Driver> = 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
@@ -319,10 +319,10 @@ impl CommandDispatch for SpiTpm {
         transport: &TransportWrapper,
     ) -> Result<Option<Box<dyn Annotate>>> {
         let context = context.downcast_ref::<SpiCommand>().unwrap();
-        let bus: Box<dyn tpm::Driver> = Box::new(tpm::SpiDriver::new(
+        let tpm_driver: Box<dyn tpm::Driver> = 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
@@ -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
@@ -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,31 +72,35 @@ 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<dyn Driver> = 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)?;
 
         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(());
                     }
                 }