On Mon, Feb 24, 2025 at 6:54 AM Tom Rini <[email protected]> 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 <[email protected]> > > > --- > > > 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
