[PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
Peter Hoyes
Peter.Hoyes at arm.com
Thu May 19 14:58:42 CEST 2022
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.
Peter
More information about the U-Boot
mailing list