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 1EB4FC433FE for ; Thu, 19 May 2022 12:59:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id CE81683C2F; Thu, 19 May 2022 14:58:58 +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 DE92481E79; Thu, 19 May 2022 14:58:55 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id B2C5683C2F for ; Thu, 19 May 2022 14:58:51 +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 1B57015DB; Thu, 19 May 2022 05:58:51 -0700 (PDT) Received: from [10.1.199.58] (unknown [10.1.199.58]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 13AC93F66F; Thu, 19 May 2022 05:58:49 -0700 (PDT) Message-ID: Date: Thu, 19 May 2022 13:58:42 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 From: Peter Hoyes Subject: Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot To: Neil Armstrong , Tom Rini Cc: "u-boot@lists.denx.de" , "bmeng.cn@gmail.com" , "sjg@chromium.org" , "kory.maincent@bootlin.com" , Andre Przywara , Diego Sueiro References: <20211014084004.3173835-1-peter.hoyes@arm.com> <20220518155253.GF3901321@bill-the-cat> <17defab5-a104-3fe9-c87a-0ba18dae799e@baylibre.com> Content-Language: en-US In-Reply-To: <17defab5-a104-3fe9-c87a-0ba18dae799e@baylibre.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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.5 at phobos.denx.de X-Virus-Status: Clean On 18/05/2022 19:15, Neil Armstrong wrote: > On 18/05/2022 17:52, Tom Rini wrote: >> On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote: >>> Hi, >>> >>> On 14/10/2021 10:40, Peter Hoyes wrote: >>>> From: Peter Hoyes >>>> >>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to >>>> configure >>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and >>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too. >>>> >>>> We are developing board support for the Armv8r64 FVP using >>>> config_distro_bootcmd. We are also using OF_BOARD and would like the >>>> PXE boot option to default to the fdt provided by >>>> board_fdt_blob_setup. >>>> >>>> Signed-off-by: Peter Hoyes >>>> --- >>>>    cmd/pxe_utils.c | 8 +++++++- >>>>    1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c >>>> index 067c24e5ff..8f8e69ca97 100644 >>>> --- a/cmd/pxe_utils.c >>>> +++ b/cmd/pxe_utils.c >>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, >>>> struct pxe_label *label) >>>>         * Scenario 2: If there is an fdt_addr specified, pass it >>>> along to >>>>         * bootm, and adjust argc appropriately. >>>>         * >>>> -     * Scenario 3: fdt blob is not available. >>>> +     * Scenario 3: If there is an fdtcontroladdr specified, pass >>>> it along to >>>> +     * bootm, and adjust argc appropriately. >>>> +     * >>>> +     * Scenario 4: fdt blob is not available. >>>>         */ >>>>        bootm_argv[3] = env_get("fdt_addr_r"); >>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, >>>> struct pxe_label *label) >>>>        if (!bootm_argv[3]) >>>>            bootm_argv[3] = env_get("fdt_addr"); >>>> +    if (!bootm_argv[3]) >>>> +        bootm_argv[3] = env_get("fdtcontroladdr"); >>>> + >>>>        if (bootm_argv[3]) { >>>>            if (!bootm_argv[2]) >>>>                bootm_argv[2] = "-"; >>> >>> This change makes a regression when using a FIT image as kernel >>> image within an extlinux.conf >>> >>> Before this change, when a DT wasn't specified, the DT in the FIT >>> image was selected, >>> not the u-boot DT is selected. >>> >>> While it should work because the U-Boot DT should work in Linux, >>> it's not always the case. >>> >>> Before: >>> Found /extlinux/extlinux.conf >>> Retrieving file: /extlinux/extlinux.conf >>> 1:    Yocto >>> Retrieving file: /fitImage >>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200 >>> ## Loading kernel from FIT Image at 08080000 ... >>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration >>>     Trying 'kernel-1' kernel subimage >>>       Description:  Linux kernel >>>       Type:         Kernel Image >>>       Compression:  gzip compressed >>>       Data Start:   0x08080120 >>>       Data Size:    9926956 Bytes = 9.5 MiB >>>       Architecture: AArch64 >>>       OS:           Linux >>>       Load Address: 0x01080000 >>>       Entry Point:  0x01080000 >>>       Hash algo:    sha256 >>>       Hash value: >>> 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e >>>     Verifying Hash Integrity ... sha256+ OK >>> ## Loading fdt from FIT Image at 08080000 ... >>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration >>>     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage >>>       Description:  Flattened Device Tree blob >>>       Type:         Flat Device Tree >>>       Compression:  uncompressed >>>       Data Start:   0x089f7b78 >>>       Data Size:    29092 Bytes = 28.4 KiB >>>       Architecture: AArch64 >>>       Hash algo:    sha256 >>>       Hash value: >>> 72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67 >>>     Verifying Hash Integrity ... sha256+ OK >>>     Booting using the fdt blob at 0x89f7b78 >>>     Uncompressing Kernel Image >>>     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 >>> ... OK >>> >>> >>> After: >>> Found /extlinux/extlinux.conf >>> Retrieving file: /extlinux/extlinux.conf >>> 1:    Yocto >>> Retrieving file: /fitImage >>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200 >>> ## Loading kernel from FIT Image at 08080000 ... >>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration >>>     Trying 'kernel-1' kernel subimage >>>       Description:  Linux kernel >>>       Type:         Kernel Image >>>       Compression:  gzip compressed >>>       Data Start:   0x08080120 >>>       Data Size:    9926956 Bytes = 9.5 MiB >>>       Architecture: AArch64 >>>       OS:           Linux >>>       Load Address: 0x01080000 >>>       Entry Point:  0x01080000 >>>       Hash algo:    sha256 >>>       Hash value: >>> 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e >>>     Verifying Hash Integrity ... sha256+ OK >>> ## Flattened Device Tree blob at 7bf40210 >>>     Booting using the fdt blob at 0x7bf40210 >>>     Uncompressing Kernel Image >>>     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 >>> ... OK >>> >>> >>> Note this is the "best in town" boot method in Yocto/OE when using >>> WIC, FIT Image and >>> generating extlinux.conf from the WIC "bootimg-partition" plugin. >>> >>> So this change will regress all platforms using this scheme. >>> >>> In case we only want the FIT image and not DTB, the fdtdir is >>> ommited at: >>> https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150 >>> >> >> Well this is certainly tricky.  Do we be "boring" and say that the >> built-in device tree should be used, or "boring" and say that the distro >> provided device tree should be used.  I assume that in the OE case we're >> not always going to be booting U-Boot that was built, so updating the >> logic there to have the correct dtb isn't right either. >> > > I'm not against this logic, it should definitely be aligned on EFI and > permit > using built-in DT, but the logic breaks fitImage loading via extlinux, > because > now we have no way to say "use the fitImage DT". > > This change should have been elsewhere, like in the non-FIT bootm path > when > no DT was supplied and DT is mandatory for the platform. > > Or by passing a keyword like fdt-built-in or ftdfile=builtin. I agree the correct logic should be:  * Use the extlinux.conf fdt file, if defined  * Use a FIT fdt, if available  * Use fdtcontroladdr, as a last resort , so FIT images was an oversight. > > ==><======================================================= > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > index 0c24becae3..22040c2340 100644 > --- a/boot/pxe_utils.c > +++ b/boot/pxe_utils.c > @@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, > struct pxe_label *label) >                         fdtfile = fdtfilefree; >                 } > > -               if (fdtfile) { > +               if (strcmp(fdtfile, "builtin") && fdtfile) { >                         int err = get_relfile_envaddr(ctx, fdtfile, > "fdt_addr_r", NULL); > > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, > struct pxe_label *label) >         if (!bootm_argv[3]) >                 bootm_argv[3] = env_get("fdt_addr"); > > -       if (!bootm_argv[3]) > +       if (!strcmp(fdtfile, "builtin")) >                 bootm_argv[3] = env_get("fdtcontroladdr"); > >         if (bootm_argv[3]) { > ==><======================================================= > > Or by checking the image format before using fdtcontroladdr like: > > ==><======================================================= > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > index 0c24becae3..f4f6369de6 100644 > --- a/boot/pxe_utils.c > +++ b/boot/pxe_utils.c > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, > struct pxe_label *label) >         if (!bootm_argv[3]) >                 bootm_argv[3] = env_get("fdt_addr"); > > -       if (!bootm_argv[3]) > +       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3]) >                 bootm_argv[3] = env_get("fdtcontroladdr"); > >         if (bootm_argv[3]) { > ==><======================================================= > > Neil Your second diff above seems like the least invasive way of achieving this behavior. I did wonder whether it would make sense to add a global 'fdtcontroladdr' fallback for bootm (i.e. in boot_get_fdt in image-fdt.c), so that:    bootm $kernel_addr_r falls back to $fdtcontroladdr (if valid) as a last resort. Then we could remove the branch in pxe_utils, and the FIT logic would happen in the right order. It looks like bootefi already does something similar, but I guess modifying the bootm behavior would affect a lot of existing boards. Peter