]> granicus.if.org Git - esp-idf/commitdiff
spi_flash: Flush flash cache if flash_mmap()ing a written-to page
authorAngus Gratton <angus@espressif.com>
Thu, 5 Jan 2017 04:51:02 +0000 (15:51 +1100)
committerIvan Grokhotkov <ivan@espressif.com>
Fri, 20 Jan 2017 11:48:46 +0000 (19:48 +0800)
Without this, it's possible for stale information to be read from
cache via mmap, even if the MMU table entry had been invalidated
prior to writing flash (if  the same MMU table entry was re-used after
writing flash.)

components/spi_flash/cache_utils.h
components/spi_flash/flash_mmap.c
components/spi_flash/flash_ops.c
components/spi_flash/test/test_config.h [new file with mode: 0644]
components/spi_flash/test/test_mmap.c
components/spi_flash/test/test_read_write.c
components/spi_flash/test/test_spi_flash.c

index 598b8fba77884c0a51f31388282ec6cf8ba8cfdf..9cd9ad6b495b69da88347f52a167126c26b6f104 100644 (file)
@@ -48,4 +48,12 @@ void spi_flash_disable_interrupts_caches_and_other_cpu_no_os();
 // This function is implied to be called when other CPU is not running or running code from IRAM.
 void spi_flash_enable_interrupts_caches_no_os();
 
+// Mark the pages containing a flash region as having been
+// erased or written to. This means the flash cache needs
+// to be evicted before these pages can be flash_mmap()ed again,
+// as they may contain stale data
+//
+// Only call this while holding spi_flash_op_lock()
+void spi_flash_mark_modified_region(uint32_t start_addr, uint32_t length);
+
 #endif //ESP_SPI_FLASH_CACHE_UTILS_H
index 15f75f36344d259b4ba8d4afaca8db99b59a2677..0b83ac33c0c1d84724a743d9c8bb4492118ab2d7 100644 (file)
 #define VADDR1_FIRST_USABLE_ADDR 0x400D0000
 #define PRO_IRAM0_FIRST_USABLE_PAGE ((VADDR1_FIRST_USABLE_ADDR - VADDR1_START_ADDR) / FLASH_PAGE_SIZE + 64)
 
+/* Ensure pages in a region haven't been marked as written via
+   spi_flash_mark_modified_region(). If the page has
+   been written, flush the entire flash cache before returning.
+
+   This ensures stale cache entries are never read after fresh calls
+   to spi_flash_mmap(), while keeping the number of cache flushes to a
+   minimum.
+*/
+static void spi_flash_ensure_unmodified_region(size_t start_addr, size_t length);
 
 typedef struct mmap_entry_{
     uint32_t handle;
@@ -91,7 +100,11 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_
     if (src_addr + size > g_rom_flashchip.chip_size) {
         return ESP_ERR_INVALID_ARG;
     }
+
     spi_flash_disable_interrupts_caches_and_other_cpu();
+
+    spi_flash_ensure_unmodified_region(src_addr, size);
+
     if (s_mmap_page_refcnt[0] == 0) {
         spi_flash_mmap_init();
     }
@@ -212,3 +225,59 @@ void spi_flash_mmap_dump()
         }
     }
 }
+
+/* 256-bit (up to 16MB of 64KB pages) bitset of all flash pages
+   that have been written to since last cache flush.
+
+   Before mmaping a page, need to flush caches if that page has been
+   written to.
+
+   Note: It's possible to do some additional performance tweaks to
+   this algorithm, as we actually only need to flush caches if a page
+   was first mmapped, then written to, then is about to be mmaped a
+   second time. This is a fair bit more complex though, so unless
+   there's an access pattern that this would significantly boost then
+   it's probably not worth it.
+*/
+static uint32_t written_pages[256/32];
+
+static void update_written_pages(size_t start_addr, size_t length, bool mark);
+
+void IRAM_ATTR spi_flash_mark_modified_region(size_t start_addr, size_t length)
+{
+    update_written_pages(start_addr, length, true);
+}
+
+static void IRAM_ATTR spi_flash_ensure_unmodified_region(size_t start_addr, size_t length)
+{
+    update_written_pages(start_addr, length, false);
+}
+
+/* generic implementation for the previous two functions */
+static inline IRAM_ATTR void update_written_pages(size_t start_addr, size_t length, bool mark)
+{
+    for (uint32_t addr = start_addr; addr < start_addr + length; addr += FLASH_PAGE_SIZE) {
+        int page = addr / FLASH_PAGE_SIZE;
+        if (page >= 256) {
+            return; /* invalid address */
+        }
+
+        int idx = page / 32;
+        uint32_t bit = 1 << (page % 32);
+
+        if (mark) {
+            written_pages[idx] |= bit;
+        } else if (written_pages[idx] & bit) {
+            /* it is tempting to write a version of this that only
+               flushes each CPU's cache as needed. However this is
+               tricky because mmaped memory can be used on un-pinned
+               cores, or the pointer passed between CPUs.
+            */
+            Cache_Flush(0);
+#ifndef CONFIG_FREERTOS_UNICORE
+            Cache_Flush(1);
+#endif
+            bzero(written_pages, sizeof(written_pages));
+        }
+    }
+}
index 22070eb5ffda7cd75733779117876a379046d06d..4bcb7b3aef94fdcc16f588e49ee92c8500ebf5d2 100644 (file)
@@ -250,6 +250,11 @@ esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size)
     }
 out:
     COUNTER_STOP(write);
+
+    spi_flash_op_lock();
+    spi_flash_mark_modified_region(dst, size);
+    spi_flash_op_unlock();
+
     return spi_flash_translate_rc(rc);
 }
 
@@ -286,6 +291,11 @@ esp_err_t IRAM_ATTR spi_flash_write_encrypted(size_t dest_addr, const void *src,
         bzero(encrypt_buf, sizeof(encrypt_buf));
     }
     COUNTER_ADD_BYTES(write, size);
+
+    spi_flash_op_lock();
+    spi_flash_mark_modified_region(dest_addr, size);
+    spi_flash_op_unlock();
+
     return spi_flash_translate_rc(rc);
 }
 
diff --git a/components/spi_flash/test/test_config.h b/components/spi_flash/test/test_config.h
new file mode 100644 (file)
index 0000000..45e7366
--- /dev/null
@@ -0,0 +1,24 @@
+// Copyright 2010-2016 Espressif Systems (Shanghai) PTE LTD
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Common header for SPI flash test data
+#pragma once
+
+/* Define a region of flash we can mess up for testing...
+
+   This is pretty ugly, better to do something with a partition but
+   this is OK for now.
+ */
+#define TEST_REGION_START 0x180000
+#define TEST_REGION_END   0x1E0000
index d3d16802eac09e1eaeae2d0a246a83dd0a8b125c..464a876428c90fac610329171d1f663b8a0faa7d 100644 (file)
@@ -1,5 +1,6 @@
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <freertos/FreeRTOS.h>
 #include <freertos/task.h>
 #include <freertos/semphr.h>
@@ -7,14 +8,17 @@
 #include <unity.h>
 #include <esp_spi_flash.h>
 #include <esp_attr.h>
+#include <esp_flash_encrypt.h>
 
-uint32_t buffer[1024];
+#include "test_config.h"
 
+static uint32_t buffer[1024];
+
+/* read-only region used for mmap tests */
 static const uint32_t start = 0x100000;
 static const uint32_t end = 0x200000;
 
 
-
 TEST_CASE("Prepare data for mmap tests", "[mmap]")
 {
     srand(0);
@@ -81,3 +85,49 @@ TEST_CASE("Can mmap into data address space", "[mmap]")
     printf("Unmapping handle3\n");
     spi_flash_munmap(handle3);
 }
+
+TEST_CASE("flash_mmap invalidates just-written data", "[spi_flash]")
+{
+    spi_flash_mmap_handle_t handle1;
+    const void *ptr1;
+
+    const size_t test_size = 128;
+
+    if (esp_flash_encryption_enabled()) {
+        TEST_IGNORE_MESSAGE("flash encryption enabled, spi_flash_write_encrypted() test won't pass as-is");
+    }
+
+    ESP_ERROR_CHECK( spi_flash_erase_sector(TEST_REGION_START / SPI_FLASH_SEC_SIZE) );
+
+    /* map erased test region to ptr1 */
+    ESP_ERROR_CHECK( spi_flash_mmap(TEST_REGION_START, test_size, SPI_FLASH_MMAP_DATA, &ptr1, &handle1) );
+    printf("mmap_res ptr1: handle=%d ptr=%p\n", handle1, ptr1);
+
+    /* verify it's all 0xFF */
+    for (int i = 0; i < test_size; i++) {
+        TEST_ASSERT_EQUAL_HEX(0xFF, ((uint8_t *)ptr1)[i]);
+    }
+
+    /* unmap the erased region */
+    spi_flash_munmap(handle1);
+
+    /* write flash region to 0xEE */
+    uint8_t buf[test_size];
+    memset(buf, 0xEE, test_size);
+    ESP_ERROR_CHECK( spi_flash_write(TEST_REGION_START, buf, test_size) );
+
+    /* re-map the test region at ptr1.
+
+       this is a fresh mmap call so should trigger a cache flush,
+       ensuring we see the updated flash.
+    */
+    ESP_ERROR_CHECK( spi_flash_mmap(TEST_REGION_START, test_size, SPI_FLASH_MMAP_DATA, &ptr1, &handle1) );
+    printf("mmap_res ptr1 #2: handle=%d ptr=%p\n", handle1, ptr1);
+
+    /* assert that ptr1 now maps to the new values on flash,
+       ie contents of buf array.
+    */
+    TEST_ASSERT_EQUAL_HEX8_ARRAY(buf, ptr1, test_size);
+
+    spi_flash_munmap(handle1);
+}
index c9067463e41dede39d1f7fc0a2142d59b47eb642..37edbbb944cba0158c14f2481c817ca39d03f80a 100644 (file)
 #include "soc/timer_group_struct.h"
 #include "soc/timer_group_reg.h"
 
+#include "test_config.h"
+
 /* Base offset in flash for tests. */
-#define FLASH_BASE 0x120000
+#define FLASH_BASE TEST_REGION_START
 
 #ifndef CONFIG_SPI_FLASH_MINIMAL_TEST
 #define CONFIG_SPI_FLASH_MINIMAL_TEST 1
index ecc472ac0e4da54e87ca4fcc2f270d75d5241ab7..90d0cc1fdcb35a2ecbb10be47fde3175bb905aa7 100644 (file)
@@ -7,6 +7,8 @@
 #include <esp_spi_flash.h>
 #include <esp_attr.h>
 
+#include "test_config.h"
+
 struct flash_test_ctx {
     uint32_t offset;
     bool fail;
@@ -31,8 +33,7 @@ static void flash_test_task(void *arg)
     vTaskDelay(0 / portTICK_PERIOD_MS);
 
     uint32_t val = 0xabcd1234;
-    const uint32_t n = 4096;
-    for (uint32_t offset = 0; offset < n; offset += 4) {
+    for (uint32_t offset = 0; offset < SPI_FLASH_SEC_SIZE; offset += 4) {
         if (spi_flash_write(sector * SPI_FLASH_SEC_SIZE + offset, (const uint8_t *) &val, 4) != ESP_OK) {
             printf("Write failed at offset=%d\r\n", offset);
             ctx->fail = true;
@@ -44,7 +45,7 @@ static void flash_test_task(void *arg)
     vTaskDelay(0 / portTICK_PERIOD_MS);
 
     uint32_t val_read;
-    for (uint32_t offset = 0; offset < n; offset += 4) {
+    for (uint32_t offset = 0; offset < SPI_FLASH_SEC_SIZE; offset += 4) {
         if (spi_flash_read(sector * SPI_FLASH_SEC_SIZE + offset, (uint8_t *) &val_read, 4) != ESP_OK) {
             printf("Read failed at offset=%d\r\n", offset);
             ctx->fail = true;