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

[Marek Vasut]

On 7/12/21 5:43 PM, Tom Rini wrote:
> On Mon, Jul 12, 2021 at 05:38:33PM +0200, Marek Vasut wrote:
>> On 7/12/21 5:15 PM, 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 at 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.
>>
>> If I recall this correctly, DT node alignment is 4 byte and that is what DTC
>> emits. If you have fitImage with embedded data, you basically end up with
>>
>> / {
>>   prop1 = "string1";
>>   prop2 = "string2";
>> };
>>
>> where the "string2" is aligned to 4 bytes. And that is what U-Boot expects
>> when it tries to access those data in-place in SPL.
>>
>> The problem with the reverted patch was that it made U-Boot assume the
>> alignment is 8 bytes, and that actually works only if you use fitImage with
>> external data (mkimage -E), but with embedded data (mkimage default) not so
>> much. That caused off-by-4 error in some cases and that made the SPL hang.
>>
>>>> Note, as I indicated in this patch, now with libfdt 1.6.1, the
>>>> alignment to 8 byte is a must-have. So we have to do such alignment
>>>> anyway.
>>>>
>>>> @Tom may fill in why libfdt commit commit 5e735860c478 ("libfdt: Check
>>>> for 8-byte address alignment in fdt_ro_probe_()") was made to have the
>>>> 8-byte alignment requirement.
>>>
>>> Note that it's not so much since libfdt 1.6.1 but that since always the
>>> device tree has required 8 byte alignment.
>>
>> DT alignment was always 4 byte , no ?
> 
> I'm pretty sure, no, 8 byte base alignment is a pretty much always
> thing.  I don't have a reference handy but I also know I couldn't have
> convinced dgibson to add the check otherwise.

DTSpec rev 0.3 says the following and I think you got confused by 
section 5.6 which talks about alignment of the entire tree, not its nodes:

5.4 Structure Block
"
Each token in the structure block, and thus the structure block itself, 
shall be located at a 4-byte aligned offset from the
beginning of the devicetree blob (see 5.6).
"

5.4.2 Tree structure
"
For each property of the node:
...
– FDT_PROP token
...
* [zeroed padding bytes to align to a 4-byte boundary]
"

5.5 Strings Block
"
The strings block contains strings representing all the property names 
used in the tree. These null terminated strings are
simply concatenated together in this section, and referred to from the 
structure block by an offset into the strings block.
The strings block has no alignment constraints and may appear at any 
offset from the beginning of the devicetree blob.
"

5.6 Alignment
"
As described in the previous sections, the structure and strings blocks 
shall have aligned offsets from the beginning of
the devicetree blob. To ensure the in-memory alignment of the blocks, it 
is sufficient to ensure that the devicetree as a
whole is loaded at an address aligned to the largest alignment of any of 
the subblocks, that is, to an 8-byte boundary.
"