[PATCH RFC 2/2] spl: opensbi: fix OS image address retrieval from FIT

[Simon Glass]

Hi Nikita,

On 2026-06-19T12:52:56, Nikita Shubin <nikita.shubin at maquefel.me> wrote:
> spl: opensbi: fix OS image address retrieval from FIT
>
> The current implementation of spl_invoke_opensbi() attempts to locate
> the next-stage OS (U-Boot proper or Linux) by parsing the FDT blob
> pointed to by spl_image->fdt_addr. This FDT, however, is the one that
> will be passed to the OS (e.g. a kernel device tree), not the FIT image
> itself. As a result, when a FIT image containing multiple components
> (firmware, loadables, fdt) is loaded, U-Boot SPL cannot find the OS node
> because it searches in the wrong FDT.
>
> Fix this by storing the load addresses of U-Boot and Linux in
> spl_image_info during the loadables phase (spl_load_fit_image()).
> In spl_invoke_opensbi(), use these stored addresses if they are
> non-zero, and fall back to parsing the FDT only if no address is stored,
> maintaining backward compatibility.
>
> This ensures that the correct entry point is passed to OpenSBI,
> allowing proper boot flow with FIT images.
>
> Signed-off-by: Nikita Shubin <nikita.shubin at maquefel.me>
>
> common/spl/spl_fit.c     |  8 ++++++++
>  common/spl/spl_opensbi.c | 33 +++++++++++++++++++--------------
>  include/spl.h            |  8 ++++++++
>  3 files changed, 35 insertions(+), 14 deletions(-)

> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> @@ -1020,6 +1020,14 @@ int spl_load_fit_image(struct spl_image_info *spl_image,
>                                    &img_data, &img_len);
>               if (ret < 0)
>                       return ret;
> +
> +#if CONFIG_IS_ENABLED(OPENSBI)
> +             if (fit_image_check_os((const void *)header, ret, IH_OS_LINUX))
> +                     spl_image->kernel_addr = img_data;
> +
> +             if (fit_image_check_os((const void *)header, ret, IH_OS_U_BOOT))
> +                     spl_image->uboot_addr = img_data;
> +#endif
>       }

I don't like this approach. The real reason spl_invoke_opensbi() fails
in the LOAD_FIT_FULL path is that nothing populates the /fit-images
node in the outgoing FDT - the simple FIT loader does this via
spl_fit_record_loadable() / fdt_record_loadable(), and spl_opensbi.c
then walks it.

I think it would be better to fix it that way: call
fdt_record_loadable() for each loadable (and for the firmware/fdt) so
the OS DT carries /fit-images just like in the simple-FIT case. Then
spl_invoke_opensbi() needs no changes, ATF gets the same fix for free,
and we avoid carving out RISC-V/OpenSBI-specific fields in struct
spl_image_info...I wonder if you are deliberately wanting the
simple-FIT case to be different? If so can you please explain why?

If you do want to cache the addresses on spl_image_info, drop the
OPENSBI-specific naming and guards - the loadables loop has no
business knowing about OpenSBI, and the fields should be filled in
both spl_load_simple_fit() and spl_load_fit_image() for consistency.

> diff --git a/include/spl.h b/include/spl.h
> @@ -288,6 +292,10 @@ struct spl_image_info {
>       ulong entry_point;
>  #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL)
>       void *fdt_addr;
> +#if CONFIG_IS_ENABLED(OPENSBI)
> +     ulong uboot_addr;
> +     ulong kernel_addr;
> +#endif
>  #endif

Nesting OPENSBI inside LOAD_FIT is fragile - if anyone enables
SPL_OPENSBI without SPL_LOAD_FIT these fields silently vanish while
spl_opensbi.c still references them. At minimum the guards should
match spl_opensbi.c.

Also, uboot_addr here is the load address of the loadable, but the
existing spl_invoke_opensbi() prefers the entry point
(fit_image_get_entry()), falling back to the load address. Storing
only the load address is a behavioural change for images that set an
explicit entry property - please mention this in the commit message,
or store entry_point instead.

> diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> @@ -81,22 +81,27 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
> -     if (CONFIG_IS_ENABLED(LOAD_FIT_OPENSBI_OS_BOOT))
> +     if (CONFIG_IS_ENABLED(LOAD_FIT_OPENSBI_OS_BOOT)) {
>               os_type = IH_OS_LINUX;
> -     else
> +             os_entry = spl_image->kernel_addr;
> +     } else {
>               os_type = IH_OS_U_BOOT;
> -
> -     ret = spl_opensbi_find_os_node(spl_image->fdt_addr, &os_node, os_type);
> -     if (ret) {
> -             pr_err("Can't find %s node for opensbi, %d\n",
> -                    genimg_get_os_name(os_type), ret);
> -             hang();
> +             os_entry = spl_image->uboot_addr;
>       }

Using 0 as a sentinel for "no cached address" is dodgy - a load
address of 0 is unusual but not impossible, and silently falling
through to FDT parsing is hard to debug. A separate bool would be
cleaner, or better, record loadables into the FDT as suggested above.

Regards,
Simon