Merge branch 'bugfix/memprot_checks' into 'master'

[System] remove Memprot specific checks and setup (heap caps, OS restart)

Closes IDF-7255

See merge request espressif/esp-idf!23433

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();
 }

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.
  */

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
 

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 )
 {

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();
 }

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

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)