[PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
Neil Armstrong
narmstrong at baylibre.com
Mon May 23 09:25:55 CEST 2022
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 at 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 at 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
>
More information about the U-Boot
mailing list