| Message ID | 20231107161802.855154-8-thomas.richard@bootlin.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Suspend to RAM support for K3 J7200 | expand |
On Tue, Nov 07, 2023 at 05:18:01PM +0100, Thomas Richard wrote: > During the boot a copy of DM-Firmware is done in a reserved memory > area before it starts. > When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit > image. > TF-A which saved itself in this same memory area, is restored in > SRAM by R5 SPL. > > Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com> > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> > > --- > > Changes in v2: > - Check if TF-A is running in DRAM, if yes no need to restore it > - Remove BL31_START macro, and get TF-A start address from the fit image > > arch/arm/mach-k3/common.c | 48 ++++++++++++++++++++++- > arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++ > arch/arm/mach-k3/sysfw-loader.c | 9 +++-- > 3 files changed, 81 insertions(+), 4 deletions(-) One problem here is we aren't updating something under doc/, possibly doc/board/ti/k3.rst unless you want to start populating something entirely new under doc/develop/. And then using kernel-doc comments here and including the code in the rst. And so this is a global comment to the series as well, and I'll leave sorting out when/where to introduce that part up to your discretion. [snip] > diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h > index e8947917a6..8e0f141ed6 100644 > --- a/arch/arm/mach-k3/include/mach/j721e_spl.h > +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h > @@ -42,4 +42,32 @@ > #define K3_PRIMARY_BOOTMODE 0x0 > #define K3_BACKUP_BOOTMODE 0x1 > > +/* Starting buffer address is 1MB before the stack address in DDR */ > +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M) Assuming this is the full level of configurability we need to BUFFER_ADDR please add some Kconfig logic to make sure we can't not set CONFIG_SPL_STACK_R_ADDR. That might take a little trial-and-error and if we just cannot enforce this via Kconfig, let me know and I'll think about what we want to do instead next. > +/* This is actually the whole size of the SRAM */ > +#define BL31_SIZE 0x20000 If this is all of SRAM then is there not already a define? > +/* This address belongs to a reserved memory region for the point of view of > + * Linux, U-boot SPL must use the same address to restore TF-A and resume > + * entry point address > + */ > +#define LPM_SAVE 0xA5000000 > +#define LPM_BL31_SAVE LPM_SAVE > +#define LPM_BL31_RESUME_SAVE LPM_BL31_SAVE + BL31_SIZE > +#define LPM_BL31_START_SAVE LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__ > +#define LPM_DM_SAVE LPM_BL31_START_SAVE + __SIZEOF_POINTER__ Is any of this somewhere in the devicetree as well? I'm not saying "and so pull it from that!" is a must, but I'd like to at least know "that would be horrible to do", and fall back to having this needed co-operation documented somewhere and ideally somewhere / somehow that getting the values wrong is clear. Debugging resume failures at run-time is much more painful than build time. > +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an > + * over memory section. > + * The resume address of TF-A is also saved in DRAM. > + * At build time we don't know the DM-Firmware size, so we keep 512k to > + * save it. > + */ > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM) > +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR) > +#error Not enough space to save DM-Firmware, TF-A and context for S2R > +#endif > +#endif This is really the kind of thing I'd like to see enforced via Kconfig if at all possible. Sometimes yes we just have to CPP and #error, I just want to know we can't.
On 11/7/23 10:18 AM, Thomas Richard wrote: > During the boot a copy of DM-Firmware is done in a reserved memory > area before it starts. > When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit > image. > TF-A which saved itself in this same memory area, is restored in > SRAM by R5 SPL. > > Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com> > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> > > --- > > Changes in v2: > - Check if TF-A is running in DRAM, if yes no need to restore it > - Remove BL31_START macro, and get TF-A start address from the fit image > > arch/arm/mach-k3/common.c | 48 ++++++++++++++++++++++- > arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++ > arch/arm/mach-k3/sysfw-loader.c | 9 +++-- > 3 files changed, 81 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c > index a35110429b..737a1a28c6 100644 > --- a/arch/arm/mach-k3/common.c > +++ b/arch/arm/mach-k3/common.c > @@ -26,6 +26,7 @@ > #include <env.h> > #include <elf.h> > #include <soc.h> > +#include <hang.h> > > #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) > enum { > @@ -221,6 +222,11 @@ void release_resources_for_core_shutdown(void) > } > } > > +__weak int board_is_resuming(void) > +{ > + return 0; > +} > + > void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) > { > typedef void __noreturn (*image_entry_noargs_t)(void); > @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) > if (ret) > panic("rproc failed to be initialized (%d)\n", ret); > > + if (board_is_resuming()) { > +#if IS_ENABLED(CONFIG_SOC_K3_J721E) > + if (!valid_elf_image(LPM_DM_SAVE)) > + panic("%s: DM-Firmware image is not valid, it cannot be loaded\n", > + __func__); > + > + loadaddr = load_elf_image_phdr(LPM_DM_SAVE); > + > + /* > + * Check if the start address of TF-A is in DRAM. > + * If not it means TF-A was running in SRAM, so it shall be > + * restored. > + */ > + if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE) > + memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE), > + (void *)LPM_BL31_SAVE, BL31_SIZE); This will not work. The memory where TF-A is running will be firewalled and SPL absolutely cannot be securely trusted to load TF-A. Especially from an unencrypted location in DDR. TF-A must be loaded as it is today using signed certificate images. You should know this, I explained it all when you tried the same in TF-A: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992 NAK Andrew > + > + ret = rproc_load(1, *(ulong *)(LPM_BL31_RESUME_SAVE), BL31_SIZE); > + if (ret) > + panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret); > + > + debug("%s: jumping to address %x\n", __func__, loadaddr); > + goto start_arm64; > +#endif > + } > + > init_env(); > > if (!fit_image_info[IMAGE_ID_DM_FW].image_start) { > @@ -250,6 +282,10 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) > fit_image_info[IMAGE_ID_ATF].image_start = > spl_image->entry_point; > > +#if IS_ENABLED(CONFIG_SOC_K3_J721E) > + *(uintptr_t *)(LPM_BL31_START_SAVE) = fit_image_info[IMAGE_ID_ATF].image_start; > +#endif > + > ret = rproc_load(1, fit_image_info[IMAGE_ID_ATF].image_start, 0x200); > if (ret) > panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret); > @@ -289,8 +325,18 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) > loadaddr = load_elf_image_phdr(loadaddr); > } else { > loadaddr = fit_image_info[IMAGE_ID_DM_FW].image_start; > - if (valid_elf_image(loadaddr)) > + if (valid_elf_image(loadaddr)) { > loadaddr = load_elf_image_phdr(loadaddr); > +#if IS_ENABLED(CONFIG_SOC_K3_J721E) > + if (fit_image_info[IMAGE_ID_DM_FW].image_len > (BUFFER_ADDR - LPM_DM_SAVE)) > + log_warning("%s\n: Not enough space to save DM-Firmware", > + __func__); > + else > + memcpy((void *)LPM_DM_SAVE, > + (void *)fit_image_info[IMAGE_ID_DM_FW].image_start, > + fit_image_info[IMAGE_ID_DM_FW].image_len); > +#endif > + } > } > > debug("%s: jumping to address %x\n", __func__, loadaddr); > diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h > index e8947917a6..8e0f141ed6 100644 > --- a/arch/arm/mach-k3/include/mach/j721e_spl.h > +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h > @@ -42,4 +42,32 @@ > #define K3_PRIMARY_BOOTMODE 0x0 > #define K3_BACKUP_BOOTMODE 0x1 > > +/* Starting buffer address is 1MB before the stack address in DDR */ > +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M) > + > +/* This is actually the whole size of the SRAM */ > +#define BL31_SIZE 0x20000 > + > +/* This address belongs to a reserved memory region for the point of view of > + * Linux, U-boot SPL must use the same address to restore TF-A and resume > + * entry point address > + */ > +#define LPM_SAVE 0xA5000000 > +#define LPM_BL31_SAVE LPM_SAVE > +#define LPM_BL31_RESUME_SAVE LPM_BL31_SAVE + BL31_SIZE > +#define LPM_BL31_START_SAVE LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__ > +#define LPM_DM_SAVE LPM_BL31_START_SAVE + __SIZEOF_POINTER__ > + > +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an > + * over memory section. > + * The resume address of TF-A is also saved in DRAM. > + * At build time we don't know the DM-Firmware size, so we keep 512k to > + * save it. > + */ > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM) > +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR) > +#error Not enough space to save DM-Firmware, TF-A and context for S2R > +#endif > +#endif > + > #endif > diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c > index 9be2d9eaea..e6d452e590 100644 > --- a/arch/arm/mach-k3/sysfw-loader.c > +++ b/arch/arm/mach-k3/sysfw-loader.c > @@ -84,13 +84,16 @@ static bool sysfw_loaded; > static void *sysfw_load_address; > > /* > - * Populate SPL hook to override the default load address used by the SPL > - * loader function with a custom address for SYSFW loading. > + * Populate SPL hook to override the default load address used by the > + * SPL loader function with a custom address for SYSFW loading. In > + * other case use also a custom address located in a reserved memory > + * region. It ensures that Linux memory won't be corrupted by SPL during > + * suspend to ram. > */ > struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size) > { > if (sysfw_loaded) > - return (struct legacy_img_hdr *)(CONFIG_TEXT_BASE + offset); > + return (struct legacy_img_hdr *)(BUFFER_ADDR + offset); > else if (sysfw_load_address) > return sysfw_load_address; > else
On 11/8/23 18:30, Andrew Davis wrote: >> void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) >> { >> typedef void __noreturn (*image_entry_noargs_t)(void); >> @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct >> spl_image_info *spl_image) >> if (ret) >> panic("rproc failed to be initialized (%d)\n", ret); >> + if (board_is_resuming()) { >> +#if IS_ENABLED(CONFIG_SOC_K3_J721E) >> + if (!valid_elf_image(LPM_DM_SAVE)) >> + panic("%s: DM-Firmware image is not valid, it cannot be >> loaded\n", >> + __func__); >> + >> + loadaddr = load_elf_image_phdr(LPM_DM_SAVE); >> + >> + /* >> + * Check if the start address of TF-A is in DRAM. >> + * If not it means TF-A was running in SRAM, so it shall be >> + * restored. >> + */ >> + if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE) >> + memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE), >> + (void *)LPM_BL31_SAVE, BL31_SIZE); > > This will not work. The memory where TF-A is running will be firewalled and > SPL absolutely cannot be securely trusted to load TF-A. Especially from an > unencrypted location in DDR. TF-A must be loaded as it is today using > signed > certificate images. You should know this, I explained it all when you tried > the same in TF-A: > > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992 Hi Andrew, We understood that GP devices are not impacted (we had this information from TI, probably Manorit I don't remember), and Manorit confirmed it in the TF-A review. Maybe I could add a check of the device type to not impact HS devices. Regards, Thomas > > NAK > > Andrew >
On 11/9/23 5:29 AM, Thomas Richard wrote: > On 11/8/23 18:30, Andrew Davis wrote: >>> void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) >>> { >>> typedef void __noreturn (*image_entry_noargs_t)(void); >>> @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct >>> spl_image_info *spl_image) >>> if (ret) >>> panic("rproc failed to be initialized (%d)\n", ret); >>> + if (board_is_resuming()) { >>> +#if IS_ENABLED(CONFIG_SOC_K3_J721E) >>> + if (!valid_elf_image(LPM_DM_SAVE)) >>> + panic("%s: DM-Firmware image is not valid, it cannot be >>> loaded\n", >>> + __func__); >>> + >>> + loadaddr = load_elf_image_phdr(LPM_DM_SAVE); >>> + >>> + /* >>> + * Check if the start address of TF-A is in DRAM. >>> + * If not it means TF-A was running in SRAM, so it shall be >>> + * restored. >>> + */ >>> + if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE) >>> + memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE), >>> + (void *)LPM_BL31_SAVE, BL31_SIZE); >> >> This will not work. The memory where TF-A is running will be firewalled and >> SPL absolutely cannot be securely trusted to load TF-A. Especially from an >> unencrypted location in DDR. TF-A must be loaded as it is today using >> signed >> certificate images. You should know this, I explained it all when you tried >> the same in TF-A: >> >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992 > > Hi Andrew, > > We understood that GP devices are not impacted (we had this information > from TI, probably Manorit I don't remember), and Manorit confirmed it in > the TF-A review. > > Maybe I could add a check of the device type to not impact HS devices. > I'm not interested in GP devices, and neither are most our customers. Those are development devices, customers go to production with secured devices. Saying "let's make it work on GP only, then we will figure it out on HS later" was a mistake we made back in OMAP class device days. It made bringing support to production secured devices (HS) miserable as we had to unroll all the hacks that only worked on the development devices (GP). Your method here is completely unusable on HS and will need a ground up rewrite for HS. Since the solution for HS will also work on GP, but not the other way around, you need to start with the HS solution. I'll make this same point over on the TF-A review then let's continue discussion over there only. If you cant get the TF-A part in then no need for this U-Boot part. Andrew > Regards, > > Thomas > >> >> NAK >> >> Andrew >> >
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index a35110429b..737a1a28c6 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -26,6 +26,7 @@ #include <env.h> #include <elf.h> #include <soc.h> +#include <hang.h> #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) enum { @@ -221,6 +222,11 @@ void release_resources_for_core_shutdown(void) } } +__weak int board_is_resuming(void) +{ + return 0; +} + void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) { typedef void __noreturn (*image_entry_noargs_t)(void); @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) if (ret) panic("rproc failed to be initialized (%d)\n", ret); + if (board_is_resuming()) { +#if IS_ENABLED(CONFIG_SOC_K3_J721E) + if (!valid_elf_image(LPM_DM_SAVE)) + panic("%s: DM-Firmware image is not valid, it cannot be loaded\n", + __func__); + + loadaddr = load_elf_image_phdr(LPM_DM_SAVE); + + /* + * Check if the start address of TF-A is in DRAM. + * If not it means TF-A was running in SRAM, so it shall be + * restored. + */ + if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE) + memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE), + (void *)LPM_BL31_SAVE, BL31_SIZE); + + ret = rproc_load(1, *(ulong *)(LPM_BL31_RESUME_SAVE), BL31_SIZE); + if (ret) + panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret); + + debug("%s: jumping to address %x\n", __func__, loadaddr); + goto start_arm64; +#endif + } + init_env(); if (!fit_image_info[IMAGE_ID_DM_FW].image_start) { @@ -250,6 +282,10 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) fit_image_info[IMAGE_ID_ATF].image_start = spl_image->entry_point; +#if IS_ENABLED(CONFIG_SOC_K3_J721E) + *(uintptr_t *)(LPM_BL31_START_SAVE) = fit_image_info[IMAGE_ID_ATF].image_start; +#endif + ret = rproc_load(1, fit_image_info[IMAGE_ID_ATF].image_start, 0x200); if (ret) panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret); @@ -289,8 +325,18 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) loadaddr = load_elf_image_phdr(loadaddr); } else { loadaddr = fit_image_info[IMAGE_ID_DM_FW].image_start; - if (valid_elf_image(loadaddr)) + if (valid_elf_image(loadaddr)) { loadaddr = load_elf_image_phdr(loadaddr); +#if IS_ENABLED(CONFIG_SOC_K3_J721E) + if (fit_image_info[IMAGE_ID_DM_FW].image_len > (BUFFER_ADDR - LPM_DM_SAVE)) + log_warning("%s\n: Not enough space to save DM-Firmware", + __func__); + else + memcpy((void *)LPM_DM_SAVE, + (void *)fit_image_info[IMAGE_ID_DM_FW].image_start, + fit_image_info[IMAGE_ID_DM_FW].image_len); +#endif + } } debug("%s: jumping to address %x\n", __func__, loadaddr); diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h index e8947917a6..8e0f141ed6 100644 --- a/arch/arm/mach-k3/include/mach/j721e_spl.h +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h @@ -42,4 +42,32 @@ #define K3_PRIMARY_BOOTMODE 0x0 #define K3_BACKUP_BOOTMODE 0x1 +/* Starting buffer address is 1MB before the stack address in DDR */ +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M) + +/* This is actually the whole size of the SRAM */ +#define BL31_SIZE 0x20000 + +/* This address belongs to a reserved memory region for the point of view of + * Linux, U-boot SPL must use the same address to restore TF-A and resume + * entry point address + */ +#define LPM_SAVE 0xA5000000 +#define LPM_BL31_SAVE LPM_SAVE +#define LPM_BL31_RESUME_SAVE LPM_BL31_SAVE + BL31_SIZE +#define LPM_BL31_START_SAVE LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__ +#define LPM_DM_SAVE LPM_BL31_START_SAVE + __SIZEOF_POINTER__ + +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an + * over memory section. + * The resume address of TF-A is also saved in DRAM. + * At build time we don't know the DM-Firmware size, so we keep 512k to + * save it. + */ +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM) +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR) +#error Not enough space to save DM-Firmware, TF-A and context for S2R +#endif +#endif + #endif diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c index 9be2d9eaea..e6d452e590 100644 --- a/arch/arm/mach-k3/sysfw-loader.c +++ b/arch/arm/mach-k3/sysfw-loader.c @@ -84,13 +84,16 @@ static bool sysfw_loaded; static void *sysfw_load_address; /* - * Populate SPL hook to override the default load address used by the SPL - * loader function with a custom address for SYSFW loading. + * Populate SPL hook to override the default load address used by the + * SPL loader function with a custom address for SYSFW loading. In + * other case use also a custom address located in a reserved memory + * region. It ensures that Linux memory won't be corrupted by SPL during + * suspend to ram. */ struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size) { if (sysfw_loaded) - return (struct legacy_img_hdr *)(CONFIG_TEXT_BASE + offset); + return (struct legacy_img_hdr *)(BUFFER_ADDR + offset); else if (sysfw_load_address) return sysfw_load_address; else