better softnpu management command reliability

oxidecomputer · Nov 25, 2023 · 7d3e4ee · 7d3e4ee
1 parent f0733f9
commit 7d3e4ee
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 8 deletions.
diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs
@@ -599,8 +599,17 @@ impl<'a> MachineInitializer<'a> {
         // NOTE: SoftNpu squats on com4.
         let pio = &self.machine.bus_pio;
         let port = ibmpc::PORT_COM4;
-        let uart =
-            LpcUart::new(chipset.device().irq_pin(ibmpc::IRQ_COM4).unwrap());
+
+        // The default uart fifo length of 1 has caused reliability problems.
+        // Messages intermittently come through with a missing bytes. It's
+        // fairly easy to make this happen with a fifo length of one by just
+        // sending a stream of softnpu commands in a loop. With a fifo length
+        // of 10, I've not observed any missing bytes, even under a continuous
+        // stream of management commands.
+        let uart = LpcUart::with_fifo_len(
+            chipset.device().irq_pin(ibmpc::IRQ_COM4).unwrap(),
+            10,
+        );
         uart.set_autodiscard(true);
         LpcUart::attach(&uart, pio, port);
         self.inv.register_instance(&uart, "softnpu-uart")?;

diff --git a/lib/propolis/src/hw/uart/lpc.rs b/lib/propolis/src/hw/uart/lpc.rs
@@ -54,6 +54,22 @@ impl LpcUart {
             notify_writable: NotifierCell::new(),
         })
     }
+
+    pub fn with_fifo_len(
+        irq_pin: Box<dyn IntrPin>,
+        fifo_len: usize,
+    ) -> Arc<Self> {
+        Arc::new(Self {
+            state: Mutex::new(UartState {
+                uart: Uart::with_fifo_len(fifo_len),
+                irq_pin,
+                auto_discard: true,
+                paused: false,
+            }),
+            notify_readable: NotifierCell::new(),
+            notify_writable: NotifierCell::new(),
+        })
+    }
     pub fn attach(self: &Arc<Self>, bus: &PioBus, port: u16) {
         let this = self.clone();
         let piofn = Arc::new(move |_port: u16, rwo: RWOp| this.pio_rw(rwo))

diff --git a/lib/propolis/src/hw/uart/uart16550.rs b/lib/propolis/src/hw/uart/uart16550.rs
@@ -45,8 +45,9 @@ pub struct Uart {
     rx_fifo: Fifo,
     tx_fifo: Fifo,
 }
-impl Uart {
-    pub fn new() -> Self {
+
+impl Default for Uart {
+    fn default() -> Self {
         Uart {
             reg_intr_enable: 0,
             reg_intr_status: ISRC_NONE,
@@ -61,11 +62,23 @@ impl Uart {
 
             thre_intr: false,
             intr_pin: false,
-            // TODO: Don't deal with "real" sized fifos for now
             rx_fifo: Fifo::new(1),
             tx_fifo: Fifo::new(1),
         }
     }
+}
+
+impl Uart {
+    pub fn new() -> Self {
+        Self::default()
+    }
+    pub fn with_fifo_len(fifo_size: usize) -> Self {
+        Self {
+            rx_fifo: Fifo::new(fifo_size),
+            tx_fifo: Fifo::new(fifo_size),
+            ..Default::default()
+        }
+    }
     /// Read UART register
     pub fn reg_read(&mut self, offset: u8) -> u8 {
         let val = match (offset, self.is_dlab()) {

diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs
@@ -44,7 +44,6 @@ use slog::{error, info, warn, Logger};
 
 // Transit jumbo frames
 const MTU: usize = 9216;
-
 const SOFTNPU_CPU_AUX_PORT: u16 = 1000;
 
 pub const MANAGEMENT_MESSAGE_PREAMBLE: u8 = 0b11100101;
@@ -810,10 +809,10 @@ impl ManagementMessageReader {
                     if p + 1 < buf.len() {
                         &buf[p + 1..]
                     } else {
-                        &buf
+                        continue;
                     }
                 }
-                None => &buf,
+                None => continue,
             };
             match serde_json::from_slice(&msgbuf) {
                 Ok(msg) => return msg,