From 974b8dd4c40cc38c451089eeb9bf90c6dc22a22d Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Thu, 18 Mar 2021 12:01:04 +0800 Subject: [PATCH 1/5] build: (Custom) App version info is now on a dedicated section, independent of the rodata alignment It is now possible to have any alignment restriction on rodata in the user applicaiton. It will not affect the first section which must be aligned on a 16-byte bound. Closes https://github.com/espressif/esp-idf/issues/6719 Closes https://github.com/espressif/esp-idf/issues/6976 --- components/esp32/ld/esp32.ld | 9 +++ components/esp32/ld/esp32.project.ld.in | 10 ++- components/esp32s2/ld/esp32s2.ld | 10 +++ components/esp32s2/ld/esp32s2.project.ld.in | 10 ++- .../build_system/ldalign_test/CMakeLists.txt | 18 ++++++ .../build_system/ldalign_test/README.txt | 7 ++ .../ldalign_test/check_alignment.py | 64 +++++++++++++++++++ .../ldalign_test/main/CMakeLists.txt | 2 + .../ldalign_test/main/test_main.c | 16 +++++ 9 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 tools/test_apps/build_system/ldalign_test/CMakeLists.txt create mode 100644 tools/test_apps/build_system/ldalign_test/README.txt create mode 100644 tools/test_apps/build_system/ldalign_test/check_alignment.py create mode 100644 tools/test_apps/build_system/ldalign_test/main/CMakeLists.txt create mode 100644 tools/test_apps/build_system/ldalign_test/main/test_main.c diff --git a/components/esp32/ld/esp32.ld b/components/esp32/ld/esp32.ld index 7ecc948303..e0fdcf99f3 100644 --- a/components/esp32/ld/esp32.ld +++ b/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 diff --git a/components/esp32/ld/esp32.project.ld.in b/components/esp32/ld/esp32.project.ld.in index 720cf1b977..018589bdec 100644 --- a/components/esp32/ld/esp32.project.ld.in +++ b/components/esp32/ld/esp32.project.ld.in @@ -250,13 +250,21 @@ SECTIONS "DRAM segment data does not fit.") /* When modifying the alignment, update tls_section_alignment in pxPortInitialiseStack */ - .flash.rodata : ALIGN(0x10) + .flash.appdesc : 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) + { mapping[flash_rodata] *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */ diff --git a/components/esp32s2/ld/esp32s2.ld b/components/esp32s2/ld/esp32s2.ld index 054c55dfa7..994b75ac39 100644 --- a/components/esp32s2/ld/esp32s2.ld +++ b/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 diff --git a/components/esp32s2/ld/esp32s2.project.ld.in b/components/esp32s2/ld/esp32s2.project.ld.in index 5abf441bb9..21b7e92eac 100644 --- a/components/esp32s2/ld/esp32s2.project.ld.in +++ b/components/esp32s2/ld/esp32s2.project.ld.in @@ -233,7 +233,7 @@ SECTIONS } > dram0_0_seg /* When modifying the alignment, update tls_section_alignment in pxPortInitialiseStack */ - .flash.rodata : ALIGN(0x10) + .flash.appdesc : ALIGN(0x10) { _rodata_reserved_start = ABSOLUTE(.); _rodata_start = ABSOLUTE(.); @@ -241,6 +241,14 @@ 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) + { mapping[flash_rodata] *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */ diff --git a/tools/test_apps/build_system/ldalign_test/CMakeLists.txt b/tools/test_apps/build_system/ldalign_test/CMakeLists.txt new file mode 100644 index 0000000000..74eb8e8377 --- /dev/null +++ b/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} $ +) diff --git a/tools/test_apps/build_system/ldalign_test/README.txt b/tools/test_apps/build_system/ldalign_test/README.txt new file mode 100644 index 0000000000..a6299c7fad --- /dev/null +++ b/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. diff --git a/tools/test_apps/build_system/ldalign_test/check_alignment.py b/tools/test_apps/build_system/ldalign_test/check_alignment.py new file mode 100644 index 0000000000..1f988fd9c7 --- /dev/null +++ b/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!" diff --git a/tools/test_apps/build_system/ldalign_test/main/CMakeLists.txt b/tools/test_apps/build_system/ldalign_test/main/CMakeLists.txt new file mode 100644 index 0000000000..1df31fac80 --- /dev/null +++ b/tools/test_apps/build_system/ldalign_test/main/CMakeLists.txt @@ -0,0 +1,2 @@ +idf_component_register(SRCS "test_main.c" + INCLUDE_DIRS ".") diff --git a/tools/test_apps/build_system/ldalign_test/main/test_main.c b/tools/test_apps/build_system/ldalign_test/main/test_main.c new file mode 100644 index 0000000000..160197ba59 --- /dev/null +++ b/tools/test_apps/build_system/ldalign_test/main/test_main.c @@ -0,0 +1,16 @@ +#include + +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]); +} From af7b21851e9d5a7d1edc128c04f2e307bd2f9d98 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 21 Apr 2021 11:49:58 +0200 Subject: [PATCH 2/5] freertos: fix TLS run-time address calculation Since dd849ffc, _rodata_start label has been moved to a different linker output section from where the TLS templates (.tdata, .tbss) are located. Since link-time addresses of thread-local variables are calculated relative to the section start address, this resulted in incorrect calculation of THREADPTR/$tp registers. Fix by introducing new linker label, _flash_rodata_start, which points to the .flash.rodata output section where TLS variables are located, and use it when calculating THREADPTR/$tp. Also remove the hardcoded rodata section alignment for Xtensa targets. Alignment of rodata can be affected by the user application, which is the issue dd849ffc was fixing. To accommodate any possible alignment, save it in a linker label (_flash_rodata_align) and then use when calculating THREADPTR. Note that this is not required on RISC-V, since this target doesn't use TPOFF. --- components/esp32/ld/esp32.project.ld.in | 5 +- components/esp32s2/ld/esp32s2.project.ld.in | 5 +- components/freertos/xtensa/port.c | 58 +++++++++++++++++---- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/components/esp32/ld/esp32.project.ld.in b/components/esp32/ld/esp32.project.ld.in index 018589bdec..da082d014b 100644 --- a/components/esp32/ld/esp32.project.ld.in +++ b/components/esp32/ld/esp32.project.ld.in @@ -249,7 +249,6 @@ 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) { _rodata_start = ABSOLUTE(.); @@ -265,6 +264,8 @@ SECTIONS .flash.rodata : ALIGN(0x10) { + _flash_rodata_start = ABSOLUTE(.); + mapping[flash_rodata] *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */ @@ -320,6 +321,8 @@ SECTIONS . = ALIGN(4); } >default_rodata_seg + _flash_rodata_align = ALIGNOF(.flash.rodata); + .flash.text : { _stext = .; diff --git a/components/esp32s2/ld/esp32s2.project.ld.in b/components/esp32s2/ld/esp32s2.project.ld.in index 21b7e92eac..4da0db6123 100644 --- a/components/esp32s2/ld/esp32s2.project.ld.in +++ b/components/esp32s2/ld/esp32s2.project.ld.in @@ -232,7 +232,6 @@ SECTIONS _heap_start = ABSOLUTE(.); } > dram0_0_seg - /* When modifying the alignment, update tls_section_alignment in pxPortInitialiseStack */ .flash.appdesc : ALIGN(0x10) { _rodata_reserved_start = ABSOLUTE(.); @@ -249,6 +248,8 @@ SECTIONS .flash.rodata : ALIGN(0x10) { + _flash_rodata_start = ABSOLUTE(.); + mapping[flash_rodata] *(.irom1.text) /* catch stray ICACHE_RODATA_ATTR */ @@ -305,6 +306,8 @@ SECTIONS . = ALIGN(4); } >default_rodata_seg + _flash_rodata_align = ALIGNOF(.flash.rodata); + .flash.text : { _stext = .; diff --git a/components/freertos/xtensa/port.c b/components/freertos/xtensa/port.c index dbf0824bed..e737c05742 100644 --- a/components/freertos/xtensa/port.c +++ b/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 - * the section alignment value from the ld script into the code, so it is hardcoded - * in both places. + * where TCB_SIZE is hardcoded to 8. + * Note this is slightly different compared to the RISC-V port, where offset = address - tls_section_vma. */ - 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) */ @@ -385,7 +423,7 @@ uint32_t xPortGetTickRateHz(void) { void __attribute__((optimize("-O3"))) vPortEnterCritical(portMUX_TYPE *mux) { BaseType_t oldInterruptLevel = portENTER_CRITICAL_NESTED(); - /* Interrupts may already be disabled (because we're doing this recursively) + /* Interrupts may already be disabled (because we're doing this recursively) * but we can't get the interrupt level after * vPortCPUAquireMutex, because it also may mess with interrupts. * Get it here first, then later figure out if we're nesting @@ -434,4 +472,4 @@ void __attribute__((weak)) vApplicationStackOverflowHook( TaskHandle_t xTask, c dest = strcat(dest, str[i]); } esp_system_abort(buf); -} \ No newline at end of file +} From 78ea042e7d26e855dc6c635c7a06c09f0d8b58b0 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Fri, 7 May 2021 12:29:36 +0800 Subject: [PATCH 3/5] esptool: Update esptool to have merge adjacent sections feature --- components/esptool_py/esptool | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esptool_py/esptool b/components/esptool_py/esptool index 4fa0bd7b0d..4698b39673 160000 --- a/components/esptool_py/esptool +++ b/components/esptool_py/esptool @@ -1 +1 @@ -Subproject commit 4fa0bd7b0d1f69f5ff22b043adc07c5e562a8931 +Subproject commit 4698b396730b23fb4aab023c5fb1744db957fc4c From 5c175721e97d56524271c4f6cee70cfa12591d1f Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Thu, 26 Nov 2020 14:42:55 +0800 Subject: [PATCH 4/5] ci: use "encrypted" information in flasher_args.json Take into account the new field "encrypted" that is part of the partition entries in flasher_args.json file Closes IDF-2231 --- tools/ci/python_packages/ttfw_idf/IDFApp.py | 76 +++++++++++++++++---- tools/ci/python_packages/ttfw_idf/IDFDUT.py | 43 ++++++++++-- 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/tools/ci/python_packages/ttfw_idf/IDFApp.py b/tools/ci/python_packages/ttfw_idf/IDFApp.py index edf9288d19..df72af0e27 100644 --- a/tools/ci/python_packages/ttfw_idf/IDFApp.py +++ b/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: + # 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_arg_file)) + flash_files, _, _, app_name = parse_flash_settings(os.path.join(self.dest_root_path, 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 @@ -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) - # The build metadata file does not currently have details, which files should be encrypted and which not. - # Assume that all files should be encrypted if flash encryption is enabled in development mode. + # If the JSON doesn't find the encrypted flag for our files, provide + # a default encrpytion flag: the macro + # 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 = [(int(offs, 0), os.path.join(self.binary_path, file_path.strip())) for (offs, file_path) in flash_files] + flash_files, encrypt_files, flash_settings, _ = parse_flash_settings(path, default_encryption) - 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), self.flash_settings def _parse_partition_table(self): """ diff --git a/tools/ci/python_packages/ttfw_idf/IDFDUT.py b/tools/ci/python_packages/ttfw_idf/IDFDUT.py index 99f7254acb..3bac0c04d6 100644 --- a/tools/ci/python_packages/ttfw_idf/IDFDUT.py +++ b/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 - flash_files = [(offs, open(path, "rb")) for (offs, path) in self.app.flash_files] + # Open the files here to prevents us from having to seek back to 0 + # 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,18 @@ 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, 'erase_all': False, }) @@ -255,6 +288,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): """ From bf4320ba681b260cb10e11881f822010a9156aa1 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Mon, 28 Dec 2020 14:09:20 +0800 Subject: [PATCH 5/5] ci: fix flasher_args.json parser (iterate over dictionary) Closes IDFCI-347 --- tools/ci/python_packages/ttfw_idf/IDFApp.py | 6 +++--- tools/ci/python_packages/ttfw_idf/IDFDUT.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/ci/python_packages/ttfw_idf/IDFApp.py b/tools/ci/python_packages/ttfw_idf/IDFApp.py index df72af0e27..fde49cefa0 100644 --- a/tools/ci/python_packages/ttfw_idf/IDFApp.py +++ b/tools/ci/python_packages/ttfw_idf/IDFApp.py @@ -30,7 +30,7 @@ except ImportError: def parse_encrypted_flag(args, offs, binary): # Find partition entries (e.g. the entries with an offset and a file) - for _, entry in args: + 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: @@ -279,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): @@ -302,7 +302,7 @@ class IDFApp(App.BaseApp): # 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), self.flash_settings + return self._int_offs_abs_paths(flash_files), self._int_offs_abs_paths(encrypt_files), flash_settings def _parse_partition_table(self): """ diff --git a/tools/ci/python_packages/ttfw_idf/IDFDUT.py b/tools/ci/python_packages/ttfw_idf/IDFDUT.py index 3bac0c04d6..aa62584d32 100644 --- a/tools/ci/python_packages/ttfw_idf/IDFDUT.py +++ b/tools/ci/python_packages/ttfw_idf/IDFDUT.py @@ -279,6 +279,7 @@ class IDFDUT(DUT.SerialDUT): 'compress': True, 'verify': False, 'encrypt': encrypt, + 'ignore_flash_encryption_efuse_setting': False, 'erase_all': False, })