[PATCH 0/2] arm: mach-k3: Fix corruption of device manager firmware

[PATCH 0/2] arm: mach-k3: Fix corruption of device manager firmware

[Nishanth Menon]

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

-- 
2.32.0

[PATCH 1/2] arm: mach-k3: common: Make sure firmware sections are loaded prior to armv8 startup

[Nishanth Menon]

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();
-- 
2.32.0

[PATCH 2/2] tools: k3_fit_atf: Change DM load address to avoid conflict

[Nishanth Menon]

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)";
-- 
2.32.0

Re: [PATCH 2/2] tools: k3_fit_atf: Change DM load address to avoid conflict

[Nishanth Menon]

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>;
>  		};
>  		spl {
>  			description = "SPL (64-bit)";
> -- 
> 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-anna@ti.com/

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 1/2] arm: mach-k3: common: Make sure firmware sections are loaded prior to armv8 startup

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]