[PATCH 0/2] arm: mach-k3: Fix corruption of device manager firmware
The following series takes care of two problems: a) A potential race when R5 (boot processor) is parsing and loading DM firmware elf sections way slower than A53 executing. b) We load the FIT image to the same address as the elf sections. See [1] as example.
NOTE: Though, in theory, the Device Manager's linker file could be constructed such as to avoid the overlap address, there is no guarentee of implicit assumptions and the fact that DM's firmware can be combined with additional functionality could result in unexpected behavior as well.
To prevent this, lets just have explicit load address and sequencing.
Nishanth Menon (2): arm: mach-k3: common: Make sure firmware sections are loaded prior to armv8 startup tools: k3_fit_atf: Change DM load address to avoid conflict
arch/arm/mach-k3/common.c | 30 ++++++++++++++++++------------ tools/k3_fit_atf.sh | 4 ++-- 2 files changed, 20 insertions(+), 14 deletions(-)
[1] $ arm-none-linux-gnueabihf-readelf -S ti-dm/j721e/ipc_echo_testb_mcu1_0_release_strip.xer5f There are 39 section headers, starting at offset 0x2aa98:
Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .freertosrst[...] PROGBITS 41010000 000038 000040 00 AX 0 0 8 [ 2] .bootCode PROGBITS 41010a60 000a98 000118 00 AX 0 0 8 [ 3] .startupCode PROGBITS 41010410 000448 000338 00 AX 0 0 8 [ 4] .startupData PROGBITS 41010748 000780 000318 00 A 0 0 8 [ 5] .text.hwi PROGBITS 41010040 000078 0003d0 00 AX 0 0 8 [ 6] .text.cache NOBITS 41010410 02a680 000000 00 AX 0 0 8 [ 7] .text.mpu NOBITS 41010410 02a680 000000 00 AX 0 0 8 [ 8] .text.boot NOBITS 41010410 02a680 000000 00 AX 0 0 8 [ 9] .text PROGBITS a05b8fb0 002000 016340 00 AX 0 0 8 [10] .const PROGBITS a05cf2f0 018340 00c470 00 A 0 0 8 [11] .cinit PROGBITS a0581400 001100 000818 00 A 0 0 8 [12] .bss NOBITS a0582000 002000 036faa 00 WA 0 0 8192 [13] .far NOBITS 00000000 000000 000000 00 0 0 4 [14] .data NOBITS a05db780 024800 008100 00 WA 0 0 128 [15] .sysmem NOBITS 00000000 000000 000000 00 0 0 1 [16] .data_buffer NOBITS 00000000 02a680 000000 00 WA 0 0 128 [17] .bss.devgroup* NOBITS a05e9700 02a680 0011f8 00 WA 0 0 4 [18] .const.devgroup* PROGBITS a05e3880 024800 004710 00 A 0 0 4 [19] .boardcfg_data PROGBITS a05e8000 028f80 001700 00 A 0 0 128 [20] .benchmark_buffer NOBITS 00000000 000000 000000 00 0 0 8 [21] ipc_data_buffer NOBITS a0400000 001100 181400 00 WA 0 0 128 [22] .resource_table PROGBITS a0180000 001000 00008c 00 A 0 0 4096 [23] .tracebuf NOBITS a0100000 000c00 080000 00 WA 0 0 1024 [24] .stack NOBITS a0ffc000 02a680 004000 00 WA 0 0 8 [25] .irqStack NOBITS a0ffae00 02a880 001000 00 A 0 0 4 [26] .fiqStack NOBITS a0ffbe00 02b880 000100 00 A 0 0 4 [27] .abortStack NOBITS a0ffbf00 02b980 000100 00 A 0 0 4 [28] .undStack NOBITS a0ffac00 02a680 000100 00 A 0 0 4 [29] .svcStack NOBITS a0ffad00 02a780 000100 00 A 0 0 4 [30] .TI.noinit NOBITS 00000000 000000 000000 00 0 0 4 [31] .TI.persistent NOBITS 00000000 000000 000000 00 0 0 128 [32] __llvm_prf_cnts NOBITS a0400000 02a680 000000 00 A 0 0 4 [33] Veneer$$CMSE NOBITS 00000000 000000 000000 00 0 0 1 [34] .ARM.attributes ARM_ATTRIBUTES 00000000 02a680 00005b 00 0 0 0 [35] .symtab SYMTAB 00000000 02a6dc 000030 10 37 1 0 [36] .TI.section.flags PROGBITS 00000000 02a70c 000020 00 0 0 0 [37] .strtab STRTAB 00000000 02a72c 000014 01 S 0 0 0 [38] .shstrtab STRTAB 00000000 02a740 0001b6 01 S 0 0 0
With Device Manager firmware in an elf file form, we cannot load the FIT image to the exact same address as any of the executable sections of the elf file itself is located.
However, the device tree descriptions for the ARMV8 bootloader/OS includes DDR regions only the final sections in DDR where the Device Manager firmware is actually executing out of.
As the R5 uC is usually operating at a slower rate than an ARMv8 MPU, by starting the Armv8 ahead of parsing the elf and copying the correct sections to the required memories creates a race condition where the ARMv8 could overwrite the elf image loaded from the FIT image prior to the R5 completing parsing and putting the correct sections of elf in the required memory locations. OR create rather obscure debug conditions where data in the section is being modified by ARMV8 OS while the elf copy is in progress.
To prevent all these conditions, lets make sure that the elf parse and copy operations are completed ahead of ARMv8 being released to execute.
We will pay a penalty of elf copy time, but that is a valid tradeoff in comparison to debug of alternate scenarios.
Signed-off-by: Nishanth Menon nm@ti.com --- arch/arm/mach-k3/common.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index bb0f64194f4e..64933933fe1f 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -198,7 +198,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) typedef void __noreturn (*image_entry_noargs_t)(void); struct ti_sci_handle *ti_sci = get_ti_sci_handle(); u32 loadaddr = 0; - int ret, size = 0; + int ret, size = 0, shut_cpu = 0;
/* Release all the exclusive devices held by SPL before starting ATF */ ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci); @@ -226,19 +226,10 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) if (ret) panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
- /* Add an extra newline to differentiate the ATF logs from SPL */ - printf("Starting ATF on ARM64 core...\n\n"); - - ret = rproc_start(1); - if (ret) - panic("%s: ATF failed to start on rproc (%d)\n", __func__, ret); if (!fit_image_info[IMAGE_ID_DM_FW].image_len && !(size > 0 && valid_elf_image(loadaddr))) { - debug("Shutting down...\n"); - release_resources_for_core_shutdown(); - - while (1) - asm volatile("wfe"); + shut_cpu = 1; + goto start_arm64; }
if (!fit_image_info[IMAGE_ID_DM_FW].image_start) { @@ -251,6 +242,21 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
debug("%s: jumping to address %x\n", __func__, loadaddr);
+start_arm64: + /* Add an extra newline to differentiate the ATF logs from SPL */ + printf("Starting ATF on ARM64 core...\n\n"); + + ret = rproc_start(1); + if (ret) + panic("%s: ATF failed to start on rproc (%d)\n", __func__, ret); + + if (shut_cpu) { + debug("Shutting down...\n"); + release_resources_for_core_shutdown(); + + while (1) + asm volatile("wfe"); + } image_entry_noargs_t image_entry = (image_entry_noargs_t)loadaddr;
image_entry();
On Tue, Aug 31, 2021 at 01:20:48PM -0500, Nishanth Menon wrote:
With Device Manager firmware in an elf file form, we cannot load the FIT image to the exact same address as any of the executable sections of the elf file itself is located.
However, the device tree descriptions for the ARMV8 bootloader/OS includes DDR regions only the final sections in DDR where the Device Manager firmware is actually executing out of.
As the R5 uC is usually operating at a slower rate than an ARMv8 MPU, by starting the Armv8 ahead of parsing the elf and copying the correct sections to the required memories creates a race condition where the ARMv8 could overwrite the elf image loaded from the FIT image prior to the R5 completing parsing and putting the correct sections of elf in the required memory locations. OR create rather obscure debug conditions where data in the section is being modified by ARMV8 OS while the elf copy is in progress.
To prevent all these conditions, lets make sure that the elf parse and copy operations are completed ahead of ARMv8 being released to execute.
We will pay a penalty of elf copy time, but that is a valid tradeoff in comparison to debug of alternate scenarios.
Signed-off-by: Nishanth Menon nm@ti.com
Applied to u-boot/master, thanks!
DM binary is expected to be an elf file. The expected address of this elf is in the range of 0xa000_0000 in DDR. In the current configuration, elf file is loaded to the exact same address and we invoke load_elf_image_phdr to decode and memcpy sections of the elf to the same address range, we are eventually going to overwrite sections of the original elf image itself corrupting the DM firmware as a result.
To prevent this, we must load the DM section to an address that does'nt overlap with the actual section address where it will eventually be copied to.
Lets add an offset of 256MB to a lesser used DDR address, yet within the typical 1GB address space.
We may need to consider going for a configuration option if this is turning to be a limitation (boards with lesser DDR space).
Signed-off-by: Nishanth Menon nm@ti.com --- tools/k3_fit_atf.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/k3_fit_atf.sh b/tools/k3_fit_atf.sh index 3a476ced98b1..6469f188e8ff 100755 --- a/tools/k3_fit_atf.sh +++ b/tools/k3_fit_atf.sh @@ -67,8 +67,8 @@ cat << __HEADER_EOF arch = "arm32"; compression = "none"; os = "DM"; - load = <0xa0000000>; - entry = <0xa0000000>; + load = <0xb0000000>; + entry = <0xb0000000>; }; spl { description = "SPL (64-bit)";
On 13:20-20210831, Nishanth Menon wrote:
DM binary is expected to be an elf file. The expected address of this elf is in the range of 0xa000_0000 in DDR. In the current configuration, elf file is loaded to the exact same address and we invoke load_elf_image_phdr to decode and memcpy sections of the elf to the same address range, we are eventually going to overwrite sections of the original elf image itself corrupting the DM firmware as a result.
To prevent this, we must load the DM section to an address that does'nt overlap with the actual section address where it will eventually be copied to.
Lets add an offset of 256MB to a lesser used DDR address, yet within the typical 1GB address space.
We may need to consider going for a configuration option if this is turning to be a limitation (boards with lesser DDR space).
Signed-off-by: Nishanth Menon nm@ti.com
tools/k3_fit_atf.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/k3_fit_atf.sh b/tools/k3_fit_atf.sh index 3a476ced98b1..6469f188e8ff 100755 --- a/tools/k3_fit_atf.sh +++ b/tools/k3_fit_atf.sh @@ -67,8 +67,8 @@ cat << __HEADER_EOF arch = "arm32"; compression = "none"; os = "DM";
load = <0xa0000000>;entry = <0xa0000000>;
load = <0xb0000000>;entry = <0xb0000000>;-- 2.32.0
Sigh.. I see Suman also discovered the exact same error in [1] - I think I prefer Suman's choice of address to mine.
[1] https://patchwork.ozlabs.org/project/uboot/patch/20210814064901.3840-1-s-ann...
participants (2)
-
Nishanth Menon -
Tom Rini