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

[Alex G <mr.nuke.me@gmail.com>]

On 7/13/21 1: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.
> 

As far as highlighting the problem, here's an excerpt from the previous 
discussion [1].


## SPL:

image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);

	(gdb) print/x (spl_image->load_addr + spl_image->size)
	$19 = 0xc01cf85c
	(gdb) print/x image_info->load_addr
	$20 = 0xc01cf860

FDT is installed at 0xc01cf860


## u-boot:

	__weak void *board_fdt_blob_setup(void)
	{
		/* FDT is at end of image */

		fdt_blob = (ulong *)&_end;

	(gdb) print &_end
	$22 = (char (*)[]) 0xc01cf85c

FDT is expected at 0xc01cf85c


Alex

[1] https://lists.denx.de/pipermail/u-boot/2020-October/430066.html