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 89F6BC433F5 for ; Mon, 23 May 2022 07:26:02 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id ACBC983B68; Mon, 23 May 2022 09:25:59 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20210112.gappssmtp.com header.i=@baylibre-com.20210112.gappssmtp.com header.b="zfg4Lnrs"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0DB0781D4B; Mon, 23 May 2022 09:25:58 +0200 (CEST) Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 114E281D4B for ; Mon, 23 May 2022 09:25:54 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=narmstrong@baylibre.com Received: by mail-wr1-x42e.google.com with SMTP id u27so19143971wru.8 for ; Mon, 23 May 2022 00:25:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:organization:in-reply-to :content-transfer-encoding; bh=3ZEp4W+Zn+t9gyGRoPW6V9W2/YjTknCEqO4ym4j1I1E=; b=zfg4LnrsfKKTEl3W7tep45wo9kex1TisSDJBtuq4OkA1gl0a4muG3VzJw6+2Y8UhuY V1pW5wDQKaa+z+YbpaPx9F2lLagmeyHFQ2X0MOdSp3d7APJ4I/eP6GpeyRnANKUcefu0 4LsgBFTiKW+JQL4UPXaYkvA5s+fdb7kI3utCxiXY4fq0n5Pc+s+Ztk/ydc+vim6tntMT QsxV78RCqSkjVLJfqGqZGGGV8wRGywin/avEh22zvJm69RDq+0gwp8DmjMIkAgtm/3c+ WPyFzsCN185WtxmOwrCCXw7aLGh07pNF+4XlfEiX0pdh5C5IKASYLmIP9S3FR95xzgNP t6eQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:organization:in-reply-to :content-transfer-encoding; bh=3ZEp4W+Zn+t9gyGRoPW6V9W2/YjTknCEqO4ym4j1I1E=; b=JjysR4t0El/bLfZ39s2bL97HVG0nvwJlDZnU76cMzxrKeCropsS+kbffB7ZRDkLl2D 4R7w/IF9vcbShzBtzLUo8N0aB4wgfitgaoT9iwaUoyJCJqsFFbfjdLy+GEpVneujWW9Z vsmOWP86wZvtt8FzkjQ++s1kizzez7wsThFFgvdDDjTFWGygZonp0gdKZDWuQDBZmGWz b8b80d4loG3OF/L34iQrH7Iem9A9zdFMOeVjjyOriCHieVVh+FNs476Vi45dGsmzphMa CYmt6g/sBJOaFTKZvYE2VeLDHGhwAXLWisyBaWuh+bN34k5BHBqt04AyTsCbucPvJ2c8 C0uA== X-Gm-Message-State: AOAM530Ew1l8MtkLiUqRi3NJS2BTzumcc/3qdjbClRnupJ6W1uKqtThd 2smhWoPjwyo77VimQDh8fd9iog== X-Google-Smtp-Source: ABdhPJzhkAlmz0SyZcYYOrlSvj6TI3vARJaLU6P6u23P14sm7Fcj0L6i6tsJcxMz/qHqaLyUA+7wzg== X-Received: by 2002:adf:a18a:0:b0:20d:1074:e69e with SMTP id u10-20020adfa18a000000b0020d1074e69emr17977002wru.44.1653290753460; Mon, 23 May 2022 00:25:53 -0700 (PDT) Received: from ?IPV6:2001:861:44c0:66c0:1d4:2e94:6a6f:da1f? ([2001:861:44c0:66c0:1d4:2e94:6a6f:da1f]) by smtp.gmail.com with ESMTPSA id n19-20020a05600c3b9300b003942a244ecbsm8166111wms.16.2022.05.23.00.25.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 May 2022 00:25:52 -0700 (PDT) Message-ID: <5736e298-d2c1-7e34-1db8-5ddd5fb6e4bf@baylibre.com> Date: Mon, 23 May 2022 09:25:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot Content-Language: en-US To: Peter Hoyes , 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> From: Neil Armstrong Organization: Baylibre In-Reply-To: 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 Hi, On 19/05/2022 14:58, Peter Hoyes wrote: > 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. Probably, but I think getting back fit image working would be the priority. Tom, should I send the last change ? Neil > > Peter >