From 9ed1b89d7015e613df3f5dd9440d7078c8aefe77 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 31 Aug 2022 16:41:25 -0400 Subject: [PATCH 1/2] mgmt-gateway: update host flash This tracks the corresponding changes to MGS messaging merged in https://github.com/oxidecomputer/omicron/pull/1684: 1. `UpdateStart` has been broken into `UpdatePrepare` and `UpdatePrepareStatus` (which MGS will continue to send periodically until we respond that preparation is done), allowing for updates that have a potentially-long running prep step (like updating host flash, which can take up to several minutes to erase!). 2. Update messages now include a stream-id that we use to correlate related messages; we reject update messages that don't match our current stream ID. 3. Add handling for the new `UpdateAbort` abort message to cancel an in-progress update. By far the most complex bit of this is 1: I've moved setting the system timer out of `mgs_gimlet` (which previously set it only in relation to flushing serial console uart packets out to MGS) and into `main`: now the MGS handler only returns the deadline it wants `main` to set. If we're in the process of prepping for a host flash update (i.e., we need to erase the host flash), we'll set our deadline to 1 tick from now. When it fires, we'll erase 8 sectors (takes about 1 second, worst case), then return and allow `main` to check for other work. This allows us to continue to be responsive to incoming notifications, importantly network requests (allowing us to respond to the `UpdatePrepareStatus` messages in a timely way!). --- Cargo.lock | 3 +- app/gimlet/rev-b.toml | 2 +- app/gimletlet/app.toml | 2 +- task/mgmt-gateway/Cargo.toml | 3 +- task/mgmt-gateway/src/main.rs | 25 +- task/mgmt-gateway/src/mgs_common.rs | 58 +++- task/mgmt-gateway/src/mgs_gimlet.rs | 356 +++++++++++++++++++++++-- task/mgmt-gateway/src/mgs_psc.rs | 73 ++++- task/mgmt-gateway/src/mgs_sidecar.rs | 73 ++++- task/mgmt-gateway/src/update_buffer.rs | 55 +++- 10 files changed, 590 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8bf0101df3..79a367ba92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1655,7 +1655,7 @@ checksum = "1847abb9cb65d566acd5942e94aea9c8f547ad02c98e1649326fc0e8910b8b1e" [[package]] name = "gateway-messages" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/omicron?rev=f2e6237e57a36873fc748b6ecd9e42b8ef208c88#f2e6237e57a36873fc748b6ecd9e42b8ef208c88" +source = "git+https://github.com/oxidecomputer/omicron?rev=e6eccffb236d0c242b5f90d30d01daa47aa9d89f#e6eccffb236d0c242b5f90d30d01daa47aa9d89f" dependencies = [ "bitflags", "hubpack 0.1.0 (git+https://github.com/cbiffle/hubpack?rev=df08cc3a6e1f97381cd0472ae348e310f0119e25)", @@ -3392,6 +3392,7 @@ name = "task-mgmt-gateway" version = "0.1.0" dependencies = [ "cfg-if", + "drv-gimlet-hf-api", "drv-stm32h7-usart", "drv-stm32xx-uid", "drv-update-api", diff --git a/app/gimlet/rev-b.toml b/app/gimlet/rev-b.toml index f446ca5a34..3ad8610a5e 100644 --- a/app/gimlet/rev-b.toml +++ b/app/gimlet/rev-b.toml @@ -231,7 +231,7 @@ uses = [ "usart1", "system_flash", # TODO also used by `net`, both to read the stm32 uid ] -task-slots = ["jefe", "net", "update_server", "sys"] +task-slots = ["jefe", "net", "update_server", "sys", "hf"] features = ["gimlet", "usart1", "vlan"] interrupts = {"usart1.irq" = 0b10} diff --git a/app/gimletlet/app.toml b/app/gimletlet/app.toml index 1407ba1ae0..bb1acfc334 100644 --- a/app/gimletlet/app.toml +++ b/app/gimletlet/app.toml @@ -148,7 +148,7 @@ uses = [ "usart1", "system_flash", # TODO also used by `net`, both to read the stm32 uid ] -task-slots = ["jefe", "net", "update_server", "sys"] +task-slots = ["jefe", "net", "update_server", "sys", "hf"] features = ["gimlet", "usart1", "vlan"] interrupts = {"usart1.irq" = 0b10} diff --git a/task/mgmt-gateway/Cargo.toml b/task/mgmt-gateway/Cargo.toml index c52996845c..55ad652c91 100644 --- a/task/mgmt-gateway/Cargo.toml +++ b/task/mgmt-gateway/Cargo.toml @@ -10,6 +10,7 @@ num-traits = {version = "0.2", default-features = false} serde = {version = "1", default-features = false, features = ["derive"]} ssmarshal = {version = "1", default-features = false} +drv-gimlet-hf-api = {path = "../../drv/gimlet-hf-api"} drv-stm32h7-usart = {path = "../../drv/stm32h7-usart", features = ["h753"]} drv-stm32xx-uid = {path = "../../drv/stm32xx-uid", features = ["family-stm32h7"]} drv-update-api = {path = "../../drv/update-api"} @@ -19,7 +20,7 @@ task-jefe-api = {path = "../jefe-api"} task-net-api = {path = "../net-api", features = ["use-smoltcp"]} userlib = {path = "../../sys/userlib", features = ["panic-messages"]} -gateway-messages = {git = "https://github.com/oxidecomputer/omicron", rev = "f2e6237e57a36873fc748b6ecd9e42b8ef208c88"} +gateway-messages = {git = "https://github.com/oxidecomputer/omicron", rev = "e6eccffb236d0c242b5f90d30d01daa47aa9d89f"} [features] gimlet = [] diff --git a/task/mgmt-gateway/src/main.rs b/task/mgmt-gateway/src/main.rs index f7de8922ac..c080d4d36c 100644 --- a/task/mgmt-gateway/src/main.rs +++ b/task/mgmt-gateway/src/main.rs @@ -6,7 +6,8 @@ #![no_main] use gateway_messages::{ - sp_impl, sp_impl::Error as MgsDispatchError, IgnitionCommand, SpPort, + sp_impl, sp_impl::Error as MgsDispatchError, IgnitionCommand, SpComponent, + SpPort, }; use mutable_statics::mutable_statics; use ringbuf::{ringbuf, ringbuf_entry}; @@ -14,7 +15,7 @@ use task_net_api::{ Address, LargePayloadBehavior, Net, RecvError, SendError, SocketName, UdpMetadata, }; -use userlib::{sys_recv_closed, task_slot, TaskId, UnwrapLite}; +use userlib::{sys_recv_closed, sys_set_timer, task_slot, TaskId, UnwrapLite}; mod mgs_common; mod update_buffer; @@ -53,6 +54,7 @@ enum Log { SerialConsoleSend { buffered: usize }, UpdatePartial { bytes_written: usize }, UpdateComplete, + HostFlashSectorsErased { num_sectors: usize }, } #[derive(Debug, Clone, Copy, PartialEq)] @@ -73,12 +75,23 @@ enum MgsMessage { length: u16, }, SerialConsoleDetach, - UpdateStart { + UpdatePrepare { + component: SpComponent, + stream_id: u64, length: u32, + slot: u16, + }, + UpdatePrepareStatus { + component: SpComponent, + stream_id: u64, }, UpdateChunk { + component: SpComponent, offset: u32, }, + UpdateAbort { + component: SpComponent, + }, SysResetPrepare, } @@ -99,6 +112,8 @@ fn main() { let mut net_handler = NetHandler::claim_static_resources(); loop { + sys_set_timer(mgs_handler.timer_deadline(), TIMER_IRQ); + let note = sys_recv_closed( &mut [], NET_IRQ | USART_IRQ | TIMER_IRQ, @@ -112,6 +127,10 @@ fn main() { mgs_handler.drive_usart(); } + if (note & TIMER_IRQ) != 0 { + mgs_handler.handle_timer_fired(); + } + if (note & NET_IRQ) != 0 || mgs_handler.wants_to_send_packet_to_mgs() { net_handler.run_until_blocked(&mut mgs_handler); } diff --git a/task/mgmt-gateway/src/mgs_common.rs b/task/mgmt-gateway/src/mgs_common.rs index d37c031d4b..029a7bda73 100644 --- a/task/mgmt-gateway/src/mgs_common.rs +++ b/task/mgmt-gateway/src/mgs_common.rs @@ -7,7 +7,8 @@ use core::convert::Infallible; use drv_update_api::stm32h7::BLOCK_SIZE_BYTES; use drv_update_api::{Update, UpdateTarget}; use gateway_messages::{ - DiscoverResponse, ResponseError, SpPort, SpState, UpdateChunk, UpdateStart, + DiscoverResponse, ResponseError, SpComponent, SpPort, SpState, UpdateChunk, + UpdatePrepare, UpdatePrepareStatusRequest, UpdatePrepareStatusResponse, }; use ringbuf::ringbuf_entry_root; @@ -73,13 +74,19 @@ impl MgsCommon { }) } - pub(crate) fn update_start( + pub(crate) fn update_prepare( &mut self, - update: UpdateStart, + update: UpdatePrepare, ) -> Result<(), ResponseError> { - ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateStart { - length: update.total_size - })); + // We should only be called to update the SP itself. + if update.component != SpComponent::SP_ITSELF { + panic!(); + } + + // SP only has one "slot" (the alternate bank). + if update.slot != 0 { + return Err(ResponseError::InvalidSlotForComponent); + } self.update_buf.ensure_no_update_in_progress()?; @@ -87,22 +94,48 @@ impl MgsCommon { .prep_image_update(UpdateTarget::Alternate) .map_err(|err| ResponseError::UpdateFailed(err as u32))?; - self.update_buf.start(update.total_size as usize); + self.update_buf + .start(update.stream_id, update.total_size as usize); Ok(()) } + pub(crate) fn update_prepare_status( + &mut self, + request: UpdatePrepareStatusRequest, + ) -> Result { + self.update_buf + .ensure_matching_stream_id(request.stream_id)?; + + // We immediately prepare for update in `update_prepare()` + // and have no followup work to do; if this stream ID + // matches, we're already prepared. + Ok(UpdatePrepareStatusResponse { done: true }) + } + pub(crate) fn update_chunk( &mut self, chunk: UpdateChunk, data: &[u8], ) -> Result<(), ResponseError> { - ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateChunk { - offset: chunk.offset, - })); - self.update_buf - .ingest_chunk(&self.update_task, chunk.offset, data) + .ingest_chunk( + chunk.stream_id, + &self.update_task, + chunk.offset, + data, + ) + .map(|_progress| ()) + } + + pub(crate) fn update_abort(&mut self) -> Result<(), ResponseError> { + self.update_task + .abort_update() + .map_err(|err| ResponseError::UpdateFailed(err as u32))?; + + self.update_buf.reset(); + + Ok(()) } pub(crate) fn reset_prepare(&mut self) -> Result<(), ResponseError> { @@ -130,7 +163,6 @@ impl MgsCommon { } } -/// Grabs reference to a static `UpdateBuffer`. Can only be called once! fn claim_update_buffer_static( ) -> &'static mut heapless::Vec { use core::sync::atomic::{AtomicBool, Ordering}; diff --git a/task/mgmt-gateway/src/mgs_gimlet.rs b/task/mgmt-gateway/src/mgs_gimlet.rs index cf40d2dbc5..a529182a58 100644 --- a/task/mgmt-gateway/src/mgs_gimlet.rs +++ b/task/mgmt-gateway/src/mgs_gimlet.rs @@ -2,23 +2,29 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use crate::update_buffer::{UpdateBuffer, UpdateProgress}; use crate::{ mgs_common::MgsCommon, vlan_id_from_sp_port, Log, MgsMessage, SYS, - TIMER_IRQ, USART_IRQ, + USART_IRQ, }; use core::convert::Infallible; +use core::ops::Range; use core::sync::atomic::{AtomicBool, Ordering}; +use drv_gimlet_hf_api::{ + HfDevSelect, HfError, HfMuxState, HostFlash, PAGE_SIZE_BYTES, + SECTOR_SIZE_BYTES, +}; use drv_stm32h7_usart::Usart; use gateway_messages::{ sp_impl::SocketAddrV6, sp_impl::SpHandler, BulkIgnitionState, DiscoverResponse, IgnitionCommand, IgnitionState, ResponseError, SpComponent, SpMessage, SpMessageKind, SpPort, SpState, UpdateChunk, - UpdateStart, + UpdatePrepare, UpdatePrepareStatusRequest, UpdatePrepareStatusResponse, }; use heapless::Deque; use ringbuf::ringbuf_entry_root; use task_net_api::{Address, UdpMetadata}; -use userlib::{sys_get_timer, sys_irq_control, sys_set_timer, UnwrapLite}; +use userlib::{sys_get_timer, sys_irq_control, UnwrapLite}; /// Buffer sizes for serial console UDP / USART proxying. /// @@ -37,8 +43,11 @@ const SP_TO_MGS_SERIAL_CONSOLE_BUFFER_SIZE: usize = /// is this old, even if our buffer isn't full yet. const SERIAL_CONSOLE_FLUSH_TIMEOUT_MILLIS: u64 = 500; +userlib::task_slot!(HOST_FLASH, hf); + pub(crate) struct MgsHandler { common: MgsCommon, + host_flash_update: HostFlashUpdate, usart: UsartHandler, attached_serial_console_mgs: Option<(SocketAddrV6, SpPort)>, serial_console_write_offset: u64, @@ -51,12 +60,37 @@ impl MgsHandler { let usart = UsartHandler::claim_static_resources(); Self { common: MgsCommon::claim_static_resources(), + host_flash_update: HostFlashUpdate::claim_static_resources(), usart, attached_serial_console_mgs: None, serial_console_write_offset: 0, } } + /// If we want to be woken by the system timer, we return a deadline here. + /// `main()` is responsible for calling this method and actually setting the + /// timer. + pub(crate) fn timer_deadline(&self) -> Option { + // If we're trying to prep for a host flash update, we have sectors that + // need to be erased, but we break that work up across multiple steps to + // avoid blocking while the entire erase happens. If we're in that case, + // set our timer for 1 tick from now to give a window for other + // interrupts/notifications to arrive. + if self.host_flash_update.needs_sectors_erased() { + Some(sys_get_timer().now + 1) + } else { + self.usart.from_rx_flush_deadline + } + } + + pub(crate) fn handle_timer_fired(&mut self) { + self.host_flash_update.erase_sectors_if_needed(); + // Even though `timer_deadline()` can return a timer related to usart + // flushing, we don't need to do anything here; `NetHandler` in main.rs + // will call `wants_to_send_packet_to_mgs()` below when it's ready to + // grab any data we want to flush. + } + pub(crate) fn drive_usart(&mut self) { self.usart.run_until_blocked(); } @@ -177,13 +211,46 @@ impl SpHandler for MgsHandler { self.common.sp_state() } - fn update_start( + fn update_prepare( &mut self, _sender: SocketAddrV6, _port: SpPort, - update: UpdateStart, + update: UpdatePrepare, ) -> Result<(), ResponseError> { - self.common.update_start(update) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepare { + length: update.total_size, + component: update.component, + stream_id: update.stream_id, + slot: update.slot, + })); + + match update.component { + SpComponent::SP_ITSELF => self.common.update_prepare(update), + SpComponent::SP3_HOST_CPU => self.host_flash_update.prepare(update), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_prepare_status( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + request: UpdatePrepareStatusRequest, + ) -> Result { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepareStatus { + component: request.component, + stream_id: request.stream_id, + })); + + match request.component { + SpComponent::SP_ITSELF => { + self.common.update_prepare_status(request) + } + SpComponent::SP3_HOST_CPU => { + self.host_flash_update.prepare_status(request) + } + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn update_chunk( @@ -193,7 +260,35 @@ impl SpHandler for MgsHandler { chunk: UpdateChunk, data: &[u8], ) -> Result<(), ResponseError> { - self.common.update_chunk(chunk, data) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateChunk { + component: chunk.component, + offset: chunk.offset, + })); + + match chunk.component { + SpComponent::SP_ITSELF => self.common.update_chunk(chunk, data), + SpComponent::SP3_HOST_CPU => { + self.host_flash_update.ingest_chunk(chunk, data) + } + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_abort( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + component: SpComponent, + ) -> Result<(), ResponseError> { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateAbort { + component + })); + + match component { + SpComponent::SP_ITSELF => self.common.update_abort(), + SpComponent::SP3_HOST_CPU => self.host_flash_update.abort(), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn serial_console_attach( @@ -351,23 +446,26 @@ impl UsartHandler { fn clear_rx_data(&mut self) { self.from_rx.clear(); self.from_rx_flush_deadline = None; - sys_set_timer(None, TIMER_IRQ); } fn drain_flushed_data(&mut self, n: usize) { self.from_rx.drain_front(n); self.from_rx_offset += n as u64; self.from_rx_flush_deadline = None; - self.start_flush_timer_if_needed(); + if !self.from_rx.is_empty() { + self.set_from_rx_flush_deadline(); + } } - fn start_flush_timer_if_needed(&mut self) { - if self.from_rx_flush_deadline.is_none() && !self.from_rx.is_empty() { - let deadline = - sys_get_timer().now + SERIAL_CONSOLE_FLUSH_TIMEOUT_MILLIS; - self.from_rx_flush_deadline = Some(deadline); - sys_set_timer(Some(deadline), TIMER_IRQ); - } + /// Panics if `self.from_rx_deadline.is_some()` or if + /// `self.from_rx.is_empty()`; callers are responsible for checking or + /// ensuring both. + fn set_from_rx_flush_deadline(&mut self) { + assert!(self.from_rx_flush_deadline.is_none()); + assert!(!self.from_rx.is_empty()); + let deadline = + sys_get_timer().now + SERIAL_CONSOLE_FLUSH_TIMEOUT_MILLIS; + self.from_rx_flush_deadline = Some(deadline); } fn run_until_blocked(&mut self) { @@ -440,7 +538,9 @@ impl UsartHandler { ringbuf_entry_root!(Log::UsartRx { num_bytes: n_received }); - self.start_flush_timer_if_needed(); + if self.from_rx_flush_deadline.is_none() { + self.set_from_rx_flush_deadline(); + } } // Re-enable USART interrupts. @@ -543,3 +643,225 @@ fn claim_sp_to_mgs_usart_buf_static( // other reference in the program. unsafe { &mut UART_RX_BUF } } + +struct HostFlashUpdate { + task: HostFlash, + buf: UpdateBuffer, + sector_erase: HostFlashSectorErase, +} + +impl HostFlashUpdate { + fn claim_static_resources() -> Self { + let buf = claim_hf_update_buffer_static(); + Self { + task: HostFlash::from(HOST_FLASH.get_task_id()), + buf: UpdateBuffer::new( + buf, + |hf_task, block_index, data| { + let address = (block_index * PAGE_SIZE_BYTES) as u32; + hf_task + .page_program(address, data) + .map_err(|err| ResponseError::UpdateFailed(err as u32)) + }, + |_hf_task| { + // nothing to do to finalize? + // TODO should we set_dev() back to what it was (if we + // changed it)? + Ok(()) + }, + ), + sector_erase: HostFlashSectorErase::default(), + } + } + + fn needs_sectors_erased(&self) -> bool { + self.sector_erase.needs_sectors_erased() + } + + fn erase_sectors_if_needed(&mut self) { + self.sector_erase.erase_sectors_if_needed(&self.task); + } + + fn prepare(&mut self, update: UpdatePrepare) -> Result<(), ResponseError> { + // Which slot are we updating? + let slot = match update.slot { + 0 => HfDevSelect::Flash0, + 1 => HfDevSelect::Flash1, + _ => return Err(ResponseError::InvalidSlotForComponent), + }; + + // Do we have control of the host flash? + match self + .task + .get_mux() + .map_err(|err| ResponseError::UpdateFailed(err as u32))? + { + HfMuxState::SP => (), + HfMuxState::HostCPU => return Err(ResponseError::UpdateSlotBusy), + } + + // Is an update already in progress? + self.buf.ensure_no_update_in_progress()?; + + // Swap to the chosen slot. + self.task + .set_dev(slot) + .map_err(|err| ResponseError::UpdateFailed(err as u32))?; + + // What is the total capacity of the device? + let capacity = self + .task + .capacity() + .map_err(|err| ResponseError::UpdateFailed(err as u32))?; + + // How many total sectors do we need to erase? For gimlet, we know that + // capacity is an exact multiple of the sector size, which is probably + // a safe assumption for future parts as well. We'll assert here in case + // that ever becomes untrue, and we can update our math. + assert!(capacity % SECTOR_SIZE_BYTES == 0); + self.sector_erase.start(capacity / SECTOR_SIZE_BYTES); + + self.buf.start(update.stream_id, update.total_size as usize); + + Ok(()) + } + + fn prepare_status( + &self, + request: UpdatePrepareStatusRequest, + ) -> Result { + self.buf.ensure_matching_stream_id(request.stream_id)?; + + // Have we failed erasing sectors? + if let Some(err) = self.sector_erase.most_recent_error() { + return Err(ResponseError::UpdateFailed(err as u32)); + } + + // We have an update in progress that matches request.stream_id; do we + // still have sectors to erase? + Ok(UpdatePrepareStatusResponse { + done: !self.needs_sectors_erased(), + }) + } + + fn ingest_chunk( + &mut self, + chunk: UpdateChunk, + data: &[u8], + ) -> Result<(), ResponseError> { + // Have we finished erasing the host flash? + if self.needs_sectors_erased() { + return Err(ResponseError::UpdateNotPrepared); + } + + match self.buf.ingest_chunk( + chunk.stream_id, + &self.task, + chunk.offset, + data, + )? { + UpdateProgress::Complete => { + // Update complete; we can now accept a new update. + self.buf.reset(); + } + UpdateProgress::Incomplete => (), + } + Ok(()) + } + + fn abort(&mut self) -> Result<(), ResponseError> { + // TODO should we erase the slot? + // TODO should we set_dev() back to what it was (if we changed it)? + self.buf.reset(); + self.sector_erase.abort(); + Ok(()) + } +} + +struct HostFlashSectorErase { + sectors_to_erase: Range, + most_recent_error: Option, +} + +impl Default for HostFlashSectorErase { + fn default() -> Self { + Self { + sectors_to_erase: 0..0, + most_recent_error: None, + } + } +} + +impl HostFlashSectorErase { + fn start(&mut self, num_sectors: usize) { + self.sectors_to_erase = 0..num_sectors; + self.most_recent_error = None; + } + + fn abort(&mut self) { + self.sectors_to_erase = 0..0; + self.most_recent_error = None; + } + + fn needs_sectors_erased(&self) -> bool { + !self.sectors_to_erase.is_empty() + } + + fn most_recent_error(&self) -> Option { + self.most_recent_error + } + + fn erase_sectors_if_needed(&mut self, task: &HostFlash) { + // While we're erasing sectors, we're not able to service other + // interrupts (e.g., incoming requests from MGS). We therefore limit how + // many sectors we're willing to erase in one call to this function, and + // it's our callers responsibility to continue to call us until we're + // done. + // + // Empirically, erasing 8 sectors can take up to a second, and raising + // it higher does not significantly improve our throughput. + const MAX_SECTORS_TO_ERASE_ONE_CALL: usize = 8; + + if !self.needs_sectors_erased() { + return; + } + + let num_sectors = usize::min( + MAX_SECTORS_TO_ERASE_ONE_CALL, + self.sectors_to_erase.end - self.sectors_to_erase.start, + ); + for i in 0..num_sectors { + let sector = self.sectors_to_erase.start + i; + let addr = sector * SECTOR_SIZE_BYTES; + match task.sector_erase(addr as u32) { + Ok(()) => (), + Err(err) => { + self.sectors_to_erase.start += i; + self.most_recent_error = Some(err); + return; + } + } + } + + self.sectors_to_erase.start += num_sectors; + self.most_recent_error = None; + ringbuf_entry_root!(Log::HostFlashSectorsErased { num_sectors }); + } +} + +fn claim_hf_update_buffer_static( +) -> &'static mut heapless::Vec { + static mut HF_UPDATE_BUF: heapless::Vec = + heapless::Vec::new(); + + static TAKEN: AtomicBool = AtomicBool::new(false); + if TAKEN.swap(true, Ordering::Relaxed) { + panic!() + } + + // Safety: unsafe because of references to mutable statics; safe because of + // the AtomicBool swap above, combined with the lexical scoping of + // `HF_UPDATE_BUF`, means that this reference can't be aliased by any + // other reference in the program. + unsafe { &mut HF_UPDATE_BUF } +} diff --git a/task/mgmt-gateway/src/mgs_psc.rs b/task/mgmt-gateway/src/mgs_psc.rs index fe1330f8c6..2b45b01187 100644 --- a/task/mgmt-gateway/src/mgs_psc.rs +++ b/task/mgmt-gateway/src/mgs_psc.rs @@ -8,7 +8,8 @@ use crate::{mgs_common::MgsCommon, Log, MgsMessage}; use gateway_messages::{ sp_impl::SocketAddrV6, sp_impl::SpHandler, BulkIgnitionState, DiscoverResponse, IgnitionCommand, IgnitionState, ResponseError, - SpComponent, SpPort, SpState, UpdateChunk, UpdateStart, + SpComponent, SpPort, SpState, UpdateChunk, UpdatePrepare, + UpdatePrepareStatusRequest, UpdatePrepareStatusResponse, }; use ringbuf::ringbuf_entry_root; use task_net_api::UdpMetadata; @@ -26,6 +27,15 @@ impl MgsHandler { } } + /// If we want to be woken by the system timer, we return a deadline here. + /// `main()` is responsible for calling this method and actually setting the + /// timer. + pub(crate) fn timer_deadline(&self) -> Option { + None + } + + pub(crate) fn handle_timer_fired(&mut self) {} + pub(crate) fn drive_usart(&mut self) {} pub(crate) fn wants_to_send_packet_to_mgs(&mut self) -> bool { @@ -92,13 +102,42 @@ impl SpHandler for MgsHandler { self.common.sp_state() } - fn update_start( + fn update_prepare( &mut self, _sender: SocketAddrV6, _port: SpPort, - update: UpdateStart, + update: UpdatePrepare, ) -> Result<(), ResponseError> { - self.common.update_start(update) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepare { + length: update.total_size, + component: update.component, + stream_id: update.stream_id, + slot: update.slot, + })); + + match update.component { + SpComponent::SP_ITSELF => self.common.update_prepare(update), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_prepare_status( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + request: UpdatePrepareStatusRequest, + ) -> Result { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepareStatus { + component: request.component, + stream_id: request.stream_id, + })); + + match request.component { + SpComponent::SP_ITSELF => { + self.common.update_prepare_status(request) + } + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn update_chunk( @@ -108,7 +147,31 @@ impl SpHandler for MgsHandler { chunk: UpdateChunk, data: &[u8], ) -> Result<(), ResponseError> { - self.common.update_chunk(chunk, data) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateChunk { + component: chunk.component, + offset: chunk.offset, + })); + + match chunk.component { + SpComponent::SP_ITSELF => self.common.update_chunk(chunk, data), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_abort( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + component: SpComponent, + ) -> Result<(), ResponseError> { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateAbort { + component + })); + + match component { + SpComponent::SP_ITSELF => self.common.update_abort(), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn serial_console_attach( diff --git a/task/mgmt-gateway/src/mgs_sidecar.rs b/task/mgmt-gateway/src/mgs_sidecar.rs index fe1330f8c6..2b45b01187 100644 --- a/task/mgmt-gateway/src/mgs_sidecar.rs +++ b/task/mgmt-gateway/src/mgs_sidecar.rs @@ -8,7 +8,8 @@ use crate::{mgs_common::MgsCommon, Log, MgsMessage}; use gateway_messages::{ sp_impl::SocketAddrV6, sp_impl::SpHandler, BulkIgnitionState, DiscoverResponse, IgnitionCommand, IgnitionState, ResponseError, - SpComponent, SpPort, SpState, UpdateChunk, UpdateStart, + SpComponent, SpPort, SpState, UpdateChunk, UpdatePrepare, + UpdatePrepareStatusRequest, UpdatePrepareStatusResponse, }; use ringbuf::ringbuf_entry_root; use task_net_api::UdpMetadata; @@ -26,6 +27,15 @@ impl MgsHandler { } } + /// If we want to be woken by the system timer, we return a deadline here. + /// `main()` is responsible for calling this method and actually setting the + /// timer. + pub(crate) fn timer_deadline(&self) -> Option { + None + } + + pub(crate) fn handle_timer_fired(&mut self) {} + pub(crate) fn drive_usart(&mut self) {} pub(crate) fn wants_to_send_packet_to_mgs(&mut self) -> bool { @@ -92,13 +102,42 @@ impl SpHandler for MgsHandler { self.common.sp_state() } - fn update_start( + fn update_prepare( &mut self, _sender: SocketAddrV6, _port: SpPort, - update: UpdateStart, + update: UpdatePrepare, ) -> Result<(), ResponseError> { - self.common.update_start(update) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepare { + length: update.total_size, + component: update.component, + stream_id: update.stream_id, + slot: update.slot, + })); + + match update.component { + SpComponent::SP_ITSELF => self.common.update_prepare(update), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_prepare_status( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + request: UpdatePrepareStatusRequest, + ) -> Result { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdatePrepareStatus { + component: request.component, + stream_id: request.stream_id, + })); + + match request.component { + SpComponent::SP_ITSELF => { + self.common.update_prepare_status(request) + } + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn update_chunk( @@ -108,7 +147,31 @@ impl SpHandler for MgsHandler { chunk: UpdateChunk, data: &[u8], ) -> Result<(), ResponseError> { - self.common.update_chunk(chunk, data) + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateChunk { + component: chunk.component, + offset: chunk.offset, + })); + + match chunk.component { + SpComponent::SP_ITSELF => self.common.update_chunk(chunk, data), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } + } + + fn update_abort( + &mut self, + _sender: SocketAddrV6, + _port: SpPort, + component: SpComponent, + ) -> Result<(), ResponseError> { + ringbuf_entry_root!(Log::MgsMessage(MgsMessage::UpdateAbort { + component + })); + + match component { + SpComponent::SP_ITSELF => self.common.update_abort(), + _ => Err(ResponseError::RequestUnsupportedForComponent), + } } fn serial_console_attach( diff --git a/task/mgmt-gateway/src/update_buffer.rs b/task/mgmt-gateway/src/update_buffer.rs index 5c1c0ce8bb..08db136ee1 100644 --- a/task/mgmt-gateway/src/update_buffer.rs +++ b/task/mgmt-gateway/src/update_buffer.rs @@ -18,6 +18,11 @@ pub type WriteBlockFn = fn( /// have been successfully written. pub type FinalizeFn = fn(user_data: &T) -> Result<(), ResponseError>; +pub enum UpdateProgress { + Complete, + Incomplete, +} + /// `UpdateBuffer` provides common logic for apply updates over the management /// network, assuming a common pattern of: /// @@ -28,10 +33,10 @@ pub type FinalizeFn = fn(user_data: &T) -> Result<(), ResponseError>; /// short block). /// 4. A function to call once all blocks have been written. pub struct UpdateBuffer { + current_update_stream_id: Option, total_length: usize, bytes_written: usize, current_block: &'static mut heapless::Vec, - update_in_progress: bool, write_block_fn: WriteBlockFn, finalize_fn: FinalizeFn, } @@ -43,17 +48,17 @@ impl UpdateBuffer { finalize_fn: FinalizeFn, ) -> Self { Self { + current_update_stream_id: None, total_length: 0, bytes_written: 0, current_block: buf, - update_in_progress: false, write_block_fn, finalize_fn, } } pub fn ensure_no_update_in_progress(&self) -> Result<(), ResponseError> { - if self.update_in_progress { + if self.current_update_stream_id.is_some() { Err(ResponseError::UpdateInProgress { bytes_received: self.bytes_written as u32, }) @@ -62,29 +67,53 @@ impl UpdateBuffer { } } + pub fn ensure_matching_stream_id( + &self, + stream_id: u64, + ) -> Result<(), ResponseError> { + match self.current_update_stream_id { + None => Err(ResponseError::UpdateNotPrepared), + Some(s) => { + if s == stream_id { + Ok(()) + } else { + Err(ResponseError::InvalidUpdateStreamId { + sp_stream_id: s, + }) + } + } + } + } + /// Panics if an update is in progress; use /// [`ensure_no_update_in_progress()`] first. - pub fn start(&mut self, total_length: usize) { - if self.update_in_progress { + pub fn start(&mut self, stream_id: u64, total_length: usize) { + if self.current_update_stream_id.is_some() { panic!(); } - self.update_in_progress = true; + self.current_update_stream_id = Some(stream_id); self.total_length = total_length; self.bytes_written = 0; self.current_block.clear(); } + pub fn reset(&mut self) { + self.current_update_stream_id = None; + self.total_length = 0; + self.bytes_written = 0; + self.current_block.clear(); + } + pub fn ingest_chunk( &mut self, + stream_id: u64, user_data: &T, offset: u32, mut data: &[u8], - ) -> Result<(), ResponseError> { - // Reject chunks if we don't have an update in progress. - if !self.update_in_progress { - return Err(ResponseError::InvalidUpdateChunk); - } + ) -> Result { + // Reject chunks that don't match our current stream. + self.ensure_matching_stream_id(stream_id)?; // Reject chunks that don't match our current progress. if offset as usize != self.bytes_written { @@ -141,12 +170,12 @@ impl UpdateBuffer { if self.bytes_written == self.total_length { (self.finalize_fn)(user_data)?; ringbuf_entry_root!(Log::UpdateComplete); + Ok(UpdateProgress::Complete) } else { ringbuf_entry_root!(Log::UpdatePartial { bytes_written: self.bytes_written }); + Ok(UpdateProgress::Incomplete) } - - Ok(()) } } From 5959e7a1260c9a91280cd8544b690fc98c052f28 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 14 Sep 2022 15:26:48 -0400 Subject: [PATCH 2/2] Replace empty/default HostFlashSectorErase with `Option<_>`. --- task/mgmt-gateway/src/mgs_gimlet.rs | 50 ++++++++++++++--------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/task/mgmt-gateway/src/mgs_gimlet.rs b/task/mgmt-gateway/src/mgs_gimlet.rs index a529182a58..9b1692794a 100644 --- a/task/mgmt-gateway/src/mgs_gimlet.rs +++ b/task/mgmt-gateway/src/mgs_gimlet.rs @@ -647,7 +647,7 @@ fn claim_sp_to_mgs_usart_buf_static( struct HostFlashUpdate { task: HostFlash, buf: UpdateBuffer, - sector_erase: HostFlashSectorErase, + sector_erase: Option, } impl HostFlashUpdate { @@ -670,16 +670,21 @@ impl HostFlashUpdate { Ok(()) }, ), - sector_erase: HostFlashSectorErase::default(), + sector_erase: None, } } fn needs_sectors_erased(&self) -> bool { - self.sector_erase.needs_sectors_erased() + self.sector_erase.is_some() } fn erase_sectors_if_needed(&mut self) { - self.sector_erase.erase_sectors_if_needed(&self.task); + if let Some(sector_erase) = self.sector_erase.as_mut() { + sector_erase.erase_sectors_if_needed(&self.task); + if sector_erase.is_done() { + self.sector_erase = None; + } + } } fn prepare(&mut self, update: UpdatePrepare) -> Result<(), ResponseError> { @@ -719,7 +724,8 @@ impl HostFlashUpdate { // a safe assumption for future parts as well. We'll assert here in case // that ever becomes untrue, and we can update our math. assert!(capacity % SECTOR_SIZE_BYTES == 0); - self.sector_erase.start(capacity / SECTOR_SIZE_BYTES); + self.sector_erase = + Some(HostFlashSectorErase::new(capacity / SECTOR_SIZE_BYTES)); self.buf.start(update.stream_id, update.total_size as usize); @@ -733,14 +739,18 @@ impl HostFlashUpdate { self.buf.ensure_matching_stream_id(request.stream_id)?; // Have we failed erasing sectors? - if let Some(err) = self.sector_erase.most_recent_error() { + if let Some(err) = self + .sector_erase + .as_ref() + .and_then(HostFlashSectorErase::most_recent_error) + { return Err(ResponseError::UpdateFailed(err as u32)); } // We have an update in progress that matches request.stream_id; do we // still have sectors to erase? Ok(UpdatePrepareStatusResponse { - done: !self.needs_sectors_erased(), + done: self.sector_erase.is_none(), }) } @@ -773,7 +783,7 @@ impl HostFlashUpdate { // TODO should we erase the slot? // TODO should we set_dev() back to what it was (if we changed it)? self.buf.reset(); - self.sector_erase.abort(); + self.sector_erase = None; Ok(()) } } @@ -783,28 +793,16 @@ struct HostFlashSectorErase { most_recent_error: Option, } -impl Default for HostFlashSectorErase { - fn default() -> Self { +impl HostFlashSectorErase { + fn new(num_sectors: usize) -> Self { Self { - sectors_to_erase: 0..0, + sectors_to_erase: 0..num_sectors, most_recent_error: None, } } -} -impl HostFlashSectorErase { - fn start(&mut self, num_sectors: usize) { - self.sectors_to_erase = 0..num_sectors; - self.most_recent_error = None; - } - - fn abort(&mut self) { - self.sectors_to_erase = 0..0; - self.most_recent_error = None; - } - - fn needs_sectors_erased(&self) -> bool { - !self.sectors_to_erase.is_empty() + fn is_done(&self) -> bool { + self.sectors_to_erase.is_empty() } fn most_recent_error(&self) -> Option { @@ -822,7 +820,7 @@ impl HostFlashSectorErase { // it higher does not significantly improve our throughput. const MAX_SECTORS_TO_ERASE_ONE_CALL: usize = 8; - if !self.needs_sectors_erased() { + if self.is_done() { return; }