From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 919DDC6FA86 for ; Thu, 22 Sep 2022 10:43:49 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3D10884CD6; Thu, 22 Sep 2022 12:43:47 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 81A9884CD4; Thu, 22 Sep 2022 12:43:46 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id B088F84CD4 for ; Thu, 22 Sep 2022 12:43:43 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=Peter.Hoyes@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 815D61595; Thu, 22 Sep 2022 03:43:49 -0700 (PDT) Received: from [10.1.199.26] (unknown [10.1.199.26]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EAC873FA00; Thu, 22 Sep 2022 03:43:41 -0700 (PDT) Message-ID: <8cfc7586-0cc4-59c3-48bd-7d9f344237b3@arm.com> Date: Thu, 22 Sep 2022 11:43:50 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] vexpress64: also consider DTB pointer in x1 Content-Language: en-US To: Andre Przywara , David Feng , Tom Rini , Simon Glass Cc: Liviu Dudau , Linus Walleij , Ross Burton , u-boot@lists.denx.de References: <20220921170946.1363372-1-andre.przywara@arm.com> From: Peter Hoyes In-Reply-To: <20220921170946.1363372-1-andre.przywara@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 Thanks for getting to the bottom of this issue Andre. You can add: Tested-by: Peter Hoyes > --- > 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) {