Merge branch 'feature/spi_flash_write_16bytes' into 'master'

SPI Flash: Allow 16 byte aligned encrypted writes

Also includes some improved documentation

See merge request !456

Author: igrr

components/spi_flash/cache_utils.h

@@ -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

components/spi_flash/flash_mmap.c

@@ -39,13 +39,21 @@
 
 #define REGIONS_COUNT 4
 #define PAGES_PER_REGION 64
-#define FLASH_PAGE_SIZE 0x10000
 #define INVALID_ENTRY_VAL 0x100
 #define VADDR0_START_ADDR 0x3F400000
 #define VADDR1_START_ADDR 0x40000000
 #define VADDR1_FIRST_USABLE_ADDR 0x400D0000
-#define PRO_IRAM0_FIRST_USABLE_PAGE ((VADDR1_FIRST_USABLE_ADDR - VADDR1_START_ADDR) / FLASH_PAGE_SIZE + 64)
+#define PRO_IRAM0_FIRST_USABLE_PAGE ((VADDR1_FIRST_USABLE_ADDR - VADDR1_START_ADDR) / SPI_FLASH_MMU_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 +99,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();
     }
@@ -111,8 +123,8 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_
         region_addr = VADDR1_FIRST_USABLE_ADDR;
     }
     // region which should be mapped
-    int phys_page = src_addr / FLASH_PAGE_SIZE;
-    int page_count = (size + FLASH_PAGE_SIZE - 1) / FLASH_PAGE_SIZE;
+    int phys_page = src_addr / SPI_FLASH_MMU_PAGE_SIZE;
+    int page_count = (size + SPI_FLASH_MMU_PAGE_SIZE - 1) / SPI_FLASH_MMU_PAGE_SIZE;
     // The following part searches for a range of MMU entries which can be used.
     // Algorithm is essentially naïve strstr algorithm, except that unused MMU
     // entries are treated as wildcards.
@@ -158,7 +170,7 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_
         new_entry->count = page_count;
         new_entry->handle = ++s_mmap_last_handle;
         *out_handle = new_entry->handle;
-        *out_ptr = (void*) (region_addr + start * FLASH_PAGE_SIZE);
+        *out_ptr = (void*) (region_addr + start * SPI_FLASH_MMU_PAGE_SIZE);
         ret = ESP_OK;
     }
     spi_flash_enable_interrupts_caches_and_other_cpu();
@@ -212,3 +224,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 += SPI_FLASH_MMU_PAGE_SIZE) {
+        int page = addr / SPI_FLASH_MMU_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));
+        }
+    }
+}

components/spi_flash/flash_ops.c

@@ -90,7 +90,7 @@ size_t spi_flash_get_chip_size()
     return g_rom_flashchip.chip_size;
 }
 
-SpiFlashOpResult IRAM_ATTR spi_flash_unlock()
+static SpiFlashOpResult IRAM_ATTR spi_flash_unlock()
 {
     static bool unlocked = false;
     if (!unlocked) {
@@ -250,42 +250,80 @@ 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);
 }
 
 esp_err_t IRAM_ATTR spi_flash_write_encrypted(size_t dest_addr, const void *src, size_t size)
 {
-    if ((dest_addr % 32) != 0) {
+    const uint8_t *ssrc = (const uint8_t *)src;
+    if ((dest_addr % 16) != 0) {
         return ESP_ERR_INVALID_ARG;
     }
-    if ((size % 32) != 0) {
+    if ((size % 16) != 0) {
         return ESP_ERR_INVALID_SIZE;
     }
-    if ((uint32_t) src < 0x3ff00000) {
-        // if source address is in DROM, we won't be able to read it
-        // from within SPIWrite
-        // TODO: consider buffering source data using heap and writing it anyway?
-        return ESP_ERR_INVALID_ARG;
-    }
+
     COUNTER_START();
     spi_flash_disable_interrupts_caches_and_other_cpu();
     SpiFlashOpResult rc;
     rc = spi_flash_unlock();
+    spi_flash_enable_interrupts_caches_and_other_cpu();
+
     if (rc == SPI_FLASH_RESULT_OK) {
         /* SPI_Encrypt_Write encrypts data in RAM as it writes,
            so copy to a temporary buffer - 32 bytes at a time.
+
+           Each call to SPI_Encrypt_Write takes a 32 byte "row" of
+           data to encrypt, and each row is two 16 byte AES blocks
+           that share a key (as derived from flash address).
         */
-        uint32_t encrypt_buf[32/sizeof(uint32_t)];
-        for (size_t i = 0; i < size; i += 32) {
-            memcpy(encrypt_buf, ((const uint8_t *)src) + i, 32);
-            rc = SPI_Encrypt_Write((uint32_t) dest_addr + i, encrypt_buf, 32);
+        uint8_t encrypt_buf[32] __attribute__((aligned(4)));
+        uint32_t row_size;
+        for (size_t i = 0; i < size; i += row_size) {
+            uint32_t row_addr = dest_addr + i;
+            if (i == 0 && (row_addr % 32) != 0) {
+                /* writing to second block of a 32 byte row */
+                row_size = 16;
+                row_addr -= 16;
+                /* copy to second block in buffer */
+                memcpy(encrypt_buf + 16, ssrc + i, 16);
+                /* decrypt the first block from flash, will reencrypt to same bytes */
+                spi_flash_read_encrypted(row_addr, encrypt_buf, 16);
+            }
+            else if (size - i == 16) {
+                /* 16 bytes left, is first block of a 32 byte row */
+                row_size = 16;
+                /* copy to first block in buffer */
+                memcpy(encrypt_buf, ssrc + i, 16);
+                /* decrypt the second block from flash, will reencrypt to same bytes */
+                spi_flash_read_encrypted(row_addr + 16, encrypt_buf + 16, 16);
+            }
+            else {
+                /* Writing a full 32 byte row (2 blocks) */
+                row_size = 32;
+                memcpy(encrypt_buf, ssrc + i, 32);
+            }
+
+            spi_flash_disable_interrupts_caches_and_other_cpu();
+            rc = SPI_Encrypt_Write(row_addr, (uint32_t *)encrypt_buf, 32);
+            spi_flash_enable_interrupts_caches_and_other_cpu();
             if (rc != SPI_FLASH_RESULT_OK) {
                 break;
             }
         }
         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);
 }
 
@@ -381,6 +419,31 @@ esp_err_t IRAM_ATTR spi_flash_read(size_t src, void *dstv, size_t size)
     return spi_flash_translate_rc(rc);
 }
 
+esp_err_t IRAM_ATTR spi_flash_read_encrypted(size_t src, void *dstv, size_t size)
+{
+    if (src + size > g_rom_flashchip.chip_size) {
+        return ESP_ERR_INVALID_SIZE;
+    }
+    if (size == 0) {
+        return ESP_OK;
+    }
+
+    esp_err_t err;
+    const uint8_t *map;
+    spi_flash_mmap_handle_t map_handle;
+    size_t map_src = src & ~(SPI_FLASH_MMU_PAGE_SIZE-1);
+    size_t map_size = size + (src - map_src);
+
+    err = spi_flash_mmap(map_src, map_size, SPI_FLASH_MMAP_DATA, (const void **)&map, &map_handle);
+    if (err != ESP_OK) {
+        return err;
+    }
+    memcpy(dstv, map + (src - map_src), size);
+    spi_flash_munmap(map_handle);
+    return err;
+}
+
+
 static esp_err_t spi_flash_translate_rc(SpiFlashOpResult rc)
 {
     switch (rc) {

components/spi_flash/include/esp_partition.h

@@ -191,6 +191,13 @@ esp_err_t esp_partition_read(const esp_partition_t* partition,
  * Before writing data to flash, corresponding region of flash needs to be erased.
  * This can be done using esp_partition_erase_range function.
  *
+ * Partitions marked with an encryption flag will automatically be
+ * written via the spi_flash_write_encrypted() function. If writing to
+ * an encrypted partition, all write offsets and lengths must be
+ * multiples of 16 bytes. See the spi_flash_write_encrypted() function
+ * for more details. Unencrypted partitions do not have this
+ * restriction.
+ *
  * @param partition Pointer to partition structure obtained using
  *                  esp_partition_find_first or esp_partition_get.
  *                  Must be non-NULL.

components/spi_flash/include/esp_spi_flash.h

@@ -31,6 +31,8 @@ extern "C" {
 
 #define SPI_FLASH_SEC_SIZE  4096    /**< SPI Flash sector size */
 
+#define SPI_FLASH_MMU_PAGE_SIZE 0x10000 /**< Flash cache MMU mapping page size */
+
 /**
  * @brief  Initialize SPI flash access driver
  *
@@ -92,14 +94,18 @@ esp_err_t spi_flash_write(size_t dest_addr, const void *src, size_t size);
  *
  * @note Flash encryption must be enabled for this function to work.
  *
- * @note Address in flash, dest, has to be 32-byte aligned.
+ * @note Flash encryption must be enabled when calling this function.
+ * If flash encryption is disabled, the function returns
+ * ESP_ERR_INVALID_STATE.  Use esp_flash_encryption_enabled()
+ * function to determine if flash encryption is enabled.
  *
- * @note If source address is in DROM, this function will return
- *       ESP_ERR_INVALID_ARG.
+ * @note Both dest_addr and size must be multiples of 16 bytes. For
+ * absolute best performance, both dest_addr and size arguments should
+ * be multiples of 32 bytes.
  *
- * @param  dest_addr destination address in Flash. Must be a multiple of 32 bytes.
+ * @param  dest_addr destination address in Flash. Must be a multiple of 16 bytes.
  * @param  src       pointer to the source buffer.
- * @param  size      length of data, in bytes. Must be a multiple of 32 bytes.
+ * @param  size      length of data, in bytes. Must be a multiple of 16 bytes.
  *
  * @return esp_err_t
  */
@@ -116,6 +122,23 @@ esp_err_t spi_flash_write_encrypted(size_t dest_addr, const void *src, size_t si
  */
 esp_err_t spi_flash_read(size_t src_addr, void *dest, size_t size);
 
+
+/**
+ * @brief  Read data from Encrypted Flash.
+ *
+ * If flash encryption is enabled, this function will transparently decrypt data as it is read.
+ * If flash encryption is not enabled, this function behaves the same as spi_flash_read().
+ *
+ * See esp_flash_encryption_enabled() for a function to check if flash encryption is enabled.
+ *
+ * @param  src   source address of the data in Flash.
+ * @param  dest  pointer to the destination buffer
+ * @param  size  length of data
+ *
+ * @return esp_err_t
+ */
+esp_err_t spi_flash_read_encrypted(size_t src, void *dest, size_t size);
+
 /**
  * @brief Enumeration which specifies memory space requested in an mmap call
  */
@@ -140,7 +163,8 @@ typedef uint32_t spi_flash_mmap_handle_t;
  * page allocation, use spi_flash_mmap_dump function.
  *
  * @param src_addr  Physical address in flash where requested region starts.
- *                  This address *must* be aligned to 64kB boundary.
+ *                  This address *must* be aligned to 64kB boundary
+ *                  (SPI_FLASH_MMU_PAGE_SIZE).
  * @param size  Size of region which has to be mapped. This size will be rounded
  *              up to a 64k boundary.
  * @param memory  Memory space where the region should be mapped

components/spi_flash/test/test_config.h

@@ -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