Merge branch 'bugfix/flash_rodata_any_alignement_v4.2' into 'release/v4.2'

build: Fix cache issue and add dedicated section for (Custom) App version info (backport v4.2)

See merge request espressif/esp-idf!13465

components/esp32/ld/esp32.ld

@@ -134,3 +134,12 @@ REGION_ALIAS("rtc_data_location", rtc_data_seg );
 #else
   REGION_ALIAS("default_rodata_seg", dram0_0_seg);
 #endif // CONFIG_APP_BUILD_USE_FLASH_SECTIONS
+
+/**
+ *  If rodata default segment is placed in `drom0_0_seg`, then flash's first rodata section must
+ *  also be first in the segment.
+ */
+#ifdef CONFIG_APP_BUILD_USE_FLASH_SECTIONS
+  ASSERT(_rodata_start == ORIGIN(default_rodata_seg),
+         ".flash.appdesc section must be placed at the beginning of the rodata segment.")
+#endif

components/esp32/ld/esp32.project.ld.in

@@ -249,14 +249,23 @@ SECTIONS
   ASSERT(((_bss_end - ORIGIN(dram0_0_seg)) <= LENGTH(dram0_0_seg)),
           "DRAM segment data does not fit.")
 
-  /* When modifying the alignment, update tls_section_alignment in pxPortInitialiseStack */
+  .flash.appdesc : ALIGN(0x10)
-  .flash.rodata : ALIGN(0x10)
   {
     _rodata_start = ABSOLUTE(.);
 
     *(.rodata_desc .rodata_desc.*)               /* Should be the first.  App version info.        DO NOT PUT ANYTHING BEFORE IT! */
     *(.rodata_custom_desc .rodata_custom_desc.*) /* Should be the second. Custom app version info. DO NOT PUT ANYTHING BEFORE IT! */
 
+    /* Create an empty gap within this section. Thanks to this, the end of this
+     * section will match .flah.rodata's begin address. Thus, both sections
+     * will be merged when creating the final bin image. */
+    . = ALIGN(ALIGNOF(.flash.rodata));
+  } >default_rodata_seg
+
+  .flash.rodata : ALIGN(0x10)
+  {
+    _flash_rodata_start = ABSOLUTE(.);
+
     mapping[flash_rodata]
 
     *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */
@@ -312,6 +321,8 @@ SECTIONS
     . = ALIGN(4);
   } >default_rodata_seg
 
+  _flash_rodata_align = ALIGNOF(.flash.rodata);
+
   .flash.text :
   {
     _stext = .;

components/esp32s2/ld/esp32s2.ld

@@ -120,3 +120,13 @@ REGION_ALIAS("rtc_data_location", rtc_data_seg );
 #else
   REGION_ALIAS("default_rodata_seg", dram0_0_seg);
 #endif // CONFIG_APP_BUILD_USE_FLASH_SECTIONS
+
+
+/**
+ *  If rodata default segment is placed in `drom0_0_seg`, then flash's first rodata section must
+ *  also be first in the segment.
+ */
+#ifdef CONFIG_APP_BUILD_USE_FLASH_SECTIONS
+  ASSERT(_rodata_reserved_start == ORIGIN(default_rodata_seg),
+         ".flash.appdesc section must be placed at the beginning of the rodata segment.")
+#endif

components/esp32s2/ld/esp32s2.project.ld.in

@@ -232,8 +232,7 @@ SECTIONS
     _heap_start = ABSOLUTE(.);
   } > dram0_0_seg
 
-  /* When modifying the alignment, update tls_section_alignment in pxPortInitialiseStack */
+  .flash.appdesc : ALIGN(0x10)
-  .flash.rodata : ALIGN(0x10)
   {
     _rodata_reserved_start = ABSOLUTE(.);
     _rodata_start = ABSOLUTE(.);
@@ -241,6 +240,16 @@ SECTIONS
     *(.rodata_desc .rodata_desc.*)               /* Should be the first.  App version info.        DO NOT PUT ANYTHING BEFORE IT! */
     *(.rodata_custom_desc .rodata_custom_desc.*) /* Should be the second. Custom app version info. DO NOT PUT ANYTHING BEFORE IT! */
 
+    /* Create an empty gap within this section. Thanks to this, the end of this
+     * section will match .flah.rodata's begin address. Thus, both sections
+     * will be merged when creating the final bin image. */
+    . = ALIGN(ALIGNOF(.flash.rodata));
+  } >default_rodata_seg
+
+  .flash.rodata : ALIGN(0x10)
+  {
+    _flash_rodata_start = ABSOLUTE(.);
+
     mapping[flash_rodata]
 
     *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */
@@ -297,6 +306,8 @@ SECTIONS
     . = ALIGN(4);
   } >default_rodata_seg
 
+  _flash_rodata_align = ALIGNOF(.flash.rodata);
+
   .flash.text :
   {
     _stext = .;

components/freertos/xtensa/port.c

@@ -164,7 +164,7 @@ StackType_t *pxPortInitialiseStack( StackType_t *pxTopOfStack, TaskFunction_t px
 	#endif
 	uint32_t *threadptr;
 	void *task_thread_local_start;
-	extern int _thread_local_start, _thread_local_end, _rodata_start;
+	extern int _thread_local_start, _thread_local_end, _flash_rodata_start, _flash_rodata_align;
 	// TODO: check that TLS area fits the stack
 	uint32_t thread_local_sz = (uint8_t *)&_thread_local_end - (uint8_t *)&_thread_local_start;
 
@@ -223,24 +223,62 @@ StackType_t *pxPortInitialiseStack( StackType_t *pxTopOfStack, TaskFunction_t px
 	frame->vpri = 0xFFFFFFFF;
 	#endif
 
-	/* Init threadptr reg and TLS vars */
+	/* Init threadptr register and set up TLS run-time area.
+	 * The following diagram illustrates the layout of link-time and run-time
+	 * TLS sections.
+	 *
+	 *          +-------------+
+	 *          |Section:     |      Linker symbols:
+	 *          |.flash.rodata|      ---------------
+	 *       0x0+-------------+ <-- _flash_rodata_start
+	 *        ^ |             |
+	 *        | | Other data  |
+	 *        | |     ...     |
+	 *        | +-------------+ <-- _thread_local_start
+	 *        | |.tbss        | ^
+	 *        v |             | |
+	 *    0xNNNN|int example; | | (thread_local_size)
+	 *          |.tdata       | v
+	 *          +-------------+ <-- _thread_local_end
+	 *          | Other data  |
+	 *          |     ...     |
+	 *          |             |
+	 *          +-------------+
+	 *
+	 *                                Local variables of
+	 *                              pxPortInitialiseStack
+	 *                             -----------------------
+	 *          +-------------+ <-- pxTopOfStack
+	 *          |.tdata (*)   |  ^
+	 *        ^ |int example; |  |(thread_local_size
+	 *        | |             |  |
+	 *        | |.tbss (*)    |  v
+	 *        | +-------------+ <-- task_thread_local_start
+	 * 0xNNNN | |             |  ^
+	 *        | |             |  |
+	 *        | |             |  |_thread_local_start - _rodata_start
+	 *        | |             |  |
+	 *        | |             |  v
+	 *        v +-------------+ <-- threadptr
+	 *
+	 *   (*) The stack grows downward!
+	 */
 	task_thread_local_start = (void *)(((uint32_t)pxTopOfStack - XT_CP_SIZE - thread_local_sz) & ~0xf);
 	memcpy(task_thread_local_start, &_thread_local_start, thread_local_sz);
 	threadptr = (uint32_t *)(sp + XT_STK_EXTRA);
-	/* Calculate THREADPTR value:
+	/* Calculate THREADPTR value.
 	 * The generated code will add THREADPTR value to a constant value determined at link time,
 	 * to get the address of the TLS variable.
 	 * The constant value is calculated by the linker as follows
 	 * (search for 'tpoff' in elf32-xtensa.c in BFD):
 	 *    offset = address - tls_section_vma + align_up(TCB_SIZE, tls_section_alignment)
-	 * where TCB_SIZE is hardcoded to 8. There doesn't seem to be a way to propagate
+	 * where TCB_SIZE is hardcoded to 8.
-	 * the section alignment value from the ld script into the code, so it is hardcoded
+	 * Note this is slightly different compared to the RISC-V port, where offset = address - tls_section_vma.
-	 * in both places.
 	 */
-	const uint32_t tls_section_alignment = 0x10;  /* has to be in sync with ALIGN value of .flash.rodata section */
+	const uint32_t tls_section_alignment = (uint32_t) &_flash_rodata_align;  /* ALIGN value of .flash.rodata section */
 	const uint32_t tcb_size = 8; /* Unrelated to FreeRTOS, this is the constant from BFD */
 	const uint32_t base = (tcb_size + tls_section_alignment - 1) & (~(tls_section_alignment - 1));
-	*threadptr = (uint32_t)task_thread_local_start - ((uint32_t)&_thread_local_start - (uint32_t)&_rodata_start) - base;
+	*threadptr = (uint32_t)task_thread_local_start - ((uint32_t)&_thread_local_start - (uint32_t)&_flash_rodata_start) - base;
 
 	#if XCHAL_CP_NUM > 0
 	/* Init the coprocessor save area (see xtensa_context.h) */

tools/ci/python_packages/ttfw_idf/IDFApp.py

@@ -28,20 +28,55 @@ except ImportError:
     gitlab_api = None
 
 
-def parse_flash_settings(path):
+def parse_encrypted_flag(args, offs, binary):
+    # Find partition entries (e.g. the entries with an offset and a file)
+    for _, entry in args.items():
+        # If the current entry is a partition, we have to check whether it is
+        # the one we are looking for or not
+        try:
+            if (entry["offset"], entry["file"]) == (offs, binary):
+                return entry["encrypted"] == "true"
+        except (TypeError, KeyError):
+            # TypeError occurs if the entry is a list, which is possible in JSON
+            # data structure.
+            # KeyError occurs if the entry doesn't have "encrypted" field.
+            continue
+
+    # The entry was not found, return None. The caller will have to check
+    # CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT macro
+    return None
+
+
+def parse_flash_settings(path, default_encryption=False):
     file_name = os.path.basename(path)
+
+    # For compatibility reasons, this list contains all the files to be
+    # flashed
+    flash_files = []
+    # The following list only contains the files that need encryption
+    encrypt_files = []
+
     if file_name == "flasher_args.json":
         # CMake version using build metadata file
         with open(path, "r") as f:
             args = json.load(f)
-        flash_files = [(offs, binary) for (offs, binary) in args["flash_files"].items() if offs != ""]
+
+        for (offs, binary) in args["flash_files"].items():
+            if offs:
+                flash_files.append((offs, binary))
+                encrypted = parse_encrypted_flag(args, offs, binary)
+
+                # default_encryption should be taken into account if and only if
+                # encrypted flag is not provided in the JSON file.
+                if (encrypted is None and default_encryption) or encrypted:
+                    encrypt_files.append((offs, binary))
+
         flash_settings = args["flash_settings"]
         app_name = os.path.splitext(args["app"]["file"])[0]
     else:
         # GNU Make version uses download.config arguments file
         with open(path, "r") as f:
             args = f.readlines()[-1].split(" ")
-            flash_files = []
             flash_settings = {}
             for idx in range(0, len(args), 2):  # process arguments in pairs
                 if args[idx].startswith("--"):
@@ -50,6 +85,9 @@ def parse_flash_settings(path):
                 else:
                     # offs, filename
                     flash_files.append((args[idx], args[idx + 1]))
+            # Parameter default_encryption tells us if the files need encryption
+            if default_encryption:
+                encrypt_files = flash_files
             # we can only guess app name in download.config.
             for p in flash_files:
                 if not os.path.dirname(p[1]) and "partition" not in p[1]:
@@ -58,7 +96,7 @@ def parse_flash_settings(path):
                     break
             else:
                 app_name = None
-    return flash_files, flash_settings, app_name
+    return flash_files, encrypt_files, flash_settings, app_name
 
 
 class Artifacts(object):
@@ -106,8 +144,7 @@ class Artifacts(object):
             self.gitlab_inst.download_artifact(job_id, [flash_arg_file], self.dest_root_path)
 
             # 2. download all binary files
-            flash_files, flash_settings, app_name = parse_flash_settings(os.path.join(self.dest_root_path,
+            flash_files, _, _, app_name = parse_flash_settings(os.path.join(self.dest_root_path, flash_arg_file))
-                                                                                      flash_arg_file))
             artifact_files = [os.path.join(base_path, p[1]) for p in flash_files]
             artifact_files.append(os.path.join(base_path, app_name + ".elf"))
 
@@ -165,7 +202,9 @@ class IDFApp(App.BaseApp):
                                   self.binary_path, self.IDF_DOWNLOAD_CONFIG_FILE)
                 raise AssertionError(msg)
 
-        self.flash_files, self.flash_settings = self._parse_flash_download_config()
+        # In order to keep backward compatibility, flash_files is unchanged.
+        # However, we now have a new attribute encrypt_files.
+        self.flash_files, self.encrypt_files, self.flash_settings = self._parse_flash_download_config()
         self.partition_table = self._parse_partition_table()
 
     @classmethod
@@ -227,6 +266,11 @@ class IDFApp(App.BaseApp):
                 ret = os.path.join(binary_path, fn)
         return ret
 
+    def _int_offs_abs_paths(self, files_list):
+        return [(int(offs, 0),
+                 os.path.join(self.binary_path, file_path.strip()))
+                for (offs, file_path) in files_list]
+
     def _parse_flash_download_config(self):
         """
         Parse flash download config from build metadata files
@@ -235,7 +279,7 @@ class IDFApp(App.BaseApp):
 
         (Called from constructor)
 
-        Returns (flash_files, flash_settings)
+        Returns (flash_files, encrypt_files, flash_settings)
         """
 
         if self.IDF_FLASH_ARGS_FILE in os.listdir(self.binary_path):
@@ -245,16 +289,20 @@ class IDFApp(App.BaseApp):
             # GNU Make version uses download.config arguments file
             path = os.path.join(self.binary_path, self.IDF_DOWNLOAD_CONFIG_FILE)
 
-        flash_files, flash_settings, app_name = parse_flash_settings(path)
+        # If the JSON doesn't find the encrypted flag for our files, provide
-        # The build metadata file does not currently have details, which files should be encrypted and which not.
+        # a default encrpytion flag: the macro
-        # Assume that all files should be encrypted if flash encryption is enabled in development mode.
+        # CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT
         sdkconfig_dict = self.get_sdkconfig()
-        flash_settings["encrypt"] = "CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT" in sdkconfig_dict
+        default_encryption = "CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT" in sdkconfig_dict
 
-        # make file offsets into integers, make paths absolute
+        flash_files, encrypt_files, flash_settings, _ = parse_flash_settings(path, default_encryption)
-        flash_files = [(int(offs, 0), os.path.join(self.binary_path, file_path.strip())) for (offs, file_path) in flash_files]
 
-        return flash_files, flash_settings
+        # Flash setting "encrypt" only and only if all the files to flash
+        # must be encrypted. Else, this parameter should be False.
+        # All files must be encrypted is both file lists are the same
+        flash_settings["encrypt"] = sorted(flash_files) == sorted(encrypt_files)
+
+        return self._int_offs_abs_paths(flash_files), self._int_offs_abs_paths(encrypt_files), flash_settings
 
     def _parse_partition_table(self):
         """

tools/ci/python_packages/ttfw_idf/IDFDUT.py

@@ -216,9 +216,28 @@ class IDFDUT(DUT.SerialDUT):
         Structured this way so @_uses_esptool will reconnect each time
         """
         flash_files = []
+        encrypt_files = []
         try:
-            # note: opening here prevents us from having to seek back to 0 each time
+            # Open the files here to prevents us from having to seek back to 0
-            flash_files = [(offs, open(path, "rb")) for (offs, path) in self.app.flash_files]
+            # each time. Before opening them, we have to organize the lists the
+            # way esptool.write_flash needs:
+            # If encrypt is provided, flash_files contains all the files to
+            # flash.
+            # Else, flash_files contains the files to be flashed as plain text
+            # and encrypt_files contains the ones to flash encrypted.
+            flash_files = self.app.flash_files
+            encrypt_files = self.app.encrypt_files
+            encrypt = self.app.flash_settings.get("encrypt", False)
+            if encrypt:
+                flash_files = encrypt_files
+                encrypt_files = []
+            else:
+                flash_files = [entry
+                               for entry in flash_files
+                               if entry not in encrypt_files]
+
+            flash_files = [(offs, open(path, "rb")) for (offs, path) in flash_files]
+            encrypt_files = [(offs, open(path, "rb")) for (offs, path) in encrypt_files]
 
             if erase_nvs:
                 address = self.app.partition_table["nvs"]["offset"]
@@ -228,7 +247,18 @@ class IDFDUT(DUT.SerialDUT):
                 nvs_file.seek(0)
                 if not isinstance(address, int):
                     address = int(address, 0)
-                flash_files.append((address, nvs_file))
+                # We have to check whether this file needs to be added to
+                # flash_files list or encrypt_files.
+                # Get the CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT macro
+                # value. If it is set to True, then NVS is always encrypted.
+                sdkconfig_dict = self.app.get_sdkconfig()
+                macro_encryption = "CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT" in sdkconfig_dict
+                # If the macro is not enabled (plain text flash) or all files
+                # must be encrypted, add NVS to flash_files.
+                if not macro_encryption or encrypt:
+                    flash_files.append((address, nvs_file))
+                else:
+                    encrypt_files.append((address, nvs_file))
 
             # fake flasher args object, this is a hack until
             # esptool Python API is improved
@@ -237,15 +267,19 @@ class IDFDUT(DUT.SerialDUT):
                     for key, value in attributes.items():
                         self.__setattr__(key, value)
 
+            # write_flash expects the parameter encrypt_files to be None and not
+            # an empty list, so perform the check here
             flash_args = FlashArgs({
                 'flash_size': self.app.flash_settings["flash_size"],
                 'flash_mode': self.app.flash_settings["flash_mode"],
                 'flash_freq': self.app.flash_settings["flash_freq"],
                 'addr_filename': flash_files,
+                'encrypt_files': encrypt_files or None,
                 'no_stub': False,
                 'compress': True,
                 'verify': False,
-                'encrypt': self.app.flash_settings.get("encrypt", False),
+                'encrypt': encrypt,
+                'ignore_flash_encryption_efuse_setting': False,
                 'erase_all': False,
             })
 
@@ -255,6 +289,8 @@ class IDFDUT(DUT.SerialDUT):
         finally:
             for (_, f) in flash_files:
                 f.close()
+            for (_, f) in encrypt_files:
+                f.close()
 
     def start_app(self, erase_nvs=ERASE_NVS):
         """

tools/test_apps/build_system/ldalign_test/CMakeLists.txt

@@ -0,0 +1,18 @@
+# The following lines of boilerplate have to be in your project's
+# CMakeLists in this exact order for cmake to work correctly
+cmake_minimum_required(VERSION 3.5)
+
+include($ENV{IDF_PATH}/tools/cmake/project.cmake)
+project(ldalign_test)
+
+idf_build_get_property(python PYTHON)
+idf_build_get_property(elf EXECUTABLE)
+
+set(check_alignment  "${CMAKE_CURRENT_LIST_DIR}/check_alignment.py")
+set(readelf "${CMAKE_TOOLCHAIN_PREFIX}readelf")
+
+add_custom_command(
+    TARGET ${elf}
+    POST_BUILD
+    COMMAND ${python} ${check_alignment} ${readelf} $<TARGET_FILE:${elf}>
+)

tools/test_apps/build_system/ldalign_test/README.txt

@@ -0,0 +1,7 @@
+Runs a build test to check alignment and position of `.flash.appdesc` and
+`.flash.rodata` sections. Indeed, `.flash.appdesc` shall ALWAYS be aligned on
+a 16-byte bounds, whereas `.flash.rodata` can have any alignment. In any case,
+the end address of first one shall match the start address of the second one.
+This will let both of them be merged when generating the final bin image.
+The Python script that performs the checks, `check_alignment.py`, automatically
+runs after the app is built.

tools/test_apps/build_system/ldalign_test/check_alignment.py

@@ -0,0 +1,64 @@
+#!/usr/bin/env python
+#
+# Copyright 2020 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.
+#
+
+import argparse
+import re
+import subprocess
+from typing import Tuple
+
+argparser = argparse.ArgumentParser()
+
+argparser.add_argument('readelf')
+argparser.add_argument('elf')
+
+args = argparser.parse_args()
+
+# Get the content of the readelf command
+contents = subprocess.check_output([args.readelf, '-S', args.elf]).decode()
+
+
+# Define a class for readelf parsing error
+class ParsingError(Exception):
+    pass
+
+
+# Look for the start address and size of any section
+def find_partition_info(sectionname):  # type: (str) -> Tuple[int, int, int]
+    match = re.search(sectionname + r'\s+PROGBITS\s+([a-f0-9]+) [a-f0-9]+ ([a-f0-9]+) \d+\s+[A-Z]+  0   0 (\d+)',
+                      contents)
+    if not match:
+        raise ParsingError('ELF header parsing error')
+    # Return the address of the section, the size and the alignment
+    address = match.group(1)
+    size = match.group(2)
+    alignment = match.group(3)
+    return (int(address, 16), int(size, 16), int(alignment, 10))
+
+
+# Get address and size for .flash.appdesc section
+app_address, app_size, app_align = find_partition_info('.flash.appdesc')
+
+# Same goes for .flash.rodata section
+rodata_address, _, rodata_align = find_partition_info('.flash.rodata')
+
+# Assert than everything is as expected:
+# appdesc is aligned on 16
+# rodata is aligned on 64
+# appdesc ends where rodata starts
+assert app_align == 16, '.flash.appdesc section should have been aligned on 16!'
+assert rodata_align == 64, '.flash.rodata section should have been aligned on 64!'
+assert app_address + app_size == rodata_address, ".flash.appdesc's end address and .flash.rodata's begin start must have no gap in between!"

tools/test_apps/build_system/ldalign_test/main/CMakeLists.txt

@@ -0,0 +1,2 @@
+idf_component_register(SRCS "test_main.c"
+                       INCLUDE_DIRS ".")

tools/test_apps/build_system/ldalign_test/main/test_main.c

@@ -0,0 +1,16 @@
+#include <stdio.h>
+
+const static uint32_t __attribute__ ((aligned (64))) testTab[] =
+  {
+   0xff445566, 0x44556677, 0x33221100,
+   0x88997755, 0x99887755, 0x88997755,
+   0x99546327, 0x7946fa9e, 0xa6b5f8ee,
+   0x12345678
+  };
+
+void app_main(void)
+{
+  /* Do something with the array, in order to avoid it being discarded. */
+  for (uint32_t i = 0; i < 10; i++)
+    printf ("%x\n", testTab[i]);
+}