[PATCH 09/17] spl: riscv: opensbi: Error on misaligned FDT

[Sam Edwards]

On Mon, Feb 24, 2025 at 6:54 AM Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Feb 24, 2025 at 09:54:56AM +0100, Heinrich Schuchardt wrote:
> > On 2/24/25 06:55, Sam Edwards wrote:
> > > libfdt 1.6.1+ requires the FDT to be 8-byte aligned and returns an error
> > > if not. OpenSBI 1.0+ includes this version of libfdt and will also
> > > reject misaligned FDTs.
> > >
> > > However, OpenSBI cannot indicate the error to the user: since it cannot
> > > access the serial console, it can only silently hang. This can be very
> > > difficult to diagnose without proper debugging facilities. Therefore,
> > > give the U-Boot SPL, which *can* print error messages, an additional
> > > check for proper FDT alignment.
> > >
> > > Signed-off-by: Sam Edwards <CFSworks at gmail.com>
> > > ---
> > >   common/spl/spl_opensbi.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> > > index 5a26d7c31a4..0ed6afeacc6 100644
> > > --- a/common/spl/spl_opensbi.c
> > > +++ b/common/spl/spl_opensbi.c
> > > @@ -57,6 +57,11 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
> > >             hang();
> > >     }
> > >
> > > +   if (!IS_ALIGNED((uintptr_t)spl_image->fdt_addr, 8)) {
> > > +           pr_err("SPL image loaded an improperly-aligned device tree\n");
> >
> > We only use pr_err() in drivers when we copy code from Linux. Otherwise
> > use log_err().
> >
> > As this code is for RISC-V, this patch should have been sent to the
> > RISC-V maintainers.
> >
> > cc: Leo, Rick
> >
> > SPL size is very restricted on boards where it is responsible for
> > initializing DRAM and therefore has to fit into cache. We should only
> > add code that is strictly needed to SPL.
> >
> > What makes you think that this problem can realistically occur?
>
> I would like to know if Sam hit this in practice as well. But we have
> had more than one place where because we don't ensure 8-byte alignment
> and have use-in-place, that we've broken things. Picking up one of the
> patches that would address this within U-Boot is on my list now that
> we've had confirmation of some padding command that's sufficiently
> portable.

Hi gents,

I have indeed hit this in practice; I will elaborate how in my reply
to patch 10/17. But this whole series is sanding down various sharp
corners that snagged me while trying to build with LLVM. :)

I agree with the aim of conserving space in the SPL, but since I spent
a fair amount of time debugging why OpenSBI was hanging, I believe the
need for a sanity check on the assumption of 8-byte alignment *before*
passing control to OpenSBI is the greater concern. I would be more
favorable to putting an assert in the common SPL code; my only aim
here is to catch this problem with a diagnosable error message, as I
suspect we have not seen the last of it.

Cheers,
Sam

>
> --
> Tom