From be9dc0e0b74e1f767e225e98da96d5c16b883ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 9 Oct 2024 12:33:31 +0200 Subject: [PATCH] Fix restoring of CPENABLE (#2315) * Add tests Cp0Disabled issue * Fix saving CPENABLE on context switch * Fix position shift of registers * Clean up --- hil-test/Cargo.toml | 40 +++++- hil-test/build.rs | 4 + hil-test/tests/esp_wifi_floats.rs | 179 ++++++++++++++++++++++++++ xtensa-lx-rt/CHANGELOG.md | 2 + xtensa-lx-rt/src/exception/asm.rs | 24 +++- xtensa-lx-rt/src/exception/context.rs | 15 ++- 6 files changed, 247 insertions(+), 17 deletions(-) create mode 100644 hil-test/tests/esp_wifi_floats.rs diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index 00dde1f3d..4e42f1de9 100644 --- a/hil-test/Cargo.toml +++ b/hil-test/Cargo.toml @@ -175,6 +175,11 @@ required-features = ["embassy"] name = "twai" harness = false +[[test]] +name = "esp_wifi_floats" +harness = false +required-features = ["esp-wifi", "esp-alloc"] + [dependencies] cfg-if = "1.0.0" critical-section = "1.1.3" @@ -191,9 +196,11 @@ esp-alloc = { path = "../esp-alloc", optional = true } esp-backtrace = { path = "../esp-backtrace", default-features = false, features = ["exception-handler", "defmt", "semihosting"] } esp-hal = { path = "../esp-hal", features = ["digest"], optional = true } esp-hal-embassy = { path = "../esp-hal-embassy", optional = true } +esp-wifi = { path = "../esp-wifi", optional = true, features = ["wifi"] } portable-atomic = "1.9.0" static_cell = { version = "2.1.0", features = ["nightly"] } semihosting = { version = "0.1", features= ["stdio", "panic-handler"] } +xtensa-lx-rt = { path = "../xtensa-lx-rt", optional = true } [dev-dependencies] crypto-bigint = { version = "0.5.5", default-features = false } @@ -225,22 +232,45 @@ esp32 = [ "esp-backtrace/esp32", "esp-hal/esp32", "esp-hal-embassy/esp32", + "esp-wifi?/esp32", +] +esp32c2 = [ + "esp-backtrace/esp32c2", + "esp-hal/esp32c2", + "esp-hal-embassy/esp32c2", + "esp-wifi?/esp32c2", +] +esp32c3 = [ + "esp-backtrace/esp32c3", + "esp-hal/esp32c3", + "esp-hal-embassy/esp32c3", + "esp-wifi?/esp32c3", +] +esp32c6 = [ + "esp-backtrace/esp32c6", + "esp-hal/esp32c6", + "esp-hal-embassy/esp32c6", + "esp-wifi?/esp32c6", +] +esp32h2 = [ + "esp-backtrace/esp32h2", + "esp-hal/esp32h2", + "esp-hal-embassy/esp32h2", + "esp-wifi?/esp32h2", ] -esp32c2 = ["esp-backtrace/esp32c2", "esp-hal/esp32c2", "esp-hal-embassy/esp32c2"] -esp32c3 = ["esp-backtrace/esp32c3", "esp-hal/esp32c3", "esp-hal-embassy/esp32c3"] -esp32c6 = ["esp-backtrace/esp32c6", "esp-hal/esp32c6", "esp-hal-embassy/esp32c6"] -esp32h2 = ["esp-backtrace/esp32h2", "esp-hal/esp32h2", "esp-hal-embassy/esp32h2"] esp32s2 = [ "embedded-test/xtensa-semihosting", "esp-backtrace/esp32s2", "esp-hal/esp32s2", "esp-hal-embassy/esp32s2", + "esp-wifi?/esp32s2", ] esp32s3 = [ "embedded-test/xtensa-semihosting", "esp-backtrace/esp32s3", "esp-hal/esp32s3", "esp-hal-embassy/esp32s3", + "esp-wifi?/esp32s3", ] # Async & Embassy: embassy = [ @@ -254,7 +284,7 @@ generic-queue = [ integrated-timers = [ "esp-hal-embassy/integrated-timers", ] -octal-psram = ["esp-hal/octal-psram", "dep:esp-alloc"] +octal-psram = ["esp-hal/octal-psram", "esp-alloc"] # https://doc.rust-lang.org/cargo/reference/profiles.html#test # Test and bench profiles inherit from dev and release respectively. diff --git a/hil-test/build.rs b/hil-test/build.rs index 1c7ecfc2f..35fa4d3a6 100644 --- a/hil-test/build.rs +++ b/hil-test/build.rs @@ -37,5 +37,9 @@ fn main() -> Result<(), Box> { // Define all necessary configuration symbols for the configured device: config.define_symbols(); + if cfg!(feature = "esp-wifi") { + println!("cargo::rustc-link-arg=-Trom_functions.x"); + } + Ok(()) } diff --git a/hil-test/tests/esp_wifi_floats.rs b/hil-test/tests/esp_wifi_floats.rs new file mode 100644 index 000000000..f509f3719 --- /dev/null +++ b/hil-test/tests/esp_wifi_floats.rs @@ -0,0 +1,179 @@ +//! Cp0Disable exception regression test + +//% CHIPS: esp32 esp32s2 esp32s3 +//% FEATURES: esp-wifi esp-alloc +//% FEATURES: esp-wifi esp-alloc xtensa-lx-rt/float-save-restore + +#![no_std] +#![no_main] + +use core::cell::RefCell; + +use critical_section::Mutex; +use esp_hal::{ + delay::Delay, + interrupt::software::{SoftwareInterrupt, SoftwareInterruptControl}, + peripherals::Peripherals, + prelude::*, + rng::Rng, + timer::timg::TimerGroup, +}; +use esp_wifi::{init, EspWifiInitFor}; +use hil_test as _; + +#[inline(never)] +fn run_float_calc(x: f32) -> f32 { + let result = core::hint::black_box(x) * 2.0; + defmt::info!("{}", defmt::Display2Format(&result)); + result +} + +static SWINT0: Mutex>>> = Mutex::new(RefCell::new(None)); + +#[handler] +fn wait() { + // Ensure esp-wifi interrupts this handler. + Delay::new().delay_millis(100); + critical_section::with(|cs| { + SWINT0.borrow_ref(cs).as_ref().unwrap().reset(); + }); +} + +cfg_if::cfg_if! { + if #[cfg(multi_core)] { + use core::sync::atomic::{AtomicBool, Ordering}; + + use esp_hal::cpu_control::CpuControl; + + static DONE: AtomicBool = AtomicBool::new(false); + static mut APP_CORE_STACK: esp_hal::cpu_control::Stack<8192> = + esp_hal::cpu_control::Stack::new(); + } +} + +#[cfg(test)] +#[embedded_test::tests] +mod tests { + use super::*; + + #[init] + fn test_init() -> Peripherals { + esp_alloc::heap_allocator!(72 * 1024); + + esp_hal::init({ + let mut config = esp_hal::Config::default(); + config.cpu_clock = CpuClock::max(); + config + }) + } + + #[test] + fn fpu_is_enabled() { + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + } + + #[cfg(multi_core)] + #[test] + #[timeout(3)] + fn fpu_is_enabled_on_core1(peripherals: Peripherals) { + let mut cpu_control = CpuControl::new(peripherals.CPU_CTRL); + + let _guard = cpu_control + .start_app_core( + unsafe { &mut *core::ptr::addr_of_mut!(APP_CORE_STACK) }, + || { + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + DONE.store(true, Ordering::Relaxed); + loop {} + }, + ) + .unwrap(); + + core::mem::forget(_guard); + + while !DONE.load(Ordering::Relaxed) {} + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + } + + #[test] + #[timeout(3)] + fn fpu_stays_enabled_with_wifi(peripherals: Peripherals) { + let timg0 = TimerGroup::new(peripherals.TIMG0); + let _init = init( + EspWifiInitFor::Wifi, + timg0.timer1, + Rng::new(peripherals.RNG), + peripherals.RADIO_CLK, + ) + .unwrap(); + + let mut sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT); + + critical_section::with(|cs| { + sw_ints.software_interrupt0.set_interrupt_handler(wait); + SWINT0 + .borrow_ref_mut(cs) + .replace(sw_ints.software_interrupt0); + // Fire a low-priority interrupt to ensure the FPU is disabled while + // esp-wifi switches tasks + SWINT0.borrow_ref(cs).as_ref().unwrap().raise(); + }); + + Delay::new().delay_millis(100); + + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + } + + #[cfg(multi_core)] + #[test] + #[timeout(3)] + fn fpu_stays_enabled_with_wifi_on_core1(peripherals: Peripherals) { + let mut cpu_control = CpuControl::new(peripherals.CPU_CTRL); + + let _guard = cpu_control + .start_app_core( + unsafe { &mut *core::ptr::addr_of_mut!(APP_CORE_STACK) }, + move || { + let timg0 = TimerGroup::new(peripherals.TIMG0); + let _init = init( + EspWifiInitFor::Wifi, + timg0.timer1, + Rng::new(peripherals.RNG), + peripherals.RADIO_CLK, + ) + .unwrap(); + + let mut sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT); + + critical_section::with(|cs| { + sw_ints.software_interrupt0.set_interrupt_handler(wait); + SWINT0 + .borrow_ref_mut(cs) + .replace(sw_ints.software_interrupt0); + // Fire a low-priority interrupt to ensure the FPU is disabled while + // esp-wifi switches tasks + SWINT0.borrow_ref(cs).as_ref().unwrap().raise(); + }); + + Delay::new().delay_millis(100); + + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + DONE.store(true, Ordering::Relaxed); + loop {} + }, + ) + .unwrap(); + + core::mem::forget(_guard); + + while !DONE.load(Ordering::Relaxed) {} + + let result = super::run_float_calc(2.0); + assert_eq!(result, 4.0); + } +} diff --git a/xtensa-lx-rt/CHANGELOG.md b/xtensa-lx-rt/CHANGELOG.md index 5d3a29173..7637163bf 100644 --- a/xtensa-lx-rt/CHANGELOG.md +++ b/xtensa-lx-rt/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed saving the state of the FPU co-processor. (#2311) + ### Removed ## 0.17.1 - 2024-09-02 diff --git a/xtensa-lx-rt/src/exception/asm.rs b/xtensa-lx-rt/src/exception/asm.rs index 9b92d4e35..4155d5f7e 100644 --- a/xtensa-lx-rt/src/exception/asm.rs +++ b/xtensa-lx-rt/src/exception/asm.rs @@ -2,13 +2,22 @@ use core::arch::{asm, global_asm}; use crate::cfg_asm; -// we could cfg symbols away and reduce frame size depending on features enabled +// We could cfg symbols away and reduce frame size depending on features enabled // i.e the frame size is a fixed size based on all the features right now // we know at compile time if a target has loops for example, if it doesn't // we could cut that memory usage. // However in order to conveniently use `addmi` we need 256-byte alignment // anyway so wasting a bit more stack space seems to be the better option. +// +// The only exception is XT_STK_F64R_LO_CPENABLE which combines two register +// values depending on `float-save-restore` feature. This is done so that their +// position stays the same in `Context` without creating a large hole in the +// struct. +// // Additionally there is a chunk of memory reserved for spilled registers. +// +// With the exception of XT_STK_TMP, the fields must be aligned with the +// `Context` struct in context.rs global_asm!( " .set XT_STK_PC, 0 @@ -44,7 +53,9 @@ global_asm!( .set XT_STK_M1, 120 .set XT_STK_M2, 124 .set XT_STK_M3, 128 - .set XT_STK_F64R_LO, 132 // Registers for double support option + // if `float-save-restore` is enabled: Registers for double support option. + // Otherwise, the saved value of CPENABLE + .set XT_STK_F64R_LO_CPENABLE, 132 .set XT_STK_F64R_HI, 136 .set XT_STK_F64S, 140 .set XT_STK_FCR, 144 // Registers for floating point coprocessor @@ -66,7 +77,6 @@ global_asm!( .set XT_STK_F14, 208 .set XT_STK_F15, 212 .set XT_STK_TMP, 216 - .set XT_STK_CPENABLE, 220 .set XT_STK_FRMSZ, 256 // needs to be multiple of 16 and enough additional free space // for the registers spilled to the stack (max 8 registers / 0x20 bytes) @@ -128,7 +138,7 @@ unsafe extern "C" fn save_context() { " /* Disable coprocessor, any use of floats in ISRs will cause an exception unless float-save-restore feature is enabled */ rsr a3, CPENABLE - s32i a3, sp, +XT_STK_CPENABLE + s32i a3, sp, +XT_STK_F64R_LO_CPENABLE movi a3, 0 wsr a3, CPENABLE rsync @@ -181,7 +191,7 @@ unsafe extern "C" fn save_context() { " // Double Precision Accelerator Option rur a3, f64r_lo - s32i a3, sp, +XT_STK_F64R_LO + s32i a3, sp, +XT_STK_F64R_LO_CPENABLE rur a3, f64r_hi s32i a3, sp, +XT_STK_F64R_HI rur a3, f64s @@ -399,7 +409,7 @@ unsafe extern "C" fn restore_context() { #[cfg(all(feature = "float-save-restore", XCHAL_HAVE_DFP_ACCEL))] " // Double Precision Accelerator Option - l32i a3, sp, +XT_STK_F64R_LO + l32i a3, sp, +XT_STK_F64R_LO_CPENABLE wur a3, f64r_lo l32i a3, sp, +XT_STK_F64R_HI wur a3, f64r_hi @@ -433,7 +443,7 @@ unsafe extern "C" fn restore_context() { #[cfg(all(XCHAL_HAVE_CP, not(feature = "float-save-restore")))] " /* Restore coprocessor state after ISR */ - l32i a3, sp, +XT_STK_CPENABLE + l32i a3, sp, +XT_STK_F64R_LO_CPENABLE wsr a3, CPENABLE rsync ", diff --git a/xtensa-lx-rt/src/exception/context.rs b/xtensa-lx-rt/src/exception/context.rs index 93964a5cc..2e7e96925 100644 --- a/xtensa-lx-rt/src/exception/context.rs +++ b/xtensa-lx-rt/src/exception/context.rs @@ -4,7 +4,7 @@ use super::ExceptionCause; /// State of the CPU saved when entering exception or interrupt /// -/// Must be aligned with assembly frame format in assembly_esp32 +/// Must be aligned with assembly frame format in asm.rs #[repr(C)] #[allow(non_snake_case)] #[derive(Debug, Clone, Copy)] @@ -43,11 +43,16 @@ pub struct Context { pub M1: u32, pub M2: u32, pub M3: u32, - #[cfg(all(feature = "float-save-restore", XCHAL_HAVE_DFP_ACCEL))] - pub F64R_LO: u32, - #[cfg(all(feature = "float-save-restore", XCHAL_HAVE_DFP_ACCEL))] + #[cfg(XCHAL_HAVE_CP)] + // Either F64R_LO or CPENABLE depending on `float-save-restore`. + pub F64R_LO_CPENABLE: u32, + // F64R_HI is only meaningful with cfg!(XCHAL_HAVE_DFP_ACCEL) but it's present in the stack + // frame unconditionally + #[cfg(feature = "float-save-restore")] pub F64R_HI: u32, - #[cfg(all(feature = "float-save-restore", XCHAL_HAVE_DFP_ACCEL))] + // F64S is only meaningful with cfg!(XCHAL_HAVE_DFP_ACCEL) but it's present in the stack frame + // unconditionally + #[cfg(feature = "float-save-restore")] pub F64S: u32, #[cfg(all(feature = "float-save-restore", XCHAL_HAVE_FP))] pub FCR: u32,