From d3b54ec84aafd973ce5be2480ff989dbe05b00be Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Thu, 12 Sep 2019 02:41:00 +0800 Subject: [PATCH] esp_flash: fix the coredump issue During coredump, dangerous-area-checking should be disabled, and cache disabling should be replaced by a safer version. Dangerous-area-checking used to be in the HAL, but it seems to be more fit to os functions. So it's moved to os functions. Interfaces are provided to switch between os functions during coredump. --- components/espcoredump/src/core_dump_flash.c | 4 +- components/soc/include/hal/spi_flash_types.h | 5 --- components/spi_flash/esp_flash_api.c | 14 ++++++- components/spi_flash/esp_flash_spi_init.c | 3 +- components/spi_flash/include/esp_flash.h | 3 ++ .../spi_flash/include/esp_flash_internal.h | 32 ++++++++++++++++ .../spi_flash/include/memspi_host_driver.h | 12 ------ components/spi_flash/memspi_host_driver.c | 12 ------ components/spi_flash/spi_flash_os_func_app.c | 37 +++++++++++++++++-- components/spi_flash/spi_flash_os_func_noos.c | 17 +++++++-- 10 files changed, 100 insertions(+), 39 deletions(-) diff --git a/components/espcoredump/src/core_dump_flash.c b/components/espcoredump/src/core_dump_flash.c index dd48d5ba8..ce7ebef4b 100644 --- a/components/espcoredump/src/core_dump_flash.c +++ b/components/espcoredump/src/core_dump_flash.c @@ -15,6 +15,7 @@ #include "esp32/rom/crc.h" #include "esp_partition.h" #include "esp_core_dump_priv.h" +#include "esp_flash_internal.h" const static DRAM_ATTR char TAG[] __attribute__((unused)) = "esp_core_dump_flash"; @@ -50,7 +51,7 @@ static inline core_dump_crc_t esp_core_dump_calc_flash_config_crc(void) return crc32_le(0, (uint8_t const *)&s_core_flash_config.partition, sizeof(s_core_flash_config.partition)); } -void esp_core_dump_flash_init() +void esp_core_dump_flash_init(void) { const esp_partition_t *core_part; @@ -216,6 +217,7 @@ void esp_core_dump_to_flash(XtExcFrame *frame) /* init non-OS flash access critical section */ spi_flash_guard_set(&g_flash_guard_no_os_ops); + esp_flash_app_disable_protect(true); memset(&wr_cfg, 0, sizeof(wr_cfg)); wr_cfg.prepare = esp_core_dump_flash_write_prepare; diff --git a/components/soc/include/hal/spi_flash_types.h b/components/soc/include/hal/spi_flash_types.h index 7babeee0c..c19eee1ed 100644 --- a/components/soc/include/hal/spi_flash_types.h +++ b/components/soc/include/hal/spi_flash_types.h @@ -142,11 +142,6 @@ struct spi_flash_host_driver_t { * modified, the cache needs to be flushed. Left NULL if not supported. */ esp_err_t (*flush_cache)(spi_flash_host_driver_t* driver, uint32_t addr, uint32_t size); - /** - * Check if the given region is protected (e.g. is the bootloader). Left - * NULL if current host doesn't need protection. - */ - bool (*region_protected)(spi_flash_host_driver_t* driver, uint32_t addr, uint32_t size); }; #ifdef __cplusplus diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index b350e3d0f..ae907b4fb 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/components/spi_flash/esp_flash_api.c @@ -22,6 +22,7 @@ #include "esp_log.h" #include "sdkconfig.h" #include "esp_heap_caps.h" +#include "esp_flash_internal.h" static const char TAG[] = "spi_flash"; @@ -42,7 +43,7 @@ static const char TAG[] = "spi_flash"; #define CHECK_WRITE_ADDRESS(CHIP, ADDR, SIZE) #else /* FAILS or ABORTS */ #define CHECK_WRITE_ADDRESS(CHIP, ADDR, SIZE) do { \ - if (CHIP && CHIP->host->region_protected && CHIP->host->region_protected(CHIP->host, ADDR, SIZE)) { \ + if (CHIP && CHIP->os_func->region_protected && CHIP->os_func->region_protected(CHIP->os_func_data, ADDR, SIZE)) { \ UNSAFE_WRITE_ADDRESS; \ } \ } while(0) @@ -600,6 +601,17 @@ inline static IRAM_ATTR bool regions_overlap(uint32_t a_start, uint32_t a_len,ui } +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL +esp_err_t esp_flash_app_disable_protect(bool disable) +{ + if (disable) { + return esp_flash_app_disable_os_functions(esp_flash_default_chip); + } else { + return esp_flash_app_init_os_functions(esp_flash_default_chip); + } +} +#endif + /*------------------------------------------------------------------------------ Adapter layer to original api before IDF v4.0 ------------------------------------------------------------------------------*/ diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index dda02130d..0839d9018 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/components/spi_flash/esp_flash_spi_init.c @@ -22,6 +22,7 @@ #include "esp_heap_caps.h" #include "hal/spi_types.h" #include "driver/spi_common.h" +#include "esp_flash_internal.h" __attribute__((unused)) static const char TAG[] = "spi_flash"; @@ -207,7 +208,7 @@ esp_err_t esp_flash_init_default_chip() esp_err_t esp_flash_app_init() { - return esp_flash_init_os_functions(&default_chip, 0); + return esp_flash_app_init_os_functions(&default_chip); } #endif diff --git a/components/spi_flash/include/esp_flash.h b/components/spi_flash/include/esp_flash.h index e5a3a1830..51cfe9997 100644 --- a/components/spi_flash/include/esp_flash.h +++ b/components/spi_flash/include/esp_flash.h @@ -45,6 +45,9 @@ typedef struct { /** Called after completing any flash operation. */ esp_err_t (*end)(void *arg); + /** Called before any erase/write operations to check whether the region is limited by the OS */ + esp_err_t (*region_protected)(void* arg, size_t start_addr, size_t size); + /** Delay for at least 'ms' milliseconds. Called in between 'start' and 'end'. */ esp_err_t (*delay_ms)(void *arg, unsigned ms); } esp_flash_os_functions_t; diff --git a/components/spi_flash/include/esp_flash_internal.h b/components/spi_flash/include/esp_flash_internal.h index b0d35f371..0496f5649 100644 --- a/components/spi_flash/include/esp_flash_internal.h +++ b/components/spi_flash/include/esp_flash_internal.h @@ -50,6 +50,19 @@ esp_err_t esp_flash_init_default_chip(void); esp_err_t esp_flash_app_init(void); #endif +/** + * Disable OS-level SPI flash protections in IDF + * + * Called by the IDF internal code (e.g. coredump). You do not need to call this in your own applications. + * + * @return always ESP_OK. + */ +#ifdef CONFIG_SPI_FLASH_USE_LEGACY_IMPL +#define esp_flash_app_disable_protect(...) ({ESP_OK;}) +#else +esp_err_t esp_flash_app_disable_protect(bool disable); +#endif + /** * Initialize OS-level functions for a specific chip. * @@ -62,6 +75,25 @@ esp_err_t esp_flash_app_init(void); */ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id); +/** + * Initialize OS-level functions for the main flash chip. + * + * @param chip The chip to init os functions. Only pointer to the default chip is supported now. + * + * @return always ESP_OK + */ +esp_err_t esp_flash_app_init_os_functions(esp_flash_t* chip); + +/** + * Disable OS-level functions for the main flash chip during special phases (e.g. coredump) + * + * @param chip The chip to init os functions. Only "esp_flash_default_chip" is supported now. + * + * @return always ESP_OK + */ +esp_err_t esp_flash_app_disable_os_functions(esp_flash_t* chip); + + #ifdef __cplusplus } diff --git a/components/spi_flash/include/memspi_host_driver.h b/components/spi_flash/include/memspi_host_driver.h index 434361d97..cb0f3e9d9 100644 --- a/components/spi_flash/include/memspi_host_driver.h +++ b/components/spi_flash/include/memspi_host_driver.h @@ -35,7 +35,6 @@ .configure_host_read_mode = spi_flash_hal_configure_host_read_mode, \ .poll_cmd_done = spi_flash_hal_poll_cmd_done, \ .flush_cache = memspi_host_flush_cache, \ - .region_protected = memspi_region_protected, \ } /// configuration for the memspi host @@ -100,14 +99,3 @@ esp_err_t memspi_host_read_status_hs(spi_flash_host_driver_t *driver, uint8_t *o * @return always ESP_OK. */ esp_err_t memspi_host_flush_cache(spi_flash_host_driver_t* driver, uint32_t addr, uint32_t size); - -/** - * Check if the given region is protected. - * - * @param driver The driver context. - * @param addr Start address of the region. - * @param size Size of the region to check. - * - * @return true if protected, otherwise false. - */ -bool memspi_region_protected(spi_flash_host_driver_t* driver, uint32_t addr, uint32_t size); \ No newline at end of file diff --git a/components/spi_flash/memspi_host_driver.c b/components/spi_flash/memspi_host_driver.c index 871616386..85b31998a 100644 --- a/components/spi_flash/memspi_host_driver.c +++ b/components/spi_flash/memspi_host_driver.c @@ -34,7 +34,6 @@ esp_err_t memspi_host_init_pointers(spi_flash_host_driver_t *host, memspi_host_d //some functions are not required if not SPI1 if (data->spi != &SPI1) { host->flush_cache = NULL; - host->region_protected = NULL; } return ESP_OK; } @@ -87,15 +86,4 @@ esp_err_t memspi_host_flush_cache(spi_flash_host_driver_t* driver, uint32_t addr spi_flash_check_and_flush_cache(addr, size); } return ESP_OK; -} - -bool memspi_region_protected(spi_flash_host_driver_t* driver, uint32_t addr, uint32_t size) -{ - if (((memspi_host_data_t*)(driver->driver_data))->spi != &SPI1) { - return false; - } - if (!esp_partition_main_flash_region_safe(addr, size)) { - return true; - } - return false; } \ No newline at end of file diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index b2e13c8e4..ad97c433c 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/components/spi_flash/spi_flash_os_func_app.c @@ -17,6 +17,8 @@ #include "esp_attr.h" #include "esp_spi_flash.h" //for ``g_flash_guard_default_ops`` #include "esp_flash.h" +#include "esp_flash_partitions.h" + /* * OS functions providing delay service and arbitration among chips, and with the cache. @@ -29,6 +31,11 @@ typedef struct { int host_id; } app_func_arg_t; +typedef struct { + int host_id; + bool no_protect; //to decide whether to check protected region (for the main chip) or not. +} spi1_app_func_arg_t; + // in the future we will have arbitration among devices, including flash on the same flash bus static IRAM_ATTR esp_err_t spi_bus_acquire(int host_id) { @@ -45,7 +52,7 @@ static IRAM_ATTR esp_err_t spi1_start(void *arg) { g_flash_guard_default_ops.start(); - spi_bus_acquire(((app_func_arg_t *)arg)->host_id); + spi_bus_acquire(((spi1_app_func_arg_t *)arg)->host_id); return ESP_OK; } @@ -53,7 +60,7 @@ static IRAM_ATTR esp_err_t spi1_end(void *arg) { g_flash_guard_default_ops.end(); - spi_bus_release(((app_func_arg_t *)arg)->host_id); + spi_bus_release(((spi1_app_func_arg_t *)arg)->host_id); return ESP_OK; } @@ -76,8 +83,24 @@ static IRAM_ATTR esp_err_t delay_ms(void *arg, unsigned ms) return ESP_OK; } -static DRAM_ATTR app_func_arg_t spi1_arg = { +static IRAM_ATTR esp_err_t main_flash_region_protected(void* arg, size_t start_addr, size_t size) +{ + if (((spi1_app_func_arg_t*)arg)->no_protect || esp_partition_main_flash_region_safe(start_addr, size)) { + //ESP_OK = 0, also means protected==0 + return ESP_OK; + } else { + return ESP_ERR_NOT_SUPPORTED; + } +} + +static DRAM_ATTR spi1_app_func_arg_t spi1_arg = { .host_id = 0, //for SPI1, + .no_protect = true, +}; + +static DRAM_ATTR spi1_app_func_arg_t main_flash_arg = { + .host_id = 0, //for SPI1, + .no_protect = false, }; static app_func_arg_t spi2_arg = { @@ -93,6 +116,7 @@ const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = { .start = spi1_start, .end = spi1_end, .delay_ms = delay_ms, + .region_protected = main_flash_region_protected, }; const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = { @@ -117,4 +141,11 @@ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id) return ESP_OK; } +esp_err_t esp_flash_app_init_os_functions(esp_flash_t* chip) +{ + chip->os_func = &esp_flash_spi1_default_os_functions; + chip->os_func_data = &main_flash_arg; + return ESP_OK; +} + diff --git a/components/spi_flash/spi_flash_os_func_noos.c b/components/spi_flash/spi_flash_os_func_noos.c index 4b494279b..910633f9b 100644 --- a/components/spi_flash/spi_flash_os_func_noos.c +++ b/components/spi_flash/spi_flash_os_func_noos.c @@ -20,13 +20,15 @@ #include "esp_attr.h" -static esp_err_t start(void *arg) + +static IRAM_ATTR esp_err_t start(void *arg) { Cache_Read_Disable(0); Cache_Read_Disable(1); return ESP_OK; } -static esp_err_t end(void *arg) + +static IRAM_ATTR esp_err_t end(void *arg) { Cache_Flush(0); Cache_Flush(1); @@ -35,14 +37,21 @@ static esp_err_t end(void *arg) return ESP_OK; } -static esp_err_t delay_ms(void *arg, unsigned ms) +static IRAM_ATTR esp_err_t delay_ms(void *arg, unsigned ms) { ets_delay_us(1000 * ms); return ESP_OK; } -const esp_flash_os_functions_t esp_flash_noos_functions = { +const DRAM_ATTR esp_flash_os_functions_t esp_flash_noos_functions = { .start = start, .end = end, .delay_ms = delay_ms, + .region_protected = NULL, }; + +esp_err_t IRAM_ATTR esp_flash_app_disable_os_functions(esp_flash_t* chip) +{ + chip->os_func = &esp_flash_noos_functions; + return ESP_OK; +}