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

Neil Armstrong
2022-05-23T07:26:02Z

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 <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.

Probably, but I think getting back fit image working would be the priority.

Tom, should I send the last change ?

Neil

>
> Peter
>