[PATCH v3 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[PATCH v3 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Marek Vasut]

In case Dcache is enabled while the ECDSA authentication function is
called via BootROM ROM API, the MMU tables are set up and the BootROM
region is not marked as executable, so an attempt to run code from it
results in a hang. Mark the BootROM region as executable as suggested
by Patrick to prevent the hang.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
V2: - Initialize reenable_dcache variable
V3: - Mark BootROM as executable instead
---
 arch/arm/mach-stm32mp/ecdsa_romapi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
index a2f63ff879f..6156526253c 100644
--- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
+++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
@@ -81,6 +81,10 @@ static int romapi_ecdsa_verify(struct udevice *dev,
 	memcpy(raw_key + 32, pubkey->y, 32);
 
 	stm32mp_rom_get_ecdsa_functions(&rom);
+
+	/* Mark BootROM region as executable. */
+	mmu_set_region_dcache_behaviour(0, SZ_2M, DCACHE_DEFAULT_OPTION);
+
 	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
 
 	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
-- 
2.35.1

[PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Marek Vasut]

The STM32MP15xx platform currently comes with two incompatible
implementations of save_boot_params() weak function override.
Factor the save_boot_params() implementation into common cpu.c
code and provide accessors to read out both ROM API table address
and DT address from any place in the code instead.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
V2: Avoid #if CONFIG... , use if (CONFIG... instead
V3: No change
---
 arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
 arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
 arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
 .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
 4 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
index e91ef1b2fc7..e40cca938ef 100644
--- a/arch/arm/mach-stm32mp/boot_params.c
+++ b/arch/arm/mach-stm32mp/boot_params.c
@@ -11,30 +11,14 @@
 #include <asm/sections.h>
 #include <asm/system.h>
 
-/*
- * Force data-section, as .bss will not be valid
- * when save_boot_params is invoked.
- */
-static unsigned long nt_fw_dtb __section(".data");
-
-/*
- * Save the FDT address provided by TF-A in r2 at boot time
- * This function is called from start.S
- */
-void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
-		      unsigned long r3)
-{
-	nt_fw_dtb = r2;
-
-	save_boot_params_ret();
-}
-
 /*
  * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
  * Non Trusted Firmware configuration file) when the pointer is valid
  */
 void *board_fdt_blob_setup(int *err)
 {
+	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
+
 	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
 
 	*err = 0;
diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 855fc755fe0..ee59866bb73 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -378,3 +378,38 @@ int arch_misc_init(void)
 
 	return 0;
 }
+
+/*
+ * Without forcing the ".data" section, this would get saved in ".bss". BSS
+ * will be cleared soon after, so it's not suitable.
+ */
+static uintptr_t rom_api_table __section(".data");
+static uintptr_t nt_fw_dtb __section(".data");
+
+/*
+ * The ROM gives us the API location in r0 when starting. This is only available
+ * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
+ * the FDT address provided by TF-A in r2 at boot time. This function is called
+ * from start.S
+ */
+void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
+		      unsigned long r3)
+{
+	if (IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY))
+		rom_api_table = r0;
+
+	if (IS_ENABLED(CONFIG_TFABOOT))
+		nt_fw_dtb = r2;
+
+	save_boot_params_ret();
+}
+
+uintptr_t get_stm32mp_rom_api_table(void)
+{
+	return rom_api_table;
+}
+
+uintptr_t get_stm32mp_bl2_dtb(void)
+{
+	return nt_fw_dtb;
+}
diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
index 6156526253c..12b42b9d59c 100644
--- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
+++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
@@ -24,26 +24,10 @@ struct ecdsa_rom_api {
 					   uint32_t ecc_algo);
 };
 
-/*
- * Without forcing the ".data" section, this would get saved in ".bss". BSS
- * will be cleared soon after, so it's not suitable.
- */
-static uintptr_t rom_api_loc __section(".data");
-
-/*
- * The ROM gives us the API location in r0 when starting. This is only available
- * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
- */
-void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
-		      unsigned long r3)
-{
-	rom_api_loc = r0;
-	save_boot_params_ret();
-}
-
 static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
 {
-	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
+	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
+			       ROM_API_OFFSET_ECDSA_VERIFY;
 
 	rom->ecdsa_verify_signature = *(void **)verify_ptr;
 }
diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
index f19a70e53e0..0d39b67178e 100644
--- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
+++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
@@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
 
 /* helper function: read data from OTP */
 u32 get_otp(int index, int shift, int mask);
+
+uintptr_t get_stm32mp_rom_api_table(void);
+uintptr_t get_stm32mp_bl2_dtb(void);
-- 
2.35.1

[PATCH v3 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Marek Vasut]

The ROM API table pointer is no longer accessible from U-Boot, fix
this by passing the ROM API pointer through. This makes it possible
for U-Boot to call ROM API functions to authenticate payload like
signed fitImages.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
V2: - Rename image_entry_noargs_t to image_entry_stm32_t
    - Add missing __noreturn
V3: No change
---
 arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index ee59866bb73..dc4112d5e6c 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -22,6 +22,7 @@
 #include <dm/device.h>
 #include <dm/uclass.h>
 #include <linux/bitops.h>
+#include <spl.h>
 
 /*
  * early TLB into the .data section so that it not get cleared
@@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
 {
 	return nt_fw_dtb;
 }
+
+#ifdef CONFIG_SPL_BUILD
+void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
+{
+	typedef void __noreturn (*image_entry_stm32_t)(u32 romapi);
+	uintptr_t romapi = get_stm32mp_rom_api_table();
+
+	image_entry_stm32_t image_entry =
+		(image_entry_stm32_t)spl_image->entry_point;
+
+	printf("image entry point: 0x%lx\n", spl_image->entry_point);
+	image_entry(romapi);
+}
+#endif
-- 
2.35.1

[PATCH v3 4/4] ARM: stm32: Make ECDSA authentication available to U-Boot

[Marek Vasut]

With U-Boot having access to ROM API call table, it is possible to use
the ROM API call it authenticate e.g. signed kernel fitImages using the
BootROM ECDSA support. Make this available by pulling the ECDSA BootROM
call support from SPL-only guard.

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>
Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
V2: Add RB from Patrice and Patrick
V3: No change
---
 arch/arm/mach-stm32mp/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/Makefile b/arch/arm/mach-stm32mp/Makefile
index 1db9057e049..a19b2797c8b 100644
--- a/arch/arm/mach-stm32mp/Makefile
+++ b/arch/arm/mach-stm32mp/Makefile
@@ -11,10 +11,10 @@ obj-y += bsec.o
 obj-$(CONFIG_STM32MP13x) += stm32mp13x.o
 obj-$(CONFIG_STM32MP15x) += stm32mp15x.o
 
+obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
 ifdef CONFIG_SPL_BUILD
 obj-y += spl.o
 obj-y += tzc400.o
-obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
 else
 obj-y += cmd_stm32prog/
 obj-$(CONFIG_CMD_STM32KEY) += cmd_stm32key.o
-- 
2.35.1

Re: [PATCH v3 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Patrick DELAUNAY]

Hi,

On 12/7/22 20:24, Marek Vasut wrote:
> In case Dcache is enabled while the ECDSA authentication function is
> called via BootROM ROM API, the MMU tables are set up and the BootROM
> region is not marked as executable, so an attempt to run code from it
> results in a hang. Mark the BootROM region as executable as suggested
> by Patrick to prevent the hang.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: - Initialize reenable_dcache variable
> V3: - Mark BootROM as executable instead
> ---
>   arch/arm/mach-stm32mp/ecdsa_romapi.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index a2f63ff879f..6156526253c 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -81,6 +81,10 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>   	memcpy(raw_key + 32, pubkey->y, 32);
>   
>   	stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +	/* Mark BootROM region as executable. */
> +	mmu_set_region_dcache_behaviour(0, SZ_2M, DCACHE_DEFAULT_OPTION);
> +
>   	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
>   
>   	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Patrick DELAUNAY]

Hi,

On 12/7/22 20:24, Marek Vasut wrote:
> The STM32MP15xx platform currently comes with two incompatible
> implementations of save_boot_params() weak function override.
> Factor the save_boot_params() implementation into common cpu.c
> code and provide accessors to read out both ROM API table address
> and DT address from any place in the code instead.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: Avoid #if CONFIG... , use if (CONFIG... instead
> V3: No change
> ---
>   arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
>   arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
>   arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
>   .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
>   4 files changed, 42 insertions(+), 36 deletions(-)



Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

Re: [PATCH v3 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Patrick DELAUNAY]

Hi,

On 12/7/22 20:24, Marek Vasut wrote:
> The ROM API table pointer is no longer accessible from U-Boot, fix
> this by passing the ROM API pointer through. This makes it possible
> for U-Boot to call ROM API functions to authenticate payload like
> signed fitImages.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: - Rename image_entry_noargs_t to image_entry_stm32_t
>      - Add missing __noreturn
> V3: No change
> ---
>   arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index ee59866bb73..dc4112d5e6c 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -22,6 +22,7 @@
>   #include <dm/device.h>
>   #include <dm/uclass.h>
>   #include <linux/bitops.h>
> +#include <spl.h>
>   
>   /*
>    * early TLB into the .data section so that it not get cleared
> @@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
>   {
>   	return nt_fw_dtb;
>   }
> +
> +#ifdef CONFIG_SPL_BUILD
> +void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
> +{
> +	typedef void __noreturn (*image_entry_stm32_t)(u32 romapi);
> +	uintptr_t romapi = get_stm32mp_rom_api_table();
> +
> +	image_entry_stm32_t image_entry =
> +		(image_entry_stm32_t)spl_image->entry_point;
> +
> +	printf("image entry point: 0x%lx\n", spl_image->entry_point);
> +	image_entry(romapi);
> +}
> +#endif


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

Re: [PATCH v3 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Patrice CHOTARD]

Hi Marek

On 12/7/22 20:24, Marek Vasut wrote:
> In case Dcache is enabled while the ECDSA authentication function is
> called via BootROM ROM API, the MMU tables are set up and the BootROM
> region is not marked as executable, so an attempt to run code from it
> results in a hang. Mark the BootROM region as executable as suggested
> by Patrick to prevent the hang.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: - Initialize reenable_dcache variable
> V3: - Mark BootROM as executable instead
> ---
>  arch/arm/mach-stm32mp/ecdsa_romapi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index a2f63ff879f..6156526253c 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -81,6 +81,10 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>  	memcpy(raw_key + 32, pubkey->y, 32);
>  
>  	stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +	/* Mark BootROM region as executable. */
> +	mmu_set_region_dcache_behaviour(0, SZ_2M, DCACHE_DEFAULT_OPTION);
> +
>  	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
>  
>  	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Patrice CHOTARD]

Hi Marek

On 12/7/22 20:24, Marek Vasut wrote:
> The STM32MP15xx platform currently comes with two incompatible
> implementations of save_boot_params() weak function override.
> Factor the save_boot_params() implementation into common cpu.c
> code and provide accessors to read out both ROM API table address
> and DT address from any place in the code instead.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: Avoid #if CONFIG... , use if (CONFIG... instead
> V3: No change
> ---
>  arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
>  arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
>  arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
>  .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
>  4 files changed, 42 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
> index e91ef1b2fc7..e40cca938ef 100644
> --- a/arch/arm/mach-stm32mp/boot_params.c
> +++ b/arch/arm/mach-stm32mp/boot_params.c
> @@ -11,30 +11,14 @@
>  #include <asm/sections.h>
>  #include <asm/system.h>
>  
> -/*
> - * Force data-section, as .bss will not be valid
> - * when save_boot_params is invoked.
> - */
> -static unsigned long nt_fw_dtb __section(".data");
> -
> -/*
> - * Save the FDT address provided by TF-A in r2 at boot time
> - * This function is called from start.S
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	nt_fw_dtb = r2;
> -
> -	save_boot_params_ret();
> -}
> -
>  /*
>   * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
>   * Non Trusted Firmware configuration file) when the pointer is valid
>   */
>  void *board_fdt_blob_setup(int *err)
>  {
> +	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
> +
>  	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
>  
>  	*err = 0;
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 855fc755fe0..ee59866bb73 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -378,3 +378,38 @@ int arch_misc_init(void)
>  
>  	return 0;
>  }
> +
> +/*
> + * Without forcing the ".data" section, this would get saved in ".bss". BSS
> + * will be cleared soon after, so it's not suitable.
> + */
> +static uintptr_t rom_api_table __section(".data");
> +static uintptr_t nt_fw_dtb __section(".data");
> +
> +/*
> + * The ROM gives us the API location in r0 when starting. This is only available
> + * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
> + * the FDT address provided by TF-A in r2 at boot time. This function is called
> + * from start.S
> + */
> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> +		      unsigned long r3)
> +{
> +	if (IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY))
> +		rom_api_table = r0;
> +
> +	if (IS_ENABLED(CONFIG_TFABOOT))
> +		nt_fw_dtb = r2;
> +
> +	save_boot_params_ret();
> +}
> +
> +uintptr_t get_stm32mp_rom_api_table(void)
> +{
> +	return rom_api_table;
> +}
> +
> +uintptr_t get_stm32mp_bl2_dtb(void)
> +{
> +	return nt_fw_dtb;
> +}
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index 6156526253c..12b42b9d59c 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -24,26 +24,10 @@ struct ecdsa_rom_api {
>  					   uint32_t ecc_algo);
>  };
>  
> -/*
> - * Without forcing the ".data" section, this would get saved in ".bss". BSS
> - * will be cleared soon after, so it's not suitable.
> - */
> -static uintptr_t rom_api_loc __section(".data");
> -
> -/*
> - * The ROM gives us the API location in r0 when starting. This is only available
> - * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	rom_api_loc = r0;
> -	save_boot_params_ret();
> -}
> -
>  static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
>  {
> -	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
> +	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
> +			       ROM_API_OFFSET_ECDSA_VERIFY;
>  
>  	rom->ecdsa_verify_signature = *(void **)verify_ptr;
>  }
> diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> index f19a70e53e0..0d39b67178e 100644
> --- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> +++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> @@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
>  
>  /* helper function: read data from OTP */
>  u32 get_otp(int index, int shift, int mask);
> +
> +uintptr_t get_stm32mp_rom_api_table(void);
> +uintptr_t get_stm32mp_bl2_dtb(void);

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH v3 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Patrice CHOTARD]

Hi Marek

On 12/7/22 20:24, Marek Vasut wrote:
> The ROM API table pointer is no longer accessible from U-Boot, fix
> this by passing the ROM API pointer through. This makes it possible
> for U-Boot to call ROM API functions to authenticate payload like
> signed fitImages.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> V2: - Rename image_entry_noargs_t to image_entry_stm32_t
>     - Add missing __noreturn
> V3: No change
> ---
>  arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index ee59866bb73..dc4112d5e6c 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -22,6 +22,7 @@
>  #include <dm/device.h>
>  #include <dm/uclass.h>
>  #include <linux/bitops.h>
> +#include <spl.h>
>  
>  /*
>   * early TLB into the .data section so that it not get cleared
> @@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
>  {
>  	return nt_fw_dtb;
>  }
> +
> +#ifdef CONFIG_SPL_BUILD
> +void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
> +{
> +	typedef void __noreturn (*image_entry_stm32_t)(u32 romapi);
> +	uintptr_t romapi = get_stm32mp_rom_api_table();
> +
> +	image_entry_stm32_t image_entry =
> +		(image_entry_stm32_t)spl_image->entry_point;
> +
> +	printf("image entry point: 0x%lx\n", spl_image->entry_point);
> +	image_entry(romapi);
> +}
> +#endif

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Patrice CHOTARD]

Hi Marek

I am preparing the next STM32 U-Boot pull request, during testing, buidlman is complaining with the following warning:

03: ARM: stm32: Factor out save_boot_params           
       arm:  +   stm32mp13
+../arch/arm/mach-stm32mp/boot_params.c: In function 'board_fdt_blob_setup':
+../arch/arm/mach-stm32mp/boot_params.c:20:35: error: implicit declaration of function 'get_stm32mp_bl2_dtb' [-Werror=implicit-function-declaration]
+   20 |         unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
+      |                                   ^~~~~~~~~~~~~~~~~~~
+cc1: all warnings being treated as errors


Same issue with stm32mp15_defconfig and stm32mp15_trusted_defconfig.

Regards
Patrice


On 1/4/23 10:00, Patrice CHOTARD wrote:
> Hi Marek
> 
> On 12/7/22 20:24, Marek Vasut wrote:
>> The STM32MP15xx platform currently comes with two incompatible
>> implementations of save_boot_params() weak function override.
>> Factor the save_boot_params() implementation into common cpu.c
>> code and provide accessors to read out both ROM API table address
>> and DT address from any place in the code instead.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>> V2: Avoid #if CONFIG... , use if (CONFIG... instead
>> V3: No change
>> ---
>>  arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
>>  arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
>>  arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
>>  .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
>>  4 files changed, 42 insertions(+), 36 deletions(-)
>>
>> diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
>> index e91ef1b2fc7..e40cca938ef 100644
>> --- a/arch/arm/mach-stm32mp/boot_params.c
>> +++ b/arch/arm/mach-stm32mp/boot_params.c
>> @@ -11,30 +11,14 @@
>>  #include <asm/sections.h>
>>  #include <asm/system.h>
>>  
>> -/*
>> - * Force data-section, as .bss will not be valid
>> - * when save_boot_params is invoked.
>> - */
>> -static unsigned long nt_fw_dtb __section(".data");
>> -
>> -/*
>> - * Save the FDT address provided by TF-A in r2 at boot time
>> - * This function is called from start.S
>> - */
>> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
>> -		      unsigned long r3)
>> -{
>> -	nt_fw_dtb = r2;
>> -
>> -	save_boot_params_ret();
>> -}
>> -
>>  /*
>>   * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
>>   * Non Trusted Firmware configuration file) when the pointer is valid
>>   */
>>  void *board_fdt_blob_setup(int *err)
>>  {
>> +	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
>> +
>>  	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
>>  
>>  	*err = 0;
>> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
>> index 855fc755fe0..ee59866bb73 100644
>> --- a/arch/arm/mach-stm32mp/cpu.c
>> +++ b/arch/arm/mach-stm32mp/cpu.c
>> @@ -378,3 +378,38 @@ int arch_misc_init(void)
>>  
>>  	return 0;
>>  }
>> +
>> +/*
>> + * Without forcing the ".data" section, this would get saved in ".bss". BSS
>> + * will be cleared soon after, so it's not suitable.
>> + */
>> +static uintptr_t rom_api_table __section(".data");
>> +static uintptr_t nt_fw_dtb __section(".data");
>> +
>> +/*
>> + * The ROM gives us the API location in r0 when starting. This is only available
>> + * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
>> + * the FDT address provided by TF-A in r2 at boot time. This function is called
>> + * from start.S
>> + */
>> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
>> +		      unsigned long r3)
>> +{
>> +	if (IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY))
>> +		rom_api_table = r0;
>> +
>> +	if (IS_ENABLED(CONFIG_TFABOOT))
>> +		nt_fw_dtb = r2;
>> +
>> +	save_boot_params_ret();
>> +}
>> +
>> +uintptr_t get_stm32mp_rom_api_table(void)
>> +{
>> +	return rom_api_table;
>> +}
>> +
>> +uintptr_t get_stm32mp_bl2_dtb(void)
>> +{
>> +	return nt_fw_dtb;
>> +}
>> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> index 6156526253c..12b42b9d59c 100644
>> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> @@ -24,26 +24,10 @@ struct ecdsa_rom_api {
>>  					   uint32_t ecc_algo);
>>  };
>>  
>> -/*
>> - * Without forcing the ".data" section, this would get saved in ".bss". BSS
>> - * will be cleared soon after, so it's not suitable.
>> - */
>> -static uintptr_t rom_api_loc __section(".data");
>> -
>> -/*
>> - * The ROM gives us the API location in r0 when starting. This is only available
>> - * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
>> - */
>> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
>> -		      unsigned long r3)
>> -{
>> -	rom_api_loc = r0;
>> -	save_boot_params_ret();
>> -}
>> -
>>  static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
>>  {
>> -	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
>> +	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
>> +			       ROM_API_OFFSET_ECDSA_VERIFY;
>>  
>>  	rom->ecdsa_verify_signature = *(void **)verify_ptr;
>>  }
>> diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
>> index f19a70e53e0..0d39b67178e 100644
>> --- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
>> +++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
>> @@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
>>  
>>  /* helper function: read data from OTP */
>>  u32 get_otp(int index, int shift, int mask);
>> +
>> +uintptr_t get_stm32mp_rom_api_table(void);
>> +uintptr_t get_stm32mp_bl2_dtb(void);
> 
> Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> Thanks
> Patrice

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Marek Vasut]

On 1/12/23 16:06, Patrice CHOTARD wrote:
> Hi Marek

Hi,

> I am preparing the next STM32 U-Boot pull request, during testing, buidlman is complaining with the following warning:
> 
> 03: ARM: stm32: Factor out save_boot_params
>         arm:  +   stm32mp13
> +../arch/arm/mach-stm32mp/boot_params.c: In function 'board_fdt_blob_setup':
> +../arch/arm/mach-stm32mp/boot_params.c:20:35: error: implicit declaration of function 'get_stm32mp_bl2_dtb' [-Werror=implicit-function-declaration]
> +   20 |         unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
> +      |                                   ^~~~~~~~~~~~~~~~~~~
> +cc1: all warnings being treated as errors
> 
> 
> Same issue with stm32mp15_defconfig and stm32mp15_trusted_defconfig.

Sorry for the inconvenience.

Does the change below help ?
If so, can you squash it into 2/4 or do you want v4 ?

diff --git a/arch/arm/mach-stm32mp/boot_params.c 
b/arch/arm/mach-stm32mp/boot_params.c
index e40cca938ef..24d04dcf0f9 100644
--- a/arch/arm/mach-stm32mp/boot_params.c
+++ b/arch/arm/mach-stm32mp/boot_params.c
@@ -8,6 +8,7 @@
  #include <common.h>
  #include <log.h>
  #include <linux/libfdt.h>
+#include <asm/arch/sys_proto.h>
  #include <asm/sections.h>
  #include <asm/system.h>

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Patrice CHOTARD]

Hi Marek

On 1/12/23 18:55, Marek Vasut wrote:
> On 1/12/23 16:06, Patrice CHOTARD wrote:
>> Hi Marek
> 
> Hi,
> 
>> I am preparing the next STM32 U-Boot pull request, during testing, buidlman is complaining with the following warning:
>>
>> 03: ARM: stm32: Factor out save_boot_params
>>         arm:  +   stm32mp13
>> +../arch/arm/mach-stm32mp/boot_params.c: In function 'board_fdt_blob_setup':
>> +../arch/arm/mach-stm32mp/boot_params.c:20:35: error: implicit declaration of function 'get_stm32mp_bl2_dtb' [-Werror=implicit-function-declaration]
>> +   20 |         unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
>> +      |                                   ^~~~~~~~~~~~~~~~~~~
>> +cc1: all warnings being treated as errors
>>
>>
>> Same issue with stm32mp15_defconfig and stm32mp15_trusted_defconfig.
> 
> Sorry for the inconvenience.
> 
> Does the change below help ?
> If so, can you squash it into 2/4 or do you want v4 ?

Yes the change you proposed is OK.
I saw that you have submitted a v4 including this fix, i will add it to the next U-Boot STM32 PR.

Thanks for your reactivity ;-)

Patrice
> 
> diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
> index e40cca938ef..24d04dcf0f9 100644
> --- a/arch/arm/mach-stm32mp/boot_params.c
> +++ b/arch/arm/mach-stm32mp/boot_params.c
> @@ -8,6 +8,7 @@
>  #include <common.h>
>  #include <log.h>
>  #include <linux/libfdt.h>
> +#include <asm/arch/sys_proto.h>
>  #include <asm/sections.h>
>  #include <asm/system.h>

Re: [PATCH v3 2/4] ARM: stm32: Factor out save_boot_params

[Marek Vasut]

On 1/13/23 13:55, Patrice CHOTARD wrote:
> Hi Marek

Hello Patrice,

> On 1/12/23 18:55, Marek Vasut wrote:
>> On 1/12/23 16:06, Patrice CHOTARD wrote:
>>> Hi Marek
>>
>> Hi,
>>
>>> I am preparing the next STM32 U-Boot pull request, during testing, buidlman is complaining with the following warning:
>>>
>>> 03: ARM: stm32: Factor out save_boot_params
>>>          arm:  +   stm32mp13
>>> +../arch/arm/mach-stm32mp/boot_params.c: In function 'board_fdt_blob_setup':
>>> +../arch/arm/mach-stm32mp/boot_params.c:20:35: error: implicit declaration of function 'get_stm32mp_bl2_dtb' [-Werror=implicit-function-declaration]
>>> +   20 |         unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
>>> +      |                                   ^~~~~~~~~~~~~~~~~~~
>>> +cc1: all warnings being treated as errors
>>>
>>>
>>> Same issue with stm32mp15_defconfig and stm32mp15_trusted_defconfig.
>>
>> Sorry for the inconvenience.
>>
>> Does the change below help ?
>> If so, can you squash it into 2/4 or do you want v4 ?
> 
> Yes the change you proposed is OK.
> I saw that you have submitted a v4 including this fix, i will add it to the next U-Boot STM32 PR.

Thanks !

> Thanks for your reactivity ;-)

:)