Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix restoring of CPENABLE #2315

Merged
merged 4 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions hil-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 }
Expand Down Expand Up @@ -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 = [
Expand All @@ -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"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dep: meant there was no implicit esp-alloc feature, which I need in the new test.


# https://doc.rust-lang.org/cargo/reference/profiles.html#test
# Test and bench profiles inherit from dev and release respectively.
Expand Down
4 changes: 4 additions & 0 deletions hil-test/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
179 changes: 179 additions & 0 deletions hil-test/tests/esp_wifi_floats.rs
Original file line number Diff line number Diff line change
@@ -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);
}
}
2 changes: 2 additions & 0 deletions xtensa-lx-rt/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 17 additions & 7 deletions xtensa-lx-rt/src/exception/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
",
Expand Down
15 changes: 10 additions & 5 deletions xtensa-lx-rt/src/exception/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
Expand Down