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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 10324 bytes --]

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@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 ?

Yes please, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]