MGS: Refine serial console messages to/from SP

gateway-messages/src/lib.rs

@@ -51,8 +51,14 @@ pub enum RequestKind {
         command: IgnitionCommand,
     },
     SpState,
+    SerialConsoleAttach(SpComponent),
     /// `SerialConsoleWrite` always includes trailing raw data.
-    SerialConsoleWrite(SpComponent),
+    SerialConsoleWrite {
+        /// Offset of the first byte of this packet, starting from 0 when this
+        /// serial console session was attached.
+        offset: u64,
+    },
+    SerialConsoleDetach,
     UpdateStart(UpdateStart),
     /// `UpdateChunk` always includes trailing raw data.
     UpdateChunk(UpdateChunk),
@@ -87,7 +93,9 @@ pub enum ResponseKind {
     SpState(SpState),
     UpdateStartAck,
     UpdateChunkAck,
-    SerialConsoleWriteAck,
+    SerialConsoleAttachAck,
+    SerialConsoleWriteAck { furthest_ingested_offset: u64 },
+    SerialConsoleDetachAck,
     SysResetPrepareAck,
     // There is intentionally no `SysResetTriggerAck` response; the expected
     // "resposne" to `SysResetTrigger` is an SP reset, which won't allow for
@@ -125,6 +133,11 @@ pub enum ResponseError {
     RequestUnsupportedForComponent,
     /// The specified ignition target does not exist.
     IgnitionTargetDoesNotExist(u8),
+    /// Cannot write to the serial console because it is not attached.
+    SerialConsoleNotAttached,
+    /// Cannot attach to the serial console because another MGS instance is
+    /// already attached.
+    SerialConsoleAlreadyAttached,
     /// An update is already in progress with the specified amount of data
     /// already provided. MGS should resume the update at that offset.
     UpdateInProgress { bytes_received: u32 },
@@ -154,6 +167,12 @@ impl fmt::Display for ResponseError {
             ResponseError::IgnitionTargetDoesNotExist(target) => {
                 write!(f, "nonexistent ignition target {}", target)
             }
+            ResponseError::SerialConsoleNotAttached => {
+                write!(f, "serial console is not attached")
+            }
+            ResponseError::SerialConsoleAlreadyAttached => {
+                write!(f, "serial console already attached")
+            }
             ResponseError::UpdateInProgress { bytes_received } => {
                 write!(f, "update still in progress ({bytes_received} bytes received so far)")
             }
@@ -190,7 +209,17 @@ pub enum SpMessageKind {
 
     /// Data traveling from an SP-attached component (in practice, a CPU) on the
     /// component's serial console.
-    SerialConsole(SpComponent),
+    ///
+    /// Note that SP -> MGS serial console messages are currently _not_
+    /// acknowledged or retried; they are purely "fire and forget" from the SP's
+    /// point of view. Once it sends data in a packet, it discards it from its
+    /// local buffer.
+    SerialConsole {
+        component: SpComponent,
+        /// Offset of the first byte in this packet's data starting from 0 when
+        /// the serial console session was attached.
+        offset: u64,
+    },
 }
 
 #[derive(

gateway-messages/src/sp_impl.rs

@@ -82,15 +82,28 @@ pub trait SpHandler {
         data: &[u8],
     ) -> Result<(), ResponseError>;
 
-    // TODO Should we return "number of bytes written" here, or is it sufficient
-    // to say "all or none"? Would be nice for the caller to not have to resend
-    // UDP chunks; can SP ensure it writes all data locally?
-    fn serial_console_write(
+    fn serial_console_attach(
         &mut self,
         sender: SocketAddrV6,
         port: SpPort,
         component: SpComponent,
+    ) -> Result<(), ResponseError>;
+
+    /// The returned u64 should be the offset we want to receive in the next
+    /// call to `serial_console_write()`; i.e., the furthest offset we've
+    /// ingested (either by writing to the console or by buffering to write it).
+    fn serial_console_write(
+        &mut self,
+        sender: SocketAddrV6,
+        port: SpPort,
+        offset: u64,
         data: &[u8],
+    ) -> Result<u64, ResponseError>;
+
+    fn serial_console_detach(
+        &mut self,
+        sender: SocketAddrV6,
+        port: SpPort,
     ) -> Result<(), ResponseError>;
 
     fn reset_prepare(
@@ -169,7 +182,8 @@ pub fn handle_message<H: SpHandler>(
     // Do we expect any trailing raw data? Only for specific kinds of messages;
     // if we get any for other messages, bail out.
     let trailing_data = match &request.kind {
-        RequestKind::UpdateChunk(_) | RequestKind::SerialConsoleWrite(_) => {
+        RequestKind::UpdateChunk(_)
+        | RequestKind::SerialConsoleWrite { .. } => {
             unpack_trailing_data(leftover)?
         }
         _ => {
@@ -203,9 +217,17 @@ pub fn handle_message<H: SpHandler>(
         RequestKind::UpdateChunk(chunk) => handler
             .update_chunk(sender, port, chunk, trailing_data)
             .map(|()| ResponseKind::UpdateChunkAck),
-        RequestKind::SerialConsoleWrite(packet) => handler
-            .serial_console_write(sender, port, packet, trailing_data)
-            .map(|()| ResponseKind::SerialConsoleWriteAck),
+        RequestKind::SerialConsoleAttach(component) => handler
+            .serial_console_attach(sender, port, component)
+            .map(|()| ResponseKind::SerialConsoleAttachAck),
+        RequestKind::SerialConsoleWrite { offset } => handler
+            .serial_console_write(sender, port, offset, trailing_data)
+            .map(|n| ResponseKind::SerialConsoleWriteAck {
+                furthest_ingested_offset: n,
+            }),
+        RequestKind::SerialConsoleDetach => handler
+            .serial_console_detach(sender, port)
+            .map(|()| ResponseKind::SerialConsoleDetachAck),
         RequestKind::SysResetPrepare => handler
             .reset_prepare(sender, port)
             .map(|()| ResponseKind::SysResetPrepareAck),

gateway-sp-comms/src/communicator.rs

@@ -198,7 +198,7 @@ impl Communicator {
     ) -> Result<(), Error> {
         let port = self.id_to_port(sp)?;
         let sp = self.switch.sp(port).ok_or(Error::SpAddressUnknown(sp))?;
-        sp.serial_console_detach().await;
+        sp.serial_console_detach().await?;
         Ok(())
     }
 
@@ -291,7 +291,11 @@ pub(crate) trait ResponseKindExt {
 
     fn expect_sp_state(self) -> Result<SpState, BadResponseType>;
 
-    fn expect_serial_console_write_ack(self) -> Result<(), BadResponseType>;
+    fn expect_serial_console_attach_ack(self) -> Result<(), BadResponseType>;
+
+    fn expect_serial_console_write_ack(self) -> Result<u64, BadResponseType>;
+
+    fn expect_serial_console_detach_ack(self) -> Result<(), BadResponseType>;
 
     fn expect_update_start_ack(self) -> Result<(), BadResponseType>;
 
@@ -314,9 +318,15 @@ impl ResponseKindExt for ResponseKind {
                 response_kind_names::IGNITION_COMMAND_ACK
             }
             ResponseKind::SpState(_) => response_kind_names::SP_STATE,
-            ResponseKind::SerialConsoleWriteAck => {
+            ResponseKind::SerialConsoleAttachAck => {
+                response_kind_names::SERIAL_CONSOLE_ATTACH_ACK
+            }
+            ResponseKind::SerialConsoleWriteAck { .. } => {
                 response_kind_names::SERIAL_CONSOLE_WRITE_ACK
             }
+            ResponseKind::SerialConsoleDetachAck => {
+                response_kind_names::SERIAL_CONSOLE_DETACH_ACK
+            }
             ResponseKind::UpdateStartAck => {
                 response_kind_names::UPDATE_START_ACK
             }
@@ -381,9 +391,31 @@ impl ResponseKindExt for ResponseKind {
         }
     }
 
-    fn expect_serial_console_write_ack(self) -> Result<(), BadResponseType> {
+    fn expect_serial_console_attach_ack(self) -> Result<(), BadResponseType> {
+        match self {
+            ResponseKind::SerialConsoleAttachAck => Ok(()),
+            other => Err(BadResponseType {
+                expected: response_kind_names::SP_STATE,
+                got: other.name(),
+            }),
+        }
+    }
+
+    fn expect_serial_console_write_ack(self) -> Result<u64, BadResponseType> {
+        match self {
+            ResponseKind::SerialConsoleWriteAck {
+                furthest_ingested_offset,
+            } => Ok(furthest_ingested_offset),
+            other => Err(BadResponseType {
+                expected: response_kind_names::SP_STATE,
+                got: other.name(),
+            }),
+        }
+    }
+
+    fn expect_serial_console_detach_ack(self) -> Result<(), BadResponseType> {
         match self {
-            ResponseKind::SerialConsoleWriteAck => Ok(()),
+            ResponseKind::SerialConsoleDetachAck => Ok(()),
             other => Err(BadResponseType {
                 expected: response_kind_names::SP_STATE,
                 got: other.name(),
@@ -428,8 +460,12 @@ mod response_kind_names {
     pub(super) const BULK_IGNITION_STATE: &str = "bulk_ignition_state";
     pub(super) const IGNITION_COMMAND_ACK: &str = "ignition_command_ack";
     pub(super) const SP_STATE: &str = "sp_state";
+    pub(super) const SERIAL_CONSOLE_ATTACH_ACK: &str =
+        "serial_console_attach_ack";
     pub(super) const SERIAL_CONSOLE_WRITE_ACK: &str =
         "serial_console_write_ack";
+    pub(super) const SERIAL_CONSOLE_DETACH_ACK: &str =
+        "serial_console_detach_ack";
     pub(super) const UPDATE_START_ACK: &str = "update_start_ack";
     pub(super) const UPDATE_CHUNK_ACK: &str = "update_chunk_ack";
     pub(super) const SYS_RESET_PREPARE_ACK: &str = "sys_reset_prepare_ack";

gateway-sp-comms/src/error.rs

@@ -25,6 +25,8 @@ pub enum SpCommunicationError {
     BadResponseType(#[from] BadResponseType),
     #[error("Error response from SP: {0}")]
     SpError(#[from] ResponseError),
+    #[error("Bogus serial console state; detach and reattach")]
+    BogusSerialConsoleState,
 }
 
 #[derive(Debug, Error)]
@@ -37,10 +39,6 @@ pub enum UpdateError {
     Chunk { offset: u32, err: SpCommunicationError },
 }
 
-#[derive(Debug, Error)]
-#[error("serial console already attached")]
-pub struct SerialConsoleAlreadyAttached;
-
 #[derive(Debug, Error)]
 pub enum StartupError {
     #[error("error binding to UDP address {addr}: {err}")]
@@ -75,14 +73,6 @@ pub enum Error {
     SpCommunicationFailed(#[from] SpCommunicationError),
     #[error("updating SP failed: {0}")]
     UpdateFailed(#[from] UpdateError),
-    #[error("serial console is already attached")]
-    SerialConsoleAttached,
-}
-
-impl From<SerialConsoleAlreadyAttached> for Error {
-    fn from(_: SerialConsoleAlreadyAttached) -> Self {
-        Self::SerialConsoleAttached
-    }
 }
 
 #[derive(Debug, Error)]