From 5bac46b3e80d8193e79ceead6ddfb24d52050090 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Mon, 20 Mar 2023 12:44:31 +0800 Subject: [PATCH] MMU: Fix stale data being read on memory mapped instruction page --- components/esp_mm/esp_mmu_map.c | 37 +++++++++ components/esp_mm/include/esp_mmu_map.h | 14 ++++ components/spi_flash/esp_flash_api.c | 4 +- components/spi_flash/flash_mmap.c | 78 ++++++------------- .../flash_mmap/main/test_flash_mmap.c | 16 ++++ 5 files changed, 92 insertions(+), 57 deletions(-) diff --git a/components/esp_mm/esp_mmu_map.c b/components/esp_mm/esp_mmu_map.c index c8ec6c6a8c..b7d927f8fe 100644 --- a/components/esp_mm/esp_mmu_map.c +++ b/components/esp_mm/esp_mmu_map.c @@ -326,6 +326,43 @@ esp_err_t esp_mmu_map_reserve_block_with_caps(size_t size, mmu_mem_caps_t caps, return ESP_OK; } +IRAM_ATTR esp_err_t esp_mmu_paddr_find_caps(const esp_paddr_t paddr, mmu_mem_caps_t *out_caps) +{ + mem_region_t *region = NULL; + mem_block_t *mem_block = NULL; + bool found = false; + mem_block_t *found_block = NULL; + if (out_caps == NULL) { + return ESP_ERR_INVALID_ARG; + } + + + for (int i = 0; i < s_mmu_ctx.num_regions; i++) { + region = &s_mmu_ctx.mem_regions[i]; + + TAILQ_FOREACH(mem_block, ®ion->mem_block_head, entries) { + if (mem_block == TAILQ_FIRST(®ion->mem_block_head) || mem_block == TAILQ_LAST(®ion->mem_block_head, mem_block_head_)) { + //we don't care the dummy_head and the dummy_tail + continue; + } + + //now we are only traversing the actual dynamically allocated blocks, dummy_head and dummy_tail are excluded already + if (mem_block->paddr_start == paddr) { + found = true; + found_block = mem_block; + break; + } + } + } + + if (!found) { + return ESP_ERR_NOT_FOUND; + } + + *out_caps = found_block->caps; + return ESP_OK; +} + static void IRAM_ATTR NOINLINE_ATTR s_do_cache_invalidate(uint32_t vaddr_start, uint32_t size) { diff --git a/components/esp_mm/include/esp_mmu_map.h b/components/esp_mm/include/esp_mmu_map.h index 75f37f4040..355b0c9750 100644 --- a/components/esp_mm/include/esp_mmu_map.h +++ b/components/esp_mm/include/esp_mmu_map.h @@ -156,6 +156,20 @@ esp_err_t esp_mmu_vaddr_to_paddr(void *vaddr, esp_paddr_t *out_paddr, mmu_target */ esp_err_t esp_mmu_paddr_to_vaddr(esp_paddr_t paddr, mmu_target_t target, mmu_vaddr_t type, void **out_vaddr); +/** + * @brief If the physical address is mapped, this API will provide the capabilities of the virtual address where the physical address is mapped to. + * + * @note: Only return value is ESP_OK(which means physically address is successfully mapped), then caps you get make sense. + * @note This API only check one page (see CONFIG_MMU_PAGE_SIZE), starting from the `paddr` + * + * @param[in] paddr Physical address + * @param[out] out_caps Bitwise OR of MMU_MEM_CAP_* flags indicating the capabilities of a virtual address where the physical address is mapped to. + * @return + * - ESP_OK: Physical address successfully mapped. + * - ESP_ERR_INVALID_ARG: Null pointer + * - ESP_ERR_NOT_FOUND: Physical address is not mapped successfully. + */ +esp_err_t esp_mmu_paddr_find_caps(const esp_paddr_t paddr, mmu_mem_caps_t *out_caps); #ifdef __cplusplus } diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index e00f946d6b..3cd3920016 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/components/spi_flash/esp_flash_api.c @@ -145,7 +145,7 @@ static esp_err_t IRAM_ATTR spiflash_end_default(esp_flash_t *chip, esp_err_t err } // check that the 'chip' parameter is properly initialised -static esp_err_t check_chip_pointer_default(esp_flash_t **inout_chip) +static IRAM_ATTR esp_err_t check_chip_pointer_default(esp_flash_t **inout_chip) { esp_flash_t *chip = *inout_chip; if (chip == NULL) { @@ -182,7 +182,7 @@ static IRAM_ATTR esp_err_t flash_end_flush_cache(esp_flash_t* chip, esp_err_t er static esp_err_t detect_spi_flash_chip(esp_flash_t *chip); -bool esp_flash_chip_driver_initialized(const esp_flash_t *chip) +bool IRAM_ATTR esp_flash_chip_driver_initialized(const esp_flash_t *chip) { if (!chip->chip_drv) return false; return true; diff --git a/components/spi_flash/flash_mmap.c b/components/spi_flash/flash_mmap.c index 3616729725..204b51fa39 100644 --- a/components/spi_flash/flash_mmap.c +++ b/components/spi_flash/flash_mmap.c @@ -13,37 +13,24 @@ #include "esp_attr.h" #include "esp_log.h" #include "hal/mmu_ll.h" +#include "hal/mmu_hal.h" +#include "hal/cache_hal.h" #include "soc/mmu.h" #include "esp_private/esp_mmu_map_private.h" #include "esp_mmu_map.h" +#include "esp_rom_spiflash.h" #if CONFIG_SPIRAM #include "esp_private/esp_psram_extram.h" #include "esp_private/mmu_psram_flash.h" #endif -#include "esp_private/cache_utils.h" -#include "spi_flash_mmap.h" - - #if CONFIG_IDF_TARGET_ESP32 -#include "esp32/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32S2 -#include "esp32s2/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32S3 -#include "esp32s3/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32C3 -#include "esp32c3/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32H4 -#include "esp32h4/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32C2 -#include "esp32c2/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32C6 -#include "esp32c6/rom/cache.h" -#elif CONFIG_IDF_TARGET_ESP32H2 -#include "esp32h2/rom/cache.h" +#include "esp_private/esp_cache_esp32_private.h" #endif +#include "esp_private/cache_utils.h" +#include "spi_flash_mmap.h" #if CONFIG_SPIRAM_FETCH_INSTRUCTIONS extern int _instruction_reserved_start; @@ -342,35 +329,24 @@ const void * spi_flash_phys2cache(size_t phys_offs, spi_flash_mmap_memory_t memo return (const void *)ptr; } - -static bool IRAM_ATTR is_page_mapped_in_cache(uint32_t phys_page, const void **out_ptr) +static bool IRAM_ATTR is_page_mapped_in_cache(uint32_t phys_addr, const void **out_ptr) { - int start[2], end[2]; - *out_ptr = NULL; + mmu_mem_caps_t caps = 0; - /* SPI_FLASH_MMAP_DATA */ - start[0] = SOC_MMU_DROM0_PAGES_START; - end[0] = SOC_MMU_DROM0_PAGES_END; - - /* SPI_FLASH_MMAP_INST */ - start[1] = SOC_MMU_PRO_IRAM0_FIRST_USABLE_PAGE; - end[1] = SOC_MMU_IROM0_PAGES_END; - - for (int j = 0; j < 2; j++) { - for (int i = start[j]; i < end[j]; i++) { - uint32_t entry_pro = mmu_ll_read_entry(MMU_TABLE_CORE0, i); - if (entry_pro == SOC_MMU_PAGE_IN_FLASH(phys_page)) { + esp_err_t err = esp_mmu_paddr_find_caps(phys_addr, &caps); + if (err == ESP_OK) { + // On ESP32, we will always flush all, so always return true, and don't care the vaddr #if !CONFIG_IDF_TARGET_ESP32 - if (j == 0) { /* SPI_FLASH_MMAP_DATA */ - *out_ptr = (const void *)(SOC_MMU_VADDR0_START_ADDR + SPI_FLASH_MMU_PAGE_SIZE * (i - start[0])); - } else { /* SPI_FLASH_MMAP_INST */ - *out_ptr = (const void *)(SOC_MMU_VADDR1_FIRST_USABLE_ADDR + SPI_FLASH_MMU_PAGE_SIZE * (i - start[1])); - } -#endif - return true; - } + uint32_t vaddr = 0; + if (caps & MMU_MEM_CAP_EXEC) { + mmu_hal_paddr_to_vaddr(0, phys_addr, MMU_TARGET_FLASH0, MMU_VADDR_INSTRUCTION, &vaddr); + } else { + mmu_hal_paddr_to_vaddr(0, phys_addr, MMU_TARGET_FLASH0, MMU_VADDR_DATA, &vaddr); } + *out_ptr = (void *)vaddr; +#endif + return true; } return false; } @@ -384,26 +360,18 @@ IRAM_ATTR bool spi_flash_check_and_flush_cache(size_t start_addr, size_t length) length += (start_addr - page_start_addr); length = (length + SPI_FLASH_MMU_PAGE_SIZE - 1) & ~(SPI_FLASH_MMU_PAGE_SIZE-1); for (uint32_t addr = page_start_addr; addr < page_start_addr + length; addr += SPI_FLASH_MMU_PAGE_SIZE) { - uint32_t page = addr / SPI_FLASH_MMU_PAGE_SIZE; - // TODO: IDF-4969 - if (page >= 256) { + if (addr >= g_rom_flashchip.chip_size) { return false; /* invalid address */ } const void *vaddr = NULL; - if (is_page_mapped_in_cache(page, &vaddr)) { + if (is_page_mapped_in_cache(addr, &vaddr)) { #if CONFIG_IDF_TARGET_ESP32 -#if CONFIG_SPIRAM - esp_psram_extram_writeback_cache(); -#endif - Cache_Flush(0); -#ifndef CONFIG_FREERTOS_UNICORE - Cache_Flush(1); -#endif + cache_sync(); return true; #else // CONFIG_IDF_TARGET_ESP32 if (vaddr != NULL) { - Cache_Invalidate_Addr((uint32_t)vaddr, SPI_FLASH_MMU_PAGE_SIZE); + cache_hal_invalidate_addr((uint32_t)vaddr, SPI_FLASH_MMU_PAGE_SIZE); ret = true; } #endif // CONFIG_IDF_TARGET_ESP32 diff --git a/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c b/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c index 836aa6c674..548de17285 100644 --- a/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c +++ b/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c @@ -449,4 +449,20 @@ TEST_CASE("no stale data read post mmap and write partition", "[spi_flash][mmap] esp_partition_munmap(handle); TEST_ASSERT_EQUAL(0, memcmp(buf, read_data, sizeof(buf))); +#if !CONFIG_SPI_FLASH_ROM_IMPL //flash mmap API in ROM does not support mmap into instruction address + // Repeat the test for instruction mmap part + setup_mmap_tests(); + TEST_ESP_OK(esp_partition_mmap(p, 0, SPI_FLASH_MMU_PAGE_SIZE, + ESP_PARTITION_MMAP_INST, (const void **) &data, &handle) ); + memcpy(read_data, data, sizeof(read_data)); + TEST_ESP_OK(esp_partition_erase_range(p, 0, SPI_FLASH_MMU_PAGE_SIZE)); + /* not using esp_partition_write here, since the partition in not marked as "encrypted" + in the partition table */ + TEST_ESP_OK(spi_flash_write_maybe_encrypted(p->address + 0, buf, sizeof(buf))); + /* This should retrigger actual flash content read */ + memcpy(read_data, data, sizeof(read_data)); + + esp_partition_munmap(handle); + TEST_ASSERT_EQUAL(0, memcmp(buf, read_data, sizeof(buf))); +#endif }