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

[Tom Rini <trini@konsulko.com>]

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

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

-- 
Tom

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