From 43f5837e93de996a10246427df33dd22c8bba3e2 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 2 Apr 2024 15:58:41 -0700 Subject: [PATCH] stm32xx-sys: statically ensure absence of panics At least in some configurations. --- app/demo-stm32h7-nucleo/app-h743.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- app/gimlet/base.toml | 2 +- app/psc/base.toml | 2 +- app/sidecar/base.toml | 2 +- drv/stm32xx-sys/Cargo.toml | 1 + drv/stm32xx-sys/src/main.rs | 64 ++++++++++++++++++--------- 7 files changed, 49 insertions(+), 26 deletions(-) diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index 438955cb6..c8b9c9c70 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h743", "exti"] +features = ["h743", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index f88658be3..efa28c158 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 0649f968e..df6658690 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -50,7 +50,7 @@ notifications = ["eth-irq", "mdio-timer-irq", "wake-timer", "jefe-state-change"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/psc/base.toml b/app/psc/base.toml index 8679a610e..050c16fd0 100644 --- a/app/psc/base.toml +++ b/app/psc/base.toml @@ -34,7 +34,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index 9fbf42d22..98cb98c8b 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -31,7 +31,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] diff --git a/drv/stm32xx-sys/Cargo.toml b/drv/stm32xx-sys/Cargo.toml index 9ed1aee85..706b2c6fc 100644 --- a/drv/stm32xx-sys/Cargo.toml +++ b/drv/stm32xx-sys/Cargo.toml @@ -37,6 +37,7 @@ g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv- g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"] no-ipc-counters = ["idol/no-counters"] +no-panic = ["userlib/no-panic"] # Enable external interrupt controller support. exti = ["dep:hubris-num-tasks", "dep:counters"] diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 96595fd00..8e7849c9f 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -397,7 +397,7 @@ fn main() -> ! { // API the way we use peripherals. let syscfg = unsafe { &*device::SYSCFG::ptr() }; - for (i, entry) in generated::EXTI_DISPATCH_TABLE.iter().enumerate() { + for (i, entry) in dispatch_table_iter() { // Process entries that are filled in... if let &Some(ExtiDispatch { port, .. }) = entry { let register = i >> 2; @@ -684,8 +684,10 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { let mut slot_mask = 0u16; for (i, _) in exti_dispatch_for(rm.sender, mask) { - // What bit do we touch for this entry? - let bit = 1 << i; + // What bit do we touch for this entry? (Mask is to ensure + // that the compiler understands this shift cannot + // overflow.) + let bit = 1 << (i & 0xF); // Record that these bits meant something. slot_mask |= bit; @@ -774,15 +776,19 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { let mut used_bits = 0u32; for (i, entry) in exti_dispatch_for(rm.sender, mask) { + // (Mask is to ensure that the compiler understands this + // shift cannot overflow.) + let imask = 1 << (i & 0xF); + used_bits |= entry.mask; // Set or clear Rising Trigger Selection // Register bit according to the rising flag self.exti.rtsr1.modify(|r, w| { let new_value = if edge.is_rising() { - r.bits() | (1 << i) + r.bits() | imask } else { - r.bits() & !(1 << i) + r.bits() & !imask }; unsafe { w.bits(new_value) @@ -793,9 +799,9 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // Register bit according to the rising flag self.exti.ftsr1.modify(|r, w| { let new_value = if edge.is_falling() { - r.bits() | (1 << i) + r.bits() | imask } else { - r.bits() & !(1 << i) + r.bits() & !imask }; unsafe { w.bits(new_value) @@ -849,17 +855,18 @@ fn exti_dispatch_for( task: TaskId, mask: u32, ) -> impl Iterator { - generated::EXTI_DISPATCH_TABLE - .iter() - .enumerate() - .filter_map(move |(i, entry)| { - let entry = entry.as_ref()?; - if task.index() == entry.task.index() && mask & entry.mask != 0 { - Some((i, entry)) - } else { - None - } - }) + // This is semantically equivalent to iter.enumerate, but winds up handing + // the compiler very different code that avoids an otherwise-difficult panic + // site on an apparently-overflowing addition (that was not actually capable + // of overflowing). + dispatch_table_iter().filter_map(move |(i, entry)| { + let entry = entry.as_ref()?; + if task.index() == entry.task.index() && mask & entry.mask != 0 { + Some((i, entry)) + } else { + None + } + }) } impl NotificationHandler for ServerImpl<'_> { @@ -898,9 +905,13 @@ impl NotificationHandler for ServerImpl<'_> { let mut bits_to_acknowledge = 0u16; - for (pin_idx, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { + for pin_idx in 0..16 { + // TODO: this sure looks like it should be using + // iter.enumerate, doesn't it? Unfortunately that's not + // currently getting inlined by rustc, resulting in rather + // silly code containing panics. This is significantly + // smaller. + let entry = &generated::EXTI_DISPATCH_TABLE[pin_idx]; if pending_and_enabled & 1 << pin_idx != 0 { // A channel is pending! We need to handle this // basically like the kernel handles native hardware @@ -1214,6 +1225,17 @@ cfg_if! { } } +#[cfg(feature = "exti")] +#[inline(always)] +fn dispatch_table_iter( +) -> impl Iterator)> { + // TODO: this sure looks like it should be using iter.enumerate, doesn't it? + // Unfortunately that's not currently getting inlined by rustc, resulting in + // rather silly code containing panics. This is significantly smaller. + (0..generated::EXTI_DISPATCH_TABLE.len()) + .zip(&generated::EXTI_DISPATCH_TABLE) +} + include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl {