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

[Neil Armstrong <narmstrong@baylibre.com>]

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.

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