From 1290004bc3ec4b5cf4b091b55e94ad199c68efe2 Mon Sep 17 00:00:00 2001 From: "Jes B. Klinke" Date: Thu, 1 Aug 2024 01:11:23 -0700 Subject: [PATCH] [opentitantool] Avoid repeated opening/closing Introduce means for users of opentitanlib to declare that for a period, no other users will be simultaneously accessing the same debugger device, allowing the driver to keep USB handles open across multiple operations. The primary motivation for this is an observation that the repeated opening and closing of the `/dev/ttyUSBn` HyperDebug console in conjuction with other activity on the USB bus may trigger what could be a bug in the Linux USB driver stack. Even absent concerns for such a bug, this change reduces the time taken by `transport init` from 0.4s to 0.1s for a set of Google configuration files, which could become significant, when running many small test cases in a row. In this first iteration, `opentitansession` is made to claim exclusive use of the USB backend all the time, and `opentitantool` is meade to claim exclusive use any time, except when a sub-command explicitly declares that it does not want exclusive use. Only the `console` command is declared in that way, as that is the only one that stays running for an unbounded period, and is often left running while opentitantool is separately invoked to manipulate GPIO or other things. Signed-off-by: Jes B. Klinke Change-Id: I1c3625d6efa87a531ae20f5f1cdb576e1096bde9 (cherry picked from commit 3e4b9738f8804f1955a2a5372a91c8a14a1d949d) --- .../opentitantool_derive/src/lib.rs | 35 +++++++++ sw/host/opentitanlib/src/app/command.rs | 10 +++ sw/host/opentitanlib/src/app/mod.rs | 21 +++++- .../src/transport/hyperdebug/mod.rs | 75 ++++++++++++++++--- sw/host/opentitanlib/src/transport/mod.rs | 17 +++++ sw/host/opentitansession/src/main.rs | 12 +++ sw/host/opentitantool/src/command/console.rs | 8 ++ sw/host/opentitantool/src/main.rs | 26 ++++++- 8 files changed, 190 insertions(+), 14 deletions(-) diff --git a/sw/host/opentitanlib/opentitantool_derive/src/lib.rs b/sw/host/opentitanlib/opentitantool_derive/src/lib.rs index b84a7aa00b164..33a62dbac733d 100644 --- a/sw/host/opentitanlib/opentitantool_derive/src/lib.rs +++ b/sw/host/opentitanlib/opentitantool_derive/src/lib.rs @@ -64,6 +64,12 @@ fn dispatch_enum(name: &Ident, e: &DataEnum) -> Result { .map(|variant| dispatch_variant(name, variant)) .collect::>>()?; + let exclusive_arms = e + .variants + .iter() + .map(|variant| exclusive_variant(name, variant)) + .collect::>>()?; + // We wrap the derived code inside an anonymous const block to give the // `extern crate` references a local namespace that wont pollute the // global namespace. @@ -83,6 +89,12 @@ fn dispatch_enum(name: &Ident, e: &DataEnum) -> Result { #(#arms),* } } + + fn exclusive_use_of_transport(&self) -> bool { + match self { + #(#exclusive_arms),* + } + } } }; }) @@ -110,3 +122,26 @@ fn dispatch_variant(name: &Ident, variant: &Variant) -> Result { opentitanlib::app::command::CommandDispatch::run(__field, context, backend) }) } + +fn exclusive_variant(name: &Ident, variant: &Variant) -> Result { + let ident = &variant.ident; + let unnamed_len = match &variant.fields { + Fields::Unnamed(u) => u.unnamed.len(), + _ => { + return Err(Error::new( + variant.ident.span(), + "CommandDispatch is only implemented for Newtype variants", + )); + } + }; + if unnamed_len != 1 { + return Err(Error::new( + variant.ident.span(), + "CommandDispatch is only implemented for Newtype variants", + )); + } + Ok(quote! { + #name::#ident(ref __field) => + opentitanlib::app::command::CommandDispatch::exclusive_use_of_transport(__field) + }) +} diff --git a/sw/host/opentitanlib/src/app/command.rs b/sw/host/opentitanlib/src/app/command.rs index 06e5ec9a3a2de..cb7ad5feb5145 100644 --- a/sw/host/opentitanlib/src/app/command.rs +++ b/sw/host/opentitanlib/src/app/command.rs @@ -22,4 +22,14 @@ pub trait CommandDispatch { context: &dyn Any, transport: &TransportWrapper, ) -> Result>>; + + /// For optimization. Indicates whether this command expects to not run concurrently with + /// other manipulations of the backend debugger. Only long-running commands such as `console` + /// will return `false` to indicate that to the contrary they expect other invocations of + /// `opentitantool` to run during their lifespan. Returning `true` here will allow + /// opentitanlib the optimization of keeping USB handles open for the duration of the `run()` + /// call, and even across `run()` of multiple commands if `--exec` is used. + fn exclusive_use_of_transport(&self) -> bool { + true + } } diff --git a/sw/host/opentitanlib/src/app/mod.rs b/sw/host/opentitanlib/src/app/mod.rs index 50b96e30902bb..78f380384b409 100644 --- a/sw/host/opentitanlib/src/app/mod.rs +++ b/sw/host/opentitanlib/src/app/mod.rs @@ -20,8 +20,8 @@ use crate::io::nonblocking_help::NonblockingHelp; use crate::io::spi::{Target, TransferMode}; use crate::io::uart::Uart; use crate::transport::{ - ioexpander, Capability, ProgressIndicator, ProxyOps, Transport, TransportError, - TransportInterfaceType, + ioexpander, Capability, MaintainConnection, ProgressIndicator, ProxyOps, Transport, + TransportError, TransportInterfaceType, }; use anyhow::{bail, ensure, Result}; @@ -937,6 +937,11 @@ impl TransportWrapper { /// Configure all pins as input/output, pullup, etc. as declared in configuration files. /// Also configure SPI port mode/speed, and other similar settings. pub fn apply_default_configuration(&self, strapping_name: Option<&str>) -> Result<()> { + // Telling the transport that this function is the exclusive user of the debugger device + // for the duration of this function, will allow the transport to keep USB handles open, + // for optimization. (For transports such as the proxy, which does not have any + // such optimization, this is a no-op.) + let _maintain_connection = self.transport.maintain_connection()?; if let Some(strapping_name) = strapping_name { if self.capabilities()?.request(Capability::PROXY).ok().is_ok() { self.proxy_ops()? @@ -969,6 +974,11 @@ impl TransportWrapper { } pub fn reset_target(&self, reset_delay: Duration, clear_uart_rx: bool) -> Result<()> { + // Telling the transport that this function is the exclusive user of the debugger device + // for the duration of this function, will allow the transport to keep USB handles open, + // for optimization. (For transports such as the proxy, which does not have any + // such optimization, this is a no-op.) + let _maintain_connection = self.transport.maintain_connection()?; log::info!("Asserting the reset signal"); if self.disable_dft_on_reset.get() { self.pin_strapping("PRERESET_DFT_DISABLE")?.apply()?; @@ -990,6 +1000,13 @@ impl TransportWrapper { std::thread::sleep(reset_delay); Ok(()) } + + /// As long as the returned `MaintainConnection` object is kept by the caller, this driver may + /// assume that no other `opentitantool` processes attempt to access the same debugger device. + /// This allows for optimzations such as keeping USB handles open across function invocations. + pub fn maintain_connection(&self) -> Result> { + self.transport.maintain_connection() + } } /// Given an pin/uart/spi/i2c port name, if the name is a known alias, return the underlying diff --git a/sw/host/opentitanlib/src/transport/hyperdebug/mod.rs b/sw/host/opentitanlib/src/transport/hyperdebug/mod.rs index b9a349b2ec7f3..0a455f6ea95da 100644 --- a/sw/host/opentitanlib/src/transport/hyperdebug/mod.rs +++ b/sw/host/opentitanlib/src/transport/hyperdebug/mod.rs @@ -17,7 +17,8 @@ use std::io::Read; use std::io::Write; use std::marker::PhantomData; use std::path::{Path, PathBuf}; -use std::rc::Rc; +use std::rc::{Rc, Weak}; +use std::time::Duration; use crate::debug::openocd::OpenOcdJtagChain; use crate::io::gpio::{GpioBitbanging, GpioMonitoring, GpioPin}; @@ -29,6 +30,7 @@ use crate::transport::chip_whisperer::board::Board; use crate::transport::chip_whisperer::ChipWhisperer; use crate::transport::common::fpga::{ClearBitstream, FpgaProgram}; use crate::transport::common::uart::flock_serial; +use crate::transport::MaintainConnection; use crate::transport::{ Capabilities, Capability, SetJtagPins, Transport, TransportError, TransportInterfaceType, UpdateFirmware, @@ -215,7 +217,7 @@ impl Hyperdebug { if !device.kernel_driver_active(interface.number())? { device.attach_kernel_driver(interface.number())?; // Wait for udev rules to apply proper permissions to new device. - std::thread::sleep(std::time::Duration::from_millis(100)); + std::thread::sleep(Duration::from_millis(100)); } if interface_name.ends_with("Shell") { @@ -300,6 +302,7 @@ impl Hyperdebug { console_tty: console_tty.ok_or_else(|| { TransportError::CommunicationError("Missing console interface".to_string()) })?, + conn: RefCell::new(Weak::new()), usb_device: RefCell::new(device), selected_spi: Cell::new(0), }), @@ -421,6 +424,7 @@ impl Hyperdebug { /// even if the caller lets the outer Hyperdebug struct run out of scope. pub struct Inner { console_tty: PathBuf, + conn: RefCell>, usb_device: RefCell, selected_spi: Cell, } @@ -436,7 +440,44 @@ pub struct CachedIo { uarts: RefCell>>, } +pub struct Conn { + console_port: RefCell, +} + +// The way that the HyperDebug allows callers to request optimization for a sequence of operations +// without other `opentitantool` processes meddling with the USB devices, is to let the caller +// hold an `Rc`-reference to the `Conn` struct, thereby keeping the USB connection alive. +impl MaintainConnection for Conn {} + impl Inner { + /// Establish connection with HyperDebug console USB interface. + pub fn connect(&self) -> Result> { + if let Some(conn) = self.conn.borrow().upgrade() { + // The driver already has a connection, use it. + return Ok(conn); + } + // Establish a new connection. + let port_name = self + .console_tty + .to_str() + .ok_or(TransportError::UnicodePathError)?; + let port = + TTYPort::open(&serialport::new(port_name, 115_200).timeout(Duration::from_millis(100))) + .context("Failed to open HyperDebug console")?; + flock_serial(&port, port_name)?; + let conn = Rc::new(Conn { + console_port: RefCell::new(port), + }); + // Return a (strong) reference to the newly opened connection, while keeping a weak + // reference to the same in this `Inner` object. The result is that if the caller keeps + // the strong reference alive long enough, the next invocation of `connect()` will be able + // to re-use the same instance. If on the other hand, the caller drops their reference, + // then the weak reference will not keep the instance alive, and next time a new + // connection will be made. + *self.conn.borrow_mut() = Rc::downgrade(&conn); + Ok(conn) + } + /// Send a command to HyperDebug firmware, expecting to receive no output. Any output will be /// reported through an `Err()` return. pub fn cmd_no_output(&self, cmd: &str) -> Result<()> { @@ -505,17 +546,19 @@ impl Inner { Ok(captures) } + /// Send a command to HyperDebug firmware, with a callback to receive any output. + fn execute_command(&self, cmd: &str, callback: impl FnMut(&str)) -> Result<()> { + // Open console device, if not already open. + let conn = self.connect()?; + // Perform requested command, passing any output to callback. + conn.execute_command(cmd, callback) + } +} + +impl Conn { /// Send a command to HyperDebug firmware, with a callback to receive any output. fn execute_command(&self, cmd: &str, mut callback: impl FnMut(&str)) -> Result<()> { - let port_name = self - .console_tty - .to_str() - .ok_or(TransportError::UnicodePathError)?; - let mut port = TTYPort::open( - &serialport::new(port_name, 115_200).timeout(std::time::Duration::from_millis(100)), - ) - .context("Failed to open HyperDebug console")?; - flock_serial(&port, port_name)?; + let port: &mut TTYPort = &mut self.console_port.borrow_mut(); // Send Ctrl-C, followed by the command, then newline. This will discard any previous // partial input, before executing our command. @@ -831,6 +874,16 @@ impl Transport for Hyperdebug { )?); Ok(new_jtag) } + + /// The way that the HyperDebug driver allows callers to request optimization for a sequence + /// of operations without other `opentitantool` processes meddling with the USB devices, is to + /// let the caller hold an `Rc`-reference to the `Conn` struct, thereby keeping the USB + /// connection alive. Callers should only hold ond to the object as long as they can + /// guarantee that no other `opentitantool` processes simultaneously attempt to access the + /// same HyperDebug USB device. + fn maintain_connection(&self) -> Result> { + Ok(self.inner.connect()?) + } } /// A `StandardFlavor` is a plain Hyperdebug board. diff --git a/sw/host/opentitanlib/src/transport/mod.rs b/sw/host/opentitanlib/src/transport/mod.rs index 52d2ed7f55822..db0162ae8b4b9 100644 --- a/sw/host/opentitanlib/src/transport/mod.rs +++ b/sw/host/opentitanlib/src/transport/mod.rs @@ -156,6 +156,14 @@ pub trait Transport { Err(TransportError::UnsupportedOperation.into()) } + /// As long as the returned `MaintainConnection` object is kept by the caller, this driver may + /// assume that no other `opentitantool` processes attempt to access the same debugger device. + /// This allows for optimzations such as keeping USB handles open across function invocations. + fn maintain_connection(&self) -> Result> { + // For implementations that have not implemented any optimizations, return a no-op object. + Ok(Rc::new(())) + } + /// Before nonblocking operations can be used on `Uart` or other traits, this /// `NonblockingHelp` object must be invoked, in order to get the `Transport` implementation a /// chance to register its internal event sources with the main event loop. @@ -164,6 +172,15 @@ pub trait Transport { } } +/// As long as this object is kept alive, the `Transport` driver may assume that no other +/// `opentitantool` processes attempt to access the same debugger device. This allows for +/// optimzations such as keeping USB handles open across function invocations. +pub trait MaintainConnection {} + +/// No-op implmentation of the trait, for use by `Transport` implementations that do not do +/// any optimizations to maintain connection between method calls. +impl MaintainConnection for () {} + /// Methods available only on the Proxy implementation of the Transport trait. pub trait ProxyOps { /// Returns a string->string map containing user-defined aspects "provided" by the testbed diff --git a/sw/host/opentitansession/src/main.rs b/sw/host/opentitansession/src/main.rs index 79a5cf6d44809..444656e9dad26 100644 --- a/sw/host/opentitansession/src/main.rs +++ b/sw/host/opentitansession/src/main.rs @@ -146,8 +146,20 @@ fn start_session(run_file_fn: impl FnOnce(u16) -> PathBuf) -> Result, backend_opts: &backend::BackendOpts) -> Result<()> { + // Open connection to transport backend (HyperDebug or other debugger device) based on + // command line arguments. let transport = backend::create(backend_opts)?; + + // We do not need other invocations of `opentitantool` to directly access the debugger device + // while this session process runs (as any such invocations ought to instead establish TCP/IP + // connection and go through this session.) Hence, we can inform the driver that it is free + // to e.g. hold on to open USB handles between function calls, or perform other similar + // optimizations. + let _maintain_connection = transport.maintain_connection()?; + + // Bind to TCP socket, in preparation for servicing requests from network. let mut session = SessionHandler::init(&transport, listen_port)?; + // Instantiation of Transport backend, and binding to a socket was successful, now go // through the process of making this process a daemon, disconnected from the // terminal that was used to start it. diff --git a/sw/host/opentitantool/src/command/console.rs b/sw/host/opentitantool/src/command/console.rs index 02548003cd36e..50b21d266473f 100644 --- a/sw/host/opentitantool/src/command/console.rs +++ b/sw/host/opentitantool/src/command/console.rs @@ -135,4 +135,12 @@ impl CommandDispatch for Console { } } } + + /// For optiimzation. Indicates that this command expects other invocations of + /// `opentitantool` to run during the lifespan of the `run()` function above. Returning + /// `false` here will prevent opentitanlib from keeping USB handles open for the duration of + /// the `run()` call. + fn exclusive_use_of_transport(&self) -> bool { + false + } } diff --git a/sw/host/opentitantool/src/main.rs b/sw/host/opentitantool/src/main.rs index 7588ec3256ceb..65350f4c3b073 100644 --- a/sw/host/opentitantool/src/main.rs +++ b/sw/host/opentitantool/src/main.rs @@ -15,12 +15,14 @@ use std::io::ErrorKind; use std::io::IsTerminal; use std::iter::{IntoIterator, Iterator}; use std::path::PathBuf; +use std::rc::Rc; use std::str::FromStr; mod command; use opentitanlib::app::command::CommandDispatch; use opentitanlib::app::TransportWrapper; use opentitanlib::backend; +use opentitanlib::transport::MaintainConnection; #[allow(clippy::large_enum_variant)] #[derive(Debug, Parser, CommandDispatch)] @@ -214,13 +216,25 @@ fn print_command_result(opts: &Opts, result: Result>>) // Execute is a convenience function for taking a list of strings, // parsing them into a command, executing the command and printing the result. -fn execute(args: I, opts: &Opts, transport: &TransportWrapper) -> Result<()> +fn execute( + args: I, + opts: &Opts, + transport: &TransportWrapper, + maintain_connection: &mut Option>, +) -> Result<()> where I: IntoIterator, { let command = RootCommandHierarchy::parse_from( std::iter::once(OsString::from("opentitantool")).chain(args), ); + if command.exclusive_use_of_transport() { + if maintain_connection.is_none() { + *maintain_connection = Some(transport.maintain_connection()?); + } + } else { + *maintain_connection = None; + } print_command_result(opts, command.run(opts, transport))?; Ok(()) } @@ -230,13 +244,23 @@ fn main() -> Result<()> { let transport = backend::create(&opts.backend_opts)?; + let mut _maintain_connection = None; + for command in &opts.exec { execute( shellwords::split(command)?.iter().map(OsString::from), &opts, &transport, + &mut _maintain_connection, )?; } + if opts.command.exclusive_use_of_transport() { + if _maintain_connection.is_none() { + _maintain_connection = Some(transport.maintain_connection()?); + } + } else { + _maintain_connection = None; + } print_command_result(&opts, opts.command.run(&opts, &transport))?; Ok(()) }