Re: [PATCH 3/3] arm64: renesas: Add TFA BL31 handoff support

Quentin Schulz
2025-01-29T16:28:29Z

On 1/29/25 5:02 PM, Quentin Schulz wrote:
> Hi Marek,
>
> On 1/12/25 11:36 PM, Marek Vasut wrote:
>> Implement custom U_BOOT_FIT_LOADABLE_HANDLER and
>> armv8_switch_to_el2_prep()
>> handling in case the TFA was loaded. The loadables handler sets up custom
>> handoff structure used by Renesas TFA fork in fixed location in DRAM and
>> indicates the TFA has been loaded.
>>
>> The custom armv8_switch_to_el2_prep() handling tests whether the TFA BL31
>> was previously loaded and the custom handoff structure was set up, and if
>> so, jumps to TFA BL31 which switches from EL3 to EL2 and then returns to
>> U-Boot just past bl in armv8_switch_to_el2() to finish starting the Linux
>> kernel.
>>
>> The jump to Linux through TFA works in such a way that the custom
>> armv8_switch_to_el2_prep() handler configures the custom handoff
>> structure
>> such that the target jump address of the TFA BL31 on exit is set to the
>> armv8_switch_to_el2() + 4, which is just past the bl, and just before the
>> U-Boot code which implements the Linux kernel boot from either EL. The
>> registers passed through the TFA BL31 are all the registers passed into
>> armv8_switch_to_el2_prep() to assure maximum compatibility with all the
>> boot modes. The armv8_switch_to_el2_prep() handler jumps to the TFA BL31,
>> which does its setup, drops EL from EL3 to EL2 and finally jumps to the
>> armv8_switch_to_el2() + 4 entry point, which then allows U-Boot to boot
>> the Linux kernel the usual way.
>>
>> In order to build suitable kernel fitImage, build TFA first, temporarily
>> from downstream repository:
>> remote: https://eur02.safelinks.protection.outlook.com/?
>> url=https%3A%2F%2Fgithub.com%2Frenesas-rcar%2Farm-trusted-
>> firmware.git&data=05%7C02%7Cquentin.schulz%40cherry.de%7C6ab2eb623e32425379f108dd407e5a7c%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638737633628498421%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=DWQfUm5mp9%2FMCUxvFFbO1yfjKlvmtfiOD8VTkN%2BEZKk%3D&reserved=0
>> branch: rcar_gen4_v2.7_v4x
>>
>> ```
>> $ git clean -fqdx
>> $ MBEDTLS_DIR=/path/to/mbedtls/ make -j$(nproc) bl31 \
>>     PLAT=rcar_gen4 ARCH=aarch64 LSI=V4H SPD=none \
>>     CTX_INCLUDE_AARCH32_REGS=0 MBEDTLS_COMMON_MK=1 \
>>     PTP_NONSECURE_ACCESS=1 LOG_LEVEL=20 DEBUG=0 \
>>     ENABLE_ASSERTIONS=0 E=0
>> ```
>>
>> Build Linux kernel Image and device tree from current mainline Linux
>> kernel repository, obtain 'Image' and 'r8a779g0-white-hawk.dtb' .
>>
>> Bundle the files together using provided fit-image.its fitImage
>> description:
>> ```
>> $ mkimage -f fit-image.its fitImage
>> ```
>>
>> To start the kernel fiImage generated in previous step, load fitImage
>> to DRAM and use the 'bootm' command to start it:
>> => load 0x58000000 ... fitImage && bootm 0x58000000
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> ---
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Caleb Connolly <caleb.connolly@linaro.org>
>> Cc: Igor Opaniuk <igor.opaniuk@gmail.com>
>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> Cc: Julien Masson <jmasson@baylibre.com>
>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Cc: Maxim Moskalets <maximmosk4@gmail.com>
>> Cc: Michael Walle <mwalle@kernel.org>
>> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
>> Cc: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Cc: Paul Barker <paul.barker.ct@bp.renesas.com>
>> Cc: Paul-Erwan Rio <paulerwan.rio@gmail.com>
>> Cc: Peter Hoyes <Peter.Hoyes@arm.com>
>> Cc: Raymond Mao <raymond.mao@linaro.org>
>> Cc: Sam Protsenko <semen.protsenko@linaro.org>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: u-boot@lists.denx.de
>> ---
>>   board/renesas/common/gen4-common.c | 118 +++++++++++++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>
>> diff --git a/board/renesas/common/gen4-common.c b/board/renesas/
>> common/gen4-common.c
>> index 52a0639073b..c7f3f9a30ab 100644
>> --- a/board/renesas/common/gen4-common.c
>> +++ b/board/renesas/common/gen4-common.c
>> @@ -7,11 +7,13 @@
>>   #include <asm/arch/renesas.h>
>>   #include <asm/arch/sys_proto.h>
>> +#include <asm/armv8/mmu.h>
>>   #include <asm/global_data.h>
>>   #include <asm/io.h>
>>   #include <asm/mach-types.h>
>>   #include <asm/processor.h>
>>   #include <asm/system.h>
>> +#include <image.h>
>>   #include <linux/errno.h>
>>   #define RST_BASE    0xE6160000 /* Domain0 */
>> @@ -88,3 +90,119 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>>   {
>>       return 0;
>>   }
>> +
>> +/* R-Car Gen4 TFA BL31 handoff structure and handling. */
>> +struct param_header {
>> +    u8            type;
>> +    u8            version;
>> +    u16            size;
>> +    u32            attr;
>> +};
>> +
>> +struct tfa_image_info {
>> +    struct param_header    h;
>> +    uintptr_t        image_base;
>> +    u32            image_size;
>> +    u32            image_max_size;
>> +};
>> +
>> +struct aapcs64_params {
>> +    u64            arg0;
>> +    u64            arg1;
>> +    u64            arg2;
>> +    u64            arg3;
>> +    u64            arg4;
>> +    u64            arg5;
>> +    u64            arg6;
>> +    u64            arg7;
>> +};
>> +
>> +struct entry_point_info {
>> +    struct param_header    h;
>> +    uintptr_t        pc;
>> +    u32            spsr;
>> +    struct aapcs64_params    args;
>> +};
>> +
>> +struct bl2_to_bl31_params_mem {
>> +    struct tfa_image_info    bl32_image_info;
>> +    struct tfa_image_info    bl33_image_info;
>> +    struct entry_point_info    bl33_ep_info;
>> +    struct entry_point_info    bl32_ep_info;
>> +};
>> +
>> +/* Default jump address, return to U-Boot */
>> +#define BL33_BASE    0x44100000
>> +/* Custom parameters address passed to TFA by ICUMXA loader */
>> +#define PARAMS_BASE    0x46422200
>> +
>
> Shouldn't we rather set those as offset compared to the load/entry
> address passed in the ITS?
>
>> +/* Usually such a structure is produced by ICUMXA and passed in at
>> 0x46422200 */
>> +static const struct bl2_to_bl31_params_mem blinfo_template = {
>> +    .bl33_ep_info.h.type = 1,    /* PARAM_EP */
>> +    .bl33_ep_info.h.version = 2,    /* Version 2 */
>> +    .bl33_ep_info.h.size = sizeof(struct entry_point_info),
>> +    .bl33_ep_info.h.attr = 0x81,    /* Executable | Non-Secure */
>> +    .bl33_ep_info.spsr = 0x2c9,    /* Mode=EL2, SP=ELX,
>> Exceptions=OFF */
>> +    .bl33_ep_info.pc = BL33_BASE,
>> +
>> +    .bl33_image_info.h.type = 1,    /* PARAM_EP */
>> +    .bl33_image_info.h.version = 2,    /* Version 2 */
>> +    .bl33_image_info.h.size = sizeof(struct image_info),
>> +    .bl33_image_info.h.attr = 0,
>> +    .bl33_image_info.image_base = BL33_BASE,
>> +};
>> +
>> +static bool tfa_bl31_image_loaded;
>> +static ulong tfa_bl31_image_addr;
>> +
>> +static void tfa_bl31_image_process(ulong image, size_t size)
>> +{
>> +    /* Custom parameters address passed to TFA by ICUMXA loader */
>> +    struct bl2_to_bl31_params_mem *blinfo = (struct
>> bl2_to_bl31_params_mem *)PARAMS_BASE;
>> +
>> +    /* Clear a page and copy template */
>> +    memset((void *)PARAMS_BASE, 0, PAGE_SIZE);
>> +    memcpy(blinfo, &blinfo_template, sizeof(*blinfo));
>> +    tfa_bl31_image_addr = image;
>> +    tfa_bl31_image_loaded = true;
>> +}
>> +
>> +U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_TFA_BL31, tfa_bl31_image_process);
>> +
>> +void armv8_switch_to_el2_prep(u64 args, u64 mach_nr, u64 fdt_addr,
>> +                  u64 arg4, u64 entry_point, u64 es_flag)
>> +{
>> +    typedef void __noreturn (*image_entry_noargs_t)(void);
>> +    image_entry_noargs_t image_entry =
>> +        (image_entry_noargs_t)(void *)tfa_bl31_image_addr;
>> +    struct bl2_to_bl31_params_mem *blinfo =
>> +        (struct bl2_to_bl31_params_mem *)PARAMS_BASE;
>> +
>> +    /*
>> +     * Destination address in arch/arm/cpu/armv8/transition.S
>> +     * right past the first bl in armv8_switch_to_el2() to let
>> +     * the rest of U-Boot pre-Linux code run. The code does run
>> +     * without stack pointer!
>> +     */
>> +    const u64 ep = ((u64)(uintptr_t)&armv8_switch_to_el2) + 4;
>> +
>> +    /* If TFA BL31 was not part of the fitImage, do regular boot. */
>> +    if (!tfa_bl31_image_loaded)
>> +        return;
>> +
>> +    /*
>> +     * Set up kernel entry point and parameters:
>> +     * x0 is FDT address, x1..x3 must be 0
>> +     */
>> +    blinfo->bl33_ep_info.pc = ep;
>> +    blinfo->bl33_ep_info.args.arg0 = args;
>> +    blinfo->bl33_ep_info.args.arg1 = mach_nr;
>> +    blinfo->bl33_ep_info.args.arg2 = fdt_addr;
>> +    blinfo->bl33_ep_info.args.arg3 = arg4;
>> +    blinfo->bl33_ep_info.args.arg4 = entry_point;
>> +    blinfo->bl33_ep_info.args.arg5 = es_flag;
>> +    blinfo->bl33_image_info.image_base = ep;
>> +
>> +    /* Jump to TFA BL31 */
>> +    image_entry();
>> +}
>
> Shouldn't we have a weak implementation that SoC vendor can override if
> they feel like it?
>
> To me this feels like it could be much quicker adopted if we had some
> default people could try out for their boards.
>
> We already have a generic spl_invoke_atf() so why not the same for this
> mechanism? As far as I could tell, we get the load address of TEE and U-
> Boot proper (bl32 and bl33) and pass it the FDT (or nothing if
> SPL_ATF_NO_PLATFORM_PARAM symbol set).
>
> I am not too sure who and how TEE will be started as it needs to be
> executing in EL3 too I believe? So we may still have to provide that
> one? BL33 would be the kernel params like you did in this patch. Then
> the bl31 param could also be either the fdt or nothing if
> ATF_NO_PLATFORM_PARAM is set.
>

Actually, I would highly prefer we don't even have this symbol and have
instead a property in the TF-A image node in the ITS to say whether
we're allowed to pass parameters or not to BL31? The user bundling TF-A
inside the linux.itb should know (well, we should probably know also
when building BL31 within u-boot.itb....).

We actually need this symbol on all Rockchip boards using the Rockchip
blob and not upstream. But this also prevents us from using some
featureset of the upstream TF-A if we rebuild without disabling the
symbol. It'd be nice if we don't have to rebuild U-Boot depending on
which variant of BL31 we use on Rockchip, this info should be stored in
the linux.itb is what I'm saying. This doesn't need to be in this
introducing commit though I believe, we can work something out later I
guess.

Cheers,
Quentin