Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot

[Peter Hoyes <Peter.Hoyes@arm.com>]

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 <Peter.Hoyes@arm.com>
>>>>
>>>> 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 <Peter.Hoyes@arm.com>
>>>> ---
>>>>    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