Merge branch 'feature/coredump_data_in_elf_format_cpu_cache_enable' into 'master'

espcoredump: Enable cpu cache in panic handler for core dump

See merge request espressif/esp-idf!5496

components/esp32/Kconfig

@@ -776,6 +776,20 @@ menu "ESP32-specific"
             To prevent interrupting DPORT workarounds,
             need to disable interrupt with a maximum used level in the system.
 
+    config ESP32_PANIC_HANDLER_IRAM
+        bool "Place panic handler code in IRAM"
+        default n
+        help
+            If this option is disabled (default), the panic handler code is placed in flash not IRAM.
+            This means that if ESP-IDF crashes while flash cache is disabled, the panic handler will
+            automatically re-enable flash cache before running GDB Stub or Core Dump. This adds some minor
+            risk, if the flash cache status is also corrupted during the crash.
+
+            If this option is enabled, the panic handler code is placed in IRAM. This allows the panic
+            handler to run without needing to re-enable cache first. This may be necessary to debug some
+            complex issues with crashes while flash cache is disabled (for example, when writing to
+            SPI flash.)
+
 endmenu  # ESP32-Specific
 
 menu "Power Management"

components/esp32/cpu_start.c

@@ -404,9 +404,11 @@ void start_cpu0_default(void)
     /* init default OS-aware flash access critical section */
     spi_flash_guard_set(&g_flash_guard_default_ops);
 
+#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
     esp_flash_app_init();
     esp_err_t flash_ret = esp_flash_init_default_chip();
     assert(flash_ret == ESP_OK);
+#endif
 
     uint8_t revision = esp_efuse_get_chip_ver();
     ESP_LOGI(TAG, "Chip Revision: %d", revision);

components/esp32/panic.c

@@ -608,6 +608,14 @@ static __attribute__((noreturn)) void commonErrorHandler(XtExcFrame *frame)
     reconfigureAllWdts();
 #endif
 
+#if !CONFIG_ESP32_PANIC_HANDLER_IRAM
+    // Re-enable CPU cache for current CPU if it was disabled
+    if (!spi_flash_cache_enabled()) {
+        spi_flash_enable_cache(core_id);
+        panicPutStr("Re-enable cpu cache.\r\n");
+    }
+#endif
+
 #if CONFIG_ESP32_PANIC_GDBSTUB
     disableAllWdts();
     rtc_wdt_disable();

components/esp_gdbstub/linker.lf

@@ -1,4 +1,7 @@
 [mapping:esp_gdbstub]
 archive: libesp_gdbstub.a
-entries:
-    * (noflash)
+entries: 
+   if ESP32_PANIC_HANDLER_IRAM = y:
+        * (noflash_text)
+   else:
+        * (default)

components/espcoredump/Kconfig

@@ -13,6 +13,7 @@ menu "Core dump"
         config ESP32_ENABLE_COREDUMP_TO_FLASH
             bool "Flash"
             select ESP32_ENABLE_COREDUMP
+            select SPI_FLASH_USE_LEGACY_IMPL
         config ESP32_ENABLE_COREDUMP_TO_UART
             bool "UART"
             select ESP32_ENABLE_COREDUMP

components/espcoredump/linker.lf

@@ -1,7 +1,10 @@
 [mapping:espcoredump]
 archive: libespcoredump.a
 entries: 
-    core_dump_uart (noflash_text)
-    core_dump_flash (noflash_text)
-    core_dump_common (noflash_text)
-    core_dump_port (noflash_text)
+    if ESP32_PANIC_HANDLER_IRAM = y:
+        core_dump_uart (noflash_text)
+        core_dump_flash (noflash_text)
+        core_dump_common (noflash_text)
+        core_dump_port (noflash_text)
+    else:
+        * (default)

components/espcoredump/src/core_dump_flash.c

@@ -200,8 +200,8 @@ static esp_err_t esp_core_dump_flash_write_data(void *priv, void * data, uint32_
 
 void esp_core_dump_to_flash(XtExcFrame *frame)
 {
-    core_dump_write_config_t wr_cfg;
-    core_dump_write_flash_data_t wr_data;
+    static core_dump_write_config_t wr_cfg;
+    static core_dump_write_flash_data_t wr_data;
 
     core_dump_crc_t crc = esp_core_dump_calc_flash_config_crc();
     if (s_core_flash_config.partition_config_crc != crc) {
@@ -214,8 +214,10 @@ void esp_core_dump_to_flash(XtExcFrame *frame)
         return;
     }
 
-    /* init non-OS flash access critical section */
+#if CONFIG_SPI_FLASH_USE_LEGACY_IMPL
+    // init non-OS flash access critical section
     spi_flash_guard_set(&g_flash_guard_no_os_ops);
+#endif
 
     memset(&wr_cfg, 0, sizeof(wr_cfg));
     wr_cfg.prepare = esp_core_dump_flash_write_prepare;

components/espcoredump/src/core_dump_port.c

@@ -94,7 +94,9 @@ bool esp_core_dump_process_stack(core_dump_task_header_t* task_snaphort, uint32_
         ESP_COREDUMP_LOG_PROCESS("Stack len = %lu (%x %x)", len,
                 task_snaphort->stack_start, task_snaphort->stack_end);
         // Take stack padding into account
-        *length = (len + sizeof(uint32_t) - 1) & ~(sizeof(uint32_t) - 1);
+        if (length) {
+            *length = (len + sizeof(uint32_t) - 1) & ~(sizeof(uint32_t) - 1);
+        }
         task_is_valid = true;
     }
     return task_is_valid;

components/spi_flash/cache_utils.c

@@ -31,6 +31,18 @@
 #include "esp_spi_flash.h"
 #include "esp_log.h"
 
+#define DPORT_CACHE_BIT(cpuid, regid) DPORT_ ## cpuid ## regid
+
+#define DPORT_CACHE_MASK(cpuid) (DPORT_CACHE_BIT(cpuid, _CACHE_MASK_OPSDRAM) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DROM0) | \
+                                DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DRAM1) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IROM0) | \
+                                DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM1) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM0) )
+
+#define DPORT_CACHE_VAL(cpuid) (~(DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DROM0) | \
+                                        DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DRAM1) | \
+                                        DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM0)))
+
+#define DPORT_CACHE_GET_VAL(cpuid) (cpuid == 0) ? DPORT_CACHE_VAL(PRO) : DPORT_CACHE_VAL(APP)
+#define DPORT_CACHE_GET_MASK(cpuid) (cpuid == 0) ? DPORT_CACHE_MASK(PRO) : DPORT_CACHE_MASK(APP)
 
 static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_state);
 static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_state);
@@ -256,13 +268,10 @@ void IRAM_ATTR spi_flash_enable_interrupts_caches_no_os(void)
  * Cache_Flush before Cache_Read_Enable, even if cached data was not modified.
  */
 
-static const uint32_t cache_mask  = DPORT_APP_CACHE_MASK_OPSDRAM | DPORT_APP_CACHE_MASK_DROM0 |
-        DPORT_APP_CACHE_MASK_DRAM1 | DPORT_APP_CACHE_MASK_IROM0 |
-        DPORT_APP_CACHE_MASK_IRAM1 | DPORT_APP_CACHE_MASK_IRAM0;
-
 static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_state)
 {
     uint32_t ret = 0;
+    const uint32_t cache_mask = DPORT_CACHE_GET_MASK(cpuid);
     if (cpuid == 0) {
         ret |= DPORT_GET_PERI_REG_BITS2(DPORT_PRO_CACHE_CTRL1_REG, cache_mask, 0);
         while (DPORT_GET_PERI_REG_BITS2(DPORT_PRO_DCACHE_DBUG0_REG, DPORT_PRO_CACHE_STATE, DPORT_PRO_CACHE_STATE_S) != 1) {
@@ -281,6 +290,7 @@ static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_st
 
 static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_state)
 {
+    const uint32_t cache_mask = DPORT_CACHE_GET_MASK(cpuid);
     if (cpuid == 0) {
         DPORT_SET_PERI_REG_BITS(DPORT_PRO_CACHE_CTRL_REG, 1, 1, DPORT_PRO_CACHE_ENABLE_S);
         DPORT_SET_PERI_REG_BITS(DPORT_PRO_CACHE_CTRL1_REG, cache_mask, saved_state, 0);
@@ -290,7 +300,6 @@ static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_sta
     }
 }
 
-
 IRAM_ATTR bool spi_flash_cache_enabled(void)
 {
     bool result = (DPORT_REG_GET_BIT(DPORT_PRO_CACHE_CTRL_REG, DPORT_PRO_CACHE_ENABLE) != 0);
@@ -299,3 +308,12 @@ IRAM_ATTR bool spi_flash_cache_enabled(void)
 #endif
     return result;
 }
+
+void IRAM_ATTR spi_flash_enable_cache(uint32_t cpuid)
+{
+    uint32_t cache_value = DPORT_CACHE_GET_VAL(cpuid);
+    cache_value &= DPORT_CACHE_GET_MASK(cpuid);
+
+    // Re-enable cache on this CPU
+    spi_flash_restore_cache(cpuid, cache_value);
+}

components/spi_flash/include/esp_spi_flash.h

@@ -295,6 +295,13 @@ const void *spi_flash_phys2cache(size_t phys_offs, spi_flash_mmap_memory_t memor
  */
 bool spi_flash_cache_enabled(void);
 
+/**
+ * @brief Re-enable cache for the core defined as cpuid parameter.
+ *
+ * @param cpuid the core number to enable instruction cache for
+ */
+void spi_flash_enable_cache(uint32_t cpuid);
+
 /**
  * @brief SPI flash critical section enter function.
  *

components/spi_flash/partition.c

@@ -172,7 +172,9 @@ static esp_err_t load_partitions(void)
             err = ESP_ERR_NO_MEM;
             break;
         }
+#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
         item->info.flash_chip = esp_flash_default_chip;
+#endif
         item->info.address = it->pos.offset;
         item->info.size = it->pos.size;
         item->info.type = it->type;
@@ -334,10 +336,11 @@ esp_err_t esp_partition_read(const esp_partition_t* partition,
 #endif // CONFIG_SPI_FLASH_USE_LEGACY_IMPL
     } else {
 #if CONFIG_SECURE_FLASH_ENC_ENABLED
+#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
         if (partition->flash_chip != esp_flash_default_chip) {
             return ESP_ERR_NOT_SUPPORTED;
         }
-
+#endif
         /* Encrypted partitions need to be read via a cache mapping */
         const void *buf;
         spi_flash_mmap_handle_t handle;
@@ -376,9 +379,11 @@ esp_err_t esp_partition_write(const esp_partition_t* partition,
 #endif // CONFIG_SPI_FLASH_USE_LEGACY_IMPL
     } else {
 #if CONFIG_SECURE_FLASH_ENC_ENABLED
+#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
         if (partition->flash_chip != esp_flash_default_chip) {
             return ESP_ERR_NOT_SUPPORTED;
         }
+#endif
         return spi_flash_write_encrypted(dst_offset, src, size);
 #else
         return ESP_ERR_NOT_SUPPORTED;
@@ -428,9 +433,11 @@ esp_err_t esp_partition_mmap(const esp_partition_t* partition, size_t offset, si
     if (offset + size > partition->size) {
         return ESP_ERR_INVALID_SIZE;
     }
+#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
     if (partition->flash_chip != esp_flash_default_chip) {
         return ESP_ERR_NOT_SUPPORTED;
     }
+#endif
     size_t phys_addr = partition->address + offset;
     // offset within 64kB block
     size_t region_offset = phys_addr & 0xffff;

docs/en/api-guides/fatal-errors.rst

@@ -62,6 +62,10 @@ Behavior of panic handler is affected by two other configuration options.
 
 - If :doc:`Core Dump <core_dump>` feature is enabled (``CONFIG_ESP32_ENABLE_COREDUMP_TO_FLASH`` or ``CONFIG_ESP32_ENABLE_COREDUMP_TO_UART`` options), then system state (task stacks and registers) will be dumped either to Flash or UART, for later analysis.
 
+- If :ref:`CONFIG_ESP32_PANIC_HANDLER_IRAM` is disabled (disabled by default), the panic handler code is placed in flash memory not IRAM. This means that if ESP-IDF crashes while flash cache is disabled, the panic handler will automatically re-enable flash cache before running GDB Stub or Core Dump. This adds some minor risk, if the flash cache status is also corrupted during the crash.
+  
+  If this option is enabled, the panic handler code is placed in IRAM. This allows the panic handler to run without needing to re-enable cache first. This may be necessary to debug some complex issues with crashes while flash cache is disabled (for example, when writing to SPI flash).
+
 The following diagram illustrates panic handler behavior:
 
 .. blockdiag::