[PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
Tom Rini
trini at konsulko.com
Mon May 23 15:23:11 CEST 2022
On Mon, May 23, 2022 at 09:25:55AM +0200, Neil Armstrong wrote:
> 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 ?
Yes please, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220523/05f3ec91/attachment.sig>
More information about the U-Boot
mailing list