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

[Nikita Shubin]

On Thu, 2026-06-25 at 11:58 +0100, Simon Glass wrote:
> 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

Neither do I. That's the reason for RFC.

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

Nope - i don't want it to be different at all. Thank you for tips -
i'll look into implementing a cleaner approach.

> 
> 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.

Agreed.

> 
> > 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.

Noted. I think most of these issues (if not all) will be resolved by
taking  spl_fit_record_loadable()/fdt_record_loadable() cleaner
approach.

Thank you!

Yours,
Nikita.

> 
> Regards,
> Simon