[PATCH] vexpress64: also consider DTB pointer in x1

[Peter Hoyes]

On 21/09/2022 18:09, Andre Przywara wrote:
> Commit c0fce929564f("vexpress64: fvp: enable OF_CONTROL") added code to
> consider a potential DTB address being passed in the x0 register, or
> revert to the built-in DTB otherwise.
> The former case was used when using the boot-wrapper, to which we sell
> U-Boot as a Linux kernel. The latter was meant for TF-A, for which we
> couldn't find an easy way to use the DTB it uses itself. We have some
> quirk to filter for a valid DTB, as TF-A happens to pass a pointer to
> some special devicetree blob in x0 as well.
>
> Now the TF-A case is broken, when enabling proper emulation of secure
> memory (-C bp.secure_memory=1). TF-A carves out some memory at the top
> of the first DRAM bank for its own purposes, and configures the
> TrustZone DRAM controller to make this region secure-only. U-Boot will
> then hang when it tries to relocate itself exactly to the end of DRAM.
> TF-A announces this by carving out that region of the /memory node, in
> the DT it passes on to BL33 in x1, but we miss that so far.
>
> Instead of repeating this carveout in our DT copy, let's try to look for
> a DTB at the address x1 points to as well. This will let U-Boot pick up
> the DTB provided by TF-A, which has the correct carveout in place,
> avoiding the hang.
> While we are at it, make the detection more robust: the length test (is
> the DT larger than 256 bytes?) is too fragile, in fact the TF-A port for
> a new FVP model already exceeds this. So we test x1 first, consider 0
> an invalid address, and also require a /memory node to detect a valid DTB.
>
> And for the records:
> Some asking around revealed what is really going on with TF-A and that
> ominous DTB pointer in x0: TF-A expects EDK-2 as its non-secure payload
> (BL33), and there apparently was some long-standing ad-hoc boot protocol
> defined just between the two: x0 would carry the MPIDR register value of
> the boot CPU, and the hardware DTB address would be stored in x1.
> Now the MPIDR of CPU 0 is typically 0, plus bit 31 set, which is defined
> as RES1 in the ARMv7 and ARMv8 architectures. This gives 0x80000000,
> which is the same value as the address of the beginning of DRAM (2GB).
> And coincidentally TF-A put some DTB structure exactly there, for its
> own purposes (passing it between stages). So U-Boot was trying to use
> this DTB, which requires the quirk to check for its validity.
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

Thanks for getting to the bottom of this issue Andre. You can add:

Tested-by: Peter Hoyes <peter.hoyes at arm.com>

> ---
>   board/armltd/vexpress64/lowlevel_init.S |  2 +-
>   board/armltd/vexpress64/vexpress64.c    | 27 +++++++++++++++++++++----
>   2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S
> index 3dcfb85d0e9..68ca3f26e96 100644
> --- a/board/armltd/vexpress64/lowlevel_init.S
> +++ b/board/armltd/vexpress64/lowlevel_init.S
> @@ -7,6 +7,6 @@
>   save_boot_params:
>   
>   	adr	x8, prior_stage_fdt_address
> -	str	x0, [x8]
> +	stp	x0, x1, [x8]
>   
>   	b	save_boot_params_ret
> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
> index 05a7a25c32e..af326dc6f45 100644
> --- a/board/armltd/vexpress64/vexpress64.c
> +++ b/board/armltd/vexpress64/vexpress64.c
> @@ -100,7 +100,7 @@ int dram_init_banksize(void)
>    * Push the variable into the .data section so that it
>    * does not get cleared later.
>    */
> -unsigned long __section(".data") prior_stage_fdt_address;
> +unsigned long __section(".data") prior_stage_fdt_address[2];
>   
>   #ifdef CONFIG_OF_BOARD
>   
> @@ -151,6 +151,23 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname)
>   }
>   #endif
>   
> +/*
> + * Filter for a valid DTB, as TF-A happens to provide a pointer to some
> + * data structure using the DTB format, which we cannot use.
> + * The address of the DTB cannot be 0, in fact this is the reserved value
> + * for x1 in the kernel boot protocol.
> + * And while the nt_fw_config.dtb used by TF-A is a valid DTB structure, it
> + * does not contain the typical nodes and properties, which we test for by
> + * probing for the mandatory /memory node.
> + */
> +static bool is_valid_dtb(uintptr_t dtb_ptr)
> +{
> +	if (dtb_ptr == 0 || fdt_magic(dtb_ptr) != FDT_MAGIC)
> +		return false;
> +
> +	return fdt_subnode_offset((void *)dtb_ptr, 0, "memory") >= 0;
> +}
> +
>   void *board_fdt_blob_setup(int *err)
>   {
>   #ifdef CONFIG_TARGET_VEXPRESS64_JUNO
> @@ -172,10 +189,12 @@ void *board_fdt_blob_setup(int *err)
>   	}
>   #endif
>   
> -	if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC &&
> -	    fdt_totalsize(prior_stage_fdt_address) > 0x100) {
> +	if (is_valid_dtb(prior_stage_fdt_address[1])) {
> +		*err = 0;
> +		return (void *)prior_stage_fdt_address[1];
> +	} else if (is_valid_dtb(prior_stage_fdt_address[0])) {
>   		*err = 0;
> -		return (void *)prior_stage_fdt_address;
> +		return (void *)prior_stage_fdt_address[0];
>   	}
>   
>   	if (fdt_magic(gd->fdt_blob) == FDT_MAGIC) {