Fix restoring of CPENABLE (#2315)

* Add tests Cp0Disabled issue

* Fix saving CPENABLE on context switch

* Fix position shift of registers

* Clean up
This commit is contained in:
Dániel Buga 2024-10-09 12:33:31 +02:00 committed by GitHub
parent dc88cb13e8
commit be9dc0e0b7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 247 additions and 17 deletions

View File

@ -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.

View File

@ -37,5 +37,9 @@ fn main() -> Result<(), Box<dyn Error>> {
// 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(())
}

View File

@ -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<RefCell<Option<SoftwareInterrupt<0>>>> = 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);
}
}

View File

@ -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

View File

@ -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
",

View File

@ -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,