[verilator] Add shutdown request mechanism to prevent early UART closes in CI

hw/dv/dpi/gpiodpi/gpiodpi.c

@@ -1,4 +1,5 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
@@ -46,6 +47,8 @@ struct gpiodpi_ctx {
   // A counter of calls into the host_to_device_tick function; used to
   // avoid excessive `read` syscalls to the pipe fd.
   uint32_t counter;
+  // A designated 'virtual' pin index which the host can use to trigger an exit.
+  uint32_t pin_req_exit;
 
   // File descriptors and paths for the device-to-host and host-to-device
   // FIFOs.
@@ -107,7 +110,7 @@ static void print_usage(char *rfifo, char *wfifo, int n_bits) {
          wfifo);
 }
 
-void *gpiodpi_create(const char *name, int n_bits) {
+void *gpiodpi_create(const char *name, int n_bits, int pin_req_exit) {
   struct gpiodpi_ctx *ctx =
       (struct gpiodpi_ctx *)malloc(sizeof(struct gpiodpi_ctx));
   assert(ctx);
@@ -117,6 +120,12 @@ void *gpiodpi_create(const char *name, int n_bits) {
   assert(n_bits <= 32 && "n_bits must be <= 32");
   ctx->n_bits = n_bits;
 
+  // pin_req_exit must not be the index of a physical pin, and also must have an
+  // index which fits in a byte for the host to trigger it.
+  assert(pin_req_exit >= n_bits && "pin_req_exit must be >= n_bits");
+  assert(pin_req_exit < 256 && "pin_req_exit must be < 256");
+  ctx->pin_req_exit = pin_req_exit;
+
   ctx->driven_pin_values = 0;
   ctx->weak_pins = 0;
   ctx->counter = 0;
@@ -211,9 +220,10 @@ static void set_bit_val(uint32_t *word, uint32_t idx, bool val) {
   }
 }
 
-uint32_t gpiodpi_host_to_device_tick(void *ctx_void, svBitVecVal *gpio_oe,
-                                     svBitVecVal *gpio_pull_en,
-                                     svBitVecVal *gpio_pull_sel) {
+svBitVecVal gpiodpi_host_to_device_tick(void *ctx_void, svBitVecVal *gpio_oe,
+                                        svBitVecVal *gpio_pull_en,
+                                        svBitVecVal *gpio_pull_sel,
+                                        svBit *req_exit) {
   struct gpiodpi_ctx *ctx = (struct gpiodpi_ctx *)ctx_void;
   assert(ctx);
 
@@ -247,7 +257,7 @@ uint32_t gpiodpi_host_to_device_tick(void *ctx_void, svBitVecVal *gpio_oe,
               }
               CLR_BIT(ctx->driven_pin_values, idx);
               set_bit_val(&ctx->weak_pins, idx, weak);
-            } else {
+            } else if (idx != ctx->pin_req_exit) {
               fprintf(stderr,
                       "GPIO: Host tried to pull invalid pin low: pin %2d\n",
                       idx);
@@ -267,6 +277,11 @@ uint32_t gpiodpi_host_to_device_tick(void *ctx_void, svBitVecVal *gpio_oe,
               }
               SET_BIT(ctx->driven_pin_values, idx);
               set_bit_val(&ctx->weak_pins, idx, weak);
+            } else if (idx == ctx->pin_req_exit) {
+              // The request exit pin triggers an exit upon being set; the
+              // corresponding GPIO DPI SystemVerilog file triggers a $finish
+              // when req_exit is set high.
+              *req_exit = 1;
             } else {
               fprintf(stderr,
                       "GPIO: Host tried to pull invalid pin high: pin %2d\n",

hw/dv/dpi/gpiodpi/gpiodpi.h

@@ -1,4 +1,5 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
@@ -17,13 +18,19 @@ extern "C" {
  * @param name a name to use when creating the inner FIFO.
  * @param n_bits number of bits to write in each direction; this must be at
  *        most 32 bits.
+ * @param pin_req_exit 'virtual' pin index which the host can use to trigger
+ *        simulation exit; this index must be at least n_bits and at most 255.
  */
-void *gpiodpi_create(const char *name, int n_bits);
+void *gpiodpi_create(const char *name, int n_bits, int pin_req_exit);
 
 /**
  * Attempt to post the current GPIO state to the outside world.
  *
  * Intended to be called from SystemVerilog.
+ *
+ * @param ctx_void void pointer to the GPIO DPI context.
+ * @param gpio_data pointer to the GPIO data bit vector.
+ * @param gpio_data pointer to the GPIO output enable bit vector.
  */
 void gpiodpi_device_to_host(void *ctx_void, svBitVecVal *gpio_data,
                             svBitVecVal *gpio_oe);
@@ -38,11 +45,18 @@ void gpiodpi_device_to_host(void *ctx_void, svBitVecVal *gpio_data,
  * commands are ignored.
  *
  * Intended to be called from SystemVerilog.
+ * @param ctx_void void pointer to the GPIO DPI context.
+ * @param gpio_oe pointer to the GPIO output enable bit vector.
+ * @param gpio_pull_en pointer to GPIO pull up/down enable bit vector.
+ * @param gpio_pull_sel pointer to the GPIO pull up/down selection bit vector.
+ * @param pin_req_exit 'virtual' pin index which the host can use to trigger
+ *        simulation exit; this index must be at least n_bits and at most 255.
  * @return the values to pull the GPIO pins to.
  */
 uint32_t gpiodpi_host_to_device_tick(void *ctx_void, svBitVecVal *gpio_oe,
                                      svBitVecVal *gpio_pull_en,
-                                     svBitVecVal *gpio_pull_sel);
+                                     svBitVecVal *gpio_pull_sel,
+                                     svBit *req_exit);
 
 /**
  * Relinquish resources held by a GPIO DPI interface.

hw/dv/dpi/gpiodpi/gpiodpi.sv

@@ -1,11 +1,13 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
 module gpiodpi
 #(
   parameter string NAME = "gpio0",
-  parameter int    N_GPIO = 32
+  parameter int    N_GPIO = 32,
+  parameter int    PIN_REQ_EXIT = 254
 )(
   input  logic              clk_i,
   input  logic              rst_ni,
@@ -18,7 +20,7 @@ module gpiodpi
   input  logic [N_GPIO-1:0] gpio_pull_sel
 );
    import "DPI-C" function
-     chandle gpiodpi_create(input string name, input int n_bits);
+     chandle gpiodpi_create(input string name, input int n_bits, input int pin_req_exit);
 
    import "DPI-C" function
      void gpiodpi_device_to_host(input chandle ctx, input logic [N_GPIO-1:0] gpio_d2p,
@@ -31,13 +33,15 @@ module gpiodpi
      int gpiodpi_host_to_device_tick(input chandle ctx,
                                      input logic [N_GPIO-1:0] gpio_en_d2p,
                                      input logic [N_GPIO-1:0] gpio_pull_en,
-                                     input logic [N_GPIO-1:0] gpio_pull_sel);
+                                     input logic [N_GPIO-1:0] gpio_pull_sel,
+                                     output bit req_exit);
 
+   bit req_exit;
    chandle ctx;
 
    function automatic void initialize();
      $display($time, "GPIO: creating gpiodpi");
-     ctx = gpiodpi_create(NAME, N_GPIO);
+     ctx = gpiodpi_create(NAME, N_GPIO, PIN_REQ_EXIT);
    endfunction
 
    // Allow being activated past initial time.
@@ -62,16 +66,21 @@ module gpiodpi
      if (gpio_d2p_r != gpio_d2p) begin
        gpiodpi_device_to_host(ctx, gpio_d2p, gpio_en_d2p);
      end
+    if (req_exit) begin
+      $display("GPIO: Host pulled REQ_EXIT high, exiting simulation.");
+      $finish;
+    end
    end
 
-   logic gpio_write_pulse;
-
+   // The req_exit signal starts low at initialization, and is set by the DPI
+   // function gpiodpi_host_to_device_tick in response to the host asserting the
+   // 'virtual' pin REQUEST_EXIT with index designated by PIN_REQ_EXIT.
    always_ff @(posedge eff_clk or negedge rst_ni) begin
      if (!rst_ni) begin
        gpio_p2d <= '0; // default value
      end else begin
-       gpio_p2d <= gpiodpi_host_to_device_tick(ctx, gpio_en_d2p, gpio_pull_en, gpio_pull_sel);
+       gpio_p2d <= gpiodpi_host_to_device_tick(ctx, gpio_en_d2p, gpio_pull_en, gpio_pull_sel,
+                                               req_exit);
      end
    end
-
 endmodule

hw/top_earlgrey/dv/verilator/chip_sim_tb.sv

@@ -1,4 +1,5 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
@@ -198,13 +199,15 @@ module chip_sim_tb (
     `SIM_SRAM_IF.start_addr = `VERILATOR_TEST_STATUS_ADDR;
     u_sw_test_status_if.sw_test_status_addr = `SIM_SRAM_IF.start_addr;
   end
-
+  // Print the test status on completion once, using the sw_test_status_printed
+  // variable to ensure we don't double-print the results.
+  bit sw_test_status_printed;
   always @(posedge clk_i) begin
-    if (u_sw_test_status_if.sw_test_done) begin
+    if (u_sw_test_status_if.sw_test_done && !sw_test_status_printed) begin
       $display("Verilator sim termination requested");
       $display("Your simulation wrote to 0x%h", u_sw_test_status_if.sw_test_status_addr);
       dv_test_status_pkg::dv_test_status(u_sw_test_status_if.sw_test_passed);
-      $finish;
+      sw_test_status_printed = 1'b1;
     end
   end
 

sw/host/ot_transports/verilator/BUILD

@@ -1,4 +1,5 @@
 # Copyright lowRISC contributors (OpenTitan project).
+# Copyright zeroRISC Inc.
 # Licensed under the Apache License, Version 2.0, see LICENSE for details.
 # SPDX-License-Identifier: Apache-2.0
 
@@ -25,6 +26,7 @@ rust_library(
         "@crate_index//:log",
         "@crate_index//:regex",
         "@crate_index//:serde",
+        "@crate_index//:wait-timeout",
     ],
 )
 

sw/host/ot_transports/verilator/src/subprocess.rs

@@ -1,11 +1,13 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
 use std::io::ErrorKind;
 use std::process::{Child, Command, Stdio};
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, Instant};
+use wait_timeout::ChildExt;
 
 use anyhow::{Context, Result, anyhow, ensure};
 use regex::Regex;
@@ -141,11 +143,22 @@ impl Subprocess {
         Err(anyhow!("Timed out"))
     }
 
-    /// Kill the verilator subprocess.
-    pub fn kill(&mut self) -> Result<()> {
-        match self.child.kill() {
-            Err(error) if error.kind() != ErrorKind::InvalidInput => Err(error.into()),
-            _ => Ok(()),
+    /// Shutdown the verilator subprocess with a timeout.
+    pub fn shutdown_with_timeout(&mut self, timeout: Duration) -> Result<()> {
+        // Try waiting for the process to exit on its own; if dispatch() was
+        // called on the parent VerilatorTransport, then the REQUEST_EXIT pin
+        // was already asserted, meaning that the verilator subprocess should be
+        // shutting down on its own now.
+        log::info!("Waiting for verilator subprocess to terminate.");
+        match self.child.wait_timeout(timeout)? {
+            Some(_) => Ok(()),
+            None => {
+                log::warn!("Verilator shutdown timeout expired, sending kill.");
+                match self.child.kill() {
+                    Err(error) if error.kind() != ErrorKind::InvalidInput => Err(error.into()),
+                    _ => Ok(()),
+                }
+            }
         }
     }
 }
@@ -178,9 +191,11 @@ mod test {
     }
 
     #[test]
-    fn test_kill() -> Result<()> {
+    fn test_shutdown_with_timeout() -> Result<()> {
+        // Subprocess will not shut itself down, so this exercises the shutdown
+        // timeout logic instead.
         let mut subprocess = echo_subprocess()?;
-        subprocess.kill()?;
+        subprocess.shutdown_with_timeout(Duration::from_secs(1))?;
         Ok(())
     }
 }

sw/host/ot_transports/verilator/src/transport.rs

@@ -1,4 +1,5 @@
 // Copyright lowRISC contributors (OpenTitan project).
+// Copyright zeroRISC Inc.
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
@@ -23,7 +24,14 @@ use opentitanlib::util::parse_int::ParseInt;
 use super::gpio::{GpioInner, VerilatorGpioPin};
 use super::subprocess::{Options, Subprocess};
 
+/// Baud for UART communication with host.
 const UART_BAUD: u32 = 40;
+/// Timeout in milliseconds to wait after requesting exit for the verilator subprocess to finish.
+const REQUEST_EXIT_TIMEOUT: u64 = 2000;
+/// 'Virtual' pin used for requesting the verilator subprocess exit gracefully.
+const REQUEST_EXIT_PIN: u8 = 254;
+/// Disconnected 'virtual' reset pin, required by opentitanlib in every transport's interface definition.
+const RESET_PIN: u8 = 255;
 
 pub(crate) struct Inner {
     uart: Option<Rc<dyn Uart>>,
@@ -79,10 +87,26 @@ impl Verilator {
         })
     }
 
+    /// Helper function to fetch a verilator GPIO pin by index.
+    fn get_gpio_pin(&self, pin: u8) -> Result<Rc<dyn GpioPin>> {
+        ensure!(
+            pin < 32 || pin == REQUEST_EXIT_PIN || pin == RESET_PIN,
+            GpioError::InvalidPinNumber(pin)
+        );
+        let mut inner = self.inner.borrow_mut();
+        Ok(inner
+            .gpio
+            .pins
+            .entry(pin)
+            .or_insert_with(|| VerilatorGpioPin::new(self.inner.clone(), pin))
+            .clone())
+    }
+
     /// Shuts down the verilator subprocess.
     pub fn shutdown(&mut self) -> Result<()> {
+        self.get_gpio_pin(REQUEST_EXIT_PIN)?.write(true)?;
         if let Some(mut subprocess) = self.subprocess.take() {
-            subprocess.kill()
+            subprocess.shutdown_with_timeout(Duration::from_millis(REQUEST_EXIT_TIMEOUT))
         } else {
             Ok(())
         }
@@ -116,14 +140,7 @@ impl Transport for Verilator {
 
     fn gpio_pin(&self, instance: &str) -> Result<Rc<dyn GpioPin>> {
         let pin = u8::from_str(instance).with_context(|| format!("can't convert {instance:?}"))?;
-        ensure!(pin < 32 || pin == 255, GpioError::InvalidPinNumber(pin));
-        let mut inner = self.inner.borrow_mut();
-        Ok(inner
-            .gpio
-            .pins
-            .entry(pin)
-            .or_insert_with(|| VerilatorGpioPin::new(self.inner.clone(), pin))
-            .clone())
+        self.get_gpio_pin(pin)
     }
 
     fn dispatch(&self, action: &dyn Any) -> Result<Option<Box<dyn erased_serde::Serialize>>> {

third_party/rust/Cargo.toml

@@ -1,4 +1,5 @@
 # Copyright lowRISC contributors (OpenTitan project).
+# Copyright zeroRISC Inc.
 # Licensed under the Apache License, Version 2.0, see LICENSE for details.
 # SPDX-License-Identifier: Apache-2.0
 
@@ -84,6 +85,7 @@ thiserror = "2"
 tiny-keccak = {version = "2.0.2", features = ["cshake"]}
 tokio = { version = "1", features = ["full"] }
 typetag = "0.2"
+wait-timeout = "0.2.1"
 zerocopy = { version = "0.8", features = ["derive"] }
 zeroize = "1.8.1"