[PATCH] vexpress64: also consider DTB pointer in x1

[PATCH] vexpress64: also consider DTB pointer in x1

[Andre Przywara]

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@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) {
-- 
2.25.1

Re: [PATCH] vexpress64: also consider DTB pointer in x1

[Tom Rini]

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

On Wed, Sep 21, 2022 at 06:09:46PM +0100, 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.

So, follow-up question. Why don't we just always look at x1 and never x0
for the DTB? That would seem to be a bugfix and should work on older
TF-A releases too, yes?

-- 
Tom

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

Re: [PATCH] vexpress64: also consider DTB pointer in x1

[Andre Przywara]

On Wed, 21 Sep 2022 13:56:44 -0400
Tom Rini <trini@konsulko.com> wrote:

Hi Tom,

> On Wed, Sep 21, 2022 at 06:09:46PM +0100, 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.  
> 
> So, follow-up question. Why don't we just always look at x1 and never x0
> for the DTB? That would seem to be a bugfix and should work on older
> TF-A releases too, yes?

There is a use case where the boot-wrapper[1] runs U-Boot, as a kind of
poor-man's TF-A replacement. As the boot-wrapper normally just runs
kernels, it follows the arm64 Linux kernel boot protocol, which puts
the DTB address in x0. So to stay flexible, we look at both registers.
The kernel boot protocol says x1 must be 0, so we check for that first,
to sort that out.

Thanks,
Andre

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-aarch64.git/

Re: [PATCH] vexpress64: also consider DTB pointer in x1

[Tom Rini]

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

On Thu, Sep 22, 2022 at 01:03:04AM +0100, Andre Przywara wrote:
> On Wed, 21 Sep 2022 13:56:44 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi Tom,
> 
> > On Wed, Sep 21, 2022 at 06:09:46PM +0100, 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.  
> > 
> > So, follow-up question. Why don't we just always look at x1 and never x0
> > for the DTB? That would seem to be a bugfix and should work on older
> > TF-A releases too, yes?
> 
> There is a use case where the boot-wrapper[1] runs U-Boot, as a kind of
> poor-man's TF-A replacement. As the boot-wrapper normally just runs
> kernels, it follows the arm64 Linux kernel boot protocol, which puts
> the DTB address in x0. So to stay flexible, we look at both registers.
> The kernel boot protocol says x1 must be 0, so we check for that first,
> to sort that out.

Ah, thanks!

-- 
Tom

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

Re: [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@arm.com>

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

Tested-by: Peter Hoyes <peter.hoyes@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) {

Re: [PATCH] vexpress64: also consider DTB pointer in x1

[Tom Rini]

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

On Wed, Sep 21, 2022 at 06:09:46PM +0100, 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@arm.com>
> Tested-by: Peter Hoyes <peter.hoyes@arm.com>

Applied to u-boot/master, thanks!

-- 
Tom

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