From d31e578b924d283f180c5ece515a7cb4c48c7cfa Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 24 Aug 2025 16:54:41 +0300 Subject: [PATCH 1/5] perf(auxflash-server): Remove panic sites --- drv/auxflash-api/src/lib.rs | 21 +++-- drv/auxflash-server/src/main.rs | 160 +++++++++++++++++++++++++------- 2 files changed, 136 insertions(+), 45 deletions(-) diff --git a/drv/auxflash-api/src/lib.rs b/drv/auxflash-api/src/lib.rs index b25762e243..317496fd46 100644 --- a/drv/auxflash-api/src/lib.rs +++ b/drv/auxflash-api/src/lib.rs @@ -158,18 +158,19 @@ where let mut inner_reader = outer_chunk.read_as_chunks(); while let Ok(Some(inner_chunk)) = inner_reader.next() { if inner_chunk.header().tag == tag { - // At this point, the inner reader is positioned *after* - // our target chunk. We back off by the full length of - // the chunk (including the header), then offset by the - // header size to get to the beginning of the blob data. - let (_, inner_offset, _) = inner_reader.into_inner(); - let pos = inner_offset - - inner_chunk.header().total_len_in_bytes() as u64 - + core::mem::size_of::() as u64; + let pos = u32::try_from(inner_chunk.body_position()) + .map_err(|_| { + AuxFlashError::TlvcReaderBeginFailed + })?; return Ok(AuxFlashBlob { slot, - start: pos as u32, - end: (pos + inner_chunk.len()) as u32, + start: pos, + // SAFETY: chunk length is encoded in a U32 so len + // is always a valid u32, and the body_position + + // body_length are checked to be valid. + end: unsafe { + pos.unchecked_add(inner_chunk.len() as u32) + }, }); } } diff --git a/drv/auxflash-server/src/main.rs b/drv/auxflash-server/src/main.rs index 5fa4a11218..7f5154f97b 100644 --- a/drv/auxflash-server/src/main.rs +++ b/drv/auxflash-server/src/main.rs @@ -5,6 +5,8 @@ #![no_std] #![no_main] +use core::hint::assert_unchecked; + use drv_auxflash_api::{ AuxFlashBlob, AuxFlashChecksum, AuxFlashError, AuxFlashId, TlvcReadAuxFlash, PAGE_SIZE_BYTES, SECTOR_SIZE_BYTES, SLOT_COUNT, @@ -14,7 +16,7 @@ use idol_runtime::{ ClientError, Leased, NotificationHandler, RequestError, R, W, }; use tlvc::{TlvcRead, TlvcReadError, TlvcReader}; -use userlib::{hl, task_slot, RecvMessage, UnwrapLite}; +use userlib::{hl, task_slot, RecvMessage}; #[cfg(feature = "h753")] use stm32h7::stm32h753 as device; @@ -45,7 +47,10 @@ impl<'a> TlvcRead for SlotReader<'a> { offset: u64, dest: &mut [u8], ) -> Result<(), TlvcReadError> { - let addr: u32 = self.base + u32::try_from(offset).unwrap_lite(); + let addr = u32::try_from(offset) + .ok() + .and_then(|offset| self.base.checked_add(offset)) + .ok_or(TlvcReadError::User(AuxFlashError::AddressOverflow))?; self.qspi .read_memory(addr, dest) .map_err(|x| TlvcReadError::User(qspi_to_auxflash(x)))?; @@ -87,8 +92,14 @@ fn main() -> ! { 5 // 200MHz kernel / 5 = 40MHz clock }; const MEMORY_SIZE: usize = SLOT_COUNT as usize * SLOT_SIZE; - assert!(MEMORY_SIZE.is_power_of_two()); - let memory_size_log2 = MEMORY_SIZE.trailing_zeros().try_into().unwrap(); + let memory_size_log2 = const { + assert!(MEMORY_SIZE.is_power_of_two()); + let memory_size_log2 = MEMORY_SIZE.trailing_zeros(); + if memory_size_log2 > u8::MAX as u32 { + panic!(); + } + memory_size_log2 as u8 + }; qspi.configure(clock, memory_size_log2); // This driver is compatible with Sidecar, Cosmo, and Grapefruit; Gimlet @@ -202,27 +213,48 @@ impl ServerImpl { self.active_slot.ok_or(AuxFlashError::NoActiveSlot)?; let spare_slot = active_slot ^ 1; + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } + // SAFETY: active_slot and spare_slot are valid slot numbers. The + // active slot is chosen using scan_for_active_slot and never + // reassigned, while spare_slot is its dual. Slots are always created + // in pairs as per above check, so flipping the 0th bit keeps slot + // validity. + unsafe { + assert_unchecked(active_slot < SLOT_COUNT); + assert_unchecked(spare_slot < SLOT_COUNT); + } + // SAFETY: spare_slot is a valid slot number, as slots are created in + // pairs. let spare_checksum = self.read_slot_checksum(spare_slot); if spare_checksum.map(|c| c.0) == Ok(AUXI_CHECKSUM) { return Ok(()); } + let active_slot_base = active_slot * SLOT_SIZE as u32; // Find the length of data by finding the final TLV-C slot let handle = SlotReader { qspi: &self.qspi, - base: active_slot * SLOT_SIZE as u32, + base: active_slot_base, }; let mut reader = TlvcReader::begin(handle) .map_err(|_| AuxFlashError::TlvcReaderBeginFailed)?; while let Ok(Some(..)) = reader.next() { // Nothing to do here } - let data_size = SLOT_SIZE - reader.remaining() as usize; + // SAFETY: this cannot wrap, as our reader's max size is SLOT_SIZE. + let data_size = + unsafe { SLOT_SIZE.unchecked_sub(reader.remaining() as usize) }; let mut buf = [0u8; PAGE_SIZE_BYTES]; - let mut read_addr = active_slot as usize * SLOT_SIZE; + let mut read_addr = active_slot_base as usize; + // Note: this cannot overflow as spare slot was checked above. let mut write_addr = spare_slot as usize * SLOT_SIZE; - let read_end = read_addr + data_size; + // SAFETY: this cannot overflow as data_size is less or equal to + // SLOT_SIZE and active_slot is valid for reads up to SLOT_SIZE. + let read_end = unsafe { read_addr.unchecked_add(data_size) }; while read_addr < read_end { let amount = (read_end - read_addr).min(buf.len()); @@ -248,8 +280,12 @@ impl ServerImpl { .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(None)?; - read_addr += amount; - write_addr += amount; + // SAFETY: these cannot overflow as amount is at most + // 'read_end - read_addr'; we can at most reach read_end here. + unsafe { + read_addr = read_addr.unchecked_add(amount); + write_addr = write_addr.unchecked_add(amount); + } } // Confirm that the spare write worked @@ -322,7 +358,11 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { if slot >= SLOT_COUNT { return Err(AuxFlashError::InvalidSlot.into()); } - let mem_start = slot as usize * SLOT_SIZE; + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } + let mem_start: usize = slot as usize * SLOT_SIZE; let mem_end = mem_start + SLOT_SIZE; if mem_end > u32::MAX as usize { return Err(AuxFlashError::AddressOverflow.into()); @@ -334,8 +374,15 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { self.qspi .sector_erase(addr as u32) .map_err(qspi_to_auxflash)?; - addr += SECTOR_SIZE_BYTES; self.poll_for_write_complete(Some(1))?; + if let Some(next) = addr.checked_add(SECTOR_SIZE_BYTES) { + addr = next; + } else { + // Technically it's possible that SECTOR_SIZE_BYTES > SLOT_SIZE, + // in which case this overflow. In that case we overflow mem_end + // as well and have arrived at the end of the loop. + break; + } } Ok(()) } @@ -348,10 +395,14 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { ) -> Result<(), RequestError> { if slot >= SLOT_COUNT { return Err(AuxFlashError::InvalidSlot.into()); - } - if offset >= SLOT_SIZE as u32 { + } else if offset >= SLOT_SIZE as u32 { return Err(AuxFlashError::AddressOverflow.into()); } + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } + // Note: these cannot overflow as per above checks. let addr = slot as usize * SLOT_SIZE + offset as usize; if addr > u32::MAX as usize { return Err(AuxFlashError::AddressOverflow.into()); @@ -372,36 +423,53 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { offset: u32, data: Leased, ) -> Result<(), RequestError> { - if Some(slot) == self.active_slot { + if slot >= SLOT_COUNT { + return Err(AuxFlashError::InvalidSlot.into()); + } else if Some(slot) == self.active_slot { return Err(AuxFlashError::SlotActive.into()); - } - if !(offset as usize).is_multiple_of(PAGE_SIZE_BYTES) { + } else if !(offset as usize).is_multiple_of(PAGE_SIZE_BYTES) { return Err(AuxFlashError::UnalignedAddress.into()); - } else if offset as usize + data.len() > SLOT_SIZE { + } else if (offset as usize) + .checked_add(data.len()) + .is_none_or(|len| len > SLOT_SIZE) + { return Err(AuxFlashError::AddressOverflow.into()); } - let mem_start = (slot as usize * SLOT_SIZE) + offset as usize; - let mem_end = mem_start + data.len(); + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } + // Note: these cannot overflow as per above checks. + let mem_start = slot as usize * SLOT_SIZE + offset as usize; + let data_len = data.len(); + let mem_end = mem_start + data_len; if mem_end > u32::MAX as usize { return Err(AuxFlashError::AddressOverflow.into()); } // The flash chip has a limited write buffer! - let mut addr = mem_start; let mut buf = [0u8; PAGE_SIZE_BYTES]; - let mut read = 0; - while addr < mem_end { - let amount = (mem_end - addr).min(buf.len()); - data.read_range(read..(read + amount), &mut buf[..amount]) + let mut mem_offset = 0usize; + while mem_offset < data_len { + let amount = (data_len - mem_offset).min(buf.len()); + // SAFETY: amount takes us to data_len in at most buf sized + // chunks. The next offset is always at most data_len. + let next_mem_offset = unsafe { mem_offset.unchecked_add(amount) }; + let buf = &mut buf[..amount]; + data.read_range(mem_offset..next_mem_offset, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; self.set_and_check_write_enable()?; + // SAFETY: mem_offset goes from 0..data_len, while mem_start + + // data_len was checked to not overflow. we're thus always within + // bounds. + let page_addr: usize = + unsafe { mem_start.unchecked_add(mem_offset) }; self.qspi - .page_program(addr as u32, &buf[..amount]) + .page_program(page_addr as u32, buf) .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(None)?; - addr += amount; - read += amount; + mem_offset = next_mem_offset; } Ok(()) } @@ -413,24 +481,41 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { offset: u32, dest: Leased, ) -> Result<(), RequestError> { - if offset as usize + dest.len() > SLOT_SIZE { + if slot >= SLOT_COUNT { + return Err(AuxFlashError::InvalidSlot.into()); + } else if (offset as usize) + .checked_add(dest.len()) + .is_none_or(|len| len > SLOT_SIZE) + { + // Adding offset to destination length would overflow usize or slot + // size. return Err(AuxFlashError::AddressOverflow.into()); } + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } - let mut addr = (slot as usize * SLOT_SIZE) + offset as usize; + let mut addr = slot as usize * SLOT_SIZE + offset as usize; let end = addr + dest.len(); - let mut write = 0; + let mut write = 0usize; let mut buf = [0u8; 256]; while addr < end { let amount = (end - addr).min(buf.len()); + let buf = &mut buf[..amount]; self.qspi - .read_memory(addr as u32, &mut buf[..amount]) + .read_memory(addr as u32, buf) .map_err(qspi_to_auxflash)?; - dest.write_range(write..(write + amount), &buf[..amount]) + // SAFETY: amount takes us from addr to end in at most buf.len() + // sized chunks. write consequently goes from 0..(end - addr) in + // those chunks. + let write_end = unsafe { write.unchecked_add(amount) }; + dest.write_range(write..write_end, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; - write += amount; - addr += amount; + write = write_end; + // SAFETY: see above; addr + amount <= end. + addr = unsafe { addr.unchecked_add(amount) }; } Ok(()) } @@ -468,6 +553,10 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { let active_slot = self .active_slot .ok_or_else(|| RequestError::from(AuxFlashError::NoActiveSlot))?; + // SAFETY: active_slot is a valid slot number chosen using + // scan_for_active_slot and never reassigned. + unsafe { assert_unchecked(active_slot < SLOT_COUNT) }; + const { assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()) }; let handle = SlotReader { qspi: &self.qspi, base: active_slot * SLOT_SIZE as u32, @@ -527,6 +616,7 @@ fn read_and_check_slot_checksum( if slot >= SLOT_COUNT { return Err(AuxFlashError::InvalidSlot); } + const { assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()) }; let handle = SlotReader { qspi, base: slot * SLOT_SIZE as u32, From 32fd4429603de427148962479514c46570330eb2 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Fri, 21 Nov 2025 00:12:34 +0200 Subject: [PATCH 2/5] perf: saturating_sub for ffree --- drv/stm32h7-qspi/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drv/stm32h7-qspi/src/lib.rs b/drv/stm32h7-qspi/src/lib.rs index cedaae654d..748ead8bcc 100644 --- a/drv/stm32h7-qspi/src/lib.rs +++ b/drv/stm32h7-qspi/src/lib.rs @@ -256,7 +256,7 @@ impl Qspi { // How much space is in the FIFO? let fl = usize::from(sr.flevel().bits()); - let ffree = FIFO_SIZE - fl; + let ffree = FIFO_SIZE.saturating_sub(fl); if ffree >= FIFO_THRESH.min(data.len()) { // Calculate the write size. Note that this may be bigger than // the threshold used above above. We'll opportunistically From 2c3150732e6932b733627c00c9e37a645a0ab050 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Fri, 21 Nov 2025 00:38:36 +0200 Subject: [PATCH 3/5] remove unnecessary unsafe --- drv/auxflash-server/src/main.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drv/auxflash-server/src/main.rs b/drv/auxflash-server/src/main.rs index 7f5154f97b..3b91f98178 100644 --- a/drv/auxflash-server/src/main.rs +++ b/drv/auxflash-server/src/main.rs @@ -252,9 +252,7 @@ impl ServerImpl { let mut read_addr = active_slot_base as usize; // Note: this cannot overflow as spare slot was checked above. let mut write_addr = spare_slot as usize * SLOT_SIZE; - // SAFETY: this cannot overflow as data_size is less or equal to - // SLOT_SIZE and active_slot is valid for reads up to SLOT_SIZE. - let read_end = unsafe { read_addr.unchecked_add(data_size) }; + let read_end = read_addr + data_size; while read_addr < read_end { let amount = (read_end - read_addr).min(buf.len()); From 5c91f3f2edb07ad5db232408d09710f70235f832 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 1 Feb 2026 12:51:55 +0200 Subject: [PATCH 4/5] feat: Slot, SlotOffset wrapper types --- drv/auxflash-server/src/main.rs | 269 +++++++++++----------------- drv/auxflash-server/src/slot.rs | 302 ++++++++++++++++++++++++++++++++ 2 files changed, 401 insertions(+), 170 deletions(-) create mode 100644 drv/auxflash-server/src/slot.rs diff --git a/drv/auxflash-server/src/main.rs b/drv/auxflash-server/src/main.rs index 3b91f98178..5c635b8fb2 100644 --- a/drv/auxflash-server/src/main.rs +++ b/drv/auxflash-server/src/main.rs @@ -5,7 +5,9 @@ #![no_std] #![no_main] -use core::hint::assert_unchecked; +mod slot; + +use slot::{Slot, SlotOffset, SlotPageOffset}; use drv_auxflash_api::{ AuxFlashBlob, AuxFlashChecksum, AuxFlashError, AuxFlashId, @@ -32,7 +34,7 @@ task_slot!(SYS, sys); #[derive(Copy, Clone)] struct SlotReader<'a> { qspi: &'a Qspi, - base: u32, + base: Slot, } impl<'a> TlvcRead for SlotReader<'a> { @@ -42,17 +44,17 @@ impl<'a> TlvcRead for SlotReader<'a> { // Hard-coded slot size, on a per-board basis Ok(SLOT_SIZE as u64) } + fn read_exact( &self, offset: u64, dest: &mut [u8], ) -> Result<(), TlvcReadError> { - let addr = u32::try_from(offset) - .ok() - .and_then(|offset| self.base.checked_add(offset)) - .ok_or(TlvcReadError::User(AuxFlashError::AddressOverflow))?; + let offset = + SlotOffset::try_from(offset).map_err(TlvcReadError::User)?; + let addr = self.base.with_offset(offset); self.qspi - .read_memory(addr, dest) + .read_memory(addr as u32, dest) .map_err(|x| TlvcReadError::User(qspi_to_auxflash(x)))?; Ok(()) } @@ -162,7 +164,7 @@ fn main() -> ! { struct ServerImpl { qspi: Qspi, - active_slot: Option, + active_slot: Option, } impl ServerImpl { @@ -200,7 +202,7 @@ impl ServerImpl { fn read_slot_checksum( &self, - slot: u32, + slot: Slot, ) -> Result { read_and_check_slot_checksum(&self.qspi, slot) } @@ -212,61 +214,52 @@ impl ServerImpl { let active_slot = self.active_slot.ok_or(AuxFlashError::NoActiveSlot)?; - let spare_slot = active_slot ^ 1; - const { - assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); - assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); - } - // SAFETY: active_slot and spare_slot are valid slot numbers. The - // active slot is chosen using scan_for_active_slot and never - // reassigned, while spare_slot is its dual. Slots are always created - // in pairs as per above check, so flipping the 0th bit keeps slot - // validity. - unsafe { - assert_unchecked(active_slot < SLOT_COUNT); - assert_unchecked(spare_slot < SLOT_COUNT); - } - // SAFETY: spare_slot is a valid slot number, as slots are created in - // pairs. + let spare_slot = active_slot.get_redundant_slot(); let spare_checksum = self.read_slot_checksum(spare_slot); if spare_checksum.map(|c| c.0) == Ok(AUXI_CHECKSUM) { return Ok(()); } - let active_slot_base = active_slot * SLOT_SIZE as u32; // Find the length of data by finding the final TLV-C slot let handle = SlotReader { qspi: &self.qspi, - base: active_slot_base, + base: active_slot, }; let mut reader = TlvcReader::begin(handle) .map_err(|_| AuxFlashError::TlvcReaderBeginFailed)?; while let Ok(Some(..)) = reader.next() { // Nothing to do here } - // SAFETY: this cannot wrap, as our reader's max size is SLOT_SIZE. - let data_size = - unsafe { SLOT_SIZE.unchecked_sub(reader.remaining() as usize) }; + // SAFETY: Saturating subtract from SLOT_SIZE cannot produce a value + // larger than SLOT_SIZE. + let end_offset = unsafe { + SlotOffset::new_unchecked( + // FIXME: it'd be nice to have a .position() API on reader. + SLOT_SIZE.saturating_sub(reader.remaining() as usize), + ) + }; let mut buf = [0u8; PAGE_SIZE_BYTES]; - let mut read_addr = active_slot_base as usize; - // Note: this cannot overflow as spare slot was checked above. - let mut write_addr = spare_slot as usize * SLOT_SIZE; - let read_end = read_addr + data_size; - while read_addr < read_end { - let amount = (read_end - read_addr).min(buf.len()); - + for (read_range, write_range) in active_slot + .as_chunks::(SlotOffset::ZERO..end_offset) + .zip( + spare_slot + .as_chunks::(SlotOffset::ZERO..end_offset), + ) + { + debug_assert_eq!(read_range.len(), write_range.len()); + let amount = read_range.len(); // Read from the active slot self.qspi - .read_memory(read_addr as u32, &mut buf[..amount]) + .read_memory(read_range.start as u32, &mut buf[..amount]) .map_err(qspi_to_auxflash)?; // If we're at the start of a sector, erase it before we start // writing the copy. - if write_addr.is_multiple_of(SECTOR_SIZE_BYTES) { + if write_range.start.is_multiple_of(SECTOR_SIZE_BYTES) { self.set_and_check_write_enable()?; self.qspi - .sector_erase(write_addr as u32) + .sector_erase(write_range.start as u32) .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(Some(1))?; } @@ -274,16 +267,9 @@ impl ServerImpl { // Write back to the redundant slot self.set_and_check_write_enable()?; self.qspi - .page_program(write_addr as u32, &buf[..amount]) + .page_program(write_range.start as u32, &buf[..amount]) .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(None)?; - - // SAFETY: these cannot overflow as amount is at most - // 'read_end - read_addr'; we can at most reach read_end here. - unsafe { - read_addr = read_addr.unchecked_add(amount); - write_addr = write_addr.unchecked_add(amount); - } } // Confirm that the spare write worked @@ -345,7 +331,7 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { _: &RecvMessage, slot: u32, ) -> Result> { - Ok(self.read_slot_checksum(slot)?) + Ok(self.read_slot_checksum(Slot::try_from(slot)?)?) } fn erase_slot( @@ -353,34 +339,14 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { _: &RecvMessage, slot: u32, ) -> Result<(), RequestError> { - if slot >= SLOT_COUNT { - return Err(AuxFlashError::InvalidSlot.into()); - } - const { - assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); - assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); - } - let mem_start: usize = slot as usize * SLOT_SIZE; - let mem_end = mem_start + SLOT_SIZE; - if mem_end > u32::MAX as usize { - return Err(AuxFlashError::AddressOverflow.into()); - } + let slot = Slot::try_from(slot).map_err(RequestError::from)?; - let mut addr = mem_start; - while addr < mem_end { + for addr in slot.as_sectors() { self.set_and_check_write_enable()?; self.qspi .sector_erase(addr as u32) .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(Some(1))?; - if let Some(next) = addr.checked_add(SECTOR_SIZE_BYTES) { - addr = next; - } else { - // Technically it's possible that SECTOR_SIZE_BYTES > SLOT_SIZE, - // in which case this overflow. In that case we overflow mem_end - // as well and have arrived at the end of the loop. - break; - } } Ok(()) } @@ -391,20 +357,11 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { slot: u32, offset: u32, ) -> Result<(), RequestError> { - if slot >= SLOT_COUNT { - return Err(AuxFlashError::InvalidSlot.into()); - } else if offset >= SLOT_SIZE as u32 { - return Err(AuxFlashError::AddressOverflow.into()); - } - const { - assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); - assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); - } - // Note: these cannot overflow as per above checks. - let addr = slot as usize * SLOT_SIZE + offset as usize; - if addr > u32::MAX as usize { - return Err(AuxFlashError::AddressOverflow.into()); - } + // FIXME: idol does not mention anything about offset having to be a + // multiple of 256 whereas all other offset APIs do; should this require + // and check that as well? + let addr = Slot::address_with_offset(slot, offset) + .map_err(RequestError::from)?; self.set_and_check_write_enable()?; self.qspi @@ -421,53 +378,41 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { offset: u32, data: Leased, ) -> Result<(), RequestError> { - if slot >= SLOT_COUNT { - return Err(AuxFlashError::InvalidSlot.into()); - } else if Some(slot) == self.active_slot { + // FIXME: idol documentation says offset must be a multiple of 256 but + // that is not checked here at all; should it be? Must data.len() also + // be a multiple of 256? + let slot = Slot::try_from(slot).map_err(RequestError::from)?; + if Some(slot) == self.active_slot { return Err(AuxFlashError::SlotActive.into()); - } else if !(offset as usize).is_multiple_of(PAGE_SIZE_BYTES) { - return Err(AuxFlashError::UnalignedAddress.into()); - } else if (offset as usize) - .checked_add(data.len()) - .is_none_or(|len| len > SLOT_SIZE) - { - return Err(AuxFlashError::AddressOverflow.into()); - } - const { - assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); - assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); - } - // Note: these cannot overflow as per above checks. - let mem_start = slot as usize * SLOT_SIZE + offset as usize; - let data_len = data.len(); - let mem_end = mem_start + data_len; - if mem_end > u32::MAX as usize { - return Err(AuxFlashError::AddressOverflow.into()); } + let offset = + SlotPageOffset::try_from(offset).map_err(RequestError::from)?; + let end_offset = offset.add(data.len()).map_err(RequestError::from)?; // The flash chip has a limited write buffer! let mut buf = [0u8; PAGE_SIZE_BYTES]; - let mut mem_offset = 0usize; - while mem_offset < data_len { - let amount = (data_len - mem_offset).min(buf.len()); - // SAFETY: amount takes us to data_len in at most buf sized - // chunks. The next offset is always at most data_len. - let next_mem_offset = unsafe { mem_offset.unchecked_add(amount) }; + // FIXME: we'd prefer to use a .zip iterator here but Leased does not + // provide an Iterator API that we could use. + for chunk in + slot.as_chunks::(SlotOffset::ZERO..end_offset) + { + let amount = chunk.len(); let buf = &mut buf[..amount]; - data.read_range(mem_offset..next_mem_offset, buf) + // SAFETY: chunk goes from slot..slot+end_offset, data range goes + // from 0..end_offset + let data_range = unsafe { + let start = chunk.start.unchecked_sub(slot.memory_start()); + let end = chunk.end.unchecked_sub(slot.memory_start()); + start..end + }; + data.read_range(data_range, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; self.set_and_check_write_enable()?; - // SAFETY: mem_offset goes from 0..data_len, while mem_start + - // data_len was checked to not overflow. we're thus always within - // bounds. - let page_addr: usize = - unsafe { mem_start.unchecked_add(mem_offset) }; self.qspi - .page_program(page_addr as u32, buf) + .page_program(chunk.start as u32, buf) .map_err(qspi_to_auxflash)?; self.poll_for_write_complete(None)?; - mem_offset = next_mem_offset; } Ok(()) } @@ -479,41 +424,34 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { offset: u32, dest: Leased, ) -> Result<(), RequestError> { - if slot >= SLOT_COUNT { - return Err(AuxFlashError::InvalidSlot.into()); - } else if (offset as usize) - .checked_add(dest.len()) - .is_none_or(|len| len > SLOT_SIZE) - { - // Adding offset to destination length would overflow usize or slot - // size. - return Err(AuxFlashError::AddressOverflow.into()); - } - const { - assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); - assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); - } - - let mut addr = slot as usize * SLOT_SIZE + offset as usize; - let end = addr + dest.len(); - - let mut write = 0usize; - let mut buf = [0u8; 256]; - while addr < end { - let amount = (end - addr).min(buf.len()); + // FIXME: idol documentation says offset must be a multiple of 256 but + // that is not checked here at all; should it be? Must dest.len() also + // be a multiple of 256? + let slot = Slot::try_from(slot).map_err(RequestError::from)?; + let start_offset = + SlotOffset::try_from(offset).map_err(RequestError::from)?; + let end_offset = + start_offset.add(dest.len()).map_err(RequestError::from)?; + + const SIZE: usize = 256; + let mut buf = [0u8; SIZE]; + // FIXME: we'd prefer to use a .zip iterator here but Leased does not + // provide an Iterator API that we could use. + for chunk in slot.as_chunks::(start_offset..end_offset) { + let amount = chunk.len(); let buf = &mut buf[..amount]; self.qspi - .read_memory(addr as u32, buf) + .read_memory(chunk.start as u32, buf) .map_err(qspi_to_auxflash)?; - // SAFETY: amount takes us from addr to end in at most buf.len() - // sized chunks. write consequently goes from 0..(end - addr) in - // those chunks. - let write_end = unsafe { write.unchecked_add(amount) }; - dest.write_range(write..write_end, buf) + // SAFETY: chunk goes from slot+start_offset..slot+end_offset, data + // range goes from start_offset..end_offset + let data_range = unsafe { + let start = chunk.start.unchecked_sub(slot.memory_start()); + let end = chunk.end.unchecked_sub(slot.memory_start()); + start..end + }; + dest.write_range(data_range, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; - write = write_end; - // SAFETY: see above; addr + amount <= end. - addr = unsafe { addr.unchecked_add(amount) }; } Ok(()) } @@ -533,6 +471,7 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { _: &RecvMessage, ) -> Result> { self.active_slot + .map(|s| s.value()) .ok_or_else(|| AuxFlashError::NoActiveSlot.into()) } @@ -540,7 +479,7 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { &mut self, _: &RecvMessage, ) -> Result<(), RequestError> { - ServerImpl::ensure_redundancy(self).map_err(Into::into) + ServerImpl::ensure_redundancy(self).map_err(RequestError::from) } fn get_blob_by_tag( @@ -551,16 +490,12 @@ impl idl::InOrderAuxFlashImpl for ServerImpl { let active_slot = self .active_slot .ok_or_else(|| RequestError::from(AuxFlashError::NoActiveSlot))?; - // SAFETY: active_slot is a valid slot number chosen using - // scan_for_active_slot and never reassigned. - unsafe { assert_unchecked(active_slot < SLOT_COUNT) }; - const { assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()) }; let handle = SlotReader { qspi: &self.qspi, - base: active_slot * SLOT_SIZE as u32, + base: active_slot, }; handle - .get_blob_by_tag(active_slot, tag) + .get_blob_by_tag(active_slot.value(), tag) .map_err(RequestError::from) } } @@ -576,11 +511,12 @@ impl NotificationHandler for ServerImpl { } } -fn scan_for_active_slot(qspi: &Qspi) -> Option { +fn scan_for_active_slot(qspi: &Qspi) -> Option { for i in 0..SLOT_COUNT { let handle = SlotReader { qspi, - base: i * SLOT_SIZE as u32, + // SAFETY: i is within SLOT_COUNT + base: unsafe { Slot::new_unchecked(i) }, }; let Ok(chck) = handle.read_stored_checksum() else { @@ -601,7 +537,7 @@ fn scan_for_active_slot(qspi: &Qspi) -> Option { }; if chck == actual { - return Some(i); + return Some(handle.base); } } None @@ -609,16 +545,9 @@ fn scan_for_active_slot(qspi: &Qspi) -> Option { fn read_and_check_slot_checksum( qspi: &Qspi, - slot: u32, + slot: Slot, ) -> Result { - if slot >= SLOT_COUNT { - return Err(AuxFlashError::InvalidSlot); - } - const { assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()) }; - let handle = SlotReader { - qspi, - base: slot * SLOT_SIZE as u32, - }; + let handle = SlotReader { qspi, base: slot }; let claimed = handle.read_stored_checksum()?; let actual = handle.calculate_checksum()?; if claimed == actual { diff --git a/drv/auxflash-server/src/slot.rs b/drv/auxflash-server/src/slot.rs new file mode 100644 index 0000000000..ce76737c1c --- /dev/null +++ b/drv/auxflash-server/src/slot.rs @@ -0,0 +1,302 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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 core::{hint::assert_unchecked, ops::Range}; +use drv_auxflash_api::{ + AuxFlashError, PAGE_SIZE_BYTES, SECTOR_SIZE_BYTES, SLOT_COUNT, SLOT_SIZE, +}; + +/// A verified slot number. +/// +/// Slots always come in pairs, `N` and `N + 1` where `N` is even. The two +/// are used as redundant pairs. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[repr(transparent)] +pub(crate) struct Slot(u32); + +impl Slot { + /// Create a Slot value from a slot index without checking its validity. + /// + /// # Safety + /// + /// The slot index must be less than [`SLOT_COUNT`], or in other words must + /// be a valid slot index on the flash. + /// + /// [`SLOT_COUNT`]: SLOT_COUNT + #[inline] + pub(crate) const unsafe fn new_unchecked(slot: u32) -> Self { + debug_assert!(slot < SLOT_COUNT); + Self(slot) + } + + /// Get the slot index. + #[inline] + pub(crate) const fn value(self) -> u32 { + self.0 + } + + /// Get the slot index as usize. + const fn as_usize(self) -> usize { + self.0 as usize + } + + /// Get the slot start memory address. + #[inline] + pub(crate) const fn memory_start(self) -> usize { + // SAFETY: Slot is checked to never overflow memory. + unsafe { self.as_usize().unchecked_mul(SLOT_SIZE) } + } + + /// Get the slot end memory address. + #[inline] + pub(crate) const fn memory_end(self) -> usize { + // SAFETY: Slot is checked to never overflow memory. + unsafe { + self.as_usize() + .unchecked_mul(SLOT_SIZE) + .unchecked_add(SLOT_SIZE) + } + } + + /// Get the redundant pair of this slot. + #[inline] + pub(crate) const fn get_redundant_slot(self) -> Self { + Self(self.0 ^ 1) + } + + /// Get the memory address of an offset within this slot. + #[inline] + pub(crate) const fn with_offset(self, offset: SlotOffset) -> usize { + // SAFETY: checked slot and offset + unsafe { self.memory_start().unchecked_add(offset.value()) } + } + + /// Get the memory address of an offset within a slot. + #[inline] + pub(crate) fn address_with_offset( + slot: u32, + offset: u32, + ) -> Result { + let slot = Self::try_from(slot)?; + let offset = SlotOffset::try_from(offset)?; + Ok(slot.with_offset(offset)) + } + + /// Get the memory range of this slot. + #[inline] + pub(crate) const fn memory_range(self) -> Range { + let mem_start = self.memory_start(); + let mem_end = self.memory_end(); + debug_assert!((mem_end - mem_start) == SLOT_SIZE); + const { + assert!( + SLOT_SIZE.is_multiple_of(SECTOR_SIZE_BYTES), + "Slot must be a multiple of sectors" + ); + }; + mem_start..mem_end + } + + /// Iterate over the slot memory range in sectors. The iterator returns the + /// start address of each sector. + #[inline] + pub(crate) fn as_sectors(self) -> impl Iterator { + let range = self.memory_range(); + debug_assert!(range.len() == SLOT_SIZE); + const { assert!(SLOT_SIZE.is_multiple_of(SECTOR_SIZE_BYTES)) }; + SlotSectorIterator { + next: range.start, + end: range.end, + } + } + + /// Iterate over a half-open range of offsets in the slot as chunks. The + /// last chunk is smaller than the chunk size if the range is not a multiple + /// of the chunk size. + #[inline] + pub(crate) const fn as_chunks( + self, + range: Range, + ) -> impl Iterator> { + let next = self.with_offset(range.start); + let end = self.with_offset(range.end); + SlotChunkIterator:: { next, end } + } +} + +impl TryFrom for Slot { + type Error = AuxFlashError; + + fn try_from(value: u32) -> Result { + if value >= SLOT_COUNT { + return Err(AuxFlashError::InvalidSlot); + } + const { + assert!(SLOT_COUNT.checked_mul(SLOT_SIZE as u32).is_some()); + assert!(SLOT_COUNT > 0 && SLOT_COUNT.is_multiple_of(2)); + } + Ok(Self(value)) + } +} + +/// Memory offset within a slot. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[repr(transparent)] +pub(crate) struct SlotOffset(usize); + +impl SlotOffset { + /// Zero offset. + pub(crate) const ZERO: Self = Self(0); + + /// Create a new SlotOffset from an offset without checking its value. + /// + /// # Safety + /// + /// The value must be less or equal to SLOT_SIZE. + pub(crate) const unsafe fn new_unchecked(value: usize) -> Self { + Self(value) + } + + pub(crate) const fn value(self) -> usize { + self.0 + } + + pub(crate) fn add(self, value: usize) -> Result { + self.0 + .checked_add(value) + .ok_or(AuxFlashError::AddressOverflow) + .and_then(Self::try_from) + } +} + +impl TryFrom for SlotOffset { + type Error = AuxFlashError; + + fn try_from(value: u32) -> Result { + Self::try_from(value as usize) + } +} + +impl TryFrom for SlotOffset { + type Error = AuxFlashError; + + fn try_from(value: usize) -> Result { + if value > SLOT_SIZE { + return Err(AuxFlashError::AddressOverflow); + } + Ok(Self(value)) + } +} + +impl TryFrom for SlotOffset { + type Error = AuxFlashError; + + fn try_from(value: u64) -> Result { + if value > SLOT_SIZE as u64 { + return Err(AuxFlashError::AddressOverflow); + } + Ok(Self(value as usize)) + } +} + +/// Memory offset within a slot that is guaranteed to be aligned to a page. +#[derive(Debug, Clone, Copy)] +#[repr(transparent)] +pub(crate) struct SlotPageOffset(SlotOffset); + +impl SlotPageOffset { + pub(crate) fn add(self, value: usize) -> Result { + self.0.add(value) + } +} + +impl TryFrom for SlotPageOffset { + type Error = AuxFlashError; + + fn try_from(value: u32) -> Result { + Self::try_from(value as usize) + } +} + +impl TryFrom for SlotPageOffset { + type Error = AuxFlashError; + + fn try_from(value: usize) -> Result { + if !value.is_multiple_of(PAGE_SIZE_BYTES) { + return Err(AuxFlashError::UnalignedAddress.into()); + } + Ok(Self(SlotOffset::try_from(value)?)) + } +} + +/// Iterator over sectors of a slot. +#[derive(Debug)] +pub(crate) struct SlotSectorIterator { + next: usize, + end: usize, +} + +impl Iterator for SlotSectorIterator { + type Item = usize; + + fn next(&mut self) -> Option { + if self.next < self.end { + let value = self.next; + // SAFETY: Slot is an exact multiple of sectors; this cannot + // overflow. + self.next = unsafe { value.unchecked_add(SECTOR_SIZE_BYTES) }; + Some(value) + } else { + None + } + } + + #[inline] + fn size_hint(&self) -> (usize, Option) { + let size = self.end.saturating_sub(self.next); + (size, Some(size)) + } +} + +impl ExactSizeIterator for SlotSectorIterator {} + +// Iterator over chunks in a slot. +#[derive(Debug)] +pub(crate) struct SlotChunkIterator { + next: usize, + end: usize, +} + +impl Iterator for SlotChunkIterator { + type Item = Range; + + fn next(&mut self) -> Option { + if self.next < self.end { + let start = self.next; + let end = start + .checked_add(CHUNK_SIZE) + .map_or(self.end, |end| end.min(self.end)); + self.next = end; + let range = Range::from(start..end); + // SAFETY: by construction. + unsafe { + assert_unchecked(range.len() <= CHUNK_SIZE); + }; + Some(range) + } else { + None + } + } + + #[inline] + fn size_hint(&self) -> (usize, Option) { + let size = self.end.saturating_sub(self.next); + (size, Some(size)) + } +} + +impl ExactSizeIterator + for SlotChunkIterator +{ +} From ee21bd9fec2bd4c13bf031bed11a914206c52d4b Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 1 Feb 2026 13:34:37 +0200 Subject: [PATCH 5/5] =?UTF-8?q?REMOVE=20BEFORE=20MERGE=20=E2=80=93=20depen?= =?UTF-8?q?d=20on=20tlvc=20branch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 32 +++++++++++++++++++++----------- Cargo.toml | 2 +- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab074128a5..95630b7bbe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1036,7 +1036,7 @@ dependencies = [ "num-traits", "serde", "sha3", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", @@ -1055,7 +1055,7 @@ dependencies = [ "idol-runtime", "num-traits", "stm32h7", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", @@ -1069,7 +1069,7 @@ dependencies = [ "derive-idol-err", "hubpack", "num-traits", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", ] @@ -1199,7 +1199,7 @@ dependencies = [ "idol-runtime", "num-traits", "sha3", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", @@ -1985,7 +1985,7 @@ dependencies = [ "drv-i2c-devices", "idol", "ringbuf", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", ] @@ -2319,7 +2319,7 @@ dependencies = [ "serde", "sprockets-common", "static_assertions", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "unwrap-lite", "userlib", "zerocopy 0.8.27", @@ -3403,7 +3403,7 @@ dependencies = [ "path-slash", "rsa", "thiserror", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc)", "tlvc-text", "toml 0.7.2", "x509-cert", @@ -3814,7 +3814,7 @@ dependencies = [ "stage0-handoff", "static_assertions", "task-jefe-api", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", @@ -5819,7 +5819,7 @@ dependencies = [ "task-net-api", "task-packrat-api", "task-sensor-api", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "userlib", "zerocopy 0.8.27", "zerocopy-derive 0.8.27", @@ -6703,6 +6703,16 @@ version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "445e881f4f6d382d5f27c034e25eb92edd7c784ceab92a0937db7f2e9471b938" +[[package]] +name = "tlvc" +version = "0.3.1" +source = "git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead#af9e86405a6a675260c71271cb92388b55808535" +dependencies = [ + "byteorder", + "crc", + "zerocopy 0.6.6", +] + [[package]] name = "tlvc" version = "0.3.1" @@ -6720,7 +6730,7 @@ source = "git+https://github.com/oxidecomputer/tlvc#e644a21a7ca973ed31499106ea92 dependencies = [ "ron", "serde", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc)", "zerocopy 0.6.6", ] @@ -7412,7 +7422,7 @@ dependencies = [ "serde_json", "sha3", "strsim", - "tlvc", + "tlvc 0.3.1 (git+https://github.com/oxidecomputer/tlvc?rev=refs%2Fpull%2F9%2Fhead)", "tlvc-text", "toml 0.9.6", "toml-patch", diff --git a/Cargo.toml b/Cargo.toml index 0a8bf838b9..dd3c861d71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -162,7 +162,7 @@ ordered-toml = { git = "https://github.com/oxidecomputer/ordered-toml", default- pmbus = { git = "https://github.com/oxidecomputer/pmbus", default-features = false } salty = { version = "0.3", default-features = false } spd = { git = "https://github.com/oxidecomputer/spd", default-features = false } -tlvc = { git = "https://github.com/oxidecomputer/tlvc", default-features = false, version = "0.3.1" } +tlvc = { git = "https://github.com/oxidecomputer/tlvc", rev = "refs/pull/9/head", default-features = false, version = "0.3.1" } tlvc-text = { git = "https://github.com/oxidecomputer/tlvc", default-features = false, version = "0.3.0" } transceiver-messages = { git = "https://github.com/oxidecomputer/transceiver-control/", default-features = false } vsc7448-pac = { git = "https://github.com/oxidecomputer/vsc7448", default-features = false }