Re: [PATCH] spl: Align device tree blob address at 8-byte boundary

[Tom Rini <trini@konsulko.com>]

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

On Mon, Jul 26, 2021 at 09:26:26PM +0800, Bin Meng wrote:
> On Wed, Jul 14, 2021 at 5:11 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 10:35:03PM +0200, Marek Vasut wrote:
> > > On 7/13/21 8:11 PM, Tom Rini wrote:
> > > > On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
> > > > > On 7/13/21 6:47 PM, Simon Glass wrote:
> > > > > > Hi Marek,
> > > > > >
> > > > > > On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
> > > > > > >
> > > > > > > On 7/13/21 4:41 PM, Tom Rini wrote:
> > > > > > > > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> > > > > > > > > On 7/13/21 3:47 PM, Tom Rini wrote:
> > > > > > > > > > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > > > > > > > > > On 7/12/21 10:15 AM, Tom Rini wrote:
> > > > > > > > > > > > On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote:
> > > > > > > > > > > > > On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle <reuben.dowle@4rf.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I submitted an almost identical patch. See https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114), because it was causing issues on some platforms that had FIT on 32 bit boundary. However I continue to use it in production code, as without it the boot on my platform aborts.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I don't have time to investigate why this was happening, but you need to check this code won't just cause exactly the same faults.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for your information.
> > > > > > > > > > > > >
> > > > > > > > > > > > > +Marek who did the revert
> > > > > > > > > > > > >
> > > > > > > > > > > > > The revert commit message says:
> > > > > > > > > > > > >
> > > > > > > > > > > > >          "The commit breaks booting of fitImage by SPL, the system simply
> > > > > > > > > > > > > hangs. This is because on arm32, the fitImage and all of its content
> > > > > > > > > > > > > can be aligned to 4 bytes and U-Boot expects just that."
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't understand this. If an address is aligned to 8, it is already
> > > > > > > > > > > > > aligned to 4, so how did this commit make the system hang on arm32?
> > > > > > > > > > > >
> > > > > > > > > > > > I think this had something to do with embedding contents somewhere in
> > > > > > > > > > > > the image?  There is a thread on the ML from then but I don't know how
> > > > > > > > > > > > informative it will end up being.
> > > > > > > > > > >
> > > > > > > > > > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > > > > > > > > > on 32-bit. The issues here are specific to u-boot.
> > > > > > > > > > >
> > > > > > > > > > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > > > > > > > > > two cases:
> > > > > > > > > > >      1) u-boot as a FIT (binary and FDT separately loaded)
> > > > > > > > > > >      2) u-boot with embedded FDT
> > > > > > > > > > >
> > > > > > > > > > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > > > > > > > > > The current logic is
> > > > > > > > > > >      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >
> > > > > > > > > > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > > > > > > > > > system must place the FDT correctly:
> > > > > > > > > > >      build:  fdt >> u-boot.bin
> > > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >
> > > > > > > > > > > We have 3 places that must agree. A correct and complete patch could change
> > > > > > > > > > > all three, but one has to consider compatibility issues when crossing u-boot
> > > > > > > > > > > and SPL versions.
> > > > > > > > > > >
> > > > > > > > > > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > > > > > > > > > to pass the location of the FDT to u-boot.
> > > > > > > > > >
> > > > > > > > > > I'm not sure that we need to worry too much about mix-and-match
> > > > > > > > > > SPL/U-Boot, but documenting what to go change if you must do it
> > > > > > > > > > somewhere under doc/ would be good.  I think we can just switch to
> > > > > > > > > > ALIGN(8) not ALIGN(4) and be done with it?
> > > > > > > > >
> > > > > > > > > Remember, there is also falcon boot. And we definitely have to be able to
> > > > > > > > > have old u-boot (SPL) boot new fitImage and vice versa.
> > > > > > > >
> > > > > > > > I don't follow you, sorry.  But since you seem to have the best
> > > > > > > > understanding of where all of the cases something could go wrong here,
> > > > > > > > can you perhaps post an RFC patch?  That is likely to be clearer than
> > > > > > > > another long thread here.
> > > > > > >
> > > > > > > I don't follow you, sorry. I believe the revert did the right thing and
> > > > > > > new systems should use mkimage -E when generating fitImages, to avoid
> > > > > > > the string alignment problem. That is all.
> > > > > >
> > > > > > Using -E should be optional and things really should work without it.
> > > > >
> > > > > See the DTSpec, I don't think that is possible unless you relocate fitImage
> > > > > components, and if you want fast boot time esp. in SPL, that is not good.
> > > >
> > > > This is why I've asked you to make up some patch to perhaps highlight
> > > > the problem.  Ensuring that the device tree, which is small, is also
> > > > 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> > > > sure where the problem case is that isn't "user put things they control
> > > > in a bad spot, fail and tell them why" but I could just be missing a
> > > > case.
> > >
> > > The fail case is this:
> > > - you update SPL with this 8 byte alignment change
> > > - you have older kernel fitImage with embedded DT for falcon mode
> > > - system no longer boots because there is off-by-4 error in the DT
> > >   address passed to the kernel
> >
> > OK.  Then I think the answer is what I said recently in another part of
> > this thread, we need to split "find the fdt" from "align the fdt".  The
> > fdt can come to us with any alignment it happens to have, but we can't
> > use that fdt in-place unless it's correctly aligned.  In the case of
> > falcon mode, it needs to end up at CONFIG_SYS_SPL_ARGS_ADDR.  The case
> > of passing it on to U-Boot proper is where we have at best a hack right
> > now (as noted by fdt_hack in common/spl/spl.c).  That would be a place
> > to, as has been also suggested in this thread, pass along more correctly
> > where the device tree in memory is.
> 
> Where are we on this issue?

Waiting for someone to do what I suggested I think.  I've not yet had
time to take a look.

-- 
Tom

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