cpu_seq: Encode ereports into static buffers (#2225)

In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.

[1]: #2214 (comment)

Author: hawkw

Cargo.lock

@@ -21,6 +21,7 @@ ringbuf = { path = "../../lib/ringbuf" }
 userlib = { path = "../../sys/userlib", features = ["panic-messages"] }
 task-jefe-api = { path = "../../task/jefe-api" }
 task-packrat-api = { path = "../../task/packrat-api", features = ["serde"] }
+static-cell = { path = "../../lib/static-cell" }
 
 cfg-if = { workspace = true }
 idol-runtime.workspace = true

drv/cosmo-seq-server/Cargo.toml

@@ -378,8 +378,13 @@ struct ServerImpl {
     hf: HostFlash,
     seq: fmc_periph::Sequencer,
     vcore: VCore,
+    /// Static buffer for encoding ereports. This is a static so that we don't
+    /// have it on the stack when encoding ereports.
+    ereport_buf: &'static mut [u8; EREPORT_BUF_LEN],
 }
 
+const EREPORT_BUF_LEN: usize = 256;
+
 impl ServerImpl {
     fn new(
         token: drv_spartan7_loader_api::Spartan7Token,
@@ -399,13 +404,21 @@ impl ServerImpl {
         let jefe = Jefe::from(JEFE.get_task_id());
         jefe.set_state(PowerState::A2 as u32);
 
+        let ereport_buf = {
+            use static_cell::ClaimOnceCell;
+            static EREPORT_BUF: ClaimOnceCell<[u8; EREPORT_BUF_LEN]> =
+                ClaimOnceCell::new([0; EREPORT_BUF_LEN]);
+            EREPORT_BUF.claim()
+        };
+
         ServerImpl {
             state: PowerState::A2,
             jefe,
             sys: Sys::from(SYS.get_task_id()),
             hf: HostFlash::from(HF.get_task_id()),
             seq,
             vcore: VCore::new(I2C.get_task_id(), packrat),
+            ereport_buf,
         }
     }
 
@@ -741,7 +754,8 @@ impl ServerImpl {
                 vddcr_cpu0: ifr.pwr_cont1_to_fpga1_alert,
                 vddcr_cpu1: ifr.pwr_cont2_to_fpga1_alert,
             };
-            self.vcore.handle_pmbus_alert(which_rails, now);
+            self.vcore
+                .handle_pmbus_alert(which_rails, now, self.ereport_buf);
 
             // We need not instruct the sequencer to reset. PMBus alerts from
             // the RAA229620As are divided into two categories, "warnings" and

drv/cosmo-seq-server/src/main.rs

@@ -162,18 +162,31 @@ impl VCore {
         Ok(())
     }
 
-    pub fn handle_pmbus_alert(&self, mut rails: Rails, now: u64) {
+    pub fn handle_pmbus_alert(
+        &self,
+        mut rails: Rails,
+        now: u64,
+        ereport_buf: &mut [u8],
+    ) {
         ringbuf_entry!(Trace::PmbusAlert {
             timestamp: now,
             rails,
         });
 
-        let cpu0_state =
-            self.record_pmbus_status(now, Rail::VddcrCpu0, rails.vddcr_cpu0);
+        let cpu0_state = self.record_pmbus_status(
+            now,
+            Rail::VddcrCpu0,
+            rails.vddcr_cpu0,
+            ereport_buf,
+        );
         rails.vddcr_cpu0 |= cpu0_state.faulted;
 
-        let cpu1_state =
-            self.record_pmbus_status(now, Rail::VddcrCpu1, rails.vddcr_cpu1);
+        let cpu1_state = self.record_pmbus_status(
+            now,
+            Rail::VddcrCpu1,
+            rails.vddcr_cpu1,
+            ereport_buf,
+        );
         rails.vddcr_cpu1 |= cpu1_state.faulted;
 
         if cpu0_state.input_fault || cpu1_state.input_fault {
@@ -246,6 +259,7 @@ impl VCore {
         now: u64,
         rail: Rail,
         alerted: bool,
+        ereport_buf: &mut [u8],
     ) -> RegulatorState {
         use pmbus::commands::raa229620a::STATUS_WORD;
 
@@ -372,7 +386,18 @@ impl VCore {
             status,
             pwr_good: power_good,
         };
-        deliver_ereport(rail, &self.packrat, &ereport);
+        match self.packrat.serialize_ereport(&ereport, ereport_buf) {
+            Ok(len) => ringbuf_entry!(Trace::EreportSent(rail, len)),
+            Err(task_packrat_api::EreportSerializeError::Packrat {
+                len,
+                err,
+            }) => {
+                ringbuf_entry!(Trace::EreportLost(rail, len, err))
+            }
+            Err(task_packrat_api::EreportSerializeError::Serialize(_)) => {
+                ringbuf_entry!(Trace::EreportTooBig(rail))
+            }
+        }
 
         RegulatorState {
             faulted,
@@ -432,25 +457,3 @@ fn retry_i2c_txn<T>(
         }
     }
 }
-
-// This is in its own function so that the ereport buffer is only on the stack
-// while we're using it, and not for the entireity of `record_pmbus_status`,
-// which calls into a bunch of other functions. This may reduce our stack depth
-// a bit.
-#[inline(never)]
-fn deliver_ereport(
-    rail: Rail,
-    packrat: &Packrat,
-    data: &impl serde::Serialize,
-) {
-    let mut ereport_buf = [0u8; 256];
-    match packrat.serialize_ereport(data, &mut ereport_buf[..]) {
-        Ok(len) => ringbuf_entry!(Trace::EreportSent(rail, len)),
-        Err(task_packrat_api::EreportSerializeError::Packrat { len, err }) => {
-            ringbuf_entry!(Trace::EreportLost(rail, len, err))
-        }
-        Err(task_packrat_api::EreportSerializeError::Serialize(_)) => {
-            ringbuf_entry!(Trace::EreportTooBig(rail))
-        }
-    }
-}

drv/cosmo-seq-server/src/vcore.rs

@@ -20,6 +20,7 @@ ringbuf = { path = "../../lib/ringbuf" }
 task-jefe-api = { path = "../../task/jefe-api" }
 task-packrat-api = { path = "../../task/packrat-api", features = ["serde"] }
 userlib = { path = "../../sys/userlib", features = ["panic-messages"] }
+static-cell = { path = "../../lib/static-cell" }
 
 cfg-if = { workspace = true }
 cortex-m = { workspace = true }

drv/gimlet-seq-server/Cargo.toml

@@ -201,9 +201,13 @@ struct ServerImpl<S: SpiServer> {
     hf: hf_api::HostFlash,
     vcore: vcore::VCore,
     deadline: u64,
+    // Buffer for encoding ereports. This is a static so that it's not on the
+    // stack when handling interrupts.
+    ereport_buf: &'static mut [u8],
 }
 
 const TIMER_INTERVAL: u32 = 10;
+const EREPORT_BUF_LEN: usize = 128;
 
 impl<S: SpiServer + Clone> ServerImpl<S> {
     fn init(
@@ -483,6 +487,13 @@ impl<S: SpiServer + Clone> ServerImpl<S> {
 
         let (device, rail) = i2c_config::pmbus::vdd_vcore(I2C.get_task_id());
 
+        let ereport_buf = {
+            use static_cell::ClaimOnceCell;
+            static EREPORT_BUF: ClaimOnceCell<[u8; EREPORT_BUF_LEN]> =
+                ClaimOnceCell::new([0; EREPORT_BUF_LEN]);
+            EREPORT_BUF.claim()
+        };
+
         let mut server = Self {
             state: PowerState::A2,
             sys: sys.clone(),
@@ -491,6 +502,7 @@ impl<S: SpiServer + Clone> ServerImpl<S> {
             hf,
             deadline: 0,
             vcore: vcore::VCore::new(sys, packrat, &device, rail),
+            ereport_buf,
         };
 
         // Power on, unless suppressed by the `stay-in-a2` feature
@@ -530,7 +542,7 @@ impl<S: SpiServer> NotificationHandler for ServerImpl<S> {
 
     fn handle_notification(&mut self, bits: u32) {
         if (bits & self.vcore.mask()) != 0 {
-            self.vcore.handle_notification();
+            self.vcore.handle_notification(self.ereport_buf);
         }
 
         if (bits & notifications::TIMER_MASK) == 0 {

drv/gimlet-seq-server/src/main.rs

@@ -139,7 +139,7 @@ impl VCore {
         Ok(())
     }
 
-    pub fn handle_notification(&self) {
+    pub fn handle_notification(&self, ereport_buf: &mut [u8]) {
         let now = sys_get_timer().now;
         let asserted = self.sys.gpio_read(VCORE_TO_SP_ALERT_L) == 0;
 
@@ -149,7 +149,7 @@ impl VCore {
         });
 
         if asserted {
-            self.read_pmbus_status(now);
+            self.read_pmbus_status(now, ereport_buf);
             // Clear the fault now so that PMALERT_L is reasserted if a
             // subsequent fault occurs. Note that if the fault *condition*
             // continues, the fault bits in the status registers will remain
@@ -162,7 +162,7 @@ impl VCore {
         let _ = self.sys.gpio_irq_control(self.mask(), IrqControl::Enable);
     }
 
-    fn read_pmbus_status(&self, now: u64) {
+    fn read_pmbus_status(&self, now: u64, ereport_buf: &mut [u8]) {
         use pmbus::commands::raa229618::STATUS_WORD;
 
         // Read PMBus status registers and prepare an ereport.
@@ -275,7 +275,21 @@ impl VCore {
             pwr_good,
             status,
         };
-        deliver_ereport(&self.packrat, &ereport);
+        match self
+            .packrat
+            .serialize_ereport(&ereport, &mut ereport_buf[..])
+        {
+            Ok(len) => ringbuf_entry!(Trace::EreportSent(len)),
+            Err(task_packrat_api::EreportSerializeError::Packrat {
+                len,
+                err,
+            }) => {
+                ringbuf_entry!(Trace::EreportLost(len, err))
+            }
+            Err(task_packrat_api::EreportSerializeError::Serialize(_)) => {
+                ringbuf_entry!(Trace::EreportTooBig)
+            }
+        }
 
         // If the `INPUT_FAULT` bit in `STATUS_WORD` is set, or any bit is hot
         // in `STATUS_INPUT`, sample Vin in order to record the voltage dip in
@@ -331,21 +345,3 @@ struct Ereport {
     pwr_good: Option<bool>,
     status: PmbusStatus,
 }
-
-// This is in its own function so that the ereport buffer and `Ereport` struct
-// are only on the stack while we're using it, and not for the entireity of
-// `record_pmbus_status`, which calls into a bunch of other functions. This may
-// reduce our stack depth a bit.
-#[inline(never)]
-fn deliver_ereport(packrat: &packrat_api::Packrat, data: &impl Serialize) {
-    let mut ereport_buf = [0u8; 128];
-    match packrat.serialize_ereport(data, &mut ereport_buf[..]) {
-        Ok(len) => ringbuf_entry!(Trace::EreportSent(len)),
-        Err(task_packrat_api::EreportSerializeError::Packrat { len, err }) => {
-            ringbuf_entry!(Trace::EreportLost(len, err))
-        }
-        Err(task_packrat_api::EreportSerializeError::Serialize(_)) => {
-            ringbuf_entry!(Trace::EreportTooBig)
-        }
-    }
-}