From 65bc1ed05530f1de49006f194966ec21007b17e8 Mon Sep 17 00:00:00 2001 From: Martin Vychodil Date: Tue, 25 Apr 2023 10:28:10 +0200 Subject: [PATCH] System: remove digital-system reset within OS restart when Memprot on --- components/esp_system/esp_system.c | 18 ---------- .../include/esp_private/system_internal.h | 8 +++++ components/esp_system/port/cpu_start.c | 4 +-- components/esp_system/port/esp_system_chip.c | 3 ++ components/esp_system/port/panic_handler.c | 20 +---------- tools/test_apps/system/panic/pytest_panic.py | 36 +++++++++++++++++-- .../system/panic/test_panic_util/panic_dut.py | 4 +++ 7 files changed, 52 insertions(+), 41 deletions(-) diff --git a/components/esp_system/esp_system.c b/components/esp_system/esp_system.c index 4419011ce2c5..cc1139942662 100644 --- a/components/esp_system/esp_system.c +++ b/components/esp_system/esp_system.c @@ -61,23 +61,5 @@ void esp_restart(void) vTaskSuspendAll(); #endif // CONFIG_FREERTOS_SMP - bool digital_reset_needed = false; -#if CONFIG_ESP_SYSTEM_MEMPROT_FEATURE -#if CONFIG_IDF_TARGET_ESP32S2 - if (esp_memprot_is_intr_ena_any() || esp_memprot_is_locked_any()) { - digital_reset_needed = true; - } -#else - bool is_on = false; - if (esp_mprot_is_intr_ena_any(&is_on) != ESP_OK || is_on) { - digital_reset_needed = true; - } else if (esp_mprot_is_conf_locked_any(&is_on) != ESP_OK || is_on) { - digital_reset_needed = true; - } -#endif -#endif - if (digital_reset_needed) { - esp_restart_noos_dig(); - } esp_restart_noos(); } diff --git a/components/esp_system/include/esp_private/system_internal.h b/components/esp_system/include/esp_private/system_internal.h index 9a8989bba050..e2e03ee8e178 100644 --- a/components/esp_system/include/esp_private/system_internal.h +++ b/components/esp_system/include/esp_private/system_internal.h @@ -18,6 +18,14 @@ extern "C" { * @note This function should not be called from FreeRTOS applications. * Use esp_restart instead. * + * This function executes a CPU reset (see TRM). + * + * CPU resets do not reset digital peripherals, but this function will + * manually reset a subset of digital peripherals (depending on target) before + * carrying out the CPU reset. + * + * Memory protection is also cleared by a CPU reset. + * * This is an internal function called by esp_restart. It is called directly * by the panic handler and brownout detector interrupt. */ diff --git a/components/esp_system/port/cpu_start.c b/components/esp_system/port/cpu_start.c index d3fdbc77d377..4e60f528c04f 100644 --- a/components/esp_system/port/cpu_start.c +++ b/components/esp_system/port/cpu_start.c @@ -613,7 +613,7 @@ void IRAM_ATTR call_start_cpu0(void) if (esp_mprot_is_conf_locked_any(&is_locked) != ESP_OK || is_locked) { #endif ESP_EARLY_LOGE(TAG, "Memprot feature locked after the system reset! Potential safety corruption, rebooting."); - esp_restart_noos_dig(); + esp_restart_noos(); } //default configuration of PMS Memprot @@ -634,7 +634,7 @@ void IRAM_ATTR call_start_cpu0(void) if (memp_err != ESP_OK) { ESP_EARLY_LOGE(TAG, "Failed to set Memprot feature (0x%08X: %s), rebooting.", memp_err, esp_err_to_name(memp_err)); - esp_restart_noos_dig(); + esp_restart_noos(); } #endif //CONFIG_ESP_SYSTEM_MEMPROT_FEATURE && !CONFIG_ESP_SYSTEM_MEMPROT_TEST diff --git a/components/esp_system/port/esp_system_chip.c b/components/esp_system/port/esp_system_chip.c index 440a0f1a7a7a..06b029c614bb 100644 --- a/components/esp_system/port/esp_system_chip.c +++ b/components/esp_system/port/esp_system_chip.c @@ -17,6 +17,8 @@ #include "esp_rom_sys.h" #include "sdkconfig.h" +// used only by ESP32 panic handler +#ifdef CONFIG_IDF_TARGET_ESP32 void IRAM_ATTR esp_restart_noos_dig(void) { // In case any of the calls below results in re-enabling of interrupts @@ -64,6 +66,7 @@ void IRAM_ATTR esp_restart_noos_dig(void) ; } } +#endif uint32_t esp_get_free_heap_size( void ) { diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c index 41e5d4eea609..a936a01b4ec7 100644 --- a/components/esp_system/port/panic_handler.c +++ b/components/esp_system/port/panic_handler.c @@ -227,29 +227,11 @@ void IRAM_ATTR xt_unhandled_exception(void *frame) void __attribute__((noreturn)) panic_restart(void) { - bool digital_reset_needed = false; #ifdef CONFIG_IDF_TARGET_ESP32 // On the ESP32, cache error status can only be cleared by system reset if (esp_cache_err_get_cpuid() != -1) { - digital_reset_needed = true; - } -#endif -#if CONFIG_ESP_SYSTEM_MEMPROT_FEATURE -#if CONFIG_IDF_TARGET_ESP32S2 - if (esp_memprot_is_intr_ena_any() || esp_memprot_is_locked_any()) { - digital_reset_needed = true; - } -#else - bool is_on = false; - if (esp_mprot_is_intr_ena_any(&is_on) != ESP_OK || is_on) { - digital_reset_needed = true; - } else if (esp_mprot_is_conf_locked_any(&is_on) != ESP_OK || is_on) { - digital_reset_needed = true; - } -#endif -#endif - if (digital_reset_needed) { esp_restart_noos_dig(); } +#endif esp_restart_noos(); } diff --git a/tools/test_apps/system/panic/pytest_panic.py b/tools/test_apps/system/panic/pytest_panic.py index 9132e77943eb..0415cc0b0241 100644 --- a/tools/test_apps/system/panic/pytest_panic.py +++ b/tools/test_apps/system/panic/pytest_panic.py @@ -57,7 +57,7 @@ def get_default_backtrace(config: str) -> List[str]: return [config, 'app_main', 'main_task', 'vPortTaskWrapper'] -def common_test(dut: PanicTestDut, config: str, expected_backtrace: Optional[List[str]] = None) -> None: +def common_test(dut: PanicTestDut, config: str, expected_backtrace: Optional[List[str]] = None, check_cpu_reset: Optional[bool] = True) -> None: if 'gdbstub' in config: dut.expect_exact('Entering gdb stub now.') dut.start_gdb() @@ -76,6 +76,9 @@ def common_test(dut: PanicTestDut, config: str, expected_backtrace: Optional[Lis dut.expect('Rebooting...') + if check_cpu_reset: + dut.expect_cpu_reset() + @pytest.mark.parametrize('config', CONFIGS, indirect=True) @pytest.mark.generic @@ -178,6 +181,7 @@ def test_panic_extram_stack(dut: PanicTestDut, config: str, test_func_name: str) dut.expect_exact('Restoring stack') # The caller must be accessible after restoring the stack dut.expect_exact('Core dump has been saved to flash.') + common_test(dut, config) @@ -252,8 +256,9 @@ def test_cache_error(dut: PanicTestDut, config: str, test_func_name: str) -> Non if dut.target in ['esp32s2', 'esp32s3']: # 'test_cache_error' missing from GDB backtrace on ESP32-S2 and ESP-S3, IDF-6561 expected_backtrace = ['die', 'app_main', 'main_task', 'vPortTaskWrapper'] + common_test( - dut, config, expected_backtrace=expected_backtrace + dut, config, expected_backtrace=expected_backtrace, check_cpu_reset=(dut.target != 'esp32') ) @@ -274,6 +279,7 @@ def test_stack_overflow(dut: PanicTestDut, config: str, test_func_name: str) -> dut.expect_stack_dump() dut.expect_elf_sha256() dut.expect_none('Guru Meditation') + common_test(dut, config, expected_backtrace=get_default_backtrace(test_func_name)) @@ -298,6 +304,7 @@ def test_instr_fetch_prohibited( dut.expect_elf_sha256() dut.expect_none('Guru Meditation') + common_test( dut, config, @@ -322,6 +329,7 @@ def test_illegal_instruction( dut.expect_stack_dump() dut.expect_elf_sha256() dut.expect_none('Guru Meditation') + common_test(dut, config, expected_backtrace=get_default_backtrace(test_func_name)) @@ -340,6 +348,7 @@ def test_storeprohibited(dut: PanicTestDut, config: str, test_func_name: str) -> dut.expect_stack_dump() dut.expect_elf_sha256() dut.expect_none('Guru Meditation') + common_test(dut, config, expected_backtrace=get_default_backtrace(test_func_name)) @@ -405,6 +414,7 @@ def test_abort_cache_disabled( dut.expect_stack_dump() dut.expect_elf_sha256() dut.expect_none(['Guru Meditation', 'Re-entered core dump']) + common_test( dut, config, @@ -456,6 +466,7 @@ def test_assert_cache_disabled( dut.expect_stack_dump() dut.expect_elf_sha256() dut.expect_none(['Guru Meditation', 'Re-entered core dump']) + common_test( dut, config, @@ -518,6 +529,7 @@ def test_panic_delay(dut: PanicTestDut) -> None: def test_dcache_read_violation(dut: PanicTestDut, test_func_name: str) -> None: dut.run_test_func(test_func_name) dut.expect_exact(r'Test error: Test function has returned') + dut.expect_cpu_reset() # TODO: IDF-6820: ESP32-S2 -> Fix multiple panic reasons in different runs @@ -530,6 +542,7 @@ def test_dcache_write_violation(dut: PanicTestDut, test_func_name: str) -> None: dut.expect(r'Write operation at address [0-9xa-f]+ not permitted \((\S+)\)') dut.expect_reg_dump(0) dut.expect_backtrace() + dut.expect_cpu_reset() @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @@ -549,6 +562,8 @@ def test_iram_reg1_write_violation(dut: PanicTestDut, test_func_name: str) -> No dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @pytest.mark.generic @@ -572,6 +587,8 @@ def test_iram_reg2_write_violation(dut: PanicTestDut, test_func_name: str) -> No dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @pytest.mark.generic @@ -595,6 +612,8 @@ def test_iram_reg3_write_violation(dut: PanicTestDut, test_func_name: str) -> No dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + # TODO: IDF-6820: ESP32-S2 -> Fix incorrect panic reason: Unhandled debug exception @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @@ -620,6 +639,8 @@ def test_iram_reg4_write_violation(dut: PanicTestDut, test_func_name: str) -> No dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + # TODO: IDF-6820: ESP32-S2 -> Fix multiple panic reasons in different runs @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @@ -638,6 +659,8 @@ def test_dram_reg1_execute_violation(dut: PanicTestDut, test_func_name: str) -> dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + # TODO: IDF-6820: ESP32-S2 -> Fix multiple panic reasons in different runs @pytest.mark.parametrize('config', CONFIGS_MEMPROT_IDRAM, indirect=True) @@ -655,12 +678,15 @@ def test_dram_reg2_execute_violation(dut: PanicTestDut, test_func_name: str) -> dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + @pytest.mark.parametrize('config', CONFIGS_MEMPROT_RTC_FAST_MEM, indirect=True) @pytest.mark.generic def test_rtc_fast_reg1_execute_violation(dut: PanicTestDut, test_func_name: str) -> None: dut.run_test_func(test_func_name) dut.expect_exact(r'Test error: Test function has returned') + dut.expect_cpu_reset() @pytest.mark.parametrize('config', CONFIGS_MEMPROT_RTC_FAST_MEM, indirect=True) @@ -681,6 +707,8 @@ def test_rtc_fast_reg2_execute_violation(dut: PanicTestDut, test_func_name: str) dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + # TODO: IDF-6820: ESP32-S2 -> Fix multiple panic reasons in different runs @pytest.mark.parametrize('config', CONFIGS_MEMPROT_RTC_FAST_MEM, indirect=True) @@ -706,6 +734,8 @@ def test_rtc_fast_reg3_execute_violation(dut: PanicTestDut, test_func_name: str) dut.expect_reg_dump(0) dut.expect_stack_dump() + dut.expect_cpu_reset() + @pytest.mark.parametrize('config', CONFIGS_MEMPROT_RTC_SLOW_MEM, indirect=True) @pytest.mark.generic @@ -715,6 +745,7 @@ def test_rtc_slow_reg1_execute_violation(dut: PanicTestDut, test_func_name: str) dut.expect(r'Read operation at address [0-9xa-f]+ not permitted \((\S+)\)') dut.expect_reg_dump(0) dut.expect_corrupted_backtrace() + dut.expect_cpu_reset() @pytest.mark.parametrize('config', CONFIGS_MEMPROT_RTC_SLOW_MEM, indirect=True) @@ -725,6 +756,7 @@ def test_rtc_slow_reg2_execute_violation(dut: PanicTestDut, test_func_name: str) dut.expect(r'Read operation at address [0-9xa-f]+ not permitted \((\S+)\)') dut.expect_reg_dump(0) dut.expect_corrupted_backtrace() + dut.expect_cpu_reset() @pytest.mark.esp32 diff --git a/tools/test_apps/system/panic/test_panic_util/panic_dut.py b/tools/test_apps/system/panic/test_panic_util/panic_dut.py index 020443676bce..05b25c60b4c3 100644 --- a/tools/test_apps/system/panic/test_panic_util/panic_dut.py +++ b/tools/test_apps/system/panic/test_panic_util/panic_dut.py @@ -88,6 +88,10 @@ def expect_reg_dump(self, core: int = 0) -> None: """Expect method for the register dump""" self.expect(r'Core\s+%d register dump:' % core) + def expect_cpu_reset(self) -> None: + # no digital system reset for panic handling restarts (see IDF-7255) + self.expect(r'.*rst:.*(RTC_SW_CPU_RST|SW_CPU_RESET|SW_CPU)') + def expect_elf_sha256(self) -> None: """Expect method for ELF SHA256 line""" elf_sha256 = sha256(self.app.elf_file)