[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 14, 2024 · fb2c10b · fb2c10b
1 parent a1cb204
commit fb2c10b
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 47 deletions.
diff --git 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<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.
@@ -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<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 +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
@@ -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(());
                     }
                 }